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

rule: fix reload signal not working #4442

Merged

Conversation

jmichalek132
Copy link
Contributor

@jmichalek132 jmichalek132 commented Jul 13, 2021

Signed-off-by: Juraj Michalek juraj.michalek132@gmail.com

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

Changes

Fixed a bug when sending sighup signal to thanos ruler didn't result in loading changes.

Verification

  1. Started thanos locally using make quickstart.
  2. Modified recording rule in rules.yml created by the quickstart option
  3. Sent sighup to thanos ruler
  4. Looked at thanos ruler ui to check if changes were successfully loaded (they were)
  5. Also added e2e test for it

Signed-off-by: Juraj Michalek <juraj.michalek132@gmail.com>
Signed-off-by: Juraj Michalek <juraj.michalek132@gmail.com>
@GiedriusS
Copy link
Member

Thanks for the PR! 💪 Maybe we could add a test for this? Because this functionality was working, then we broke it, and now this fixes it again. Having a test would make sure that this works. What do you think? 🤔 The seems like a good fit for the e2e tests however you will probably have to improve the e2e framework for Cortex to add a KillSignal(signal int) error interface or something like that.

@jmichalek132
Copy link
Contributor Author

Yeah I was thinking adding the test would make sense too. Should I do it as part of this pr or could it be a follow up pr? Since making changes necessary in cortex might take some time.

I was also thinking exposing api to exec commands inside the containers started by the cortex e2e framework might make a bit more sense since it could be useful in other cases not just this one.

@jmichalek132
Copy link
Contributor Author

jmichalek132 commented Jul 14, 2021

So it seems the e2e services already provides Exec which can be used to send the signal, now I just need to figure out how to check that the reload actually worked.

Signed-off-by: Juraj Michalek <juraj.michalek132@gmail.com>
@jmichalek132
Copy link
Contributor Author

I added the test for reloading using sighup. Currently I am checking that there 3 rule groups (there are 2 before the reload, one is added by the reload). Is the test okay like this?

@jmichalek132
Copy link
Contributor Author

Not sure why the unit tests failed but I don't think it's caused by my change. Could we re-run them?

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.

Just a few nits but overall LGTM 💪 I hope my suggestion is clear here, feel free to ask any questions 😄

test/e2e/rule_test.go Outdated Show resolved Hide resolved
test/e2e/rule_test.go Outdated Show resolved Hide resolved
resp, err := http.DefaultClient.Do(req)
testutil.Ok(t, err)
testutil.Equals(t, 200, resp.StatusCode)
defer resp.Body.Close()
Copy link
Member

Choose a reason for hiding this comment

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

Let's just do: testutil.Ok(t, resp.Body.Close())

Copy link
Member

Choose a reason for hiding this comment

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

@jmichalek132 could you please simply do testutil.Ok(t, resp.Body.Close()) after ioutil.ReadAll()? It will be simpler + you'll get a failure if Close() fails

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Like this?

test/e2e/rule_test.go Outdated Show resolved Hide resolved
Signed-off-by: Juraj Michalek <juraj.michalek132@gmail.com>
Signed-off-by: Juraj Michalek <juraj.michalek132@gmail.com>
GiedriusS
GiedriusS previously approved these changes Jul 19, 2021
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.

Awesome work! Thanks ❤️ could you please do the last small change with regards to the defer resp.Body.Close()?

resp, err := http.DefaultClient.Do(req)
testutil.Ok(t, err)
testutil.Equals(t, 200, resp.StatusCode)
defer resp.Body.Close()
Copy link
Member

Choose a reason for hiding this comment

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

@jmichalek132 could you please simply do testutil.Ok(t, resp.Body.Close()) after ioutil.ReadAll()? It will be simpler + you'll get a failure if Close() fails

Signed-off-by: Juraj Michalek <juraj.michalek132@gmail.com>
test/e2e/rule_test.go Outdated Show resolved Hide resolved
Signed-off-by: Juraj Michalek <juraj.michalek132@gmail.com>
@GiedriusS GiedriusS merged commit e9ef82b into thanos-io:main Jul 19, 2021
GiedriusS pushed a commit to GiedriusS/thanos that referenced this pull request Jul 20, 2021
* rule: fix reload signal not working

Signed-off-by: Juraj Michalek <juraj.michalek132@gmail.com>

* CHANGELOG: added PR

Signed-off-by: Juraj Michalek <juraj.michalek132@gmail.com>

* rule: added test for realoding using sighup signal

Signed-off-by: Juraj Michalek <juraj.michalek132@gmail.com>

* rule: adressed comments for test of realoding using sighup

Signed-off-by: Juraj Michalek <juraj.michalek132@gmail.com>

* ran make format

Signed-off-by: Juraj Michalek <juraj.michalek132@gmail.com>

* addressed comment for test of realoding using sighup

Signed-off-by: Juraj Michalek <juraj.michalek132@gmail.com>

* make linter happy

Signed-off-by: Juraj Michalek <juraj.michalek132@gmail.com>
GiedriusS pushed a commit to GiedriusS/thanos that referenced this pull request Jul 20, 2021
* rule: fix reload signal not working

Signed-off-by: Juraj Michalek <juraj.michalek132@gmail.com>

* CHANGELOG: added PR

Signed-off-by: Juraj Michalek <juraj.michalek132@gmail.com>

* rule: added test for realoding using sighup signal

Signed-off-by: Juraj Michalek <juraj.michalek132@gmail.com>

* rule: adressed comments for test of realoding using sighup

Signed-off-by: Juraj Michalek <juraj.michalek132@gmail.com>

* ran make format

Signed-off-by: Juraj Michalek <juraj.michalek132@gmail.com>

* addressed comment for test of realoding using sighup

Signed-off-by: Juraj Michalek <juraj.michalek132@gmail.com>

* make linter happy

Signed-off-by: Juraj Michalek <juraj.michalek132@gmail.com>
@GiedriusS GiedriusS mentioned this pull request Jul 20, 2021
GiedriusS pushed a commit to GiedriusS/thanos that referenced this pull request Jul 20, 2021
* rule: fix reload signal not working

Signed-off-by: Juraj Michalek <juraj.michalek132@gmail.com>

* CHANGELOG: added PR

Signed-off-by: Juraj Michalek <juraj.michalek132@gmail.com>

* rule: added test for realoding using sighup signal

Signed-off-by: Juraj Michalek <juraj.michalek132@gmail.com>

* rule: adressed comments for test of realoding using sighup

Signed-off-by: Juraj Michalek <juraj.michalek132@gmail.com>

* ran make format

Signed-off-by: Juraj Michalek <juraj.michalek132@gmail.com>

* addressed comment for test of realoding using sighup

Signed-off-by: Juraj Michalek <juraj.michalek132@gmail.com>

* make linter happy

Signed-off-by: Juraj Michalek <juraj.michalek132@gmail.com>

Signed-off-by: Giedrius Statkevičius <giedrius.statkevicius@vinted.com>
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.

2 participants