-
Notifications
You must be signed in to change notification settings - Fork 3.9k
feat(helm): allow setting labels for ingesters #18536
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
7b56c6e to
854427d
Compare
jkroepke
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi, thanks for your PR. Please use lowercase feat as PR title prefix.
Also take note, that ingest may have rollout-group: {{ include "loki.prefixRolloutGroup" . }}ingester label that feel ready for the rollout operatior.
Please add the new value to the values.yaml as mention below.
| app.kubernetes.io/part-of: memberlist | ||
| name: {{ include "loki.prefixIngesterName" . }}ingester-zone-a | ||
| rollout-group: {{ include "loki.prefixRolloutGroup" . }}ingester | ||
| {{- with .Values.ingester.labels }} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please add labels: {} to the values.yaml and run make helm-docs
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Apologies and added! Thanks for looking
7051581 to
deb963e
Compare
production/helm/loki/CHANGELOG.md
Outdated
| - [BUGFIX] Move loki-sc-rules container from first location in `containers` to second to avoid it being selected as the default for `kubectl logs` or `kubectl exec`. [#17937](https://github.com/grafana/loki/pull/17937) | ||
|
|
||
| - [BUGFIX] Move loki-sc-rules container from first location in `containers` to second to avoid it being selected as the default for `kubectl logs` or `kubectl exec` | ||
| - [FEATURE] Allow setting custom labels for ingester statefulsets [#18536](https://github.com/grafana/loki/pull/18536) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi, it seems you have doublicate the lines here, which seems not correct. ensure that your changes are on top, over the lastest release.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed
jkroepke
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
Co-authored-by: Jan-Otto Kröpke <[email protected]> Signed-off-by: Emmanuel Meinen <[email protected]>
production/helm/loki/CHANGELOG.md
Outdated
| - [BUGFIX] Add release namespace metadata to HorizontalPodAutoscaling that lack it. [#18453](https://github.com/grafana/loki/pull/18453) | ||
| - [BUGFIX] Move loki-sc-rules container from first location in `containers` to second to avoid it being selected as the default for `kubectl logs` or `kubectl exec`. [#17937](https://github.com/grafana/loki/pull/17937) | ||
| - [FEATURE] Added support for chunk-cache-l2 [#17556](https://github.com/grafana/loki/pull/17556) | ||
| - [ENHANCEMENT] Add FOLDER_ANNOTATATION logic for sidecar container. [#13289](https://github.com/grafana/loki/pull/13289) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this is fine, (dup of line 20)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I seem to have caused some hiccups rebasing my fork. Let me know and I can revert this - otherwise I'll leave as-is to keep the review moving.
What this PR does / why we need it:
Ingester statefulsets do not support any sort of custom labels. Services like rollout-operator require a label to allow the service to be notified when it will be scaled down. This PR adds an option for users to specify any labels they may require with no impact to users who do not require them.
Which issue(s) this PR fixes:
none which I could find
Special notes for your reviewer:
None, thank you!
Checklist
CONTRIBUTING.mdguide (required)featPRs are unlikely to be accepted unless a case can be made for the feature actually being a bug fix to existing behavior.docs/sources/setup/upgrade/_index.mddeprecated-config.yamlanddeleted-config.yamlfiles respectively in thetools/deprecated-config-checkerdirectory. Example PR