OCPBUGS-62627: cluster operator ingress reported Progressing=True with reason=Reconciling for a node reboot

Signed-off-by: Davide Salerno <[email protected]>

Author: davidesalerno

pkg/operator/controller/ingress/status.go

@@ -7,6 +7,7 @@ import (
 	"errors"
 	"fmt"
 	"reflect"
+	"slices"
 	"sort"
 	"strings"
 	"time"
@@ -47,6 +48,33 @@ type expectedCondition struct {
 	// or else the condition is not checked.
 	ifConditionsTrue []string
 	gracePeriod      time.Duration
+	// ignoreReasons specifies condition reasons that should be ignored when
+	// determining if the expected condition is met. This allows distinguishing
+	// between significant state changes (configuration rollouts) and transient
+	// infrastructure events (node reboots, pod restarts).
+	ignoreReasons []string
+}
+
+// DeploymentRollingOutConditionReason enum to map the different reasons a deployment could be in rolling out condition
+// since several functions are using this condition's reason for setting the Progressing condition to true or false
+type DeploymentRollingOutConditionReason int
+
+const (
+	DeploymentRollingOut DeploymentRollingOutConditionReason = iota
+	DeploymentNotRollingOut
+	PodsStarting
+	ReplicasStabilizing
+)
+
+var deploymentRollingOutConditionReason = map[DeploymentRollingOutConditionReason]string{
+	DeploymentRollingOut:    "DeploymentRollingOut",
+	DeploymentNotRollingOut: "DeploymentNotRollingOut",
+	PodsStarting:            "PodsStarting",
+	ReplicasStabilizing:     "ReplicasStabilizing",
+}
+
+func (ss DeploymentRollingOutConditionReason) String() string {
+	return deploymentRollingOutConditionReason[ss]
 }
 
 // syncIngressControllerStatus computes the current status of ic and
@@ -330,7 +358,7 @@ func checkConditions(expectedConds []expectedCondition, conditions []operatorv1.
 		if !haveCondition {
 			continue
 		}
-		if condition.Status == expected.status {
+		if condition.Status == expected.status || slices.Contains(expected.ignoreReasons, condition.Reason) {
 			continue
 		}
 		failedPredicates := false
@@ -503,34 +531,49 @@ func computeDeploymentReplicasAllAvailableCondition(deployment *appsv1.Deploymen
 // of expected or available replicas.
 // See Reference: https://github.com/kubernetes/kubectl/blob/master/pkg/polymorphichelpers/rollout_status.go
 func computeDeploymentRollingOutCondition(deployment *appsv1.Deployment) operatorv1.OperatorCondition {
+	// Check if the deployment has the "NewReplicaSetAvailable" condition.
+	// This indicates that Kubernetes has successfully completed a rollout.
+	hasNewReplicaSetAvailable := false
+	for _, cond := range deployment.Status.Conditions {
+		if cond.Type == appsv1.DeploymentProgressing &&
+			cond.Status == corev1.ConditionTrue &&
+			cond.Reason == "NewReplicaSetAvailable" {
+			hasNewReplicaSetAvailable = true
+			break
+		}
+	}
+
 	// If have replicas is less than want replicas, then we are waiting for replicas to be updated.
 	if deployment.Spec.Replicas != nil && deployment.Status.UpdatedReplicas < *deployment.Spec.Replicas {
+		reason := computeDeploymentRollingOutStatusAndReason(hasNewReplicaSetAvailable, deployment)
 		return operatorv1.OperatorCondition{
 			Type:   IngressControllerDeploymentRollingOutConditionType,
 			Status: operatorv1.ConditionTrue,
-			Reason: "DeploymentRollingOut",
+			Reason: reason,
 			Message: fmt.Sprintf(
 				"Waiting for router deployment rollout to finish: %d out of %d new replica(s) have been updated...\n",
 				deployment.Status.UpdatedReplicas, *deployment.Spec.Replicas),
 		}
 	}
 	// If have replicas greater than updated replicas, then we are waiting for old replicas to terminate.
 	if deployment.Status.Replicas > deployment.Status.UpdatedReplicas {
+		reason := computeDeploymentRollingOutStatusAndReason(hasNewReplicaSetAvailable, deployment)
 		return operatorv1.OperatorCondition{
 			Type:   IngressControllerDeploymentRollingOutConditionType,
 			Status: operatorv1.ConditionTrue,
-			Reason: "DeploymentRollingOut",
+			Reason: reason,
 			Message: fmt.Sprintf(
 				"Waiting for router deployment rollout to finish: %d old replica(s) are pending termination...\n",
 				deployment.Status.Replicas-deployment.Status.UpdatedReplicas),
 		}
 	}
 	// If available replicas less than updated replicas, then we are waiting for updated replicas to become available.
 	if deployment.Status.AvailableReplicas < deployment.Status.UpdatedReplicas {
+		reason := computeDeploymentRollingOutStatusAndReason(hasNewReplicaSetAvailable, deployment)
 		return operatorv1.OperatorCondition{
 			Type:   IngressControllerDeploymentRollingOutConditionType,
 			Status: operatorv1.ConditionTrue,
-			Reason: "DeploymentRollingOut",
+			Reason: reason,
 			Message: fmt.Sprintf(
 				"Waiting for router deployment rollout to finish: %d of %d updated replica(s) are available...\n",
 				deployment.Status.AvailableReplicas, deployment.Status.UpdatedReplicas),
@@ -540,11 +583,49 @@ func computeDeploymentRollingOutCondition(deployment *appsv1.Deployment) operato
 	return operatorv1.OperatorCondition{
 		Type:    IngressControllerDeploymentRollingOutConditionType,
 		Status:  operatorv1.ConditionFalse,
-		Reason:  "DeploymentNotRollingOut",
+		Reason:  DeploymentNotRollingOut.String(),
 		Message: "Deployment is not actively rolling out",
 	}
 }
 
+// computeDeploymentRollingOutStatusAndReason computes the ingresscontroller's Status and Reason for DeploymentRollingOut
+// condition type checking if the deployment has the "NewReplicaSetAvailable" condition and the status of the pod replicas
+// and the generation metadata, in order to detect if the deployment is rolling out for am ingresscontroller configuration
+// change or due to an infrastructure change (i.e. node reboot due to update or scale up or down)
+func computeDeploymentRollingOutStatusAndReason(hasNewReplicaSetAvailable bool, deployment *appsv1.Deployment) string {
+	var (
+		wantReplicas      = int32(0)
+		haveReplicas      = deployment.Status.Replicas
+		updatedReplicas   = deployment.Status.UpdatedReplicas
+		availableReplicas = deployment.Status.AvailableReplicas
+		readyReplicas     = deployment.Status.ReadyReplicas
+	)
+
+	if deployment.Spec.Replicas != nil {
+		wantReplicas = *deployment.Spec.Replicas
+	}
+
+	if hasNewReplicaSetAvailable && deployment.Status.ObservedGeneration == deployment.Generation &&
+		updatedReplicas == wantReplicas {
+		if haveReplicas != wantReplicas {
+			return ReplicasStabilizing.String()
+		}
+		if readyReplicas < wantReplicas || availableReplicas < wantReplicas {
+			return PodsStarting.String()
+		}
+	}
+
+	// Additional safety check: Even without NewReplicaSetAvailable, if everything
+	// matches except readiness, treat it as pods starting up
+	if deployment.Status.ObservedGeneration == deployment.Generation &&
+		updatedReplicas == wantReplicas &&
+		haveReplicas == wantReplicas &&
+		(readyReplicas < wantReplicas || availableReplicas < wantReplicas) {
+		return PodsStarting.String()
+	}
+	return DeploymentRollingOut.String()
+}
+
 // computeIngressDegradedCondition computes the ingresscontroller's "Degraded"
 // status condition, which aggregates other status conditions that can indicate
 // a degraded state.  In addition, computeIngressDegradedCondition returns a
@@ -593,12 +674,7 @@ func computeIngressDegradedCondition(conditions []operatorv1.OperatorCondition,
 	// Only check the default ingress controller for the canary
 	// success status condition.
 	if icName == manifests.DefaultIngressControllerName {
-		canaryCond := struct {
-			condition        string
-			status           operatorv1.ConditionStatus
-			ifConditionsTrue []string
-			gracePeriod      time.Duration
-		}{
+		canaryCond := expectedCondition{
 			condition:   IngressControllerCanaryCheckSuccessConditionType,
 			status:      operatorv1.ConditionTrue,
 			gracePeriod: time.Second * 60,
@@ -765,6 +841,12 @@ func computeIngressProgressingCondition(conditions []operatorv1.OperatorConditio
 		{
 			condition: IngressControllerDeploymentRollingOutConditionType,
 			status:    operatorv1.ConditionFalse,
+			// Ignore infrastructure-driven deployment rollouts when computing
+			// the IngressController's Progressing condition.
+			ignoreReasons: []string{
+				"ReplicasStabilizing", // Node reboots, pod evictions
+				"PodsStarting",        // Pods restarting after infrastructure events
+			},
 		},
 	}
 

pkg/operator/controller/ingress/status_test.go

@@ -503,6 +503,9 @@ func Test_computeDeploymentRollingOutCondition(t *testing.T) {
 		replicasAvailable     *int32
 		expectStatus          operatorv1.ConditionStatus
 		expectMessageContains string
+		expectReasonContains  string
+		differentGeneration   bool
+		hasNewReplicaSet      bool
 	}{
 		{
 			name:                  "Router pod replicas not rolling out",
@@ -576,6 +579,42 @@ func Test_computeDeploymentRollingOutCondition(t *testing.T) {
 			replicasAvailable:     pointer.Int32(2),
 			expectMessageContains: "Deployment is not actively rolling out",
 		},
+		{
+			name:                  "Router pods replicas available < updated and generation is equal to observedGeneration",
+			expectStatus:          operatorv1.ConditionTrue,
+			replicasHave:          pointer.Int32(4),
+			replicasWanted:        pointer.Int32(4),
+			replicasUpdated:       pointer.Int32(4),
+			replicasAvailable:     pointer.Int32(1),
+			expectMessageContains: "1 of 4 updated replica(s) are available",
+			expectReasonContains:  PodsStarting.String(),
+			differentGeneration:   false,
+			hasNewReplicaSet:      true,
+		},
+		{
+			name:                  "Router pod replicas have > updated, generation is equal to observedGeneration and NewReplicaSet created",
+			expectStatus:          operatorv1.ConditionTrue,
+			replicasHave:          pointer.Int32(3),
+			replicasWanted:        pointer.Int32(1),
+			replicasUpdated:       pointer.Int32(1),
+			replicasAvailable:     pointer.Int32(1),
+			expectMessageContains: "2 old replica(s) are pending termination",
+			expectReasonContains:  ReplicasStabilizing.String(),
+			differentGeneration:   false,
+			hasNewReplicaSet:      true,
+		},
+		{
+			name:                  "Router pod replicas have/updated < want, generation is equal to observedGeneration and NewReplicaSet created",
+			expectStatus:          operatorv1.ConditionTrue,
+			replicasHave:          pointer.Int32(4),
+			replicasWanted:        pointer.Int32(4),
+			replicasUpdated:       pointer.Int32(4),
+			replicasAvailable:     pointer.Int32(1),
+			expectMessageContains: "Waiting for router deployment rollout to finish: 1 of 4 updated replica(s) are available...\n",
+			expectReasonContains:  PodsStarting.String(),
+			differentGeneration:   false,
+			hasNewReplicaSet:      true,
+		},
 	}
 	for _, test := range tests {
 		t.Run(test.name, func(t *testing.T) {
@@ -589,13 +628,29 @@ func Test_computeDeploymentRollingOutCondition(t *testing.T) {
 					UpdatedReplicas:   *test.replicasUpdated,
 				},
 			}
+			if !test.differentGeneration {
+				routerDeploy.ObjectMeta.Generation = 1
+				routerDeploy.Status.ObservedGeneration = 1
+			}
+			if test.hasNewReplicaSet {
+				routerDeploy.Status.Conditions = []appsv1.DeploymentCondition{
+					{
+						Type:   appsv1.DeploymentProgressing,
+						Status: corev1.ConditionTrue,
+						Reason: "NewReplicaSetAvailable",
+					},
+				}
+			}
 			actual := computeDeploymentRollingOutCondition(routerDeploy)
 			if actual.Status != test.expectStatus {
 				t.Errorf("expected status to be %s, got %s", test.expectStatus, actual.Status)
 			}
 			if len(test.expectMessageContains) != 0 && !strings.Contains(actual.Message, test.expectMessageContains) {
 				t.Errorf("expected message to include %q, got %q", test.expectMessageContains, actual.Message)
 			}
+			if len(test.expectReasonContains) != 0 && !strings.Contains(actual.Reason, test.expectReasonContains) {
+				t.Errorf("expected reason to include %q, got %q", test.expectReasonContains, actual.Reason)
+			}
 		})
 	}
 }
@@ -1701,6 +1756,60 @@ func Test_computeIngressProgressingCondition(t *testing.T) {
 			},
 			expect: operatorv1.OperatorCondition{Type: operatorv1.OperatorStatusTypeProgressing, Status: operatorv1.ConditionTrue},
 		},
+		{
+			description: "load balancer is not progressing, but unmanaged load balancer type and router deployment is rolling out due to node reboot (pods starting)",
+			conditions: []operatorv1.OperatorCondition{
+				{Type: IngressControllerLoadBalancerProgressingConditionType, Status: operatorv1.ConditionFalse},
+				{Type: IngressControllerDeploymentRollingOutConditionType, Status: operatorv1.ConditionTrue, Reason: PodsStarting.String()},
+				{Type: operatorv1.LoadBalancerManagedIngressConditionType, Status: operatorv1.ConditionFalse},
+			},
+			expect: operatorv1.OperatorCondition{Type: operatorv1.OperatorStatusTypeProgressing, Status: operatorv1.ConditionFalse},
+		},
+		{
+			description: "load balancer is progressing, but unmanaged load balancer type and router deployment is rolling out due to node reboot (pods starting)",
+			conditions: []operatorv1.OperatorCondition{
+				{Type: IngressControllerLoadBalancerProgressingConditionType, Status: operatorv1.ConditionTrue},
+				{Type: IngressControllerDeploymentRollingOutConditionType, Status: operatorv1.ConditionTrue, Reason: PodsStarting.String()},
+				{Type: operatorv1.LoadBalancerManagedIngressConditionType, Status: operatorv1.ConditionFalse},
+			},
+			expect: operatorv1.OperatorCondition{Type: operatorv1.OperatorStatusTypeProgressing, Status: operatorv1.ConditionFalse},
+		},
+		{
+			description: "load balancer and unmanaged load balancer are progressing, but router deployment is rolling out due to node reboot (pods starting)",
+			conditions: []operatorv1.OperatorCondition{
+				{Type: IngressControllerLoadBalancerProgressingConditionType, Status: operatorv1.ConditionTrue},
+				{Type: IngressControllerDeploymentRollingOutConditionType, Status: operatorv1.ConditionTrue, Reason: PodsStarting.String()},
+				{Type: operatorv1.LoadBalancerManagedIngressConditionType, Status: operatorv1.ConditionTrue},
+			},
+			expect: operatorv1.OperatorCondition{Type: operatorv1.OperatorStatusTypeProgressing, Status: operatorv1.ConditionTrue},
+		},
+		{
+			description: "load balancer is not progressing, but unmanaged load balancer type and router deployment is rolling out due to node reboot (replicas stabilizing)",
+			conditions: []operatorv1.OperatorCondition{
+				{Type: IngressControllerLoadBalancerProgressingConditionType, Status: operatorv1.ConditionFalse},
+				{Type: IngressControllerDeploymentRollingOutConditionType, Status: operatorv1.ConditionTrue, Reason: ReplicasStabilizing.String()},
+				{Type: operatorv1.LoadBalancerManagedIngressConditionType, Status: operatorv1.ConditionFalse},
+			},
+			expect: operatorv1.OperatorCondition{Type: operatorv1.OperatorStatusTypeProgressing, Status: operatorv1.ConditionFalse},
+		},
+		{
+			description: "load balancer is progressing, but unmanaged load balancer type and router deployment is rolling out due to node reboot (replicas stabilizing)",
+			conditions: []operatorv1.OperatorCondition{
+				{Type: IngressControllerLoadBalancerProgressingConditionType, Status: operatorv1.ConditionTrue},
+				{Type: IngressControllerDeploymentRollingOutConditionType, Status: operatorv1.ConditionTrue, Reason: ReplicasStabilizing.String()},
+				{Type: operatorv1.LoadBalancerManagedIngressConditionType, Status: operatorv1.ConditionFalse},
+			},
+			expect: operatorv1.OperatorCondition{Type: operatorv1.OperatorStatusTypeProgressing, Status: operatorv1.ConditionFalse},
+		},
+		{
+			description: "load balancer and unmanaged load balancer are progressing, but router deployment is rolling out due to node reboot (replicas stabilizing)",
+			conditions: []operatorv1.OperatorCondition{
+				{Type: IngressControllerLoadBalancerProgressingConditionType, Status: operatorv1.ConditionTrue},
+				{Type: IngressControllerDeploymentRollingOutConditionType, Status: operatorv1.ConditionTrue, Reason: ReplicasStabilizing.String()},
+				{Type: operatorv1.LoadBalancerManagedIngressConditionType, Status: operatorv1.ConditionTrue},
+			},
+			expect: operatorv1.OperatorCondition{Type: operatorv1.OperatorStatusTypeProgressing, Status: operatorv1.ConditionTrue},
+		},
 		{
 			description: "load balancer is not progressing and router deployment is rolling out",
 			conditions: []operatorv1.OperatorCondition{

test/e2e/operator_test.go

@@ -78,6 +78,7 @@ var (
 		{Type: operatorv1.IngressControllerAvailableConditionType, Status: operatorv1.ConditionTrue},
 		{Type: operatorv1.LoadBalancerManagedIngressConditionType, Status: operatorv1.ConditionFalse},
 		{Type: operatorv1.DNSManagedIngressConditionType, Status: operatorv1.ConditionFalse},
+		{Type: operatorv1.OperatorStatusTypeProgressing, Status: operatorv1.ConditionFalse},
 	}
 	availableConditionsForIngressControllerWithNodePort = []operatorv1.OperatorCondition{
 		{Type: operatorv1.IngressControllerAvailableConditionType, Status: operatorv1.ConditionTrue},