-
Notifications
You must be signed in to change notification settings - Fork 38
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
BUG 1804738: Ensure DeleteNodes doesn't delete a node twice #125
BUG 1804738: Ensure DeleteNodes doesn't delete a node twice #125
Conversation
cc374c2
to
40d7460
Compare
/hold |
@enxebre: once the present PR merges, I will cherry-pick it on top of release-4.4 in a new PR and assign it to you. In response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
1088ec3
to
b47c402
Compare
@JoelSpeed: This pull request references Bugzilla bug 1804738, which is invalid:
Comment In response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
/bugzilla refresh |
@alexander-demichev: This pull request references Bugzilla bug 1804738, which is valid. The bug has been moved to the POST state. The bug has been updated to refer to the pull request using the external bug tracker. In response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
/hold cancel |
lgtm PTAL @frobware, would like your blessing here |
/lgtm |
thanks! @JoelSpeed please make sure to follow up with the cherry-picks |
/retest |
@JoelSpeed: All pull requests linked via external trackers have merged. Bugzilla bug 1804738 has been moved to the MODIFIED state. In response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
@enxebre: new pull request created: #128 In response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
/cherry-pick release-4.3 |
@JoelSpeed: new pull request created: #129 In response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
Yep, sorry (github outage yesterday). I did have one minor nit, but not important now. |
I have seen the autoscaler call
DeleteNodes
several times while a node it is trying to delete has not been fully deleted (finalizer still present). Currently, it does not check if the node's Machine has already been deleted and blindly reduces the size of the node group by 1 on every call toDeleteNodes
This PR adds a check, so that if a node's
Machine
already has a deletion timestamp, we do not decrease the size of the node group any furtherI've added a test that calls the
DeleteNodes
twice and expects the node group size to not decrease on the second call, which passes with the fix, and fails without