Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

[Kubeadm]: Fix CoreDNS migration logic #97016

Merged
merged 1 commit into from Dec 3, 2020
Merged

[Kubeadm]: Fix CoreDNS migration logic #97016

merged 1 commit into from Dec 3, 2020

Conversation

rajansandeep
Copy link
Contributor

@rajansandeep rajansandeep commented Dec 2, 2020 •
edited

What type of PR is this?

/kind bug

What this PR does / why we need it:
As stated in #96833 (comment), this PR fixes the logic around migrating the CoreDNS Corefile. I've added extensive tests to ensure all scenarios are covered.

Which issue(s) this PR fixes:

Fixes #96833

Special notes for your reviewer:

Does this PR introduce a user-facing change?:

kubeadm: Fixes a kubeadm upgrade bug that could cause a custom CoreDNS configuration to be replaced with the default.

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

@k8s-ci-robot k8s-ci-robot added kind/bug Categorizes issue or PR as related to a bug. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. do-not-merge/release-note-label-needed Indicates that a PR should not merge because it's missing one of the release note labels. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. do-not-merge/needs-sig Indicates an issue or PR lacks a `sig/foo` label and requires one. needs-triage Indicates an issue or PR lacks a `triage/foo` label and requires one. needs-priority Indicates a PR lacks a `priority/foo` label and requires one. release-note-none Denotes a PR that doesn't merit a release note. area/kubeadm sig/cluster-lifecycle Categorizes an issue or PR as relevant to SIG Cluster Lifecycle. and removed do-not-merge/release-note-label-needed Indicates that a PR should not merge because it's missing one of the release note labels. do-not-merge/needs-sig Indicates an issue or PR lacks a `sig/foo` label and requires one. labels Dec 2, 2020
@rajansandeep
Copy link
Contributor Author

/cc @chrisohaver @neolit123

