Repository: kubernetes/kops (go)

Issue:
rolling-update does not validate cluster correctly after bastion
I am using 1.17.0-alpha.1 in openstack and see following:

```
kops rolling-update cluster --instance-group bastions,master-zone-1-1,master-zone-2-1,master-zone-3-1,nodes-z1,nodes-z2,nodes-z3 --validation-timeout 20m --yes

NAME		STATUS		NEEDUPDATE	READY	MIN	MAX	NODES
bastions	NeedsUpdate	1		0	1	1	0
master-zone-1-1	NeedsUpdate	1		0	1	1	1
master-zone-2-1	NeedsUpdate	1		0	1	1	1
master-zone-3-1	NeedsUpdate	1		0	1	1	1
nodes-z1	NeedsUpdate	2		0	2	2	2
nodes-z2	NeedsUpdate	2		0	2	2	2
nodes-z3	NeedsUpdate	1		0	1	1	1
I1209 12:52:50.771059      96 instancegroups.go:304] Stopping instance "7da46c57-0315-4f53-92df-1966a7c32fc7", in group "cortex-poc.k8s.local-bastions" (this may take a while).
W1209 12:52:50.771124      96 instance.go:105] This does not work without running kops update cluster --yes in another terminal
I1209 12:52:51.900333      96 instancegroups.go:189] waiting for 15s after terminating instance
I1209 12:53:06.901075      96 instancegroups.go:193] Deleted a bastion instance, 7da46c57-0315-4f53-92df-1966a7c32fc7, and continuing with rolling-update.
master not healthy after update, stopping rolling-update: "cluster \"cortex-poc.k8s.local\" did not pass validation: InstanceGroup \"bastions\" did not have enough nodes 0 vs 1"
I1209 12:53:13.534549       1 batch.go:835] error running kops rolling-update cluster --instance-group bastions,master-zone-1-1,master-zone-2-1,master-zone-3-1,nodes-z1,nodes-z2,nodes-z3 --validation-timeout 20m --yes
error running: exit status 1
```

So the kops is waiting 15 seconds to get bastion instance back, and because none autoscaler can handle that. The code will fail. Temporary fix to this is insert `--bastion-timeout=2m`. However, I think this retry logic should be fixed in kops. We need to retry validation also after bastion until validation-timeout.

Pull request description:
Validate cluster after updating bastions
/kind bug

The fix #7872 exposed an issue where rolling update starts updating the first master before the terminated bastion is replaced by the ASG. Before that fix, kops was relying on the fact that the resulting validation failure was ignored.

This fixes the issue by waiting for validation to succeed after terminating a bastion. It does not change the behavior of updating the first bastion without validating the cluster.

The other possible fix would be to make validation ignore any shortfall in members of bastion instance groups.

Fixes #8070 


Patch:
diff --git a/cmd/kops/rollingupdatecluster.go b/cmd/kops/rollingupdatecluster.go
index ddbcfe234f..acb9824940 100644
--- a/cmd/kops/rollingupdatecluster.go
+++ b/cmd/kops/rollingupdatecluster.go
@@ -413,6 +413,8 @@ func RunRollingUpdateCluster(f *util.Factory, out io.Writer, options *RollingUpd
 		ClusterName:       options.ClusterName,
 		PostDrainDelay:    options.PostDrainDelay,
 		ValidationTimeout: options.ValidationTimeout,
+		// TODO should we expose this to the UI?
+		ValidateTickDuration: 30 * time.Second,
 	}
 	return d.RollingUpdate(groups, cluster, list)
 }
