-
Notifications
You must be signed in to change notification settings - Fork 39.6k
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
Add pod-selector kubectl drain #56864
Conversation
@juanvallejo: Reiterating the mentions to trigger a notification: In response to this:
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. |
/ok-to-test |
1ab2f76
to
ccd37a1
Compare
ping @fabianofranz or @soltysh |
@fabianofranz friendly ping |
ccd37a1
to
31b2bd5
Compare
pkg/kubectl/cmd/drain.go
Outdated
} | ||
return nil, fmt.Errorf("Unknown controller kind %q", controllerRef.Kind) | ||
} | ||
|
||
func (o *DrainOptions) getPodController(pod corev1.Pod) (*metav1.OwnerReference, error) { |
There was a problem hiding this comment.
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.
Fix method signature, squash in your tests, then lgtm. |
2edca06
to
512f708
Compare
pkg/kubectl/cmd/drain.go
Outdated
@@ -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) |
There was a problem hiding this comment.
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}
pkg/kubectl/cmd/drain.go
Outdated
@@ -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) |
There was a problem hiding this comment.
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
comments and squash in that last commit. |
This allows pods with third-party, or unknown controllers to be drained successfully.
512f708
to
2f11084
Compare
@deads2k thanks, done |
/lgtm |
/approve no-issue |
[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 |
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. |
…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
:100644 100644 910654f6f6... 37f3ef1175... M pkg/kubectl/cmd/drain.go
:100644 100644 910654f6f6... 37f3ef1175... M pkg/kubectl/cmd/drain.go
…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
Release note:
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 akind
not explicitly handled in the command itself. This causesdrain
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#17563cc @fabianofranz @deads2k @kubernetes/sig-cli-misc