Make sure we use correct CoreDNS image tags in our upgrade tests

[sbueringer]

Currently we have a mixture of using CoreDNS tags with and without v in our Kubernetes upgrade test.
We are also not verifying that CoreDNS actually comes up, we only wait until the deployment has the new image tag.

Some context:

• It's about the CoreDNS tag we're upgrading to which is configured via COREDNS_VERSION_UPGRADE_TO
• There was a migration from k8s.gcr.io/coredns to k8s.gcr.io/coredns/coredns and the following images are available in GCR:
  • k8s.gcr.io/coredns <= 1.6.5 / 1.6.6 / 1.6.7 / 1.7.0
  • k8s.gcr.io/coredns/coredns v1.6.6 / v1.6.7 / v1.6.9 / v1.7.0 / v1.7.1 / v1.8.0 / v1.8.3 / v1.84 / v1.8.5 / v1.8.6
  • gcloud container images list-tags k8s.gcr.io/coredns & gcloud container images list-tags k8s.gcr.io/coredns/coredns
• In KCP we are automatically switching to the new image repository with v1.8.0

So I would suggest that we use the v prefix for CoreDNS >= v1.8.0

Tasks:

• change COREDNS_VERSION_UPGRADE_TO test/e2e/config/docker.yaml: 1.8.4 => v1.8.6 (we should use the default CoreDNS version of Kubernetes 1.23)
• Check periodic and presubmit Kubernetes upgrade jobs in test-infra
• Improve Kubernetes upgrade test to verify that the Deployment is rolled out correctly
  • I think we should additionally check in WaitForDNSUpgrade that Deployment.Status.ObservedGeneration >= Deployment.Generation and Deployment.Spec.Replicas and Deployment.Status.{AvailableReplicas,UpdatedReplicas} are equal
  • kubectl rollout status checks a bit more, but I think a simpler version is good enough for us: https://github.com/kubernetes/kubernetes/blob/bfa4188123ed334d4f5dda3a79994cadf663d8f2/staging/src/k8s.io/kubectl/pkg/polymorphichelpers/rollout_status.go#L59-L92
  • I'm aware that this will make the test a bit slower, but I would prefer that over not being able to tell if our CoreDNS upgrade works.
• I think we should do something similar for the kube-proxy Daemonset in WaitForKubeProxyUpgrade
  • Let's discuss this separately and try it in a separate PR.
  • I think this could break upgrade tests which are using CI images. IIRC when we are injecting a Kubernetes CI version in CAPA/CAPO we download a specific kube-proxy version from GCS which is not available in any registry. I.e. a new kube-proxy version won't come up on old nodes as the new image wasn't downloaded there.

/kind bug
[One or more /area label. See https://github.com/kubernetes-sigs/cluster-api/labels?q=area for the list of labels]

[sbueringer]

/cc @fabriziopandini @detiber

I'm absolutely not sure about the last task, but I would flag the issue as good-first issue for the first and third if it sounds good to you.

[fabriziopandini]

/area testing
/milestone v1.2
+1 on the overall plan; should consider to add the last point behind a flag, so we allow to opt-out if required
as an additional point, we should consider also hardening webhook validation

[sbueringer]

we should consider also hardening webhook validation

Agree. My plan was to tackle that as part of: #2599 (comment)

[detiber]

/cc @fabriziopandini @detiber

I'm absolutely not sure about the last task, but I would flag the issue as good-first issue for the first and third if it sounds good to you.

The versions of CoreDNS and etcd images are provided through the e2e test config, so regardless of what versions would be used by default for CI versions, I would expect them to be overridden through config and the upgrade to still complete. The same way the default versions are not used for non-CI based upgrades today.

[detiber]

One other thing that I will add....

There is currently no verification of progress when checking for all control plane machines to be available, all machine deployment machines to be available, scaling operations for KCP/MD, and rolling out updates for KCP/MD today, so if there is a terminal failure (failure message/reason set on an owned Machine), then ;you need to wait for the wait-machine-upgrade, wait-control-plane, and/or wait-worker-nodes timeout to trigger. It would be nice if there was also a separate timeout for progress to be made that would help these types of failures to cause the test to fail quicker and in an easier to debug way.

[sbueringer]

One other thing that I will add....

There is currently no verification of progress when checking for all control plane machines to be available, all machine deployment machines to be available, scaling operations for KCP/MD, and rolling out updates for KCP/MD today, so if there is a terminal failure (failure message/reason set on an owned Machine), then ;you need to wait for the wait-machine-upgrade, wait-control-plane, and/or wait-worker-nodes timeout to trigger. It would be nice if there was also a separate timeout for progress to be made that would help these types of failures to cause the test to fail quicker and in an easier to debug way.

I wonder if we need another timeout or if we can just immediately fail once we discover that a Machine has a a terminal failure. Do you want to open a separate issue for that one? (so we can discuss further there)

The versions of CoreDNS and etcd images are provided through the e2e test config, so regardless of what versions would be used by default for CI versions, I would expect them to be overridden through config and the upgrade to still complete. The same way the default versions are not used for non-CI based upgrades today.

I'm not sure if I understand that. In the cluster upgrade test we:

• are creating KCP without CoreDNS/etcd tags set. So kubeadm will use its default versions
• are upgrading to the versions we pass in. Either the one from the e2e config or e.g. an overwrite via an env var

Today the CoreDNS version in our docker.yaml is not used at all in CI, because we always set the env var.

What part would you like to change?

[detiber]

The versions of CoreDNS and etcd images are provided through the e2e test config, so regardless of what versions would be used by default for CI versions, I would expect them to be overridden through config and the upgrade to still complete. The same way the default versions are not used for non-CI based upgrades today.

I'm not sure if I understand that. In the cluster upgrade test we:

• are creating KCP without CoreDNS/etcd tags set. So kubeadm will use its default versions
• are upgrading to the versions we pass in. Either the one from the e2e config or e.g. an overwrite via an env var

Today the CoreDNS version in our docker.yaml is not used at all in CI, because we always set the env var.

What part would you like to change?

For the CI jobs, are we not still overriding the CoreDNS/etcd image versions through config or env variables?

If so, then both CoreDNS and etcd should both be working following the upgrade just like any other upgrade scenario (assuming that the CoreDNS/etcd versions are >= the starting versions). In which case I would expect that we could verify that not only the CoreDNS Deployment is updated with the new image versions (what it appears we are doing today), but also validate that the CoreDNS Deployment has successfully rolled out and in a good state.

As it is right now, it's possible for the upgrade test to succeed even when the cluster is no longer functional after the upgrade.

I don't think this is as much of an issue for etcd, since the upgrade process would not finish if etcd is broken.

[detiber]

I think we should do something similar for the kube-proxy Daemonset in WaitForKubeProxyUpgrade

• Let's discuss this separately and try it in a separate PR.
• I think this could break upgrade tests which are using CI images. IIRC when we are injecting a Kubernetes CI version in CAPA/CAPO we download a specific kube-proxy version from GCS which is not available in any registry. I.e. a new kube-proxy version won't come up on old nodes as the new image wasn't downloaded there.

Ah, reading a bit closer, I think I understand now. Apologies for my earlier confusion. I was thinking purely about CoreDNS/etcd and not kube-proxy. For the kube-proxy daemonset, I think there are potentially a few ways we could accomplish it:

• Add the necessary steps to PreKubeadmCommands to load the images into a ci-focused template
• Add the needed images to PrePullImages in the DockerMachineSpec configs for a ci-focused template
  • I suspect that this will require extending support for images that are not in the local registry, either that or pre-adding the required image(s) to the local registry

[sbueringer]

Okay I think we have the same opinion :)

I'll flag the issue as good first issue for the first and third task (fixing docker.yaml and improving the e2e test to validate that CoreDNS comes up). I'll probably then create a new issue only for kube-proxy to see if anyone wants to take that up / if there is demand to improve this.

/good-first-issue

[k8s-ci-robot]

@sbueringer:
This request has been marked as suitable for new contributors.

Guidelines

Please ensure that the issue body includes answers to the following questions:

• Why are we solving this issue?
• To address this issue, are there any code changes? If there are code changes, what needs to be done in the code and what places can the assignee treat as reference points?
• Does this issue have zero to low barrier of entry?
• How can the assignee reach out to you for help?

For more details on the requirements of such an issue, please see here and ensure that they are met.

If this request no longer meets these requirements, the label can be removed
by commenting with the /remove-good-first-issue command.

In response to this:

Okay I think we have roughly the same opinion.

I'll flag the issue as good first issue for the first and third task (fixing docker.yaml and improving the e2e test to validate that CoreDNS comes up). I'll probably then create a new issue only for kube-proxy to see if anyone wants to take that up / if there is demand to improve this.

/good-first-issue

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.