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

sidecar: not ready when Prometheus is unavailable #4939

Merged
merged 1 commit into from
Dec 15, 2021

Conversation

GiedriusS
Copy link
Member

@GiedriusS GiedriusS commented Dec 9, 2021

Changes

Set Sidecar to not be ready when Prometheus is unavailable. This avoids
problems when Prometheus is replaying WAL and Sidecar is "up" but then
Query hangs while waiting for a response from Prometheus/Sidecar. Also,
it gets shown as UP in the Stores page with no ext. labels during this
time that is confusing.

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

Verification

Improved e2e tests which pass.

Set Sidecar to not be ready when Prometheus is unavailable. This avoids
problems when Prometheus is replaying WAL and Sidecar is "up" but then
Query hangs while waiting for a response from Prometheus/Sidecar. Also,
it gets shown as UP in the Stores page with no ext. labels during this
time that is confusing.

Signed-off-by: Giedrius Statkevičius <[email protected]>
Copy link
Contributor

@Namanl2001 Namanl2001 left a comment

Choose a reason for hiding this comment

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

just a comment suggestion, otherwise LGTM. Thanks

defer runutil.CloseWithErrCapture(&rerr, resp.Body, "closing resp body")

if resp.StatusCode == 200 {
return fmt.Errorf("got status code %d", resp.StatusCode)
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
return fmt.Errorf("got status code %d", resp.StatusCode)
return fmt.Errorf("sidecar should not be ready, got HTTP status code %d OK", resp.StatusCode)

Copy link
Contributor

Choose a reason for hiding this comment

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

Why OK?

Copy link
Member Author

Choose a reason for hiding this comment

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

Because that's what usually goes after 200 in the HTTP response https://developer.mozilla.org/en-US/docs/Web/HTTP/Status/200? (:

Copy link
Contributor

Choose a reason for hiding this comment

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

Because that's what usually goes after 200 in the HTTP response https://developer.mozilla.org/en-US/docs/Web/HTTP/Status/200? (:

yes, in case someone doesn't know what status code 200 signifies.

Copy link
Contributor

Choose a reason for hiding this comment

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

Printing the status code and OK the same time seems redundant.

Copy link
Member Author

Choose a reason for hiding this comment

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

So, can we merge this then @yeya24 ?

Copy link
Contributor

@yeya24 yeya24 left a comment

Choose a reason for hiding this comment

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

LGTM

@yeya24 yeya24 merged commit 961a7dd into thanos-io:main Dec 15, 2021
@GiedriusS GiedriusS deleted the set_not_ready_when_err branch December 15, 2021 09:13
philipgough pushed a commit to philipgough/thanos that referenced this pull request Dec 15, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants