[helm-chart] add values for setting annotations and labels for rollout-operator #6733

m1rp · 2023-11-24T14:17:39Z

What this PR does

add labels and annotations used by the rollout-operator.

Which issue(s) this PR fixes or relates to

Fixes #6650
Also very related to This PR over in the rollout-operator repo

Checklist

Tests updated.
Documentation added.
CHANGELOG.md updated - the order of entries should be [CHANGE], [FEATURE], [ENHANCEMENT], [BUGFIX].
about-versioning.md updated with experimental features.

Extra

i'd love some feedback on this, because not all possible annotation/labels have been added nor are they configurable so some input would be appreciated if this is something worth investing more time into.

CLAassistant · 2023-11-24T14:17:46Z

All committers have signed the CLA.

m1rp · 2023-11-24T14:26:44Z

@56quarters tagging you here as you have some context for these changes, as you're also tagged here and i think it makes sense for these things to be aligned.

56quarters

Thank you for this!

56quarters · 2023-11-24T17:23:37Z

operations/helm/charts/mimir-distributed/templates/alertmanager/alertmanager-statefulset.yaml

@@ -53,11 +53,20 @@ spec:
    metadata:
      labels:
        {{- include "mimir.podLabels" $args | nindent 8 }}
+        grafana.com/no-downscale: {{$rolloutZone.noDownscale}}


I don't think this is needed for the alertmanager, I'm pretty sure they're safe to scale down at any time. Could one of the alerting folks confirm? @stevesg et al

this might also be the case for the store-gateway right?
the resource that needs extra safety when scaling down is mainly the ingester if i'm not mistaken

@56quarters @stevesg There are 3 components called out in the current Scaling Out documentation as having limitations for scaling down:

Alertmanagers

Ingesters

Store-gateways

That being said, do you still feel like this section should be removed, or is it safe to leave in considering an inadvertent downscale could lead to downtime?

56quarters · 2023-11-24T17:24:52Z

operations/helm/charts/mimir-distributed/templates/ingester/ingester-statefulset.yaml

@@ -52,8 +52,17 @@ spec:
    metadata:
      labels:
        {{- include "mimir.podLabels" $args | nindent 8 }}
+        grafana.com/no-downscale: {{$rolloutZone.noDownscale}}


These two annotations are mutually exclusive:

A statefulset can either be "no-downscale" in which case, it's not allowed to be scaled down by anything.

Or it can be scaled down by the rollout-operator

does it make sense that the no-downscale is the deciding factor here?
by that i mean that the condition for rendering the prepare-scaledown is that it's value is true and that no-downscaleis false?

@dimitarvdimitrov has suggested that no downscale be the default behavior which slightly simplifies this

sorry, I was interfering. I think having the prepare-scaledown and graceful scaledown procedure by the rollout operator are easier to reason about from an operator's POV - you just scale the StatfulSets as you would normally and the rollout-operator sorts out preparations and wait times.

But I suspect Nick's point was that setting both doesn't make sense. So it would be useful if we add some validations that they both cannot be set. Perhaps you can add a named template (helm function) to this file which checks the values of mimir.podLabels and mimir.podAnnotations and fails the chart rendering when both are set.

oh i see, i didn't know about this possibility!

sorry I didn't make this clear - I think it's better if we have the prepare-downscale and the downscale leader on by default. This would remove some toil from operators as opposed to only preventing accidental downscaling.

Including suggested changes in next PR

56quarters · 2023-11-24T17:26:17Z

operations/helm/charts/mimir-distributed/templates/ingester/ingester-statefulset.yaml

+        grafana.com/prepare-downscale-http-port: 80
+        {{- end-}}
+        {{- if $rolloutZone.downscaleLeader }}
+        grafana.com/rollout-downscale-leader: downscaleLeader


The value of this should be the name of the leader statefulset. For example, the leader of the zone B statefulset should be mimir-ingester-zone-a.

my mistake, i meant to add this as a variable.

This was already fixed:

grafana.com/rollout-downscale-leader: $rolloutZone.downscaleLeader

56quarters · 2023-11-24T17:26:52Z

operations/helm/charts/mimir-distributed/templates/ingester/ingester-statefulset.yaml

      annotations:
        {{- include "mimir.podAnnotations" $args | nindent 8 }}
+        {{- if $rolloutZone.prepareDownscale }}
+        grafana.com/prepare-downscale-http-path: ingester/prepare-shutdown
+        grafana.com/prepare-downscale-http-port: 80


I don't remember if the default HTTP port for Mimir is changed in the helm chart. This might be 8080 these days.

It's dynamic based on {{ include "mimir.serverHttpListenPort" . }}. It's set in the same file. I think we should use the same template here

This change has already been incorporated in the current version of this PR

grafana.com/prepare-downscale-http-port: {{ include "mimir.serverHttpListenPort" . }}

dimitarvdimitrov

thanks for the contribution! Can you also add a changelog entry in operations/helm/charts/mimir-distributed/CHANGELOG.md

dimitarvdimitrov · 2023-11-27T19:42:29Z

operations/helm/charts/mimir-distributed/values.yaml

@@ -950,6 +974,14 @@ ingester:
      # If set to "-", then use `storageClassName: ""`, which disables dynamic provisioning
      # If undefined or set to null (the default), then fall back to the value of `ingester.persistentVolume.storageClass`.
      storageClass: null
+      # -- noDownscale adds a label that can be used by the rollout-operator as documented in the rollout-operator repo: https://github.com/grafana/rollout-operator#webhooks
+      noDownscale: false


should we enable this by default in all ingester zones? The idea behind this annotation was to prevent accidental downscaling

In another comment above:

sorry I didn't make this clear - I think it's better if we have the prepare-downscale and the downscale leader on by default. This would remove some toil from operators as opposed to only preventing accidental downscaling.

Since noDownscale and prepareDownscale should not be enabled at the same time, which one should take precedence and be set to true in the values.yaml file by default? My understanding was that the preferred sequence is:

noDownscale: false prepareDownscale: true

operations/helm/charts/mimir-distributed/templates/store-gateway/store-gateway-statefulset.yaml

dimitarvdimitrov · 2023-11-27T19:44:53Z

operations/helm/charts/mimir-distributed/templates/ingester/ingester-statefulset.yaml

      annotations:
        {{- include "mimir.podAnnotations" $args | nindent 8 }}
+        {{- if $rolloutZone.prepareDownscale }}
+        grafana.com/prepare-downscale-http-path: ingester/prepare-shutdown
+        grafana.com/prepare-downscale-http-port: 80


It's dynamic based on {{ include "mimir.serverHttpListenPort" . }}. It's set in the same file. I think we should use the same template here

operations/helm/charts/mimir-distributed/values.yaml

…t-operator Signed-off-by: sam clulow <[email protected]>

- default `no-downscale` to true - downscale labels are now conditionally rendered - fix path and port

Signed-off-by: sam clulow <[email protected]>

dimitarvdimitrov · 2024-01-09T11:52:11Z

operations/helm/charts/mimir-distributed/templates/_helpers.tpl

+  "noDownscale":  $rolloutZone.noDownscale
+  "downscaleLeader": $rolloutZone.downscaleLeader
+  "prepareDownscale": $rolloutZone.prepareDownscale


i think the linter is complaining about the colons here

Colons no longer present in current version

dimitarvdimitrov · 2024-03-25T13:12:42Z

hey, there's something off with the commits in this PR. I see quite a lot of changes and number of commits. Can you take another look please @m1rp?

Signed-off-by: sam clulow <[email protected]>

m1rp · 2024-03-25T13:32:43Z

apologies, i managed to spectacularly mess up my git history/commit! should be sorted now.

dimitarvdimitrov · 2024-04-08T21:12:36Z

really sorry for the delay 🙈 I was away the week before and swiped away all GH notifications

I can try to take a look later this week after GrafanaCon. @56quarters can you give this one more look?

armandgrillet · 2024-12-18T14:40:02Z

@m1rp Thank you for the contribution, can you please do the changes mentioned in the comments? Otherwise, we will close this PR.

chencs · 2024-12-20T23:10:12Z

