Add gRPC expander plugin #4452

evansheng · 2021-11-09T01:05:33Z

Adds a new expander grpc, implementing the gRPC plugin outlined in: #4134

This PR adds a gRPC client as an expander in CA, and calls out to a gRPC server with config specified by two new flags --grpcExpanderCert and --grpcExpanderUrl. Should initialization of the client fail, or any returned response be invalid or malformed, the expander fails open, and doesn't filter any Options.

evansheng · 2021-11-09T01:10:38Z

I've built this and deployed it locally, alongside an internal gRPC server - verified that this is working e2e successfully. I'm able to return options successfully via bestOptions.
I've also successfully tested failure modes in which the gRPC server doesn't exist, we fall back to appropriate specified expanders in the list.

MaciekPytel · 2021-11-15T14:09:48Z

cluster-autoscaler/core/autoscaler.go

@@ -48,6 +49,7 @@ type AutoscalerOptions struct {
 	EstimatorBuilder       estimator.EstimatorBuilder
 	Processors             *ca_processors.AutoscalingProcessors
 	Backoff                backoff.Backoff
+	GRPCOpts               grpcplugin.GRPCOpts


I don't think this is needed here. It's not used by anything except the grpc plugin and I think it's just leaking implementation details of this specific expander.

MaciekPytel · 2021-11-15T14:13:22Z

cluster-autoscaler/core/autoscaler.go

@@ -101,9 +103,12 @@ func initializeDefaultOptions(opts *AutoscalerOptions) error {
 	if opts.CloudProvider == nil {
 		opts.CloudProvider = cloudBuilder.NewCloudProvider(opts.AutoscalingOptions)
 	}
+	if strings.Contains(opts.ExpanderNames, expander.GRPCExpanderName) && opts.GRPCExpanderURL != "" && opts.GRPCExpanderCert != "" {
+		opts.GRPCOpts = grpcplugin.GRPCOpts{opts.GRPCExpanderCert, opts.GRPCExpanderURL}


Similar to the comment above - I don't think code in core/ should contain any logic specific to a particular expander. I'd much rather pass AutoscalingOptions to expander factory (so it has access to all flags) or just define expander-specific flags directly in the expander.

Opted for passing the grpc expander flags as two more args, instead of either creating an import cycle (core & expander factory), or moving the AutoscalerOptions struct to config

MaciekPytel · 2021-11-15T14:20:58Z

cluster-autoscaler/go.mod

@@ -25,6 +25,8 @@ require (
 	golang.org/x/crypto v0.0.0-20210513164829-c07d793c2f9a
 	golang.org/x/oauth2 v0.0.0-20210514164344-f6687ab2804c
 	google.golang.org/api v0.46.0
+	google.golang.org/grpc v1.38.0


Kubernetes already uses grpc in a different version: https://github.com/kubernetes/kubernetes/blob/master/go.mod#L102, which makes it a transitive dependency of CA. In my experience is that different grpc versions don't always play well together. Right now the difference in version number is pretty small, but is there some way in which we can guarantee the same grpc version is used by CA and kubernetes going forward?

I can use the same version that that kubernetes repo is using pretty easily, but do you have suggestions on how to keep it synced? I did a cursory search across other grpc uses in the k8s repo and found many repos seem to use many different versions:

https://github.com/kubernetes/node-problem-detector/blob/fc0edbd222e16b775ec58a253366a41822ecb3d5/vendor/modules.txt#L348

https://github.com/kubernetes/ingress-gce/blob/b1b62298cfc7adb08154402f2b26607d1a94d65e/vendor/modules.txt#L191

https://github.com/kubernetes/kops/blob/e40fd308b26676ef3f054e702640d32fecaa9271/vendor/modules.txt#L946

https://github.com/kubernetes/cloud-provider-openstack/blob/197604cb32c5d1850aeb8c903b7ea280f9bc0e1e/go.mod#L32
etc...

MaciekPytel · 2021-11-15T14:28:05Z

cluster-autoscaler/expander/grpcplugin/grpc_client.go

+	schedulerframework "k8s.io/kubernetes/pkg/scheduler/framework"
+
+	"google.golang.org/grpc"
+	"google.golang.org/grpc/credentials"


That is a big dependency to push on anyone forking CA (which is how a lot of cloudproviders work), especially given that they're quite likely to already use grpc, possibly in some other way. Can we try and make it easy to get rid of this dependency by tightly controlling where we import this expander from (ideally try and avoid importing it directly anywhere outside of expander/)?

good call - removing the grpcOpts struct and passing flags directly to expander factory directly fixes this

evansheng · 2021-11-30T20:51:08Z

added the autogenerated protobuf code to exclusion from golint

evansheng · 2021-11-30T20:51:36Z

/assign @aleksandra-malinowska

evansheng · 2021-11-30T20:52:15Z

@MaciekPytel I believe this PR is ready for another round of review when you get the chance

elmiko

hey @evansheng , i'm still reviewing this code, but one thing i am wondering about is documenting how to use this expander. is the enhancement meant to be the main documentation or have you considered adding detailed usage instructions to one of our READMEs?

i would like to try this out if possible, but i am still understanding how to setup the grpc expander. perhaps a small "fake" expander would be nice to demonstrate how a user could build one. or is the mock expander meant to be an example?

evansheng · 2021-12-27T22:14:45Z

@elmiko added a README on how to use this new expander, and added some starter code
PTAL when you get the change

elmiko

thanks @evansheng , that readme and example are exactly what i was looking for. i'd like to test drive this a little more, but i'm +1 for merging.

evansheng · 2022-01-13T07:52:53Z

@MaciekPytel friendly ping for another review on this pr

bpineau · 2022-01-17T14:42:52Z

cluster-autoscaler/expander/grpcplugin/protos/expander.proto

+message BestOptionsRequest {
+  repeated Option options = 1;
+  // key is node id from options
+  map<string, k8s.io.api.core.v1.Node> nodeInfoMap = 2;


So over gRPC nodeInfoMap is really a Node map: is that required to save rpc calls throughput and simplify the proto?

That's a very good point. If it's a node map we should name it nodeMap (or maybe templateNodeMap?) and not nodeInfoMap. NodeInfo is a well defined concept that represent a Node + pods running on that node.

The limitation of passing just the nodes is that any information about daemonsets and static pods is no longer available to expander. It's probably not a blocker (most expander implementations can probably live with that), but it's probably worth calling out in the README.

Alternatively you could consider actually making it an actual nodeInfoMap by including both the node and pods running on it in the value.

called out in readme, and renamed variable to be more appropriate

MaciekPytel · 2022-01-17T14:51:49Z

cluster-autoscaler/expander/factory/expander_factory.go

@@ -67,6 +70,9 @@ func ExpanderStrategyFromStrings(expanderFlags []string, cloudProvider cloudprov
 			stopChannel := make(chan struct{})
 			lister := kubernetes.NewConfigMapListerForNamespace(kubeClient, stopChannel, configNamespace)
 			filters = append(filters, priority.NewFilter(lister.ConfigMaps(configNamespace), autoscalingKubeClients.Recorder))
+		case expander.GRPCExpanderName:
+			klog.V(1).Info("GRPC expander chosen")


nit: that feels somewhat redundant, we already log all flag values by default.

MaciekPytel · 2022-01-17T15:01:17Z

cluster-autoscaler/expander/grpcplugin/fake_grpc_server.go

+// copy the protos/expander.pb.go to your other application's repo, so it has access to the protobuf definitions
+
+// Serve should be called by the main() function in main.go of the Expander Server repo to start serving
+func Serve(certPath string, keyPath string, port uint) {


I'd prefer to move this file to a separate package (grpcplugin/example?) to make it clear it's an example implementation and not part of CA integration.

Also this is already really helpful, but if you already have all the logic maybe also consider adding the main function and a simple Makefile so it's a working example?

including a simple main function but leaving out the makefile, as it is up to the user how they would like to structure their project repo

MaciekPytel · 2022-01-17T15:56:03Z

cluster-autoscaler/expander/grpcplugin/protos/expander.proto

+message BestOptionsRequest {
+  repeated Option options = 1;
+  // key is node id from options
+  map<string, k8s.io.api.core.v1.Node> nodeInfoMap = 2;


That's a very good point. If it's a node map we should name it nodeMap (or maybe templateNodeMap?) and not nodeInfoMap. NodeInfo is a well defined concept that represent a Node + pods running on that node.

The limitation of passing just the nodes is that any information about daemonsets and static pods is no longer available to expander. It's probably not a blocker (most expander implementations can probably live with that), but it's probably worth calling out in the README.

Alternatively you could consider actually making it an actual nodeInfoMap by including both the node and pods running on it in the value.

MaciekPytel · 2022-01-17T16:00:35Z

cluster-autoscaler/expander/grpcplugin/protos/expander.proto

+
+package grpcplugin;
+import "k8s.io/api/core/v1/generated.proto";
+//import "google/protobuf/struct.proto";


Please remove if unused.

MaciekPytel · 2022-01-17T16:38:47Z

cluster-autoscaler/expander/grpcplugin/grpc_client.go

+
+	// if no Cert file specified, use insecure
+	if expanderCert == "" {
+		dialOpt = grpc.WithInsecure()


That seems like an unnecessary risk. Kubernetes itself moved away from allowing insecure connections and I think we also shouldn't allow them.

removed this option

MaciekPytel · 2022-01-17T16:46:50Z

cluster-autoscaler/expander/grpcplugin/grpc_client.go

+
+	// call gRPC server to get BestOption
+	klog.V(2).Info("GPRC call of best options to server with ", len(nodeGroupIDOptionMap), " options")
+	ctx, cancel := context.WithTimeout(context.Background(), 5*time.Second)


Consider making 5s a const

MaciekPytel · 2022-01-17T16:53:14Z

cluster-autoscaler/expander/grpcplugin/grpc_client.go

+	}
+}
+
+// populateNodeInfoForGRPC modifies the nodeInfo object, and replaces it with the v1.Node to pass through grpc


That comment is misleading, original nodeInfos are not modified (and they must not be!).

rewrote comment so it is more accurate

MaciekPytel · 2022-01-17T16:54:21Z

cluster-autoscaler/expander/grpcplugin/grpc_client.go

+	for _, option := range bestOptionsResponseOptions {
+		if option == nil {
+			klog.Errorf("gRPC server returned nil Option")
+			return nil


Wouldn't continue make more sense here? Other options may not be nil.

good catch, fixed

MaciekPytel · 2022-01-17T16:54:56Z

cluster-autoscaler/expander/grpcplugin/grpc_client.go

+			options = append(options, nodeGroupIDOptionMap[option.NodeGroupId])
+		} else {
+			klog.Errorf("gRPC server returned invalid nodeGroup ID: ", option.NodeGroupId)
+			return nil


Same as above, continue seems more natural.

MaciekPytel · 2022-01-17T16:56:33Z

cluster-autoscaler/expander/grpcplugin/grpc_client_test.go

+	}
+)
+
+func TestPopulateOptionsForGrpc(t *testing.T) {


Please consider a table-based test to cover various inputs, including edge cases (ex. empty list of options).

added some tests, as requested

evansheng · 2022-02-03T01:27:08Z

@MaciekPytel addressed your comments, PTAL! sorry for the delay

elmiko · 2022-02-14T18:19:57Z

we talked about this pr at the sig meeting today, i think things are mostly looking good, but there was a suggestion from the sig that these commits should be squashed before merging. ideally to the minimum number of logical, atomic, commits as possible.

evansheng · 2022-02-16T20:41:20Z

@elmiko @MaciekPytel - addressed all comments, and squashed into two commits.
I think this PR is ready for another round of review!

mwielgus

/lgtm
/approve

k8s-ci-robot · 2022-02-21T14:53:17Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: evansheng, mwielgus

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~~ [mwielgus]
~~hack/OWNERS~~ [mwielgus]

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

Add gRPC expander plugin

k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. labels Nov 9, 2021

k8s-ci-robot requested review from aleksandra-malinowska and feiskyer November 9, 2021 01:05

jbartosik added the area/cluster-autoscaler label Nov 9, 2021

MaciekPytel reviewed Nov 15, 2021

View reviewed changes

k8s-ci-robot assigned aleksandra-malinowska Nov 30, 2021

evansheng requested a review from MaciekPytel November 30, 2021 20:51

evansheng force-pushed the es--grpc-expander-plugin branch from 269d9d0 to d821603 Compare November 30, 2021 22:00

elmiko reviewed Dec 14, 2021

View reviewed changes

evansheng mentioned this pull request Dec 14, 2021

CA expander plugin proposal #4134

Merged

elmiko reviewed Jan 4, 2022

View reviewed changes

bpineau reviewed Jan 17, 2022

View reviewed changes

MaciekPytel reviewed Jan 17, 2022

View reviewed changes

k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jan 29, 2022

evansheng force-pushed the es--grpc-expander-plugin branch from 4a607d9 to 9855a94 Compare February 2, 2022 23:55

k8s-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Feb 2, 2022

evansheng added 2 commits February 16, 2022 12:34

Add grpc expander and tests

4504f55

add starter code and readme for grpc expander usage

a2b24e0

evansheng force-pushed the es--grpc-expander-plugin branch from 9855a94 to a2b24e0 Compare February 16, 2022 20:36

mwielgus approved these changes Feb 21, 2022

View reviewed changes

k8s-ci-robot assigned mwielgus Feb 21, 2022

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Feb 21, 2022

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Feb 21, 2022

k8s-ci-robot merged commit 0123869 into kubernetes:master Feb 21, 2022

evansheng pushed a commit to airbnb/autoscaler that referenced this pull request Mar 24, 2022

Merge pull request kubernetes#4452 from airbnb/es--grpc-expander-plugin

bc9e09a

Add gRPC expander plugin

drmorr0 mentioned this pull request Jun 1, 2022

REQUEST: New membership for @drmorr0 kubernetes/org#3468

Closed

9 tasks

evansheng mentioned this pull request Jun 8, 2022

REQUEST: New membership for evansheng kubernetes/org#3492

Closed

9 tasks

jiancheung pushed a commit to airbnb/autoscaler that referenced this pull request Jul 29, 2022

Merge pull request kubernetes#4452 from airbnb/es--grpc-expander-plugin

39da39c

Add gRPC expander plugin

akirillov pushed a commit to airbnb/autoscaler that referenced this pull request Oct 27, 2022

Merge pull request kubernetes#4452 from airbnb/es--grpc-expander-plugin

c98a480

Add gRPC expander plugin

akirillov mentioned this pull request Oct 27, 2022

cluster autoscaler patch cluster autoscaler 1.21.3 airbnb0 airbnb/autoscaler#29

Merged

akirillov pushed a commit to airbnb/autoscaler that referenced this pull request Nov 2, 2022

Merge pull request kubernetes#4452 from airbnb/es--grpc-expander-plugin

1ad7ded

Add gRPC expander plugin

akirillov mentioned this pull request Nov 2, 2022

cluster autoscaler patch cluster autoscaler 1.23.1 airbnb0 airbnb/autoscaler#30

Merged

bcostabatista pushed a commit to airbnb/autoscaler that referenced this pull request Nov 7, 2022

Merge pull request kubernetes#4452 from airbnb/es--grpc-expander-plugin

e0f4c11

Add gRPC expander plugin

bcostabatista mentioned this pull request Nov 7, 2022

cluster autoscaler 1.22.14 release airbnb/autoscaler#32

Merged

Add gRPC expander plugin #4452

Add gRPC expander plugin #4452

Conversation

evansheng commented Nov 9, 2021

evansheng commented Nov 9, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

evansheng Nov 20, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

evansheng Nov 20, 2021 • edited Loading

Choose a reason for hiding this comment

evansheng commented Nov 30, 2021

evansheng commented Nov 30, 2021

evansheng commented Nov 30, 2021

elmiko left a comment

Choose a reason for hiding this comment

evansheng commented Dec 27, 2021

elmiko left a comment

Choose a reason for hiding this comment

evansheng commented Jan 13, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

evansheng Feb 16, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

evansheng commented Feb 3, 2022 • edited Loading

elmiko commented Feb 14, 2022

evansheng commented Feb 16, 2022

mwielgus left a comment

Choose a reason for hiding this comment

k8s-ci-robot commented Feb 21, 2022

evansheng Nov 20, 2021 •

edited

Loading

evansheng Nov 20, 2021 •

edited

Loading

evansheng Feb 16, 2022 •

edited

Loading

evansheng commented Feb 3, 2022 •

edited

Loading