Remove deadlock when deploying PCS with ComputeDomain (#215)

unmarshall · web-flow · commit 964077903578 · 2025-10-08T10:11:42.000+05:30
* Check scheduled pods against minAvailable for SchedulingGate removal
  for pods belonging to scaled pod gangs. Previously we used to check
  for ready pods. This is required to remove deadlock when using
  computing domain (DRA).
* Update docstring for PodCliqueScalingGroupConfig.MinAvailable
* Regenerated the api docs and crds

Signed-off-by: Madhav Bhargava &lt;madhav.bhargava@sap.com&gt;

---------

Signed-off-by: Madhav Bhargava &lt;madhav.bhargava@sap.com&gt;
diff --git a/docs/api-reference/operator-api.md b/docs/api-reference/operator-api.md
@@ -233,7 +233,7 @@ _Appears in:_
 | `name` _string_ | Name is the name of the PodCliqueScalingGroupConfig. This should be unique within the PodCliqueSet.<br />It allows consumers to give a semantic name to a group of PodCliques that needs to be scaled together. |  |  |
 | `cliqueNames` _string array_ | CliqueNames is the list of names of the PodClique's that are part of the scaling group. |  |  |
 | `replicas` _integer_ | Replicas is the desired number of replicas for the scaling group at template level.<br />This allows one to control the replicas of the scaling group at startup.<br />If not specified, it defaults to 1. | 1 |  |
-| `minAvailable` _integer_ | MinAvailable specifies the minimum number of ready replicas required for a PodCliqueScalingGroup to be considered operational.<br />A PodCliqueScalingGroup replica is considered "ready" when its associated PodCliques have sufficient ready or starting pods.<br />If MinAvailable is breached, it will be used to signal that the PodCliqueScalingGroup is no longer operating with the desired availability.<br />MinAvailable cannot be greater than Replicas. If ScaleConfig is defined then its MinAvailable should not be less than ScaleConfig.MinReplicas.<br />If not specified, it defaults to 1. | 1 |  |
+| `minAvailable` _integer_ | MinAvailable serves two purposes:<br />Gang Scheduling:<br />It defines the minimum number of replicas that are guaranteed to be gang scheduled.<br />Gang Termination:<br />It defines the minimum requirement of available replicas for a PodCliqueScalingGroup.<br />Violation of this threshold for a duration beyond TerminationDelay will result in termination of the PodCliqueSet replica that it belongs to.<br />Default: If not specified, it defaults to 1.<br />Constraints:<br />MinAvailable cannot be greater than Replicas.<br />If ScaleConfig is defined then its MinAvailable should not be less than ScaleConfig.MinReplicas. | 1 |  |
 | `scaleConfig` _[AutoScalingConfig](#autoscalingconfig)_ | ScaleConfig is the horizontal pod autoscaler configuration for the pod clique scaling group. |  |  |
 
 
diff --git a/operator/api/core/v1alpha1/crds/grove.io_podcliquesets.yaml b/operator/api/core/v1alpha1/crds/grove.io_podcliquesets.yaml
@@ -9071,11 +9071,16 @@ spec:
                         minAvailable:
                           default: 1
                           description: |-
-                            MinAvailable specifies the minimum number of ready replicas required for a PodCliqueScalingGroup to be considered operational.
-                            A PodCliqueScalingGroup replica is considered "ready" when its associated PodCliques have sufficient ready or starting pods.
-                            If MinAvailable is breached, it will be used to signal that the PodCliqueScalingGroup is no longer operating with the desired availability.
-                            MinAvailable cannot be greater than Replicas. If ScaleConfig is defined then its MinAvailable should not be less than ScaleConfig.MinReplicas.
-                            If not specified, it defaults to 1.
+                            MinAvailable serves two purposes:
+                            Gang Scheduling:
+                            It defines the minimum number of replicas that are guaranteed to be gang scheduled.
+                            Gang Termination:
+                            It defines the minimum requirement of available replicas for a PodCliqueScalingGroup.
+                            Violation of this threshold for a duration beyond TerminationDelay will result in termination of the PodCliqueSet replica that it belongs to.
+                            Default: If not specified, it defaults to 1.
+                            Constraints:
+                            MinAvailable cannot be greater than Replicas.
+                            If ScaleConfig is defined then its MinAvailable should not be less than ScaleConfig.MinReplicas.
                           format: int32
                           type: integer
                         name:
diff --git a/operator/api/core/v1alpha1/podcliqueset.go b/operator/api/core/v1alpha1/podcliqueset.go
@@ -216,11 +216,16 @@ type PodCliqueScalingGroupConfig struct {
 	// +optional
 	// +kubebuilder:default=1
 	Replicas *int32 `json:"replicas,omitempty"`
-	// MinAvailable specifies the minimum number of ready replicas required for a PodCliqueScalingGroup to be considered operational.
-	// A PodCliqueScalingGroup replica is considered "ready" when its associated PodCliques have sufficient ready or starting pods.
-	// If MinAvailable is breached, it will be used to signal that the PodCliqueScalingGroup is no longer operating with the desired availability.
-	// MinAvailable cannot be greater than Replicas. If ScaleConfig is defined then its MinAvailable should not be less than ScaleConfig.MinReplicas.
-	// If not specified, it defaults to 1.
+	// MinAvailable serves two purposes:
+	// Gang Scheduling:
+	// It defines the minimum number of replicas that are guaranteed to be gang scheduled.
+	// Gang Termination:
+	// It defines the minimum requirement of available replicas for a PodCliqueScalingGroup.
+	// Violation of this threshold for a duration beyond TerminationDelay will result in termination of the PodCliqueSet replica that it belongs to.
+	// Default: If not specified, it defaults to 1.
+	// Constraints:
+	// MinAvailable cannot be greater than Replicas.
+	// If ScaleConfig is defined then its MinAvailable should not be less than ScaleConfig.MinReplicas.
 	// +optional
 	// +kubebuilder:default=1
 	MinAvailable *int32 `json:"minAvailable,omitempty"`
diff --git a/operator/internal/controller/podclique/components/pod/syncflow.go b/operator/internal/controller/podclique/components/pod/syncflow.go
@@ -240,15 +240,15 @@ func (r _resource) checkAndRemovePodSchedulingGates(sc *syncContext, logger logr
 	tasks := make([]utils.Task, 0, len(sc.existingPCLQPods))
 	skippedScheduleGatedPods := make([]string, 0, len(sc.existingPCLQPods))
 
-	// Pre-compute base PodGang readiness once for all pods in this PodClique
+	// Pre-compute if the base PodGang is scheduled once for all pods in this PodClique
 	// All pods in the same PodClique have the same base PodGang
-	basePodGangReady, basePodGangName, err := r.checkBasePodGangReadinessForPodClique(sc.ctx, logger, sc.pclq)
+	basePodGangScheduled, basePodGangName, err := r.checkBasePodGangScheduledForPodClique(sc.ctx, logger, sc.pclq)
 	if err != nil {
-		logger.Error(err, "Error checking base PodGang readiness for PodClique - will requeue")
+		logger.Error(err, "Error checking if base PodGang is scheduled for PodClique - will requeue")
 		return nil, groveerr.WrapError(err,
 			errCodeRemovePodSchedulingGate,
 			component.OperationSync,
-			"failed to check base PodGang readiness for PodClique",
+			"failed to check if base PodGang is scheduled for PodClique",
 		)
 	}
 
@@ -260,7 +260,7 @@ func (r _resource) checkAndRemovePodSchedulingGates(sc *syncContext, logger logr
 				skippedScheduleGatedPods = append(skippedScheduleGatedPods, p.Name)
 				continue
 			}
-			shouldSkip := r.shouldSkipPodSchedulingGateRemoval(logger, p, basePodGangReady, basePodGangName)
+			shouldSkip := r.shouldSkipPodSchedulingGateRemoval(logger, p, basePodGangScheduled, basePodGangName)
 			if shouldSkip {
 				skippedScheduleGatedPods = append(skippedScheduleGatedPods, p.Name)
 				continue
@@ -297,10 +297,10 @@ func (r _resource) checkAndRemovePodSchedulingGates(sc *syncContext, logger logr
 	return skippedScheduleGatedPods, nil
 }
 
-// isBasePodGangReady checks if the base PodGang (identified by name) is ready, returning errors for API failures.
-// A base PodGang is considered "ready" when ALL of its constituent PodCliques have achieved
-// their minimum required number of ready pods (PodClique.Status.ReadyReplicas >= PodGroup.MinReplicas).
-func (r _resource) isBasePodGangReady(ctx context.Context, logger logr.Logger, namespace, basePodGangName string) (bool, error) {
+// isBasePodGangScheduled checks if the base PodGang (identified by name) is scheduled, returning errors for API failures.
+// A base PodGang is considered "scheduled" when ALL of its constituent PodCliques have achieved
+// their minimum required number of scheduled pods (PodClique.Status.ScheduledReplicas >= PodGroup.MinReplicas).
+func (r _resource) isBasePodGangScheduled(ctx context.Context, logger logr.Logger, namespace, basePodGangName string) (bool, error) {
 	// Get the base PodGang - treat all errors (including NotFound) as requeue-able
 	basePodGang, err := componentutils.GetPodGang(ctx, r.client, basePodGangName, namespace)
 	if err != nil {
@@ -315,11 +315,9 @@ func (r _resource) isBasePodGangReady(ctx context.Context, logger logr.Logger, n
 	// Each PodGroup represents a PodClique within the base PodGang and must meet its MinReplicas requirement
 	for _, podGroup := range basePodGang.Spec.PodGroups {
 		pclqName := podGroup.Name
-
-		// Get the PodClique
 		pclq := &grovecorev1alpha1.PodClique{}
 		pclqKey := client.ObjectKey{Name: pclqName, Namespace: namespace}
-		if err := r.client.Get(ctx, pclqKey, pclq); err != nil {
+		if err = r.client.Get(ctx, pclqKey, pclq); err != nil {
 			// All errors (including NotFound) should trigger requeue for reliable retry
 			// This ensures PodClique exists before we evaluate base PodGang readiness
 			return false, groveerr.WrapError(err,
@@ -329,13 +327,11 @@ func (r _resource) isBasePodGangReady(ctx context.Context, logger logr.Logger, n
 			)
 		}
 
-		// CRITICAL READINESS CHECK: Compare actual ready pods vs required minimum
-		// If ANY PodClique in the base PodGang fails this check, the entire base is considered not ready
-		if pclq.Status.ReadyReplicas < podGroup.MinReplicas {
-			logger.Info("Base PodGang not ready: PodClique has insufficient ready replicas",
+		if pclq.Status.ScheduledReplicas < podGroup.MinReplicas {
+			logger.Info("Base PodGang not scheduled: PodClique has insufficient scheduled replicas",
 				"basePodGangName", basePodGangName,
 				"pclqName", pclqName,
-				"readyReplicas", pclq.Status.ReadyReplicas,
+				"scheduledReplicas", pclq.Status.ScheduledReplicas,
 				"minReplicas", podGroup.MinReplicas)
 			return false, nil // Not ready, but no error - legitimate state
 		}
@@ -345,22 +341,22 @@ func (r _resource) isBasePodGangReady(ctx context.Context, logger logr.Logger, n
 	return true, nil
 }
 
-// checkBasePodGangReadinessForPodClique determines if there's a base PodGang that needs to be checked
-// for readiness, and if so, performs that check once for the entire PodClique.
-func (r _resource) checkBasePodGangReadinessForPodClique(ctx context.Context, logger logr.Logger, pclq *grovecorev1alpha1.PodClique) (bool, string, error) {
+// checkBasePodGangScheduledForPodClique determines if there's a base PodGang for the PodClique. If there is one,
+// this function checks if it is scheduled.
+func (r _resource) checkBasePodGangScheduledForPodClique(ctx context.Context, logger logr.Logger, pclq *grovecorev1alpha1.PodClique) (bool, string, error) {
 	// Check if this PodClique has a base PodGang dependency
 	basePodGangName, hasBasePodGangLabel := pclq.GetLabels()[common.LabelBasePodGang]
 	if !hasBasePodGangLabel {
 		// This PodClique is a base PodGang itself - no dependency
 		return true, "", nil
 	}
 
-	ready, err := r.isBasePodGangReady(ctx, logger, pclq.Namespace, basePodGangName)
+	scheduled, err := r.isBasePodGangScheduled(ctx, logger, pclq.Namespace, basePodGangName)
 	if err != nil {
 		return false, basePodGangName, err
 	}
 
-	return ready, basePodGangName, nil
+	return scheduled, basePodGangName, nil
 }
 
 // shouldSkipPodSchedulingGateRemoval implements the core PodGang scheduling gate logic.
diff --git a/operator/internal/controller/podclique/components/pod/syncflow_test.go b/operator/internal/controller/podclique/components/pod/syncflow_test.go
@@ -194,7 +194,7 @@ func TestCheckAndRemovePodSchedulingGates_MinAvailableAware(t *testing.T) {
 			}
 
 			// For scaled PodGang tests, add the base PodGang label to the PodClique
-			// This is what the production code expects to read in checkBasePodGangReadinessForPodClique
+			// This is what the production code expects to read in checkBasePodGangScheduledForPodClique
 			if isScaledPodGangTest {
 				testPclq.Labels = map[string]string{
 					common.LabelBasePodGang: "simple1-0",
@@ -320,54 +320,54 @@ func TestCheckAndRemovePodSchedulingGates_ConcurrentExecution(t *testing.T) {
 	}
 }
 
-func TestIsBasePodGangReady(t *testing.T) {
+func TestIsBasePodGangScheduled(t *testing.T) {
 	tests := []struct {
 		name              string
 		basePodGangExists bool
 		podCliques        []testPodClique
-		expectedReady     bool
+		expectedScheduled bool
 		expectError       bool
 		description       string
 	}{
 		{
-			name:              "Base PodGang ready - all PodCliques meet MinAvailable",
+			name:              "Base PodGang is scheduled - all PodCliques meet MinAvailable",
 			basePodGangExists: true,
 			podCliques: []testPodClique{
-				{name: "simple1-0-pcb", minAvailable: 2, readyReplicas: 2},
-				{name: "simple1-0-pcc", minAvailable: 1, readyReplicas: 3},
+				{name: "simple1-0-pcb", minAvailable: 2, scheduledReplicas: 2},
+				{name: "simple1-0-pcc", minAvailable: 1, scheduledReplicas: 3},
 			},
-			expectedReady: true,
-			expectError:   false,
-			description:   "All PodCliques meet their MinAvailable requirements",
+			expectedScheduled: true,
+			expectError:       false,
+			description:       "All PodCliques meet their MinAvailable requirements",
 		},
 		{
-			name:              "Base PodGang not ready - one PodClique below MinAvailable",
+			name:              "Base PodGang not scheduled - one PodClique below MinAvailable",
 			basePodGangExists: true,
 			podCliques: []testPodClique{
-				{name: "simple1-0-pcb", minAvailable: 2, readyReplicas: 2},
-				{name: "simple1-0-pcc", minAvailable: 3, readyReplicas: 2}, // Below MinAvailable
+				{name: "simple1-0-pcb", minAvailable: 2, scheduledReplicas: 2},
+				{name: "simple1-0-pcc", minAvailable: 3, scheduledReplicas: 2}, // Below MinAvailable
 			},
-			expectedReady: false,
-			expectError:   false,
-			description:   "One PodClique below MinAvailable makes base PodGang not ready",
+			expectedScheduled: false,
+			expectError:       false,
+			description:       "One PodClique below MinAvailable makes base PodGang not scheduled",
 		},
 		{
 			name:              "Base PodGang missing",
 			basePodGangExists: false,
 			podCliques:        []testPodClique{},
-			expectedReady:     false,
+			expectedScheduled: false,
 			expectError:       true,
 			description:       "Missing base PodGang should return error for requeue",
 		},
 		{
 			name:              "Base PodGang ready - single PodClique",
 			basePodGangExists: true,
 			podCliques: []testPodClique{
-				{name: "simple1-0-pcb", minAvailable: 1, readyReplicas: 1},
+				{name: "simple1-0-pcb", minAvailable: 1, scheduledReplicas: 1},
 			},
-			expectedReady: true,
-			expectError:   false,
-			description:   "Single PodClique meeting MinAvailable",
+			expectedScheduled: true,
+			expectError:       false,
+			description:       "Single PodClique meeting MinAvailable",
 		},
 	}
 
@@ -402,7 +402,7 @@ func TestIsBasePodGangReady(t *testing.T) {
 
 				// Add corresponding PodCliques
 				for _, pclq := range tt.podCliques {
-					podClique := createTestPodClique(pclq.name, pclq.minAvailable, pclq.readyReplicas)
+					podClique := createTestPodClique(pclq.name, pclq.minAvailable, pclq.scheduledReplicas)
 					objects = append(objects, podClique)
 				}
 			}
@@ -415,15 +415,15 @@ func TestIsBasePodGangReady(t *testing.T) {
 
 			// Test the readiness check
 			r := &_resource{client: fakeClient}
-			result, err := r.isBasePodGangReady(context.Background(), logr.Discard(), "default", "simple1-0")
+			result, err := r.isBasePodGangScheduled(context.Background(), logr.Discard(), "default", "simple1-0")
 
 			if tt.expectError {
 				require.Error(t, err, "Expected error for test case: %s", tt.name)
-				// When error occurs, result should be false and we don't check expectedReady
+				// When error occurs, result should be false and we don't check expectedScheduled
 				assert.False(t, result, "Result should be false when error occurs")
 			} else {
 				require.NoError(t, err, "Unexpected error for test case: %s", tt.name)
-				assert.Equal(t, tt.expectedReady, result, tt.description)
+				assert.Equal(t, tt.expectedScheduled, result, tt.description)
 			}
 		})
 	}
@@ -432,9 +432,9 @@ func TestIsBasePodGangReady(t *testing.T) {
 // Test helper types and functions
 
 type testPodClique struct {
-	name          string
-	minAvailable  int32
-	readyReplicas int32
+	name              string
+	minAvailable      int32
+	scheduledReplicas int32
 }
 
 func createTestPod(podGangName string, hasGate bool, inPodGang bool) *corev1.Pod {
@@ -548,7 +548,7 @@ func createTestBasePodGangWithPodCliques(podCliques []testPodClique) *grovesched
 	}
 }
 
-func createTestPodClique(name string, minAvailable, readyReplicas int32) *grovecorev1alpha1.PodClique {
+func createTestPodClique(name string, minAvailable, scheduledReplicas int32) *grovecorev1alpha1.PodClique {
 	return &grovecorev1alpha1.PodClique{
 		ObjectMeta: metav1.ObjectMeta{
 			Name:      name,
@@ -558,7 +558,7 @@ func createTestPodClique(name string, minAvailable, readyReplicas int32) *grovec
 			MinAvailable: ptr.To(minAvailable),
 		},
 		Status: grovecorev1alpha1.PodCliqueStatus{
-			ReadyReplicas: readyReplicas,
+			ScheduledReplicas: scheduledReplicas,
 		},
 	}
 }
