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.

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

fix daemon set rolling update hang #77773

Merged
merged 1 commit into from May 20, 2019

Conversation

@DaiHao
Copy link
Member

commented May 11, 2019

What type of PR is this?

Uncomment only one /kind <> line, hit enter to put that in a new line, and remove leading whitespaces from that line:

/kind bug

What this PR does / why we need it:

daemon set rolling update hang when there exsits not-ready node in cluster

Which issue(s) this PR fixes:

Fixes #63465

Special notes for your reviewer:
When daemon controller delete pod on not-ready node, pod will stuck in terminating state.
Then informer will not receives pod delete event, which cause expectation never be satisfied and daemon controller not exec manage method.

Does this PR introduce a user-facing change?:

Fix a bug that causes DaemonSet rolling update to hang when its pod gets stuck at terminating. 
@k8s-ci-robot

This comment has been minimized.

Copy link
Contributor

commented May 11, 2019

Hi @DaiHao. Thanks for your PR.

I'm waiting for a kubernetes member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

@DaiHao

This comment has been minimized.

Copy link
Member Author

commented May 11, 2019

/sig apps
/kind bug
/priority important-soon

@k8s-ci-robot k8s-ci-robot added kind/bug and removed needs-kind labels May 11, 2019

@DaiHao

This comment has been minimized.

Copy link
Member Author

commented May 11, 2019

@draveness

This comment has been minimized.

Copy link
Member

commented May 12, 2019

/ok-to-test

@draveness

This comment has been minimized.

Copy link
Member

commented May 12, 2019

/assign @soltysh

@DaiHao

This comment has been minimized.

Copy link
Member Author

commented May 12, 2019

/test pull-kubernetes-kubemark-e2e-gce-big

dsc.deletePod(curPod)
return
}

This comment has been minimized.

Copy link
@krmayankk

krmayankk May 13, 2019

Contributor

i am confused how this fixes the issue ? Did you try with this fix and it solves the issue ?

This comment has been minimized.

Copy link
@DaiHao

DaiHao May 13, 2019

Author Member

The reason why ds rolling update stucked is expectations not be satisfied. In this fix, we lower deletion expectation once pod's DeletionTimestamp is not nil to satisfy expectation and let syncloop resume.

This comment has been minimized.

Copy link
@DaiHao

DaiHao May 13, 2019

Author Member

Before this fix, if a pod stucked in terminating, deamonset will never satisfy expectation.

This comment has been minimized.

Copy link
@krmayankk

krmayankk May 17, 2019

Contributor

shouldnt lowering of expectation be done using dsc.expectations.DeletionObserved(dsKey) ?

This comment has been minimized.

Copy link
@krmayankk

krmayankk May 17, 2019

Contributor

@janetkuo could you help understand this fix ? Do we need a test ?

This comment has been minimized.

Copy link
@DaiHao

DaiHao May 18, 2019

Author Member

shouldnt lowering of expectation be done using dsc.expectations.DeletionObserved(dsKey) ?

of course you could just use dsc.expectations.DeletionObserved(dsKey)
but in this fix we could reuse the code in "deletePod" and keep code consistent with replicas-controller and job-controller. see #77773 (comment)

@DaiHao

This comment has been minimized.

Copy link
Member Author

commented May 15, 2019

pkg/controller/daemon/daemon_controller.go Outdated Show resolved Hide resolved
@janetkuo

This comment has been minimized.

Copy link
Member

commented May 15, 2019

/approve

@k8s-ci-robot

This comment has been minimized.

Copy link
Contributor

commented May 15, 2019

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: DaiHao, janetkuo

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

@janetkuo

This comment has been minimized.

Copy link
Member

commented May 15, 2019

I just reworded release note.

@janetkuo

This comment has been minimized.

Copy link
Member

commented May 15, 2019

/assign @k82cn

@DaiHao DaiHao force-pushed the DaiHao:daemon branch from 0771120 to e25ff46 May 16, 2019

@answer1991

This comment has been minimized.

Copy link
Contributor

commented May 16, 2019

awesome work!

@zhangxiaoyu-zidif

@zhangxiaoyu-zidif

This comment has been minimized.

Copy link
Member

commented May 16, 2019

ping @k82cn for lgtm

@answer1991

This comment has been minimized.

Copy link
Contributor

commented May 16, 2019

is there same issue in ReplicaSet-Controller?

@DaiHao

This comment has been minimized.

Copy link
Member Author

commented May 16, 2019

is there same issue in ReplicaSet-Controller?

job controller and replicaset controller has already contains this logic.

func (jm *JobController) updatePod(old, cur interface{}) {
curPod := cur.(*v1.Pod)
oldPod := old.(*v1.Pod)
if curPod.ResourceVersion == oldPod.ResourceVersion {
// Periodic resync will send update events for all known pods.
// Two different versions of the same pod will always have different RVs.
return
}
if curPod.DeletionTimestamp != nil {
// when a pod is deleted gracefully it's deletion timestamp is first modified to reflect a grace period,
// and after such time has passed, the kubelet actually deletes it from the store. We receive an update
// for modification of the deletion timestamp and expect an job to create more pods asap, not wait
// until the kubelet actually deletes the pod.
jm.deletePod(curPod)
return
}

func (rsc *ReplicaSetController) updatePod(old, cur interface{}) {
curPod := cur.(*v1.Pod)
oldPod := old.(*v1.Pod)
if curPod.ResourceVersion == oldPod.ResourceVersion {
// Periodic resync will send update events for all known pods.
// Two different versions of the same pod will always have different RVs.
return
}
labelChanged := !reflect.DeepEqual(curPod.Labels, oldPod.Labels)
if curPod.DeletionTimestamp != nil {
// when a pod is deleted gracefully it's deletion timestamp is first modified to reflect a grace period,
// and after such time has passed, the kubelet actually deletes it from the store. We receive an update
// for modification of the deletion timestamp and expect an rs to create more replicas asap, not wait
// until the kubelet actually deletes the pod. This is different from the Phase of a pod changing, because
// an rs never initiates a phase change, and so is never asleep waiting for the same.
rsc.deletePod(curPod)
if labelChanged {
// we don't need to check the oldPod.DeletionTimestamp because DeletionTimestamp cannot be unset.
rsc.deletePod(oldPod)
}
return
}

@k82cn

This comment has been minimized.

Copy link
Member

commented May 16, 2019

I'm going to review this PR today :)

@@ -535,6 +535,15 @@ func (dsc *DaemonSetsController) updatePod(old, cur interface{}) {
return
}

if curPod.DeletionTimestamp != nil {

This comment has been minimized.

Copy link
@answer1991

answer1991 May 18, 2019

Contributor
if oldPod.DeletionTimestamp == nil && curPod.DeletionTimestamp != nil {
...
}

Check oldPod's DeletionTimestamp will be better. Same as ReplicaSet-Controller.

This comment has been minimized.

Copy link
@DaiHao

DaiHao May 18, 2019

Author Member

Yes, it might reduce the nonsense syncloop.
The only risk I have considered is that once handler(updatePod or deletePod) returns before lowering of expectation, we will also lose the chance to satisfy the expectation.
@k82cn please help to review this PR, thanks

This comment has been minimized.

Copy link
@answer1991

answer1991 May 18, 2019

Contributor

If my suggestion is acceptable, please create another PR to fix ReplicaSet-Controller.

@k82cn

This comment has been minimized.

Copy link
Member

commented May 20, 2019

/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm label May 20, 2019

@k8s-ci-robot k8s-ci-robot merged commit 6ba13bf into kubernetes:master May 20, 2019

20 checks passed

cla/linuxfoundation DaiHao authorized
Details
pull-kubernetes-bazel-build Job succeeded.
Details
pull-kubernetes-bazel-test Job succeeded.
Details
pull-kubernetes-conformance-image-test Skipped.
pull-kubernetes-cross Skipped.
pull-kubernetes-dependencies Job succeeded.
Details
pull-kubernetes-e2e-gce Job succeeded.
Details
pull-kubernetes-e2e-gce-100-performance Job succeeded.
Details
pull-kubernetes-e2e-gce-csi-serial Skipped.
pull-kubernetes-e2e-gce-device-plugin-gpu Job succeeded.
Details
pull-kubernetes-e2e-gce-storage-slow Skipped.
pull-kubernetes-godeps Skipped.
pull-kubernetes-integration Job succeeded.
Details
pull-kubernetes-kubemark-e2e-gce-big Job succeeded.
Details
pull-kubernetes-local-e2e Skipped.
pull-kubernetes-node-e2e Job succeeded.
Details
pull-kubernetes-typecheck Job succeeded.
Details
pull-kubernetes-verify Job succeeded.
Details
pull-publishing-bot-validate Skipped.
tide In merge pool.
Details

@DaiHao DaiHao deleted the DaiHao:daemon branch May 21, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.