pkg/rule: Fix bugs where rules were out of sync. #1439

johncming · 2019-08-20T02:52:11Z

Thanos rule loading alerts about out-of-sync rule files

Specifically, the following scenario:

Rule file rule. Yaml, which contains some rules, through the reload command, thanos rule load rules.

If you clear the contents of rule. Yaml, but leave the file in, the old rules in thanos rule will not be cleared.

This bug leads to a common situation (all rules in one file)

If thanos rule has 100 rules, it can only clear 99 rules at most.

Closes #1739

bwplotka

Nice! I think that makes sense but have one suggestion

Thanks for helping 👍

On top of the suggestion below I think we really need more unit tests to:

Detect those bug early
Do not repeat same bug anymore
Confirm fix works (:

Do you think we can add some tests?

bwplotka · 2019-08-22T09:40:18Z

pkg/rule/rule.go

@@ -113,7 +113,10 @@ func (r RuleGroup) MarshalYAML() (interface{}, error) {
 func (m *Managers) Update(dataDir string, evalInterval time.Duration, files []string) error {
 	var (
 		errs     = tsdberrors.MultiError{}
-		filesMap = map[storepb.PartialResponseStrategy][]string{}
+		filesMap = map[storepb.PartialResponseStrategy][]string{


Hm I think that will work, but to make it verbose, clearer and bit more efficient I would suggest add one more loop at the end:

for s, updater := range *m { if _, ok := filesMap[s]; ok { // We already updated those above. continue } // Rules removed update updater if needed. if len(updater.RuleGroups()) == 0 { continue } if err := updater.Update(evalInterval, []string{}, nil); err != nil { errs = append(errs, err) } }

What do you think? (:

it 's more clear. how about merge the last loop with your loop?

for s, updater := range *m { fs := filesMap[s] if err := updater.Update(evalInterval, fs, nil); err != nil { errs = append(errs, err) continue } }

if ok, i will update the pr.

I will add a testcase for this.

johncming · 2019-08-28T01:10:41Z

Nice! I think that makes sense but have one suggestion

Thanks for helping 👍

On top of the suggestion below I think we really need more unit tests to:

Detect those bug early

Do not repeat same bug anymore

Confirm fix works (:

Do you think we can add some tests?

Yes, it is necessary to add test cases.

GiedriusS · 2019-11-16T19:15:19Z

@johncming do you have any free time to finish up this PR? :p

simonpasquier · 2019-12-19T09:32:43Z

@johncming do you have time to rebase?

johncming · 2019-12-20T06:10:34Z

@johncming do you have time to rebase?

fixed.

simonpasquier

LGTM

simonpasquier · 2020-01-06T15:41:27Z

pkg/rule/rule_test.go

+	err = m.Update(1*time.Second, []string{
+		filepath.Join(dir, "no_strategy.yaml"),
+	})
+


(nit) remove blank line

simonpasquier · 2020-01-06T15:51:44Z

pkg/rule/rule_test.go

+	dir, err := ioutil.TempDir("", "test_rule_rule_groups")
+	testutil.Ok(t, err)
+	defer func() { testutil.Ok(t, os.RemoveAll(dir)) }()
+	err = os.MkdirAll(filepath.Join(dir, "subdir"), 0775)


this line and the next one aren't needed AFAICT.

simonpasquier · 2020-01-06T15:57:24Z

I think you'll need to update the CHANGELOG and reference #1739 too.

Signed-off-by: johncming <johncming@yahoo.com>

johncming · 2020-01-07T02:37:09Z

I think you'll need to update the CHANGELOG and reference #1739 too.

Updated.

When I read the CHANGELOG, there was no place to put this reference #1739. Where should I put this reference #1739? Or I reference it in the pr conversations?

Thanks.

simonpasquier · 2020-01-07T09:49:25Z

When I read the CHANGELOG, there was no place to put this reference #1739. Where should I put this reference #1739? Or I reference it in the pr conversations?

Yes I meant the latter (adding to Closes #1739 to the PR description).

johncming · 2020-01-08T01:16:53Z

When I read the CHANGELOG, there was no place to put this reference #1739. Where should I put this reference #1739? Or I reference it in the pr conversations?

Yes, I meant the latter (adding to Closes #1739 to the PR description).

Yes, it is updated.

daixiang0 · 2020-01-13T02:42:11Z

@johncming can you rebase?

johncming · 2020-01-15T03:12:28Z

@johncming can you rebase?

rebased.

simonpasquier · 2020-01-15T07:42:51Z

CHANGELOG.md

@@ -44,6 +44,7 @@ Compactor now properly handles partial block uploads for all operation like rete
 - [#1882](https://github.com/thanos-io/thanos/pull/1882) Receive: upload to object storage as 'receive' rather than 'sidecar'.
 - [#1907](https://github.com/thanos-io/thanos/pull/1907) Store: Fixed the duration unit for the metric `thanos_bucket_store_series_gate_duration_seconds`.
 - [#1931](https://github.com/thanos-io/thanos/pull/1931) Compact: Fixed the compactor successfully exiting when actually an error occurred while compacting a blocks group.
+- [#1439](https://github.com/thanos-io/thanos/pull/1439) Rule: Fix bugs where rules were out of sync.


It should go in the Unreleased section.

simonpasquier · 2020-01-15T08:00:20Z

pkg/rule/rule.go

@@ -213,6 +213,21 @@ func (m *Manager) Update(evalInterval time.Duration, files []string) error {
 			continue
 		}
 	}
+
+	for s, mgr := range m.mgrs {


it might be worth adding a comment explaining that this section removes the rules from a manager when a strategy has no more rule.

bwplotka

Thanks! Sorry for lag in review. Let's fix @simonpasquier comments and merge 👍

LGTM after fixes.

GiedriusS · 2020-01-31T22:58:53Z

@johncming thanks for your persistence! Care to fix the code according to the last comments so that we could merge it and fix it?

stale · 2020-03-01T23:32:34Z

This issue/PR has been automatically marked as stale because it has not had recent activity. Please comment on status otherwise the issue will be closed in a week. Thank you for your contributions.

lilic · 2020-04-02T15:12:46Z

Any reason why this PR was never merged, was this issue fixed by another PR? Thanks! @johncming @bwplotka

simonpasquier · 2020-04-02T15:22:29Z

@lilic it has been closed by the bot because it has seen no activity in a while. @johncming would you be able to reopen it and address the comments?

lilic · 2020-04-02T15:34:36Z

@johncming if you don't have time to address the comments for any reason, I can also takeover and pick up this PR from where you leftover with your commits of course? Thanks!

johncming changed the title ~~pkg/rule: Fixed bugs where rules were out of sync.~~ pkg/rule: Fix bugs where rules were out of sync. Aug 20, 2019

johncming force-pushed the empty-rule branch from 1100ac9 to b75d521 Compare August 20, 2019 12:51

bwplotka self-requested a review August 22, 2019 09:00

bwplotka approved these changes Aug 22, 2019

View reviewed changes

bwplotka added bugfix state: changes-requested labels Aug 27, 2019

GiedriusS mentioned this pull request Nov 11, 2019

rule: Alerts will not be deleted after curl -X POST localhost:10902/-/reload. #1739

Closed

johncming force-pushed the empty-rule branch from b75d521 to 5eae069 Compare December 20, 2019 09:33

simonpasquier reviewed Jan 6, 2020

View reviewed changes

johncming force-pushed the empty-rule branch from 5eae069 to f841060 Compare January 7, 2020 02:25

fix bugs when rule clear.

f5a8ae1

Signed-off-by: johncming <johncming@yahoo.com>

johncming force-pushed the empty-rule branch from f841060 to f5a8ae1 Compare January 7, 2020 02:31

Merge branch 'master' into empty-rule

19a10f4

Merge branch 'master' into empty-rule

4afa846

simonpasquier reviewed Jan 15, 2020

View reviewed changes

simonpasquier mentioned this pull request Jan 23, 2020

fix bug: reloading rule files with empty group doesn't take effect #1885

Closed

bwplotka approved these changes Jan 25, 2020

View reviewed changes

stale bot added the stale label Mar 1, 2020

stale bot closed this Mar 9, 2020

lilic mentioned this pull request May 15, 2020

rule: Fix bug when rules were out of sync #2615

Merged

2 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

pkg/rule: Fix bugs where rules were out of sync. #1439

pkg/rule: Fix bugs where rules were out of sync. #1439

johncming commented Aug 20, 2019 •

edited

Loading

bwplotka left a comment

bwplotka Aug 22, 2019

johncming Aug 28, 2019 •

edited

Loading

johncming commented Aug 28, 2019

GiedriusS commented Nov 16, 2019

simonpasquier commented Dec 19, 2019

johncming commented Dec 20, 2019 •

edited

Loading

simonpasquier left a comment

simonpasquier Jan 6, 2020

simonpasquier Jan 6, 2020

simonpasquier commented Jan 6, 2020

johncming commented Jan 7, 2020

simonpasquier commented Jan 7, 2020

johncming commented Jan 8, 2020

daixiang0 commented Jan 13, 2020 •

edited

Loading

johncming commented Jan 15, 2020

simonpasquier Jan 15, 2020

bwplotka Jan 23, 2020

simonpasquier Jan 15, 2020

bwplotka left a comment •

edited

Loading

GiedriusS commented Jan 31, 2020

stale bot commented Mar 1, 2020

lilic commented Apr 2, 2020

simonpasquier commented Apr 2, 2020

lilic commented Apr 2, 2020

pkg/rule: Fix bugs where rules were out of sync. #1439

pkg/rule: Fix bugs where rules were out of sync. #1439

Conversation

johncming commented Aug 20, 2019 • edited Loading

bwplotka left a comment

Choose a reason for hiding this comment

bwplotka Aug 22, 2019

Choose a reason for hiding this comment

johncming Aug 28, 2019 • edited Loading

Choose a reason for hiding this comment

johncming commented Aug 28, 2019

GiedriusS commented Nov 16, 2019

simonpasquier commented Dec 19, 2019

johncming commented Dec 20, 2019 • edited Loading

simonpasquier left a comment

Choose a reason for hiding this comment

simonpasquier Jan 6, 2020

Choose a reason for hiding this comment

simonpasquier Jan 6, 2020

Choose a reason for hiding this comment

simonpasquier commented Jan 6, 2020

johncming commented Jan 7, 2020

simonpasquier commented Jan 7, 2020

johncming commented Jan 8, 2020

daixiang0 commented Jan 13, 2020 • edited Loading

johncming commented Jan 15, 2020

simonpasquier Jan 15, 2020

Choose a reason for hiding this comment

bwplotka Jan 23, 2020

Choose a reason for hiding this comment

simonpasquier Jan 15, 2020

Choose a reason for hiding this comment

bwplotka left a comment • edited Loading

Choose a reason for hiding this comment

GiedriusS commented Jan 31, 2020

stale bot commented Mar 1, 2020

lilic commented Apr 2, 2020

simonpasquier commented Apr 2, 2020

lilic commented Apr 2, 2020

johncming commented Aug 20, 2019 •

edited

Loading

johncming Aug 28, 2019 •

edited

Loading

johncming commented Dec 20, 2019 •

edited

Loading

daixiang0 commented Jan 13, 2020 •

edited

Loading

bwplotka left a comment •

edited

Loading