Retain CoreDNS corefile when migration fails in kubeadm #84523

rajansandeep · 2019-10-29T15:13:06Z

What type of PR is this?

/kind bug

What this PR does / why we need it:

Currently, when a user chooses to skip the preflight check of CoreDNS corefile migration, the kubeadm upgrade fails. This fix bypasses the migration step and retains the existing Corefile in case there is a preflight error and the user chooses to skip.

Which issue(s) this PR fixes:

Fixes #84326

Special notes for your reviewer:

Does this PR introduce a user-facing change?:

NONE

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

rajansandeep · 2019-10-29T15:13:39Z

/cc @chrisohaver @neolit123

neolit123 · 2019-10-29T15:16:40Z

cmd/kubeadm/app/phases/addons/dns/dns.go

-			return err
+			// Errors in Corefile Migration is verified during preflight checks. This part will be executed should a user choose
+			// to ignore preflight check errors.
+			klog.V(2).Infof("the Corefile Migration did not occur due to an error: %v. The existing CoreDNS Corefile configuration has been retained.", err)


probably better to have this as a klog.Warning(

a user choose -> a user chose
chose in past tense here?

neolit123 · 2019-10-29T15:18:16Z

cmd/kubeadm/app/phases/upgrade/preflight.go

@@ -108,7 +108,7 @@ func checkMigration(client clientset.Interface) error {

 	_, err = migration.Migrate(currentInstalledCoreDNSversion, kubeadmconstants.CoreDNSVersion, corefile, false)
 	if err != nil {
-		return err
+		return errors.Errorf("the CoreDNS configuration migration will not occur due to an error: %v", err)


should be errors.Wrap(err, ....)

neolit123 · 2019-10-29T15:21:56Z

thanks @rajansandeep
just noting, i think this is the first time we add logic in kubeadm where ignoring a preflight check propagates into areas outside of preflight. but it feels fine to me.

/priority important-longterm
/approve
leaving LGTM to the folks on CC.

k8s-ci-robot · 2019-10-29T15:22:52Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: neolit123, rajansandeep

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:

~~cmd/kubeadm/OWNERS~~ [neolit123]

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

neolit123 · 2019-10-29T15:22:54Z

/hold
for the minor nits.

chrisohaver

wording tweaks, and spell out possible repercussions of not migrating the coredns config

chrisohaver · 2019-10-29T15:19:41Z

cmd/kubeadm/app/phases/upgrade/preflight.go

@@ -108,7 +108,7 @@ func checkMigration(client clientset.Interface) error {

 	_, err = migration.Migrate(currentInstalledCoreDNSversion, kubeadmconstants.CoreDNSVersion, corefile, false)
 	if err != nil {
-		return err
+		return errors.Errorf("the CoreDNS configuration migration will not occur due to an error: %v", err)


Suggested change

return errors.Errorf("the CoreDNS configuration migration will not occur due to an error: %v", err)

return errors.Errorf("the CoreDNS configuration will not be migrated, and may be incompatible with the upgraded version of CoreDNS: %v", err)

chrisohaver · 2019-10-29T15:21:09Z

cmd/kubeadm/app/phases/addons/dns/dns.go

-			return err
+			// Errors in Corefile Migration is verified during preflight checks. This part will be executed should a user choose
+			// to ignore preflight check errors.
+			klog.V(2).Infof("the Corefile Migration did not occur due to an error: %v. The existing CoreDNS Corefile configuration has been retained.", err)


Suggested change

klog.V(2).Infof("the Corefile Migration did not occur due to an error: %v. The existing CoreDNS Corefile configuration has been retained.", err)

klog.V(2).Infof("the CoreDNS Configuration was not migrated: %v. The existing CoreDNS Corefile configuration has been retained.", err)

chrisohaver · 2019-10-29T15:36:05Z

IMO, this fix isn't going to solve custom CoreDNS problems alone.

It needs to be documented somewhere that users with custom CoreDNS need to use upgrade phases instead, skipping the CoreDNS upgrade, and doing the CoreDNS upgrade manually (and manually migrating CoreDNS config if necessary).
If a user of a custom CoreDNS upgrades using the normal kubeadm upgrade process, and ignores the warning, then their custom CoreDNS will get replaced with a stock version of CoreDNS. In most cases this would result in a non-functioning DNS after the upgrade finishes. The most common reason for using a custom CoreDNS compile is to use external plugins, and an external plugin present in a Corefile will cause a stock CoreDNS to exit before starting.

Perhaps we could succinctly explain this in the preflight warning...

"... clusters with CoreDNS deployments should use upgrade phases to skip CoreDNS upgrade ..."

neolit123 · 2019-10-29T15:57:40Z

"... clusters with CoreDNS deployments should use upgrade phases to skip CoreDNS upgrade ..."

adding something like that SGTM, we can do that once phase support for upgrade apply is added:
kubernetes/kubeadm#1318

chrisohaver · 2019-10-29T16:17:15Z

i think this is the first time we add logic in kubeadm where ignoring a preflight check propagates into areas outside of preflight. but it feels fine to me.

I don't think this change actually does that.

neolit123 · 2019-10-29T16:29:09Z

I don't think this change actually does that.

my mistake, it does not.

chrisohaver · 2019-10-29T16:36:46Z

Actually, now i'm thinking it makes more sense retain both the original corefile AND the original coredns deployment when the migration fails for any reason.

That changes the preflight warning to something along the lines of: "CoreDNS will not be upgraded: (error)"

neolit123 · 2019-11-04T20:16:40Z

@rajansandeep WDYT about @chrisohaver 's last comment?
it seems fine to me.

rajansandeep · 2019-11-04T20:21:36Z

Actually, now i'm thinking it makes more sense retain both the original corefile AND the original coredns deployment when the migration fails for any reason.
That changes the preflight warning to something along the lines of: "CoreDNS will not be upgraded: (error)"

@neolit123 Yes, I'll push out a commit to make the changes @chrisohaver suggested.

chrisohaver · 2019-11-06T16:12:40Z

cmd/kubeadm/app/phases/upgrade/preflight.go

@@ -108,7 +108,7 @@ func checkMigration(client clientset.Interface) error {

 	_, err = migration.Migrate(currentInstalledCoreDNSversion, kubeadmconstants.CoreDNSVersion, corefile, false)
 	if err != nil {
-		return errors.Wrap(err, "the CoreDNS configuration will not be migrated, and may be incompatible with the upgraded version of CoreDNS")
+		return errors.Wrap(err, "CoreDNS will not be upgraded")


~~I think we should include the error in the message.~~

Never mind - it's already doing that. didn't notice the "Wrap"

neolit123 · 2019-11-07T16:30:59Z

SGTM, thanks for the updates.
the PR is approved.
leaving LGTM to @chrisohaver

chrisohaver · 2019-11-07T16:36:45Z

/lgtm

rajansandeep · 2019-11-07T16:38:32Z

/retest

rajansandeep · 2019-11-08T14:07:37Z

/hold cancel

…#84523-upstream-release-1.16 Automated cherry pick of #84523: retain corefile when migration fails

k8s-ci-robot requested review from chrisohaver and neolit123 October 29, 2019 15:13

k8s-ci-robot added area/kubeadm sig/cluster-lifecycle Categorizes an issue or PR as relevant to SIG Cluster Lifecycle. and removed needs-sig Indicates an issue or PR lacks a `sig/foo` label and requires one. labels Oct 29, 2019

k8s-ci-robot requested review from kad and yagonobre October 29, 2019 15:15

neolit123 reviewed Oct 29, 2019

View reviewed changes

k8s-ci-robot added priority/important-longterm Important over the long term, but may not be staffed and/or may need multiple releases to complete. and removed needs-priority Indicates a PR lacks a `priority/foo` label and requires one. labels Oct 29, 2019

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Oct 29, 2019

k8s-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Oct 29, 2019

chrisohaver reviewed Oct 29, 2019

View reviewed changes

retain corefile when migration fails

7074f28

rajansandeep force-pushed the skipmigration branch from 94a9972 to 7074f28 Compare October 29, 2019 16:12

skip deployment update if migration fails

16191db

k8s-ci-robot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. and removed size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. labels Nov 6, 2019

chrisohaver reviewed Nov 6, 2019

View reviewed changes

rajansandeep requested a review from neolit123 November 7, 2019 15:52

k8s-ci-robot assigned chrisohaver Nov 7, 2019

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Nov 7, 2019

chrisohaver mentioned this pull request Nov 8, 2019

Coredns migration causing upgrade fail to 1.16.x using kubeadm coredns/corefile-migration#23

Closed

k8s-ci-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Nov 8, 2019

k8s-ci-robot merged commit ab1b374 into kubernetes:master Nov 8, 2019

k8s-ci-robot added this to the v1.17 milestone Nov 8, 2019

rajansandeep mentioned this pull request Nov 11, 2019

Automated cherry pick of #84523: retain corefile when migration fails #85096

Merged

k8s-ci-robot added a commit that referenced this pull request Dec 2, 2019

Merge pull request #85096 from rajansandeep/automated-cherry-pick-of-…

dabc2bd

…#84523-upstream-release-1.16 Automated cherry pick of #84523: retain corefile when migration fails

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Retain CoreDNS corefile when migration fails in kubeadm #84523

Retain CoreDNS corefile when migration fails in kubeadm #84523

rajansandeep commented Oct 29, 2019

rajansandeep commented Oct 29, 2019

neolit123 Oct 29, 2019

neolit123 Oct 29, 2019 •

edited

neolit123 Oct 29, 2019

neolit123 commented Oct 29, 2019

k8s-ci-robot commented Oct 29, 2019

neolit123 commented Oct 29, 2019

chrisohaver left a comment

chrisohaver Oct 29, 2019

chrisohaver Oct 29, 2019

chrisohaver commented Oct 29, 2019 •

edited

neolit123 commented Oct 29, 2019

chrisohaver commented Oct 29, 2019

neolit123 commented Oct 29, 2019

chrisohaver commented Oct 29, 2019

neolit123 commented Nov 4, 2019

rajansandeep commented Nov 4, 2019

chrisohaver Nov 6, 2019 •

edited

neolit123 commented Nov 7, 2019

chrisohaver commented Nov 7, 2019

rajansandeep commented Nov 7, 2019

rajansandeep commented Nov 8, 2019

	return errors.Errorf("the CoreDNS configuration migration will not occur due to an error: %v", err)
	return errors.Errorf("the CoreDNS configuration will not be migrated, and may be incompatible with the upgraded version of CoreDNS: %v", err)

	klog.V(2).Infof("the Corefile Migration did not occur due to an error: %v. The existing CoreDNS Corefile configuration has been retained.", err)
	klog.V(2).Infof("the CoreDNS Configuration was not migrated: %v. The existing CoreDNS Corefile configuration has been retained.", err)

Retain CoreDNS corefile when migration fails in kubeadm #84523

Retain CoreDNS corefile when migration fails in kubeadm #84523

Conversation

rajansandeep commented Oct 29, 2019

rajansandeep commented Oct 29, 2019

neolit123 Oct 29, 2019

Choose a reason for hiding this comment

neolit123 Oct 29, 2019 • edited

Choose a reason for hiding this comment

neolit123 Oct 29, 2019

Choose a reason for hiding this comment

neolit123 commented Oct 29, 2019

k8s-ci-robot commented Oct 29, 2019

neolit123 commented Oct 29, 2019

chrisohaver left a comment

Choose a reason for hiding this comment

chrisohaver Oct 29, 2019

Choose a reason for hiding this comment

chrisohaver Oct 29, 2019

Choose a reason for hiding this comment

chrisohaver commented Oct 29, 2019 • edited

neolit123 commented Oct 29, 2019

chrisohaver commented Oct 29, 2019

neolit123 commented Oct 29, 2019

chrisohaver commented Oct 29, 2019

neolit123 commented Nov 4, 2019

rajansandeep commented Nov 4, 2019

chrisohaver Nov 6, 2019 • edited

Choose a reason for hiding this comment

neolit123 commented Nov 7, 2019

chrisohaver commented Nov 7, 2019

rajansandeep commented Nov 7, 2019

rajansandeep commented Nov 8, 2019

neolit123 Oct 29, 2019 •

edited

chrisohaver commented Oct 29, 2019 •

edited

chrisohaver Nov 6, 2019 •

edited