Address code review feedback for observability controller

1. Fix CSV readiness check to handle multiple CSVs during upgrades
   - Check all matching CSVs instead of just the first one
   - Prioritize CSVs in "Succeeded" state to avoid checking old versions
   - Continue checking if status not found instead of failing

2. Return error when markNetworkObservabilityDeployed fails
   - Previously only logged the error, causing reconciliation to appear successful
   - Now sets Degraded status and returns error to trigger retry
   - Prevents duplicate FlowCollector creation attempts

3. Add automatic retry when operator readiness check times out
   - Previously returned RequeueAfter: 0, causing no retry
   - Now returns RequeueAfter: 5 minutes to check again later
   - Ensures operator installation is eventually noticed and FlowCollector is created

Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>

Author: OlivierCazade

pkg/controller/observability/observability_controller.go

@@ -42,6 +42,7 @@ const (
 
 	checkInterval = 10 * time.Second
 	checkTimeout  = 10 * time.Minute
+	requeueAfter  = 5 * time.Minute // Requeue interval when operator is not ready yet
 
 	// NetworkObservabilityDeployed is the condition type that indicates Network Observability was successfully deployed
 	NetworkObservabilityDeployed = "NetworkObservabilityDeployed"
@@ -156,9 +157,9 @@ func (r *ReconcileObservability) Reconcile(ctx context.Context, req ctrl.Request
 	klog.Info("Wait for Network Observability to be ready")
 	if err := r.waitForNetObservOperator(ctx); err != nil {
 		if err == context.DeadlineExceeded {
-			klog.Errorf("Timed out waiting for Network Observability Operator to be ready after %v. Stopping reconciliation.", checkTimeout)
+			klog.Errorf("Timed out waiting for Network Observability Operator to be ready after %v. Will retry in %v.", checkTimeout, requeueAfter)
 			r.status.SetDegraded(statusmanager.ObservabilityConfig, "OperatorNotReady", fmt.Sprintf("Timed out waiting for Network Observability Operator to be ready after %v", checkTimeout))
-			return ctrl.Result{RequeueAfter: 0}, nil // Don't requeue
+			return ctrl.Result{RequeueAfter: requeueAfter}, nil
 		}
 		r.status.SetDegraded(statusmanager.ObservabilityConfig, "WaitOperatorError", fmt.Sprintf("Failed waiting for Network Observability Operator: %v", err))
 		return ctrl.Result{}, err
@@ -181,7 +182,8 @@ func (r *ReconcileObservability) Reconcile(ctx context.Context, req ctrl.Request
 
 	// Mark as deployed in Network CR status
 	if err := r.markNetworkObservabilityDeployed(ctx, &network); err != nil {
-		klog.Warningf("Failed to update Network Observability deployment status: %v", err)
+		r.status.SetDegraded(statusmanager.ObservabilityConfig, "UpdateStatusError", fmt.Sprintf("Failed to update Network Observability deployment status: %v", err))
+		return ctrl.Result{}, err
 	}
 
 	r.status.SetNotDegraded(statusmanager.ObservabilityConfig)
@@ -337,6 +339,8 @@ func (r *ReconcileObservability) waitForNetObservOperator(ctx context.Context) e
 		}
 
 		// Find the netobserv operator CSV
+		// During upgrades, there can be multiple CSVs. Check all of them and
+		// prioritize ones in "Succeeded" state to avoid checking the wrong CSV.
 		for _, csv := range csvs.Items {
 			name := csv.GetName()
 			// CSV names are typically like "netobserv-operator.v1.2.3"
@@ -346,11 +350,15 @@ func (r *ReconcileObservability) waitForNetObservOperator(ctx context.Context) e
 					return false, err
 				}
 				if !found {
-					return false, nil
+					continue
+				}
+				// If we find a CSV in Succeeded state, return true immediately
+				if phase == "Succeeded" {
+					return true, nil
 				}
-				return phase == "Succeeded", nil
 			}
 		}
+		// If we found matching CSVs but none in Succeeded state, keep waiting
 		return false, nil
 	}
 	return wait.PollUntilContextTimeout(ctx, checkInterval, checkTimeout, true, condition)

pkg/controller/observability/observability_controller_test.go

@@ -928,9 +928,9 @@ func TestReconcile_OperatorNotReady(t *testing.T) {
 
 	result, err := r.Reconcile(ctx, req)
 
-	// Controller returns no error on timeout, but result.RequeueAfter should be 0
+	// Controller returns no error on timeout, but should requeue after 5 minutes
 	g.Expect(err).NotTo(HaveOccurred())
-	g.Expect(result.RequeueAfter).To(BeZero())
+	g.Expect(result.RequeueAfter).To(Equal(5 * time.Minute))
 }
 
 // TestReconcile_FlowCollectorDeleted tests that reconciliation recreates
@@ -1147,13 +1147,13 @@ func TestReconcile_RecoveryAfterOperatorBecomesReady(t *testing.T) {
 
 	req := ctrl.Request{NamespacedName: types.NamespacedName{Name: "cluster"}}
 
-	// First reconciliation should timeout (returns no error but RequeueAfter=0)
+	// First reconciliation should timeout (returns no error but RequeueAfter=5 minutes)
 	ctx1, cancel1 := context.WithTimeout(context.Background(), 1*time.Second)
 	defer cancel1()
 
 	result1, err1 := r.Reconcile(ctx1, req)
 	g.Expect(err1).NotTo(HaveOccurred())
-	g.Expect(result1.RequeueAfter).To(BeZero())
+	g.Expect(result1.RequeueAfter).To(Equal(5 * time.Minute))
 
 	// Update CSV to Succeeded phase
 	_ = unstructured.SetNestedField(csv.Object, "Succeeded", "status", "phase")