Faster rolling upgrades. #325

tomwilkie · 2017-03-09T13:50:52Z

Fixes #321

It works! And its quick...

TODO

move ingester lifecycle code into ingester, work out the coupling a bit better
move ingester shutdown code into ingester (from main.go)
fallback to flush it transfer fails.
naming
check ring logic for new ingester states
~~ensure ingester flushing stops before transferring chunks~~ - shouldn't do this, as we don't want chunks that won't flush to block the transfer.
~~add an ever-extending timeout on the receiving ingester, when we do timeout auto-join~~ - going to leave this for now, can always kill ingesters if they get stuck, will fall back to flushing.

tomwilkie · 2017-03-09T19:30:29Z

cortex.proto

+  // Claim allows leaving ingester to stream chunks directly to joining ingesters.
+  rpc ClaimStart(ClaimStartRequest) returns (ClaimStartResponse) {};
+  rpc ClaimStream(stream TimeSeriesChunk) returns (ClaimStreamResponse) {};
+  rpc ClaimFinish(ClaimFinishRequest) returns (ClaimFinishResponse) {};


Rename to Transfer{StartStreamFinish}?

Also, can the {Start,Finish} methods be avoided?

tomwilkie · 2017-03-09T19:33:49Z

ingester/ingester_claim.go

+	return &cortex.ClaimFinishResponse{}, nil
+}
+
+func toWireChunks(descs []*desc) ([]cortex.Chunk, error) {


Move to util/compat.go

Nah, uses the ingester/desc type which I want to keep internal.

jml

Thanks. Really cool. There are enough changes & comments that I won't LGTM, but I don't anticipate problems.

jml · 2017-03-10T16:09:25Z

cortex.proto

@@ -13,6 +13,9 @@ service Ingester {
  rpc LabelValues(LabelValuesRequest) returns (LabelValuesResponse) {};
  rpc UserStats(UserStatsRequest) returns (UserStatsResponse) {};
  rpc MetricsForLabelMatchers(MetricsForLabelMatchersRequest) returns (MetricsForLabelMatchersResponse) {};
+
+  // TransferChunks allows leaving ingester to stream chunks directly to joining ingesters.


Say which of the two is supposed to call this.

jml · 2017-03-10T16:11:12Z

distributor/distributor.go

@@ -88,7 +80,7 @@ type Config struct {
 	IngestionBurstSize  int

 	// for testing
-	ingesterClientFactory func(string) cortex.IngesterClient
+	ingesterClientFactory func(string, time.Duration) (cortex.IngesterClient, error)


Please update comment say what string and time.Duration represent

jml · 2017-03-10T16:20:58Z

ingester/ingester.go

+	ringConfig       ring.Config
+	userStatesConfig UserStatesConfig
+
+	// Config for the ingester lifecycle contol


jml · 2017-03-10T16:21:12Z

ingester/ingester.go

+	ChunkEncoding     string
+
+	// For testing
+	Addr           string


Addr of what?

jml · 2017-03-10T16:21:25Z

ingester/ingester.go

+	userStatesConfig UserStatesConfig
+
+	// Config for the ingester lifecycle contol
+	ListenPort      *int


Why is this a pointer?

Its shared with the server config - see https://github.com/weaveworks/cortex/blob/master/cmd/ingester/main.go#L33

jml · 2017-03-10T17:33:36Z

ring/model.go

@@ -56,3 +58,39 @@ func (d *Desc) removeIngester(id string) {
 	}
 	d.Tokens = output
 }
+
+// ClaimTokens transfters all the tokens from one ingester to another.


"returning the claimed tokens"

jml · 2017-03-10T17:33:50Z

ring/model.go

+	return result
+}
+
+// FindIngestersByState returns the list of ingesterd in the given state


jml · 2017-03-10T17:34:42Z

ingester/util.go

+func (ts sortableUint32) Less(i, j int) bool { return ts[i] < ts[j] }
+
+// getFirstAddressOf returns the first IPv4 address of the supplied interface name.
+func getFirstAddressOf(name string) (string, error) {


Please remove the implementation that's in alertmanager.

They do different things - alertmangers gets the first non-empty address, this gets the address for a given interface. I'll file a ticket to reconcile them later.

jml · 2017-03-10T17:36:35Z

ingester/ingester_lifecycle_test.go

+		ingester, err := New(config, nil)
+		if err != nil {
+			t.Fatal(err)
+		}


Can we use testify, so we can spelling this require.NoError(err)?

Thanks! If you're really keen, you can also import assert from testify and use its Equal function to replace some of the DeepEqual business.

jml · 2017-03-10T17:37:02Z

ingester/ingester_lifecycle_test.go

+	}
+}
+
+func TestIngesterRestart(t *testing.T) {


What about restart are we testing? (Please add comment)

…g ingesters.

jml · 2017-03-13T13:35:04Z

ingester/ingester_claim.go

+
+		// We can't send "extra" fields with a streaming call, so we repeat
+		// wireSeries.FromIngesterId and assume it is the same every time
+		// tound this loop.


jml · 2017-03-13T13:36:02Z

ingester/ingester_lifecycle.go

+	return i.ready
+}
+
+// ChangeState of the ingester, for use off the loop() goroutine.


