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.

Sign up for GitHub

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

Jump to bottom

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

Merged
merged 1 commit into from
May 17, 2020
Merged

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

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
bwplotka
bwplotka approved these changes May 15, 2020
View reviewed changes
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.

cmd/thanos/rule.go
@@ -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 •
edited
Loading

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 •
edited
Loading

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.

simonpasquier
simonpasquier reviewed May 15, 2020
View reviewed changes
cmd/thanos/rule.go Outdated Show resolved Hide resolved
@lilic @johncming
rule: Fix bug when rules were out of sync
b7a08b3 
Co-authored-by: johncming <johncming@yahoo.com>
Signed-off-by: Lili Cosic <cosiclili@gmail.com>
GiedriusS
GiedriusS approved these changes May 17, 2020
View reviewed changes
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
@simonpasquier @johncming @lilic
Rule: update manager when all rule files are removed
23b77c3 
This bug was already fixed in thanos-io#2615
but it got lost when we merged
thanos-io#2200.

Co-authored-by: johncming <johncming@yahoo.com>
Co-authored-by: Lili Cosic <cosiclili@gmail.com>
Signed-off-by: Simon Pasquier <spasquie@redhat.com>
@simonpasquier simonpasquier mentioned this pull request Aug 28, 2020
Merged
2 tasks
simonpasquier added a commit to simonpasquier/thanos that referenced this pull request Aug 28, 2020
@simonpasquier @johncming @lilic
Rule: update manager when all rule files are removed
7535187 
This bug was already fixed in thanos-io#2615
but it got lost when we merged
thanos-io#2200.

