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(cloudprovider/exteranlgrpc): properly handle unimplemented methods #5937

Merged

Conversation

dbonfigli
Copy link
Member

What type of PR is this?

/kind bug
/kind feature

What this PR does / why we need it:

Some CloudProvider and nodeGroup methods are optional: if a cloud provider does not implemented an optional method, it should return the cloudprovider.ErrNotImplemented error, this error is expected and the CA gracefully handles it.

Currently there is no way to pass this specific error from a grpc based cloud provider: The protobuf definition incorrectly states that an rpc is optional or a grpc error will result in using defaults, but this is not correct at the moment: in case of grpc errors, the client passes a generic error that is not correctly handled by the CA, so currently all rpc must be implemented.

This PR allows grpc cloud providers to not implement optional methods: a not implemented method should pass the grpc error code 12 Unimplemented, if so, the CA grpc client will transform this to the go cloudprovider.ErrNotImplemented error that is then correctly handled.

Which issue(s) this PR fixes:

Fixes #

Special notes for your reviewer:

Does this PR introduce a user-facing change?

externalgrpc cloud provider: grpc based cloud providers can now pass the grpc error code 12, Unimplemented, to signal they do not implement optional methods.

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


@k8s-ci-robot k8s-ci-robot added do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. kind/bug Categorizes issue or PR as related to a bug. kind/feature Categorizes issue or PR as related to a new feature. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. labels Jul 8, 2023
@k8s-ci-robot k8s-ci-robot requested a review from BigDarkClown July 8, 2023 13:37
@k8s-ci-robot k8s-ci-robot requested a review from x13n July 8, 2023 13:37
@k8s-ci-robot k8s-ci-robot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Jul 8, 2023
@dbonfigli dbonfigli force-pushed the fix-externalgrpc-unimplemented branch from 0c1749c to ead5dc9 Compare July 8, 2023 13:43
@dbonfigli
Copy link
Member Author

keeping this on draft until #5936 is merged so to cleanly regenerate the code for the protobuf.

@dbonfigli dbonfigli force-pushed the fix-externalgrpc-unimplemented branch from ead5dc9 to 59df332 Compare September 4, 2023 17:24
@k8s-ci-robot k8s-ci-robot added the area/provider/externalgrpc Issues or PRs related to the External gRPC provider label Sep 4, 2023
@dbonfigli dbonfigli marked this pull request as ready for review September 4, 2023 17:55
@k8s-ci-robot k8s-ci-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Sep 4, 2023
@dbonfigli
Copy link
Member Author

after merging #5936, this is now ready to be reviewed, tagging @vadasambar as you expressed interest on this part of the code and i am the only one listed (informally, commented out) on the OWNERS file for this cloud provider.

@@ -41,13 +41,13 @@ service CloudProvider {

// PricingNodePrice returns a theoretical minimum price of running a node for
// a given period of time on a perfectly matching machine.
// Implementation optional.
// Implementation optional: if so should return error code 12 Unimplemented
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: the wording feels a little ambiguous. Here's a suggestion:

Suggested change
// Implementation optional: if so should return error code 12 Unimplemented
// Implementation optional: if unimplemented return error code 12 (for `Unimplemented`)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ok, suggestion accepted

@vadasambar
Copy link
Member

Overall looks good. I will do another pass and approve it.

@vadasambar
Copy link
Member

/assign vadasambar

@dbonfigli dbonfigli force-pushed the fix-externalgrpc-unimplemented branch from 59df332 to 436a8a0 Compare September 6, 2023 18:00
@vadasambar
Copy link
Member

/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Sep 7, 2023
@dbonfigli
Copy link
Member Author

after the lgtm label , I am assigning this to @towca for the approval.

/assign @towca

@x13n
Copy link
Member

x13n commented Sep 14, 2023

/approve

@dbonfigli Did you think about becoming k8s org member so you can approve changes for externalgrpc?

@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: dbonfigli, vadasambar, x13n

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

The pull request process is described 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 added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Sep 14, 2023
@k8s-ci-robot k8s-ci-robot merged commit 43fd1fa into kubernetes:master Sep 14, 2023
@dbonfigli
Copy link
Member Author

@dbonfigli Did you think about becoming k8s org member so you can approve changes for externalgrpc?

If you are willing to sponsor me and we find another reviewer that wants to sponsor me I am willing to keep contributing to this project as an official k8s org member.

@x13n
Copy link
Member

x13n commented Sep 15, 2023

Happy to do so! If you don't have another sponsor you could ask on the SIG meeting.

@vadasambar
Copy link
Member

If you are willing to sponsor me and we find another reviewer that wants to sponsor me I am willing to keep contributing to this project as an official k8s org member.

Added it to the agenda for Monday: https://docs.google.com/document/d/1RvhQAEIrVLHbyNnuaT99-6u9ZUMp7BfkPupT2LAZK7w/edit#bookmark=id.xtfu11692zyn

@elmiko
Copy link
Contributor

elmiko commented Sep 18, 2023

i'm happy to help with sponsoring as well, i reviewed some of the original grpc work.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. 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. kind/feature Categorizes issue or PR as related to a new feature. lgtm "Looks good to me", indicates that a PR is ready to be merged. size/L Denotes a PR that changes 100-499 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants