Implement cluster autoscaler as bootstrap addon #9787

olemarkus · 2020-08-20T11:06:22Z

We see that when managing a larger number of clusters, getting the cluster autoscaler version correct is a bit of a hassle. We also need to take care to properly update CAS after cluster upgrades etc. So adding CAS as a configurable bootstrap addon seems like a good idea to make this more manageable.

If no one objects to this, I will finish this and add documentation and validation.

olemarkus · 2020-08-20T11:06:57Z

/cc @hakman

moshevayner · 2020-08-20T12:39:39Z

That's a great idea, thanks for adding that @olemarkus !!

k8s-ci-robot · 2020-08-20T12:44:43Z

@olemarkus: you cannot LGTM your own PR.

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.

olemarkus · 2020-08-20T12:45:51Z

/retest

moshevayner · 2020-08-20T13:10:00Z

BTW, I guess it would be also worth adding some documentation for it? Or will that be done through a separate PR?

olemarkus · 2020-08-20T14:55:50Z

Docs will come as part of this PR. Just want to ensure there are no objections before starting on that.

pkg/model/components/clusterautoscaler.go

upup/models/cloudup/resources/addons/cluster-autoscaler.addons.k8s.io/k8s-1.15.yaml.template

upup/pkg/fi/cloudup/bootstrapchannelbuilder.go

upup/pkg/fi/cloudup/template_functions.go

docs/cluster_spec.md

pkg/apis/kops/cluster.go

pkg/apis/kops/componentconfig.go

pkg/apis/kops/v1alpha2/cluster.go

pkg/apis/kops/componentconfig.go

pkg/apis/kops/v1alpha2/componentconfig.go

upup/pkg/fi/cloudup/bootstrapchannelbuilder.go

upup/pkg/fi/cloudup/template_functions.go

olemarkus · 2020-08-27T09:00:31Z

/retest

olemarkus · 2020-08-28T07:19:00Z

/retest

k8s/crds/kops.k8s.io_clusters.yaml

pkg/apis/kops/cluster.go

pkg/model/components/clusterautoscaler.go

k8s/crds/kops.k8s.io_clusters.yaml

olemarkus · 2020-08-31T09:17:44Z

/retest

olemarkus · 2020-08-31T10:03:28Z

/test pull-kops-e2e-cni-cilium

olemarkus · 2020-08-31T12:45:51Z

/retest

pkg/apis/kops/validation/validation.go

pkg/model/components/clusterautoscaler.go

upup/pkg/fi/cloudup/template_functions.go

pkg/model/components/clusterautoscaler.go

Use provider-agnostic node definition for cas instead of aws auto-discovery Validate clusterAutoscalerSpec Add spec documentation Add cas docs Make CRDs Apply suggestions from code review Co-authored-by: John Gardiner Myers <[email protected]> Add enabled flag to cas config Apply suggestions from code review Co-authored-by: Guy Templeton <[email protected]> Add support for custom cas image Support more k8s versions Use full image names

hakman · 2020-09-03T11:16:34Z

@johngmyers leaving the approval to you, in case there is anything left from your point of view.
/lgtm

johngmyers

Neither of the review comments is blocking. Holding to give opportunity to address.

/approve
/hold

johngmyers · 2020-09-04T17:23:25Z

pkg/model/components/clusterautoscaler.go

+
+	if cas.Image == nil {
+
+		image := "k8s.gcr.io/autoscaling/cluster-autoscaler:latest"


I'm not a big fan of mutable tags such as "latest"; I'd prefer using "v1.19.0" (or whatever the highest known version is) for newer Kubernetes versions.

I don't have any preference here.

johngmyers · 2020-09-04T17:26:21Z

upup/pkg/fi/cloudup/bootstrapchannelbuilder.go

@@ -521,6 +521,27 @@ func (b *BootstrapChannelBuilder) buildAddons() *channelsapi.Addons {
 		}
 	}

