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.

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

Add pod-selector kubectl drain #56864

Conversation

juanvallejo
Copy link
Contributor

@juanvallejo juanvallejo commented Dec 5, 2017

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 k8s-ci-robot added release-note Denotes a PR that will be considered when it comes time to generate release notes. sig/cli Categorizes an issue or PR as relevant to SIG CLI. size/M Denotes a PR that changes 30-99 lines, ignoring generated files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Dec 5, 2017
@k8s-ci-robot
Copy link
Contributor

@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
Copy link
Member

cblecker commented Dec 5, 2017

/ok-to-test

@k8s-ci-robot 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 kubernetes deleted a comment from k8s-github-robot Dec 6, 2017
@juanvallejo 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 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
@juanvallejo
Copy link
Contributor Author

ping @fabianofranz or @soltysh

@juanvallejo
Copy link
Contributor Author

@fabianofranz friendly ping

@k8s-github-robot 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 juanvallejo force-pushed the jvallejo/add-selector-kubectl-drain branch from ccd37a1 to 31b2bd5 Compare December 15, 2017 15:48
@k8s-github-robot k8s-github-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Dec 15, 2017
}
return nil, fmt.Errorf("Unknown controller kind %q", controllerRef.Kind)
}

func (o *DrainOptions) getPodController(pod corev1.Pod) (*metav1.OwnerReference, error) {
Copy link
Contributor

Choose a reason for hiding this comment

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

This method no longer returns an error.

@deads2k
Copy link
Contributor

deads2k commented Dec 18, 2017

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

@juanvallejo juanvallejo force-pushed the jvallejo/add-selector-kubectl-drain branch from 2edca06 to 512f708 Compare December 18, 2017 15:15
@@ -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)
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this just?

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

return false, nil, &fatal{kUnmanagedFatal}

@@ -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)
Copy link
Contributor

Choose a reason for hiding this comment

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

Use if's and early returns please

@deads2k
Copy link
Contributor

deads2k commented Dec 18, 2017

comments and squash in that last commit.

This allows pods with third-party, or unknown controllers to be drained
successfully.
@juanvallejo juanvallejo force-pushed the jvallejo/add-selector-kubectl-drain branch from 512f708 to 2f11084 Compare December 18, 2017 18:09
@juanvallejo
Copy link
Contributor Author

@deads2k thanks, done

@deads2k
Copy link
Contributor

deads2k commented Dec 18, 2017

/lgtm

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

deads2k commented Dec 18, 2017

/approve no-issue

@k8s-ci-robot
Copy link
Contributor

[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:

You can indicate your approval by writing /approve in a comment
You can cancel your approval by writing /approve cancel in a comment

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

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.

@k8s-github-robot k8s-github-robot merged commit 3ef6741 into kubernetes:master Dec 19, 2017
@juanvallejo juanvallejo deleted the jvallejo/add-selector-kubectl-drain branch December 19, 2017 21:24
openshift-merge-robot added a commit to openshift/origin that referenced this pull request Dec 21, 2017
…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
deads2k pushed a commit to openshift/kubernetes that referenced this pull request Jan 2, 2018
: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
: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
:100644 100644 910654f6f6... 37f3ef1175... M	pkg/kubectl/cmd/drain.go
openshift-merge-robot added a commit to openshift/origin that referenced this pull request Jan 8, 2018
…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
liggitt pushed a commit to openshift/kubernetes that referenced this pull request Jan 10, 2018
…k of origin kubernetes#17616

:100644 100644 5a7c9db38b... 4e59f91ec8... M	pkg/kubectl/cmd/drain.go
wking added a commit to wking/kubernetes that referenced this pull request Feb 24, 2022
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
k8s-publishing-bot pushed a commit to kubernetes/kubectl that referenced this pull request Mar 5, 2022
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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
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 Denotes a PR that will be considered when it comes time to generate release notes. sig/cli Categorizes an issue or PR as relevant to SIG CLI. size/L Denotes a PR that changes 100-499 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants