Add pod-selector kubectl drain #56864

juanvallejo · 2017-12-05T22:07:05Z

Release note:

Added the ability to select pods in a chosen node to be drained, based on given pod label-selector

This patch adds the ability to select pods in a chosen node to be drained, based on given pod label-selector. Related downstream issue: openshift/origin#17554

Further, it removes explicit, specific, pod-controller check. The drain command currently fails if a pod has a controller of a kind not explicitly handled in the command itself. This causes drain to be unusable if a node contains pods managed by third-party, or "unknown" controllers.

Based on this comment, the expectation was to fail if a pod's controller was not found for whatever reason. I believe that the drain command should not care about the existence of a pod controller. It should only care whether a pod has one, and act according to that controller kind. This solves a downstream bug: openshift/origin#17563

cc @fabianofranz @deads2k @kubernetes/sig-cli-misc

k8s-ci-robot · 2017-12-05T22:07:14Z

@juanvallejo: Reiterating the mentions to trigger a notification:
@kubernetes/sig-cli-misc

In response to this:

Release note:
Added the ability to select pods in a chosen node to be drained, based on given pod label-selector
This patch adds the ability to select pods in a chosen node to be drained, based on given pod label-selector. Related downstream issue: openshift/origin#17554

Further, it removes explicit, specific, pod-controller check. The drain command currently fails if a pod has a controller of a kind not explicitly handled in the command itself. This causes drain to be unusable if a node contains pods managed by third-party, or "unknown" controllers.

Based on this comment, the expectation was to fail if a pod's controller was not found for whatever reason. I believe that the drain command should not care about the existence of a pod controller. It should only care whether a pod has one, and act according to that controller kind. This solves a downstream bug: openshift/origin#17563

Will add tests

cc @fabianofranz @deads2k @kubernetes/sig-cli-misc

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.

cblecker · 2017-12-05T23:27:39Z

/ok-to-test

juanvallejo · 2017-12-06T18:03:11Z

ping @fabianofranz or @soltysh

juanvallejo · 2017-12-13T00:12:28Z

@fabianofranz friendly ping

deads2k · 2017-12-18T13:04:35Z

pkg/kubectl/cmd/drain.go

