Add endpoints to deactivate and restore config #629

lelenanam · 2017-12-22T22:04:24Z

should call this to deactivate config after instance deletion

lelenanam · 2017-12-23T02:49:16Z

bboreham · 2018-01-02T15:26:08Z

You need to run make update-vendor after dep commands - see https://github.com/weaveworks/cortex/blob/master/README.md#vendoring

bboreham · 2018-01-03T15:10:04Z

Now I've looked at this a bit more, it will do something slightly surprising.

The DB has potentially many configs for an instance, each with a new ID number.
DB.GetConfig() selects the non-deleted ones, limit 1 order by id DESC.

The code in this PR will mark all rows for an instance as deleted on DeactivateConfig() and mark all of them as undeleted on RestoreConfig(). So that's ok, but surprising to me.

This wouldn't play well if, for instance, some other component was marking individual rows as deleted. (Although I can't see anything like that).

Another issue is that ruler will only see the config as changed if there is one for the same instance with a higher ID number.

I'm not sure what to do about this.

#332 covers garbage-collecting old rows

bboreham · 2018-01-03T15:58:23Z

There is an issue to report deleted configs - #329

rndstr

just a few comments while passing by!

rndstr · 2018-01-04T11:00:15Z

pkg/configs/db/timed.go

"DeactivateConfig" & same in traced.go

rndstr · 2018-01-04T11:00:25Z

pkg/configs/db/timed.go

"RestoreConfig" & same in traced.go

rndstr · 2018-01-04T11:05:21Z

pkg/configs/db/memory/memory.go

return c.deletedAt.IsZero()?

rndstr · 2018-01-04T11:41:59Z

pkg/configs/db/postgres/postgres.go

What about using WARN here? Unless DEBUG is the default level, it doesn't say that it's stalling

bboreham · 2018-01-09T10:43:49Z

In regard to "ruler will only see the config as changed if there is one for the same instance with a higher ID number" - #329, Elena and I discussed IRL an idea to change the way deleted configs are interpreted.
(@lelenanam if you wrote this up somewhere I apologise; I can't find it)

At present, the DB queries fetch the last active (non-deleted) config. So you can revert from the latest config to the one before by setting deleted_at on one row. We don't know of any code that does this.

The code that updates ruler has a note of the highest ID number it has seen, and polls looking for active configs with higher numbers. So it will never report a deleted row, new or otherwise.

Suppose we change the behaviour to return the config with the highest ID, deleted or not. Then we can implement "delete a config" by adding a single new row with deleted_at set. Ruler would be notified of this new row; it would have to be enhanced to tell the difference between active and deleted configs.

If we wanted a 'revert to previous' feature, it would now be implemented by adding a new row with the previous config.

cc @jml @tomwilkie

lelenanam · 2018-01-20T02:45:57Z

Changed deactivation as @bboreham described (add new config row with deletedAt).
But we also need extra code to stop receiving alerts.

lelenanam · 2018-01-20T02:46:28Z

@bboreham PTAL

bboreham

Good overall: I think you'd have to change ruler to inspect DeletedAt before this could be merged.
And at least make an issue to make any other users of the data (GUIs, ...) aware that they can now receive deleted records.

bboreham · 2018-01-24T09:55:11Z

pkg/configs/configs.go

-	Config Config `json:"config"`
+	ID        ID     `json:"id"`
+	Config    Config `json:"config"`
+	DeletedAt time.Time


why doesn't this have a json tag?

bboreham · 2018-01-24T10:05:12Z

pkg/configs/db/postgres/postgres.go

 }

+// GetLastConfig gets a last configuration (active or deleted).
+func (d DB) GetLastConfig(userID string) (configs.View, error) {


I have a feeling this function isn't necessary: GetConfig is good enough for the purpose.

bboreham · 2018-01-24T10:07:03Z

pkg/configs/db/postgres/postgres.go

+	return cfgView, err
+}
+
+// SetDeletedAtConfig sets a deletedAt for configuration.


I think it's worth commenting why "set" is actually an "insert".

Fixed review comments.

bboreham · 2018-01-28T00:11:02Z

What happens in scheduler.addNewConfigs() when a deleted record update arrives in the middle of operation?

