-
Notifications
You must be signed in to change notification settings - Fork 3.8k
fix(helm): Add startup probe read #19708
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
base: main
Are you sure you want to change the base?
fix(helm): Add startup probe read #19708
Conversation
…t if unset or null, does not provoke any errors
…t if unset or null, does not provoke any errors
…ngerFromTheAbyss/loki into fix-read-liveness-probe
…iness instead of liveness
Signed-off-by: someStrangerFromTheAbyss <151858007+someStrangerFromTheAbyss@users.noreply.github.com>
someStrangerFromTheAbyss
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.
Default value of values.yaml file can be changed. Just want to make sure the issue no longer appears
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.
please see my comments.
…gerFromTheAbyss/loki into add-startup-probe-read
|
Tests failing because of the startup probe: |
Hummm 401. Probably some authentication....It was bold of me to assume that there would be no authentication on read pods. Ill remove it from the default values, but comment the startupProbe code with a link to the issue the default values.yaml. That way, if futur user have the issue, they can just uncomment the comment block. Will do the commit in a couple of minutes. |
|
@someStrangerFromTheAbyss The problem is different. I guess the proposed solution works only, if https://grafana.com/docs/loki/latest/operations/multi-tenancy/ is not enabled. In that case, the header X-Scope-OrgID is missing. Maybe the canary tenant could be re-used. Just document the solution wont help either. Maybe another loki Rest call would fit better. And is this needed on the |
Hummm... i'll see what i can do.
I never deployed in Microservices mode, so i don't know. From the linked issue, this has been reported only in Simple scalable mode. |
|
httpProbe supports header, maybe |
…hen multi-tenancy is not enabled
|
Hi, I can confirm, it works on my test setup. However, I would not set this as default. having a 60 second delay feels unnecessary for most users. Instead I would recommend to document that, similar how istio is documented /cc @JStickler |
What this PR does / why we need it:
Which issue(s) this PR fixes:
Fixes [#15191]
Special notes for your reviewer:
Check the default value in the value file for the helm chart
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