+	if b.Cluster.Spec.ClusterAutoscaler != nil && fi.BoolValue(b.Cluster.Spec.ClusterAutoscaler.Enabled) {


Perhaps we should default Enabled to true if ClusterAutoscaler is non-nil

We agreed to keep it disabled by default at last office hours.
Maybe in 1.20 we materialise it into to the cluster spec so that people know it's enabled.

But default is to have a nil ClusterAutoscaler field. Why would someone specify a ClusterAutoscaler field yet want Enabled to default to false?

I agree that it may be a good idea in general, just don't know how used this pattern is in kops.
Sometimes when I test or just want to disable something I just comment the enabled: true line, without actually but keep all other setting to make it easy to reenable.

One of the reasons for starting the discussion on #9661 was to establish some guidelines for enabled/disabled and managed/unmanaged, but seems not very popular topic. Maybe should open a more generic one.

k8s-ci-robot · 2020-09-04T17:30:49Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: johngmyers, olemarkus

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:

~~OWNERS~~ [johngmyers]

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

olemarkus · 2020-09-05T04:59:02Z

I am a bit back and forth on what I prefer on the above. So I think I'll just let this one go in and then do a follow-up. Especially if cluster-autoscaler should prove to be slower with their support for newer k8s in the future.
/hold cancel

paalkr · 2020-09-21T08:13:34Z

Thanks for contributing with this addition, that makes deploying the CAS easier. We have a use case that uses a mix of different cluster AutoScaling methods (external and CAS) for various node pools. I think it would be vice to add a keyword to the instance group spec to either include or exclude the group with CAS.

olemarkus · 2020-09-21T11:10:24Z

Thanks. I'd add this as a separate issue. I may not be able to implement this in the near future, but it should be a pretty easy "good first issue".

avdhoot · 2020-10-02T12:52:33Z

Might late to join the party. Thanks @olemarkus for this feature. It will remove extra step to manage autoscaler version separately . But it will be good if we add support for priority expander. When you have multiple instance group mix of on-demand & spot instance group. priority expander help to set priority to spot instance group.

k8s-ci-robot added do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. labels Aug 20, 2020

k8s-ci-robot requested review from gjtempleton and johngmyers August 20, 2020 11:06

k8s-ci-robot added area/addons area/api labels Aug 20, 2020

k8s-ci-robot requested a review from hakman August 20, 2020 11:06

johngmyers requested changes Aug 21, 2020

View reviewed changes

olemarkus force-pushed the cas branch from 377f995 to f397ac0 Compare August 21, 2020 09:40

k8s-ci-robot added the area/documentation label Aug 21, 2020

olemarkus force-pushed the cas branch from 49bd474 to 485a532 Compare August 21, 2020 10:13

olemarkus changed the title ~~WIP Implement cluster autoscaler as bootstrap addon~~ Implement cluster autoscaler as bootstrap addon Aug 21, 2020

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 Aug 21, 2020

olemarkus requested a review from johngmyers August 21, 2020 11:15

johngmyers requested changes Aug 27, 2020

View reviewed changes

olemarkus force-pushed the cas branch 3 times, most recently from b1b88e1 to d598472 Compare August 27, 2020 07:55

gjtempleton reviewed Aug 28, 2020

View reviewed changes

k8s/crds/kops.k8s.io_clusters.yaml Show resolved Hide resolved

gjtempleton reviewed Aug 28, 2020

View reviewed changes

pkg/apis/kops/cluster.go Outdated Show resolved Hide resolved

gjtempleton reviewed Aug 28, 2020

View reviewed changes

pkg/model/components/clusterautoscaler.go Outdated Show resolved Hide resolved

k8s/crds/kops.k8s.io_clusters.yaml Show resolved Hide resolved

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

olemarkus force-pushed the cas branch from 5cf116f to d5a1323 Compare August 31, 2020 07:38

johngmyers requested changes Sep 2, 2020

View reviewed changes

olemarkus force-pushed the cas branch 3 times, most recently from e00a2d4 to feacaf1 Compare September 2, 2020 19:44

hakman reviewed Sep 3, 2020

View reviewed changes

pkg/model/components/clusterautoscaler.go Outdated Show resolved Hide resolved

olemarkus force-pushed the cas branch 2 times, most recently from 38f5fdf to ce5a2cf Compare September 3, 2020 07:48

olemarkus force-pushed the cas branch from ce5a2cf to a0e9fab Compare September 3, 2020 07:52

olemarkus requested a review from hakman September 3, 2020 09:26

k8s-ci-robot assigned hakman Sep 3, 2020

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

johngmyers reviewed Sep 4, 2020

View reviewed changes

k8s-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Sep 4, 2020

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Sep 4, 2020

k8s-ci-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Sep 5, 2020

k8s-ci-robot merged commit d8b7310 into kubernetes:master Sep 5, 2020

k8s-ci-robot added this to the v1.19 milestone Sep 5, 2020

olemarkus deleted the cas branch September 21, 2020 11:06

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implement cluster autoscaler as bootstrap addon #9787

Implement cluster autoscaler as bootstrap addon #9787

olemarkus commented Aug 20, 2020

olemarkus commented Aug 20, 2020

moshevayner commented Aug 20, 2020

k8s-ci-robot commented Aug 20, 2020

olemarkus commented Aug 20, 2020

moshevayner commented Aug 20, 2020

olemarkus commented Aug 20, 2020

olemarkus commented Aug 27, 2020

olemarkus commented Aug 28, 2020

olemarkus commented Aug 31, 2020

olemarkus commented Aug 31, 2020

olemarkus commented Aug 31, 2020

hakman commented Sep 3, 2020

johngmyers left a comment

johngmyers Sep 4, 2020

hakman Sep 4, 2020

johngmyers Sep 4, 2020

hakman Sep 4, 2020

johngmyers Sep 4, 2020

hakman Sep 5, 2020

k8s-ci-robot commented Sep 4, 2020

olemarkus commented Sep 5, 2020

paalkr commented Sep 21, 2020

olemarkus commented Sep 21, 2020

avdhoot commented Oct 2, 2020


		if cas.Image == nil {

		image := "k8s.gcr.io/autoscaling/cluster-autoscaler:latest"

Implement cluster autoscaler as bootstrap addon #9787

Implement cluster autoscaler as bootstrap addon #9787

Conversation

olemarkus commented Aug 20, 2020

olemarkus commented Aug 20, 2020

moshevayner commented Aug 20, 2020

k8s-ci-robot commented Aug 20, 2020

olemarkus commented Aug 20, 2020

moshevayner commented Aug 20, 2020

olemarkus commented Aug 20, 2020

olemarkus commented Aug 27, 2020

olemarkus commented Aug 28, 2020

olemarkus commented Aug 31, 2020

olemarkus commented Aug 31, 2020

olemarkus commented Aug 31, 2020

hakman commented Sep 3, 2020

johngmyers left a comment

Choose a reason for hiding this comment

johngmyers Sep 4, 2020

Choose a reason for hiding this comment

hakman Sep 4, 2020

Choose a reason for hiding this comment

johngmyers Sep 4, 2020

Choose a reason for hiding this comment

hakman Sep 4, 2020

Choose a reason for hiding this comment

johngmyers Sep 4, 2020

Choose a reason for hiding this comment

hakman Sep 5, 2020

Choose a reason for hiding this comment

k8s-ci-robot commented Sep 4, 2020

olemarkus commented Sep 5, 2020

paalkr commented Sep 21, 2020

olemarkus commented Sep 21, 2020

avdhoot commented Oct 2, 2020