New ruler endpoints #620

jml · 2017-12-06T21:48:21Z

Part of #619

creates new endpoints for getting & setting ruler configs; puts these endpoints on the ruler itself
- these endpoints only exposed if db flags provided
creates new database methods that use the existing data structure but mask the fact that ruler & alertmanager configs are mushed together
updates ruler to use database for getting rules if provided right flag; falls back to config server otherwise

I was going to do alertmanager in the same PR, but this got big quickly.

I'm not wedded to putting the endpoints on the ruler itself.

Pros:

we can delete the configs service
the ruler scheduler doesn't need to poll the database

Cons:

if the ruler scheduler slows down, setting & getting configs on the UI will be slower (although given they both talk to the same DB, their performance will be correlated anyhow)

I say "ruler scheduler", because I'm intending to split the scheduler from workers as soon as #619 is done.

One thing I'll insist on is that these endpoints with new behaviours be at different URLs to the ones currently being served.

jml · 2017-12-06T22:10:15Z

Just remembered I want to move over some API tests and run them against in memory and Postgres databases. The query over jsonb data is quite funky and I could easily get it wrong.

bboreham

Broadly looks ok but I had a couple of questions.

bboreham · 2017-12-11T14:15:09Z

pkg/ruler/configs.go

+// RulesAPI is what the ruler needs from a config store to process rules.
+type RulesAPI interface {
+	// GetConfigs returns all Cortex configurations from a configs API server
+	// // TODO: hat have been updated after the given configs.ID was last updated.


Why is this a TODO?

Emacs typo. t<TAB> triggers a TODO autocomplete.

bboreham · 2017-12-11T14:19:17Z

pkg/ruler/configs.go

+// GetConfigs implements RulesAPI.
+func (c configsClient) GetConfigs(since configs.ID) (map[string]configs.VersionedRulesConfig, error) {
+	suffix := ""
+	if since != 0 {


Why doesn't this make use of AlertManagerConfigsAPI.GetConfigs() ?

It's for rules, not alertmanager configs, but "why doesn't this use RulerConfigsAPI.GetConfigs()?" is a valid question. The answer is that it replaces RulerConfigsAPI, and has a slightly different API (configs.VersionedRulesConfig is new in this PR).

We're replacing it because we don't want to have a client library per se. The ruler will be directly responsible for querying the database, so there's no need for code in another package that knows how to get ruler configs.

bboreham · 2017-12-11T15:07:44Z

pkg/ruler/api.go

+		http.Error(w, "No configuration", http.StatusNotFound)
+		return
+	} else if err != nil {
+		// XXX: Untested


What is the expectation with this comment?

It's cargo-culted from the config API. The vague expectation is that we'd test the error path one of these days. I've removed it & its like.

jml · 2017-12-21T12:35:26Z

Still needs some more tests before I'm happy to merge it.

jml · 2017-12-21T12:36:01Z

Also changed new API to CAS, which addresses part of #330

jml · 2018-01-05T17:29:54Z

Wrote the tests. I'm personally happy for this to be merged.

@bboreham PTAL?

bboreham

I haven't finished reviewing, but I got to a point where I can't see why it's correct, so please look at the comments so far and I'll come back to it when you respond.

Will you clean up the commit history? It's harder to work though the steps when they sometimes backtrace.

bboreham · 2018-01-09T10:08:21Z

pkg/ruler/api_test.go

@@ -43,22 +44,22 @@ func cleanup(t *testing.T) {
 }

 // request makes a request to the configs API.
-func request(t *testing.T, method, urlStr string, body io.Reader) *httptest.ResponseRecorder {
+func request(t *testing.T, handler http.Handler, method, urlStr string, body io.Reader) *httptest.ResponseRecorder {


Please include this change in the commit description, or in a separate commit if that makes sense.

I've split this into a different commit.

bboreham · 2018-01-09T10:12:36Z

pkg/configs/db/db.go

@@ -24,12 +24,12 @@ func (cfg *Config) RegisterFlags(f *flag.FlagSet) {

 // RulesDB has ruler-specific DB interfaces.
 type RulesDB interface {
-	// GetRulesConfig gets the user's alertmanager config
+	// GetRulesConfig gets the user's ruler config


Please include this change in the commit description, or move to a separate commit if that makes more sense.

I've moved it to be part of 029f17c, which introduces this interface.

bboreham · 2018-01-09T10:17:08Z

pkg/configs/db/postgres/postgres.go

+		// The supplied oldConfig must match the current config. If no config
+		// exists, then oldConfig must be nil. Otherwise, it must exactly
+		// equal the existing config.
+		if !((err == sql.ErrNoRows && oldConfig == nil) || reflect.DeepEqual(current.Config.RulesFiles, oldConfig)) {


Less repetition if you moved the details of the check into a RulesConfig.Equal() method, and that could be coded as a straightforward loop rather than using reflect.

FWIW, in my early morning fog, it wasn't that straightforward. I have become too used to just writing deriving Eq.

bboreham · 2018-01-09T10:47:50Z

pkg/configs/db/postgres/postgres.go

+		Options("DISTINCT ON (owner_id)").
+		From("configs").
+		Where(filter).
+		Where("config ->> 'rules_files' <> '{}'").


Good question. I've added a fairly detailed comment, repasted here in case that assists your follow-up review.

// `->>` gets a JSON object field as text. When a config row exists // and alertmanager config is provided but ruler config has not yet // been, the 'rules_files' key will have an empty JSON object as its // value. This is (probably) the most efficient way to test for a // non-empty `rules_files` key. // // This whole situation is way too complicated. See // https://github.com/weaveworks/cortex/issues/619 for the whole // story, and our plans to improve it.

This seems to assume we only ever go from blank to non-blank in the history; if it goes from non-blank to blank then you will select the previous version.

bboreham · 2018-01-09T10:50:06Z

pkg/configs/db/postgres/postgres.go

+		From("configs").
+		Where(filter).
+		Where("config ->> 'rules_files' <> '{}'").
+		OrderBy("owner_id, id DESC").


Why don't we have a Limit(1) here like in GetConfig() ?

Because GetConfig() gets the configuration for a single user, whereas findRulesConfigs helps GetAllRulesConfigs() and GetRulesConfigs() get the active configurations for all users.

I've added a doc comment that highlights the purpose, but doesn't explicitly answer the "why no Limit(1)?" question:

// findRulesConfigs helps GetAllRulesConfigs and GetRulesConfigs retrieve the
// set of all active rules configurations across all our users.

I'm open to renaming some or all of these functions if you can suggest names that more clearly communicate intent.

bboreham · 2018-01-09T10:51:14Z

pkg/configs/db/db.go

+type RulesDB interface {
+	// GetRulesConfig gets the user's ruler config
+	GetRulesConfig(userID string) (configs.VersionedRulesConfig, error)
+	// SetRulesConfig sets the user's ruler config


Mention the CAS-ness of the call

Will fall back to using configs service if no DB information supplied.

Now an internal implementation detail of ruler while we migrate away from configs server for ruler configs.

Comes with tests, and updates to our Makefile to ensure that the tests run against both postgres and in-memory database.

This allows us to re-use the helpers to communicate to other endpoints served by completely different API objects.

Fixes errors that occur when alertmanager config is set but rules files are not.

Fixes 6d4f8be

jml · 2018-01-10T10:58:56Z

@bboreham I've addressed all your review points. I think I could sensibly split this into two PRs, where the first PR goes from master..3dc22b9 (Public RulesAPI no longer used. Delete it.) and changes the ruler to go directly to the database for getting all configs, and the second PR has the rest and adds the new endpoints & tests. I only want to do this if it accelerates getting both merged (as I think this is blocking #629).

PTAL.

Fixes 6d4f8be

Fixes 029f17c

bboreham · 2018-01-11T14:54:51Z

pkg/configs/db/postgres/postgres.go

+// findRulesConfigs helps GetAllRulesConfigs and GetRulesConfigs retrieve the
+// set of all active rules configurations across all our users.
+func (d DB) findRulesConfigs(filter squirrel.Sqlizer) (map[string]configs.VersionedRulesConfig, error) {
+	rows, err := d.Select("id", "owner_id", "config ->> 'rules_files'").


OK, now I don't understand why this doesn't bring back the entire history, so you will end up with some random version.

ah, "SELECT DISTINCT ON ( expression [, ...] ) keeps only the first row of each set of rows"
and you have order by id DESC below.

So effectively the same as LIMIT 1 elsewhere

Want me to add a comment along those lines? If so, can you share the link to the doc you just quoted?

https://www.postgresql.org/docs/9.5/static/sql-select.html#SQL-DISTINCT

bboreham

I'm still not sure about selecting for non-blank rules, but guess that will be rare in the wild.

jml · 2018-01-11T15:28:47Z

Thanks! My intent is that we won't be running that query for long, as we'll be migrating the recording rules to a table entirely separate from the alertmanager configs.

…

On Thu, 11 Jan 2018 at 15:11 Bryan Boreham ***@***.***> wrote: ***@***.**** approved this pull request. I'm still not sure about selecting for non-blank rules, but guess that will be rare in the wild. — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub <#620 (review)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAHq6u9YXAKGiMN9JNw8TtmAVhYCXoS6ks5tJiSJgaJpZM4Q4pBm> .

This reverts commit 108f77f.

This reverts commit 860d2fb.

* Revert "Revert "New ruler endpoints (#620)" (#648)" This reverts commit 860d2fb. * Use DB only if Configs API *not* provided Works around the fact that `database.uri` has a default value, so we cannot detect whether the user has explicitly asked for a database or not.

jml force-pushed the 619-new-ruler-endpoints branch from 0a72c4e to e6431a3 Compare December 6, 2017 21:49

jml requested a review from bboreham December 7, 2017 18:43

jml mentioned this pull request Dec 11, 2017

Split alertmanager & ruler configs #619

Closed

bboreham reviewed Dec 11, 2017

View reviewed changes

This was referenced Dec 13, 2017

Switch to new-style rules format #622

Closed

Notifications for event type "monitor" are generating with wrong instance ID weaveworks/service#1595

Closed

jml changed the title ~~New ruler endpoints~~ [WIP] New ruler endpoints Dec 21, 2017

jml mentioned this pull request Jan 5, 2018

Report deleted configurations when reporting changed configurations #329

Closed

Remove redundant config struct

c989ef2

jml force-pushed the 619-new-ruler-endpoints branch from 2f7e2c8 to 9c22c68 Compare January 5, 2018 17:29

jml changed the title ~~[WIP] New ruler endpoints~~ New ruler endpoints Jan 5, 2018

bboreham reviewed Jan 9, 2018

View reviewed changes

jml added 10 commits January 10, 2018 10:45

Separate DB interface and implementation for rules configs

029f17c

Use rules DB directly from ruler

82b9ee8

Will fall back to using configs service if no DB information supplied.

Public RulesAPI no longer used. Delete it.

3dc22b9

Now an internal implementation detail of ruler while we migrate away from configs server for ruler configs.

Provide APIs for getting & setting rules on ruler itself

a8702b3

Make new set endpoint CAS

a82b28a

Comes with tests, and updates to our Makefile to ensure that the tests run against both postgres and in-memory database.

Tests for getting all configs

a61dc30

Tests for invalid configs

095d09f

Explicitly provide handler in ruler API test helpers

7f2099e

This allows us to re-use the helpers to communicate to other endpoints served by completely different API objects.

Tests for alertmanager interaction

53265d7

Fixes errors that occur when alertmanager config is set but rules files are not.

Update documentation to refer to CAS

4585474

Fixes 6d4f8be

jml force-pushed the 619-new-ruler-endpoints branch from 9c22c68 to 4305b19 Compare January 10, 2018 10:55

jml added 2 commits January 10, 2018 11:48

Specialized implementation of Equal for RulesConfig

a9d2874

Fixes 6d4f8be

Better documentation for findRulesConfig

fd9fc8d

Fixes 029f17c

jml force-pushed the 619-new-ruler-endpoints branch from 4305b19 to fd9fc8d Compare January 10, 2018 11:48

bboreham reviewed Jan 11, 2018

View reviewed changes

bboreham approved these changes Jan 11, 2018

View reviewed changes

jml merged commit 108f77f into master Jan 11, 2018

jml added a commit that referenced this pull request Jan 11, 2018

Revert "New ruler endpoints (#620)"

8d161c0

This reverts commit 108f77f.

jml mentioned this pull request Jan 11, 2018

Revert "New ruler endpoints" #648

Merged

jml added a commit that referenced this pull request Jan 11, 2018

Revert "New ruler endpoints (#620)" (#648)

860d2fb

This reverts commit 108f77f.

jml added a commit that referenced this pull request Jan 11, 2018

Revert "Revert "New ruler endpoints (#620)" (#648)"

cb84080

This reverts commit 860d2fb.

jml mentioned this pull request Jan 11, 2018

New CAS endpoints for ruler configs on ruler itself #649

Merged

tomwilkie deleted the 619-new-ruler-endpoints branch August 29, 2018 15:02

bboreham mentioned this pull request Apr 15, 2019

Future of the Config Service #1244

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

New ruler endpoints #620

New ruler endpoints #620

jml commented Dec 6, 2017

jml commented Dec 6, 2017

bboreham left a comment

bboreham Dec 11, 2017

jml Dec 20, 2017

bboreham Dec 11, 2017

jml Jan 3, 2018

bboreham Dec 11, 2017

jml Dec 20, 2017

jml commented Dec 21, 2017

jml commented Dec 21, 2017

jml commented Jan 5, 2018

bboreham left a comment

bboreham Jan 9, 2018

jml Jan 10, 2018

bboreham Jan 9, 2018

jml Jan 10, 2018

bboreham Jan 9, 2018

jml Jan 10, 2018

bboreham Jan 9, 2018

jml Jan 10, 2018

bboreham Jan 11, 2018

bboreham Jan 9, 2018

jml Jan 10, 2018

bboreham Jan 9, 2018

jml commented Jan 10, 2018

bboreham Jan 11, 2018

bboreham Jan 11, 2018

jml Jan 11, 2018

bboreham Jan 11, 2018

bboreham left a comment

jml commented Jan 11, 2018 via email

New ruler endpoints #620

New ruler endpoints #620

Conversation

jml commented Dec 6, 2017

jml commented Dec 6, 2017

bboreham left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jml commented Dec 21, 2017

jml commented Dec 21, 2017

jml commented Jan 5, 2018

bboreham left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jml commented Jan 10, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bboreham left a comment

Choose a reason for hiding this comment

jml commented Jan 11, 2018 via email