The CHANGELOG has just been cut to prepare for the next release. Please rebase main and eventually move the CHANGELOG entry added / updated in this PR to the top of the operations/helm/charts/mimir-distributed/CHANGELOG.md document. Thanks!

seanankenbruck · 2025-02-18T14:40:44Z

@armandgrillet @chencs Are the comments and required rebase the only items outstanding for this PR? These code changes would provide some much needed downscaling features to the helm chart so it would be great to keep it open.

…t-operator Signed-off-by: sam clulow <[email protected]>

- default `no-downscale` to true - downscale labels are now conditionally rendered - fix path and port

Signed-off-by: sam clulow <[email protected]>

Signed-off-by: Dimitar Dimitrov <[email protected]>

… into rollout-operator-config

Rebase main to rollout operator config

dimitarvdimitrov · 2025-02-24T19:04:30Z

armandgrillet chencs Are the comments and required rebase the only items outstanding for this PR? These code changes would provide some much needed downscaling features to the helm chart so it would be great to keep it open.

@seanankenbruck yes, i believe these are the only remaining things.

…fana#6733 (comment)

…e not used together

Additional changes to resolve outstanding comments

seanankenbruck · 2025-03-04T14:14:11Z

@dimitarvdimitrov all outstanding comments have been addressed and this PR should be ready for code owner review.

m1rp force-pushed the rollout-operator-config branch 2 times, most recently from 7f9e087 to 840edc0 Compare November 24, 2023 14:58

56quarters reviewed Nov 24, 2023

View reviewed changes

dimitarvdimitrov reviewed Nov 27, 2023

View reviewed changes

m1rp force-pushed the rollout-operator-config branch from 840edc0 to 446ac64 Compare December 4, 2023 14:33

dimitarvdimitrov reviewed Dec 7, 2023

View reviewed changes

operations/helm/charts/mimir-distributed/values.yaml Outdated Show resolved Hide resolved

m1rp added 3 commits January 2, 2024 14:53

[helm-chart] add values for setting annotations and labels for rollou…

28f8ec9

…t-operator Signed-off-by: sam clulow <[email protected]>

implement feedback

f4705ac

- default `no-downscale` to true - downscale labels are now conditionally rendered - fix path and port

update changelog

6ff0205

Signed-off-by: sam clulow <[email protected]>

m1rp force-pushed the rollout-operator-config branch from a844db4 to 6ff0205 Compare January 2, 2024 13:57

m1rp marked this pull request as ready for review January 4, 2024 13:42

m1rp requested a review from a team as a code owner January 4, 2024 13:42

dimitarvdimitrov reviewed Jan 9, 2024

View reviewed changes

m1rp requested review from grafanabot, a team, wilfriedroset, vaxvms, bubu11e and jdbaldry as code owners March 25, 2024 12:46

fix more linting issues

2df4381

Signed-off-by: sam clulow <[email protected]>

m1rp force-pushed the rollout-operator-config branch from 4b4a6c4 to 2df4381 Compare March 25, 2024 13:14

dimitarvdimitrov mentioned this pull request Mar 27, 2024

Support rollout prepare-downscale in helm chart #7726

Open

dimitarvdimitrov mentioned this pull request Apr 8, 2024

Helm : add hpa for the ingesters #7843

Closed

4 tasks

Merge branch 'main' into rollout-operator-config

ebef306

implement feedback

81dbb5a

dimitarvdimitrov added the helm label Jun 21, 2024

Merge branch 'main' into rollout-operator-config

ab3a28a

zzehring removed the request for review from grafanabot October 1, 2024 15:10

zerok removed the request for review from a team October 2, 2024 09:45

chencs added the release/notified-changelog-cut label Dec 20, 2024

m1rp and others added 9 commits February 18, 2025 15:21

[helm-chart] add values for setting annotations and labels for rollou…

5656c5b

…t-operator Signed-off-by: sam clulow <[email protected]>

implement feedback

d95fbd6

- default `no-downscale` to true - downscale labels are now conditionally rendered - fix path and port

update changelog

eb3654d

Signed-off-by: sam clulow <[email protected]>

fix more linting issues

e5e5660

Signed-off-by: sam clulow <[email protected]>

Move changelog entry

b3da90a

Signed-off-by: Dimitar Dimitrov <[email protected]>

