Repository navigation
[raft] Add and remove nodes of Raft groups - #1780
Conversation
bff9fbd to
55c6611
Compare
|
|
||
| cctx := confChangeContext{ | ||
| ID: uuid.NewString(), | ||
| URL: peerURL, |
There was a problem hiding this comment.
Isn't this information duplicated? I.e. once here in ConfChangeV2.Context, and once more in the inner ConfChangeSingle.Context?
There was a problem hiding this comment.
ConfChangeSingle does not have a Context field, only conf change type and node ID. Were you referring to something else ?
|
|
||
| err = c.publishEntries(entries, snapshotInterval) | ||
| if errors.Is(err, errSelfRemoved) { | ||
| c.logger.Warn("this node was removed from the Raft group, stopping") |
There was a problem hiding this comment.
Is this the appropriate course of action?
IIUC this means the routine will just stop but the process will keep living but doing nothing, and with no hope of recovering somehow. In that case we might as well panic?
But on the other hand, can we expect having the node being added back? If so we should probably be handling that. Probably by resetting the state to what it is right after the first start.
There was a problem hiding this comment.
Handling this is I think part of #1610. Panicking when an error is "expected" seems odd doesn't it ?
When the raft consumer loop exits, #1610 aims for a graceful stop but we'll have to consider that a node could be removed from one group but not the others (e.g. removed from rid but still active in aux and scd).
And no a node that has been removed cannot be added back as is, it would have to be re-added with another node ID.
There was a problem hiding this comment.
but we'll have to consider that a node could be removed from one group but not the others (e.g. removed from rid but still active in aux and scd)
Right, I haven't thought of that, but that actually make the issue worse IMO.
And no a node that has been removed cannot be added back as is, it would have to be re-added with another node ID.
So we actually have another issue: right now there is only one node ID passed to the CLI, to be shared to the 3 groups. So if the node is removed from one group, it actually cannot rejoin that group without leaving the other groups too. I think we need to decouple that by having a separate node ID per group instead.
That leads me to another question, why is the node ID passed by the CLI and not automatically generated?
If passed by the CLI, and if the node ID needs to be different when the node leaves the group, that means that the CLI argument needs to be changed for the node to rejoin the group. That's an issue too (c.f. discussion on deploying in k8s environment).
Aaand that leads me to yet another question: did we address the behavior of the raft node in a cluster environment? I.e. what happens when k8s spawns multiple pods? Wouldn't we have in that case multiple nodes with the same ID running?
There was a problem hiding this comment.
Those are good points, I'd like to take some time to reflect on this. I suggest we discuss the direction we want to take for #1802 live during tomorrow's sync, what do you think ?
There was a problem hiding this comment.
That's the wise thing to do :)
55c6611 to
c6e75a2
Compare
|
|
||
| err = c.publishEntries(entries, snapshotInterval) | ||
| if errors.Is(err, errSelfRemoved) { | ||
| c.logger.Warn("this node was removed from the Raft group, stopping") |
There was a problem hiding this comment.
but we'll have to consider that a node could be removed from one group but not the others (e.g. removed from rid but still active in aux and scd)
Right, I haven't thought of that, but that actually make the issue worse IMO.
And no a node that has been removed cannot be added back as is, it would have to be re-added with another node ID.
So we actually have another issue: right now there is only one node ID passed to the CLI, to be shared to the 3 groups. So if the node is removed from one group, it actually cannot rejoin that group without leaving the other groups too. I think we need to decouple that by having a separate node ID per group instead.
That leads me to another question, why is the node ID passed by the CLI and not automatically generated?
If passed by the CLI, and if the node ID needs to be different when the node leaves the group, that means that the CLI argument needs to be changed for the node to rejoin the group. That's an issue too (c.f. discussion on deploying in k8s environment).
Aaand that leads me to yet another question: did we address the behavior of the raft node in a cluster environment? I.e. what happens when k8s spawns multiple pods? Wouldn't we have in that case multiple nodes with the same ID running?
c6e75a2 to
0172b84
Compare
0172b84 to
75a8f28
Compare
9a6fef4 to
75ce248
Compare
75ce248 to
03ccad5
Compare
| if err != nil { | ||
| c.logger.Warn("rejecting config change", zap.String("id", cctx.ID), zap.Error(err)) | ||
| // A rejected change is treated as a no-op. | ||
| c.tracker.untrack(cctx.ID, ProposalResult{Error: err}) |
There was a problem hiding this comment.
May not be within scope of this PR, but this is making me wonder: IIUC we may end up with stale proposals, right? Because Node.Propose() may drop the proposal without returning an error (if I understand the doc correctly):
// Propose proposes that data be appended to the log. Note that proposals can be lost without
// notice, therefore it is user's job to ensure proposal retries.How do we deal with that?
There was a problem hiding this comment.
The dropped proposals gap is tracked by #1771 but not yet addressed.
This PR is a follow-up to #1775 and partially addresses #1719 by implementing the
Add/RemoveNodemethods for consensus. If removed, a leader steps down to allow for new elections.This completes the config change handling implementation on the consensus side. Follow-up PRs will implement the admin endpoint and CLI as well as the authentication.