Try end2end testing the bug

Signed-off-by: Oliver Trosien <oliver.trosien@zalando.de>

Author: otrosien

operator/autoscaler_test.go

@@ -729,6 +729,194 @@ func TestGetManagedNodes(t *testing.T) {
 	require.Equal(t, "1.2.3.4", actual[0].IP)
 }
 
+// TestScalingBoundaryChange verifies that when scaling boundaries change,
+// the autoscaler correctly detects the need to scale and updates spec.replicas.
+// This test validates the fix for the bug where edsReplicas() was prematurely
+// enforcing boundary constraints, causing the autoscaler to skip spec updates.
+//
+// Scenario:
+// - Initial state: spec.replicas = 3, spec.scaling.minReplicas = 2
+// - Boundary change: set spec.scaling.minReplicas = 4
+// - Expected: autoscaler should detect that 3 < 4 and return NodeReplicas = 4
+//
+// With the bug: edsReplicas() would return max(3, 4) = 4, causing the
+// condition (4 != 4) to be false, skipping the spec update.
+//
+// With the fix: edsReplicas() returns 3 (actual value), allowing the
+// condition (4 != 3) to be true, triggering the spec update.
+func TestScalingBoundaryChange(t *testing.T) {
+	// Initial state: cluster running with 3 replicas, minReplicas = 2
+	initialReplicas := int32(3)
+	eds := &zv1.ElasticsearchDataSet{
+		ObjectMeta: metav1.ObjectMeta{
+			Name:      "test-eds",
+			Namespace: "default",
+		},
+		Spec: zv1.ElasticsearchDataSetSpec{
+			Scaling: &zv1.ElasticsearchDataSetScaling{
+				Enabled:                            true,
+				MinReplicas:                        2,
+				MaxReplicas:                        10,
+				MinShardsPerNode:                   1,
+				MaxShardsPerNode:                   10,
+				MinIndexReplicas:                   0,
+				MaxIndexReplicas:                   2,
+				ScaleUpCPUBoundary:                 50,
+				ScaleUpThresholdDurationSeconds:    60,
+				ScaleDownCPUBoundary:               20,
+				ScaleDownThresholdDurationSeconds:  60,
+				DiskUsagePercentScaledownWatermark: 0,
+			},
+			Replicas: &initialReplicas,
+		},
+	}
+
+	// Verify edsReplicas returns the actual spec.replicas value (3), not max(3, 2)
+	actualReplicas := edsReplicas(eds)
+	require.Equal(t, int32(3), actualReplicas, "edsReplicas should return actual spec.replicas value, not max(spec.replicas, minReplicas)")
+
+	// Now simulate a boundary change: increase minReplicas to 4
+	eds.Spec.Scaling.MinReplicas = 4
+
+	// Create some test indices to trigger scaling logic
+	esIndices := map[string]ESIndex{
+		"test-index": {
+			Index:     "test-index",
+			Replicas:  1,
+			Primaries: 2,
+		},
+	}
+	esNodes := []ESNode{}
+
+	as := systemUnderTest(eds, nil, nil)
+
+	// Call calculateScalingOperation with UP hint
+	scalingOperation := as.calculateScalingOperation(esIndices, esNodes, UP)
+
+	// Verify that the autoscaler correctly identifies scaling is needed
+	require.NotNil(t, scalingOperation.NodeReplicas, "NodeReplicas should not be nil when scaling is needed")
+	require.Equal(t, int32(4), *scalingOperation.NodeReplicas, "Should scale to minReplicas (4)")
+	require.Equal(t, UP, scalingOperation.ScalingDirection, "Should indicate scale up direction")
+
+	// The key assertion: with the fix, edsReplicas() still returns 3 (not 4)
+	// This allows the comparison at operator/elasticsearch.go:945 to work correctly:
+	// if *scalingOperation.NodeReplicas != currentReplicas (4 != 3 = true)
+	currentReplicasAfter := edsReplicas(eds)
+	require.Equal(t, int32(3), currentReplicasAfter, "edsReplicas should still return actual spec.replicas (3), allowing autoscaler to detect change")
+	require.NotEqual(t, *scalingOperation.NodeReplicas, currentReplicasAfter, "Target replicas (4) should differ from current replicas (3), triggering spec update")
+}
+
+// TestScalingBoundaryChangeScaleDown verifies the same behavior for scale-down scenarios.
+// When maxReplicas is decreased below current replicas, the autoscaler should detect
+// the need to scale down based on shard distribution and boundary constraints.
+func TestScalingBoundaryChangeScaleDown(t *testing.T) {
+	// Initial state: cluster running with 6 replicas, maxReplicas = 10
+	// We have enough shards to justify 6 nodes initially
+	initialReplicas := int32(6)
+	eds := &zv1.ElasticsearchDataSet{
+		ObjectMeta: metav1.ObjectMeta{
+			Name:      "test-eds",
+			Namespace: "default",
+		},
+		Spec: zv1.ElasticsearchDataSetSpec{
+			Scaling: &zv1.ElasticsearchDataSetScaling{
+				Enabled:                            true,
+				MinReplicas:                        1,
+				MaxReplicas:                        10,
+				MinShardsPerNode:                   2,
+				MaxShardsPerNode:                   4,
+				MinIndexReplicas:                   1,
+				MaxIndexReplicas:                   2,
+				ScaleUpCPUBoundary:                 50,
+				ScaleUpThresholdDurationSeconds:    60,
+				ScaleDownCPUBoundary:               20,
+				ScaleDownThresholdDurationSeconds:  60,
+				DiskUsagePercentScaledownWatermark: 0,
+			},
+			Replicas: &initialReplicas,
+		},
+	}
+
+	// Verify edsReplicas returns the actual spec.replicas value (6)
+	actualReplicas := edsReplicas(eds)
+	require.Equal(t, int32(6), actualReplicas, "edsReplicas should return actual spec.replicas value")
+
+	// Now simulate a boundary change: decrease maxReplicas to 3
+	eds.Spec.Scaling.MaxReplicas = 3
+
+	// Create test indices: 2 indices with 2 primaries each and 2 replicas
+	// Total shards: 2 * 2 * (2+1) = 12 shards
+	// With MaxShardsPerNode=4, we need at least 3 nodes
+	// But after scale-down, we'll reduce to 1 replica: 2 * 2 * (1+1) = 8 shards
+	// With MaxShardsPerNode=4, we need 2 nodes, which fits within maxReplicas=3
+	esIndices := map[string]ESIndex{
+		"test-index-1": {
+			Index:     "test-index-1",
+			Replicas:  2,
+			Primaries: 2,
+		},
+		"test-index-2": {
+			Index:     "test-index-2",
+			Replicas:  2,
+			Primaries: 2,
+		},
+	}
+	esNodes := []ESNode{}
+
+	as := systemUnderTest(eds, nil, nil)
+
+	// Call calculateScalingOperation with DOWN hint
+	scalingOperation := as.calculateScalingOperation(esIndices, esNodes, DOWN)
+
+	// Verify that the autoscaler correctly identifies scaling down is needed
+	require.NotNil(t, scalingOperation.NodeReplicas, "NodeReplicas should not be nil when scaling is needed")
+	// The autoscaler will want to scale down index replicas from 2 to 1
+	// and adjust node count accordingly, bounded by maxReplicas=3
+	require.LessOrEqual(t, *scalingOperation.NodeReplicas, int32(3), "Should not exceed maxReplicas (3)")
+	require.Equal(t, DOWN, scalingOperation.ScalingDirection, "Should indicate scale down direction")
+
+	// Verify edsReplicas() still returns the actual value, not the bounded value
+	currentReplicasAfter := edsReplicas(eds)
+	require.Equal(t, int32(6), currentReplicasAfter, "edsReplicas should still return actual spec.replicas (6)")
+	require.NotEqual(t, *scalingOperation.NodeReplicas, currentReplicasAfter, "Target replicas should differ from current replicas (6), triggering spec update")
+}
+
+// TestInitialScalingWithNilReplicas verifies that when spec.replicas is nil
+// and autoscaling is enabled, edsReplicas() returns 0, allowing the autoscaler
+// to make an explicit initial scaling decision.
+func TestInitialScalingWithNilReplicas(t *testing.T) {
+	eds := &zv1.ElasticsearchDataSet{
+		ObjectMeta: metav1.ObjectMeta{
+			Name:      "test-eds",
+			Namespace: "default",
+		},
+		Spec: zv1.ElasticsearchDataSetSpec{
+			Scaling: &zv1.ElasticsearchDataSetScaling{
+				Enabled:                            true,
+				MinReplicas:                        3,
+				MaxReplicas:                        10,
+				MinShardsPerNode:                   1,
+				MaxShardsPerNode:                   10,
+				MinIndexReplicas:                   0,
+				MaxIndexReplicas:                   2,
+				ScaleUpCPUBoundary:                 50,
+				ScaleUpThresholdDurationSeconds:    60,
+				ScaleDownCPUBoundary:               20,
+				ScaleDownThresholdDurationSeconds:  60,
+				DiskUsagePercentScaledownWatermark: 0,
+			},
+			Replicas: nil, // Not yet initialized
+		},
+	}
+
+	// With the fix: should return 0 when replicas is nil and autoscaling is enabled
+	actualReplicas := edsReplicas(eds)
+	require.Equal(t, int32(0), actualReplicas, "edsReplicas should return 0 when spec.replicas is nil and autoscaling is enabled")
+
+	// This allows the autoscaler to initialize spec.replicas properly
+	// The autoscaler will see currentReplicas = 0 and can scale to minReplicas
+}
+
 func systemUnderTest(eds *zv1.ElasticsearchDataSet, metricSet *zv1.ElasticsearchMetricSet, pods []v1.Pod) *AutoScaler {
 	es := &ESResource{
 		ElasticsearchDataSet: eds,