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

oc adm drain no longer handles BuildControllers #17563

Closed
dustymabe opened this issue Dec 1, 2017 · 4 comments
Closed

oc adm drain no longer handles BuildControllers #17563

dustymabe opened this issue Dec 1, 2017 · 4 comments
Assignees
Labels
component/cli kind/bug Categorizes issue or PR as related to a bug. priority/P1

Comments

@dustymabe
Copy link
Member

dustymabe commented Dec 1, 2017

Older versions of oc adm drain binary handled BuildControllers just fine. Seems like after it was upstreamed into k8s it no longer does

[root@ip-10-0-172-3 updated]# oc get bc --all-namespaces                                                          
NAMESPACE   NAME                  TYPE      FROM      LATEST                                                          
foo1        django-psql-example   Source    Git       1

[root@ip-10-0-172-3 updated]# oc adm drain --ignore-daemonsets  10.0.225.178                                      
node "10.0.225.178" already cordoned                                                                                  
error: Unknown controller kind "Build": django-psql-example-1-build; pods with local storage (use --delete-local-data to override): postgresql-1-lcdv7
Version:
[root@ip-10-0-172-3 updated]# oc version
oc v3.8.0-alpha.0+21e534e-350-dirty
kubernetes v1.8.1+0d5291c
features: Basic-Auth GSSAPI Kerberos SPNEGO

Server https://10.0.172.3:8443
kubernetes v1.6.1+5115d708d7
@juanvallejo
Copy link
Contributor

@deads2k
Copy link
Contributor

deads2k commented Dec 1, 2017

@juanvallejo drain really shouldn't care what controller is dealing with a particular pod.

@smarterclayton
Copy link
Contributor

Wow, that's fail.

@juanvallejo
Copy link
Contributor

juanvallejo commented Dec 1, 2017

Per a brief discussion with @smarterclayton and David's feedback in kubernetes/kubernetes#56713 (comment):

  • drain should be updated to not care about the existence of a controller (only care if a pod is managed by one)
  • Pods should have an annotation / label that drain can use to decide if a pod is safe to delete or not
    • This annotation / label is set by the pod's controller.
    • If the pod does not have a controller, should a user be responsible for setting this?
  • PodFilters belong in a factory

@mfojtik mfojtik added component/cli kind/bug Categorizes issue or PR as related to a bug. priority/P1 labels Dec 4, 2017
k8s-github-robot pushed a commit to kubernetes/kubernetes that referenced this issue Dec 19, 2017
…ctl-drain

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 <a href="https://github.com/kubernetes/community/blob/master/contributors/devel/cherry-picks.md">here</a>.

Add pod-selector kubectl drain

**Release note**:
```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](https://github.com/kubernetes/kubernetes/blob/master/pkg/kubectl/cmd/drain.go#L331). This causes `drain` to be unusable if a node contains pods managed by third-party, or "unknown" controllers.

Based on [this comment](https://github.com/kubernetes/kubernetes/blob/master/pkg/kubectl/cmd/drain.go#L353), 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
openshift-merge-robot added a commit that referenced this issue 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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
component/cli kind/bug Categorizes issue or PR as related to a bug. priority/P1
Projects
None yet
Development

No branches or pull requests

5 participants