Move the ring package from cortex to dskit #45

treid314 · 2021-09-23T21:31:53Z

What this PR does: Adds the ring package from cortex to dskit. Also included is the proto package with ParseProtoReader function.

Which issue(s) this PR fixes:

Checklist

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

instead of spinning round the loop as fast as possible

This makes the codebase work better together with the Prometheus components which expect an injected go-kit logger, and also makes things more consistent overall. I'm introducing a Cortex-wide shared logger for now so that this change doesn't become even bigger. Eventually, it would be good to change all Cortex components to take injected loggers.

* Replace four backoff implementations with common one

* Remove unused WatchPrefix() * Break out of backoff loop if Context is terminated Previously we would only stop if the Context signalled cancellation while we were in a sleep. Now, loops that check `backoff.Ongoing()` will end if the Context is terminated any time. A new `backoff.Err()` function is added to report back the reason for termination

* Adjust log levels Things that are normal occurences are just debugs. * Don't close stream until we're sure we've got the chunks This fixes a race condition in `TestIngesterTransfer`. I don't think it makes the actually-running-in-production situation any worse, and might even make it better. * Log result of `TransferChunks`

If we don't emit anything then Prometheus takes the last value until it is stale, i.e. for 5 minutes.

* Unify the replication calculation between query and push. * Replace 'circle' with 'ring'. * Deal with ingesters coming / going better.

* Move ingester_lifecycle.go to ring pkg, remove ingester_ prefix from other files in pkg/ingester. * Decouples the lifecycle of the ingester from the ingester itself, and put it into its own struct. Laying groundwork for the incremental transfer of token, to unify rolling upgrades and elastic scaling. * s/Transfer/TransferOut/ Signed-off-by: Tom Wilkie <tom.wilkie@gmail.com>

* Add client-side request metrics for consul. Signed-off-by: Tom Wilkie <tom.wilkie@gmail.com> * s/Put/List/ Signed-off-by: Tom Wilkie <tom.wilkie@gmail.com> * Update github.com/hashicorp/consul Signed-off-by: Tom Wilkie <tom.wilkie@gmail.com> * Add contexts to the consul api so we can get traces of heartbeats. Signed-off-by: Tom Wilkie <tom.wilkie@gmail.com> * Moar context. Signed-off-by: Tom Wilkie <tom.wilkie@gmail.com> * Missed one. Signed-off-by: Tom Wilkie <tom.wilkie@gmail.com> * Make consul consistent reads optional. Signed-off-by: Tom Wilkie <tom.wilkie@gmail.com>

* Remove bazel. Signed-off-by: Tom Wilkie <tom.wilkie@gmail.com> * Review feedback. Signed-off-by: Tom Wilkie <tom.wilkie@gmail.com>

* allow specifying ingester port for registration in case it gets transformed * use "advertise" for clarity of description * default the advertised port to grpc-listen-port instead of 9095 so that overrides of grpc port work

Should reduce CPU load on ingesters & network load as chunks as compressed. Signed-off-by: Tom Wilkie <tom.wilkie@gmail.com>

* Also make sure the percentage is exposed for ingesters that are not having any tokens Signed-off-by: Goutham Veeramachaneni <gouthamve@gmail.com>

Signed-off-by: Goutham Veeramachaneni <gouthamve@gmail.com>

* Delay more than the minimum ingester queries If one of the initial queries fails, or takes a long time, then the extra queries will be executed. Signed-off-by: Chris Marchbanks <csmarchbanks@gmail.com>

As it says in the comment on `time.After`: // The underlying Timer is not recovered by the garbage collector // until the timer fires. If efficiency is a concern, use NewTimer // instead and call Timer.Stop if the timer is no longer needed.

* Remove leftover build.BAZEL files * Add test-exporter to .gitignore * Rename weaveworks/cortex to cortexproject/cortex everywhere Signed-off-by: Chris Marchbanks <csmarchbanks@gmail.com>

