[addons/CA] Add support for specifying resources and metrics #10281

dntosas · 2020-11-20T19:15:50Z

In this PR, we make some additions on Cluster Autoscaler Addon Specs

Resources

We enable users to set their desired capacity for cluster-autoscaler addon.

There are edge cases, especially in big clusters, where autoscaler needs
to reconcile a large number of objects thus may need increased memory to
avoid OOMkills or increased cpu to avoid saturation.

Metrics

Cluster autoscaler provides valuable insights for monitoring capacity
allocation and scheduling aspects of a cluster. In this commit, we
enable users to add proper annotation on deployment to scrape metrics
via Prometheus.

Signed-off-by: dntosas ntosas@gmail.com

k8s-ci-robot · 2020-11-20T19:15:58Z

Hi @dntosas. Thanks for your PR.

I'm waiting for a kubernetes member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

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.

hakman · 2020-11-20T19:25:51Z

/ok-to-test
/cc @olemarkus

olemarkus · 2020-11-20T19:31:39Z

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

@@ -134,6 +134,12 @@ spec:
    metadata:
      labels:
        app: cluster-autoscaler
+      {{ if .Metrics }}


These annotations are not much in use anymore and we tend not to set them. Instead we expect users to create pod/service monotors themselves.

There are cases like coreDNS where these annotations seem useful. Do you suggest completely drop from here or migrating to a ServiceMonitor? I am cool with both options ^^

Note that since Metrics is a pointer you need to use WithDefaultBool.

olemarkus · 2020-11-20T19:33:23Z

I am not sure we should set any limits. It is rather dangerous to have CAS crashing due to oom. Maybe it is better to remove limits alltoghether.

dntosas · 2020-11-20T19:39:31Z

I am not sure we should set any limits. It is rather dangerous to have CAS crashing due to oom. Maybe it is better to remove limits alltoghether.

Will do that ^

Two more thoughts to consider:

Do you think is worth make replicas editable?
Should we also bump container image patch versions?
If yes, could these two included on this PR or we need separate ones?

Sorry for the many questions 😋

olemarkus · 2020-11-20T19:46:29Z

Bumping patch versions is good. I didn't do that as I suspect more patch versions will be released before kops 1.20. but feel free to add to this PR.

I don't think making replicas configurable is that useful. But if someone makes a good case for it, I will certainly consider it.

olemarkus · 2021-01-10T11:06:27Z

@dntosas do you have time to look at wrapping this one up?

dntosas · 2021-01-10T20:50:44Z

@dntosas do you have time to look at wrapping this one up?

sure ^^ i let it stale because i didn't understand if we wanted to close this PR as not needed

k8s-ci-robot · 2021-01-10T22:07:31Z

@dntosas: The following test failed, say /retest to rerun all failed tests:

Test name	Commit	Details	Rerun command
pull-kops-verify-hashes	`6f09f45`	link	`/test pull-kops-verify-hashes`

Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR.

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. I understand the commands that are listed here.

olemarkus

Sorry for the confusion. My intention was to only provide feedback on improving this.

olemarkus · 2021-01-11T12:27:50Z

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

@@ -134,6 +134,12 @@ spec:
    metadata:
      labels:
        app: cluster-autoscaler
+      {{ if WithDefaultBool .Metrics false }}


If the only thing this flag does is add the annotation, it is not that useful. You could just add the annotations without the flag. Typically such a flag would be used for enabling/disabling CAS serving metrics in the first place.

removed flag also ^^

olemarkus · 2021-01-11T12:28:37Z

pkg/model/components/clusterautoscaler.go

