Move node draining from actuator into machine controller #174
Conversation
Node draining is a generic operation independent of a specific actuator. Thus, it makes sense to move the code from actuator into the machine controllers. The node draining code itself is imported from github.com/openshift/kubernetes-drain. At the same time it's currently impossible to use the controller-runtime client for node draining due to missing Patch operation (kubernetes-sigs/controller-runtime#235). Thus, the machine controller needs to initialize kubeclient as well in order to implement the node draining logic. Once the Patch operation is implemented, the draining logic can be updated to replace kube client with controller runtime client. Also, initialize event recorder to generate node draining event.
openshift-ci-robot
added
the
size/L
| NodeNameEnvVar = "NODE_NAME" | ||
|
|
||
| // ExcludeNodeDrainingAnnotation annotation explicitly skips node draining if set | ||
| ExcludeNodeDrainingAnnotation = "machine.openshift.io/exclude-node-draining" |
There was a problem hiding this comment.
there was some discussion with @frobware and @kalexand-rh on slack about reconsidering this annotation name, otherwise looks good to me
There was a problem hiding this comment.
Definitely not a blocker for the PR as we already have it merged. Though, I am fine changing the name to something more proper.
|
/approve |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: enxebre The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
openshift-ci-robot
added
the
approved
| // deleted without a manual intervention. | ||
| if _, exists := m.ObjectMeta.Annotations[ExcludeNodeDrainingAnnotation]; !exists && m.Status.NodeRef != nil { | ||
| if err := func() error { | ||
| kubeClient, err := kubernetes.NewForConfig(r.config) |
There was a problem hiding this comment.
Maybe we should add the kubeClient to the reconciler as with do with oc client?
There was a problem hiding this comment.
I intentionally did no do that since this is only a temporary solution until controller-runtime implements Patch. Once that happens, the kube client initialization can be completely dropped.
| @@ -145,6 +163,51 @@ func (r *ReconcileMachine) Reconcile(request reconcile.Request) (reconcile.Resul | |||
| return reconcile.Result{}, nil | |||
| } | |||
| klog.Infof("reconciling machine object %v triggers delete.", name) | |||
|
|
|||
| // Drain node before deletion | |||
There was a problem hiding this comment.
We should think about breaking this large function apart, it's going to be very difficult if not impossible to test.
There was a problem hiding this comment.
there are not unit tests so far other than the ones provided by https://github.com/openshift/kubernetes-drain/. From the functional point of view we have https://github.com/openshift/cluster-api-actuator-pkg/blob/master/pkg/e2e/actuators/actuators.go#L182.
Signed-off-by: Vince Prignano <[email protected]>
Node draining is a generic operation independent of a specific actuator.
Thus, it makes sense to move the code from actuator into the machine controllers.
The node draining code itself is imported from github.com/openshift/kubernetes-drain.
At the same time it's currently impossible to use the controller-runtime client for node draining
due to missing Patch operation (kubernetes-sigs/controller-runtime#235).
Thus, the machine controller needs to initialize kubeclient as well in order to
implement the node draining logic. Once the Patch operation is implemented,
the draining logic can be updated to replace kube client with controller runtime client.
Also, initialize event recorder to generate node draining event.
Corresponding openshift upstream PR here: openshift/cluster-api#11