Skip drain on Single Node deployment #2457

sinnykumari · 2021-03-09T15:25:03Z

With the introduction of different ControlPlaneTopology types
in the OpenShift cluster, the behaviour of the cluster may differ
based on cluster type. For example: cluster with Single
controlPlane node, it doesn't make sense to perform workload drain.

ControllerConfig now understands ControlPlaneTopology:TopologyMode
set in the cluster. Node controller later on reads the value from
controllerConfig and sets value into node annotation
machineconfiguration.openshift.io/controlPlaneTopology.

While performing a configuration update, machine-config-daemon
will read the annotation and based on controlPlaneTopology
type, it will decide drain action.
MCD skips drain if controlPlaneTopology is SingleReplica.

Part of - openshift/enhancements#560

This PR also:

refactors drain logic
adds related unit tests

With the introduction of different ControlPlaneTopology types in the OpenShift cluster, the behaviour of the cluster may differ based on cluster type. For example: cluster with Single controlPlane node, it doesn't make sense to perform workload drain. ControllerConfig now understands ControlPlaneTopology:TopologyMode set in the cluster. Node controller later on reads the value from controllerConfig and sets value into node annotation `machineconfiguration.openshift.io/controlPlaneTopology`. While performing a configuration update, machine-config-daemon will read the annotation and based on controlPlaneTopology type, it will decide drain action. MCD skips drain if controlPlaneTopology is SingleReplica. In any other cases, it will default to perform drain.

sinnykumari · 2021-03-09T15:27:00Z

/test e2e-aws-single-node

yuqi-zhang · 2021-03-09T15:33:43Z

pkg/daemon/daemon.go

Since you use the dn.Node reference instead, the linter is complaining the node variable isn't used anymore here

yeah, fixed it

Refactored drain logic and moved drain related functions into drain.go for easier maintenance.

Updated existing tests and added new test where node controller reads ControlPlaneTopology value from controllerConfig and compares with annotation `machineconfiguration.openshift.io/controlPlaneTopology` value set on the node.

sinnykumari · 2021-03-09T15:57:27Z

unit test seems to be failing due to unrelated test
/retest

sinnykumari · 2021-03-09T16:24:08Z

/test e2e-aws-single-node

sinnykumari · 2021-03-09T18:21:09Z

/retest

yuqi-zhang

Overall looks good! Just some minor questions/nits.

Also, let's say a user has a default 3-3 cluster. If they set that controllerconfig spec, would they be able to skip drains on their updates? Or does something overwrite that?

One last question: I wonder if ensureControllerConfigSpec function needs to be updated or not

pkg/controller/node/node_controller.go

pkg/daemon/daemon.go

pkg/daemon/drain.go

yuqi-zhang · 2021-03-10T01:22:12Z

manifests/controllerconfig.crd.yaml

                        like the web console to tell users where to find the Kubernetes
                        API.
                      type: string
+                    controlPlaneTopology:


where is controlPlaneTopology coming from? I see in the commit message:
ControllerConfig now understands ControlPlaneTopology:TopologyMode set in the cluster.
But I don't see how we're syncing that into the controlPlaneTopology field in the controllerconfig, maybe I'm missing something

syncMachineConfigController() calls resourceapply.ApplyControllerConfig that calls resourcemerge.EnsureControllerConfig which calls ensureControllerConfigSpec() and here we check if Infra object has been modified and update controllerconfig if changed . Infra object is the one where controlPlaneTopology value exist as well along with other data https://github.com/openshift/api/blob/master/config/v1/types_infrastructure.go#L86 . MCO was already reading and updating Infra content, so adding controlPlaneToplogy field in crd populates its value as well.

And operator reads infrastructure object from cluster at https://github.com/openshift/machine-config-operator/blob/master/pkg/operator/sync.go#L220 and syncs the controllerconfigspec . There are too many things to followup...

Ahh, ok thanks for the explanation! Right it gets synced as part of the infra field. Not the clearest of code paths 🤦

sinnykumari · 2021-03-10T10:34:34Z

Overall looks good! Just some minor questions/nits.

Also, let's say a user has a default 3-3 cluster. If they set that controllerconfig spec, would they be able to skip drains on their updates? Or does something overwrite that?

No, MCO operator pod keeps resyncing value of controllerConfigspec and we read data from infrastructure cluster object, so in next sync it will revert the value to whatever is set cluster wide.

One last question: I wonder if ensureControllerConfigSpec function needs to be updated or not

not needed, as we already do that at

machine-config-operator/lib/resourcemerge/machineconfig.go

