🐛 Allow KCP to Update when CoreDNS version doesn't change #5986
Conversation
k8s-ci-robot
added
the
cncf-cla: yes
killianmuldoon
changed the title
k8s-ci-robot
added
the
size/S
|
@sbueringer I wasn't able to run the e2e this is supposed to fix properly locally - can we run them here? |
killianmuldoon
force-pushed
the
fix/kcp-coredns-check
branch
from
19ac20b to
cab94d2
Compare
| @@ -500,7 +500,7 @@ func (in *KubeadmControlPlane) validateCoreDNSVersion(prev *KubeadmControlPlane) | |||
| ) | |||
| return allErrs | |||
| } | |||
|
|
|||
| // Note: This check allows an "upgrade" in the case where the versions are equal. | |||
| if err := migration.ValidUpMigration(fromVersion.String(), toVersion.String()); err != nil { | |||
There was a problem hiding this comment.
Based on the log messages I don't think that's true:
https://storage.googleapis.com/kubernetes-jenkins/logs/periodic-cluster-api-e2e-workload-upgrade-1-23-latest-release-1-0/1483442872060481536/artifacts/clusters/bootstrap/controllers/capi-kubeadm-control-plane-controller-manager/capi-kubeadm-control-plane-controller-manager-744575bddc-qwdqp/manager.log
There was a problem hiding this comment.
Would be nice if you can also adjust the error message in l.509:
fmt.Sprintf("cannot migrate CoreDNS from '%v' to '%v': %v", fromVersion, toVersion, err),Feels way easier to read like that to me :)
There was a problem hiding this comment.
I think if we want to be able to support "no-ops" regarding CoreDNS version we can only use the validation of the migration tool here when fromVersion and toVersion are not equal
There was a problem hiding this comment.
Agreed
The behavior of ValidUpMigration is kind of subtle because it handles fromVersion==ToVersion but only if the version is known (I got this digging into the code; also the inner error in the failing jobs is "start version '1.8.6' not supported").
So, if I got it right, what we want to do here is to support upgrading the kubernetes version of Clusters created with unsupported CoreDNS version, but without changing the CoreDNS version.
In order to achieve this, IMO the simplest option is to not call ValidUpMigration if fromVersion==ToVersion; eventually in a follow-up PR we can work on a better error message the return the maximum CoreDNS version a KCP release supports (by looking at migration.ValidVersions), but let's keep this out of the table for now so we have a minimal fix that can be backported
There was a problem hiding this comment.
Another thing we should consider, it to prevent users from creating clusters with unsupported CoreDNS versions; this will make things less clear and enforce the up limit of our version support matrix, but I think we should discuss this with the community first
| if x := from.Compare(to); x > 0 || (x == 0 && fromTag == toTag) { | ||
| return fmt.Errorf("toVersion %q must be greater than fromVersion %q", toTag, fromTag) | ||
| // make sure that the version we're upgrading to is greater than or equal to the current version. | ||
| if to.LT(from) { |
There was a problem hiding this comment.
TBH I'm not sure that we need this change given that this func is not called if version are equal
cluster-api/controlplane/kubeadm/internal/workload_cluster_coredns.go
Lines 128 to 131 in 5b9922a
killianmuldoon
force-pushed
the
fix/kcp-coredns-check
branch
from
cab94d2 to
baea6e4
Compare
k8s-ci-robot
added
size/M
killianmuldoon
force-pushed
the
fix/kcp-coredns-check
branch
3 times, most recently
from
a9eeefd to
7de85c6
Compare
controlplane/kubeadm/api/v1beta1/kubeadm_control_plane_webhook.go
Outdated
Show resolved
Hide resolved
killianmuldoon
force-pushed
the
fix/kcp-coredns-check
branch
from
7de85c6 to
5dd4c6a
Compare
killianmuldoon
changed the title
|
I've tested this locally and it looks like it's solving the base issue in our e2e upgrades. |
|
@killianmuldoon |
|
@sbueringer: The specified target(s) for
The following commands are available to trigger optional jobs:
Use
In response to this:
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. |
Signed-off-by: killianmuldoon <[email protected]>
killianmuldoon
force-pushed
the
fix/kcp-coredns-check
branch
from
5dd4c6a to
d57f0f6
Compare
k8s-ci-robot
added
size/S
|
/lgtm To be absolutely safe we can wait for the upgrade test results on the cherry-pick PRs (before merging all of those) |
k8s-ci-robot
added
the
lgtm
|
/cherry-pick release-1.1 Assuming that we want to include this fix in all our currently supported versions (we already have cherry-picks for v0.4 and v1.0). |
|
@sbueringer: once the present PR merges, I will cherry-pick it on top of release-1.1 in a new PR and assign it to you. In response to this:
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. |
|
/test pull-cluster-api-e2e-workload-upgrade-1-23-latest-main |
|
@fabriziopandini I checked the test results and logs on the v0.4 and v1.0 PR, everything looks fine. I think we should be able to merge this PR and the cherry-picks. |
|
Great work @sbueringer and @killianmuldoon! |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: fabriziopandini 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:
Approvers can indicate their approval by writing |
k8s-ci-robot
added
the
approved
|
@sbueringer: new pull request created: #6011 In response to this:
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. |
Signed-off-by: killianmuldoon [email protected]
This change allows KCP to perform an upgrade when the version of CoreDNS is not increased. This will allow older branches to stay on the same CoreDNS version when performing upgrade e2e tests for later versions of Kubernetes.
Fixes #5952