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

Deleting a deployment doesn't delete the pods #44046

Closed
vascofg opened this issue Apr 4, 2017 · 20 comments · Fixed by #44058
Closed

Deleting a deployment doesn't delete the pods #44046

vascofg opened this issue Apr 4, 2017 · 20 comments · Fixed by #44058
Assignees
@caesarxuchao
Labels
area/workload-api/deployment kind/bug Categorizes issue or PR as related to a bug. sig/api-machinery Categorizes an issue or PR as relevant to SIG API Machinery. sig/apps Categorizes an issue or PR as relevant to SIG Apps.
Milestone
v1.7

Comments

@vascofg
Copy link

vascofg commented Apr 4, 2017

BUG REPORT:

Kubernetes version: 1.6.1

Environment:

  • Hardware configuration: 2 identical machines, with i7-2600K CPUs and 4GB RAM.
  • OS: CentOS 7
  • Kernel: 3.10.0-514.10.2.el7.x86_64
  • Install tools: KubeADM

What happened:
Deleting a deployment via the API with the DeleteOptions settings orphanDependants: false or propagationPolicy: Background deletes the Deployment and the replica set but NOT the pods.
Deleting it with the DeleteOptions setting propagationPolicy: Foreground works as intended.

What you expected to happen:
The replica set deletion should propagate to the pods. I've read that the garbage collection should work for Deployments in 1.6.

How to reproduce it:

kubectl create -f https://raw.githubusercontent.com/kubernetes/kubernetes.github.io/master/docs/concepts/workloads/controllers/nginx-deployment.yaml
curl -X DELETE localhost:8001/apis/extensions/v1beta1/namespaces/default/deployments/nginx-deployment -d '{"kind":"DeleteOptions","apiVersion":"v1","propagationPolicy":"Background"}' -H "Content-Type: application/json"

Anything else we need to know:
I am using the Python Client and at the moment it only supports the orphanDependants option, since it's still at api version 1.5.
@0xmichalis
Copy link
Contributor

cc: @caesarxuchao @krmayankk @enisoc

@0xmichalis 0xmichalis added area/workload-api/deployment sig/apps Categorizes an issue or PR as relevant to SIG Apps. labels Apr 4, 2017
@caesarxuchao
Copy link
Member

We discussed the problem in kubernetes/kubeadm#149 (comment).

I'll change the GC to make sure it's cascading.

@caesarxuchao caesarxuchao mentioned this issue Apr 4, 2017
Merged
@caesarxuchao
Copy link
Member

Sent #44058

@vascofg
Copy link
Author

vascofg commented Apr 18, 2017

Any progress on this?

@caesarxuchao
Copy link
Member

Still trying to figure out a sane fix: #44058 (comment)

@vladimir-kozyrev
Copy link

I've encountered the same issue. My pods are still running even though I deleted the deployment.

@0xmichalis
Copy link
Contributor

@caesarxuchao lets come up with a fix for 1.7. This should work from the first time we introduced cascading deletion in Deployments.

@0xmichalis 0xmichalis added this to the v1.7 milestone Apr 25, 2017
@caesarxuchao
Copy link
Member

@Kargakis Yes, we should. Do you have any comments on #44058 (comment)?

@0xmichalis
Copy link
Contributor

@caesarxuchao replied. Let's move this forward. If the fix is small, I am going to backport it to 1.6.

@krmayankk krmayankk mentioned this issue May 13, 2017
Merged
@0xmichalis 0xmichalis self-assigned this May 21, 2017
@vascofg vascofg mentioned this issue Jun 13, 2017
Closed
@dchen1107
Copy link
Member

Offline discussion with @janetkuo, we punt this to v1.8.

@dchen1107 dchen1107 modified the milestones: v1.8, v1.7 Jun 14, 2017
@caesarxuchao
Copy link
Member

Discussed with @dchen1107, if we can get the PR #45764 fixed soon, it can still go in 1.7. It's a real bug fix.

@0xmichalis
Copy link
Contributor

Yes, we need to fix this for 1.7 and also backport to 1.6.

@0xmichalis 0xmichalis modified the milestones: v1.7, v1.8 Jun 14, 2017
@0xmichalis 0xmichalis added the kind/bug Categorizes issue or PR as related to a bug. label Jun 14, 2017
k8s-github-robot pushed a commit that referenced this issue Jun 15, 2017
Merge pull request #45764 from krmayankk/depl-del-rs
289de0e 
Automatic merge from submit-queue (batch tested with PRs 47492, 47542, 46800, 47545, 45764)

delete dependent pods for rs when deleting deployments

Fix #44046, where user reported that the garbage collector didn't delete pods when a deployment was deleted with PropagationPolicy=Background.
@liggitt liggitt reopened this Jun 23, 2017
@marun
Copy link
Contributor

marun commented Jun 23, 2017

@liggitt Should this be targeting 1.7?

@liggitt
Copy link
Member

liggitt commented Jun 23, 2017 •
edited
Loading

yes, the fix that went in in #45764 is going to cause us data issues we don't want

@marun
Copy link
Contributor

marun commented Jun 23, 2017

@Kargakis Are you working on this? What's the timeline to resolution?

@dchen1107
Copy link
Member

Based on @liggitt above comment, looks like #45764 actually introduced a new regression. Am I right?

@caesarxuchao
Copy link
Member

In #44058, I reverted what's introduced in #45764.

@janetkuo janetkuo assigned caesarxuchao and unassigned 0xmichalis Jun 24, 2017
@janetkuo
Copy link
Member

janetkuo commented Jun 24, 2017 •
edited
Loading

@marun - @caesarxuchao is working on it #44058

@dchen1107
Copy link
Member

dchen1107 commented Jun 24, 2017 •
edited
Loading

I approved this for 1.7 based on @liggitt's comment at #44046 (comment). We should revert #45764, but are we ok with @caesarxuchao's #44058 since it also introducing a new behavior. I will leave @kubernetes/sig-apps-bugs to decide.

@liggitt liggitt added the sig/api-machinery Categorizes an issue or PR as relevant to SIG API Machinery. label Jun 24, 2017
@liggitt
Copy link
Member

liggitt commented Jun 24, 2017

@dchen1107:
I should revert #45764, but are we ok with @caesarxuchao's #44058 since it also introducing a new behavior.

#44058 replaces #45764 with a fix that addresses this issue systemically. While this bug did exist in 1.6.x, more things rely on GC in 1.7 than in 1.6, and without that fix, GC leaves more resources vulnerable to unintentional orphaning.

I will leave @kubernetes/sig-apps-bugs to decide.

Since the issue is actually with general GC behavior, @kubernetes/sig-api-machinery-bugs should weigh in.

k8s-github-robot pushed a commit that referenced this issue Jun 26, 2017
Merge pull request #44058 from caesarxuchao/background-cascading
6a28658 
Automatic merge from submit-queue (batch tested with PRs 44058, 48085, 48077, 48076, 47823)

Make background garbage collection cascading

Fix #44046, fix #47843 where user reported that the garbage collector didn't delete pods when a deployment was deleted with PropagationPolicy=Background.

The cause is that when propagating background garbage collection request, the garbage collector deletes dependents with DeleteOptions.PropagationPolicy=nil, which means the default GC policy of a resource (defined by its REST strategy) and the existing GC-related finalizers will decide how the delete request is propagated further. Unfortunately, the default GC policy for RS is orphaning, so the pods are behind when a deployment is deleted.

This PR changes the garbage collector to delete dependents with DeleteOptions.PropagationPolicy=Background when the owner is deleted in background. This means the dependent's existing GC finalizers will be overridden, making orphaning less flexible (see this made-up [case](kubernetes/kubeadm#149 (comment))). I think sacrificing the flexibility of orphaning is worthwhile, because making the behavior of background garbage collection matching users' expectation is more important.

cc @lavalamp @Kargakis @krmayankk @enisoc 

```release-note
The garbage collector now cascades deletion properly when deleting an object with propagationPolicy="background". This resolves issue [#44046](#44046), so that when a deployment is deleted with propagationPolicy="background", the garbage collector ensures dependent pods are deleted as well.
```
anguslees added a commit to anguslees/kubecfg-1 that referenced this issue Jul 10, 2017
@anguslees
Set delete propagation policy (to "foreground")
a78be14 
Kubernetes 1.6 adds a `PpropagationPolicy` to the delete options,
obsoleting the previous `OrphanDependents` boolean.

This change sets `PropagationPolicy=foreground`, keeping the legacy
`OrphanDependents=false` for older k8s servers.

Note that PropagationPolicy=background is broken in k8s 1.6 and (eg)
leaks pods from a deployment.  See kubernetes/kubernetes#44046 for
upstream discussion.
@anguslees anguslees mentioned this issue Jul 10, 2017
Merged
@ngtuna ngtuna mentioned this issue Sep 29, 2017
Merged
@stephen-soltesz stephen-soltesz mentioned this issue Nov 16, 2017
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@caesarxuchao caesarxuchao

Labels
area/workload-api/deployment kind/bug Categorizes issue or PR as related to a bug. sig/api-machinery Categorizes an issue or PR as relevant to SIG API Machinery. sig/apps Categorizes an issue or PR as relevant to SIG Apps.
Projects
None yet
Milestone
v1.7
Development

Successfully merging a pull request may close this issue.

Make background garbage collection cascading caesarxuchao/kubernetes
8 participants
@marun @vascofg @liggitt @caesarxuchao @0xmichalis @janetkuo @vladimir-kozyrev @dchen1107