Fix deadlock in DecreaseTargetSize by filtering placeholders Fixes #6128 #6817
Conversation
This commit resolves an issue where the DecreaseTargetSize function could enter a deadlock state by attempting to scale down when only placeholder nodes, marked as 'placeholderUnfulfillableStatus', are present. The updated function now filters out these placeholder nodes before calculating if a decrease in target size is permissible, ensuring that only operational nodes are considered in the scaling process. This prevents erroneous scaling activities that could impact cluster stability and node management in AWS Auto Scaling Groups. - Added checks to exclude placeholders in DecreaseTargetSize calculations. - Enhanced logging for better clarity when instance statuses are fetched but continue despite errors. Fixes kubernetes#6128
k8s-ci-robot
added
do-not-merge/invalid-commit-message
|
k8s-ci-robot
added
the
cncf-cla: no
|
Hi @ruiscosta. Thanks for your PR. I'm waiting for a kubernetes member to verify that this patch is reasonable to test. If it is, they should reply with Once the patch is verified, the new status will be reflected by the I understand the commands that are listed here. 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-sigs/prow repository. |
k8s-ci-robot
added
the
needs-ok-to-test
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: ruiscosta The full list of commands accepted by this bot can be found here.
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
k8s-ci-robot
added
area/cluster-autoscaler
area/provider/aws
|
/ok-to-test |
k8s-ci-robot
added
ok-to-test
|
@ruiscosta thanks for the PR! You will need to get a CLA signed before we can merge it. I'll leave a few general comments in a review, though. |
There was a problem hiding this comment.
These sorts of changes are very hard to reason about in isolation, and while I think the approach makes sense, it's unclear to me 100% that this will resolve the issues mentioned in #6128. I think at the very least we need some test coverage for this case that "fails before" your change and "passes afterwards". If possible, I'd also like to see some tests from a real cluster that show that this resolves the unwanted behaviour? It seems like from the comment thread that there is a reasonable set of steps to reproduce the issue.
|
/easycla |
k8s-ci-robot
added
cncf-cla: yes
This commit improves the `DecreaseTargetSize` function in the AWS cloud provider module of the cluster-autoscaler by refining the logic and enhancing the test coverage to ensure the changes effectively address the issues identified in kubernetes#6128. The updates include: - Expanded test coverage: Added new unit tests that demonstrate the failure before the changes and success after, verifying that the updates mitigate the issue effectively. These tests focus on ensuring that placeholder nodes are correctly filtered out and do not influence the scaling decisions, which was a critical concern. - Code refinements: Refined the handling of instance statuses by adjusting how filtered nodes are used directly in subsequent logic, removing the need to re-assign to the original `nodes` variable. This change simplifies the code and reduces the risk of errors during node filtering processes. - Comment optimization: Removed redundant comments and refined existing ones. Reference to discussion in issue kubernetes#6128 for more details.
k8s-ci-robot
added
cncf-cla: no
|
Keywords which can automatically close issues and at(@) or hashtag(#) mentions are not allowed in commit messages. The list of commits with invalid commit messages:
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-sigs/prow repository. I understand the commands that are listed here. |
|
Keywords which can automatically close issues and at(@) mentions are not allowed in the title of a Pull Request. You can edit the title by writing /retitle in a comment.
When GitHub merges a Pull Request, the title is included in the merge commit. To avoid invalid keywords in the merge commit, please edit the title of the PR.
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-sigs/prow repository. I understand the commands that are listed here. |
|
Closed due to duplicate created to address feedback - #6866 |
This commit resolves an issue where the DecreaseTargetSize function could enter a deadlock state by attempting to scale down when only placeholder nodes, marked as 'placeholderUnfulfillableStatus', are present. The updated function now filters out these placeholder nodes before calculating if a decrease in target size is permissible, ensuring that only operational nodes are considered in the scaling process. This prevents erroneous scaling activities that could impact cluster stability and node management in AWS Auto Scaling Groups.
Added checks to exclude placeholders in DecreaseTargetSize calculations.
Enhanced logging for better clarity when instance statuses are fetched but continue despite errors.
Fixes #6128
What type of PR is this?
/kind bug
What this PR does / why we need it:
This PR fixes a critical deadlock issue in the DecreaseTargetSize function within the AWS cloud provider implementation of the Kubernetes Cluster Autoscaler. By ensuring only operational nodes are considered for scaling down, it prevents the Cluster Autoscaler from making incorrect scaling decisions that could affect cluster stability.
Which issue(s) this PR fixes:
Fixes #6128
Special notes for your reviewer:
Please review the changes to the DecreaseTargetSize function, especially the new checks for placeholder nodes and the updated error handling logic.
Does this PR introduce a user-facing change?