test validateCluster twice to make sure it does not flap

[zetaab]

fixes #8078

/kind bug

[k8s-ci-robot]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: zetaab

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

• OWNERS [zetaab]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

[johngmyers]

I think this is just papering over the problem. If a node is briefly validating before starting to throw failures again, this will cause rolling update to start draining the next node before the previous one is truly ready.

I think that after a cluster validates after terminating a node, we might want to wait a few seconds and then validate again to make sure it isn't flapping.

[johngmyers]

There are no unit tests for the new functionality. There should be.

[zetaab]

what happens when this happens:

1. master1 instancegroup is updated and finally validated using validateClusterWithDuration()
2. between two validations, the cloud-controller-manager will start in master which was just created
3. master2 instancegroup starts updating validateCluster() and fails

I do not see how this problem can be avoided without retrying? How we could know beforehand what kind of pods we have there "starting soon"? Master service pods do not exist at all in cluster when we do rolling updates to master.

I just updated 15 cluster using 1.17.0-alpha.1. The problem existed about almost half of them, so it was needed to execute rolling-update command many times before all masters was rotated.

cc @mikesplain @justinsb

[zetaab]

@johngmyers what you think about this solution what I modified now? It is much simpler, but is it so good?

[johngmyers]

I think you could meld together the logic in failAfterOneNodeClusterValidator and failThreeTimesClusterValidator to write a unit test for this.

[johngmyers]

For me, the Cilium pod gets created before the node goes into ready state and takes so long to become ready that all the other kube-system pods get created first. So I don't have a good feel for what's going on in your masters.

There is the possibility that a kube-system pod could just flap unready due to ambient chaos, coinciding with the initial validateCluster() for an instancegroup and thus causing the rolling update to fail. This is unlikely at any given time, but over multiple rolling updates could crop up noticeably.

[zetaab]

I tried to understand how these tests works without success. So I cannot write tests for this if someone does not tell what is the logic in these tests? How I could reproduce flapping behaviour in tests. Any of these validate function calls in tests is not called multiple times if I increase validation timeout

[johngmyers]

The tests use a test-specific fake ClusterValidator to produce a specified series of results and assert that the rolling update proceeds as expected.

failAfterOneNodeClusterValidator has the logic to query the state of the AutoScalingGroups in order to figure out when to act. The fake ClusterValidator for a new test would probably wait until there is one node left in the master-1 group before engaging the rest of its logic.

failThreeTimesClusterValidator has the logic to change the results from subsequent calls from validateClusterWithDuration(). So once the rolling update gets to one node left in master-1, it could return a defined sequence such as fail, succeed, fail, succeed, fail, succeed, succeed, succeed, ... One would then need an assertion somewhere (possibly in the fake validator after the rolling update has zero nodes in master-1) that the validator had been called the expected number of times.

[zetaab]

it does not work like that. Validate() is only called once, no matter how many time I will call validate in tryvalidatecluster

[zetaab]

/test pull-kops-e2e-kubernetes-aws

[joshbranham]

Seems like a sane change to me for added validation

/lgtm

[joshbranham]

/test pull-kops-e2e-kubernetes-aws