fix(cloudprovider/externalgrpc): `GetOptions` returns `MaxNodeProvisionTime` as 0 seconds #5936

vadasambar · 2023-07-06T19:20:25Z

Signed-off-by: vadasambar surajrbanakar@gmail.com

What type of PR is this?

/kind bug

What this PR does / why we need it:

Adds missing MaxNodeProvisionTime in externalgrpc proto
Sets default MaxNodeProvisionTime for nodegroup's GetOptions function
Check When using externalgrpc provider, nodes are created and then immediately deleted next loop iteration #5935 for more info

Which issue(s) this PR fixes:

Fixes #5935

Special notes for your reviewer:

Does this PR introduce a user-facing change?

Fixed: cluster-autoscaler thinks newly scaled up nodegroup using externalgrpc provider has `MaxNodeProvisionTime` set as 0 seconds and expects the new node to be registered in 0-10 seconds instead of the default 15m. Check https://github.com/kubernetes/autoscaler/issues/5935 for more info.

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

vadasambar · 2023-07-06T19:38:33Z

cluster-autoscaler/cloudprovider/externalgrpc/README.md

@@ -56,8 +56,8 @@ To regenerate the gRPC code:
 1. install `protoc` and `protoc-gen-go-grpc`:

 ```bash
-go install google.golang.org/protobuf/cmd/protoc-gen-go@v1.26
-go install google.golang.org/grpc/cmd/protoc-gen-go-grpc@v1.1
+go install google.golang.org/protobuf/cmd/protoc-gen-go@v1.28


Updated to latest version at the time of creating this PR.

https://grpc.io/docs/languages/go/quickstart/

vadasambar · 2023-07-06T19:39:52Z

cluster-autoscaler/cloudprovider/externalgrpc/protos/externalgrpc.pb.go

@@ -16,17 +16,17 @@ limitations under the License.

 // Code generated by protoc-gen-go. DO NOT EDIT.
 // versions:
-// 	protoc-gen-go v1.26.0
-// 	protoc        v3.19.1
+// 	protoc-gen-go v1.28.1


Everything in this file is auto-generated using

protoc \ -I ./cluster-autoscaler \ -I ./cluster-autoscaler/vendor \ --go_out=. \ --go-grpc_out=. \ ./cluster-autoscaler/cloudprovider/externalgrpc/protos/externalgrpc.proto

https://github.com/kubernetes/autoscaler/tree/master/cluster-autoscaler/cloudprovider/externalgrpc#code-generation

vadasambar · 2023-07-06T19:40:06Z

cluster-autoscaler/cloudprovider/externalgrpc/protos/externalgrpc_grpc.pb.go

@@ -15,6 +15,10 @@ limitations under the License.
 */

 // Code generated by protoc-gen-go-grpc. DO NOT EDIT.
+// versions:


Everything in this file is auto-generated using

protoc \ -I ./cluster-autoscaler \ -I ./cluster-autoscaler/vendor \ --go_out=. \ --go-grpc_out=. \ ./cluster-autoscaler/cloudprovider/externalgrpc/protos/externalgrpc.proto

https://github.com/kubernetes/autoscaler/tree/master/cluster-autoscaler/cloudprovider/externalgrpc#code-generation

vadasambar · 2023-07-07T06:02:32Z

cluster-autoscaler/cloudprovider/externalgrpc/protos/externalgrpc.pb.go

@@ -786,7 +786,7 @@ type GetAvailableGPUTypesResponse struct {
 	unknownFields protoimpl.UnknownFields

 	// GPU types passed in as opaque key-value pairs.
-	GpuTypes map[string]*anypb.Any `protobuf:"bytes,1,rep,name=gpuTypes,proto3" json:"gpuTypes,omitempty" protobuf_key:"bytes,1,opt,name=key,proto3" protobuf_val:"bytes,2,opt,name=value,proto3"`
+	GpuTypes map[string]*any1.Any `protobuf:"bytes,1,rep,name=gpuTypes,proto3" json:"gpuTypes,omitempty" protobuf_key:"bytes,1,opt,name=key,proto3" protobuf_val:"bytes,2,opt,name=value,proto3"`


Note that any1 is imported from "github.com/golang/protobuf/ptypes/any". "github.com/golang/protobuf/ptypes/any" internally uses "google.golang.org/protobuf/types/known/anypb"'s anypb

autoscaler/cluster-autoscaler/vendor/github.com/golang/protobuf/ptypes/any/any.pb.go

Lines 4 to 15 in b569db4

package any

import (

protoreflect "google.golang.org/protobuf/reflect/protoreflect"

protoimpl "google.golang.org/protobuf/runtime/protoimpl"

anypb "google.golang.org/protobuf/types/known/anypb"

reflect "reflect"

)

// Symbols defined in public import of google/protobuf/any.proto.

type Any = anypb.Any

dbonfigli

Thank you for this.

There is another field that has been recently added to NodeGroupAutoscalingOptions , ZeroOrMaxNodeScaling, but it is constantly worked on and changed name many times recently, since the default is false and it is not causing issues I would not put it in the protobuf until the feature settles down.

Could you also please update the example at

autoscaler/cluster-autoscaler/cloudprovider/externalgrpc/examples/external-grpc-cloud-provider-service/wrapper/wrapper.go

Line 349 in b569db4

defaults := config.NodeGroupAutoscalingOptions{

with the new field?

dbonfigli · 2023-07-08T12:16:34Z

cluster-autoscaler/cloudprovider/externalgrpc/externalgrpc_node_group_test.go

@@ -248,6 +250,8 @@ func TestCloudProvider_GetOptions(t *testing.T) {
 	assert.Equal(t, 0.7, opts.ScaleDownGpuUtilizationThreshold)
 	assert.Equal(t, time.Minute, opts.ScaleDownUnneededTime)
 	assert.Equal(t, time.Hour, opts.ScaleDownUnreadyTime)
+	assert.Equal(t, time.Hour, opts.ScaleDownUnreadyTime)


duplicated line

vadasambar · 2023-07-11T18:50:33Z

@dbonfigli I have addressed the review changes. Thanks for reviewing my PR 🙏

I guess I need approval from @BigDarkClown to merge the PR.

dbonfigli · 2023-07-11T19:23:29Z

.../cloudprovider/externalgrpc/examples/external-grpc-cloud-provider-service/wrapper/wrapper.go

@@ -351,6 +351,7 @@ func (w *Wrapper) NodeGroupGetOptions(_ context.Context, req *protos.NodeGroupAu
 		ScaleDownGpuUtilizationThreshold: pbDefaults.GetScaleDownGpuUtilizationThreshold(),
 		ScaleDownUnneededTime:            pbDefaults.GetScaleDownUnneededTime().Duration,
 		ScaleDownUnreadyTime:             pbDefaults.GetScaleDownUnneededTime().Duration,
+		MaxNodeProvisionTime:             pbDefaults.GetMaxNodeProvisionTime().Duration,


can you please also add the new field in the return value below?

…onTime` as 0 seconds Signed-off-by: vadasambar <surajrbanakar@gmail.com> chore(cloudprovider/externalgrpc): fix CI failing because of wrong header - CI complains if I remove or change the copyright header Signed-off-by: vadasambar <surajrbanakar@gmail.com> chore(cloudprovider/externalgrpc): fix spacing for license headers Signed-off-by: vadasambar <surajrbanakar@gmail.com> chore(cloudprovider/externalgrpc): remove extra space in copyright header Signed-off-by: vadasambar <surajrbanakar@gmail.com> test(cloudprovider/externalgrpc): add assert for `MaxNodeProvisionTime` - add extra assert for err test case Signed-off-by: vadasambar <surajrbanakar@gmail.com> fix: duplicate line Signed-off-by: vadasambar <surajrbanakar@gmail.com> docs: add example with `MaxNodeProvisionTime` Signed-off-by: vadasambar <surajrbanakar@gmail.com> refactor: return `MaxNodeProvisionTime` from `NodeGroupGetOptions` in response Signed-off-by: vadasambar <surajrbanakar@gmail.com>

