Skip to content

Delay Deletion of a Pod until volumes are cleaned up #41456

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
Merged
k8s-github-robot merged 1 commit into from
Feb 16, 2017
Merged

Delay Deletion of a Pod until volumes are cleaned up #41456

k8s-github-robot merged 1 commit into from
Feb 16, 2017

Conversation

@dashpole
Copy link
Contributor

@dashpole dashpole commented Feb 15, 2017

#41436 fixed the bug that caused #41095 and #40239 to have to be reverted. Now that the bug is fixed, this shouldn't cause problems.

@vishh @derekwaynecarr @sjenning @jingxu97 @kubernetes/sig-storage-misc

@k8s-ci-robot k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Feb 15, 2017
@k8s-reviewable
Copy link

k8s-reviewable commented Feb 15, 2017

This change is Reviewable

@k8s-github-robot
Copy link

k8s-github-robot commented Feb 15, 2017

[APPROVALNOTIFIER] This PR is NOT APPROVED

The following people have approved this PR: dashpole

Needs approval from an approver in each of these OWNERS Files:

We suggest the following people:
cc @dchen1107
You can indicate your approval by writing /approve in a comment
You can cancel your approval by writing /approve cancel in a comment

@k8s-github-robot k8s-github-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. release-note-label-needed labels Feb 15, 2017
@dashpole
Revert "Merge pull request kubernetes#41202 from dashpole/revert-4109…
1d38818 
…5-deletion_pod_lifecycle"

This reverts commit ff87d13, reversing
changes made to 46becf2.
@jingxu97
Copy link
Contributor

jingxu97 commented Feb 15, 2017

@dashpole Could you put some details about this issue you discovered in the previous PR so that we could learn something from it? Thanks!

@dashpole
Copy link
Contributor Author

dashpole commented Feb 15, 2017

@jingxu97 The issue from the previous PR, as shown here, was that some updates to the API server were breaking the invariant that pod status always progresses. For example, Status can go PENDING -> RUNNING -> FAILED, but should never go PENDING -> RUNNING -> FAILED -> RUNNING, which was what was happening in the previous PR. By adding debugging statements, and reproducing the bug, I was able to see that calls to statusManager.SetPodStatus, was correctly updating the status to FAILED, and that a call to statusManager.TerminatePod was resetting the status to RUNNING. This led to the discovery of #41436 in TerminatePod, which I believe has solved the problem.

vishh
vishh reviewed Feb 15, 2017
View reviewed changes
pkg/kubelet/status/status_manager.go
if !ok {
return false
}
return !kubepod.IsMirrorPod(pod) && m.podDeletionSafety.OkToDeletePod(pod)
Copy link
Contributor

@vishh vishh Feb 15, 2017

Choose a reason for hiding this comment

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

#41507 might be relevant here.

@vishh
Copy link
Contributor

vishh commented Feb 15, 2017

Here we go again :)

@vishh
Copy link
Contributor

vishh commented Feb 15, 2017

/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Feb 15, 2017
@vishh vishh added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Feb 15, 2017
@k8s-github-robot k8s-github-robot added the do-not-merge DEPRECATED. Indicates that a PR should not merge. Label can only be manually applied/removed. label Feb 15, 2017
@vishh vishh added release-note Denotes a PR that will be considered when it comes time to generate release notes. and removed release-note-label-needed do-not-merge DEPRECATED. Indicates that a PR should not merge. Label can only be manually applied/removed. labels Feb 16, 2017
@k8s-github-robot
Copy link

k8s-github-robot commented Feb 16, 2017

Automatic merge from submit-queue (batch tested with PRs 41466, 41456, 41550, 41238, 41416)

@k8s-github-robot k8s-github-robot merged commit 3c606cd into kubernetes:master Feb 16, 2017
@dashpole dashpole deleted the pod_volume_cleanup branch February 16, 2017 18:34
@k8s-ci-robot
Copy link
Contributor

k8s-ci-robot commented Feb 16, 2017

@dashpole: The following test(s) failed:

Test name Commit Details Rerun command
Jenkins CRI GCE e2e 1d38818 link @k8s-bot cri e2e test this

Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR.

Details

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. I understand the commands that are listed here.

@dashpole dashpole mentioned this pull request Mar 8, 2017
Merged
@dchen1107 dchen1107 mentioned this pull request Mar 9, 2017
Closed
jwhonce pushed a commit to jwhonce/kubernetes that referenced this pull request Mar 9, 2017
Merge pull request kubernetes#42734 from dashpole/deletion_timeout
7c08e81 
Automatic merge from submit-queue (batch tested with PRs 42734, 42745, 42758, 42814, 42694)

Create DefaultPodDeletionTimeout for e2e tests

In our e2e and e2e_node tests, we had a number of different timeouts for deletion.
Recent changes to the way deletion works (kubernetes#41644, kubernetes#41456) have resulted in some timeouts in e2e tests.  kubernetes#42661 was the most recent fix for this.
Most of these tests are not meant to test pod deletion latency, but rather just to clean up pods after a test is finished.
For this reason, we should change all these tests to use a standard, fairly high timeout for deletion.

cc @vishh @Random-Liu
@dashpole dashpole mentioned this pull request Mar 15, 2017
Closed
@msau42 msau42 mentioned this pull request Oct 15, 2018
Closed
@cofyc cofyc mentioned this pull request Feb 26, 2019
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

1 more reviewer

@vishh vishh vishh approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

@vishh vishh

Labels

approved Indicates a PR has been approved by an approver from all required OWNERS files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. 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. size/L Denotes a PR that changes 100-499 lines, ignoring generated files.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

6 participants

@dashpole @k8s-reviewable @k8s-github-robot @jingxu97 @vishh @k8s-ci-robot