neolit123
neolit123 reviewed Dec 2, 2020
View reviewed changes
cmd/kubeadm/app/phases/addons/dns/dns.go Outdated
return err
}
} else if corefileMigrationRequired {
//If migration is required, try and migrate the Corefile
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: missing space after //

neolit123
neolit123 reviewed Dec 2, 2020
View reviewed changes
Copy link
Member

@neolit123 neolit123 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

thanks @rajansandeep added one minor nit.
should this be backported and to which releases?

/approve

@neolit123
Copy link
Member

/priority important-soon
/milestone v1.20
/triage accepted

@k8s-ci-robot k8s-ci-robot added priority/important-soon Must be staffed and worked on either currently, or very soon, ideally in time for the next release. triage/accepted Indicates an issue or PR is ready to be actively worked on. labels Dec 2, 2020
@k8s-ci-robot k8s-ci-robot added this to the v1.20 milestone Dec 2, 2020
@k8s-ci-robot k8s-ci-robot added approved Indicates a PR has been approved by an approver from all required OWNERS files. and removed needs-priority Indicates a PR lacks a `priority/foo` label and requires one. needs-triage Indicates an issue or PR lacks a `triage/foo` label and requires one. labels Dec 2, 2020
@neolit123
Copy link
Member

NONE

we should add a release note here, e.g.:

kubeadm: fix a bug related to the detection if CoreDNS Corefile migration is required

(please, adjust as see fit)

@k8s-ci-robot k8s-ci-robot added the release-note Denotes a PR that will be considered when it comes time to generate release notes. label Dec 2, 2020
@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: chrisohaver, 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:

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

@neolit123
Copy link
Member

/hold cancel
should i sent the cherry pick for 1.19 @rajansandeep ?

@k8s-ci-robot k8s-ci-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Dec 2, 2020
@rajansandeep
Copy link
Contributor Author

should i sent the cherry pick for 1.19 @rajansandeep ?

I can send the cherry-pick. Do we have to wait for this PR to merge or I can send the cherry-pick now itself.

@neolit123
Copy link
Member

I can send the cherry-pick. Do we have to wait for this PR to merge or I can send the cherry-pick now itself.

ok, should be possible to send before merge too.

@rajansandeep
Copy link
Contributor Author

/retest

@fejta-bot
Copy link

/retest
This bot automatically retries jobs that failed/flaked on approved PRs (send feedback to fejta).

Review the full test history for this PR.

Silence the bot with an /lgtm cancel or /hold comment for consistent failures.

@pacoxu
Copy link
Member

pacoxu commented Dec 2, 2020

https://github.com/coredns/corefile-migration/blob/master/README.md#example-kubernetes-cluster-managemnt-tool-usage

I read the migration corefile suggestion. The only thing that kubeadm doesn't check here after this pr， is “unsupported plugins”.

👍

@pacoxu
Copy link
Member

pacoxu commented Dec 3, 2020 •
edited

/lgtm
/retest
For timed out waiting for the condition

@pacoxu
Copy link
Member

pacoxu commented Dec 3, 2020

/retest

@pacoxu
Copy link
Member

pacoxu commented Dec 3, 2020

still timeout
/retest

@fejta-bot
Copy link

/retest
This bot automatically retries jobs that failed/flaked on approved PRs (send feedback to fejta).

Review the full test history for this PR.

Silence the bot with an /lgtm cancel or /hold comment for consistent failures.

1 similar comment
@fejta-bot
Copy link

/retest
This bot automatically retries jobs that failed/flaked on approved PRs (send feedback to fejta).

Review the full test history for this PR.

Silence the bot with an /lgtm cancel or /hold comment for consistent failures.

@chrisohaver
Copy link
Contributor

https://github.com/coredns/corefile-migration/blob/master/README.md#example-kubernetes-cluster-managemnt-tool-usage

I read the migration corefile suggestion. The only thing that kubeadm doesn't check here after this pr， is “unsupported plugins”.

👍

IIRC, the original concept was to check for unsupported at preflight, so no need to check at this part. Don’t know if that has changed. Perhaps it’s worth adding a code comment here.

@fejta-bot
Copy link

/retest
This bot automatically retries jobs that failed/flaked on approved PRs (send feedback to fejta).

Review the full test history for this PR.

Silence the bot with an /lgtm cancel or /hold comment for consistent failures.

@rajansandeep
Copy link
Contributor Author

/retest

1 similar comment
@rajansandeep
Copy link
Contributor Author

/retest

@neolit123 neolit123 mentioned this pull request Dec 3, 2020
Closed
@neolit123
Copy link
Member

the failing tests in pull-kubernetes-e2e-gce-ubuntu-containerd are a known problem:
#97002

@fejta-bot
Copy link

/retest
This bot automatically retries jobs that failed/flaked on approved PRs (send feedback to fejta).

Review the full test history for this PR.

Silence the bot with an /lgtm cancel or /hold comment for consistent failures.

@k8s-ci-robot k8s-ci-robot merged commit 29d79de into kubernetes:master Dec 3, 2020
@rajansandeep rajansandeep mentioned this pull request Dec 3, 2020
Merged
k8s-ci-robot added a commit that referenced this pull request Dec 4, 2020
@k8s-ci-robot
Merge pull request #97059 from rajansandeep/automated-cherry-pick-of-…
06edabf 
…#97016-upstream-release-1.19

Automated cherry pick of #97016: fix migration logic
@bai
Copy link

bai commented Dec 7, 2020

/priority important-soon
/milestone v1.20
/triage accepted

Hey there, just wanted to mention that this was after test freeze and the branch cut. If you’d like this in 1.20, it needs to be cherry picked and the release is tomorrow so this probably won’t make it until 1.20.1.

@neolit123
Copy link
Member

oh well,
@rajansandeep would you like to cherry pick this to the release-1.20 branch?

@neolit123 neolit123 mentioned this pull request Dec 7, 2020
Merged
@neolit123
Copy link
Member

i've sent the cherry pick here:
#97106

k8s-ci-robot added a commit that referenced this pull request Dec 17, 2020
@k8s-ci-robot
Merge pull request #97106 from neolit123/automated-cherry-pick-of-#97…
6af63cd 
…016-origin-release-1.20

[1.20.1] Automated cherry pick of #97016: fix migration logic
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@neolit123 neolit123 neolit123 left review comments

@yagonobre yagonobre yagonobre left review comments

@chrisohaver chrisohaver chrisohaver approved these changes

@kad kad Awaiting requested review from kad

Assignees

@pacoxu pacoxu

@yagonobre yagonobre

@chrisohaver chrisohaver

Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. area/kubeadm cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. kind/bug Categorizes issue or PR as related to a bug. lgtm "Looks good to me", indicates that a PR is ready to be merged. priority/important-soon Must be staffed and worked on either currently, or very soon, ideally in time for the next release. release-note Denotes a PR that will be considered when it comes time to generate release notes. sig/cluster-lifecycle Categorizes an issue or PR as relevant to SIG Cluster Lifecycle. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. triage/accepted Indicates an issue or PR is ready to be actively worked on.
Projects
None yet
Milestone
v1.20
Development

Successfully merging this pull request may close these issues.

kubeadm upgrade (v1.19+) does not retain coredns configmap
8 participants
@rajansandeep @neolit123 @yagonobre @chrisohaver @k8s-ci-robot @fejta-bot @pacoxu @bai