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

Wait for container cleanup before deletion #50350

Merged
merged 1 commit into from Sep 6, 2017
Merged

Wait for container cleanup before deletion #50350

merged 1 commit into from Sep 6, 2017

Conversation

dashpole
Copy link
Contributor

@dashpole dashpole commented Aug 9, 2017

We should wait to delete pod API objects until the pod's containers have been cleaned up. See issue: #50268 for background.

This changes the kubelet container gc, which deletes containers belonging to pods considered "deleted".
It adds two conditions under which a pod is considered "deleted", allowing containers to be deleted:
Pods where deletionTimestamp is set, and containers are not running
Pods that are evicted

This PR also changes the function PodResourcesAreReclaimed by making it return false if containers still exist.
The eviction manager will wait for containers of previous evicted pod to be deleted before evicting another pod.
The status manager will wait for containers to be deleted before removing the pod API object.

/assign @vishh

@k8s-ci-robot k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Aug 9, 2017
@dashpole
Copy link
Contributor Author

dashpole commented Aug 9, 2017

/release-note-none

@k8s-ci-robot k8s-ci-robot added the release-note-none Denotes a PR that doesn't merit a release note. label Aug 9, 2017
@k8s-github-robot k8s-github-robot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Aug 9, 2017
@dashpole
Copy link
Contributor Author

cc @kubernetes/sig-node-pr-reviews

@k8s-ci-robot k8s-ci-robot added the sig/node Categorizes an issue or PR as relevant to SIG Node. label Aug 15, 2017
@dashpole
Copy link
Contributor Author

/assign @dchen1107

resouer
resouer reviewed Aug 25, 2017
View reviewed changes
pkg/kubelet/kubelet_pods.go Outdated
@@ -741,6 +742,22 @@ func (kl *Kubelet) podIsTerminated(pod *v1.Pod) bool {
return status.Phase == v1.PodFailed || status.Phase == v1.PodSucceeded || (pod.DeletionTimestamp != nil && notRunning(status.ContainerStatuses))
}

// podIsDeleted returns true if the pod is deleted. For the pod to be deleted, either:
Copy link
Contributor

Choose a reason for hiding this comment

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

nits: IsPodDeleted ...

@dashpole
Copy link
Contributor Author

dashpole commented Sep 1, 2017

/retest

@dchen1107
Copy link
Member

/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Sep 1, 2017
@k8s-github-robot k8s-github-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Sep 1, 2017
@vishh
Copy link
Contributor

vishh commented Sep 1, 2017

/lgtm
/approve

@dashpole
Copy link
Contributor Author

dashpole commented Sep 1, 2017

/retest

@k8s-github-robot k8s-github-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Sep 1, 2017
@k8s-github-robot k8s-github-robot removed lgtm "Looks good to me", indicates that a PR is ready to be merged. needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. labels Sep 2, 2017
@dashpole
Copy link
Contributor Author

dashpole commented Sep 2, 2017

/test pull-kubernetes-e2e-kops-aws

@dashpole
Copy link
Contributor Author

dashpole commented Sep 2, 2017

lgtm removed because of rebase. This should be in the 1.8 milestone

@calebamiles calebamiles added this to the v1.8 milestone Sep 2, 2017
@calebamiles calebamiles added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Sep 2, 2017
@calebamiles
Copy link
Contributor

  • added to the 1.8 milestone.
  • manually applied lgtm after removal for rebase

cc: @kubernetes/kubernetes-release-managers

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

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.

@k8s-github-robot k8s-github-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Sep 2, 2017
@dashpole
Copy link
Contributor Author

dashpole commented Sep 5, 2017

rebased

@k8s-github-robot k8s-github-robot removed lgtm "Looks good to me", indicates that a PR is ready to be merged. needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. labels Sep 5, 2017
@feiskyer
Copy link
Member

feiskyer commented Sep 5, 2017

/retest

1 similar comment
@dims
Copy link
Member

dims commented Sep 5, 2017

/retest

@dims
Copy link
Member

dims commented Sep 5, 2017

re-applying lgtm after rebase, originally lgtm was from @dchen1107

@dchen1107
Copy link
Member

/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Sep 5, 2017
@k8s-github-robot
Copy link

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: dashpole, dchen1107, vishh

Associated issue: 50268

The full list of commands accepted by this bot can be found here.

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

You can indicate your approval by writing /approve in a comment
You can cancel your approval by writing /approve cancel in a 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.

@k8s-github-robot
Copy link

Automatic merge from submit-queue (batch tested with PRs 51186, 50350, 51751, 51645, 51837)

@k8s-github-robot k8s-github-robot merged commit 78c8208 into kubernetes:master Sep 6, 2017
@k8s-ci-robot
Copy link
Contributor

@dashpole: The following test failed, say /retest to rerun them all:

Test name Commit Details Rerun command
pull-kubernetes-e2e-kops-aws 9ac30e2 link /test pull-kubernetes-e2e-kops-aws

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.

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 Sep 6, 2017
Closed
@shyamjvs shyamjvs mentioned this pull request Sep 27, 2017
Closed
@dashpole dashpole deleted the eviction_container_deletion branch September 27, 2017 21:28
@dashpole dashpole mentioned this pull request Sep 28, 2017
Closed
dashpole added a commit to dashpole/kubernetes that referenced this pull request Sep 28, 2017
@dashpole
add feature gate to critical portion of kubernetes#50350
ac80d4d
@shyamjvs shyamjvs mentioned this pull request Oct 10, 2017
Closed
@dashpole dashpole mentioned this pull request Oct 12, 2017
Merged
@joelsmith joelsmith mentioned this pull request Oct 13, 2017
Merged
openshift-merge-robot added a commit to openshift/origin that referenced this pull request Oct 17, 2017
@openshift-merge-robot
Merge pull request #16865 from joelsmith/node-events
84e0585 
Automatic merge from submit-queue (batch tested with PRs 16889, 16865).

UPSTREAM: 53857: kubelet sync pod throws more detailed events

Also includes the following upstream dependant PRs:

UPSTREAM: 50350: Wait for container cleanup before deletion
UPSTREAM: 48970: Recreate pod sandbox when the sandbox does not have an IP address.
UPSTREAM: 48589: When faild create pod sandbox record event.
UPSTREAM: 48584: Move event type
UPSTREAM: 47599: Rerun init containers when the pod needs to be restarted

xrefs:
kubernetes/kubernetes#53857
kubernetes/kubernetes#50350
kubernetes/kubernetes#48970
kubernetes/kubernetes#48589
kubernetes/kubernetes#48584
kubernetes/kubernetes#47599
@liggitt liggitt mentioned this pull request Oct 17, 2017
Merged
@ddongchen ddongchen mentioned this pull request Mar 1, 2019
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@resouer resouer resouer left review comments

Assignees

@vishh vishh

@dchen1107 dchen1107

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-none Denotes a PR that doesn't merit a release note. sig/node Categorizes an issue or PR as relevant to SIG Node. size/L Denotes a PR that changes 100-499 lines, ignoring generated files.
Projects
None yet
Milestone
v1.8
Development

Successfully merging this pull request may close these issues.

None yet
10 participants
@dashpole @dchen1107 @vishh @calebamiles @fejta-bot @feiskyer @dims @k8s-github-robot @k8s-ci-robot @resouer