Co-authored-by: johncming <johncming@yahoo.com>
Co-authored-by: Lili Cosic <cosiclili@gmail.com>
Signed-off-by: Simon Pasquier <spasquie@redhat.com>
bwplotka pushed a commit that referenced this pull request Aug 28, 2020
@simonpasquier @johncming @lilic
Rule: update manager when all rule files are removed (#3095)
905d7f1 
This bug was already fixed in #2615
but it got lost when we merged
#2200.

Co-authored-by: johncming <johncming@yahoo.com>
Co-authored-by: Lili Cosic <cosiclili@gmail.com>
Signed-off-by: Simon Pasquier <spasquie@redhat.com>

Co-authored-by: johncming <johncming@yahoo.com>
Co-authored-by: Lili Cosic <cosiclili@gmail.com>
kakkoyun pushed a commit to kakkoyun/thanos that referenced this pull request Sep 7, 2020
@simonpasquier @johncming
@lilic @kakkoyun
Rule: update manager when all rule files are removed (thanos-io#3095)
0ac12db 
This bug was already fixed in thanos-io#2615
but it got lost when we merged
thanos-io#2200.

Co-authored-by: johncming <johncming@yahoo.com>
Co-authored-by: Lili Cosic <cosiclili@gmail.com>
Signed-off-by: Simon Pasquier <spasquie@redhat.com>

Co-authored-by: johncming <johncming@yahoo.com>
Co-authored-by: Lili Cosic <cosiclili@gmail.com>
kakkoyun pushed a commit to kakkoyun/thanos that referenced this pull request Sep 7, 2020
@simonpasquier @johncming
@lilic @kakkoyun
Rule: update manager when all rule files are removed (thanos-io#3095)
a060c41 
This bug was already fixed in thanos-io#2615
but it got lost when we merged
thanos-io#2200.

Co-authored-by: johncming <johncming@yahoo.com>
Co-authored-by: Lili Cosic <cosiclili@gmail.com>
Signed-off-by: Simon Pasquier <spasquie@redhat.com>

Co-authored-by: johncming <johncming@yahoo.com>
Co-authored-by: Lili Cosic <cosiclili@gmail.com>
Signed-off-by: Kemal Akkoyun <kakkoyun@gmail.com>
kakkoyun added a commit that referenced this pull request Sep 7, 2020
@kakkoyun @bwplotka
@krasi-georgiev @simonpasquier @johncming @lilic @onprem @yeya24
Merge v0.15.0 to master (#3130)
03b6991 
* Made sure old sse S3 option if specified, produces error.

Signed-off-by: Bartlomiej Plotka <bwplotka@gmail.com>
Signed-off-by: Kemal Akkoyun <kakkoyun@gmail.com>

* Cut v0.15.0-rc.0

Signed-off-by: Bartlomiej Plotka <bwplotka@gmail.com>
Signed-off-by: Kemal Akkoyun <kakkoyun@gmail.com>

* tsdbstore: Optimized response framing if iterator finished.

Signed-off-by: Bartlomiej Plotka <bwplotka@gmail.com>
Signed-off-by: Kemal Akkoyun <kakkoyun@gmail.com>

* Apply suggestions from code review

Co-authored-by: Krasimir Georgiev <8903888+krasi-georgiev@users.noreply.github.com>
Signed-off-by: Kemal Akkoyun <kakkoyun@gmail.com>

* 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 <johncming@yahoo.com>
Co-authored-by: Lili Cosic <cosiclili@gmail.com>
Signed-off-by: Simon Pasquier <spasquie@redhat.com>

Co-authored-by: johncming <johncming@yahoo.com>
Co-authored-by: Lili Cosic <cosiclili@gmail.com>
Signed-off-by: Kemal Akkoyun <kakkoyun@gmail.com>

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

* ui: Fix Block viewer for Compactor and Store

Signed-off-by: Prem Kumar <prmsrswt@gmail.com>

* Add an entry in CHANGELOG.md

Signed-off-by: Prem Kumar <prmsrswt@gmail.com>

* ui: react: Fixed tests for Blocks

Signed-off-by: Prem Kumar <prmsrswt@gmail.com>
Signed-off-by: Kemal Akkoyun <kakkoyun@gmail.com>

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

* Fix overwriting maxSourceResolution when auto downsampling is enabled

Signed-off-by: Ben Ye <yb532204897@gmail.com>

* add changelog

Signed-off-by: Ben Ye <yb532204897@gmail.com>
Signed-off-by: Kemal Akkoyun <kakkoyun@gmail.com>

* Author: Krasi Georgiev <8903888+krasi-georgiev@users.noreply.github.com> Date:   Thu Aug 27 14:10:19 2020 +0300 (#3010)

Make the PromQL lookBack configurable with a flag

Signed-off-by: Krasi Georgiev <8903888+krasi-georgiev@users.noreply.github.com>
Signed-off-by: Kemal Akkoyun <kakkoyun@gmail.com>

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

* Cut release v0.15.0-rc.1

Signed-off-by: Kemal Akkoyun <kakkoyun@gmail.com>

* Address review issues

Signed-off-by: Kemal Akkoyun <kakkoyun@gmail.com>

* Fix changelog

Signed-off-by: Kemal Akkoyun <kakkoyun@gmail.com>

* Cut release v0.15.0 (#3129)

Signed-off-by: Kemal Akkoyun <kakkoyun@gmail.com>

* Minor fix

Signed-off-by: Kemal Akkoyun <kakkoyun@gmail.com>

* Address review comments

Signed-off-by: Kemal Akkoyun <kakkoyun@gmail.com>

Co-authored-by: Bartlomiej Plotka <bwplotka@gmail.com>
Co-authored-by: Krasimir Georgiev <8903888+krasi-georgiev@users.noreply.github.com>
Co-authored-by: Simon Pasquier <spasquie@redhat.com>
Co-authored-by: johncming <johncming@yahoo.com>
Co-authored-by: Lili Cosic <cosiclili@gmail.com>
Co-authored-by: Prem Kumar <prmsrswt@gmail.com>
Co-authored-by: Ben Ye <yb532204897@gmail.com>
brancz pushed a commit to brancz/objstore that referenced this pull request Jan 28, 2022
@kakkoyun @bwplotka
@krasi-georgiev @simonpasquier @johncming @lilic @onprem @yeya24
Merge v0.15.0 to master (#3130)
bb9c5e8 
* Made sure old sse S3 option if specified, produces error.

Signed-off-by: Bartlomiej Plotka <bwplotka@gmail.com>
Signed-off-by: Kemal Akkoyun <kakkoyun@gmail.com>

* Cut v0.15.0-rc.0

Signed-off-by: Bartlomiej Plotka <bwplotka@gmail.com>
Signed-off-by: Kemal Akkoyun <kakkoyun@gmail.com>

* tsdbstore: Optimized response framing if iterator finished.

Signed-off-by: Bartlomiej Plotka <bwplotka@gmail.com>
Signed-off-by: Kemal Akkoyun <kakkoyun@gmail.com>

* Apply suggestions from code review

Co-authored-by: Krasimir Georgiev <8903888+krasi-georgiev@users.noreply.github.com>
Signed-off-by: Kemal Akkoyun <kakkoyun@gmail.com>

* 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 <johncming@yahoo.com>
Co-authored-by: Lili Cosic <cosiclili@gmail.com>
Signed-off-by: Simon Pasquier <spasquie@redhat.com>

Co-authored-by: johncming <johncming@yahoo.com>
Co-authored-by: Lili Cosic <cosiclili@gmail.com>
Signed-off-by: Kemal Akkoyun <kakkoyun@gmail.com>

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

* ui: Fix Block viewer for Compactor and Store

Signed-off-by: Prem Kumar <prmsrswt@gmail.com>

* Add an entry in CHANGELOG.md

Signed-off-by: Prem Kumar <prmsrswt@gmail.com>

* ui: react: Fixed tests for Blocks

Signed-off-by: Prem Kumar <prmsrswt@gmail.com>
Signed-off-by: Kemal Akkoyun <kakkoyun@gmail.com>

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

* Fix overwriting maxSourceResolution when auto downsampling is enabled

Signed-off-by: Ben Ye <yb532204897@gmail.com>

* add changelog

Signed-off-by: Ben Ye <yb532204897@gmail.com>
Signed-off-by: Kemal Akkoyun <kakkoyun@gmail.com>

* Author: Krasi Georgiev <8903888+krasi-georgiev@users.noreply.github.com> Date:   Thu Aug 27 14:10:19 2020 +0300 (#3010)

Make the PromQL lookBack configurable with a flag

Signed-off-by: Krasi Georgiev <8903888+krasi-georgiev@users.noreply.github.com>
Signed-off-by: Kemal Akkoyun <kakkoyun@gmail.com>

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

* Cut release v0.15.0-rc.1

Signed-off-by: Kemal Akkoyun <kakkoyun@gmail.com>

* Address review issues

Signed-off-by: Kemal Akkoyun <kakkoyun@gmail.com>

* Fix changelog

Signed-off-by: Kemal Akkoyun <kakkoyun@gmail.com>

* Cut release v0.15.0 (#3129)

Signed-off-by: Kemal Akkoyun <kakkoyun@gmail.com>

* Minor fix

Signed-off-by: Kemal Akkoyun <kakkoyun@gmail.com>

* Address review comments

Signed-off-by: Kemal Akkoyun <kakkoyun@gmail.com>

Co-authored-by: Bartlomiej Plotka <bwplotka@gmail.com>
Co-authored-by: Krasimir Georgiev <8903888+krasi-georgiev@users.noreply.github.com>
Co-authored-by: Simon Pasquier <spasquie@redhat.com>
Co-authored-by: johncming <johncming@yahoo.com>
Co-authored-by: Lili Cosic <cosiclili@gmail.com>
Co-authored-by: Prem Kumar <prmsrswt@gmail.com>
Co-authored-by: Ben Ye <yb532204897@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@simonpasquier simonpasquier simonpasquier left review comments

@GiedriusS GiedriusS GiedriusS approved these changes

@bwplotka bwplotka bwplotka approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@lilic @GiedriusS @simonpasquier @bwplotka