Repository navigation
[raft] Add and remove nodes of Raft groups - #1780
MariemBaccari wants to merge 3 commits into
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
| m.members = map[uint64]string{} | ||
| } | ||
| m.removedIDs = removedIDs | ||
| m.Unlock() |
There was a problem hiding this comment.
Why not deferring? Is it acceptable for m.members and m.removedIDs to be out of sync?
In addition: if below an error is returned, they will be out of sync. We probably do not want that.
There was a problem hiding this comment.
The lock is only used to protect m.members and m.removedIDs after the unlock we only call the transport methods which only open and close streams. I don't see how m.members and m.removedIDs could become out of sync here. The idea behind not deferring is that since transport methods can take longer, we shouldn't hold the lock for longer than needed.
There was a problem hiding this comment.
Sorry, I meant m.members and m.removedIDs being out of sync with the peers in m.transport.
Isn't that an issue? E.g. if a node is added concurrently to being removed, I could see a race condition where the removal from the transport happens before its addition.
And if it is not: please put the rationale why it is not an issue.
There was a problem hiding this comment.
I just added a comment to explain why this is not an issue.
|
|
||
| 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
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.