Minor changes

1. Renamed PreferNoSchedule to PreferNoScheduleKey
2. Fixed map key check

Author: prashanth26

pkg/controller/controller_utils.go

@@ -881,7 +881,9 @@ func RemoveTaintOffNode(c clientset.Interface, nodeName string, node *v1.Node, t
 	})
 }
 
-// PatchNodeTaints patches node's taints.
+// PatchNodeTaints is for updating the node taints from oldNode to the newNode
+// It makes a TwoWayMergePatch by comparing the two objects
+// It calls the Patch() method to do the final patch
 func PatchNodeTaints(c clientset.Interface, nodeName string, oldNode *v1.Node, newNode *v1.Node) error {
 	oldData, err := json.Marshal(oldNode)
 	if err != nil {
@@ -905,13 +907,18 @@ func PatchNodeTaints(c clientset.Interface, nodeName string, oldNode *v1.Node, n
 	return err
 }
 
-// UpdateNodeTaints updates node's taints.
+// UpdateNodeTaints is for updating the node taints from oldNode to the newNode
+// using the nodes Update() method
 func UpdateNodeTaints(c clientset.Interface, nodeName string, oldNode *v1.Node, newNode *v1.Node) error {
 	newNodeClone := oldNode.DeepCopy()
 	newNodeClone.Spec.Taints = newNode.Spec.Taints
 
 	_, err := c.Core().Nodes().Update(newNodeClone)
-	return err
+	if err != nil {
+		return fmt.Errorf("failed to create update taints for node %q: %v", nodeName, err)
+	}
+
+	return nil
 }
 
 // WaitForCacheSync is a wrapper around cache.WaitForCacheSync that generates log messages

pkg/controller/deployment_rollback.go

@@ -62,9 +62,16 @@ func (dc *controller) rollback(d *v1alpha1.MachineDeployment, isList []*v1alpha1
 			glog.V(4).Infof("Found machine set %q with desired revision %d", is.Name, v)
 
 			// Remove PreferNoSchedule taints from nodes which were backing the machineSet
-			err = dc.removeTaintNodesBackingMachineSets(is)
+			err = dc.removeTaintNodesBackingMachineSet(
+				is,
+				&v1.Taint{
+					Key:    PreferNoScheduleKey,
+					Value:  "True",
+					Effect: "PreferNoSchedule",
+				},
+			)
 			if err != nil {
-				glog.Warningf("Failed to remove taints %s off nodes. Error: %s", PreferNoSchedule, err)
+				glog.Warningf("Failed to remove taints %s off nodes. Error: %s", PreferNoScheduleKey, err)
 			}
 
 			// rollback by copying podTemplate.Spec from the machine set
@@ -130,16 +137,16 @@ func (dc *controller) updateMachineDeploymentAndClearRollbackTo(d *v1alpha1.Mach
 	return err
 }
 
-// removeTaintNodesBackingMachineSets removes taints from all nodes backing the machineSets
-func (dc *controller) removeTaintNodesBackingMachineSets(machineSet *v1alpha1.MachineSet) error {
+// removeTaintNodesBackingMachineSet removes taints from all nodes backing the machineSets
+func (dc *controller) removeTaintNodesBackingMachineSet(machineSet *v1alpha1.MachineSet, taint *v1.Taint) error {
 
-	if machineSet.Annotations[PreferNoSchedule] == "" {
+	if _, exists := machineSet.Annotations[taint.Key]; !exists {
 		// No taint exists
-		glog.Warningf("No taint exits on machineSet: %s. Hence not removing.", machineSet.Name)
+		glog.Warningf("No taint exists on machineSet: %s. Hence not removing.", machineSet.Name)
 		return nil
 	}
 
-	glog.V(2).Infof("Trying to untaint MachineSet object %q with %s to enable scheduling of pods", machineSet.Name, PreferNoSchedule)
+	glog.V(2).Infof("Trying to untaint MachineSet object %q with %s to enable scheduling of pods", machineSet.Name, taint.Key)
 	selector, err := metav1.LabelSelectorAsSelector(machineSet.Spec.Selector)
 	if err != nil {
 		return err
@@ -159,12 +166,6 @@ func (dc *controller) removeTaintNodesBackingMachineSets(machineSet *v1alpha1.Ma
 		return err
 	}
 
-	taints := v1.Taint{
-		Key:    PreferNoSchedule,
-		Value:  "True",
-		Effect: "PreferNoSchedule",
-	}
-
 	// Iterate through all machines and remove the PreferNoSchedule taint
 	// to avoid scheduling on older machines
 	for _, machine := range filteredMachines {
@@ -179,7 +180,7 @@ func (dc *controller) removeTaintNodesBackingMachineSets(machineSet *v1alpha1.Ma
 				dc.targetCoreClient,
 				machine.Status.Node,
 				node,
-				&taints,
+				taint,
 			)
 			if err != nil {
 				glog.Warningf("Node taint removal failed for node: %s, Error: %s", machine.Status.Node, err)
@@ -202,7 +203,7 @@ func (dc *controller) removeTaintNodesBackingMachineSets(machineSet *v1alpha1.Ma
 		}
 
 		msCopy := machineSet.DeepCopy()
-		delete(msCopy.Annotations, PreferNoSchedule)
+		delete(msCopy.Annotations, taint.Key)
 
 		machineSet, err = dc.controlMachineClient.MachineSets(msCopy.Namespace).Update(msCopy)
 
@@ -219,7 +220,7 @@ func (dc *controller) removeTaintNodesBackingMachineSets(machineSet *v1alpha1.Ma
 		// Break out of loop when update succeeds
 		break
 	}
-	glog.V(2).Infof("Removed taint %s from MachineSet object %q", PreferNoSchedule, machineSet.Name)
+	glog.V(2).Infof("Removed taint %s from MachineSet object %q", taint.Key, machineSet.Name)
 
 	return nil
 }

pkg/controller/deployment_rollback_test.go

@@ -21,13 +21,14 @@ import (
 	. "github.com/onsi/ginkgo/extensions/table"
 	. "github.com/onsi/gomega"
 	corev1 "k8s.io/api/core/v1"
+	v1 "k8s.io/api/core/v1"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 	"k8s.io/apimachinery/pkg/runtime"
 )
 
 var _ = Describe("deployment_rollback", func() {
 
-	Describe("#removeTaintNodesBackingMachineSets", func() {
+	Describe("#removeTaintNodesBackingMachineSet", func() {
 		type setup struct {
 			nodes       []*corev1.Node
 			machineSets []*machinev1.MachineSet
@@ -67,7 +68,7 @@ var _ = Describe("deployment_rollback", func() {
 			nil,
 			nil,
 			map[string]string{
-				PreferNoSchedule: "True",
+				PreferNoScheduleKey: "True",
 			},
 		)
 
@@ -93,7 +94,13 @@ var _ = Describe("deployment_rollback", func() {
 				defer trackers.Stop()
 				waitForCacheSync(stop, controller)
 
-				err := controller.removeTaintNodesBackingMachineSets(data.action)
+				err := controller.removeTaintNodesBackingMachineSet(
+					data.action,
+					&v1.Taint{
+						Key:    PreferNoScheduleKey,
+						Value:  "True",
+						Effect: "PreferNoSchedule",
+					})
 
 				if !data.expect.err {
 					Expect(err).To(BeNil())
@@ -129,7 +136,7 @@ var _ = Describe("deployment_rollback", func() {
 						&corev1.NodeSpec{
 							Taints: []corev1.Taint{
 								corev1.Taint{
-									Key:    PreferNoSchedule,
+									Key:    PreferNoScheduleKey,
 									Value:  "True",
 									Effect: "PreferNoSchedule",
 								},
@@ -154,7 +161,7 @@ var _ = Describe("deployment_rollback", func() {
 					nil,
 					nil,
 					map[string]string{
-						PreferNoSchedule: "True",
+						PreferNoScheduleKey: "True",
 					},
 				)[0],
 				expect: expect{

pkg/controller/deployment_rolling.go

@@ -50,9 +50,15 @@ func (dc *controller) rolloutRolling(d *v1alpha1.MachineDeployment, isList []*v1
 	}
 	allISs := append(oldISs, newIS)
 
-	err = dc.taintNodesBackingMachineSets(oldISs)
+	err = dc.taintNodesBackingMachineSets(
+		oldISs, &v1.Taint{
+			Key:    PreferNoScheduleKey,
+			Value:  "True",
+			Effect: "PreferNoSchedule",
+		},
+	)
 	if err != nil {
-		glog.Warningf("Failed to add %s on all nodes. Error: %s", PreferNoSchedule, err)
+		glog.Warningf("Failed to add %s on all nodes. Error: %s", PreferNoScheduleKey, err)
 	}
 
 	// Scale up, if we can.
@@ -253,15 +259,16 @@ func (dc *controller) scaleDownOldMachineSetsForRollingUpdate(allISs []*v1alpha1
 }
 
 // taintNodesBackingMachineSets taints all nodes backing the machineSets
-func (dc *controller) taintNodesBackingMachineSets(MachineSets []*v1alpha1.MachineSet) error {
+func (dc *controller) taintNodesBackingMachineSets(MachineSets []*v1alpha1.MachineSet, taint *v1.Taint) error {
 
 	for _, machineSet := range MachineSets {
 
-		if machineSet.Annotations[PreferNoSchedule] != "" {
+		if _, exists := machineSet.Annotations[taint.Key]; exists {
+			// Taint exists, hence just continue
 			continue
 		}
 
-		glog.V(3).Infof("Trying to taint MachineSet object %q with %s to avoid scheduling of pods", machineSet.Name, PreferNoSchedule)
+		glog.V(3).Infof("Trying to taint MachineSet object %q with %s to avoid scheduling of pods", machineSet.Name, taint.Key)
 		selector, err := metav1.LabelSelectorAsSelector(machineSet.Spec.Selector)
 		if err != nil {
 			return err
@@ -281,20 +288,14 @@ func (dc *controller) taintNodesBackingMachineSets(MachineSets []*v1alpha1.Machi
 			return err
 		}
 
-		taints := v1.Taint{
-			Key:    PreferNoSchedule,
-			Value:  "True",
-			Effect: "PreferNoSchedule",
-		}
-
 		// Iterate through all machines and place the PreferNoSchedule taint
 		// to avoid scheduling on older machines
 		for _, machine := range filteredMachines {
 			if machine.Status.Node != "" {
 				err = AddOrUpdateTaintOnNode(
 					dc.targetCoreClient,
 					machine.Status.Node,
-					&taints,
+					taint,
 				)
 				if err != nil {
 					glog.Warningf("Node tainting failed for node: %s, %s", machine.Status.Node, err)
@@ -319,7 +320,7 @@ func (dc *controller) taintNodesBackingMachineSets(MachineSets []*v1alpha1.Machi
 			if msCopy.Annotations == nil {
 				msCopy.Annotations = make(map[string]string, 0)
 			}
-			msCopy.Annotations[PreferNoSchedule] = "True"
+			msCopy.Annotations[taint.Key] = "True"
 
 			_, err = dc.controlMachineClient.MachineSets(msCopy.Namespace).Update(msCopy)
 			if err != nil && time.Now().Before(retryDeadline) {
@@ -335,7 +336,7 @@ func (dc *controller) taintNodesBackingMachineSets(MachineSets []*v1alpha1.Machi
 			// Break out of loop when update succeeds
 			break
 		}
-		glog.V(2).Infof("Tainted MachineSet object %q with %s to avoid scheduling of pods", machineSet.Name, PreferNoSchedule)
+		glog.V(2).Infof("Tainted MachineSet object %q with %s to avoid scheduling of pods", machineSet.Name, taint.Key)
 	}
 
 	return nil

pkg/controller/deployment_rolling_test.go

@@ -21,6 +21,7 @@ import (
 	. "github.com/onsi/ginkgo/extensions/table"
 	. "github.com/onsi/gomega"
 	corev1 "k8s.io/api/core/v1"
+	v1 "k8s.io/api/core/v1"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 	"k8s.io/apimachinery/pkg/runtime"
 )
@@ -91,8 +92,14 @@ var _ = Describe("deployment_rolling", func() {
 				defer trackers.Stop()
 				waitForCacheSync(stop, controller)
 
-				err := controller.taintNodesBackingMachineSets(data.action)
-
+				err := controller.taintNodesBackingMachineSets(
+					data.action,
+					&v1.Taint{
+						Key:    PreferNoScheduleKey,
+						Value:  "True",
+						Effect: "PreferNoSchedule",
+					},
+				)
 				if !data.expect.err {
 					Expect(err).To(BeNil())
 				} else {
@@ -164,15 +171,15 @@ var _ = Describe("deployment_rolling", func() {
 						nil,
 						nil,
 						map[string]string{
-							PreferNoSchedule: "True",
+							PreferNoScheduleKey: "True",
 						},
 					),
 					nodes: newNodes(
 						1,
 						&corev1.NodeSpec{
 							Taints: []corev1.Taint{
 								corev1.Taint{
-									Key:    PreferNoSchedule,
+									Key:    PreferNoScheduleKey,
 									Value:  "True",
 									Effect: "PreferNoSchedule",
 								},

pkg/controller/deployment_util.go

@@ -106,9 +106,9 @@ const (
 	// is deployment.spec.replicas + maxSurge. Used by the underlying machine sets to estimate their
 	// proportions in case the deployment has surge replicas.
 	MaxReplicasAnnotation = "deployment.kubernetes.io/max-replicas"
-	// PreferNoSchedule is used to identify machineSet nodes on which PreferNoSchedule taint is added on
+	// PreferNoScheduleKey is used to identify machineSet nodes on which PreferNoSchedule taint is added on
 	// older machineSets during a rolling update
-	PreferNoSchedule = "deployment.machine.sapcloud.io/prefer-no-schedule"
+	PreferNoScheduleKey = "deployment.machine.sapcloud.io/prefer-no-schedule"
 
 	// RollbackRevisionNotFound is not found rollback event reason
 	RollbackRevisionNotFound = "DeploymentRollbackRevisionNotFound"
@@ -347,7 +347,7 @@ var annotationsToSkip = map[string]bool{
 	DesiredReplicasAnnotation:      true,
 	MaxReplicasAnnotation:          true,
 	LastReplicaUpdate:              true,
-	PreferNoSchedule:               true,
+	PreferNoScheduleKey:            true,
 }
 
 // skipCopyAnnotation returns true if we should skip copying the annotation with the given annotation key