Signed-off-by: Tom Wilkie <tom.wilkie@gmail.com>

This should remove the repetition of the ingester ID - a ~25 character string, repeated 128 times (3kb) for each ingester, allocated every 5 seconds. For a 10 ingester cluster, thats 60KB/s of allocations (1/5s * 10 ingesters * 128 tokens * 10 ingester). Yes, its N^2. Signed-off-by: Tom Wilkie <tom.wilkie@gmail.com>

Signed-off-by: Tom Wilkie <tom.wilkie@gmail.com>

This allows us to start Cortex on the Mac. Signed-off-by: Tom Wilkie <tom.wilkie@gmail.com>

Signed-off-by: Tom Wilkie <tom.wilkie@gmail.com>

…entation as the ring does. Also, remove the two instances of KVClient mock hooks. Signed-off-by: Tom Wilkie <tom.wilkie@gmail.com>

Signed-off-by: Tom Wilkie <tom.wilkie@gmail.com>

New version should be slightly more efficient as it avoids creating an object on every call. Signed-off-by: Bryan Boreham <bryan@weave.works>

This is to ensure we can set it to 0 in single node mode Signed-off-by: Goutham Veeramachaneni <gouthamve@gmail.com>

Signed-off-by: Tyler Reid <tyler.reid@grafana.com>

treid314 · 2021-09-23T21:32:30Z

There's still a couple of things that need tidying up regarding package visibility and some other odds and ends. So I'm leaving this as a draft for the time being.

treid314 · 2021-09-27T15:35:44Z

ring/lifecycler.go

