Finished pods can be drained

[fraenkel]

fixes #26080

---

This change is

Kubectl drain will now drain finished Pods

[k8s-bot]

Can one of the admins verify that this patch is reasonable to test? If so, please reply "ok to test".
(Note: "add to whitelist" is no longer supported. Please update configurations in kubernetes/test-infra/jenkins/job-configs/kubernetes-jenkins-pull instead.)

This message will repeat several times in short succession due to jenkinsci/ghprb-plugin#292. Sorry.

[k8s-bot]

Can one of the admins verify that this patch is reasonable to test? If so, please reply "ok to test".
(Note: "add to whitelist" is no longer supported. Please update configurations in kubernetes/test-infra/jenkins/job-configs/kubernetes-jenkins-pull instead.)

This message will repeat several times in short succession due to jenkinsci/ghprb-plugin#292. Sorry.

[k8s-bot]

Can one of the admins verify that this patch is reasonable to test? If so, please reply "ok to test".
(Note: "add to whitelist" is no longer supported. Please update configurations in kubernetes/test-infra/jenkins/job-configs/kubernetes-jenkins-pull instead.)

This message will repeat several times in short succession due to jenkinsci/ghprb-plugin#292. Sorry.

[k8s-bot]

Can one of the admins verify that this patch is reasonable to test? If so, please reply "ok to test".
(Note: "add to whitelist" is no longer supported. Please update configurations in kubernetes/test-infra/jenkins/job-configs/kubernetes-jenkins-pull instead.)

This message will repeat several times in short succession due to jenkinsci/ghprb-plugin#292. Sorry.

[k8s-bot]

Can one of the admins verify that this patch is reasonable to test? If so, please reply "ok to test".
(Note: "add to whitelist" is no longer supported. Please update configurations in kubernetes/test-infra/jenkins/job-configs/kubernetes-jenkins-pull instead.)

This message will repeat several times in short succession due to jenkinsci/ghprb-plugin#292. Sorry.

[k8s-bot]

Can one of the admins verify that this patch is reasonable to test? If so, please reply "ok to test".
(Note: "add to whitelist" is no longer supported. Please update configurations in kubernetes/test-infra/jenkins/job-configs/kubernetes-jenkins-pull instead.)

This message will repeat several times in short succession due to jenkinsci/ghprb-plugin#292. Sorry.

[k8s-bot]

Can one of the admins verify that this patch is reasonable to test? If so, please reply "ok to test".
(Note: "add to whitelist" is no longer supported. Please update configurations in kubernetes/test-infra/jenkins/job-configs/kubernetes-jenkins-pull instead.)

This message will repeat several times in short succession due to jenkinsci/ghprb-plugin#292. Sorry.

[k8s-bot]

Can one of the admins verify that this patch is reasonable to test? If so, please reply "ok to test".
(Note: "add to whitelist" is no longer supported. Please update configurations in kubernetes/test-infra/jenkins/job-configs/kubernetes-jenkins-pull instead.)

This message will repeat several times in short succession due to jenkinsci/ghprb-plugin#292. Sorry.

[k8s-bot]

Can one of the admins verify that this patch is reasonable to test? If so, please reply "ok to test".
(Note: "add to whitelist" is no longer supported. Please update configurations in kubernetes/test-infra/jenkins/job-configs/kubernetes-jenkins-pull instead.)

This message will repeat several times in short succession due to jenkinsci/ghprb-plugin#292. Sorry.

[adohe-zz]

Generally lgtm.

[adohe-zz]

pkg/kubectl/cmd/drain.go, line 350 [r1] (raw file):

      if pod.Status.Phase == api.PodSucceeded || pod.Status.Phase == api.PodFailed {
          continue
      }

it would be better to draw some comments here.

---

Comments from Reviewable

[fraenkel]

Review status: 0 of 1 files reviewed at latest revision, 1 unresolved discussion.

---

pkg/kubectl/cmd/drain.go, line 350 [r1] (raw file):

 
podOk := true
for _, filt := range []podFilter{mirrorPodFilter, o.localStorageFilter, o.unreplicatedFilter, o.daemonsetFilter} {
filterOk, w, f := filt(pod)
So really one question would be whether this check should be global to all filters or targetted to the unreplicationFilter where there is no Replication Controller found.
But I will add a comment once I know how targeted we would like this check.
Part of me was wondering what would happen if a deamonset were restarting and I caught it in the middle where it is Failed, unlikely but possible.

---

Comments from Reviewable

[adohe-zz]

pkg/kubectl/cmd/drain.go, line 350 [r1] (raw file):

Previously, fraenkel (Michael Fraenkel) wrote…

 
podOk := true
for _, filt := range []podFilter{mirrorPodFilter, o.localStorageFilter, o.unreplicatedFilter, o.daemonsetFilter} {
filterOk, w, f := filt(pod)
So really one question would be whether this check should be global to all filters or targetted to the unreplicationFilter where there is no Replication Controller found.
But I will add a comment once I know how targeted we would like this check.
Part of me was wondering what would happen if a deamonset were restarting and I caught it in the middle where it is Failed, unlikely but possible.

yes, this is possible. any way to check daemonsets on node?

---

Comments from Reviewable

[fraenkel]

Review status: 0 of 1 files reviewed at latest revision, 1 unresolved discussion.

---

pkg/kubectl/cmd/drain.go, line 350 [r1] (raw file):

Previously, AdoHe (TonyAdo) wrote…

yes, this is possible. any way to check daemonsets on node?

daemonSets seem to be treated differently. There is already a flag on drain to ignore daemonSets (--ignore-daemonsets). However, it too has a similar underlying issue. If the Controller or Namespace cannot be found, drain will always fail.

We can choose to fix the filters or go fix the root problem which is what to do with the Controller is not found.
There is a comment saying that a missing Controller is an error but something more sophisticated is needed. This comment has been there since drain was first created. If one cannot find a pod's controller is it safe to remove the pod?

The other option is to allow the --force and --ignore-daemonsets to actually work in all cases rather than fail when the controller is not found. Although this seems more like a bug if the Controller and Namespace are missing for a DaemonSet.

My plan is to move the current logic to the unreplicatedFilter and let all the other filters apply. If we run across some new situation, we can review the appropriate fix at that time.

---

Comments from Reviewable

[brendandburns]

LGTM from the bulk LGTM tool (testing...)

[k8s-bot]

GCE e2e build/test passed for commit 9cff11d.

• Test Results
• Build Log
• Test Artifacts
• Internal Jenkins Results

[liggitt]

untagging until assignee comments
@brendandburns, do we know why the tag was added here?

[pwittrock]

@k8s-bot test this issue: #IGNORE

[k8s-ci-robot]

Jenkins GKE smoke e2e failed for commit 9cff11d. Full PR test history.

The magic incantation to run this job again is @k8s-bot gke e2e test this. Please help us cut down flakes by linking to an open flake issue when you hit one in your PR.

[k8s-github-robot]

@k8s-bot test this [submit-queue is verifying that this PR is safe to merge]

[k8s-github-robot]

Automatic merge from submit-queue