Update DecreaseTargetSize to Exclude Placeholders #6866
Conversation
This commit updates the DecreaseTargetSize function in the AWS cloud provider module of the cluster-autoscaler to address a potential deadlock scenario. The function was modified to filter out placeholder nodes marked as 'placeholderUnfulfillableStatus' before determining if a decrease in target size is allowable. This change ensures that the scaling process considers only operational nodes, which enhances the stability and manageability of AWS Auto Scaling Groups. - Added checks to specifically exclude placeholders in the DecreaseTargetSize calculations. - Enhanced the logging to provide better clarity when instance statuses are fetched, continuing the process despite potential errors. Refer to discussion in issue 6128 for more details on the scenarios and reproduction steps that guided these enhancements.
k8s-ci-robot
added
kind/bug
|
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
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
|
xref: older PR #6817 |
| @@ -739,3 +740,44 @@ func TestHasInstance(t *testing.T) { | |||
| assert.NoError(t, err) | |||
| assert.False(t, present) | |||
| } | |||
|
|
|||
| // MockNodeGroup simulates the behavior of a NodeGroup for testing. | |||
| type MockNodeGroup struct { | |||
There was a problem hiding this comment.
Can we use testify.mock here instead of rolling a new mock interface by hand? There are some other examples in this package, e.g., the autoScalingMock defined in aws_wrapper_test.go.
| } | ||
|
|
||
| // TestDecreaseTargetSizeWithMock verifies functionality and error handling. | ||
| func TestDecreaseTargetSizeWithMock(t *testing.T) { |
There was a problem hiding this comment.
I think we need a test to show that, if the difference between the current size and the new size is (say) -10, and 4 of those nodes are "real" and the other 6 are placeholders, that we only delete the placeholders. Right now it looks like we're just testing a decrease of -1 node.
| } | ||
|
|
||
| // TestDecreaseTargetSizeWithInvalidInput checks the method's response to invalid input. | ||
| func TestDecreaseTargetSizeWithInvalidInput(t *testing.T) { |
There was a problem hiding this comment.
We probably want to use a table-driven test here instead of individual test cases.
This commit updates the DecreaseTargetSize function in the AWS cloud provider module of the cluster-autoscaler to address a potential deadlock scenario. The function was modified to filter out placeholder nodes marked as 'placeholderUnfulfillableStatus' before determining if a decrease in target size is allowable. This change ensures that the scaling process considers only operational nodes, which enhances the stability and manageability of AWS Auto Scaling Groups.
For more details on the scenarios and reproduction steps that guided these enhancements, please refer to the discussion in issue #6128.
What type of PR is this?
/kind bug
What this PR does / why we need it:
This PR is crucial for improving the reliability and efficiency of node scaling operations within the AWS cloud provider for the Kubernetes cluster-autoscaler. By ensuring that placeholder nodes do not influence scaling decisions, it prevents potential deadlocks and enhances the manageability of clusters, particularly in complex environments.
Which issue(s) this PR refers to:
For further discussion and a detailed exploration of the problem, see issue #6128.