Skip to content
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

Store: make the liveness probe timeout configurable #286

Merged
merged 3 commits into from
Oct 11, 2022

Conversation

douglascamata
Copy link
Contributor

@douglascamata douglascamata commented Oct 10, 2022

Signed-off-by: Douglas Camata 159076+douglascamata@users.noreply.github.com

  • I added CHANGELOG entry for this change.
  • Change is not relevant to the end user.

Changes

Under heavy load, the 1s default timeout for the Store liveness probe can be often triggered, which leads to Store restarts. This PR makes such timeout configurable.

The default value is 1s, to keep the current behavior, and in affected environments one can increase it.

Verification

Under heavy load, the 1s default timeout for the Store liveness probe
can be often triggered, which leads to Store restarts.

The default value is 1s, to keep the current behavior, and in affected
environments one can increase it.

Signed-off-by: Douglas Camata <159076+douglascamata@users.noreply.github.com>
Signed-off-by: Douglas Camata <159076+douglascamata@users.noreply.github.com>
Copy link
Contributor

@philipgough philipgough left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

makes sense and lgtm

scheme: 'HTTP',
port: ts.config.ports.http,
path: '/-/healthy',
},
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Umm, maybe we can just make the whole livenessProbe configurable and use ts.config. livenessProbe? In case we want to expose other fields in the future.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was torn between making only exactly what I need configurable vs making the whole thing configurable. As a poorly configured liveness probe can cause substantial problems I refrained from making it completely configurable.

I could make the failureThreshold and periodSeconds configurable in addition to the timeoutSeconds. What do you think, @yeya24?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I feel it is fine to expose the field completely. Since we leave the defaults here so users can take their own risk to change to what they need.
But exposing failureThreshold, periodSeconds and timeoutSeconds is also a way to go. Let's expose these fields for now.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done it in 449e423. 👍

Adds configuration for failure threshold and period seconds.

Signed-off-by: Douglas Camata <159076+douglascamata@users.noreply.github.com>
@yeya24 yeya24 merged commit 5c8b734 into thanos-io:main Oct 11, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants