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.

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

kubeadm: skip upgrade if manifest is not changed #69886

Merged
merged 1 commit into from Oct 19, 2018

Conversation

@bart0sh
Contributor

bart0sh commented Oct 16, 2018

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
@neolit123

@bart0sh thanks! added a couple of comments.

return false, err
}
return string(content1) == string(content2), nil

This comment has been minimized.

@neolit123
@@ -162,6 +163,19 @@ func (spm *KubeStaticPodPathManager) CleanupDirs() error {
return nil
}
func filesEqual(path1, path2 string) (bool, error) {

This comment has been minimized.

@neolit123

neolit123 Oct 16, 2018

Member

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?

return err
}
if equal {
fmt.Printf("[upgrade/staticpods] current and new manifests of %s are equal, skipping upgrade", component)

This comment has been minimized.

@neolit123

neolit123 Oct 16, 2018

Member

i think it needs a \n at the end.

@neolit123

This comment has been minimized.

Member

neolit123 commented Oct 16, 2018

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

@neolit123

This comment has been minimized.

Member

neolit123 commented Oct 16, 2018

/kind bug

@k8s-ci-robot k8s-ci-robot added kind/bug size/M and removed size/S labels Oct 16, 2018

@bart0sh

This comment has been minimized.

Contributor

bart0sh commented Oct 16, 2018

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

@bart0sh

This comment has been minimized.

Contributor

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

This comment has been minimized.

Contributor

bart0sh commented Oct 16, 2018

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

@xiangpengzhao

just one nit. otherwise LGTM

@@ -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

This comment has been minimized.

@xiangpengzhao

xiangpengzhao Oct 17, 2018

Member

s/Returns/Return ?

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

This comment has been minimized.

Contributor

bart0sh commented Oct 17, 2018

@xiangpengzhao Thanks for the review. done.

@bart0sh

This comment has been minimized.

Contributor

bart0sh commented Oct 17, 2018

/retest

@neolit123

This comment has been minimized.

Member

neolit123 commented Oct 17, 2018

/lgtm
thanks @bart0sh

@bart0sh

This comment has been minimized.

Contributor

bart0sh commented Oct 17, 2018

/assign @timothysc

@timothysc

/approve

thx @bart0sh !

@k8s-ci-robot

This comment has been minimized.

Contributor

k8s-ci-robot commented Oct 19, 2018

[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 merged commit b47510c into kubernetes:master Oct 19, 2018

18 checks passed

cla/linuxfoundation bart0sh authorized
Details
pull-kubernetes-bazel-build Job succeeded.
Details
pull-kubernetes-bazel-test Job succeeded.
Details
pull-kubernetes-cross Skipped
pull-kubernetes-e2e-gce Job succeeded.
Details
pull-kubernetes-e2e-gce-100-performance Job succeeded.
Details
pull-kubernetes-e2e-gce-device-plugin-gpu Job succeeded.
Details
pull-kubernetes-e2e-gke Skipped
pull-kubernetes-e2e-kops-aws Job succeeded.
Details
pull-kubernetes-e2e-kubeadm-gce Skipped
pull-kubernetes-integration Job succeeded.
Details
pull-kubernetes-kubemark-e2e-gce-big Job succeeded.
Details
pull-kubernetes-local-e2e Skipped
pull-kubernetes-local-e2e-containerized Skipped
pull-kubernetes-node-e2e Job succeeded.
Details
pull-kubernetes-typecheck Job succeeded.
Details
pull-kubernetes-verify Job succeeded.
Details
tide In merge pool.
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment