-
Notifications
You must be signed in to change notification settings - Fork 3.8k
fix(helm): Use correct serviceName in zone-aware ingester statefulsets #18558
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
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, just one nit.
apologies for missing that and thank you for reviewing! |
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]>
cce47c2 to
b125910
Compare
2585c2c to
b61ac2d
Compare
|
@jkroepke I was introducing some conflicts while rebasing to update my fork. I'll stop doing that and I have addressed new changelog issues. Apologies for the noise on this PR. |
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.
Thank you!
LGTM
…tefulsets (grafana#18558)" This reverts commit 2706302.
|
For record: this PR breaks existing setups:
We are currently discuss, how we handle this. (extensive note in the CHANGELOG OR revert of the PR) |
|
We pick-up latest chart 6.34.0 and got that same bug. Is it possible to provide us with a fix in 6.35.0 since we need to use some feature in latest chart. |
|
@abenbachir Quick Workaround is delete the Statefulset manually before running helm upgrade. |
|
Hey all, so we have two options here, and we are happy to take a vote on which you would prefer: 👎 - We move to removing this PR which fixes the serviceName in the zone-aware ingesters and consider fixing it when we move to Loki 4.0 helm chart in the future. |
|
I can do the workaround, but how do we know you guys won't rollback this change in next release and we will be doing another workaround. Also can you provide excat kubctl commands to delete the ingesters, we would like to try it in our staging env. |
|
The mentioned workaround is now part of the official upgrading documentation: https://grafana.com/docs/loki/next/setup/upgrade/upgrade-to-6x/#breaking-zone-aware-ingester-statefulset-servicename-fix-6340 |
What this PR does / why we need it:
When running with zone-aware ingesters, the chart creates three services for
loki-testing-ingester-zone-<zone a-c>-headlessbut when rollout-operator attempts to call/ingester/prepare-downscaleit getsserviceName: loki-testing-ingester-zone-afrom the statefulset. This does not have the -headless suffix and the call fails. This causes the ingesters to fail to scale down properly.Which issue(s) this PR fixes:
Fixes #18174
Special notes for your reviewer:
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