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

kubectl: Allow 'drain --force' to remove orphaned pods #41864

Merged
merged 1 commit into from
Feb 26, 2017
Merged

kubectl: Allow 'drain --force' to remove orphaned pods #41864

merged 1 commit into from
Feb 26, 2017

Conversation

marun
Copy link
Contributor

@marun marun commented Feb 22, 2017 •
edited by derekwaynecarr

If the managing resource of a given pod (e.g. DaemonSet/ReplicaSet/etc) is deleted (effectively orphaning the pod), and kubectl drain --force is invoked on the node hosting the pod, the command would fail with an error indicating that the managing resource was not found. This PR reduces the error to a warning if --force is specified, allowing nodes with orphaned pods to be drained.

Reference: https://bugzilla.redhat.com/show_bug.cgi?id=1424678

cc: @derekwaynecarr

Allow drain --force to remove pods whose managing resource is deleted.

@k8s-ci-robot k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Feb 22, 2017
@k8s-github-robot k8s-github-robot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. release-note-label-needed labels Feb 22, 2017
@k8s-reviewable
Copy link

This change is Reviewable

liggitt
liggitt reviewed Feb 22, 2017
View reviewed changes
pkg/kubectl/cmd/drain.go Outdated
@@ -308,7 +308,10 @@ func (o *DrainOptions) unreplicatedFilter(pod api.Pod) (bool, *warning, *fatal)
}

sr, err := o.getPodCreator(pod)
if err != nil {
removeOrphandedPod := apierrors.IsNotFound(err) && o.Force
Copy link
Member

Choose a reason for hiding this comment

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

removeOrphanedPod

Copy link
Member

Choose a reason for hiding this comment

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

just nesting the condition might be easier to follow

if err != nil {
  // if we're forcing, remove orphaned pods with a warning
  if apierrors.IsNotFound(err) && o.Force {
    return true, &warning{err.Error()}, nil
  }
  return false, nil, &fatal{err.Error()}
}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

@liggitt
Copy link
Member

liggitt commented Feb 22, 2017

does any of the help need updating?

@marun
Copy link
Contributor Author

marun commented Feb 22, 2017

@liggitt Help has been updated.

liggitt
liggitt reviewed Feb 22, 2017
View reviewed changes
pkg/kubectl/cmd/drain.go Outdated
@@ -151,7 +151,8 @@ var (
DaemonSet controller, which ignores unschedulable markings. If there are any
pods that are neither mirror pods nor managed by ReplicationController,
ReplicaSet, DaemonSet, StatefulSet or Job, then drain will not delete any pods unless you
use --force.
use --force. --force will also allow deletion to proceed if the managing resource of one
or more pods is missing.
Copy link
Member

Choose a reason for hiding this comment

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

indent

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done. Tabs, yuck.

@liggitt
Copy link
Member

liggitt commented Feb 22, 2017

nit on help indent, LGTM

@derekwaynecarr derekwaynecarr added release-note Denotes a PR that will be considered when it comes time to generate release notes. and removed release-note-label-needed labels Feb 23, 2017
@derekwaynecarr
Copy link
Member

i updated text w/ a release-note, @deads2k can we get an review please?

@deads2k
Copy link
Contributor

deads2k commented Feb 23, 2017

/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 Feb 23, 2017
@k8s-github-robot
Copy link

[APPROVALNOTIFIER] This PR is APPROVED

The following people have approved this PR: deads2k, marun

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-github-robot k8s-github-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Feb 23, 2017
@marun
Copy link
Contributor Author

marun commented Feb 25, 2017

@derekwaynecarr How to merge post-approval?

@liggitt
Copy link
Member

liggitt commented Feb 25, 2017 •
edited

wait in the long, long queue. if the submit queue has a green check mark, you're in the line.

@k8s-github-robot
Copy link

Automatic merge from submit-queue (batch tested with PRs 41857, 41864, 40522, 41835, 41991)

@k8s-github-robot k8s-github-robot merged commit c4835f2 into kubernetes:master Feb 26, 2017
@marun marun deleted the kubectl-drain-orphans branch February 27, 2017 17:40
@KnVerey KnVerey mentioned this pull request Nov 8, 2022
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@liggitt liggitt liggitt left review comments

Assignees

@deads2k deads2k

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. size/M Denotes a PR that changes 30-99 lines, ignoring generated files.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
7 participants
@marun @k8s-reviewable @liggitt @derekwaynecarr @deads2k @k8s-github-robot @k8s-ci-robot