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 bug when rules were out of sync #2615

Merged
merged 1 commit into from
May 17, 2020

Conversation

lilic
Copy link
Contributor

@lilic lilic commented May 15, 2020

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

Note: The commits were taken from this PR #1439, as that PR was closed too early but the bug still exists and we hit it in our setup. I added the original author as the co-commiter, as they were not responsive on the original PR.

Changes

From original PR:

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.

Note this also fixes when you have one rule loaded only, and you delete that rule it doesn't get updated, the number or rules is not important, it's the last rule.

Verification

There is a unit test added, I also tested this manually in our cluster setup by loading the rule via yaml file as the OP suggested and deleting the rule from the file, the rule was now cleared correctly.

@lilic lilic force-pushed the rules-out-sync branch 2 times, most recently from e7f8ac8 to 00dd8d8 Compare May 15, 2020 09:31
Copy link
Member

@bwplotka bwplotka left a comment

Choose a reason for hiding this comment

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

NIce, LGTM, just some questions.

Also how this was fixed?

Note this also fixes when you have one rule loaded only, and you delete that rule it doesn't get updated, the number or rules is not important, it's the last rule.

@@ -775,6 +776,13 @@ func reloadRules(logger log.Logger,
}

files = append(files, fs...)
for _, fp := range fs {
if _, ok := seenFiles[fp]; ok {
Copy link
Member

Choose a reason for hiding this comment

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

Is that necessary?

Copy link
Member

Choose a reason for hiding this comment

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

Also don't see you describe this being a bug in PR desc 🤔 am I missing something?

Copy link
Contributor Author

@lilic lilic May 15, 2020

Choose a reason for hiding this comment

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

I was just picking up from the previous PR which went through review from what I gathered, as mentioned in the description, if you feel like it's not needed let me know! :)

Copy link
Contributor

Choose a reason for hiding this comment

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

IIUC the idea is to remove duplicate rule files (e.g. thanos rule --rule-file foo.yaml --rule-file foo.yaml) but in practice it shouldn't matter as the rule manager will only load the file once eventually.

Copy link
Member

@GiedriusS GiedriusS May 16, 2020

Choose a reason for hiding this comment

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

That's true but I don't see a problem in avoiding processing the same file twice or more. I would say we should keep this change.

@lilic
Copy link
Contributor Author

lilic commented May 15, 2020

Note this also fixes when you have one rule loaded only, and you delete that rule it doesn't get updated, the number or rules is not important, it's the last rule.

This was fixed as the consequence of this PR, or more like the bug scope was previously thought to be only when you have a lot of rules from what I gather.

cmd/thanos/rule.go Outdated Show resolved Hide resolved
Co-authored-by: johncming <[email protected]>
Signed-off-by: Lili Cosic <[email protected]>
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.

LGTM, well tested 🏅

@GiedriusS GiedriusS merged commit 334a41b into thanos-io:master May 17, 2020
@lilic lilic deleted the rules-out-sync branch May 18, 2020 06:51
simonpasquier added a commit to simonpasquier/thanos that referenced this pull request Aug 28, 2020
This bug was already fixed in thanos-io#2615
but it got lost when we merged
thanos-io#2200.

Co-authored-by: johncming <[email protected]>
Co-authored-by: Lili Cosic <[email protected]>
Signed-off-by: Simon Pasquier <[email protected]>
simonpasquier added a commit to simonpasquier/thanos that referenced this pull request Aug 28, 2020
This bug was already fixed in thanos-io#2615
but it got lost when we merged
thanos-io#2200.

Co-authored-by: johncming <[email protected]>
Co-authored-by: Lili Cosic <[email protected]>
Signed-off-by: Simon Pasquier <[email protected]>
bwplotka pushed a commit that referenced this pull request Aug 28, 2020
This bug was already fixed in #2615
but it got lost when we merged
#2200.

Co-authored-by: johncming <[email protected]>
Co-authored-by: Lili Cosic <[email protected]>
Signed-off-by: Simon Pasquier <[email protected]>

Co-authored-by: johncming <[email protected]>
Co-authored-by: Lili Cosic <[email protected]>
kakkoyun pushed a commit to kakkoyun/thanos that referenced this pull request Sep 7, 2020
This bug was already fixed in thanos-io#2615
but it got lost when we merged
thanos-io#2200.

Co-authored-by: johncming <[email protected]>
Co-authored-by: Lili Cosic <[email protected]>
Signed-off-by: Simon Pasquier <[email protected]>

Co-authored-by: johncming <[email protected]>
Co-authored-by: Lili Cosic <[email protected]>
kakkoyun pushed a commit to kakkoyun/thanos that referenced this pull request Sep 7, 2020
This bug was already fixed in thanos-io#2615
but it got lost when we merged
thanos-io#2200.

Co-authored-by: johncming <[email protected]>
Co-authored-by: Lili Cosic <[email protected]>
Signed-off-by: Simon Pasquier <[email protected]>

Co-authored-by: johncming <[email protected]>
Co-authored-by: Lili Cosic <[email protected]>
Signed-off-by: Kemal Akkoyun <[email protected]>
kakkoyun added a commit that referenced this pull request Sep 7, 2020
* Made sure old sse S3 option if specified, produces error.

Signed-off-by: Bartlomiej Plotka <[email protected]>
Signed-off-by: Kemal Akkoyun <[email protected]>

* Cut v0.15.0-rc.0

Signed-off-by: Bartlomiej Plotka <[email protected]>
Signed-off-by: Kemal Akkoyun <[email protected]>

* tsdbstore: Optimized response framing if iterator finished.

Signed-off-by: Bartlomiej Plotka <[email protected]>
Signed-off-by: Kemal Akkoyun <[email protected]>

* Apply suggestions from code review

Co-authored-by: Krasimir Georgiev <[email protected]>
Signed-off-by: Kemal Akkoyun <[email protected]>

* Rule: update manager when all rule files are removed (#3095)

This bug was already fixed in #2615
but it got lost when we merged
#2200.

Co-authored-by: johncming <[email protected]>
Co-authored-by: Lili Cosic <[email protected]>
Signed-off-by: Simon Pasquier <[email protected]>

Co-authored-by: johncming <[email protected]>
Co-authored-by: Lili Cosic <[email protected]>
Signed-off-by: Kemal Akkoyun <[email protected]>

* ui: Fix Block Viewer for Compactor and Store (#3098)

* ui: Fix Block viewer for Compactor and Store

Signed-off-by: Prem Kumar <[email protected]>

* Add an entry in CHANGELOG.md

Signed-off-by: Prem Kumar <[email protected]>

* ui: react: Fixed tests for Blocks

Signed-off-by: Prem Kumar <[email protected]>
Signed-off-by: Kemal Akkoyun <[email protected]>

* querier: Fix overwriting maxSourceResolution when auto downsampling is enabled (#3105)

* Fix overwriting maxSourceResolution when auto downsampling is enabled

Signed-off-by: Ben Ye <[email protected]>

* add changelog

Signed-off-by: Ben Ye <[email protected]>
Signed-off-by: Kemal Akkoyun <[email protected]>

* Author: Krasi Georgiev <[email protected]> Date:   Thu Aug 27 14:10:19 2020 +0300 (#3010)

Make the PromQL lookBack configurable with a flag

Signed-off-by: Krasi Georgiev <[email protected]>
Signed-off-by: Kemal Akkoyun <[email protected]>

* Cut release v0.15.0-rc.1 (#3103)

* Cut release v0.15.0-rc.1

Signed-off-by: Kemal Akkoyun <[email protected]>

* Address review issues

Signed-off-by: Kemal Akkoyun <[email protected]>

* Fix changelog

Signed-off-by: Kemal Akkoyun <[email protected]>

* Cut release v0.15.0 (#3129)

Signed-off-by: Kemal Akkoyun <[email protected]>

* Minor fix

Signed-off-by: Kemal Akkoyun <[email protected]>

* Address review comments

Signed-off-by: Kemal Akkoyun <[email protected]>

Co-authored-by: Bartlomiej Plotka <[email protected]>
Co-authored-by: Krasimir Georgiev <[email protected]>
Co-authored-by: Simon Pasquier <[email protected]>
Co-authored-by: johncming <[email protected]>
Co-authored-by: Lili Cosic <[email protected]>
Co-authored-by: Prem Kumar <[email protected]>
Co-authored-by: Ben Ye <[email protected]>
brancz pushed a commit to brancz/objstore that referenced this pull request Jan 28, 2022
* Made sure old sse S3 option if specified, produces error.

Signed-off-by: Bartlomiej Plotka <[email protected]>
Signed-off-by: Kemal Akkoyun <[email protected]>

* Cut v0.15.0-rc.0

Signed-off-by: Bartlomiej Plotka <[email protected]>
Signed-off-by: Kemal Akkoyun <[email protected]>

* tsdbstore: Optimized response framing if iterator finished.

Signed-off-by: Bartlomiej Plotka <[email protected]>
Signed-off-by: Kemal Akkoyun <[email protected]>

* Apply suggestions from code review

Co-authored-by: Krasimir Georgiev <[email protected]>
Signed-off-by: Kemal Akkoyun <[email protected]>

* Rule: update manager when all rule files are removed (#3095)

This bug was already fixed in thanos-io/thanos#2615
but it got lost when we merged
thanos-io/thanos#2200.

Co-authored-by: johncming <[email protected]>
Co-authored-by: Lili Cosic <[email protected]>
Signed-off-by: Simon Pasquier <[email protected]>

Co-authored-by: johncming <[email protected]>
Co-authored-by: Lili Cosic <[email protected]>
Signed-off-by: Kemal Akkoyun <[email protected]>

* ui: Fix Block Viewer for Compactor and Store (#3098)

* ui: Fix Block viewer for Compactor and Store

Signed-off-by: Prem Kumar <[email protected]>

* Add an entry in CHANGELOG.md

Signed-off-by: Prem Kumar <[email protected]>

* ui: react: Fixed tests for Blocks

Signed-off-by: Prem Kumar <[email protected]>
Signed-off-by: Kemal Akkoyun <[email protected]>

* querier: Fix overwriting maxSourceResolution when auto downsampling is enabled (#3105)

* Fix overwriting maxSourceResolution when auto downsampling is enabled

Signed-off-by: Ben Ye <[email protected]>

* add changelog

Signed-off-by: Ben Ye <[email protected]>
Signed-off-by: Kemal Akkoyun <[email protected]>

* Author: Krasi Georgiev <[email protected]> Date:   Thu Aug 27 14:10:19 2020 +0300 (#3010)

Make the PromQL lookBack configurable with a flag

Signed-off-by: Krasi Georgiev <[email protected]>
Signed-off-by: Kemal Akkoyun <[email protected]>

* Cut release v0.15.0-rc.1 (#3103)

* Cut release v0.15.0-rc.1

Signed-off-by: Kemal Akkoyun <[email protected]>

* Address review issues

Signed-off-by: Kemal Akkoyun <[email protected]>

* Fix changelog

Signed-off-by: Kemal Akkoyun <[email protected]>

* Cut release v0.15.0 (#3129)

Signed-off-by: Kemal Akkoyun <[email protected]>

* Minor fix

Signed-off-by: Kemal Akkoyun <[email protected]>

* Address review comments

Signed-off-by: Kemal Akkoyun <[email protected]>

Co-authored-by: Bartlomiej Plotka <[email protected]>
Co-authored-by: Krasimir Georgiev <[email protected]>
Co-authored-by: Simon Pasquier <[email protected]>
Co-authored-by: johncming <[email protected]>
Co-authored-by: Lili Cosic <[email protected]>
Co-authored-by: Prem Kumar <[email protected]>
Co-authored-by: Ben Ye <[email protected]>
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.

4 participants