@@ -83,6 +83,9 @@ func (b *ClusterAutoscalerOptionsBuilder) BuildOptions(o interface{}) error {
 	if cas.NewPodScaleUpDelay == nil {
 		cas.NewPodScaleUpDelay = fi.String("0s")
 	}
+	if cas.Metrics == nil {


You already default this one to false in the manifest. So you do not need this bit

removed it ^^

olemarkus · 2021-01-11T18:03:28Z

docs/cluster_spec.md

+    skipNodesWithSystemPods: true
+    cpuRequest: "100m"
+    memoryRequest: "300Mi"
+    metrics: true


If you remove this one, we should be good to go

- Resources We enable users to set their desired capacity for cluster-autoscaler addon. There are edge cases, especially in big clusters, where autoscaler needs to reconcile a large number of objects thus may need increased memory or increased cpu to avoid saturation. - Metrics Cluster autoscaler provides valuable insights for monitoring capacity allocation and scheduling aspects of a cluster. In this commit, we add proper annotation on deployment to enable Prometheus scrape metrics. We also bump patch version of container images. Signed-off-by: dntosas <ntosas@gmail.com>

olemarkus · 2021-01-11T19:26:20Z

Cool. Thanks
/lgtm
/approve

k8s-ci-robot · 2021-01-11T19:26:38Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: dntosas, 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~~ [olemarkus]

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 added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Nov 20, 2020

k8s-ci-robot added needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Nov 20, 2020

k8s-ci-robot requested review from hakman and joshbranham November 20, 2020 19:16

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

dntosas force-pushed the cluster-autoscaler-improvements branch from 79851ef to ba32976 Compare November 20, 2020 19:22

k8s-ci-robot requested a review from olemarkus November 20, 2020 19:25

k8s-ci-robot added ok-to-test Indicates a non-member PR verified by an org member that is safe to test. and removed needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Nov 20, 2020

dntosas force-pushed the cluster-autoscaler-improvements branch from ba32976 to ec2b3e2 Compare November 20, 2020 19:27

k8s-ci-robot added the area/documentation label Nov 20, 2020

dntosas force-pushed the cluster-autoscaler-improvements branch from ec2b3e2 to 0b98e56 Compare November 20, 2020 19:28

olemarkus reviewed Nov 20, 2020

View reviewed changes

dntosas force-pushed the cluster-autoscaler-improvements branch from 0b98e56 to 6f09f45 Compare November 20, 2020 19:35

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

dntosas force-pushed the cluster-autoscaler-improvements branch from 6f09f45 to 48be140 Compare January 10, 2021 22:02

k8s-ci-robot added size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. and removed needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Jan 10, 2021

dntosas force-pushed the cluster-autoscaler-improvements branch from 48be140 to d1db788 Compare January 10, 2021 22:04

k8s-ci-robot removed the size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. label Jan 10, 2021

k8s-ci-robot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Jan 10, 2021

dntosas force-pushed the cluster-autoscaler-improvements branch from d1db788 to ac11e07 Compare January 10, 2021 22:51

dntosas requested a review from olemarkus January 11, 2021 07:48

olemarkus requested changes Jan 11, 2021

View reviewed changes

dntosas force-pushed the cluster-autoscaler-improvements branch 2 times, most recently from dc0cc6b to 8b7fb57 Compare January 11, 2021 17:53

k8s-ci-robot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Jan 11, 2021

olemarkus reviewed Jan 11, 2021

View reviewed changes

dntosas force-pushed the cluster-autoscaler-improvements branch from 8b7fb57 to 56fe4ba Compare January 11, 2021 18:53

k8s-ci-robot assigned olemarkus Jan 11, 2021

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jan 11, 2021

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jan 11, 2021

k8s-ci-robot merged commit 695be26 into kubernetes:master Jan 11, 2021

k8s-ci-robot added this to the v1.20 milestone Jan 11, 2021

dntosas deleted the cluster-autoscaler-improvements branch January 11, 2021 20:54

dntosas mentioned this pull request Oct 7, 2022

REQUEST: New membership for dntosas kubernetes/org#3753

Closed

9 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[addons/CA] Add support for specifying resources and metrics #10281

[addons/CA] Add support for specifying resources and metrics #10281

dntosas commented Nov 20, 2020 •

edited

Loading

k8s-ci-robot commented Nov 20, 2020

hakman commented Nov 20, 2020

olemarkus Nov 20, 2020

dntosas Nov 20, 2020

olemarkus Dec 6, 2020

olemarkus commented Nov 20, 2020

dntosas commented Nov 20, 2020

olemarkus commented Nov 20, 2020

olemarkus commented Jan 10, 2021

dntosas commented Jan 10, 2021

k8s-ci-robot commented Jan 10, 2021 •

edited

Loading

olemarkus left a comment

olemarkus Jan 11, 2021

dntosas Jan 11, 2021

olemarkus Jan 11, 2021

dntosas Jan 11, 2021

olemarkus Jan 11, 2021

olemarkus commented Jan 11, 2021

k8s-ci-robot commented Jan 11, 2021

[addons/CA] Add support for specifying resources and metrics #10281

[addons/CA] Add support for specifying resources and metrics #10281

Conversation

dntosas commented Nov 20, 2020 • edited Loading

Resources

Metrics

k8s-ci-robot commented Nov 20, 2020

hakman commented Nov 20, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

olemarkus commented Nov 20, 2020

dntosas commented Nov 20, 2020

olemarkus commented Nov 20, 2020

olemarkus commented Jan 10, 2021

dntosas commented Jan 10, 2021

k8s-ci-robot commented Jan 10, 2021 • edited Loading

olemarkus left a 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

olemarkus commented Jan 11, 2021

k8s-ci-robot commented Jan 11, 2021

dntosas commented Nov 20, 2020 •

edited

Loading

k8s-ci-robot commented Jan 10, 2021 •

edited

Loading