Pause MCP before draining/rebooting node #93

pliurh · 2021-03-23T03:13:06Z

The config daemon instance on each node need to compete for the 'Draining' lock before
it can draining/rebooting a node.

After a config daemon instance gets the lock, it checks its MachineConfigPool conditions.
If the MCP is in ready state, it will pause the MCP then process, otherwise, it wait for
the MCP getting ready. The MCP will be resumed after the config daemon and release the
lock after the config dameon finishes its work.

pkg/daemon/daemon.go

zshi-redhat · 2021-03-23T07:56:08Z

pkg/daemon/daemon.go

+			if newMcp.GetName() != dn.mcpName {
+				return
+			}
+			if mcfgv1.IsMachineConfigPoolConditionFalse(newMcp.Status.Conditions, mcfgv1.MachineConfigPoolDegraded) &&


By checking poolDegraded, it makes sure pool is not paused by user, right?

@yuqi-zhang @sinnykumari, do you think the checks here can 100% guarantee there is no race condition between MCO and SNO trying to start an update simultaneously?

By checking poolDegraded, it makes sure pool is not paused by user, right?

No, this is to check MCP is not in 'Degraded' state.

Pausing and degraded are separate concepts. I think the SRIOV operator shouldn't do anything if a pool is degraded (which means the pool is in an error state), as you have here. I think the checks (not degraded, not updating, updated) should be good.

zshi-redhat · 2021-03-23T08:03:28Z

pkg/daemon/daemon.go

+			if newMcp.GetName() != dn.mcpName {
+				return
+			}
+			if mcfgv1.IsMachineConfigPoolConditionFalse(newMcp.Status.Conditions, mcfgv1.MachineConfigPoolDegraded) &&


When node gets rebooted by sriov config daemon, will the drainNode be called again (in which case MCP is degraded due to pause:true)?

Pausing an MCP will not make it 'Degraded'.

zshi-redhat · 2021-03-23T08:15:27Z

pkg/daemon/daemon.go

