Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix MaxNodeProvisionTime and ZeroOrMaxNodeScaling node-groups handlin… #5998

Closed

Conversation

kruftik
Copy link

@kruftik kruftik commented Jul 29, 2023

What type of PR is this?

/kind bug

What this PR does / why we need it:

current code has a bug: externalgrpc service proto specification has GetOptions(...) optional method but the response type does not have MaxNodeProvisionTime ZeroOrMaxNodeScaling fields. GetOption(..) wrapper does not respect such a situration it results in 0 (zero) MaxNodeProvisionTime duration and false value for ZeroOrMaxNodeScaling options since they are default type values in case if the specific externalgrpc service implements the GetOptions() method.

Special notes for your reviewer:

Does this PR introduce a user-facing change?

No

Additional documentation e.g., KEPs (Kubernetes Enhancement Proposals), usage docs, etc.:

@k8s-ci-robot k8s-ci-robot added the kind/bug Categorizes issue or PR as related to a bug. label Jul 29, 2023
@linux-foundation-easycla
Copy link

linux-foundation-easycla bot commented Jul 29, 2023

CLA Signed

The committers listed above are authorized under a signed CLA.

@k8s-ci-robot k8s-ci-robot added the cncf-cla: no Indicates the PR's author has not signed the CNCF CLA. label Jul 29, 2023
@k8s-ci-robot
Copy link
Contributor

Welcome @kruftik!

It looks like this is your first PR to kubernetes/autoscaler 🎉. Please refer to our pull request process documentation to help your PR have a smooth ride to approval.

You will be prompted by a bot to use commands during the review process. Do not be afraid to follow the prompts! It is okay to experiment. Here is the bot commands documentation.

You can also check if kubernetes/autoscaler has its own contribution guidelines.

You may want to refer to our testing guide if you run into trouble with your tests not passing.

If you are having difficulty getting your pull request seen, please follow the recommended escalation practices. Also, for tips and tricks in the contribution process you may want to read the Kubernetes contributor cheat sheet. We want to make sure your contribution gets all the attention it needs!

Thank you, and welcome to Kubernetes. 😃

@k8s-ci-robot k8s-ci-robot added the size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. label Jul 29, 2023
@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: kruftik
Once this PR has been reviewed and has the lgtm label, please assign x13n for approval. For more information see the Kubernetes Code Review Process.

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@k8s-ci-robot k8s-ci-robot requested a review from feiskyer July 29, 2023 08:29
@kruftik kruftik force-pushed the fix-externalgrpc-options-handling branch from 248242e to e678493 Compare July 29, 2023 08:34
@k8s-ci-robot k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. and removed cncf-cla: no Indicates the PR's author has not signed the CNCF CLA. labels Jul 29, 2023
@k8s-ci-robot k8s-ci-robot added cncf-cla: no Indicates the PR's author has not signed the CNCF CLA. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. labels Jul 31, 2023
@kruftik kruftik force-pushed the fix-externalgrpc-options-handling branch from 9635b24 to b0efd1a Compare July 31, 2023 04:06
@k8s-ci-robot k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. and removed cncf-cla: no Indicates the PR's author has not signed the CNCF CLA. labels Jul 31, 2023
@kruftik kruftik force-pushed the fix-externalgrpc-options-handling branch from b0efd1a to 2d28b69 Compare July 31, 2023 04:26
@k8s-ci-robot k8s-ci-robot added size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Jul 31, 2023
@k8s-ci-robot k8s-ci-robot added size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. and removed size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. labels Jul 31, 2023
@Shubham82
Copy link
Contributor

/area provider/externalgrpc

@k8s-ci-robot
Copy link
Contributor

@Shubham82: The label(s) area/provider/externalgrpc cannot be applied, because the repository doesn't have them.

In response to this:

/area provider/externalgrpc

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

@kruftik
Copy link
Author

kruftik commented Aug 5, 2023

@BigDarkClown , @feiskyer ,
could you take a look at the PR please ?

@towca
Copy link
Collaborator

towca commented Sep 1, 2023

Cloud provider PRs should be reviewed by the relevant OWNERS - in this case it looks like @dbonfigli. I took a look and the overall approach looks good, but I can't verify the proto/grpc parts.

/assign @dbonfigli
/label area/provider/externalgrpc

@k8s-ci-robot
Copy link
Contributor

@towca: GitHub didn't allow me to assign the following users: dbonfigli.

Note that only kubernetes members with read permissions, repo collaborators and people who have commented on this issue/PR can be assigned. Additionally, issues/PRs can only have 10 assignees at the same time.
For more information please see the contributor guide

In response to this:

Cloud provider PRs should be reviewed by the relevant OWNERS - in this case it looks like @dbonfigli. I took a look and the overall approach looks good, but I can't verify the proto/grpc parts.

/assign @dbonfigli
/label area/provider/externalgrpc

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

@k8s-ci-robot
Copy link
Contributor

@towca: The label(s) /label area/provider/externalgrpc cannot be applied. These labels are supported: api-review, tide/merge-method-merge, tide/merge-method-rebase, tide/merge-method-squash, team/katacoda, refactor. Is this label configured under labels -> additional_labels or labels -> restricted_labels in plugin.yaml?

In response to this:

Cloud provider PRs should be reviewed by the relevant OWNERS - in this case it looks like @dbonfigli. I took a look and the overall approach looks good, but I can't verify the proto/grpc parts.

/assign @dbonfigli
/label area/provider/externalgrpc

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

@towca towca added the area/provider/externalgrpc Issues or PRs related to the External gRPC provider label Sep 1, 2023
@dbonfigli
Copy link
Member

I am not an official kubernetes member so i cannot give a review that can be used to approve this PR even if i am listed in the ownes file for this cloud provider , anyway, it looks good but the same changes and a bit more are covered on PRs #5936 and #5937 that have been created before this, @towca can we give priority to those PRs, especially #5936 that in turn is needed for the final version of #5937 that otherwise would conflict on the proto files?

@k8s-ci-robot k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Sep 4, 2023
@k8s-ci-robot
Copy link
Contributor

PR needs rebase.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

@x13n
Copy link
Member

x13n commented Sep 14, 2023

Per the comment above it looks like this PR should be closed in favor of #5936 and #5937.

/close

@k8s-ci-robot
Copy link
Contributor

@x13n: Closed this PR.

In response to this:

Per the comment above it looks like this PR should be closed in favor of #5936 and #5937.

/close

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/cluster-autoscaler area/provider/externalgrpc Issues or PRs related to the External gRPC provider cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. kind/bug Categorizes issue or PR as related to a bug. needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. size/XL Denotes a PR that changes 500-999 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants