split coredns dependency to decouple kubeadm and kube-up

[yastij]

Signed-off-by: Yassine TIJANI ytijani@vmware.com

What type of PR is this?

/kind bug
/sig release
/sig architecture
/priority important-soon

What this PR does / why we need it: decouples kubeadm and kube-up and allows bumping coredns separately. Unblocks #78033

Which issue(s) this PR fixes: ref #80546

Special notes for your reviewer:

/assign @cblecker @neolit123

Does this PR introduce a user-facing change?:

NONE

Additional documentation e.g., KEPs (Kubernetes Enhancement Proposals), usage docs, etc.:

[neolit123]

this seems like a good way to fix the blockade in #78033
TL;DR: is that we are making a change in how kubeadm is doing coredns upgrades and also bumping the coredns version in kubeadm. but we are not bumping the version in kube-up and CI is failing.

it feels to me that the sync between the two deployers is much desired in this regard, yet it should not be forced, thus this PR.

/lgtm
@kubernetes/sig-cluster-lifecycle-pr-reviews
/assign @justaugustus
cc @MrHohn

[yastij]

/retest

[justaugustus]

Just noting here that I don't want us to get in the habit of bifurcating versions for the same dependency.
However, reading through #78302 suggests that there are a few things unrelated to the CoreDNS version for kube-up.sh that would hold up that PR.

Let's get this merged to unblock the kubeadm work with a future goal to dedupe the dependency.

/lgtm
/approve

/hold
(@yastij -- can you remove the Fixes #80546 from the PR description? There's more work/discussion to be had there and I don't want that to close once this merges)

[cblecker]

I'd echo @justaugustus, that this concerns me.

I'd like some way to make sure that when we get to release time, both officially supported install paths should install the same version of CoreDNS. Is that at least documented somewhere in policy?

I have a couple questions:

• Where are we testing the kubeadm deployment path? Do we have proper coverage? (Add the ability to migrate CoreDNS configmap in kubeadm #78033 (comment))
• If we are splitting the upgrade work into two PRs, when is the deadline for the kube-up path to go in? If we don't make this deadline, will we revert the kubeadm bump?

[neolit123]

@justaugustus

Just noting here that I don't want us to get in the habit of bifurcating versions for the same dependency.

that is true, at the same time ideally kubeadm PRs should not be touching /cluster.
perhaps the alternative is to apply the upgrade logic and only then bump the versions in both deployers in a single PR to comply with the new verify/dep test.

However, reading through #78302 suggests that there are a few things unrelated to the CoreDNS version for kube-up.sh that would hold up that PR.

i see the same. will defer to the list of folks bellow [1].

@cblecker

Where are we testing the kubeadm deployment path? Do we have proper coverage? (#78033 (comment))

the test jobs here exercise both the deploy and deploy + upgrade paths for kubeadm using coredns:
https://k8s-testgrid.appspot.com/sig-cluster-lifecycle-kubeadm

If we are splitting the upgrade work into two PRs, when is the deadline for the kube-up path to go in?

[1] possibly @rajansandeep , @chrisohaver and @MrHohn can answer that.
i think the plan was - 1.16 for both deployers.

If we don't make this deadline, will we revert the kubeadm bump?

that's a good question. i personally think that we should not. there has been a few important bug fixes in coredns after 1.3.1 including scalability and it's time to move on. if kube-up is delayed maybe it can catch up next cycle.

to quote myself from earlier:

it feels to me that the sync between the two deployers is much desired in this regard, yet it should not be forced

[cblecker]

Having it be enforced technically in such a way that creates unreviewably large PRs, I can see being a problem. But considering that all our presubmits use kube-up right now, having code merging into master that isn't being tested with all components doesn't seem right to me... especially letting a release go out without it.

The board of jobs you linked is all for periodic (ci-*) jobs, rather than presubmit (pull-*).

[cblecker]

To quote my comment on the other PR (#78033 (comment)):

I'm concerned about any other possible regression (such as performance) that we aren't testing by moving kubeadm before kube-up.

[neolit123]

But considering that all our presubmits use kube-up right now, having code merging into master that isn't being tested with all components doesn't seem right to me...especially letting a release go out without it.

a few of points:

• AFAIK we don't have upgrade presubmits for kube-up either and these coredns PRs need that.
• kubeadm presubmits that are always_run: true saw resistance in the past. maybe the work @BenTheElder is doing right now will make this happen.
• the PR for kubeadm only touches /cmd/kubeadm, which is technically an addon modification for a certain deployer and it's up to the deployer maintainers to resolve regressions.
• i was actually trying to completely disable kube-up based presubmits on the /cmd/kubeadm namespace, but run_if_changed makes this difficult.
• enabling kubeadm presubmits for run_if_changed: /cmd/kubeadm` is viable today.

The board of jobs you linked is all for periodic (ci-) jobs, rather than presubmit (pull-).

these periodics are pretty well monitored.

[rajansandeep]

@cblecker @neolit123 I will be updating the upgrade logic for CoreDNS in kube-up (soon) the same way the upgrade migration logic is underway for kubeadm.
Since this is done is separate PRs, it is better that the CoreDNS version is bumped up on the respective PRs.
This is mainly to avoid what happened in the last release, where I had initially updated the CoreDNS version and then we started to observe CI test failures (the PRs for the migration logic wasn’t merged yet), causing panic and eventually we had to revert back the CoreDNS version.

I foresee the split to update the version to happen only this time since this is a major change to how the upgrade in CoreDNS will be handled.
It is the intention to always try and ensure that the CoreDNS version always be in sync in kubeadm and kube-up.

[neolit123]

I'm concerned about any other possible regression (such as performance) that we aren't testing by moving kubeadm before kube-up.

that is true. we do not have performance tests in the kubeadm jobs, so some of the kube-up tests might catch that.

@chrisohaver do you have performance tests on the coredns repository side?
(i'm sure there should be some).

[cblecker]

I foresee the split to update the version to happen only this time since this is a major change to how the upgrade in CoreDNS will be handled.
It is the intention to always try and ensure that the CoreDNS version always be in sync in kubeadm and kube-up.

Then perhaps we revert this change after this upgrade is done? Either way, the intent here is to update kube-up in short order, but just not the same exact PR. That addresses my main concern.. I just don't want to forget kube-up, or leave it to really really late in the cycle and then have to make tough decisions.

Do you have a time estimate on the kube-up version of this upgrade work @rajansandeep? Is there an PR open already (it's okay if not, but if there is I'd like to follow it)?

cc: @lachie83 -- Just to make sure this is on your radar.

[rajansandeep]

@cblecker yes, I think it’s safe to say that this PR can be reverted after the upgrade in kubeadm(#78033) and Kube-up(#78302) is done.

I’ll most likely update the PR for kube-up tomorrow so that there is enough time to test out the upgrade/downgrade in the CI tests.

[cblecker]

@rajansandeep Fantastic! All my concerns are more than addressed. Will follow along in those PRs.

/lgtm
/approve
/hold cancel

[k8s-ci-robot]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: cblecker, justaugustus, yastij

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:

• build/OWNERS [cblecker]

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

[BenTheElder]

sounds good to me

this problem [coordinating versions and testing ...] will be tricker as more things move out of the mono repo ... something to think about going forward 🤔

[neolit123]

this problem [coordinating versions and testing ...] will be tricker as more things move out of the mono repo ... something to think about going forward thinking

once kubeadm moves out of tree we are unlikely to enforce matching of the same coredns version as kube-up, unless the coredns maintainers prefer that.

then again, the hardcoded coredns addon in kubeadm itself will eventually move outside of kubeadm, and into the "cluster-addons" project, which will probably allow the users to install any version, but still have a sane default.

[BenTheElder]

once kubeadm moves out of tree we are unlikely to enforce matching of the same coredns version as kube-up, unless the coredns maintainers prefer that.

right, that just means there may be even more spread on what we test and more places to need to go track down updates...

then again, the hardcoded coredns addon in kubeadm itself will eventually move outside of kubeadm, and into the "cluster-addons" project, which will probably allow the users to install any version, but still have a sane default.

that would be very nice I'm hopeful for consuming this sort of thing in kind as well :-)