lelenanam · 2018-01-31T01:09:31Z

What happens in scheduler.addNewConfigs() when a deleted record update arrives in the middle of operation?

I suppose the same when config changes in the middle of the operation, this config will be updated/deleted on next scheduler.pollInterval tick.

lelenanam · 2018-01-31T01:18:42Z

@bboreham PTAL

- call deactivate on instance deletion

lelenanam force-pushed the cleanup branch 3 times, most recently from 9abb7c0 to 62bcea7 Compare December 22, 2017 22:07

lelenanam requested review from bboreham and jml December 22, 2017 22:09

lelenanam force-pushed the cleanup branch from 62bcea7 to 83280f8 Compare December 23, 2017 02:45

lelenanam force-pushed the cleanup branch 4 times, most recently from a3a2106 to 820a5a4 Compare January 3, 2018 00:42

bboreham mentioned this pull request Jan 3, 2018

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

Closed

rndstr reviewed Jan 4, 2018

View reviewed changes

lelenanam removed the request for review from jml January 4, 2018 17:03

lelenanam force-pushed the cleanup branch from 820a5a4 to fc53af5 Compare January 4, 2018 17:40

bboreham mentioned this pull request Jan 9, 2018

Report deleted configurations when reporting changed configurations #329

Closed

jml mentioned this pull request Jan 10, 2018

New ruler endpoints #620

Merged

lelenanam force-pushed the cleanup branch 2 times, most recently from c903b6a to 2432316 Compare January 20, 2018 02:41

bboreham previously requested changes Jan 24, 2018

View reviewed changes

lelenanam force-pushed the cleanup branch from 5891928 to 9fbbd67 Compare January 25, 2018 02:08

lelenanam mentioned this pull request Jan 25, 2018

Handle deleted configuration (for deleted instances) in GUI etc #669

Closed

lelenanam force-pushed the cleanup branch 2 times, most recently from 6e27d2d to 15878d3 Compare January 31, 2018 00:26

lelenanam force-pushed the cleanup branch from 15878d3 to 36ec6c7 Compare January 31, 2018 01:11

bboreham approved these changes Feb 1, 2018

View reviewed changes

Add endpoints to deactivate and restore config

0ff7de4

- call deactivate on instance deletion

lelenanam force-pushed the cleanup branch from 36ec6c7 to 0ff7de4 Compare February 1, 2018 18:01

lelenanam merged commit b292164 into master Feb 1, 2018

lelenanam deleted the cleanup branch February 1, 2018 18:09

bboreham mentioned this pull request Feb 8, 2018

Shut down AlertManager when config is deleted #692

Closed

Add endpoints to deactivate and restore config #629

Add endpoints to deactivate and restore config #629

Uh oh!

Conversation

lelenanam commented Dec 22, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

lelenanam commented Dec 23, 2017

Uh oh!

bboreham commented Jan 2, 2018

Uh oh!

bboreham commented Jan 3, 2018

Uh oh!

bboreham commented Jan 3, 2018

Uh oh!

rndstr left a comment

Choose a reason for hiding this comment

Uh oh!

rndstr Jan 4, 2018

Choose a reason for hiding this comment

Uh oh!

rndstr Jan 4, 2018

Choose a reason for hiding this comment

Uh oh!

rndstr Jan 4, 2018

Choose a reason for hiding this comment

Uh oh!

rndstr Jan 4, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bboreham commented Jan 9, 2018

Uh oh!

lelenanam commented Jan 20, 2018

Uh oh!

lelenanam commented Jan 20, 2018

Uh oh!

bboreham left a comment

Choose a reason for hiding this comment

Uh oh!

bboreham Jan 24, 2018

Choose a reason for hiding this comment

Uh oh!

bboreham Jan 24, 2018

Choose a reason for hiding this comment

Uh oh!

bboreham Jan 24, 2018

Choose a reason for hiding this comment

Uh oh!

bboreham commented Jan 28, 2018

Uh oh!

lelenanam commented Jan 31, 2018

Uh oh!

lelenanam commented Jan 31, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

lelenanam commented Dec 22, 2017 •

edited

Loading

rndstr Jan 4, 2018 •

edited

Loading