Line 81 in 82868e6

    
           if required.Infra != nil && !equality.Semantic.DeepEqual(existing.Infra, required.Infra) {

.

sinnykumari · 2021-03-10T10:51:17Z

log from a SNO cluster created using clusterbot and applied a MachineConfig:

I0310 10:38:15.014517    8164 update.go:543] Checking Reconcilable for config rendered-master-dd9d479a34df41e1ba15683557aac98f to rendered-master-032640efd4eaabc7a384aa49328dc323
I0310 10:38:15.115007    8164 update.go:1851] Starting update from rendered-master-dd9d479a34df41e1ba15683557aac98f to rendered-master-032640efd4eaabc7a384aa49328dc323: &{osUpdate:false kargs:false fips:false passwd:false files:false units:false kernelType:false extensions:true}
I0310 10:38:15.149990    8164 update.go:1851] Node has been successfully cordoned
I0310 10:38:15.158087    8164 update.go:1851] Drain not required, skipping
I0310 10:38:15.163018    8164 update.go:1166] Updating files

sinnykumari · 2021-03-10T11:11:27Z

/test e2e-aws-single-node

Also, made some minor fixes and spell checks

sinnykumari · 2021-03-10T15:11:07Z

Overall looks good! Just some minor questions/nits.
Also, let's say a user has a default 3-3 cluster. If they set that controllerconfig spec, would they be able to skip drains on their updates? Or does something overwrite that?

No, MCO operator pod keeps resyncing value of controllerConfigspec and we read data from infrastructure cluster object, so in next sync it will revert the value to whatever is set cluster wide.

For double safety, confirmed same on a regular HA cluster. Updated controlPlaneTopology value in controllerconfig from HighlyAvailable to SingleReplica. Value got reverted back to HighlyAvailable

sinnykumari · 2021-03-10T15:12:26Z

/retest
/test e2e-aws-single-node

sinnykumari · 2021-03-10T18:29:39Z

gcp-op failed due to unrelated reason, retesting
/test e2e-gcp-op

sinnykumari · 2021-03-11T13:40:06Z

Talked to SNO team, aws-single-node test is good from MCO side. Failing sub-tests should pass with openshift/origin#25936 .

yuqi-zhang

Code is looking good! Just a few more minor questions/nits. Approving 🎉

Will try to do some manual testing as well as give others a chance to provide feedback before LGTM

yuqi-zhang · 2021-03-11T22:05:19Z

pkg/controller/node/node_controller_test.go

 	f.run(getKey(mcp, t))
 }

+func TestControlPlaneTopology(t *testing.T) {


Sorry, I'm a bit confused by what this is trying to test. Is it checking to see if the controllerconfig topology propagates to node annotations? Where is that being checked?

Here, we are currently checking that with a valid controlPlaneToplogy i.e. SingleReplica , setClusterConfigAnnotation() gets called and works as expected. Since, in this unit test we have created controllerConfig with SingleReplica and we have set node annotation machineconfiguration.openshift.io/controlPlaneTopology to SingleReplica , expected result is no node change action such as patch.

Later on, I am thinking of extending the test to also perform machineconfiguration.openshift.io/controlPlaneTopology annotation change action. Adding this is a bit tricky.

Hmm, sorry for the dumb question, but I'm still not sure how we are testing that. If an action change happens as a bug, which line of code would return the error? I see we do the same for the above TestShouldDoNothing but I don't see any expectations for "nothing"

when in test run() calls runController(), node-controller syncHandler(pool) gets called at https://github.com/openshift/machine-config-operator/blob/master/pkg/controller/node/node_controller_test.go#L114 . As you know when this runs, various actions can be generated based on what all operation on node has been performed. In test we filter out list and watch actionhttps://github.com/openshift/machine-config-operator/blob/master/pkg/controller/node/node_controller_test.go#L121as these actions doesn't really update anything. For other actions like patch, additional action will be check in later line during checkAction()

pkg/daemon/drain.go

yuqi-zhang · 2021-03-11T22:14:34Z

pkg/daemon/drain.go

+	}
+
+	// We are here, that means we need to cordon and drain node
+	MCDDrainErr.WithLabelValues(dn.node.Name, "").Set(0)


Hmm, why do we set this here again? Is it to clear a previously failing drain's error if that one hit the global timeout?

maybe, I am not sure. I kept logic from earlier implementation. @kikisdeliveryservice would know better.

pkg/daemon/drain.go

yuqi-zhang · 2021-03-12T22:07:31Z

Tried to do some manual testing but failed, will try again next week and LGTM if nobody else has any concerns

yuqi-zhang · 2021-03-16T21:33:50Z

I think this looks good! Let's get this in, and iterate from there if there are any issues.
/lgtm

openshift-ci-robot · 2021-03-16T21:34:13Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: sinnykumari, yuqi-zhang

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Details

Needs approval from an approver in each of these files:

~~OWNERS~~ [sinnykumari,yuqi-zhang]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

openshift-bot · 2021-03-16T22:11:10Z

/retest