diff --git a/pkg/instancegroups/instancegroups.go b/pkg/instancegroups/instancegroups.go
index b324ec9fe2..3b02488d53 100644
--- a/pkg/instancegroups/instancegroups.go
+++ b/pkg/instancegroups/instancegroups.go
@@ -189,11 +189,7 @@ func (r *RollingUpdateInstanceGroup) RollingUpdate(rollingUpdateData *RollingUpd
 		klog.Infof("waiting for %v after terminating instance", sleepAfterTerminate)
 		time.Sleep(sleepAfterTerminate)
 
-		if isBastion {
-			klog.Infof("Deleted a bastion instance, %s, and continuing with rolling-update.", instanceId)
-
-			continue
-		} else if rollingUpdateData.CloudOnly {
+		if rollingUpdateData.CloudOnly {
 			klog.Warningf("Not validating cluster as cloudonly flag is set.")
 
 		} else if featureflag.DrainAndValidateRollingUpdate.Enabled() {
@@ -227,16 +223,14 @@ func (r *RollingUpdateInstanceGroup) RollingUpdate(rollingUpdateData *RollingUpd
 
 // validateClusterWithDuration runs validation.ValidateCluster until either we get positive result or the timeout expires
 func (r *RollingUpdateInstanceGroup) validateClusterWithDuration(rollingUpdateData *RollingUpdateCluster, duration time.Duration) error {
-	// TODO should we expose this to the UI?
-	tickDuration := 30 * time.Second
 	// Try to validate cluster at least once, this will handle durations that are lower
 	// than our tick time
-	if r.tryValidateCluster(rollingUpdateData, duration, tickDuration) {
+	if r.tryValidateCluster(rollingUpdateData, duration, rollingUpdateData.ValidateTickDuration) {
 		return nil
 	}
 
 	timeout := time.After(duration)
-	tick := time.Tick(tickDuration)
+	tick := time.Tick(rollingUpdateData.ValidateTickDuration)
 	// Keep trying until we're timed out or got a result or got an error
 	for {
 		select {
@@ -245,7 +239,7 @@ func (r *RollingUpdateInstanceGroup) validateClusterWithDuration(rollingUpdateDa
 			return fmt.Errorf("cluster did not validate within a duration of %q", duration)
 		case <-tick:
 			// Got a tick, validate cluster
-			if r.tryValidateCluster(rollingUpdateData, duration, tickDuration) {
+			if r.tryValidateCluster(rollingUpdateData, duration, rollingUpdateData.ValidateTickDuration) {
 				return nil
 			}
 			// ValidateCluster didn't work yet, so let's try again
diff --git a/pkg/instancegroups/rollingupdate.go b/pkg/instancegroups/rollingupdate.go
index a943a48a11..7ae9cd27e7 100644
--- a/pkg/instancegroups/rollingupdate.go
+++ b/pkg/instancegroups/rollingupdate.go
@@ -60,6 +60,9 @@ type RollingUpdateCluster struct {
 
 	// ValidationTimeout is the maximum time to wait for the cluster to validate, once we start validation
 	ValidationTimeout time.Duration
+
+	// ValidateTickDuration is the amount of time to wait between cluster validation attempts
+	ValidateTickDuration time.Duration
 }
 
 // RollingUpdate performs a rolling update on a K8s Cluster.


Test patch:
diff --git a/pkg/instancegroups/rollingupdate_test.go b/pkg/instancegroups/rollingupdate_test.go
index a1dcb0dac9..1518cd515a 100644
--- a/pkg/instancegroups/rollingupdate_test.go
+++ b/pkg/instancegroups/rollingupdate_test.go
@@ -25,7 +25,6 @@ import (
 	"github.com/aws/aws-sdk-go/aws"
 	"github.com/aws/aws-sdk-go/service/autoscaling"
 	"github.com/stretchr/testify/assert"
-
 	v1 "k8s.io/api/core/v1"
 	v1meta "k8s.io/apimachinery/pkg/apis/meta/v1"
 	"k8s.io/client-go/kubernetes/fake"
@@ -47,14 +46,15 @@ func getTestSetup() (*RollingUpdateCluster, awsup.AWSCloud, *kopsapi.Cluster) {
 	cluster.Name = "test.k8s.local"
 
 	c := &RollingUpdateCluster{
-		Cloud:            mockcloud,
-		MasterInterval:   1 * time.Millisecond,
-		NodeInterval:     1 * time.Millisecond,
-		BastionInterval:  1 * time.Millisecond,
-		Force:            false,
-		K8sClient:        k8sClient,
-		ClusterValidator: &successfulClusterValidator{},
-		FailOnValidate:   true,
+		Cloud:                mockcloud,
+		MasterInterval:       1 * time.Millisecond,
+		NodeInterval:         1 * time.Millisecond,
+		BastionInterval:      1 * time.Millisecond,
+		Force:                false,
+		K8sClient:            k8sClient,
+		ClusterValidator:     &successfulClusterValidator{},
+		FailOnValidate:       true,
+		ValidateTickDuration: 1 * time.Millisecond,
 	}
 
 	return c, mockcloud, cluster
@@ -499,6 +499,41 @@ func TestRollingUpdateClusterErrorsValidationAfterOneNode(t *testing.T) {
 	assertGroupInstanceCount(t, cloud, "node-1", 1)
 }
 
+type failThreeTimesClusterValidator struct {
+	invocationCount int
+}
+
+func (v *failThreeTimesClusterValidator) Validate() (*validation.ValidationCluster, error) {
+	v.invocationCount++
+	if v.invocationCount <= 3 {
+		return &validation.ValidationCluster{
+			Failures: []*validation.ValidationError{
+				{
+					Kind:    "testing",
+					Name:    "testingfailure",
+					Message: "testing failure",
+				},
+			},
+		}, nil
+	}
+	return &validation.ValidationCluster{}, nil
+}
+
+func TestRollingUpdateValidatesAfterBastion(t *testing.T) {
+	c, cloud, cluster := getTestSetup()
+
+	c.ValidationTimeout = 10 * time.Millisecond
+	c.ClusterValidator = &failThreeTimesClusterValidator{}
+
+	err := c.RollingUpdate(getGroupsAllNeedUpdate(), cluster, &kopsapi.InstanceGroupList{})
+	assert.NoError(t, err, "rolling update")
+
+	assertGroupInstanceCount(t, cloud, "node-1", 0)
+	assertGroupInstanceCount(t, cloud, "node-2", 0)
+	assertGroupInstanceCount(t, cloud, "master-1", 0)
+	assertGroupInstanceCount(t, cloud, "bastion-1", 0)
+}
+
 func assertGroupInstanceCount(t *testing.T, cloud awsup.AWSCloud, groupName string, expected int) {
 	asgGroups, _ := cloud.Autoscaling().DescribeAutoScalingGroups(&autoscaling.DescribeAutoScalingGroupsInput{
 		AutoScalingGroupNames: []*string{aws.String(groupName)},