MaciekPytel · 2023-07-31T13:50:44Z

/approve
I'm going to leave lgtm to @dbonfigli as our general policy is for cloudprovider reviews to be done by their OWNERS and not core CA. The change does look good to me for whatever it's worth.

A separate point - I expect that this will keep happening as more options are moved into GetOptions. And I expect that there will be more fields added there over time - there seem to be a lot of interest in making CA configurable on per-nodegroup level and there isn't really any particular reason for us not to support that.
With that in mind - is there some long-term solution here? Should we expect anyone adding new fields to GetOptions to extend grpc? If so - can the steps be documented?

Note: I don't think my point above should be blocking for this PR; just a potential follow-up idea.

k8s-ci-robot · 2023-07-31T13:50:52Z

[APPROVALNOTIFIER] This PR is APPROVED

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

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:

~~cluster-autoscaler/OWNERS~~ [MaciekPytel]

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

dbonfigli · 2023-07-31T18:31:02Z

A separate point - I expect that this will keep happening as more options are moved into GetOptions. And I expect that there will be more fields added there over time - there seem to be a lot of interest in making CA configurable on per-nodegroup level and there isn't really any particular reason for us not to support that. With that in mind - is there some long-term solution here? Should we expect anyone adding new fields to GetOptions to extend grpc? If so - can the steps be documented?