+
+	hostname, err := os.Hostname()
+	if err != nil {
+		level.Error(log.NewNopLogger()).Log("msg", "failed to get hostname", "err", err)


This was originally using the (util_logger) global logger, but now we don't wish to do that. I could add a logger to the config, but I'm not sure if that's the best path here. Right now I just create a new no-op logger which preserves the line, but doesn't log. Is there a system logger or a generic logger I could use in go? Or could I return an error and have that be uncaught since we send an os exit after this log?

I think you're right, we definitely don't want os.Exit in library code. A panic feels fine in the case we can't look up the hostname of the machine this code is on. That seems like something that Should Never Fail.

Not sure how to deal with this, but using the log.NewNopLogger() is useless.

I took @56quarters suggestion and used a panic here.

pracucci

You did a good job! I diffed all files with their Cortex counterparts and left few comments here and there. I didn't comment on metrics with cortex_ prefix because I would suggest to get this PR merged as soon as possible (with the least changes) and then work on removing the cortex_ prefix in a follow up PR.

However, we need a CI step to make sure compiled protos are updated (see check-protos target in Cortex Makefile for reference).

ring/http.go

pracucci · 2021-09-28T18:47:53Z

ring/http.go

+	if req.Method == http.MethodPost {
+		ingesterID := req.FormValue("forget")
+		if err := r.forget(req.Context(), ingesterID); err != nil {
+			level.Error(r.logger).Log("msg", "error forgetting instance", "err", err)


Suggested change

level.Error(r.logger).Log("msg", "error forgetting instance", "err", err)

level.Error(log.WithContext(req.Context(), log.Logger)).Log("msg", "error forgetting instance", "err", err)

This is waiting on dslog/wrappers in #42 https://github.com/grafana/dskit/pull/42/files#diff-81c5fa15cb0fdbd5f36ef1f9711920c590bb2bacca88040102dfda58e80d2a48R32

ring/http.go

ring/lifecycler.go

ring/ring.proto

ring/tokens.go

ring/shard/yolo.go

ring/testutils/testutils.go

proto/http.go

Signed-off-by: Tyler Reid <tyler.reid@grafana.com>

…d by ring

called after all workers finish, which is what we want. (It is now also called on early error exit.)

Signed-off-by: Tyler Reid <tyler.reid@grafana.com>

pracucci

LGTM, let's merge it! We can add the check-protos linter in a follow up PR (but please do it 🙏 )

bboreham and others added 30 commits September 1, 2021 11:36

In WatchKey(), back-off when value not found (grafana#560)

0dcfcd9

instead of spinning round the loop as fast as possible

Unify four backoff implementations (#615)

3279063

* Replace four backoff implementations with common one

Report 0 metric for all ring states (#737)

a99c41e

If we don't emit anything then Prometheus takes the last value until it is stale, i.e. for 5 minutes.

Unify the replication calculation between query and push. (#681)

8a1ca4e

* Unify the replication calculation between query and push. * Replace 'circle' with 'ring'. * Deal with ingesters coming / going better.

allow specifying an ACL token for consul interaction (#894)

6c7e252

Remove bazel. (#903)

fb3cbeb

* Remove bazel. Signed-off-by: Tom Wilkie <tom.wilkie@gmail.com> * Review feedback. Signed-off-by: Tom Wilkie <tom.wilkie@gmail.com>

allow specifying ingester port for registration (#896)

bd74b8c

* allow specifying ingester port for registration in case it gets transformed * use "advertise" for clarity of description * default the advertised port to grpc-listen-port instead of 9095 so that overrides of grpc port work

Stream chunks from ingester to querier on queries.

a324583

Should reduce CPU load on ingesters & network load as chunks as compressed. Signed-off-by: Tom Wilkie <tom.wilkie@gmail.com>

Expose the number of tokens owned by an ingester

736dc34

* Also make sure the percentage is exposed for ingesters that are not having any tokens Signed-off-by: Goutham Veeramachaneni <gouthamve@gmail.com>

Expose tokens owned and to own on the ingester end

5b96e48

Signed-off-by: Goutham Veeramachaneni <gouthamve@gmail.com>

Delay more than the minimum ingester queries (#978)

c9b02e0

* Delay more than the minimum ingester queries If one of the initial queries fails, or takes a long time, then the extra queries will be executed. Signed-off-by: Chris Marchbanks <csmarchbanks@gmail.com>

Switch imports (#1038)

da4839e

* Remove leftover build.BAZEL files * Add test-exporter to .gitignore * Rename weaveworks/cortex to cortexproject/cortex everywhere Signed-off-by: Chris Marchbanks <csmarchbanks@gmail.com>

Reduce number of pointers in the ring protos.

cb13a77

Signed-off-by: Tom Wilkie <tom.wilkie@gmail.com>

Fix bugs from test migration.

2c2ded7

Signed-off-by: Tom Wilkie <tom.wilkie@gmail.com>

Preallocated the slice of tokens.

b7ea5f4

Signed-off-by: Tom Wilkie <tom.wilkie@gmail.com>

Specify multiple interfaces to look for an address on.

07ac2dd

This allows us to start Cortex on the Mac. Signed-off-by: Tom Wilkie <tom.wilkie@gmail.com>

Add yaml tags to ring.Config.

569b013

Signed-off-by: Tom Wilkie <tom.wilkie@gmail.com>

Rename some config, so we can override it in YAML.

58c1a87

Signed-off-by: Tom Wilkie <tom.wilkie@gmail.com>

Make the ring lifecycler use the same code to pick the KSStore implem…

225ecc5

…entation as the ring does. Also, remove the two instances of KVClient mock hooks. Signed-off-by: Tom Wilkie <tom.wilkie@gmail.com>

Allow setting interface names via yaml.

761d9de

Signed-off-by: Tom Wilkie <tom.wilkie@gmail.com>

Allow overiding of the advertised address in yaml.

cb40e6a

Signed-off-by: Tom Wilkie <tom.wilkie@gmail.com>

Update instrumentation calls to remove deprecated interface

2532b86

New version should be slightly more efficient as it avoids creating an object on every call. Signed-off-by: Bryan Boreham <bryan@weave.works>

Make minReadyDuration configurable

374a096

This is to ensure we can set it to 0 in single node mode Signed-off-by: Goutham Veeramachaneni <gouthamve@gmail.com>

Tyler Reid added 12 commits September 10, 2021 15:24

Fix comment grammar

a2bd0ed

Signed-off-by: Tyler Reid <tyler.reid@grafana.com>

Merge remote-tracking branch 'upstream/main' into util-http

72b91d4

Signed-off-by: Tyler Reid <tyler.reid@grafana.com>

Review changes

a568d0d

Signed-off-by: Tyler Reid <tyler.reid@grafana.com>

Merge remote-tracking branch 'upstream/main' into dskit-move-ring

a00693e

start migration and making ring generic

aea2884

Signed-off-by: Tyler Reid <tyler.reid@grafana.com>

Fix linter error

48e693a

Signed-off-by: Tyler Reid <tyler.reid@grafana.com>

WIP more ring changes

1f62366

Merge branch 'util-shard' into dskit-move-ring

2cc4be3

Add in shard utils

a925878

Merge branch 'util-http' into dskit-move-ring

a69f731

Add http functions and other various fixes to make ring generic

2e9809b

Signed-off-by: Tyler Reid <tyler.reid@grafana.com>

General lint and code cleaning

ae1a749

Signed-off-by: Tyler Reid <tyler.reid@grafana.com>

treid314 requested review from aknuds1, pstibrany, pracucci and bboreham September 23, 2021 21:31

treid314 commented Sep 27, 2021

View reviewed changes

pracucci reviewed Sep 28, 2021

View reviewed changes

Tyler Reid added 5 commits September 28, 2021 15:56

review comments pt1

90780c1

Signed-off-by: Tyler Reid <tyler.reid@grafana.com>

tidy mods

fa7c2ef

move stringContains to a new package to prevent it from being exporte…

e870630

…d by ring

Move the cancel() call to the DoBatch cleanup function; this is

4572320

called after all workers finish, which is what we want. (It is now also called on early error exit.)

Use panic instead of os.Exit

6ec4a08

Signed-off-by: Tyler Reid <tyler.reid@grafana.com>

treid314 marked this pull request as ready for review September 28, 2021 23:11

Add changelog message

5c26e79

treid314 requested review from pracucci and 56quarters September 29, 2021 13:48

pracucci approved these changes Sep 29, 2021

View reviewed changes

treid314 merged commit d49be0b into grafana:main Sep 29, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Move the ring package from cortex to dskit #45

Move the ring package from cortex to dskit #45

treid314 commented Sep 23, 2021 •

edited

Loading

treid314 commented Sep 23, 2021

treid314 Sep 27, 2021

56quarters Sep 27, 2021 •

edited

Loading

pracucci Sep 28, 2021

treid314 Sep 28, 2021

pracucci left a comment

pracucci Sep 28, 2021

treid314 Sep 28, 2021

pracucci left a comment

	level.Error(r.logger).Log("msg", "error forgetting instance", "err", err)
	level.Error(log.WithContext(req.Context(), log.Logger)).Log("msg", "error forgetting instance", "err", err)

Move the ring package from cortex to dskit #45

Move the ring package from cortex to dskit #45

Conversation

treid314 commented Sep 23, 2021 • edited Loading

treid314 commented Sep 23, 2021

treid314 Sep 27, 2021

Choose a reason for hiding this comment

56quarters Sep 27, 2021 • edited Loading

Choose a reason for hiding this comment

pracucci Sep 28, 2021

Choose a reason for hiding this comment

treid314 Sep 28, 2021

Choose a reason for hiding this comment

pracucci left a comment

Choose a reason for hiding this comment

pracucci Sep 28, 2021

Choose a reason for hiding this comment

treid314 Sep 28, 2021

Choose a reason for hiding this comment

pracucci left a comment

Choose a reason for hiding this comment

treid314 commented Sep 23, 2021 •

edited

Loading

56quarters Sep 27, 2021 •

edited

Loading