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
Merged
Show file tree
Hide file tree
Changes from 3 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,8 @@ We use *breaking :warning:* to mark changes that are not backward compatible (re

### Fixed

- [#4442](https://github.com/thanos-io/thanos/pull/4442) rule: fix reload signal not working

### Changed

## [v0.22.0 - in progress](https://github.com/thanos-io/thanos/tree/release-0.22)
Expand Down
5 changes: 3 additions & 2 deletions cmd/thanos/rule.go
Original file line number Diff line number Diff line change
Expand Up @@ -80,7 +80,6 @@ type ruleConfig struct {
ruleFiles []string
objStoreConfig *extflag.PathOrContent
dataDir string
reloadSignal <-chan struct{}
lset labels.Labels
}

Expand Down Expand Up @@ -195,6 +194,7 @@ func registerRule(app *extkingpin.App) {
tracer,
comp,
*conf,
reload,
getFlagsMap(cmd.Flags()),
httpLogOpts,
grpcLogOpts,
Expand Down Expand Up @@ -257,6 +257,7 @@ func runRule(
tracer opentracing.Tracer,
comp component.Component,
conf ruleConfig,
reloadSignal <-chan struct{},
flagsMap map[string]string,
httpLogOpts []logging.Option,
grpcLogOpts []grpc_logging.Option,
Expand Down Expand Up @@ -483,7 +484,7 @@ func runRule(
}
for {
select {
case <-conf.reloadSignal:
case <-reloadSignal:
if err := reloadRules(logger, conf.ruleFiles, ruleMgr, conf.evalInterval, metrics); err != nil {
level.Error(logger).Log("msg", "reload rules by sighup failed", "err", err)
}
Expand Down
43 changes: 38 additions & 5 deletions test/e2e/rule_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -58,7 +58,7 @@ groups:
annotations:
summary: "I always complain and allow partial response in query."
`
testAlertRuleAddedLaterWebHandler = `
testAlertRuleAddedLater = `
groups:
- name: example
interval: 1s
Expand Down Expand Up @@ -97,6 +97,34 @@ func reloadRulesHTTP(t *testing.T, ctx context.Context, endpoint string) {
testutil.Equals(t, 200, resp.StatusCode)
}

func reloadRulesSignal(t *testing.T, r *e2ethanos.Service) {
c := e2e.NewCommand("kill", "-1", "1")
_, _, err := r.Exec(c)
testutil.Ok(t, err)
}

func checkReloadSuccessful(t *testing.T, ctx context.Context, endpoint string) {
req, err := http.NewRequestWithContext(ctx, "GET", "http://"+endpoint+"/api/v1/rules", ioutil.NopCloser(bytes.NewReader(nil)))
testutil.Ok(t, err)
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?


body, err := ioutil.ReadAll(resp.Body)
jmichalek132 marked this conversation as resolved.
Show resolved Hide resolved
testutil.Ok(t, err)

var data struct {
GiedriusS marked this conversation as resolved.
Show resolved Hide resolved
Status string
Data *rulespb.RuleGroups
}

testutil.Ok(t, json.Unmarshal(body, &data))
testutil.Equals(t, "success", data.Status)

testutil.Assert(t, len(data.Data.Groups) == 3, "expected there to be 3 rule groups")
GiedriusS marked this conversation as resolved.
Show resolved Hide resolved
}

func rulegroupCorrectData(t *testing.T, ctx context.Context, endpoint string) {
req, err := http.NewRequestWithContext(ctx, "GET", "http://"+endpoint+"/api/v1/rules", ioutil.NopCloser(bytes.NewReader(nil)))
testutil.Ok(t, err)
Expand Down Expand Up @@ -317,11 +345,16 @@ func TestRule(t *testing.T) {
rulegroupCorrectData(t, ctx, r.HTTPEndpoint())
})

t.Run("reload works", func(t *testing.T) {
// Add a new rule via /-/reload.
// TODO(GiedriusS): add a test for reloading via SIGHUP. Need to extend e2e framework to expose PIDs.
t.Run("signal reload works", func(t *testing.T) {
// Add a new rule via sending sighup
createRuleFile(t, fmt.Sprintf("%s/newrule.yaml", rulesPath), testAlertRuleAddedLater)
reloadRulesSignal(t, r)
checkReloadSuccessful(t, ctx, r.HTTPEndpoint())
})

createRuleFile(t, fmt.Sprintf("%s/newrule.yaml", rulesPath), testAlertRuleAddedLaterWebHandler)
t.Run("http reload works", func(t *testing.T) {
// Add a new rule via /-/reload.
createRuleFile(t, fmt.Sprintf("%s/newrule.yaml", rulesPath), testAlertRuleAddedLater)
GiedriusS marked this conversation as resolved.
Show resolved Hide resolved
reloadRulesHTTP(t, ctx, r.HTTPEndpoint())
})

Expand Down