Refactor node draining racing avoid condition #130
Conversation
| }, 3*time.Second, 3, true, ctx.Done()) | ||
|
|
||
| done := make(chan bool) | ||
| go dn.getDrainLock(ctx, done) |
There was a problem hiding this comment.
Can we run dn.getDrainLock in foreground and not use done chan?
I assume leaderelection.RunOrDie is a loop w/o timeout, is it true?
There was a problem hiding this comment.
The leaderelection.RunOrDie runs forever unless the context is canceled. I didn't run it in the foreground, because I want it keeps leading until the node finishes draining. So the rest nodes can only start the election after the prior one finishes draining if there is no reboot required.
pkg/daemon/daemon.go
Outdated
| @@ -18,6 +18,14 @@ import ( | |||
| "time" | |||
|
|
|||
| "github.com/golang/glog" | |||
| sriovnetworkv1 "github.com/k8snetworkplumbingwg/sriov-network-operator/api/v1" | |||
There was a problem hiding this comment.
nit: could you separate local imports from external imports
There was a problem hiding this comment.
This was updated by some plugin of my editor automatically. I'll change it back.
| } | ||
|
|
||
| // start the leader election | ||
| leaderelection.RunOrDie(ctx, leaderelection.LeaderElectionConfig{ |
There was a problem hiding this comment.
looking at leaderelection docs:
Package leaderelection implements leader election of a set of endpoints. It uses an annotation in the endpoints object to store the record of the election state. This implementation does not guarantee that only one client is acting as a leader (a.k.a. fencing).
is it not an issue ? i think what we are doing here is considered fencing
how will the system behave when there is only one endpoint trying to take the lead on LeaseLock ?
There was a problem hiding this comment.
Instead of using endpoint, I use Lease API for leader election here. I don't think that statement applies. As all the clients race for the same Lease object. So I don't think there could be more than one acting as leader.
There was a problem hiding this comment.
OK thanks for explaining
| time.Sleep(1 * time.Second) | ||
| if dn.drainable { | ||
| glog.V(2).Info("drainNode(): no other node is draining") | ||
| err = dn.annotateNode(dn.name, annoDraining) |
There was a problem hiding this comment.
using this mechanism, do we still need to annotate the node with annotDraining ?
There was a problem hiding this comment.
That is the trick. The leader election mechanism requires the leader to keep updating the Lease object. But in our case, the node may reboot itself, then lose leadership. So I use a 2 layers lock here. The node can only start draining with 2 conditions: 1) it becomes the leader 2) no other node is draining which is indicated by the annotation.
There was a problem hiding this comment.
I see, thanks for clarifying
There was a problem hiding this comment.
@pliurh mind mentioning this in the commit message ? so its clear in the commit message how this mechanism is used to control node draining
pkg/daemon/daemon.go
Outdated
| RetryPeriod: 1 * time.Second, | ||
| Callbacks: leaderelection.LeaderCallbacks{ | ||
| OnStartedLeading: func(ctx context.Context) { | ||
| glog.V(2).Info("drainNode(): started leading") |
There was a problem hiding this comment.
I see in your log messages you've drainNode() - should it not be getDrainLock()?
| glog.V(2).Info("drainNode(): started leading") | ||
| for { | ||
| time.Sleep(1 * time.Second) | ||
| if dn.drainable { |
There was a problem hiding this comment.
Outside of any concern for this PR because this pattern was here before this PR - but have you folks ever seen nodes getting stuck on this condition? It could happen if a node reboots and doesn't startup and daemonset is unable to update its draining status.
Then line 847 is blocked indefinitely.
There was a problem hiding this comment.
That is intentional. We don't want a configuration mistake to break more nodes. If users encounter such a problem, they'd better do some troubleshooting to find out why the node cannot come back.
pliurh
force-pushed
the
leader_election
branch
2 times, most recently
from
55f619a
to
fb30f8c
Compare
There was a problem hiding this comment.
I would prefer the package updates be performed in a separate commit in the PR (easier to review this way). but would not block on it.
@SchSeba i see below you were requested as reviewer. once you approve this can be merged IMO
Utilize k8s leader election mechanism to prevent annotating nodes at the same time. The leader election mechanism requires the leader to keep updating the Lease object. But in our case, the node may reboot itself, then lose leadership. So I use a 2 layers lock here. The node can only start draining with 2 conditions: 1) it becomes the leader 2) no other node is draining which is indicated by the annotation.
Utilize k8s leader election mechanism to prevent annotating nodes
at the same time.