From 64fce07464ee17febf79a50749661b4855096adc Mon Sep 17 00:00:00 2001 From: John Gardiner Myers Date: Tue, 20 Apr 2021 17:58:26 -0700 Subject: [PATCH 1/2] Exclude nodes from load balancers upon cordoning --- pkg/instancegroups/instancegroups.go | 39 ++++++++++++++++++++++++ pkg/instancegroups/rollingupdate_test.go | 33 ++++++++++++++++++-- 2 files changed, 70 insertions(+), 2 deletions(-) diff --git a/pkg/instancegroups/instancegroups.go b/pkg/instancegroups/instancegroups.go index 86861062c790b..9084686993e37 100644 --- a/pkg/instancegroups/instancegroups.go +++ b/pkg/instancegroups/instancegroups.go @@ -325,6 +325,38 @@ func (c *RollingUpdateCluster) patchTaint(node *corev1.Node) error { return err } +func (c *RollingUpdateCluster) patchExcludeFromLB(node *corev1.Node) error { + oldData, err := json.Marshal(node) + if err != nil { + return err + } + + if node.Labels == nil { + node.Labels = map[string]string{} + } + + if _, ok := node.Labels[corev1.LabelNodeExcludeBalancers]; ok { + return nil + } + node.Labels[corev1.LabelNodeExcludeBalancers] = "" + + newData, err := json.Marshal(node) + if err != nil { + return err + } + + patchBytes, err := strategicpatch.CreateTwoWayMergePatch(oldData, newData, node) + if err != nil { + return err + } + + _, err = c.K8sClient.CoreV1().Nodes().Patch(c.Ctx, node.Name, types.StrategicMergePatchType, patchBytes, metav1.PatchOptions{}) + if apierrors.IsNotFound(err) { + return nil + } + return err +} + func (c *RollingUpdateCluster) drainTerminateAndWait(u *cloudinstances.CloudInstance, sleepAfterTerminate time.Duration) error { instanceID := u.ID @@ -588,6 +620,13 @@ func (c *RollingUpdateCluster) drainNode(u *cloudinstances.CloudInstance) error return fmt.Errorf("error cordoning node: %v", err) } + if err := c.patchExcludeFromLB(u.Node); err != nil { + if apierrors.IsNotFound(err) { + return nil + } + return fmt.Errorf("error excluding node from load balancer: %v", err) + } + if err := drain.RunNodeDrain(helper, u.Node.Name); err != nil { if apierrors.IsNotFound(err) { return nil diff --git a/pkg/instancegroups/rollingupdate_test.go b/pkg/instancegroups/rollingupdate_test.go index 9cfa4edbff765..f4691d2a7dacf 100644 --- a/pkg/instancegroups/rollingupdate_test.go +++ b/pkg/instancegroups/rollingupdate_test.go @@ -45,8 +45,9 @@ import ( ) const ( - cordonPatch = "{\"spec\":{\"unschedulable\":true}}" - taintPatch = "{\"spec\":{\"taints\":[{\"effect\":\"PreferNoSchedule\",\"key\":\"kops.k8s.io/scheduled-for-update\"}]}}" + cordonPatch = "{\"spec\":{\"unschedulable\":true}}" + excludeLBPatch = "{\"metadata\":{\"labels\":{\"node.kubernetes.io/exclude-from-external-load-balancers\":\"\"}}}" + taintPatch = "{\"spec\":{\"taints\":[{\"effect\":\"PreferNoSchedule\",\"key\":\"kops.k8s.io/scheduled-for-update\"}]}}" ) func getTestSetup() (*RollingUpdateCluster, *awsup.MockAWSCloud) { @@ -209,6 +210,7 @@ func TestRollingUpdateAllNeedUpdate(t *testing.T) { assert.NoError(t, err, "rolling update") cordoned := "" + excluded := "" tainted := map[string]bool{} deleted := map[string]bool{} for _, action := range c.K8sClient.(*fake.Clientset).Actions() { @@ -219,6 +221,11 @@ func TestRollingUpdateAllNeedUpdate(t *testing.T) { assert.Equal(t, "", cordoned, "at most one node cordoned at a time") assert.True(t, tainted[a.GetName()], "node", a.GetName(), "tainted") cordoned = a.GetName() + } else if string(a.GetPatch()) == excludeLBPatch { + assertExclude(t, a) + assert.Equal(t, "", excluded, "at most one node excluded at a time") + assert.True(t, tainted[a.GetName()], "node", a.GetName(), "tainted") + excluded = a.GetName() } else { assertTaint(t, a) assert.Equal(t, "", cordoned, "not tainting while node cordoned") @@ -228,6 +235,7 @@ func TestRollingUpdateAllNeedUpdate(t *testing.T) { case testingclient.DeleteAction: assert.Equal(t, "nodes", a.GetResource().Resource) assert.Equal(t, cordoned, a.GetName(), "node was cordoned before delete") + assert.Equal(t, excluded, a.GetName(), "node was excluded before delete") assert.False(t, deleted[a.GetName()], "node", a.GetName(), "already deleted") if !strings.HasPrefix(a.GetName(), "master-") { assert.True(t, deleted["master-1a.local"], "master-1a was deleted before node", a.GetName()) @@ -235,6 +243,7 @@ func TestRollingUpdateAllNeedUpdate(t *testing.T) { } deleted[a.GetName()] = true cordoned = "" + excluded = "" case testingclient.ListAction: // Don't care default: @@ -809,6 +818,7 @@ func TestRollingUpdateTaintAllButOneNeedUpdate(t *testing.T) { assert.NoError(t, err, "rolling update") cordoned := "" + excluded := "" tainted := map[string]bool{} deleted := map[string]bool{} for _, action := range c.K8sClient.(*fake.Clientset).Actions() { @@ -818,6 +828,10 @@ func TestRollingUpdateTaintAllButOneNeedUpdate(t *testing.T) { assertCordon(t, a) assert.Equal(t, "", cordoned, "at most one node cordoned at a time") cordoned = a.GetName() + } else if string(a.GetPatch()) == excludeLBPatch { + assertExclude(t, a) + assert.Equal(t, "", excluded, "at most one node excluded at a time") + excluded = a.GetName() } else { assertTaint(t, a) assert.False(t, tainted[a.GetName()], "node", a.GetName(), "already tainted") @@ -826,10 +840,12 @@ func TestRollingUpdateTaintAllButOneNeedUpdate(t *testing.T) { case testingclient.DeleteAction: assert.Equal(t, "nodes", a.GetResource().Resource) assert.Equal(t, cordoned, a.GetName(), "node was cordoned before delete") + assert.Equal(t, excluded, a.GetName(), "node was excluded before delete") assert.Len(t, tainted, 2, "all nodes tainted before any delete") assert.False(t, deleted[a.GetName()], "node", a.GetName(), "already deleted") deleted[a.GetName()] = true cordoned = "" + excluded = "" case testingclient.ListAction: // Don't care default: @@ -855,6 +871,7 @@ func TestRollingUpdateMaxSurgeIgnoredForMaster(t *testing.T) { assert.NoError(t, err, "rolling update") cordoned := "" + excluded := "" tainted := map[string]bool{} deleted := map[string]bool{} for _, action := range c.K8sClient.(*fake.Clientset).Actions() { @@ -865,6 +882,11 @@ func TestRollingUpdateMaxSurgeIgnoredForMaster(t *testing.T) { assert.Equal(t, "", cordoned, "at most one node cordoned at a time") assert.True(t, tainted[a.GetName()], "node", a.GetName(), "tainted") cordoned = a.GetName() + } else if string(a.GetPatch()) == excludeLBPatch { + assertExclude(t, a) + assert.Equal(t, "", excluded, "at most one node excluded at a time") + assert.True(t, tainted[a.GetName()], "node", a.GetName(), "tainted") + excluded = a.GetName() } else { assertTaint(t, a) assert.Equal(t, "", cordoned, "not tainting while node cordoned") @@ -874,9 +896,11 @@ func TestRollingUpdateMaxSurgeIgnoredForMaster(t *testing.T) { case testingclient.DeleteAction: assert.Equal(t, "nodes", a.GetResource().Resource) assert.Equal(t, cordoned, a.GetName(), "node was cordoned before delete") + assert.Equal(t, excluded, a.GetName(), "node was excluded before delete") assert.False(t, deleted[a.GetName()], "node", a.GetName(), "already deleted") deleted[a.GetName()] = true cordoned = "" + excluded = "" case testingclient.ListAction: // Don't care default: @@ -1480,6 +1504,11 @@ func assertCordon(t *testing.T, action testingclient.PatchAction) { assert.Equal(t, cordonPatch, string(action.GetPatch())) } +func assertExclude(t *testing.T, action testingclient.PatchAction) { + assert.Equal(t, "nodes", action.GetResource().Resource) + assert.Equal(t, excludeLBPatch, string(action.GetPatch())) +} + func assertTaint(t *testing.T, action testingclient.PatchAction) { assert.Equal(t, "nodes", action.GetResource().Resource) assert.Equal(t, taintPatch, string(action.GetPatch())) From 53aaa72a8f53964718457901533e740df87843f5 Mon Sep 17 00:00:00 2001 From: John Gardiner Myers Date: Tue, 20 Apr 2021 21:35:20 -0700 Subject: [PATCH 2/2] Older version of k8s.io/api missing const --- pkg/instancegroups/instancegroups.go | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/pkg/instancegroups/instancegroups.go b/pkg/instancegroups/instancegroups.go index 9084686993e37..4bb4a602a98d2 100644 --- a/pkg/instancegroups/instancegroups.go +++ b/pkg/instancegroups/instancegroups.go @@ -41,6 +41,7 @@ import ( ) const rollingUpdateTaintKey = "kops.k8s.io/scheduled-for-update" +const labelNodeExcludeBalancers = "node.kubernetes.io/exclude-from-external-load-balancers" // promptInteractive asks the user to continue, mostly copied from vendor/google.golang.org/api/examples/gmail.go. func promptInteractive(upgradedHostID, upgradedHostName string) (stopPrompting bool, err error) { @@ -335,10 +336,10 @@ func (c *RollingUpdateCluster) patchExcludeFromLB(node *corev1.Node) error { node.Labels = map[string]string{} } - if _, ok := node.Labels[corev1.LabelNodeExcludeBalancers]; ok { + if _, ok := node.Labels[labelNodeExcludeBalancers]; ok { return nil } - node.Labels[corev1.LabelNodeExcludeBalancers] = "" + node.Labels[labelNodeExcludeBalancers] = "" newData, err := json.Marshal(node) if err != nil {