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: skip upgrade if manifest is not changed #69886

Merged
merged 1 commit into from
Oct 19, 2018
Merged

kubeadm: skip upgrade if manifest is not changed #69886

merged 1 commit into from
Oct 19, 2018

Conversation

bart0sh
Copy link
Contributor

@bart0sh bart0sh commented Oct 16, 2018 •
edited

What this PR does / why we need it:

When doing upgrades kubeadm generates new manifests and
waites for kubelet to restarts correspondent pods.

However, kubelet won't restart pods if there are no changes
in the manifests. That makes kubeadm stuck waiting for
restarted pod.

Skipping upgrades if new component manifest is the same as
current manifest should solve this.

Which issue(s) this PR fixes
Fixes: kubernetes/kubeadm#1054

Release note:

fix 'kubeadm upgrade' infinite loop waiting for pod restart

@k8s-ci-robot k8s-ci-robot added release-note-none Denotes a PR that doesn't merit a release note. size/S Denotes a PR that changes 10-29 lines, ignoring generated files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. needs-kind Indicates a PR lacks a `kind/foo` label and requires one. needs-sig Indicates an issue or PR lacks a `sig/foo` label and requires one. 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 16, 2018
neolit123
neolit123 reviewed Oct 16, 2018
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.

@bart0sh thanks! added a couple of comments.

cmd/kubeadm/app/phases/upgrade/staticpods.go Outdated
return false, err
}

return string(content1) == string(content2), nil
Copy link
Member

Choose a reason for hiding this comment

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

we should use https://golang.org/pkg/bytes/#Compare here

cmd/kubeadm/app/phases/upgrade/staticpods.go Outdated
@@ -162,6 +163,19 @@ func (spm *KubeStaticPodPathManager) CleanupDirs() error {
return nil
}

func filesEqual(path1, path2 string) (bool, error) {
Copy link
Member

Choose a reason for hiding this comment

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

i couldn't find a utility for this in k8s and kubeadm.

we should probably move it here for now:
https://github.com/kubernetes/kubernetes/blob/master/cmd/kubeadm/app/util/staticpod/utils.go
and add a unit test.

possibly call it ManifestFilesAreEqual?

cmd/kubeadm/app/phases/upgrade/staticpods.go Outdated
return err
}
if equal {
fmt.Printf("[upgrade/staticpods] current and new manifests of %s are equal, skipping upgrade", component)
Copy link
Member

Choose a reason for hiding this comment

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

i think it needs a \n at the end.

@neolit123
Copy link
Member

i think it would be best to add a release note explaining the change.

@k8s-ci-robot k8s-ci-robot added kind/cleanup Categorizes issue or PR as related to cleaning up code, process, or technical debt. and removed needs-kind Indicates a PR lacks a `kind/foo` label and requires one. labels Oct 16, 2018
@neolit123
Copy link
Member

/kind bug

@k8s-ci-robot k8s-ci-robot added the kind/bug Categorizes issue or PR as related to a bug. label Oct 16, 2018
@bart0sh bart0sh force-pushed the PR0030-kubeadm-fix-1054-upgrade-same-version branch from 3d30e73 to 9601f6e Compare October 16, 2018 18:51
@k8s-ci-robot k8s-ci-robot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. and removed size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels Oct 16, 2018
@bart0sh
Copy link
Contributor Author

bart0sh commented Oct 16, 2018

@neolit123 thank you for the review. PR has been updated, please review again.

@k8s-ci-robot k8s-ci-robot added release-note Denotes a PR that will be considered when it comes time to generate release notes. and removed release-note-none Denotes a PR that doesn't merit a release note. labels Oct 16, 2018
@bart0sh bart0sh force-pushed the PR0030-kubeadm-fix-1054-upgrade-same-version branch from 9601f6e to af11ebb Compare October 16, 2018 18:59
@k8s-ci-robot k8s-ci-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. sig/testing Categorizes an issue or PR as relevant to SIG Testing. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Oct 16, 2018
@bart0sh bart0sh force-pushed the PR0030-kubeadm-fix-1054-upgrade-same-version branch from af11ebb to b9ade1d Compare October 16, 2018 19:04
@k8s-ci-robot k8s-ci-robot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Oct 16, 2018
@bart0sh bart0sh force-pushed the PR0030-kubeadm-fix-1054-upgrade-same-version branch from b9ade1d to 0555f30 Compare October 16, 2018 19:48
@bart0sh bart0sh force-pushed the PR0030-kubeadm-fix-1054-upgrade-same-version branch from 0555f30 to 8af5738 Compare October 16, 2018 20:22
@bart0sh
Copy link
Contributor Author

bart0sh commented Oct 16, 2018

tests are failing with this error:
W1016 18:55:01.139] GoCompile: missing strict dependencies:
W1016 18:55:01.139] /bazel-scratch/.cache/bazel/_bazel_prow/48d5366022b4e3197674c8d6e2bee219/sandbox/linux-sandbox/3083/execroot/main/cmd/kubeadm/app/phases/upgrade/staticpods.go: import of "k8s.io/kubernetes/cmd/kubeadm/app/util/staticpod"

