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

Fix drain for evicting terminal DS pods and pods with local storage #68767

Merged
merged 1 commit into from
Sep 19, 2018

Conversation

ravisantoshgudimetla
Copy link
Contributor

What this PR does / why we need it:

As of now draining a node is not possible even if there are only terminal pods(whose state is successful/failed) with local volumes, managed by DS. Since a terminal pod can be safely deleted any time, we should allow this operation.

Which issue(s) this PR fixes (optional, in fixes #<issue number>(, fixes #<issue_number>, ...) format, will close the issue(s) when PR gets merged):
Fixes #

Special notes for your reviewer:

Release note:

Drain should delete terminal pods.

@k8s-ci-robot k8s-ci-robot added release-note Denotes a PR that will be considered when it comes time to generate release notes. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. needs-kind Indicates a PR lacks a `kind/foo` label and requires one. area/kubectl needs-sig Indicates an issue or PR lacks a `sig/foo` label and requires one. sig/cli Categorizes an issue or PR as relevant to SIG CLI. and removed needs-sig Indicates an issue or PR lacks a `sig/foo` label and requires one. labels Sep 18, 2018
@ravisantoshgudimetla
Copy link
Contributor Author

Copy link
Contributor

@soltysh soltysh left a comment

Choose a reason for hiding this comment

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

/lgtm
/approve

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

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: ravisantoshgudimetla, soltysh

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

The pull request process is described here

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel 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 Sep 18, 2018
@soltysh soltysh added the kind/bug Categorizes issue or PR as related to a bug. label Sep 18, 2018
@k8s-ci-robot k8s-ci-robot removed the needs-kind Indicates a PR lacks a `kind/foo` label and requires one. label Sep 18, 2018
@soltysh
Copy link
Contributor

soltysh commented Sep 18, 2018

This is a significant blocker from a drain POV, setting milestone to v1.12

@soltysh soltysh added this to the v1.12 milestone Sep 18, 2018
@ravisantoshgudimetla
Copy link
Contributor Author

/priority critical-urgent

@k8s-ci-robot k8s-ci-robot added the priority/critical-urgent Highest priority. Must be actively worked on as someone's top priority right now. label Sep 18, 2018
@ravisantoshgudimetla
Copy link
Contributor Author

ravisantoshgudimetla commented Sep 18, 2018

/cherrypick release-1.11

@dims
Copy link
Member

dims commented Sep 18, 2018

/uncc

@k8s-ci-robot k8s-ci-robot removed the request for review from dims September 18, 2018 12:53
@tpepper
Copy link
Member

tpepper commented Sep 18, 2018

@soltysh and others here: Given the lateness in the 1.12 cycle, and that this appears to be a regression perhaps back as far as 1.10 if I'm researching the OpenShift<->Kubernetes versions mappings correctly, how critically urgent is it that this merge in the next four or five days? Versus being merged after code freeze and then backported to multiple releases where needed?

@ravisantoshgudimetla
Copy link
Contributor Author

ravisantoshgudimetla commented Sep 18, 2018

@tpepper If you are talking in terms of tests, I think this won't cause regression failure but yes, it is regression from previous releases(it is mostly a bug that went unnoticed). If this is causing upgrade tests to fail, I will look more into this. So, let's proceed with the merge, we can always revert, if we are not able to fix it in time.

@tpepper
Copy link
Member

tpepper commented Sep 18, 2018

Honestly given the description, I'd prefer we hold off on merging this at least until tomorrow. I'm trying to manage a small and specific set of critical bug fix changes today and clearly view their test impact ahead of building RC later today.

@guineveresaenger
Copy link
Contributor

/retest
/hold

Agreeing with Tim.

@k8s-ci-robot k8s-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Sep 18, 2018
@ravisantoshgudimetla
Copy link
Contributor Author

ravisantoshgudimetla commented Sep 18, 2018

Honestly given the description, I'd prefer we hold off on merging this at least until tomorrow. I'm trying to manage a small and specific set of critical bug fix changes today and clearly view their test impact ahead of building RC later today.

I understand your concern, let's hold it back till tomorrow.

EDIT: Just saw @guineveresaenger's hold. Thanks @guineveresaenger, I am fine with holding this PR until tomorrow.

@derekwaynecarr
Copy link
Member

@ravisantoshgudimetla upgrade tests should have been failing without this fix as we know upgrades fail due to this condition. should we augment upgrade tests to cover this scenario?

@ravisantoshgudimetla
Copy link
Contributor Author

@derekwaynecarr I am planning to cherrypick this fix till 1.10. Do you mean upgrade from 1.9 to 1.10 should fail without this fix?

@tpepper
Copy link
Member

tpepper commented Sep 19, 2018

We got the test results we were looking for today. I'm intrigued to see if this does help on some upgrade failures.

/hold cancel

@k8s-ci-robot k8s-ci-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Sep 19, 2018
@k8s-ci-robot k8s-ci-robot merged commit 4009817 into kubernetes:master Sep 19, 2018
@ravisantoshgudimetla
Copy link
Contributor Author

/cherrypick release-1.11

This pull request was closed.
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. area/kubectl cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. kind/bug Categorizes issue or PR as related to a bug. lgtm "Looks good to me", indicates that a PR is ready to be merged. priority/critical-urgent Highest priority. Must be actively worked on as someone's top priority right now. 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