Stop 'kubectl drain' deleting pods with local storage. #26667
Conversation
mml
added
priority/important-soon
k8s-github-robot
added
kind/old-docs
size/L
|
|
k8s-bot test this please: issue #20576 |
|
Well, at least this one's more time efficient. |
2 similar comments
| if !o.IgnoreDaemonsets { | ||
| return false, nil, &fatal{kDaemonsetFatal} | ||
| } | ||
| return false, &warning{kDaemonsetWarning}, nil |
There was a problem hiding this comment.
No. No matter what, we never delete the daemonset pods because the DS controller doesn't respect unschedulable. The flag only toggles whether we consider their presence an error or not.
I will add a comment that clarifies this.
|
Sorry for the delay in reviewing this. I like the simplification. I just had a few comments. |
|
|
||
| // getPodsForDeletion returns all the pods we're going to delete. If there are | ||
| // any unmanaged pods and the user didn't pass --force, we return that list in | ||
| // an error. |
There was a problem hiding this comment.
Note to self: update this comment.
| @@ -33,6 +33,10 @@ will make the node schedulable again. | |||
|
|
|||
| .SH OPTIONS | |||
| .PP | |||
| \fB\-\-delete\-local\-data\fP=false | |||
| Continue even if there are pods with local data (EmptyDir). | |||
There was a problem hiding this comment.
I don't think this should just say "local data" since HostDir is also local data. Maybe say "Continue even if there are pods using emptyDir (local data that will be deleted when the node is drained)."
There was a problem hiding this comment.
Note: I think the flag name is fine, I'm just referring to the way you described it (here and elsewhere in the PR).
|
@davidopp fixed docs PTAL |
|
LGTM Please squash the commits and then apply LGTM label. |
|
@mml |
k8s-github-robot
removed
the
lgtm
|
@spxtr any idea what's going on with the bot? It's talking nonsense about a re-test, and then it took the lgtm label off without saying why. |
k8s-github-robot
added
the
needs-rebase
Unless forced with --delete-local-data. Also a refactoring of the kubectl drain logic that selects/rejects pods and produces error/warning messages.
k8s-github-robot
removed
lgtm
No idea. Note that if you ask for a retest without adding the issue: it will delete your comment. |
|
GCE e2e build/test passed for commit d09af4a. |
|
@k8s-bot test this [submit-queue is verifying that this PR is safe to merge] |
|
GCE e2e build/test passed for commit d09af4a. |
|
Automatic merge from submit-queue |
|
|
||
| func hasLocalStorage(pod api.Pod) bool { | ||
| for _, volume := range pod.Spec.Volumes { | ||
| if volume.EmptyDir != nil { |
There was a problem hiding this comment.
Matt and I discussed this and eventually decided to leave it out. We don't know what people are going to do after the drain. They might be doing something that doesn't delete all the data (for example, upgrade OS and reboot). Arguably we should have yet another flag to control whether to consider hostPath, but for simplicity we decided to just block on the case we know for sure will be deleted, namely emptyDir.
For cluster scale-down, the node goes away "forever" so it makes sense to block on hostPath. But for kubectl drain I think it's not necessary.
Kubectl drain will not continue if there are pods with local storage unless
forced with --delete-local-data.
Fixes #23972