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: set to not ready when failing to fetch prometheus version #7291

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

SamJamS
Copy link

@SamJamS SamJamS commented Apr 19, 2024

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

Changes

Update ready state in "fetch prometheus version" loop - Currently, if no ready Prometheus behind sidecar then this loops indefinitely and never sets sidecar as not ready. As a result, sidecar reports as ready if it starts up and there is not a running Prometheus behind it.

Verification

  • Added end-to-end test

Signed-off-by: Samuel Scard <10246309+SamJamS@users.noreply.github.com>
Signed-off-by: Samuel Scard <10246309+SamJamS@users.noreply.github.com>

// Start sidecar without ever starting Prometheus.
_, sidecar := e2ethanos.NewPrometheusWithSidecar(e, "alone", e2ethanos.DefaultPromConfig("prom-alone", 0, "", "", e2ethanos.LocalPrometheusTarget), "", e2ethanos.DefaultPrometheusImage(), "")
testutil.Ok(t, sidecar.Start())
Copy link
Author

Choose a reason for hiding this comment

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

maybe use e2e.StartAndWaitReady() here, relying on sidecar's health endpoint as readiness check to make sure we have a running sidecar before testing?

@@ -189,6 +189,8 @@ func runSidecar(
"msg", "failed to fetch prometheus version. Is Prometheus running? Retrying",
"err", err,
)
promUp.Set(0)
statusProber.NotReady(err)
Copy link
Author

Choose a reason for hiding this comment

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

Can update here or default readiness to false at the beginning of runSidecar()

Copy link
Member

@GiedriusS GiedriusS left a comment

Choose a reason for hiding this comment

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

Does #7323 solve your problem?

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.

None yet

2 participants