⚠️ Add MachinePool to e2e framework log collector

[CecileRobertMichon]

What this PR does / why we need it: Currently, framework only looks for Machines when collecting node logs. This PR adds MachinePools and implements CollectMachinePoolLog for the Docker provider.

It also fixes the MachinePool upgrade test according to this comment.

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 partly kubernetes-sigs/cluster-api-provider-azure#1352

[CecileRobertMichon]

/test
pull-cluster-api-e2e-full-main

[k8s-ci-robot]

@CecileRobertMichon: The /test command needs one or more targets.
The following commands are available to trigger jobs:

• /test pull-cluster-api-build-main
• /test pull-cluster-api-make-main
• /test pull-cluster-api-apidiff-main
• /test pull-cluster-api-verify
• /test pull-cluster-api-test-main
• /test pull-cluster-api-e2e-main
• /test pull-cluster-api-e2e-full-main
• /test pull-cluster-api-e2e-workload-upgrade-1-21-latest-main

Use /test all to run the following jobs:

• pull-cluster-api-build-main
• pull-cluster-api-apidiff-main
• pull-cluster-api-verify
• pull-cluster-api-test-main
• pull-cluster-api-e2e-main

In response to this:

/test
pull-cluster-api-e2e-full-main

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

[CecileRobertMichon]

/test pull-cluster-api-e2e-full-main

[CecileRobertMichon]

/test pull-cluster-api-e2e-full-main

[CecileRobertMichon]

/test pull-cluster-api-e2e-full-main

[CecileRobertMichon]

/test pull-cluster-api-e2e-full-main

[CecileRobertMichon]

/test pull-cluster-api-e2e-full-main

[CecileRobertMichon]

Logs aren't getting fetched properly, it's failing with Error: No such container: machine-pool-4d1lsp-worker-4v91v0, I'm not entirely sure why since that should be the name of the container for each mp node (clusterName + instanceName) 😕

https://storage.googleapis.com/kubernetes-jenkins/pr-logs/pull/kubernetes-sigs_cluster-api/4575/pull-cluster-api-e2e-full-main/1390445581310103552/artifacts/clusters/machine-pool-4d1lsp/machine-pools/machine-pool-4d1lsp-mp-0/machine-pool-4d1lsp-worker-4v91v0/containerd.log

[fabriziopandini]

I will try to give this a spin ASAP
Also I think we should mark this PR as breaking change because providers are required to implement an additional method in their LogCollector.

[CecileRobertMichon]

/test pull-cluster-api-e2e-full-main

[CecileRobertMichon]

➜  cluster-api git:(machinepool-logs) ✗ k get cluster
NAME         PHASE
my-cluster   Provisioned
➜  cluster-api git:(machinepool-logs) ✗ k get mp worker-mp-0 -o json | jq .status.nodeRefs
[
  {
    "name": "my-cluster-worker-3vdu7w",
    "uid": "8b0a069d-48c9-4474-9607-19da526aa2b0"
  },
  {
    "name": "my-cluster-worker-pwdjur",
    "uid": "4bfac3a9-8309-48c1-bfd8-f7748d492803"
  }
]
➜  cluster-api git:(machinepool-logs) ✗ docker exec --privileged my-cluster-worker-3vdu7w kubelet --version
Kubernetes v1.19.1

not sure what I'm missing here...

[fabriziopandini]

@CecileRobertMichon There is a race condition here, because WaitForMachinePoolInstancesToBeUpgraded looks at DockerMachinePool object in order to call the upgrade completed, while instead CollectMachinePoolLog looks at MachinePool object to identify the machines to collects logs from.
So, if MachinePools lags a little bit behind in keeping up DockerMachine changes, the log collector tries to get logs for machines not existing anymore.

I see a few possible way forwards

• to make MachinePools to watch for infrastructure machines, so the object will sync up in a more deterministic way (recommended)
• to make WaitForMachinePoolInstancesToBeUpgraded to wait for the machine to be ready, thus giving some more time for things to (nice to have)
• to make CollectMachinePoolLog to look at the DockerMachinePool instead of the MachinePool object (so-so)

[CecileRobertMichon]

So I ended up updating WaitForMachinePoolInstancesToBeUpgraded. There were two problems with the current implementation:

1. it makes assumptions on the structure of DockerMachinePool.Status which aren't part of the infrastructure provider contract, which means it might not work with other infrastructure providers. Since the goal of the framework is to reuse these tests across infrastructures, this is problematic.

2. It wasn't actually waiting for the instances to be fully upgraded. This is the DockerMachinePool resource at the end of a recent test run:

status:
spec:
  providerID: docker:////machine-pool-f0c58k-dmp-machine-pool-f0c58k-dmp-0
  providerIDList:
  - docker:////machine-pool-f0c58k-worker-7wbojh
status:
  instances:
  - instanceName: worker-p29qwp
    ready: false
    version: v1.19.1
  ready: true
  replicas: 1

https://storage.googleapis.com/kubernetes-jenkins/pr-logs/pull/kubernetes-sigs_cluster-api/4575/pull-cluster-api-e2e-full-main/1394774955073212416/artifacts/clusters/bootstrap/resources/machine-pool-17nh8r/DockerMachinePool/machine-pool-f0c58k-dmp-0.yaml

The version of the instance is indeed v.1.19.1 (which is the version we're upgrading to), but the instance is not "ready", and the providerIDList still contains the old instance.

What I'm doing to attempt to solve 1) and 2) is changing WaitForMachinePoolInstancesToBeUpgraded to check for the kubelet version of the nodes that match the nodeRefs in MachinePool.Status.

EDIT: 1) to make MachinePools to watch for infrastructure machines, so the object will sync up in a more deterministic way (recommended) was already done https://github.com/kubernetes-sigs/cluster-api/blob/master/exp/controllers/machinepool_controller_phases.go#L134-L135

[CecileRobertMichon]

/test pull-cluster-api-e2e-full-main
/retest

[CecileRobertMichon]

/test pull-cluster-api-e2e-full-main

[CecileRobertMichon]

/test pull-cluster-api-e2e-full-main

[CecileRobertMichon]

/test pull-cluster-api-e2e-full-main

[CecileRobertMichon]

ok this is ready now, upgrade is working and logs are getting collected (eg)

/assign @devigned @fabriziopandini

[devigned]

One comment, but otherwise, lgtm.

[CecileRobertMichon]

/test pull-cluster-api-e2e-full-main

[CecileRobertMichon]

KCP upgrade test failed (seems unrelated)

/retest

[fabriziopandini]

/lgtm

[sbueringer]

/lgtm

[CecileRobertMichon]

/approve

[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

[sbueringer]

/retest
test-main seems to be pretty flaky at the moment

Especially interesting is: TestBootstrapTokenRotationMachinePool (https://prow.k8s.io/view/gs/kubernetes-jenkins/pr-logs/pull/kubernetes-sigs_cluster-api/4575/pull-cluster-api-test-main/1396864477156085760) (looks like we're relying on timestamps)

[k8s-ci-robot]

@CecileRobertMichon: The following test failed, say /retest to rerun all failed tests:

Test name	Commit	Details	Rerun command
pull-cluster-api-apidiff-main	5f50069	link	/test pull-cluster-api-apidiff-main

Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here.