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

Adding the Ruler to the Single Binary #2854

Merged
merged 23 commits into from
Jul 16, 2020
Merged

Adding the Ruler to the Single Binary #2854

merged 23 commits into from
Jul 16, 2020

Conversation

joe-elliott
Copy link
Contributor

@joe-elliott joe-elliott commented Jul 8, 2020 •
edited
Loading

What this PR does:

  • Adds the ruler to the single binary
    • Adds labels engine=querier and engine=ruler to prevent metrics collisions on promql.Engine.
  • Adds a new local storage engine for single binary local mode
    • Per discussions with @jtlisi it is read only. Jacob, if you could review this piece in detail I would appreciate it.
  • Adds appropriate unit/integration tests.
  • Updates single-process config yamls

Which issue(s) this PR fixes:
Fixes #2833

Checklist

  • Tests updated
  • Documentation added
  • CHANGELOG.md updated - the order of entries should be [CHANGE], [FEATURE], [ENHANCEMENT], [BUGFIX]

@joe-elliott joe-elliott changed the title Ruler single binary Adding the Ruler to the Single Binary Jul 8, 2020
@joe-elliott
lint + doc
c3b066c 
Signed-off-by: Joe Elliott <[email protected]>
@joe-elliott
changelog
6fab147 
Signed-off-by: Joe Elliott <[email protected]>
@joe-elliott
Fixed test
a3222bf 
Signed-off-by: Joe Elliott <[email protected]>
@joe-elliott
Url => URL
0782259 
Signed-off-by: Joe Elliott <[email protected]>
@joe-elliott
Url => URL
9347b3a 
Signed-off-by: Joe Elliott <[email protected]>
@joe-elliott
Added local storage option for ruler and added tests
b698acb 
Signed-off-by: Joe Elliott <[email protected]>
@joe-elliott
lint
befddc8 
Signed-off-by: Joe Elliott <[email protected]>
@joe-elliott
make doc
c3f651e 
Signed-off-by: Joe Elliott <[email protected]>
pracucci
pracucci reviewed Jul 9, 2020
View reviewed changes
Copy link
Contributor

@pracucci pracucci left a comment

Choose a reason for hiding this comment

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

Hey @joe-elliott, I know this is still a draft PR but I wanted to look at it anyway. I left few minor comments here and there and a couple of questions. Thanks!

@joe-elliott
Copy link
Contributor Author

Allright. Feeling good about this one. Dropping draft status.

@pracucci thank you for the early notes!

@joe-elliott joe-elliott marked this pull request as ready for review July 9, 2020 18:58
@joe-elliott
Clarified creation/deletion
569599d 
Signed-off-by: Joe Elliott <[email protected]>
pstibrany
pstibrany approved these changes Jul 14, 2020
View reviewed changes
Copy link
Contributor

@pstibrany pstibrany left a comment

Choose a reason for hiding this comment

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

LGTM. I'd like to see experience with no ruler config improved.

@joe-elliott
readme cleanup
d0e1254 
Signed-off-by: Joe Elliott <[email protected]>
@joe-elliott
Removed ruler config from integration tests. Allow single binary to s…
2eab3d7 
…tart without a ruler config

Signed-off-by: Joe Elliott <[email protected]>
@joe-elliott
Copy link
Contributor Author

joe-elliott commented Jul 14, 2020 •
edited
Loading

@pstibrany
I added a check and an info level log to prevent the single binary from failing if the ruler was not configured. The check is pretty gross, but I don't know if there's a cleaner option. Integration tests should warn us if there's ever drift.

2eab3d7

@pstibrany
Copy link
Contributor

@pstibrany
I added a check and an info level log to prevent the single binary from failing if the ruler was not configured. The check is pretty gross, but I don't know if there's a cleaner option. Integration tests should warn us if there's ever drift.

Thanks Joe. This works fine (I'd use a shared constant string between the two, but that's just a minor nit). I already approved.

jtlisi
jtlisi approved these changes Jul 14, 2020
View reviewed changes
Copy link
Contributor

@jtlisi jtlisi left a comment

Choose a reason for hiding this comment

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

LGTM, great work

pkg/cortex/modules.go
// unfortunately there is no way to generate a "default" config and compare default against actual
// to determine if it's unconfigured. the following check, however, correctly tests this.
// Single binary integration tests will break if this ever drifts
if t.Cfg.Target == All && t.Cfg.Ruler.StoreConfig.Type == "configdb" && t.Cfg.Ruler.StoreConfig.ConfigDB.ConfigsAPIURL.URL == nil {
Copy link
Contributor

Choose a reason for hiding this comment

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

No active work is being done on the configdb so I doubt this will break in the near future.

annanay25
annanay25 approved these changes Jul 15, 2020
View reviewed changes
Copy link
Contributor

@annanay25 annanay25 left a comment

Choose a reason for hiding this comment

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

lgtm!

pkg/ruler/rules/local/local.go
Comment on lines +24 to +28
// Client expects to load already existing rules located at:
// cfg.Directory / userID / namespace
type Client struct {
cfg Config
}
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: This can be named RuleStore to be consistent with the obj store naming convention

@pracucci
Reworked the CHANGELOG
41a65f5 
Signed-off-by: Marco Pracucci <[email protected]>
pracucci
pracucci approved these changes Jul 16, 2020
View reviewed changes
Copy link
Contributor

@pracucci pracucci left a comment

Choose a reason for hiding this comment

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

LGTM! I've just reworked the CHANGELOG.

@pracucci pracucci merged commit edaab80 into cortexproject:master Jul 16, 2020
@pstibrany pstibrany mentioned this pull request Aug 14, 2020
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@pstibrany pstibrany pstibrany approved these changes

@pracucci pracucci pracucci approved these changes

@jtlisi jtlisi jtlisi approved these changes

@annanay25 annanay25 annanay25 approved these changes

Assignees
No one assigned
Labels
size/L
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Add sample configuration file with ruler config
5 participants
@joe-elliott @pstibrany @pracucci @jtlisi @annanay25