From 81cc655ce61e22a44780544079bf690e1ce29bb1 Mon Sep 17 00:00:00 2001 From: prashanth26 Date: Tue, 22 Jan 2019 17:56:16 +0530 Subject: [PATCH] Minor changes 1. Renamed PreferNoSchedule to PreferNoScheduleKey 2. Fixed map key check --- pkg/controller/controller_utils.go | 13 +++++++-- pkg/controller/deployment_rollback.go | 33 +++++++++++----------- pkg/controller/deployment_rollback_test.go | 17 +++++++---- pkg/controller/deployment_rolling.go | 29 ++++++++++--------- pkg/controller/deployment_rolling_test.go | 15 +++++++--- pkg/controller/deployment_util.go | 6 ++-- 6 files changed, 68 insertions(+), 45 deletions(-) diff --git a/pkg/controller/controller_utils.go b/pkg/controller/controller_utils.go index b057ab757..57e9cbf15 100644 --- a/pkg/controller/controller_utils.go +++ b/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 diff --git a/pkg/controller/deployment_rollback.go b/pkg/controller/deployment_rollback.go index 3e49cade1..27bee0e8b 100644 --- a/pkg/controller/deployment_rollback.go +++ b/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 } diff --git a/pkg/controller/deployment_rollback_test.go b/pkg/controller/deployment_rollback_test.go index 091bbd469..5642362dc 100644 --- a/pkg/controller/deployment_rollback_test.go +++ b/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{ diff --git a/pkg/controller/deployment_rolling.go b/pkg/controller/deployment_rolling.go index 7bb7e0a23..44b61765d 100644 --- a/pkg/controller/deployment_rolling.go +++ b/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,12 +288,6 @@ 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 { @@ -294,7 +295,7 @@ func (dc *controller) taintNodesBackingMachineSets(MachineSets []*v1alpha1.Machi 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 diff --git a/pkg/controller/deployment_rolling_test.go b/pkg/controller/deployment_rolling_test.go index 582d1e56f..9357ddd24 100644 --- a/pkg/controller/deployment_rolling_test.go +++ b/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,7 +171,7 @@ var _ = Describe("deployment_rolling", func() { nil, nil, map[string]string{ - PreferNoSchedule: "True", + PreferNoScheduleKey: "True", }, ), nodes: newNodes( @@ -172,7 +179,7 @@ var _ = Describe("deployment_rolling", func() { &corev1.NodeSpec{ Taints: []corev1.Taint{ corev1.Taint{ - Key: PreferNoSchedule, + Key: PreferNoScheduleKey, Value: "True", Effect: "PreferNoSchedule", }, diff --git a/pkg/controller/deployment_util.go b/pkg/controller/deployment_util.go index 49ec9ad8a..5d9965564 100644 --- a/pkg/controller/deployment_util.go +++ b/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