-	}
-	return nil, fmt.Errorf("Unknown controller kind %q", controllerRef.Kind)
-}
-
 func (o *DrainOptions) getPodController(pod corev1.Pod) (*metav1.OwnerReference, error) {


This method no longer returns an error.

deads2k · 2017-12-18T13:05:16Z

Fix method signature, squash in your tests, then lgtm.

deads2k · 2017-12-18T15:58:36Z

pkg/kubectl/cmd/drain.go

@@ -368,18 +348,14 @@ func (o *DrainOptions) unreplicatedFilter(pod corev1.Pod) (bool, *warning, *fata
 		return true, nil, nil
 	}

-	controllerRef, err := o.getPodController(pod)
-	if err != nil {
+	controllerRef := o.getPodController(pod)


Is this just?

if controllerRef != nil{ return true, nil, nil } if o.Force{ return true, &warning{kUnmanagedWarning}, nil } return false, nil, &fatal{kUnmanagedFatal}

deads2k · 2017-12-18T15:58:59Z

pkg/kubectl/cmd/drain.go

@@ -393,23 +369,20 @@ func (o *DrainOptions) daemonsetFilter(pod corev1.Pod) (bool, *warning, *fatal)
 	// The exception is for pods that are orphaned (the referencing
 	// management resource - including DaemonSet - is not found).
 	// Such pods will be deleted if --force is used.
-	controllerRef, err := o.getPodController(pod)
-	if err != nil {
+	controllerRef := o.getPodController(pod)


Use if's and early returns please

deads2k · 2017-12-18T15:59:18Z

comments and squash in that last commit.

This allows pods with third-party, or unknown controllers to be drained successfully.

juanvallejo · 2017-12-18T18:09:43Z

@deads2k thanks, done

deads2k · 2017-12-18T18:58:01Z

/lgtm

deads2k · 2017-12-18T18:58:20Z

/approve no-issue

k8s-ci-robot · 2017-12-18T18:58:26Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: deads2k, juanvallejo

Associated issue requirement bypassed by: deads2k

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

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

~~hack/OWNERS~~ [deads2k]
~~pkg/kubectl/OWNERS~~ [deads2k]

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 · 2017-12-19T02:50:44Z

Automatic merge from submit-queue (batch tested with PRs 55751, 57337, 56406, 56864, 57347). If you want to cherry-pick this change to another branch, please follow the instructions here.

@deads2k

…rain Automatic merge from submit-queue (batch tested with PRs 17072, 17616). Add --selector, --pod-selector flags `oc adm drain` Fixes #17554 Fixes #17563 Fixes https://bugzilla.redhat.com/show_bug.cgi?id=1525340 Picks kubernetes/kubernetes#52917, kubernetes/kubernetes#54083, and kubernetes/kubernetes#56864 to bring in `--selector` and `--pod-selector` flag support to `oc adm drain`. cc @openshift/cli-review @deads2k @dustymabe

:100644 100644 910654f6f6... 37f3ef1175... M pkg/kubectl/cmd/drain.go

@mfojtik

…lector Automatic merge from submit-queue. Pick 17616: Add --selector, --pod-selector flags `oc adm drain` UPSTREAM kubernetes/kubernetes#56864 Pick of #17616 Fixes https://bugzilla.redhat.com/show_bug.cgi?id=1525340 cc @mfojtik @deads2k @soltysh

…k of origin kubernetes#17616 :100644 100644 5a7c9db38b... 4e59f91ec8... M pkg/kubectl/cmd/drain.go

When it initially landed in kubernetes/kubernetes@c6e9ad066e (Initial node drain implementation for kubernetes#3885, 2015-08-30, kubernetes#16698), the drain logic looked in a created-by annotation for recognized kinds [1], so listing the set of recognized kinds was a clear approach. Sometime later, the source moved into ownerReferences, but the hard-coded set of recognized controller kinds remained. When kubernetes/kubernetes@2f1108451f (Remove hard-coded pod-controller check, 2017-12-05, kubernetes#56864) removed the hard-coded set of recognized controller kinds, it should have also updated these messages to remove stale references to the previous hard-coded values. This commit catches the message strings up with that commit. [1]: kubernetes@c6e9ad0#diff-211259b8a8ec42f105264c10897dad48029badb538684e60e43eaead68c3d219R216

When it initially landed in kubernetes/kubernetes@c6e9ad066e (Initial node drain implementation for #3885, 2015-08-30, kubernetes/kubernetes#16698), the drain logic looked in a created-by annotation for recognized kinds [1], so listing the set of recognized kinds was a clear approach. Sometime later, the source moved into ownerReferences, but the hard-coded set of recognized controller kinds remained. When kubernetes/kubernetes@2f1108451f (Remove hard-coded pod-controller check, 2017-12-05, kubernetes/kubernetes#56864) removed the hard-coded set of recognized controller kinds, it should have also updated these messages to remove stale references to the previous hard-coded values. This commit catches the message strings up with that commit. [1]: kubernetes/kubernetes@c6e9ad0#diff-211259b8a8ec42f105264c10897dad48029badb538684e60e43eaead68c3d219R216 Kubernetes-commit: 587f4f04cc5fc18f4e85ed6a4a06bbf1bfee0496

k8s-github-robot assigned soltysh and adohe-zz Dec 5, 2017

juanvallejo mentioned this pull request Dec 5, 2017

Add --selector, --pod-selector flags oc adm drain openshift/origin#17616

Merged

k8s-ci-robot removed the needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. label Dec 5, 2017

kubernetes deleted a comment from k8s-github-robot Dec 6, 2017

juanvallejo force-pushed the jvallejo/add-selector-kubectl-drain branch 3 times, most recently from 1ab2f76 to ccd37a1 Compare December 6, 2017 16:41

k8s-ci-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Dec 6, 2017

k8s-github-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Dec 14, 2017

juanvallejo force-pushed the jvallejo/add-selector-kubectl-drain branch from ccd37a1 to 31b2bd5 Compare December 15, 2017 15:48

k8s-github-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Dec 15, 2017

deads2k mentioned this pull request Dec 18, 2017

Shim build config to oc adm drain openshift/origin#17850

Closed

deads2k reviewed Dec 18, 2017

View reviewed changes

add --pod-selector opt kubectl drain

151398e

juanvallejo force-pushed the jvallejo/add-selector-kubectl-drain branch from 2edca06 to 512f708 Compare December 18, 2017 15:15

deads2k reviewed Dec 18, 2017

View reviewed changes

Remove hard-coded pod-controller check

2f11084

This allows pods with third-party, or unknown controllers to be drained successfully.

juanvallejo force-pushed the jvallejo/add-selector-kubectl-drain branch from 512f708 to 2f11084 Compare December 18, 2017 18:09

k8s-ci-robot assigned deads2k Dec 18, 2017

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Dec 18, 2017

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Dec 18, 2017

juanvallejo mentioned this pull request Dec 18, 2017

Pick 17616: Add --selector, --pod-selector flags oc adm drain openshift/origin#17868

Merged

k8s-github-robot merged commit 3ef6741 into kubernetes:master Dec 19, 2017

juanvallejo deleted the jvallejo/add-selector-kubectl-drain branch December 19, 2017 21:24

deads2k pushed a commit to openshift/kubernetes that referenced this pull request Jan 2, 2018

UPSTREAM: 56864: pick pod-selector changes from kubernetes#56864

6cd0c69

:100644 100644 910654f6f6... 37f3ef1175... M pkg/kubectl/cmd/drain.go

deads2k pushed a commit to deads2k/kubernetes that referenced this pull request Jan 5, 2018

UPSTREAM: 56864: pick pod-selector changes from kubernetes#56864

89def23

:100644 100644 910654f6f6... 37f3ef1175... M pkg/kubectl/cmd/drain.go

deads2k pushed a commit to deads2k/kubernetes that referenced this pull request Jan 5, 2018

UPSTREAM: 56864: pick pod-selector changes from kubernetes#56864

b8ff124

:100644 100644 910654f6f6... 37f3ef1175... M pkg/kubectl/cmd/drain.go

liggitt pushed a commit to openshift/kubernetes that referenced this pull request Jan 10, 2018

UPSTREAM: 56864: pick pod-selector changes from kubernetes#56864: pic…

b2f1fa0

…k of origin kubernetes#17616 :100644 100644 5a7c9db38b... 4e59f91ec8... M pkg/kubectl/cmd/drain.go

wking mentioned this pull request Feb 24, 2022

kubectl drain: Generic "declare a controller" message #108314

Merged

atiratree mentioned this pull request Sep 12, 2023

kubectl drain error message confusing when using with operator #57049

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add pod-selector kubectl drain #56864

Add pod-selector kubectl drain #56864

juanvallejo commented Dec 5, 2017 •

edited

Loading

k8s-ci-robot commented Dec 5, 2017

cblecker commented Dec 5, 2017

juanvallejo commented Dec 6, 2017

juanvallejo commented Dec 13, 2017

deads2k Dec 18, 2017

deads2k commented Dec 18, 2017

deads2k Dec 18, 2017

deads2k Dec 18, 2017

deads2k commented Dec 18, 2017

juanvallejo commented Dec 18, 2017

deads2k commented Dec 18, 2017

deads2k commented Dec 18, 2017

k8s-ci-robot commented Dec 18, 2017

k8s-github-robot commented Dec 19, 2017

Add pod-selector kubectl drain #56864

Add pod-selector kubectl drain #56864

Conversation

juanvallejo commented Dec 5, 2017 • edited Loading

k8s-ci-robot commented Dec 5, 2017

cblecker commented Dec 5, 2017

juanvallejo commented Dec 6, 2017

juanvallejo commented Dec 13, 2017

deads2k Dec 18, 2017

Choose a reason for hiding this comment

deads2k commented Dec 18, 2017

deads2k Dec 18, 2017

Choose a reason for hiding this comment

deads2k Dec 18, 2017

Choose a reason for hiding this comment

deads2k commented Dec 18, 2017

juanvallejo commented Dec 18, 2017

deads2k commented Dec 18, 2017

deads2k commented Dec 18, 2017

k8s-ci-robot commented Dec 18, 2017

k8s-github-robot commented Dec 19, 2017

juanvallejo commented Dec 5, 2017 •

edited

Loading