+			if mcfgv1.IsMachineConfigPoolConditionFalse(newMcp.Status.Conditions, mcfgv1.MachineConfigPoolDegraded) &&
+				mcfgv1.IsMachineConfigPoolConditionTrue(newMcp.Status.Conditions, mcfgv1.MachineConfigPoolUpdated) &&
+				mcfgv1.IsMachineConfigPoolConditionFalse(newMcp.Status.Conditions, mcfgv1.MachineConfigPoolUpdating) {
+				glog.Infof("drainNode(): MCP %s is ready", newMcp.GetName())


Check readyMachineCount == machineCount as well ?

I suppose the 'Updated' condition of MCP is sufficient.

yuqi-zhang

Overall this approach I think is more safe with the MCO and race conditions. Some more specific comments below

yuqi-zhang · 2021-03-23T22:00:04Z

pkg/daemon/daemon.go

@@ -580,8 +590,17 @@ func (dn *Daemon) completeDrain() error {
 		return err
 	}

+	if utils.ClusterType == utils.ClusterTypeOpenshift {
+		glog.Infof("completeDrain(): resume MCP %s", dn.mcpName)


Making sure I understand this properly, the completeDrain() function gets called after the reboot correct? To uncordon the node?

Yes. It will be executed after reboot or after draining without a reboot.

yuqi-zhang · 2021-03-23T22:04:03Z

pkg/daemon/daemon.go

+			if newMcp.GetName() != dn.mcpName {
+				return
+			}
+			if mcfgv1.IsMachineConfigPoolConditionFalse(newMcp.Status.Conditions, mcfgv1.MachineConfigPoolDegraded) &&


Pausing and degraded are separate concepts. I think the SRIOV operator shouldn't do anything if a pool is degraded (which means the pool is in an error state), as you have here. I think the checks (not degraded, not updating, updated) should be good.

yuqi-zhang · 2021-03-23T22:06:36Z

pkg/daemon/daemon.go

+		<-ctx.Done()
+
+		glog.Infof("drainNode(): pause MCP %s", dn.mcpName)
+		pausePatch := []byte("{\"spec\":{\"paused\":true}}")


if we want to ensure maximum safety and elimate races, after the pause goes through here, we should add another check like above to see if the pool is progressing, since there is a tiny chance that the MCP started some kind of update in between.

If we check here and the pool is still good, then we can progress with drain. If the pool somehow is updating now, we should consider unpausing and then waiting in a loop for the MCP to go back to not updating.

Do we also want to check that pause is not already set by user?

I updated the patch, PTAL.

sinnykumari

Thanks for implementing it in this way! overall this approach looks good.

pkg/daemon/daemon.go

sinnykumari · 2021-03-24T13:00:39Z

pkg/daemon/daemon.go

+			}
+			if paused {
+				glog.Infof("drainNode(): MCP is processing, resume MCP %s", dn.mcpName)
+				pausePatch := []byte("{\"spec\":{\"paused\":false}}")


For my understanding, can you please explain the condition in which we are unpausing the pool here.

The MCP informer will not be stoped right after we pause the MCP. So the mcpEventHandler will be invoked once more as we updated the MCD. In this cycle, we check if the MCP is processing, if yes and the MCP was paused during the last cycle (indicated by the variable 'paused' here). We resume the MCP and wait until it finishes.

This is to address #93 (comment)

got it, makes sense.

SchSeba

looks good I just have small comments thanks!

deploy/clusterrole.yaml

pkg/daemon/daemon.go

SchSeba · 2021-03-30T12:29:27Z

/lgtm

pkg/daemon/daemon.go

pliurh · 2021-04-01T04:45:50Z

@zshi-redhat @SchSeba I just updated the patch. A new annotation value Draining_MCP_Paused was introduced to indicate whether the MCP is paused by the config daemon. Also, I changed the needDrainNode, so that changing MTU will not need a drain if ifaceStatus.NumVfs is 0.

zshi-redhat · 2021-04-01T07:57:06Z

/lgtm

SchSeba

just two small nits and I think the PR is ready

SchSeba · 2021-04-06T20:25:41Z

pkg/daemon/daemon.go

+				pausePatch := []byte("{\"spec\":{\"paused\":true}}")
+				_, err = dn.mcClient.MachineconfigurationV1().MachineConfigPools().Patch(context.Background(), dn.mcpName, types.MergePatchType, pausePatch, metav1.PatchOptions{})
+				if err != nil {
+					glog.V(2).Infof("drainNode(): fail to pause MCP %s: %v", dn.mcpName, err)


nit: fail -> Failed

SchSeba · 2021-04-06T20:38:58Z

pkg/daemon/daemon.go

-	if err != nil {
-		glog.Errorf("drainNode(): Failed to annotate node: %v", err)
-		return err
+		mcpInformer.AddEventHandler(cache.ResourceEventHandlerFuncs{


nit: can we add a check if this is the MCP that the node is part of even before we send it to the mcpEventHandler function?

Hmm, I don't know how we can do that. Normally, the informer watches all the objects. I think it's sufficient that we check it in the mcpEventHandler.

The config daemon instance on each node need to compete for the 'Draining' lock before it can draining/rebooting a node. After a config daemon instance gets the lock, it checks its MachineConfigPool conditions. If the MCP is in ready state, it will pause the MCP then process, otherwise, it wait for the MCP getting ready. The MCP will be resumed after the config daemon and release the lock after the config dameon finishes its work.

pliurh requested review from SchSeba and zshi-redhat March 23, 2021 03:13

pliurh changed the title ~~Pause MCP before draining/rebooting node~~ WIP: Pause MCP before draining/rebooting node Mar 23, 2021

pliurh mentioned this pull request Mar 23, 2021

WIP: Check MCO node state before trigger a reboot #89

Closed

zshi-redhat reviewed Mar 23, 2021

View reviewed changes

pliurh force-pushed the pause_mcp branch 2 times, most recently from cc8430e to 6f92a10 Compare March 23, 2021 16:11

yuqi-zhang reviewed Mar 23, 2021

View reviewed changes

pliurh force-pushed the pause_mcp branch 2 times, most recently from 741949c to 0d42c93 Compare March 24, 2021 05:27

sinnykumari reviewed Mar 24, 2021

View reviewed changes

pliurh force-pushed the pause_mcp branch from 0d42c93 to f552cd8 Compare March 25, 2021 03:35

SchSeba reviewed Mar 25, 2021

View reviewed changes

deploy/clusterrole.yaml Show resolved Hide resolved

pkg/daemon/daemon.go Show resolved Hide resolved

pkg/daemon/daemon.go Show resolved Hide resolved

github-actions bot added the lgtm label Mar 30, 2021

zshi-redhat requested changes Apr 1, 2021

View reviewed changes

pkg/daemon/daemon.go Show resolved Hide resolved

pliurh force-pushed the pause_mcp branch from f552cd8 to 28f61e0 Compare April 1, 2021 04:47

SchSeba reviewed Apr 6, 2021

View reviewed changes

pliurh force-pushed the pause_mcp branch from 28f61e0 to 1a5861e Compare April 7, 2021 07:08

pliurh changed the title ~~WIP: Pause MCP before draining/rebooting node~~ Pause MCP before draining/rebooting node Apr 7, 2021

pliurh merged commit f17625b into k8snetworkplumbingwg:master Apr 7, 2021

Pause MCP before draining/rebooting node #93

Pause MCP before draining/rebooting node #93

pliurh commented Mar 23, 2021

zshi-redhat Mar 23, 2021

zshi-redhat Mar 23, 2021

pliurh Mar 23, 2021

yuqi-zhang Mar 23, 2021

zshi-redhat Mar 23, 2021

pliurh Mar 23, 2021

zshi-redhat Mar 23, 2021

pliurh Mar 23, 2021

yuqi-zhang left a comment

yuqi-zhang Mar 23, 2021

pliurh Mar 24, 2021

yuqi-zhang Mar 23, 2021

yuqi-zhang Mar 23, 2021

zshi-redhat Mar 24, 2021

pliurh Mar 24, 2021

sinnykumari left a comment

sinnykumari Mar 24, 2021

pliurh Mar 24, 2021

pliurh Mar 24, 2021

sinnykumari Mar 24, 2021

SchSeba left a comment

SchSeba commented Mar 30, 2021

pliurh commented Apr 1, 2021

zshi-redhat commented Apr 1, 2021

SchSeba left a comment

SchSeba Apr 6, 2021

pliurh Apr 7, 2021

SchSeba Apr 6, 2021

pliurh Apr 7, 2021

Pause MCP before draining/rebooting node #93

Pause MCP before draining/rebooting node #93

Conversation

pliurh commented Mar 23, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yuqi-zhang left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sinnykumari left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

SchSeba left a comment

Choose a reason for hiding this comment

SchSeba commented Mar 30, 2021

pliurh commented Apr 1, 2021

zshi-redhat commented Apr 1, 2021

SchSeba left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment