kubelet: storage: teardown terminated pod volumes#37228
Conversation
d64543e to
76be092
Compare
76be092 to
48a5d14
Compare
|
Jenkins unit/integration failed for commit 48a5d14ae7c78abeed116bd62ad7636d41584f20. Full PR test history. The magic incantation to run this job again is |
|
Jenkins verification failed for commit 48a5d14ae7c78abeed116bd62ad7636d41584f20. Full PR test history. The magic incantation to run this job again is |
|
In the long run I could imagine tagging specific containers and pods as having a "retainPolicy" that allows debugging of other aspects, not just volumes, post pod termination. Things like logs, network artifacts, etc. But that doesn't block or inhibit this work. |
48a5d14 to
22500b6
Compare
|
Jenkins Bazel Build failed for commit 22500b6cf904c0b7e7fd2180b0b8a85aaf508a4e. Full PR test history. The magic incantation to run this job again is |
|
Jenkins kops AWS e2e failed for commit 22500b6cf904c0b7e7fd2180b0b8a85aaf508a4e. Full PR test history. The magic incantation to run this job again is |
|
In the long run, i want to be able to schedule on local disk, so keeping disk cleanup with pod lifetime is important. I guess we could do something like retainPolicy, but it would need to be visible to scheduler. |
|
@k8s-bot bazel test this, issue kubernetes/test-infra#1152 |
22500b6 to
0248edf
Compare
|
@saad-ali @kubernetes/sig-storage I would like to get this in early in the 1.6 dev cycle so it has time to soak. Reviews appreciated! |
|
cc @dashpole who is working on cleaning up all volumes as part of pod termination. |
Ack, will take a look as soon as 1.5 release is out. |
|
for example, it seems like we will need to have a flag like this to preserve legacy behavior no matter any future solution |
60ddd40 to
ffdf5de
Compare
|
@saad-ali I think I addressed your feedback in the latest commits. Can you look again? @jingxu97 This is the first I'm learning of a controller the cares about volumes. I see from #20262 this was created to mount cloud volumes (GPD, EBS, Cinder, etc) to the nodes. Looks like a rough copy of the kubelet volume manager. Not sure how the states of the world sync between the two. Does anything need to be done wrt the attachdeatch controller? |
|
/lgtm |
|
Feel free to readd LGTM label after rebase. And please add a release note. |
ffdf5de to
e2750a3
Compare
|
can I re-add an lgtm on my own PR? /lgtm |
|
@sjenning: you can't LGTM your own PR. DetailsIn response to this comment:
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. |
|
/lgtm
…On Fri, Jan 20, 2017 at 11:11 AM, k8s-ci-robot ***@***.***> wrote:
@sjenning <https://github.com/sjenning>: you can't LGTM your own PR.
In response to this comment
<#37228 (comment)>
:
can I re-add an lgtm on my own PR?
/lgtm
Instructions for interacting with me using PR comments are available here
<https://github.com/kubernetes/community/blob/master/contributors/devel/pull-request-commands.md>.
If you have questions or suggestions related to my behavior, please file an
issue against the kubernetes/test-infra
<https://github.com/kubernetes/test-infra/issues/new?title=Prow%20issue:>
repository. I understand the commands that are listed here
<https://github.com/kubernetes/test-infra/blob/master/prow/commands.md>.
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#37228 (comment)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AGvIKI1kkPryIObeg0H2Lyuy_lc7Nv5mks5rUQbFgaJpZM4K4imT>
.
|
|
@vishh: you can't LGTM a PR unless you are an assignee. DetailsIn response to this comment:
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. |
|
/lgtm |
|
Automatic merge from submit-queue (batch tested with PRs 37228, 40146, 40075, 38789, 40189) |
Automatic merge from submit-queue Delay deletion of pod from the API server until volumes are deleted Depends on #37228, and will not pass tests until that PR is merged, and this is rebased. Keeps all kubelet behavior the same, except the kubelet will not make the "Delete" call (kubeClient.Core().Pods(pod.Namespace).Delete(pod.Name, deleteOptions)) until the volumes associated with that pod are removed. I will perform some performance testing so that we better understand the latency impact of this change. Is kubelet_pods.go the correct file to include the "when can I delete this pod" logic? cc: @vishh @sjenning @derekwaynecarr
Automatic merge from submit-queue (batch tested with PRs 49284, 49555, 47639, 49526, 49724) skip WaitForAttachAndMount for terminated pods in syncPod Fixes #49663 I tried to tread lightly with a small localized change because this needs to be picked to 1.7 and 1.6 as well. I suspect this has been as issue since we started unmounting volumes on pod termination #37228 xref openshift/origin#14383 @derekwaynecarr @eparis @smarterclayton @saad-ali @jwforres /release-note-none
This is a continuation of the work done in #36779
There really is no reason to keep volumes for terminated pods attached on the node. This PR extends the removal of volumes on the node from memory-backed (the current policy) to all volumes.
@pmorie raised a concern an impact debugging volume related issues if terminated pod volumes are removed. To address this issue, the PR adds a
--keep-terminated-pod-volumesflag the kubelet and sets it forhack/local-up-cluster.sh.For consideration in 1.6.
Fixes #35406
@derekwaynecarr @vishh @dashpole
This change is