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.

Sign up for GitHub

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

Jump to bottom

Support eviction and statefulset in kubectl drain #35483

Merged
merged 1 commit into from
Nov 8, 2016
Merged

Support eviction and statefulset in kubectl drain #35483

merged 1 commit into from
Nov 8, 2016

Conversation

mengqiy
Copy link
Member

@mengqiy mengqiy commented Oct 25, 2016 •
edited
Loading

Support deleting pets for kubectl drain.
Use evict to delete pods.

Fixes: #33727

Adds support for StatefulSets in kubectl drain.
Switches to use the eviction sub-resource instead of deletion in kubectl drain, if server supports.

@foxish @caesarxuchao

This change is Reviewable

@mengqiy
Copy link
Member Author

mengqiy commented Oct 25, 2016

I will add some tests after someone confirms the work flow is correct.

@k8s-github-robot k8s-github-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. release-note-label-needed labels Oct 25, 2016
@foxish foxish assigned foxish and erictune and unassigned lavalamp Oct 25, 2016
@mengqiy mengqiy force-pushed the use_evict_for_drain branch from 1643721 to 68e6d92 Compare October 25, 2016 03:31
@0xmichalis
Copy link
Contributor

@soltysh the eviction client we were talking about

@mengqiy mengqiy force-pushed the use_evict_for_drain branch 2 times, most recently from 3070c82 to b409e59 Compare October 25, 2016 20:34
@maisem maisem mentioned this pull request Oct 27, 2016
Closed
@chrislovecnm
Copy link
Contributor

@foxish this is kinda a show stopper. Can we bump to a p0?

@chrislovecnm
Copy link
Contributor

chrislovecnm commented Oct 29, 2016 •
edited
Loading

@ymqytw this is in on of the builds I1024 19:40:17.565] FAILED hack/make-rules/../../hack/verify-bazel.sh for the verification tests. Please review the unit tests and verifications tests. The e2e's can be pissy, but usually these are spot on.

@chrislovecnm
Copy link
Contributor

@k8s-bot gci gce e2e test this

@foxish
Copy link
Contributor

foxish commented Oct 29, 2016

@chrislovecnm will review this later today.

@chrislovecnm
Copy link
Contributor

@foxish thanks!!

@mengqiy
Copy link
Member Author

mengqiy commented Oct 29, 2016

@chrislovecnm The unit tests and verify test have not been fixed, I will fix them next week.

@k8s-github-robot k8s-github-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Oct 29, 2016
@pwittrock
Copy link
Member

@kubernetes/sig-cluster-lifecycle @erictune How critical is this given we are taking taking PetSets out of beta?

@mikedanese
Copy link
Member

mikedanese commented Oct 31, 2016 •
edited
Loading

@pwittrock why @kubernetes/sig-cluster-lifecycle ? AFAIK, nothing in lifecycle is actively gated by petset.

@pwittrock
Copy link
Member

@mikedanese I thought the capability to drain nodes for maintenance / removal from the cluster would be something sig-cluster-lifecycle would have a stake in. Is this incorrect?

@mikedanese
Copy link
Member

Yes that is correct. Thanks for clarifying.

@mengqiy mengqiy force-pushed the use_evict_for_drain branch from b409e59 to c75f98f Compare November 1, 2016 19:22
@k8s-github-robot k8s-github-robot added kind/api-change Categorizes issue or PR as related to adding, removing, or otherwise changing an API and removed needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. labels Nov 1, 2016
@mengqiy
Copy link
Member Author

mengqiy commented Nov 1, 2016

I updated the code to use Delete instead of Evict.
The codec for unit test doesn't work for eviction. I will ask @caesarxuchao for help.

@mengqiy mengqiy force-pushed the use_evict_for_drain branch from c75f98f to a231cc8 Compare November 1, 2016 20:27
@foxish foxish added the release-note Denotes a PR that will be considered when it comes time to generate release notes. label Nov 1, 2016
@mengqiy mengqiy force-pushed the use_evict_for_drain branch from d1a0971 to 57b877e Compare November 7, 2016 17:44
@mengqiy
Copy link
Member Author

