-
Notifications
You must be signed in to change notification settings - Fork 39.7k
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 kubectl drain error handling bug. #122574
Conversation
/hold for review /cc @eddiezane |
/triage accepted |
Fixed a bug where kubectl drain would consider a pod as having been deleted if an error occurs while calling the API.
55536a8
to
4aecb15
Compare
/approve |
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: ardaguclu, brianpursley 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 |
/unhold |
@ardaguclu can you lgtm if you think this is good to go? |
Thanks for the fix |
LGTM label has been added. Git tree hash: 167b8a70e9b51f83d5cc6296edc0f413bd47398b
|
What type of PR is this?
/kind bug
What this PR does / why we need it:
Fixed a bug where kubectl drain would consider a pod as having been deleted if an error occurs while calling the API.
This can happen if you reboot the master node(s) while draining, for example. In this case an error occurs, but the pod has not actually been deleted yet.
The problem occurs because the client-go returns an empty struct instead of nil, even when there is an error, as shown here:
kubernetes/staging/src/k8s.io/client-go/kubernetes/typed/core/v1/pod.go
Lines 75 to 85 in 6aac45f
The kubectl drain code assumed that an error did not occur if the returned pod was not nil, which is incorrect.
This PR fixes that bug and updates the existing unit tests to use an empty struct instead of nil, matching what is actually happening. I also added a new test case to cover the case where a pod is returned, but the UID is different.
Which issue(s) this PR fixes:
Fixes kubernetes/kubectl#1532
Special notes for your reviewer:
Does this PR introduce a user-facing change?
Additional documentation e.g., KEPs (Kubernetes Enhancement Proposals), usage docs, etc.: