[release-4.10] Bug 2076308: Move removeUpdateInProgressTaint functionality to mcc

pkg/controller/node/node_controller.go

@@ -771,9 +771,18 @@ func (ctrl *Controller) syncMachineConfigPool(key string) error {
 		// All the nodes that need to be upgraded should have `NodeUpdateInProgressTaint` so that they're less likely
 		// to be chosen during the scheduling cycle.
 		targetConfig := pool.Spec.Configuration.Name
-		if node.Annotations[daemonconsts.DesiredMachineConfigAnnotationKey] != targetConfig {
-			if err := ctrl.setUpdateInProgressTaint(ctx, node.Name); err != nil {
-				return goerrs.Wrapf(err, "failed applying %s taint for node %s", constants.NodeUpdateInProgressTaint.Key, node.Name)
+		hasInProgressTaint := checkIfNodeHasInProgressTaint(node)
+		if node.Annotations[daemonconsts.DesiredMachineConfigAnnotationKey] == targetConfig {
+			if hasInProgressTaint {
+				if err := ctrl.removeUpdateInProgressTaint(ctx, node.Name); err != nil {
+					return goerrs.Wrapf(err, "failed removing %s taint for node %s", constants.NodeUpdateInProgressTaint.Key, node.Name)
+				}
+			}
+		} else {
+			if !hasInProgressTaint {
+				if err := ctrl.setUpdateInProgressTaint(ctx, node.Name); err != nil {
+					return goerrs.Wrapf(err, "failed applying %s taint for node %s", constants.NodeUpdateInProgressTaint.Key, node.Name)
+				}
 			}
 		}
 	}
@@ -790,6 +799,16 @@ func (ctrl *Controller) syncMachineConfigPool(key string) error {
 	return ctrl.syncStatusOnly(pool)
 }
 
+// checkIfNodeHasInProgressTaint checks if the given node has in progress taint
+func checkIfNodeHasInProgressTaint(node *corev1.Node) bool {
+	for _, taint := range node.Spec.Taints {
+		if taint.MatchTaint(constants.NodeUpdateInProgressTaint) {
+			return true
+		}
+	}
+	return false
+}
+
 func (ctrl *Controller) getNodesForPool(pool *mcfgv1.MachineConfigPool) ([]*corev1.Node, error) {
 	selector, err := metav1.LabelSelectorAsSelector(pool.Spec.NodeSelector)
 	if err != nil {
@@ -980,7 +999,7 @@ func (ctrl *Controller) updateCandidateMachines(pool *mcfgv1.MachineConfigPool,
 }
 
 // setUpdateInProgressTaint applies in progress taint to all the nodes that are to be updated.
-// The taint on the individual node is removed by MCD once the update of the node is complete.
+// The taint on the individual node is removed by MCC once the update of the node is complete.
 // This is to ensure that the updated nodes are being preferred to non-updated nodes there by
 // reducing the number of reschedules.
 func (ctrl *Controller) setUpdateInProgressTaint(ctx context.Context, nodeName string) error {
@@ -999,13 +1018,47 @@ func (ctrl *Controller) setUpdateInProgressTaint(ctx context.Context, nodeName s
 			newNode.Spec.Taints = []corev1.Taint{}
 		}
 
+		newNode.Spec.Taints = append(newNode.Spec.Taints, *constants.NodeUpdateInProgressTaint)
+		newData, err := json.Marshal(newNode)
+		if err != nil {
+			return err
+		}
+
+		patchBytes, err := strategicpatch.CreateTwoWayMergePatch(oldData, newData, corev1.Node{})
+		if err != nil {
+			return fmt.Errorf("failed to create patch for node %q: %v", nodeName, err)
+		}
+		_, err = ctrl.kubeClient.CoreV1().Nodes().Patch(ctx, nodeName, types.StrategicMergePatchType, patchBytes, metav1.PatchOptions{})
+		return err
+	})
+}
+
+// removeUpdateInProgressTaint removes the update in progress taint for given node.
+func (ctrl *Controller) removeUpdateInProgressTaint(ctx context.Context, nodeName string) error {
+	return clientretry.RetryOnConflict(constants.NodeUpdateBackoff, func() error {
+		oldNode, err := ctrl.kubeClient.CoreV1().Nodes().Get(ctx, nodeName, metav1.GetOptions{})
+		if err != nil {
+			return err
+		}
+		oldData, err := json.Marshal(oldNode)
+		if err != nil {
+			return err
+		}
+
+		newNode := oldNode.DeepCopy()
+
+		// New taints to be copied.
+		var taintsAfterUpgrade []corev1.Taint
 		for _, taint := range newNode.Spec.Taints {
 			if taint.MatchTaint(constants.NodeUpdateInProgressTaint) {
-				return nil
+				continue
+			} else {
+				taintsAfterUpgrade = append(taintsAfterUpgrade, taint)
 			}
 		}
 
-		newNode.Spec.Taints = append(newNode.Spec.Taints, *constants.NodeUpdateInProgressTaint)
+		// Remove the NodeUpdateInProgressTaint.
+		newNode.Spec.Taints = taintsAfterUpgrade
 		newData, err := json.Marshal(newNode)
 		if err != nil {
 			return err

pkg/controller/node/node_controller_test.go

@@ -786,42 +786,45 @@ func TestSetDesiredMachineConfigAnnotation(t *testing.T) {
 func TestShouldMakeProgress(t *testing.T) {
 	// nodeWithDesiredConfigTaints is at desired config, so need to do a get on the nodeWithDesiredConfigTaints to check for the taint status
 	nodeWithDesiredConfigTaints := newNodeWithLabel("nodeWithDesiredConfigTaints", "v1", "v1", map[string]string{"node-role/worker": "", "node-role/infra": ""})
-	// Update nodeWithDesiredConfigTaints to have the needed taint, this should still have no effect
-	nodeWithDesiredConfigTaints.Spec.Taints = []corev1.Taint{*constants.NodeUpdateInProgressTaint}
+	// Update nodeWithDesiredConfigTaints to have the needed taint and some dummy taint, UpdateInProgress taint should be removed
+	nodeWithDesiredConfigTaints.Spec.Taints = []corev1.Taint{*constants.NodeUpdateInProgressTaint, {Key: "dummy", Effect: corev1.TaintEffectPreferNoSchedule}}
 	// nodeWithNoDesiredConfigButTaints
 	nodeWithNoDesiredConfigButTaints := newNodeWithLabel("nodeWithNoDesiredConfigButTaints", "v0", "v0", map[string]string{"node-role/worker": "", "node-role/infra": ""})
 	nodeWithNoDesiredConfigButTaints.Spec.Taints = []corev1.Taint{*constants.NodeUpdateInProgressTaint}
 	tests := []struct {
-		description           string
-		node                  *corev1.Node
-		expectAnnotationPatch bool
-		expectTaintsPatch     bool
-		expectTaintsGet       bool
+		description             string
+		node                    *corev1.Node
+		expectAnnotationPatch   bool
+		expectTaintsAddPatch    bool
+		expectTaintsRemovePatch bool
+		expectTaintsGet         bool
+		expectedNodeGet         int
 	}{
 		{
 			description:           "node at desired config no patch on annotation or taints",
 			node:                  newNodeWithLabel("nodeAtDesiredConfig", "v1", "v1", map[string]string{"node-role/worker": "", "node-role/infra": ""}),
 			expectAnnotationPatch: false,
-			expectTaintsPatch:     false,
+			expectTaintsAddPatch:  false,
 		},
 		{
 			description:           "node not at desired config, patch on annotation and taints",
 			node:                  newNodeWithLabel("nodeNeedingUpdates", "v0", "v0", map[string]string{"node-role/worker": "", "node-role/infra": ""}),
 			expectAnnotationPatch: true,
-			expectTaintsPatch:     true,
+			expectTaintsAddPatch:  true,
 		},
 		{
-			description:           "node at desired config, no patch on annotation or taints",
-			node:                  nodeWithDesiredConfigTaints,
-			expectAnnotationPatch: false,
-			expectTaintsPatch:     false,
+			description:             "node at desired config, no patch on annotation but taint should be removed",
+			node:                    nodeWithDesiredConfigTaints,
+			expectAnnotationPatch:   false,
+			expectTaintsAddPatch:    false,
+			expectTaintsRemovePatch: true,
+			expectedNodeGet:         1,
 		},
 		{
 			description:           "node not at desired config, patch on annotation but not on taint",
 			node:                  nodeWithNoDesiredConfigButTaints,
 			expectAnnotationPatch: true,
-			expectTaintsPatch:     false,
-			expectTaintsGet:       true,
+			expectTaintsAddPatch:  false,
 		},
 	}
 	for _, test := range tests {
@@ -848,9 +851,21 @@ func TestShouldMakeProgress(t *testing.T) {
 			var oldData, newData, exppatch []byte
 			var err error
 			expNode := nodes[1].DeepCopy()
-			if test.expectTaintsPatch {
-				f.expectGetNodeAction(nodes[1])
+			if test.expectTaintsRemovePatch {
+				var taints []corev1.Taint
+				for _, taint := range expNode.Spec.Taints {
+					if taint.MatchTaint(constants.NodeUpdateInProgressTaint) {
+						continue
+					} else {
+						taints = append(taints, taint)
+					}
+				}
+				expNode.Spec.Taints = taints
+			} else if test.expectTaintsAddPatch {
 				expNode.Spec.Taints = append(expNode.Spec.Taints, *constants.NodeUpdateInProgressTaint)
+			}
+			if test.expectTaintsAddPatch || test.expectTaintsRemovePatch {
+				f.expectGetNodeAction(nodes[1])
 				oldData, err = json.Marshal(nodes[1])
 				if err != nil {
 					t.Fatal(err)

pkg/daemon/daemon.go

@@ -23,10 +23,7 @@ import (
 	"golang.org/x/time/rate"
 	corev1 "k8s.io/api/core/v1"
 	apierrors "k8s.io/apimachinery/pkg/api/errors"
-	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
-	"k8s.io/apimachinery/pkg/types"
 	utilruntime "k8s.io/apimachinery/pkg/util/runtime"
-	"k8s.io/apimachinery/pkg/util/strategicpatch"
 	"k8s.io/apimachinery/pkg/util/wait"
 	coreinformersv1 "k8s.io/client-go/informers/core/v1"
 	"k8s.io/client-go/kubernetes"
@@ -35,14 +32,12 @@ import (
 	corev1lister "k8s.io/client-go/listers/core/v1"
 	"k8s.io/client-go/tools/cache"
 	"k8s.io/client-go/tools/record"
-	clientretry "k8s.io/client-go/util/retry"
 	"k8s.io/client-go/util/workqueue"
 	"k8s.io/kubectl/pkg/drain"
 
 	configv1 "github.com/openshift/api/config/v1"
 	mcoResourceRead "github.com/openshift/machine-config-operator/lib/resourceread"
 	mcfgv1 "github.com/openshift/machine-config-operator/pkg/apis/machineconfiguration.openshift.io/v1"
-	commonconstants "github.com/openshift/machine-config-operator/pkg/constants"
 	ctrlcommon "github.com/openshift/machine-config-operator/pkg/controller/common"
 	"github.com/openshift/machine-config-operator/pkg/daemon/constants"
 	mcfginformersv1 "github.com/openshift/machine-config-operator/pkg/generated/informers/externalversions/machineconfiguration.openshift.io/v1"
@@ -1401,60 +1396,12 @@ func (dn *Daemon) completeUpdate(desiredConfigName string) error {
 	if err := dn.cordonOrUncordonNode(false); err != nil {
 		return err
 	}
-
-	ctx := context.TODO()
-	if err := dn.removeUpdateInProgressTaint(ctx); err != nil {
-		return err
-	}
-
 	dn.logSystem("Update completed for config %s and node has been successfully uncordoned", desiredConfigName)
 	dn.recorder.Eventf(getNodeRef(dn.node), corev1.EventTypeNormal, "Uncordon", fmt.Sprintf("Update completed for config %s and node has been uncordoned", desiredConfigName))
 
 	return nil
 }
 
-func (dn *Daemon) removeUpdateInProgressTaint(ctx context.Context) error {
-	return clientretry.RetryOnConflict(commonconstants.NodeUpdateBackoff, func() error {
-		oldNode, err := dn.kubeClient.CoreV1().Nodes().Get(ctx, dn.name, metav1.GetOptions{})
-		if err != nil {
-			return err
-		}
-		oldData, err := json.Marshal(oldNode)
-		if err != nil {
-			return err
-		}
-
-		newNode := oldNode.DeepCopy()
-
-		// New taints to be copied.
-		var taintsAfterUpgrade []corev1.Taint
-		for _, taint := range newNode.Spec.Taints {
-			if taint.MatchTaint(commonconstants.NodeUpdateInProgressTaint) {
-				continue
-			} else {
-				taintsAfterUpgrade = append(taintsAfterUpgrade, taint)
-			}
-		}
-		// updateInProgress taint is not there, so no need to patch the node object, return immediately
-		if len(taintsAfterUpgrade) == len(newNode.Spec.Taints) {
-			return nil
-		}
-		// Remove the NodeUpdateInProgressTaint.
-		newNode.Spec.Taints = taintsAfterUpgrade
-		newData, err := json.Marshal(newNode)
-		if err != nil {
-			return err
-		}
-
-		patchBytes, err := strategicpatch.CreateTwoWayMergePatch(oldData, newData, corev1.Node{})
-		if err != nil {
-			return fmt.Errorf("failed to create patch for node %q: %v", dn.name, err)
-		}
-		_, err = dn.kubeClient.CoreV1().Nodes().Patch(ctx, dn.name, types.StrategicMergePatchType, patchBytes, metav1.PatchOptions{})
-		return err
-	})
-}
-
 // triggerUpdateWithMachineConfig starts the update. It queries the cluster for
 // the current and desired config if they weren't passed.
 func (dn *Daemon) triggerUpdateWithMachineConfig(currentConfig, desiredConfig *mcfgv1.MachineConfig) error {