Added KV Store client that mirrors one backend to another #1749

pstibrany · 2019-10-23T09:07:40Z

Since we support multiple KV Store backends now (Consul, Etcd, memberlist-gossiping soon), we want to be able to migrate from one backend to a different one gracefully, without shutting down all components at once. (Issue #1525) This PR implements one way of doing that.

It adds yet another KV Store backend, called multiClient. This client is configured with several backends, one of them is designated as primary. Primary one is used for all client operations like Get, CAS, WatchKey, and WatchPrefix. ~~At the same time, multiClient reads all changes done in primary backend and copies them to secondary backend. (That is done using WatchPrefix, with "" prefix [update: now configurable])~~ Updates via CAS are also written to secondary store, if mirroring is enabled.

It is possible to switch the primary backend in the runtime by using existing overrides mechanism. This mechanism has been extended and refactored a bit. [update: it's now also possible to enable/disable mirroring in runtime via overrides]

Cortex has new "runtimeConfigValues" struct that is reloaded using OverridesManager (now runtime_config.Manager). runtimeConfigValues currently contains limit overrides and multi-client primary store. runtime_config.Manager now lives in its own runtime_config package, and its job is to reload runtime YAML file (overrides.yaml), and provide new value to clients and listeners.

Components interested in using runtimeConfigValues setup themselves in main Cortex setup code (cortex.go, modules.go). At the moment, interested components are Overrides (using limits) and Ring (when using multi-client, for listening on primary store changes).

After all this introduction, how is this supposed to work? In scenario, where one wants to migrate from Consul to Etcd, here are the steps for migration:

setup all components to use multi-client, with consul as primary and etcd as secondary store, and restart them. Components will keep using consul, but will also mirror the ring to etcd.
switch primary store to etcd in runtime (eg. by pushing new config map)
once done, all components are using etcd (and mirroring to consul)
remove multi-client configuration, and let components use etcd only

All this can be done without any downtime. ~~Cost of this is higher load on consul/etcd when components do mirroring, but that can be allieviated by using rate limits on consul client. [update: mirroring itself now has rate limit]~~ [Watch+mirroring functionality has been replaced with simple update of values in secondary stores when performing CAS operation]

pstibrany · 2019-10-24T15:31:28Z

TODO: think about how to do this without killing consul/etcd with too many requests. If every component (distributor/ingester/querier) does the mirroring all the time, that's probably little too much.

pstibrany · 2019-10-25T12:25:46Z

Cleaned up the code a little bit, and added:

rate limit for watching for changes in the primary store
config option for watch prefix
flag for enabling/disabling mirroring in runtime (via overrides)

jtlisi

LGTM, I just have a few questions

pkg/ring/kv/multi.go

rfratto

LGTM, just some nitpicky things 🙂

pkg/cortex/modules.go

pkg/ring/kv/multi.go

pracucci

Very good job @pstibrany! I left few minor non-blocking comments.

Tomorrow morning I will re-review the cancel fn logic in the multi client, cause I'm feeling tired and I may have miss something. Great job again!

pkg/util/override.go

pkg/ring/kv/consul/client.go

pkg/ring/kv/client.go

pkg/ring/kv/multi.go

jtlisi

LGTM

pstibrany · 2019-11-01T07:34:51Z

Rebased on top of master, and squashed to single commit, to ease possible future rebases.

sandeepsukhani

Changes look really good to me, great job!
Just a small change to do to fix an issue with limits changes.

pkg/util/validation/limits.go

pracucci

Good job @pstibrany! I left a couple of non blocking comments, so I'm approving it, given - to my 👀 - the changes look good.

pkg/ring/kv/multi.go

pkg/ring/kv/client.go

pkg/ring/kv/multi.go

gouthamve

So my big comment would be around simplifying the entire thing to be: Write to both, read from primary. Also, forget about inflight ops, just do it on the primary client when we initiated the call.

pkg/ring/kv/multi.go

gouthamve · 2019-11-06T04:56:57Z

pkg/ring/kv/multi.go

+
+// Runs supplied fn with current primary client. If primary client changes, fn is restarted.
+// When fn finishes (with or without error), this method returns given error value.
+func (m *MultiClient) runWithPrimaryClient(origCtx context.Context, fn func(newCtx context.Context, primary kvclient) error) error {


Actually can we make the requirements laxer? Instead of the whole inProgress and cancellation, can we just run the function against the primary client and forget about it changing midway?

I wonder what the worst case scenario is then.

WatchKey and WatchPrefix operations are long-running, basically designed to never return (eg. see usage in pkg/ring/ring.go:155 or pkg/distributor/ha_tracker.go:144). So we need to either preserve that behaviour (what this PR is trying to do), or modify clients to restart if it returns early.

Get and CAS are short operations, and switching midway isn't necessary there.

gouthamve · 2019-11-06T04:57:08Z

pkg/ring/kv/multi.go

+}
+
+// watchChanges performs mirroring -- it watches changes in the primary client, and puts them into secondary.
+func (m *MultiClient) watchChanges(prefix string) {


Alternatively, what about writing to both clients but the reads go off only primary? Shouldn't that achieve the same?

I think it would achieve the same, at the cost of making CAS slower for clients. But that is probably just fine. (There is question of what to do when CAS to primary succeeds, but CAS to secondary fails, but the only reasonable thing we can do is to ignore it... we should not call f again)

pkg/cortex/runtime_config.go

pkg/ring/kv/client_test.go

pkg/ring/kv/multi.go

pstibrany · 2019-11-06T10:54:46Z

So my big comment would be around simplifying the entire thing to be: Write to both, read from primary. Also, forget about inflight ops, just do it on the primary client when we initiated the call.

I've changed functionality to follow this logic: writes via CAS function are now mirrored (if enabled) to secondary store, Get always goes to primary. WatchKey and WatchPrefix are left as they were (explanation why).

I've removed mirroring goroutine and rate limiting, and added metrics. I've also introduced new config value, MirrorTimeout, used when forwarding write to secondary store. (I've observed very long delays when doing Consul -> Etcd mirroring and Etcd was down -- over minute, and ingester got to Unhealthy state.)

Tons of changes have been made after and this requires another look.

jtlisi

LGTM

The KV code was much clearer than the last time I reviewed this PR. The logic around the KV store looks good and has good associated metrics/logs.

I want to note I would prefer if the OverridesManager and associated code lived in it's own package. We don't have a strong style guide around these types of things as a project. However, since a significant part of Cortex is going to be consumed by downstream projects I think it is important to maintain well named, clearly defined packages throughout the code base.

pkg/cortex/cortex.go

pkg/util/override.go

pstibrany · 2019-11-18T10:29:29Z

LGTM

The KV code was much clearer than the last time I reviewed this PR. The logic around the KV store looks good and has good associated metrics/logs.

I want to note I would prefer if the OverridesManager and associated code lived in it's own package. We don't have a strong style guide around these types of things as a project. However, since a significant part of Cortex is going to be consumed by downstream projects I think it is important to maintain well named, clearly defined packages throughout the code base.

Thanks @jtlisi for your review.

"much cleaner" -- I think I mostly removed code from KV code since your last review. It's now not doing the "mirroring" as a background task, but updates values as part of CAS.

re OverridesManager: I've now moved it and renamed to runtime_config.Manager. Originally I didn't want to do big changes to it, as this PR is primarily about something else, but I also think this is a good change.

pracucci

Good job @pstibrany! I checked out the change set again - to catch up with last commits - and left few comments.

pkg/ring/kv/multi.go

pracucci

Thanks @pstibrany for addressing my feedback. LGTM (once tests will be fixed)!

pstibrany · 2019-11-28T09:41:13Z

I've added two small changes:

~~when server starts, we reload overrides after setting YAML defaults for config. Otherwise they would only be applied after next reload.~~ initialize YAML defaults for limits before starting runtimeconfig.Manager. This is to fix bug observed in production, when in first 10 seconds of ingester running, incorrect limits were applied.
Communicate configuration updates via channels: runtimeconfig.Manager no longer spawns new goroutine to tell Listener about new configuration, but sends value to channel. If there is no receiver, or channel buffer is full, update is discarded.

pracucci

👍

pstibrany · 2019-12-05T08:34:49Z

Updated CHANGELOG.md and documented runtime config section and updated format of overrides/runtime config yaml file.

gouthamve

Thanks for all this @pstibrany! LGTM!

docs/arguments.md

pkg/ring/kv/multi.go

gouthamve · 2020-01-06T19:36:14Z

pkg/util/runtimeconfig/manager.go

+		}
+		go mgr.loop()
+	} else {
+		level.Info(util.Logger).Log("msg", "config disabled")


rutime config disabled.

what about "runtime config file not specified, reload disabled" to explain "why" as well.

or rather runtime config disabled: file not specified

This client is configured with multiple stores, one of them is designated as primary. All client operations are forwarded to the primary store. MultiClient also does "mirroring" of values from primary to secondary store. MultiClient can listen on changes in runtime configuration (via overrides mechanism), and switch primary store and enable/disable mirroring. Signed-off-by: Peter Štibraný <[email protected]>

Signed-off-by: Peter Štibraný <[email protected]>

Mirroring goroutine was removed, and replaced by forwarding writes done via CAS function to secondary client. Rate limits config was removed, but there is now timeout for secondary write, to avoid blocking CAS function for too long, if secondary write is slow (eg. etcd being down can cause very long writes). Only WatchKey and WatchPrefix functions now react on change of primary client. Signed-off-by: Peter Štibraný <[email protected]>

Signed-off-by: Peter Štibraný <[email protected]>

Without watch-and-mirror functionality, there is no need to check if value is already present in the secondary store. Signed-off-by: Peter Štibraný <[email protected]>

Signed-off-by: Peter Štibraný <[email protected]>

…mes, removed forgotten log. Signed-off-by: Peter Štibraný <[email protected]>

Signed-off-by: Peter Štibraný <[email protected]>

Instead of spawning new goroutine for each config update, we now use channels to communicate config updates. Signed-off-by: Peter Štibraný <[email protected]>

Signed-off-by: Peter Štibraný <[email protected]>

Addressed other review feedback. Signed-off-by: Peter Štibraný <[email protected]>

pstibrany mentioned this pull request Oct 23, 2019

Come up with a way of migrating from consul to etcd. #1525

Closed

tomwilkie requested review from gouthamve and jtlisi October 29, 2019 09:11

jtlisi suggested changes Oct 29, 2019

View reviewed changes

pkg/ring/kv/multi.go Outdated Show resolved Hide resolved

pkg/ring/kv/multi.go Outdated Show resolved Hide resolved

pkg/ring/kv/multi.go Outdated Show resolved Hide resolved

rfratto reviewed Oct 29, 2019

View reviewed changes

pkg/cortex/modules.go Outdated Show resolved Hide resolved

pkg/ring/kv/multi.go Outdated Show resolved Hide resolved

pkg/ring/kv/multi.go Outdated Show resolved Hide resolved

pracucci reviewed Oct 29, 2019

View reviewed changes

pracucci reviewed Oct 30, 2019

View reviewed changes

pkg/ring/kv/multi.go Outdated Show resolved Hide resolved

pkg/ring/kv/multi.go Outdated Show resolved Hide resolved

jtlisi previously approved these changes Oct 30, 2019

View reviewed changes

sandeepsukhani suggested changes Nov 1, 2019

View reviewed changes

pkg/util/validation/limits.go Show resolved Hide resolved

pracucci approved these changes Nov 4, 2019

View reviewed changes

pkg/ring/kv/multi.go Show resolved Hide resolved

pkg/ring/kv/client.go Outdated Show resolved Hide resolved

pkg/ring/kv/multi.go Outdated Show resolved Hide resolved

gouthamve reviewed Nov 6, 2019

View reviewed changes

pracucci reviewed Nov 6, 2019

View reviewed changes

pkg/ring/kv/multi.go Outdated Show resolved Hide resolved

pkg/ring/kv/multi.go Show resolved Hide resolved

gouthamve requested a review from jtlisi November 10, 2019 14:36

jtlisi approved these changes Nov 15, 2019

View reviewed changes

pkg/cortex/cortex.go Outdated Show resolved Hide resolved

pkg/util/override.go Outdated Show resolved Hide resolved

pracucci reviewed Nov 26, 2019

View reviewed changes

pracucci approved these changes Nov 26, 2019

View reviewed changes

tomwilkie requested a review from gouthamve November 26, 2019 14:51

pracucci approved these changes Nov 28, 2019

View reviewed changes

gouthamve approved these changes Jan 6, 2020

View reviewed changes

pstibrany added 3 commits January 7, 2020 19:38

Use Stop, which is now part of kv.Client.

1b2f37a

Signed-off-by: Peter Štibraný <[email protected]>

Put back setting of defaultLimits -- used when loading YAML files.

6dc84fc

Signed-off-by: Peter Štibraný <[email protected]>

pstibrany added 28 commits January 7, 2020 19:38

Moved setting of default limits for YAML unmarshal to separate function.

50c013c

Signed-off-by: Peter Štibraný <[email protected]>

Pass multi-client context as argument.

0fa6cb2

Signed-off-by: Peter Štibraný <[email protected]>

watchConfigChannel now reacts on context being done as well

e2efe9e

Signed-off-by: Peter Štibraný <[email protected]>

Changed Mirroring to *bool.

56491b2

Signed-off-by: Peter Štibraný <[email protected]>

Ignore mock by yaml.

364bc9c

Signed-off-by: Peter Štibraný <[email protected]>

Renamed mirroring to mirror-enabled to be consistent with MultiConfig.

2d4f1c0

Signed-off-by: Peter Štibraný <[email protected]>

Renamed 'multi' to 'multi_kv_config' in overrides.yaml.

9665933

Signed-off-by: Peter Štibraný <[email protected]>

Added metrics to multi client.

d91a5a7

Signed-off-by: Peter Štibraný <[email protected]>

Removed equality check when writing to secondary store.

43ec606

Without watch-and-mirror functionality, there is no need to check if value is already present in the secondary store. Signed-off-by: Peter Štibraný <[email protected]>

Renamed OverridesManager and moved it to its own package.

9bbea9b

Signed-off-by: Peter Štibraný <[email protected]>

Make lint happy, 3.

8c2f557

Signed-off-by: Peter Štibraný <[email protected]>

Make lint happy, 4.

d7f7f92

Signed-off-by: Peter Štibraný <[email protected]>

Add metric type to variable names, yaml name changes, fixed metric na…

df3c6e0

…mes, removed forgotten log. Signed-off-by: Peter Štibraný <[email protected]>

Fixed yet one more yaml name.

103dbaf

Signed-off-by: Peter Štibraný <[email protected]>

Fixed tests after changing yaml fields.

7de6ad8

Signed-off-by: Peter Štibraný <[email protected]>

Fix bug when default limits are not applied until next overrides reload.

63d566f

Signed-off-by: Peter Štibraný <[email protected]>

Ignore LoadConfig if LoadPath is empty.

666e17e

Signed-off-by: Peter Štibraný <[email protected]>

Use channels to communicate config updates.

c83361b

Instead of spawning new goroutine for each config update, we now use channels to communicate config updates. Signed-off-by: Peter Štibraný <[email protected]>

Initialize limits before starting runtimeconfig Manager.

5714d21

Signed-off-by: Peter Štibraný <[email protected]>

Updated CHANGELOG.md and arguments.md.

ecfd16d

Signed-off-by: Peter Štibraný <[email protected]>

Typo

e5ba1db

Signed-off-by: Peter Štibraný <[email protected]>

Fix compilation error in ingester_v2_test.go.

7245d4d

Signed-off-by: Peter Štibraný <[email protected]>

Fixed error after rebase.

b16a010

Signed-off-by: Peter Štibraný <[email protected]>

Fixed error after rebase.

45237f8

Signed-off-by: Peter Štibraný <[email protected]>

Use logger with component="multikv" to log messages.

ff24854

Signed-off-by: Peter Štibraný <[email protected]>

Improve log message when runtime config file is not specified.

713619a

Signed-off-by: Peter Štibraný <[email protected]>

Don't use memberlist in the example, as it is still experimental.

cc476c6

Addressed other review feedback. Signed-off-by: Peter Štibraný <[email protected]>

gouthamve merged commit b43bd74 into cortexproject:master Jan 7, 2020

pstibrany mentioned this pull request Jan 20, 2020

Prevents panic using freed slice in pool #2000

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Added KV Store client that mirrors one backend to another #1749

Added KV Store client that mirrors one backend to another #1749

pstibrany commented Oct 23, 2019 •

edited

Loading

pstibrany commented Oct 24, 2019

pstibrany commented Oct 25, 2019

jtlisi left a comment

rfratto left a comment

pracucci left a comment

jtlisi left a comment

pstibrany commented Nov 1, 2019

sandeepsukhani left a comment

pracucci left a comment

gouthamve left a comment

gouthamve Nov 6, 2019

pstibrany Nov 6, 2019

gouthamve Nov 6, 2019

pstibrany Nov 6, 2019

pstibrany commented Nov 6, 2019 •

edited

Loading

jtlisi left a comment •

edited

Loading

pstibrany commented Nov 18, 2019

pracucci left a comment

pracucci left a comment •

edited

Loading

pstibrany commented Nov 28, 2019 •

edited

Loading

pracucci left a comment

pstibrany commented Dec 5, 2019

gouthamve left a comment

gouthamve Jan 6, 2020

pstibrany Jan 7, 2020

pstibrany Jan 7, 2020

Added KV Store client that mirrors one backend to another #1749

Added KV Store client that mirrors one backend to another #1749

Conversation

pstibrany commented Oct 23, 2019 • edited Loading

pstibrany commented Oct 24, 2019

pstibrany commented Oct 25, 2019

jtlisi left a comment

Choose a reason for hiding this comment

rfratto left a comment

Choose a reason for hiding this comment

pracucci left a comment

Choose a reason for hiding this comment

jtlisi left a comment

Choose a reason for hiding this comment

pstibrany commented Nov 1, 2019

sandeepsukhani left a comment

Choose a reason for hiding this comment

pracucci left a comment

Choose a reason for hiding this comment

gouthamve left a comment

Choose a reason for hiding this comment

gouthamve Nov 6, 2019

Choose a reason for hiding this comment

pstibrany Nov 6, 2019

Choose a reason for hiding this comment

gouthamve Nov 6, 2019

Choose a reason for hiding this comment

pstibrany Nov 6, 2019

Choose a reason for hiding this comment

pstibrany commented Nov 6, 2019 • edited Loading

jtlisi left a comment • edited Loading

Choose a reason for hiding this comment

pstibrany commented Nov 18, 2019

pracucci left a comment

Choose a reason for hiding this comment

pracucci left a comment • edited Loading

Choose a reason for hiding this comment

pstibrany commented Nov 28, 2019 • edited Loading

pracucci left a comment

Choose a reason for hiding this comment

pstibrany commented Dec 5, 2019

gouthamve left a comment

Choose a reason for hiding this comment

gouthamve Jan 6, 2020

Choose a reason for hiding this comment

pstibrany Jan 7, 2020

Choose a reason for hiding this comment

pstibrany Jan 7, 2020

Choose a reason for hiding this comment

pstibrany commented Oct 23, 2019 •

edited

Loading

pstibrany commented Nov 6, 2019 •

edited

Loading

jtlisi left a comment •

edited

Loading

pracucci left a comment •

edited

Loading

pstibrany commented Nov 28, 2019 •

edited

Loading