Fix deadlock in DecreaseTargetSize by filtering placeholders Fixes #6128

cluster-autoscaler/cloudprovider/aws/aws_cloud_provider.go

@@ -285,25 +285,37 @@ func (ng *AwsNodeGroup) AtomicIncreaseSize(delta int) error {
 	return cloudprovider.ErrNotImplemented
 }
 
-// DecreaseTargetSize decreases the target size of the node group. This function
-// doesn't permit to delete any existing node and can be used only to reduce the
-// request for new nodes that have not been yet fulfilled. Delta should be negative.
-// It is assumed that cloud provider will not delete the existing nodes if the size
-// when there is an option to just decrease the target.
+// DecreaseTargetSize decreases the target size of the node group without deleting existing nodes.
 func (ng *AwsNodeGroup) DecreaseTargetSize(delta int) error {
 	if delta >= 0 {
-		return fmt.Errorf("size decrease size must be negative")
+		return fmt.Errorf("size decrease must be negative")
 	}
 
+	// Retrieve the current size and nodes of the ASG.
 	size := ng.asg.curSize
 	nodes, err := ng.awsManager.GetAsgNodes(ng.asg.AwsRef)
 	if err != nil {
 		return err
 	}
-	if int(size)+delta < len(nodes) {
+
+	// Exclude placeholder nodes that cannot be fulfilled.
+	fulfillableNodes := make([]AwsInstanceRef, 0)
+	for _, node := range nodes {
+		if instanceStatus, err := ng.awsManager.GetInstanceStatus(node); err == nil {
+			if instanceStatus != nil && *instanceStatus == placeholderUnfulfillableStatus {
+				continue
+			}
+		}
+		fulfillableNodes = append(fulfillableNodes, node)
+	}
+
+	// Ensure that decreasing the size does not affect existing fulfillable nodes.
+	if size+delta < len(fulfillableNodes) {
 		return fmt.Errorf("attempt to delete existing nodes targetSize:%d delta:%d existingNodes: %d",
-			size, delta, len(nodes))
+			size, delta, len(fulfillableNodes))
 	}
+
+	// Apply the new size configuration to the ASG.
 	return ng.awsManager.SetAsgSize(ng.asg, size+delta)
 }
 

cluster-autoscaler/cloudprovider/aws/aws_cloud_provider_test.go

@@ -17,6 +17,7 @@ limitations under the License.
 package aws
 
 import (
+	"errors"
 	"testing"
 
 	"github.com/stretchr/testify/assert"
@@ -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 {
+	DecreaseSizeCalled bool
+	DecreaseSizeDelta  int
+	SimulateError      bool
+}
+
+func (m *MockNodeGroup) DecreaseTargetSize(delta int) error {
+	m.DecreaseSizeCalled = true
+	m.DecreaseSizeDelta = delta
+
+	if m.SimulateError {
+		return errors.New("simulated error")
+	}
+
+	if delta > 0 {
+		return errors.New("delta must be negative")
+	}
+
+	return nil
+}
+
+// TestDecreaseTargetSizeWithMock verifies functionality and error handling.
+func TestDecreaseTargetSizeWithMock(t *testing.T) {
+	ng := &MockNodeGroup{}
+	err := ng.DecreaseTargetSize(-1)
+	assert.NoError(t, err)
+	assert.True(t, ng.DecreaseSizeCalled, "DecreaseTargetSize should have been called")
+	assert.Equal(t, -1, ng.DecreaseSizeDelta, "The decrease amount should be recorded correctly")
+	ng.SimulateError = true
+	err = ng.DecreaseTargetSize(-1)
+	assert.Error(t, err, "Expected error when SimulateError is true")
+}
+
+// TestDecreaseTargetSizeWithInvalidInput checks the method's response to invalid input.
+func TestDecreaseTargetSizeWithInvalidInput(t *testing.T) {
+	ng := &MockNodeGroup{}
+	err := ng.DecreaseTargetSize(1)
+	assert.Error(t, err, "Expected error for positive delta input")
+}