memberlist: reresolve members with every 100 nodes on Join #411
Conversation
This change solves three problems: 1. Context cancelations aren't respected in Join. Sometimes join might take as long as 25 minutes and there is no way to cancel it. Now we check the context on every 100 joined nodes 2. `Join` will attempt to join member IPs that become obsolete between the time discoverMembers runs and memberlist initiates a push-pull with the node. Reresolve `JoinMembers` addresses after joining every 100 nodes. This helps to clean up obsolete IPs that will be tried at the end of the Join procedure 3. (minor) fast joining on startup doesn't respect context cancelations The side effect of this change is that there will be more DNS resolutions for the JoinMembers. This is in draft beacuse
dimitarvdimitrov
force-pushed
the
dimitar/memberlist-reresolve-members-on-join
branch
from
a16c50b
to
29aae25
Compare
|
thank you for the review @pracucci. I think I addressed your comments. I decided to go with 3 functions: 1) manage retries, 2) manage batches, 3) join a single batch. The code become a bit longer, but I think it's more clear |
| if attemptedNodes[n] { | ||
| continue | ||
| } | ||
| if len(batch) >= batchSize { |
There was a problem hiding this comment.
nit: should we move this if after batch = append(batch, n)?
There was a problem hiding this comment.
then we don't know whether there are moreAvailableNodes. If we have filled the batch size and there is another node which wasn't attempted already, then we know. This is covering the edge case where the batch fills with the last item in nodes
kv/memberlist/memberlist_client.go
Outdated
| if ctx.Err() != nil { | ||
| return successfullyJoined, fmt.Errorf("joining batch: %w", context.Cause(ctx)) | ||
| } | ||
| // Attempt to join a single node. Complexity shouldn't be different from passing all the node IPs to Join. |
There was a problem hiding this comment.
Complexity shouldn't be different from passing all the node IPs to Join.
What is this comment trying to say?
There was a problem hiding this comment.
that it doesn't matter whether we call Join with all nodes once or call it with one node N times. I reworded the comment
| level.Error(m.logger).Log("msg", "joining memberlist cluster failed", "last_error", lastErr, "elapsed_time", time.Since(startTime)) | ||
| return false | ||
| // joinMembersBatch returns an error only if it couldn't successfully join any nodes or if ctx is cancelled. | ||
| func (m *KV) joinMembersBatch(ctx context.Context, nodes []string) (successfullyJoined int, lastErr error) { |
There was a problem hiding this comment.
suggestion: joinMembersBatch -> joinAllMembers
There was a problem hiding this comment.
i prefer joinMembersBatch because it complements joinMembersInBatches and it's more clear that InBatches uses Batch
|
thanks for the reviews @pracucci, @pstibrany. I will merge this after CI passes |
|
there is a race condition with some printing of atomics in the test. I don't expect this was introduced by this PR. I will investigate it and push a fix. |
This prevents a race in test cases when a service is failed. The DescribeService method prints the service struct value which is detected as a data race when printing the mutex struct
|
i pushed a commit which gets rid of the data race. The race was there when a service was being described. The race detector treats printing a mutex field and using that mutex as a race condition. This can be circumvented by giving the memberlist kv a name, so it doesn't have to be printed. The change is in ab6e2d1 |
dimitarvdimitrov
force-pushed
the
dimitar/memberlist-reresolve-members-on-join
branch
from
14436a5
to
dbc716e
Compare
dimitarvdimitrov
deleted the
dimitar/memberlist-reresolve-members-on-join
branch
Description
This change solves three problems:
Joinwill attempt to join member IPs that become obsolete between the time discoverMembers runs and memberlist initiates a push-pull with the node. ReresolveJoinMembersaddresses after joining every 100 nodes. This helps to clean up obsolete IPs that will be tried at the end of the Join procedureThe side effect of this change is that there will be more DNS resolutions for the JoinMembers.
Note to reviewers:
TestJoinMembersWithRetryBackoffseems to sometimes fail locally, but it should be an unrelated failure. I believe it should be fixed by this #412.Checklist
CHANGELOG.mdupdated - the order of entries should be[CHANGE],[FEATURE],[ENHANCEMENT],[BUGFIX]