FWIW, "by" or "from" are probably better than the "off of the", but it's such an incredibly minor point.

jml · 2017-03-13T13:47:11Z

I missed this. Sorry, I don't have a better suggestion. No strong objections to `c` but don't think it's a huge improvement either. No big deal.

…

On Mon, 13 Mar 2017 at 11:34 Tom Wilkie ***@***.***> wrote: ***@***.**** commented on this pull request. ------------------------------ In ingester/ingester_lifecycle.go <#325 (comment)>: > + } + + ringDesc, err := i.consul.Get(ring.ConsulKey) + if err != nil { + log.Error("Error talking to consul: %v", err) + return false + } + + i.ready = i.ready || ringDesc.(*ring.Desc).Ready(i.cfg.ringConfig.HeartbeatTimeout) + return i.ready +} + +// ChangeState of the ingester, for use off the loop() goroutine. +func (i *Ingester) ChangeState(state ring.IngesterState) error { + err := make(chan error) + i.actorChan <- func() { better suggestion? c? — You are receiving this because you commented. Reply to this email directly, view it on GitHub <#325 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAHq6kpNP4rQyYFxmDpvUgURwNKKvlyGks5rlSmzgaJpZM4MYGwP> .

tomwilkie self-assigned this Mar 9, 2017

tomwilkie force-pushed the 321-faster-rolling-upgrade branch from f06ac7b to 2c1a25c Compare March 9, 2017 14:24

tomwilkie commented Mar 9, 2017

View reviewed changes

tomwilkie changed the title ~~[WIP] Faster rolling upgrades.~~ Faster rolling upgrades. Mar 10, 2017

tomwilkie force-pushed the 321-faster-rolling-upgrade branch from 4f81de5 to 587f34a Compare March 10, 2017 14:16

jml reviewed Mar 10, 2017

View reviewed changes

Faster rolling upgrades by transferring chunks from leaving to joinin…

7eb0723

…g ingesters.

tomwilkie mentioned this pull request Mar 13, 2017

Consolidate util.GetFirstAddressOf and alertmanger.mustHardwareAddr #343

Closed

Review feedback

b14c4a5

tomwilkie force-pushed the 321-faster-rolling-upgrade branch from 8113f83 to b14c4a5 Compare March 13, 2017 11:58

tomwilkie added 2 commits March 13, 2017 12:04

Last comment

4ae67e0

Typo

7a6c8c5

tomwilkie removed their assignment Mar 13, 2017

jml reviewed Mar 13, 2017

View reviewed changes

jml approved these changes Mar 13, 2017

View reviewed changes

Review feedback

37995b7

tomwilkie merged commit bdbd0f5 into master Mar 13, 2017

tomwilkie deleted the 321-faster-rolling-upgrade branch March 13, 2017 14:58

bboreham mentioned this pull request Aug 8, 2019

Remove claim-on-rollout option #1566

Merged

Faster rolling upgrades. #325

Faster rolling upgrades. #325

Uh oh!

Conversation

tomwilkie commented Mar 9, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jml left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jml commented Mar 13, 2017 via email

Uh oh!

Uh oh!

tomwilkie commented Mar 9, 2017 •

edited

Loading