review: address comments

address coderabbit AI comments

AI-attribution: AIA PAI Ce Hin R claude-4.6-opus-1M v1.0
Signed-off-by: Francesco Romani <fromani@redhat.com>

Author: ffromani

internal/controller/numaresourcesoperator_controller.go

@@ -820,7 +820,9 @@ func (r *NUMAResourcesOperatorReconciler) applyObjects(ctx context.Context, inst
 	defer klog.V(4).InfoS("Applyied objects", "count", len(objStates))
 	for _, objState := range objStates {
 		if objState.Error != nil {
-			klog.Warningf("error loading object: %v", objState.Error)
+			if !objState.IsNotFoundError() {
+				return fmt.Errorf("failed to load object state for %s/%s: %w", objState.Desired.GetNamespace(), objState.Desired.GetName(), objState.Error)
+			}
 		}
 		if objState.UpdateError != nil {
 			return fmt.Errorf("failed to update (%s) %s/%s: %w", objState.Desired.GetObjectKind().GroupVersionKind(), objState.Desired.GetNamespace(), objState.Desired.GetName(), objState.UpdateError)
@@ -900,7 +902,7 @@ func shouldReplaceStep(current, candidate intreconcile.Step) bool {
 		return true
 	}
 	if candidate.Ongoing() && current.Ongoing() {
-		return candidate.Result.RequeueAfter < current.Result.RequeueAfter
+		return candidate.Result.RequeueAfter > 0 && candidate.Result.RequeueAfter < current.Result.RequeueAfter
 	}
 	return false
 }

internal/controller/numaresourcesoperator_controller_test.go

@@ -2296,6 +2296,19 @@ var _ = Describe("Test NUMAResourcesOperator Reconcile", func() {
 				Expect(progressingCond).ToNot(BeNil())
 				Expect(progressingCond.Status).To(Equal(metav1.ConditionTrue))
 
+				By("Verify pool1 DaemonSet does not exist yet")
+				mcp1DSKey := client.ObjectKey{
+					Name:      objectnames.GetComponentName(nro.Name, mcp1.Name),
+					Namespace: testNamespace,
+				}
+				err = reconciler.Client.Get(ctx, mcp1DSKey, ds)
+				Expect(apierrors.IsNotFound(err)).To(BeTrue(), "unexpected error: %v", err)
+
+				By("Verify global Available condition is false")
+				availableCond := getConditionByType(nro.Status.Conditions, status.ConditionAvailable)
+				Expect(availableCond).ToNot(BeNil())
+				Expect(availableCond.Status).To(Equal(metav1.ConditionFalse))
+
 				By("Make pool1 ready and reconcile again")
 				Expect(reconciler.Client.Get(ctx, client.ObjectKeyFromObject(mcp1), mcp1)).To(Succeed())
 				ensureMCPIsReady(mcp1, nro.Name)
@@ -2304,7 +2317,7 @@ var _ = Describe("Test NUMAResourcesOperator Reconcile", func() {
 				Expect(reconciler.Reconcile(ctx, reconcile.Request{NamespacedName: key})).ToNot(CauseRequeue())
 
 				By("Now both DaemonSets exist and status is Available")
-				mcp1DSKey := client.ObjectKey{
+				mcp1DSKey = client.ObjectKey{
 					Name:      objectnames.GetComponentName(nro.Name, mcp1.Name),
 					Namespace: testNamespace,
 				}

internal/reconcile/step.go

@@ -45,7 +45,7 @@ func (rs Step) EarlyStop() bool {
 
 // Ongoing returns true if the reconciliation is still ongoing/progressing
 func (rs Step) Ongoing() bool {
-	return rs.Result.RequeueAfter > 0
+	return rs.ConditionInfo.Type == status.ConditionProgressing
 }
 
 // Failed returns true if the reconciliation was not succesfull

internal/reconcile/step_test.go

@@ -48,6 +48,12 @@ func TestStepOngoingIsOngoing(t *testing.T) {
 	assert.False(t, st.Failed())
 }
 
+func TestStepOngoingZeroDurationIsOngoing(t *testing.T) {
+	st := StepOngoing(0)
+	assert.True(t, st.Ongoing())
+	assert.False(t, st.Done())
+}
+
 func TestStepFailedIsFailed(t *testing.T) {
 	st := StepFailed(errors.New("fake error"))
 	assert.True(t, st.Failed())