feat: adding external-drainer nodestate annotation for drain-controller (Mellanox#1774)

Author: heyvister1

README.md

@@ -259,6 +259,8 @@ Setting above environment variables can be used in values.yaml, in case network-
     # drain controller requestor ID to be used in nodeMaintenance objects
     drainControllerRequestorID: "nvidia.network-operator-drain-controller"
 ```
+> __Note__: in order to use network-operator drain-controller, make sure that `sriovnetwork.openshift.io/use-external-drainer=true`
+> exists under nodestate annotations. Is should be added by SRIOV operator
 
 Using requestor mode supports a `shared-requestor` flow where multiple operators can coordinate node maintenance operations:
 Assumptions:

controllers/drain_controller.go

@@ -124,6 +124,14 @@ func (r *DrainReconcile) Reconcile(ctx context.Context, req ctrl.Request) (ctrl.
 		return ctrl.Result{}, nil
 	}
 
+	// check if nodestate has annotation
+	// TODO: Replace "sriovnetwork.openshift.io/use-external-drainer" with SRIOV constants.NodeExternalDrainerAnnotation
+	if !utils.ObjectHasAnnotation(nodeNetworkState, "sriovnetwork.openshift.io/use-external-drainer", "true") {
+		reqLogger.Info(`nodestate is not set with external drainer annotation, don't requeue the request. Make sure that 
+		SRIOV operator is configured to use external drainer`, "nodeState", req.Name)
+		return ctrl.Result{}, nil
+	}
+
 	// create the drain state annotation if it doesn't exist in the sriovNetworkNodeState object
 	nodeStateDrainAnnotationCurrent, currentNodeStateExist,
 		err := r.ensureAnnotationExists(ctx, nodeNetworkState, constants.NodeStateDrainAnnotationCurrent)
@@ -344,7 +352,7 @@ func (d DrainAnnotationPredicate) Update(e event.UpdateEvent) bool {
 	oldAnno, hasOldAnno := e.ObjectOld.GetAnnotations()[constants.NodeDrainAnnotation]
 	newAnno, hasNewAnno := e.ObjectNew.GetAnnotations()[constants.NodeDrainAnnotation]
 
-	d.log.V(consts.LogLevelDebug).Info("Update", "oldAnno", oldAnno, "newAnno", newAnno)
+	d.log.V(consts.LogLevelDebug).Info("Update node state drain annotation", "oldAnno", oldAnno, "newAnno", newAnno)
 	if !hasOldAnno && hasNewAnno {
 		return true
 	}
@@ -381,6 +389,16 @@ func (d DrainStateAnnotationPredicate) Update(e event.UpdateEvent) bool {
 		return true
 	}
 
+	// Check if the external drainer annotation has been added
+	// TODO: Replace "sriovnetwork.openshift.io/use-external-drainer" with SRIOV constants.NodeExternalDrainerAnnotation
+	_, hasOldAnno = e.ObjectOld.GetAnnotations()["sriovnetwork.openshift.io/use-external-drainer"]
+	_, hasNewAnno = e.ObjectNew.GetAnnotations()["sriovnetwork.openshift.io/use-external-drainer"]
+	if !hasOldAnno && hasNewAnno {
+		d.log.V(consts.LogLevelDebug).Info("Update nodestate external drainer annotation", "oldAnno",
+			oldAnno, "newAnno", newAnno)
+		return true
+	}
+
 	return oldAnno != newAnno
 }
 

controllers/drain_controller_test.go

@@ -18,6 +18,7 @@ package controllers //nolint:dupl
 
 import (
 	"context"
+	"fmt"
 	"sync/atomic"
 	"time"
 
@@ -38,13 +39,16 @@ import (
 	sriovnetworkv1 "github.com/k8snetworkplumbingwg/sriov-network-operator/api/v1"
 	constants "github.com/k8snetworkplumbingwg/sriov-network-operator/pkg/consts"
 	"github.com/k8snetworkplumbingwg/sriov-network-operator/pkg/utils"
+
+	"github.com/Mellanox/network-operator/pkg/drain"
 )
 
 var (
-	testRequestorID       = "secondary.requestor.com"
-	drainRequestorID      = "primary-drain.requestor.com"
-	drainRequestorNS      = "default"
-	maxParallelOperations = atomic.Int32{}
+	testRequestorID           = "secondary.requestor.com"
+	drainRequestorID          = "primary-drain.requestor.com"
+	drainRequestorNS          = "default"
+	testNodeMaintenancePrefix = drain.DefaultNodeMaintenanceNamePrefix
+	maxParallelOperations     = atomic.Int32{}
 )
 
 var _ = Describe("Drain Controller", Ordered, func() {
@@ -97,9 +101,19 @@ var _ = Describe("Drain Controller", Ordered, func() {
 	})
 
 	Context("when there is only one node", func() {
+		It("should not drain node on drain require while use-external-drainer annotation is not set",
+			func(ctx context.Context) {
+				node, nodeState := createNode(ctx, "node1", false)
+
+				simulateDaemonSetAnnotation(node, constants.DrainRequired)
+
+				expectNodeStateAnnotation(nodeState, constants.DrainIdle)
+				expectNodeIsSchedulable(node)
+
+			})
 
 		It("should drain single node on drain require", func(ctx context.Context) {
-			node, nodeState := createNode(ctx, "node1")
+			node, nodeState := createNode(ctx, "node1", true)
 
 			simulateDaemonSetAnnotation(node, constants.DrainRequired)
 
@@ -114,7 +128,7 @@ var _ = Describe("Drain Controller", Ordered, func() {
 		})
 
 		It("should not drain on reboot for single node", func(ctx context.Context) {
-			node, nodeState := createNode(ctx, "node1")
+			node, nodeState := createNode(ctx, "node1", true)
 
 			simulateDaemonSetAnnotation(node, constants.RebootRequired)
 
@@ -127,8 +141,8 @@ var _ = Describe("Drain Controller", Ordered, func() {
 		})
 
 		It("should drain on reboot for multiple node", func(ctx context.Context) {
-			node, nodeState := createNode(ctx, "node1")
-			createNode(ctx, "node2")
+			node, nodeState := createNode(ctx, "node1", true)
+			createNode(ctx, "node2", true)
 
 			simulateDaemonSetAnnotation(node, constants.RebootRequired)
 
@@ -141,25 +155,26 @@ var _ = Describe("Drain Controller", Ordered, func() {
 		})
 
 		It("should drain single node on drain require, with additional requestor", func(ctx context.Context) {
-			node, nodeState := createNode(ctx, "node1")
-			_ = newNodeMaintenance(ctx, k8sClient, "node1", drainRequestorNS)
+			node, nodeState := createNode(ctx, "node1", true)
+			nmName := fmt.Sprintf("%s-%s", testNodeMaintenancePrefix, node.Name)
+			_ = newNodeMaintenance(ctx, k8sClient, node.Name, drainRequestorNS)
 
 			simulateDaemonSetAnnotation(node, constants.DrainRequired)
 			expectNodeStateAnnotation(nodeState, constants.DrainComplete)
 			expectNodeIsNotSchedulable(node)
 
 			nm := maintenancev1alpha1.NodeMaintenance{}
-			Expect(k8sClient.Get(ctx, types.NamespacedName{Name: node.Name, Namespace: drainRequestorNS},
+			Expect(k8sClient.Get(ctx, types.NamespacedName{Name: nmName, Namespace: drainRequestorNS},
 				&nm)).ToNot(HaveOccurred())
-			expectNodeMaintenanceUpdate(node.Name, []string{drainRequestorID})
+			expectNodeMaintenanceUpdate(nmName, []string{drainRequestorID})
 
 			expectNodeStateAnnotation(nodeState, constants.DrainComplete)
 			simulateDaemonSetAnnotation(node, constants.DrainIdle)
-			expectNodeMaintenanceUpdate(node.Name, nil)
+			expectNodeMaintenanceUpdate(nmName, nil)
 			// expect node to be unschedulable
 			expectNodeIsNotSchedulable(node)
 
-			Expect(k8sClient.Get(ctx, types.NamespacedName{Name: node.Name, Namespace: drainRequestorNS},
+			Expect(k8sClient.Get(ctx, types.NamespacedName{Name: nmName, Namespace: drainRequestorNS},
 				&nm)).ToNot(HaveOccurred())
 			Eventually(k8sClient.Delete(ctx, &nm)).ToNot(HaveOccurred())
 			expectNodeStateAnnotation(nodeState, constants.DrainIdle)
@@ -170,9 +185,9 @@ var _ = Describe("Drain Controller", Ordered, func() {
 	Context("when there are multiple nodes", func() {
 
 		It("should drain nodes serially", func(ctx context.Context) {
-			node1, nodeState1 := createNode(ctx, "node1")
-			node2, nodeState2 := createNode(ctx, "node2")
-			node3, nodeState3 := createNode(ctx, "node3")
+			node1, nodeState1 := createNode(ctx, "node1", true)
+			node2, nodeState2 := createNode(ctx, "node2", true)
+			node3, nodeState3 := createNode(ctx, "node3", true)
 
 			// Two nodes require to drain at the same time
 			maxParallelOperations.Store(1)
@@ -211,9 +226,9 @@ var _ = Describe("Drain Controller", Ordered, func() {
 		})
 
 		It("should drain nodes in parallel", func(ctx context.Context) {
-			node1, nodeState1 := createNode(ctx, "node1")
-			node2, nodeState2 := createNode(ctx, "node2")
-			node3, nodeState3 := createNode(ctx, "node3")
+			node1, nodeState1 := createNode(ctx, "node1", true)
+			node2, nodeState2 := createNode(ctx, "node2", true)
+			node3, nodeState3 := createNode(ctx, "node3", true)
 
 			// two nodes require to drain at the same time
 			maxParallelOperations.Store(2)
@@ -309,7 +324,8 @@ func simulateDaemonSetAnnotation(node *corev1.Node, drainAnnotationValue string)
 		ToNot(HaveOccurred())
 }
 
-func createNode(ctx context.Context, nodeName string) (*corev1.Node, *sriovnetworkv1.SriovNetworkNodeState) {
+func createNode(ctx context.Context, nodeName string, useExternalDrainer bool) (*corev1.Node,
+	*sriovnetworkv1.SriovNetworkNodeState) {
 	node := corev1.Node{
 		ObjectMeta: metav1.ObjectMeta{
 			Name: nodeName,
@@ -327,11 +343,15 @@ func createNode(ctx context.Context, nodeName string) (*corev1.Node, *sriovnetwo
 		ObjectMeta: metav1.ObjectMeta{
 			Name:      nodeName,
 			Namespace: namespaceName,
-			Labels: map[string]string{
+			Annotations: map[string]string{
 				constants.NodeStateDrainAnnotationCurrent: constants.DrainIdle,
 			},
 		},
 	}
+	if useExternalDrainer {
+		// TODO: change annotation name to constants.NodeExternalDrainerAnnotation once SRIOV PR is merged
+		nodeState.Annotations["sriovnetwork.openshift.io/use-external-drainer"] = "true"
+	}
 
 	Expect(k8sClient.Create(ctx, &node)).ToNot(HaveOccurred())
 	Expect(k8sClient.Create(ctx, &nodeState)).ToNot(HaveOccurred())
@@ -403,7 +423,7 @@ func processItems(ctx context.Context, c client.Client, maxParallel *atomic.Int3
 
 		// Handle deletion case
 		if !nm.DeletionTimestamp.IsZero() {
-			By("maintenance operator: remove finalizer")
+			By("maintenance operator: remove node-maintenance finalizer")
 			if controllerutil.ContainsFinalizer(&nm, maintenancev1alpha1.MaintenanceFinalizerName) {
 				original := nm.DeepCopy()
 				nm.SetFinalizers([]string{})
@@ -414,22 +434,23 @@ func processItems(ctx context.Context, c client.Client, maxParallel *atomic.Int3
 			// uncordon node
 			By("maintenance operator: uncordon node")
 			node := &corev1.Node{}
-			Expect(c.Get(ctx, types.NamespacedName{Name: nm.Name}, node)).ToNot(HaveOccurred())
+			Expect(c.Get(ctx, types.NamespacedName{Name: nm.Spec.NodeName}, node)).ToNot(HaveOccurred())
 			node.Spec.Unschedulable = false
 			Expect(c.Update(ctx, node)).To(Succeed())
 			continue
 		}
 
 		// Handle creation/update case
 		if !controllerutil.ContainsFinalizer(&nm, maintenancev1alpha1.MaintenanceFinalizerName) {
-			By("maintenance operator: add finalizer")
+			By("maintenance operator: add node-maintenance finalizer")
 			nm.Finalizers = append(nm.Finalizers, maintenancev1alpha1.MaintenanceFinalizerName)
 			Expect(c.Update(ctx, &nm)).To(Succeed())
 			continue
 		}
 
 		// Update status conditions
 		if nm.Status.Conditions == nil {
+			By("maintenance operator: add node-maintenance conditions")
 			Expect(nm.Finalizers).To(ContainElement(maintenancev1alpha1.MaintenanceFinalizerName))
 			// Update status conditions
 			meta.SetStatusCondition(&nm.Status.Conditions, desiredCondition)
@@ -439,7 +460,7 @@ func processItems(ctx context.Context, c client.Client, maxParallel *atomic.Int3
 			// cordon node
 			By("maintenance operator: cordon node")
 			node := &corev1.Node{}
-			Expect(c.Get(ctx, types.NamespacedName{Name: nm.Name}, node)).ToNot(HaveOccurred())
+			Expect(c.Get(ctx, types.NamespacedName{Name: nm.Spec.NodeName}, node)).ToNot(HaveOccurred())
 			node.Spec.Unschedulable = true
 			Expect(c.Update(ctx, node)).To(Succeed())
 		}
@@ -450,7 +471,7 @@ func newNodeMaintenance(ctx context.Context, c client.Client,
 	name, namespace string) *maintenancev1alpha1.NodeMaintenance {
 	nm := &maintenancev1alpha1.NodeMaintenance{
 		ObjectMeta: metav1.ObjectMeta{
-			Name:      name,
+			Name:      fmt.Sprintf("%s-%s", testNodeMaintenancePrefix, name),
 			Namespace: namespace,
 		},
 		Spec: maintenancev1alpha1.NodeMaintenanceSpec{

controllers/suite_test.go

@@ -130,6 +130,7 @@ var _ = BeforeSuite(func() {
 
 	Expect(os.Setenv("DRAIN_CONTROLLER_REQUESTOR_NAMESPACE", drainRequestorNS)).NotTo(HaveOccurred())
 	Expect(os.Setenv("DRAIN_CONTROLLER_REQUESTOR_ID", drainRequestorID)).NotTo(HaveOccurred())
+	Expect(os.Setenv("DRAIN_CONTROLLER_NODE_MAINTENANCE_PREFIX", testNodeMaintenancePrefix)).NotTo(HaveOccurred())
 	Expect(os.Setenv("DRAIN_CONTROLLER_SRIOV_NODE_STATE_NAMESPACE", namespaceName)).NotTo(HaveOccurred())
 
 	By("bootstrapping test environment")

deployment/network-operator/charts/sriov-network-operator/templates/operator.yaml

@@ -86,8 +86,8 @@ spec:
               value: {{ .Values.operator.metricsExporter.certificates.secretName }}
             - name: METRICS_EXPORTER_KUBE_RBAC_PROXY_IMAGE
               value: {{ .Values.images.metricsExporterKubeRbacProxy }}
-            - name: USE_MAINTENANCE_OPERATOR_DRAINER
-              value: {{ .Values.operator.maintenanceOperatorDrainer.enabled | quote }}
+            - name: USE_EXTERNAL_DRAINER
+              value: {{ .Values.operator.externalDrainer.enabled | quote }}
             {{- if .Values.operator.metricsExporter.prometheusOperator.enabled }}
             - name: METRICS_EXPORTER_PROMETHEUS_OPERATOR_ENABLED
               value: {{ .Values.operator.metricsExporter.prometheusOperator.enabled | quote}}

deployment/network-operator/charts/sriov-network-operator/values.yaml

@@ -40,8 +40,8 @@ operator:
       serviceAccount: "prometheus-k8s"
       namespace: "monitoring"
       deployRules: false
-  # use external drain controller, utilizing NVIDIA maintenance operator
-  maintenanceOperatorDrainer:
+  # use external drain controller
+  externalDrainer:
     enabled: false
   admissionControllers:
     enabled: false