E2E: verify daemonset pods after machines

[jackfrancis]

What type of PR is this?

/kind failing-test

What this PR does / why we need it:

This PR moves the "validate expected daemonset pods" after "verify control plane is ready", because at that point in the cluster creation flow machines are not actually online, and so daemonset pods are never going to be scheduled.

Instead, we move daemonset pod validation after ApplyClusterTemplateAndWait, which waits for control plane and worker machines. At this point in the E2E flow we will have all of our expected number of machines verified as Ready, at which point we can check for the expected daemonset pods.

Which issue(s) this PR fixes (optional, in fixes #<issue number>(, fixes #<issue_number>, ...) format, will close the issue(s) when PR gets merged):
Fixes #2933

Special notes for your reviewer:

Please confirm that if this PR changes any image versions, then that's the sole change this PR makes.

TODOs:

• squashed commits
• includes documentation
• adds unit tests

Release note:

NONE

[jackfrancis]

/assign @CecileRobertMichon @marosset @jsturtevant

[jackfrancis]

@CecileRobertMichon do you prefer this updated use of the PostMachinesProvisioned func?

@marosset this doesn't really address your valid observation. I'm not entirely certain why we wrap this test cases in a func and then invoke the input vars "just in time", rather than just passing them into the func as values. I assume there is some async value mutation that we need to properly track and that these func closures enable. Maybe @nojnhuh has more context?

[CecileRobertMichon]

@CecileRobertMichon do you prefer this updated use of the PostMachinesProvisioned func?

yes 100%. Although not sure why we need to pass in a func as param, commented on that.

@marosset has a good point, there is a lot of duplication. Perhaps we could refactor our uses of the clusterctl ApplyClusterTemplateAndWait with a wrapper helper func that adds all the common stuff like this by default and just takes the stuff that changes as parameters. That's probably out of scope here but would be a nice code cleanup.

[jackfrancis]

Now that we're actually testing for the presence of the calico-node daemonset, it's failing three of the E2E test scenarios 🤯

[CecileRobertMichon]

Now that we're actually testing for the presence of the calico-node daemonset, it's failing three of the E2E test scenarios 🤯

@jackfrancis looking at the test output it's not even finding the daemonset, not failing to wait for pods (that part was passing before). I think that's because you are using the bootstrap cluster proxy to look for the daemonsets instead of the workload cluster where calico is actually installed https://github.com/kubernetes-sigs/cluster-api-provider-azure/pull/2950/files#diff-c95013f3c197cb1edeb15be6787e403afd41244efd8ee819d5cefd24deca32dcR79

[jackfrancis]

@jsturtevant @marosset see @CecileRobertMichon's comment above, I'm attempting to address that.

tl;dr 🤦, not 🤯.

[jackfrancis]

It definitely seems like our VMSS cluster template is failing to launch Windows csi-node-driver pods. See:

https://storage.googleapis.com/kubernetes-jenkins/pr-logs/pull/kubernetes-sigs_cluster-api-provider-azure/2950/pull-cluster-api-provider-azure-e2e/1611109298186752000/artifacts/clusters/capz-e2e-jfwxpp-vmss/kube-system/csi-azuredisk-node-win-6cc4v/azuredisk.log

@marosset @jsturtevant any ideas?

[CecileRobertMichon]

It definitely seems like our VMSS cluster template is failing to launch Windows csi-node-driver pods. See:

That seems like the same error I was trying to fix in #2947

[jackfrancis]

@CecileRobertMichon indeed: #2992

[jackfrancis]

/retest

(cluster delete timeout flake)

[jackfrancis]

@CecileRobertMichon @marosset this is passing tests and ready for another review round

[CecileRobertMichon]

This lgtm and is equivalent to previous functionality (but better because we're actually waiting for machines now). One thought: have you considered listing all daemonsets and waiting for all of them to to be available instead hardcoding a select few we can about?

If we want to make sure the daemonsets for calico exist we could potentially leave the existing code waiting for them to be available in the install Calico func and then do a general "wait for all daemonsets" post node provisioning

[jackfrancis]

@CecileRobertMichon this generalized approach appears to work fine, you can see the outcomes if you search for "Waiting for all DaemonSet Pods to be Running" in E2E output.

[jackfrancis]

/test pull-cluster-api-provider-azure-e2e-optional

[mboersma]

This looks good. Squash?

[jackfrancis]

@mboersma yeah just wanna get one more eyeball on this approach so I can revert if necessary

[CecileRobertMichon]

lgtm

[jackfrancis]

/retest

[CecileRobertMichon]

/lgtm
/approve
/retest

[k8s-ci-robot]

LGTM label has been added.

Git tree hash: 98c019c3a8441432643ad24f8f5031f4f3fd7669

[k8s-ci-robot]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: CecileRobertMichon

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 [CecileRobertMichon]

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

[jackfrancis]

/retest

[jackfrancis]

/retest

[jackfrancis]

/retest

[jackfrancis]

/retest

[CecileRobertMichon]

what's up with all the flakes 👀 anything related to this change?

[jackfrancis]

• make lint timeout
• default/web-windows7jo8s4 flake during LB IIS test
• HA test delete cluster timeout
• AKS E2E flake during autoscale test (addressed here: Always run AKS E2E get+update as transaction #3058)
• AKS E2E flake during public IP test (also addressed here: Always run AKS E2E get+update as transaction #3058)

I think that's it!

[jackfrancis]

@CecileRobertMichon sorry, not related to this change, I've seen every one of those flakes on other PRs :(