Fix comment spacing

852e7a9

Signed-off-by: Dimitar Dimitrov <[email protected]>

implement feedback

dc56ee9

Merge branch 'rollout-operator-config' of https://github.com/m1rp/mimir…

c8df137

… into rollout-operator-config

moved changelog ref to top of file

339d914

seanankenbruck mentioned this pull request Feb 18, 2025

Rebase main to rollout operator config m1rp/mimir#2

Merged

4 tasks

Merge pull request #2 from seanankenbruck/rollout-operator-config

096dc67

Rebase main to rollout operator config

dimitarvdimitrov self-assigned this Feb 24, 2025

seanankenbruck added 3 commits March 3, 2025 15:53

set prepare-downscale and downscale leader by default per comment gra…

79653ae

…fana#6733 (comment)

added a validation step to verify noDownscale and prepareDownscale ar…

8c98d28

…e not used together

fix downscaleLeader reference

42ceae2

seanankenbruck mentioned this pull request Mar 3, 2025

Additional changes to resolve outstanding comments m1rp/mimir#3

Merged

4 tasks

seanankenbruck and others added 3 commits March 3, 2025 17:13

rebuilt helm tests with 'make build-helm-tests'

4c534cd

clean white noise

1ac7705

Merge pull request #3 from seanankenbruck/rollout-operator-config

d415a93

Additional changes to resolve outstanding comments

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[helm-chart] add values for setting annotations and labels for rollout-operator #6733

[helm-chart] add values for setting annotations and labels for rollout-operator #6733

m1rp commented Nov 24, 2023 •

edited

Loading

CLAassistant commented Nov 24, 2023 •

edited

Loading

m1rp commented Nov 24, 2023

56quarters left a comment

56quarters Nov 24, 2023

m1rp Dec 4, 2023

seanankenbruck Mar 3, 2025

56quarters Nov 24, 2023

m1rp Dec 4, 2023

m1rp Dec 4, 2023

dimitarvdimitrov Dec 7, 2023

m1rp Jan 2, 2024

dimitarvdimitrov Jan 9, 2024

seanankenbruck Mar 3, 2025

56quarters Nov 24, 2023

m1rp Dec 4, 2023

seanankenbruck Mar 3, 2025

56quarters Nov 24, 2023

dimitarvdimitrov Nov 27, 2023

seanankenbruck Mar 3, 2025

dimitarvdimitrov left a comment

dimitarvdimitrov Nov 27, 2023

seanankenbruck Mar 3, 2025

dimitarvdimitrov Nov 27, 2023

dimitarvdimitrov Jan 9, 2024

seanankenbruck Mar 4, 2025

dimitarvdimitrov commented Mar 25, 2024

m1rp commented Mar 25, 2024 •

edited

Loading

dimitarvdimitrov commented Apr 8, 2024

armandgrillet commented Dec 18, 2024

chencs commented Dec 20, 2024

seanankenbruck commented Feb 18, 2025

dimitarvdimitrov commented Feb 24, 2025

seanankenbruck commented Mar 4, 2025

[helm-chart] add values for setting annotations and labels for rollout-operator #6733

Are you sure you want to change the base?

[helm-chart] add values for setting annotations and labels for rollout-operator #6733

Conversation

m1rp commented Nov 24, 2023 • edited Loading

What this PR does

Which issue(s) this PR fixes or relates to

Checklist

Extra

CLAassistant commented Nov 24, 2023 • edited Loading

m1rp commented Nov 24, 2023

56quarters 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

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

dimitarvdimitrov 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

dimitarvdimitrov commented Mar 25, 2024

m1rp commented Mar 25, 2024 • edited Loading

dimitarvdimitrov commented Apr 8, 2024

armandgrillet commented Dec 18, 2024

chencs commented Dec 20, 2024

seanankenbruck commented Feb 18, 2025

dimitarvdimitrov commented Feb 24, 2025

seanankenbruck commented Mar 4, 2025

m1rp commented Nov 24, 2023 •

edited

Loading

CLAassistant commented Nov 24, 2023 •

edited

Loading

m1rp commented Mar 25, 2024 •

edited

Loading