I can't reproduce this locally. BUILD file is updated, so dependencies should be correct. 'make bazel-build' and 'make bazel-release' work just fine in my setup.

Can anybody help?

@bart0sh
Copy link
Contributor Author

bart0sh commented Oct 16, 2018

/test pull-kubernetes-e2e-gce-100-performance
/test pull-kubernetes-e2e-kops-aws

xiangpengzhao
xiangpengzhao reviewed Oct 17, 2018
View reviewed changes
Copy link
Contributor

@xiangpengzhao xiangpengzhao left a comment

Choose a reason for hiding this comment

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

just one nit. otherwise LGTM

cmd/kubeadm/app/util/staticpod/utils.go Outdated
@@ -288,3 +289,17 @@ func GetProbeAddress(cfg *kubeadmapi.InitConfiguration, componentName string) st
}
return "127.0.0.1"
}

// ManifestFilesAreEqual compares 2 files. Returns true if their contents are equal, false otherwise
Copy link
Contributor

Choose a reason for hiding this comment

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

s/Returns/Return ?

@bart0sh bart0sh force-pushed the PR0030-kubeadm-fix-1054-upgrade-same-version branch from 8af5738 to 6c1c42c Compare October 17, 2018 08:06
@bart0sh
kubeadm: skip upgrade if manifest is not changed
ad01798 
When doing upgrades kubeadm generates new manifest and
waits until kubelet restarts correspondent pod.

However, kubelet won't restart pod if there are no changes
in the manifest. That makes kubeadm stuck waiting for
restarted pod.

Skipping upgrade if new component manifest is the same as
current manifest should solve this.

Fixes: kubernetes/kubeadm#1054
@bart0sh bart0sh force-pushed the PR0030-kubeadm-fix-1054-upgrade-same-version branch from 6c1c42c to ad01798 Compare October 17, 2018 08:09
@bart0sh
Copy link
Contributor Author

bart0sh commented Oct 17, 2018

@xiangpengzhao Thanks for the review. done.

@bart0sh
Copy link
Contributor Author

bart0sh commented Oct 17, 2018

/retest

@neolit123
Copy link
Member

/lgtm
thanks @bart0sh

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Oct 17, 2018
@bart0sh
Copy link
Contributor Author

bart0sh commented Oct 17, 2018

/assign @timothysc

@bart0sh bart0sh mentioned this pull request Oct 18, 2018
Closed
timothysc
timothysc approved these changes Oct 19, 2018
View reviewed changes
Copy link
Member

@timothysc timothysc left a comment

Choose a reason for hiding this comment

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

/approve

thx @bart0sh !

@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: bart0sh, timothysc

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

@k8s-ci-robot k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Oct 19, 2018
@k8s-ci-robot k8s-ci-robot merged commit b47510c into kubernetes:master Oct 19, 2018
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

@xiangpengzhao xiangpengzhao xiangpengzhao left review comments

@timothysc timothysc timothysc approved these changes

Assignees

@timothysc timothysc

@neolit123 neolit123

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. kind/cleanup Categorizes issue or PR as related to cleaning up code, process, or technical debt. lgtm "Looks good to me", indicates that a PR is ready to be merged. 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. sig/testing Categorizes an issue or PR as relevant to SIG Testing. size/M Denotes a PR that changes 30-99 lines, ignoring generated files.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

kubeadm upgrade apply failure with same kubernetes version
5 participants
@bart0sh @neolit123 @k8s-ci-robot @timothysc @xiangpengzhao