mengqiy commented Nov 7, 2016

@liggitt @caesarxuchao @janetkuo Any further comments?

@mengqiy
Copy link
Member Author

mengqiy commented Nov 7, 2016

@kubernetes/kubectl

@janetkuo
Copy link
Member

janetkuo commented Nov 7, 2016

LGTM

@janetkuo janetkuo dismissed caesarxuchao’s stale review November 7, 2016 22:58

Chao LGTM'ed but forgot to approve changes

@janetkuo
Copy link
Member

janetkuo commented Nov 7, 2016

Please squash commits.

@caesarxuchao
Copy link
Member

@ymqytw thank you for bearing with me.

@foxish
Copy link
Contributor

foxish commented Nov 8, 2016

Ok to apply the LGTM label here and queue it up for merge?

@mengqiy mengqiy force-pushed the use_evict_for_drain branch from 57b877e to b73fae6 Compare November 8, 2016 00:13
@mengqiy
Copy link
Member Author

mengqiy commented Nov 8, 2016

Squashed.

@foxish foxish added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Nov 8, 2016
@foxish
Copy link
Contributor

foxish commented Nov 8, 2016

Adding LGTM based on @caesarxuchao and @janetkuo's. Bumping up priority to P2 to avoid having to rebase.

@foxish foxish added the priority/backlog Higher priority than priority/awaiting-more-evidence. label Nov 8, 2016
@k8s-github-robot
Copy link

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

@k8s-github-robot
Copy link

Automatic merge from submit-queue

@k8s-github-robot k8s-github-robot merged commit 13cc43a into kubernetes:master Nov 8, 2016
@mengqiy mengqiy mentioned this pull request Nov 8, 2016
Closed
@mengqiy mengqiy deleted the use_evict_for_drain branch November 8, 2016 02:26
@erictune
Copy link
Member

erictune commented Nov 9, 2016

Thank so much for this @ymqytw

@erictune erictune mentioned this pull request Nov 22, 2016
Closed
@mengqiy mengqiy mentioned this pull request Dec 2, 2016
Closed
@chentao1596 chentao1596 mentioned this pull request Dec 5, 2016
Closed
@mengqiy mengqiy changed the title Fix kubectl drain for statefulset Support eviction and statefulset in kubectl drain Mar 3, 2017
@wking wking mentioned this pull request Jul 17, 2018
Merged
wking added a commit to wking/kubernetes that referenced this pull request Sep 24, 2018
@wking
kubectl: Drop backOff from DrainOptions
ab993e3 
The property was added in b73fae6 (Fix kubectl drain for statefulset
and use eviciton for drain if possible, 2016-10-20, kubernetes#35483), but
b358b2d (make drain retry forever and use new timeout,
2016-11-28, kubernetes#37604) removed the last consumer.
vithati pushed a commit to vithati/kubernetes that referenced this pull request Oct 25, 2018
@wking @vithati
kubectl: Drop backOff from DrainOptions
b085d8e 
The property was added in b73fae6 (Fix kubectl drain for statefulset
and use eviciton for drain if possible, 2016-10-20, kubernetes#35483), but
b358b2d (make drain retry forever and use new timeout,
2016-11-28, kubernetes#37604) removed the last consumer.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@janetkuo janetkuo janetkuo left review comments

@0xmichalis 0xmichalis 0xmichalis left review comments

@caesarxuchao caesarxuchao caesarxuchao left review comments

@liggitt liggitt liggitt left review comments

@foxish foxish foxish left review comments

Assignees

@caesarxuchao caesarxuchao

@janetkuo janetkuo

Labels
kind/api-change Categorizes issue or PR as related to adding, removing, or otherwise changing an API lgtm "Looks good to me", indicates that a PR is ready to be merged. priority/backlog Higher priority than priority/awaiting-more-evidence. release-note Denotes a PR that will be considered when it comes time to generate release notes. size/XL Denotes a PR that changes 500-999 lines, ignoring generated files.
Projects
None yet
Milestone
v1.5
Development

Successfully merging this pull request may close these issues.