Note: I don't think my point above should be blocking for this PR; just a potential follow-up idea.

I think the problem is similar for in-tree cloud providers that implement GetOption (luckily, only aws, gce, azure, ovh, civo): changing the NodeGroupAutoscalingOptions struct can result in problems if also cloud providers are not updated to handle it. For example, I am pretty sure #5649 is also breaking the ovh cloud provider, since

autoscaler/cluster-autoscaler/cloudprovider/ovhcloud/ovh_cloud_node_group.go

Line 317 in 66b56c5

    
           func (ng *NodeGroup) GetOptions(defaults config.NodeGroupAutoscalingOptions) (*config.NodeGroupAutoscalingOptions, error) {

is not defining or passing back the default for MaxNodeProvisionTime. So, the log term plan for the externalgrpc cloud provider could be the same as for the in-tree ones, either trying to make non breaking changes with new fields that have defaults values that result in working code (like for example with the new ZeroOrMaxNodeScaling), or adding the feature, or reaching out to cloudprovider maintainers putting them on notice for the upcoming breaking change.

For the externalgrcp cloud provider this problem was particular noticeable also because in practice it was mandatory to also implement GetOption, a problem that should be fixed with #5937 .

vadasambar · 2023-08-28T14:10:35Z

@dbonfigli I think this PR needs your lgtm.

dbonfigli · 2023-08-28T16:13:54Z

/lgtm

k8s-ci-robot · 2023-08-28T16:13:59Z

@dbonfigli: changing LGTM is restricted to collaborators

In response to this:

/lgtm

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.

dbonfigli · 2023-08-28T16:19:47Z

@dbonfigli I think this PR needs your lgtm.

unfortunately i am not a member of the kubernetes org.

vadasambar · 2023-09-01T18:24:10Z

@dbonfigli I think this PR needs your lgtm.

unfortunately i am not a member of the kubernetes org.

Thanks for the response. I saw your comment in #5998 (comment)

@MaciekPytel we need an LGTM on this from you because @dbonfigli is not a Kubernetes member and cannot add LGTM label to the PR.

x13n · 2023-09-04T14:07:03Z

/lgtm

(based on @dbonfigli's comment above)

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. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. labels Jul 6, 2023

k8s-ci-robot requested a review from BigDarkClown July 6, 2023 19:20

k8s-ci-robot added the area/cluster-autoscaler label Jul 6, 2023

k8s-ci-robot requested a review from x13n July 6, 2023 19:20

k8s-ci-robot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Jul 6, 2023

vadasambar mentioned this pull request Jul 6, 2023

When using externalgrpc provider, nodes are created and then immediately deleted next loop iteration #5935

Closed

vadasambar commented Jul 6, 2023

View reviewed changes

vadasambar commented Jul 7, 2023

View reviewed changes

vadasambar force-pushed the fix/add-max-node-provision-time-to-external-grpc-provider branch from ee49e32 to 76092a2 Compare July 7, 2023 06:19

vadasambar marked this pull request as ready for review July 7, 2023 06:21

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 Jul 7, 2023

k8s-ci-robot requested a review from feiskyer July 7, 2023 06:21

dbonfigli approved these changes Jul 8, 2023

View reviewed changes

vadasambar force-pushed the fix/add-max-node-provision-time-to-external-grpc-provider branch from 289c10e to fd1a95a Compare July 11, 2023 18:48

vadasambar mentioned this pull request Jul 11, 2023

Jul 2023 vadafoss/daily-updates#11

Closed

dbonfigli reviewed Jul 11, 2023

View reviewed changes

vadasambar force-pushed the fix/add-max-node-provision-time-to-external-grpc-provider branch from fd1a95a to 6df52ec Compare July 13, 2023 04:33

dbonfigli mentioned this pull request Jul 13, 2023

fix(cloudprovider/exteranlgrpc): properly handle unimplemented methods #5937

Merged

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jul 31, 2023

vadasambar mentioned this pull request Aug 1, 2023

Aug 2023 vadafoss/daily-updates#12

Closed

vadasambar mentioned this pull request Sep 1, 2023

Sep 2023 vadafoss/daily-updates#13

Closed

dbonfigli mentioned this pull request Sep 1, 2023

fix MaxNodeProvisionTime and ZeroOrMaxNodeScaling node-groups handlin… #5998

Closed

k8s-ci-robot assigned x13n Sep 4, 2023

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Sep 4, 2023

k8s-ci-robot merged commit 57f2814 into kubernetes:master Sep 4, 2023

vadasambar mentioned this pull request Oct 3, 2023

Oct 2023 vadafoss/daily-updates#14

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(cloudprovider/externalgrpc): `GetOptions` returns `MaxNodeProvisionTime` as 0 seconds #5936

fix(cloudprovider/externalgrpc): `GetOptions` returns `MaxNodeProvisionTime` as 0 seconds #5936

vadasambar commented Jul 6, 2023 •

edited

Loading

vadasambar Jul 6, 2023

vadasambar Jul 6, 2023

vadasambar Jul 6, 2023

vadasambar Jul 7, 2023 •

edited

Loading

dbonfigli left a comment •

edited

Loading

dbonfigli Jul 8, 2023

vadasambar commented Jul 11, 2023

dbonfigli Jul 11, 2023

vadasambar Jul 13, 2023

MaciekPytel commented Jul 31, 2023

k8s-ci-robot commented Jul 31, 2023

dbonfigli commented Jul 31, 2023 •

edited

Loading

vadasambar commented Aug 28, 2023

dbonfigli commented Aug 28, 2023

k8s-ci-robot commented Aug 28, 2023

dbonfigli commented Aug 28, 2023

vadasambar commented Sep 1, 2023

x13n commented Sep 4, 2023

	package any

	import (
	protoreflect "google.golang.org/protobuf/reflect/protoreflect"
	protoimpl "google.golang.org/protobuf/runtime/protoimpl"
	anypb "google.golang.org/protobuf/types/known/anypb"
	reflect "reflect"
	)

	// Symbols defined in public import of google/protobuf/any.proto.

	type Any = anypb.Any

fix(cloudprovider/externalgrpc): GetOptions returns MaxNodeProvisionTime as 0 seconds #5936

fix(cloudprovider/externalgrpc): GetOptions returns MaxNodeProvisionTime as 0 seconds #5936

Conversation

vadasambar commented Jul 6, 2023 • edited Loading

What type of PR is this?

What this PR does / why we need it:

Which issue(s) this PR fixes:

Special notes for your reviewer:

Does this PR introduce a user-facing change?

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

vadasambar Jul 6, 2023

Choose a reason for hiding this comment

vadasambar Jul 6, 2023

Choose a reason for hiding this comment

vadasambar Jul 6, 2023

Choose a reason for hiding this comment

vadasambar Jul 7, 2023 • edited Loading

Choose a reason for hiding this comment

dbonfigli left a comment • edited Loading

Choose a reason for hiding this comment

dbonfigli Jul 8, 2023

Choose a reason for hiding this comment

vadasambar commented Jul 11, 2023

dbonfigli Jul 11, 2023

Choose a reason for hiding this comment

vadasambar Jul 13, 2023

Choose a reason for hiding this comment

MaciekPytel commented Jul 31, 2023

k8s-ci-robot commented Jul 31, 2023

dbonfigli commented Jul 31, 2023 • edited Loading

vadasambar commented Aug 28, 2023

dbonfigli commented Aug 28, 2023

k8s-ci-robot commented Aug 28, 2023

dbonfigli commented Aug 28, 2023

vadasambar commented Sep 1, 2023

x13n commented Sep 4, 2023

fix(cloudprovider/externalgrpc): `GetOptions` returns `MaxNodeProvisionTime` as 0 seconds #5936

fix(cloudprovider/externalgrpc): `GetOptions` returns `MaxNodeProvisionTime` as 0 seconds #5936

vadasambar commented Jul 6, 2023 •

edited

Loading

vadasambar Jul 7, 2023 •

edited

Loading

dbonfigli left a comment •

edited

Loading

dbonfigli commented Jul 31, 2023 •

edited

Loading