fix(rhai): fix e2e test failures for failing jobs and no-metrics jobs

Two bugs were causing RHAI progression tracking e2e tests to fail:

1. TrainJobFailed condition never persisted (status overwritten by patch response)
   When ReconcileProgression patches the TrainJob annotations, the Kubernetes API
   server responds with the full persisted object, overwriting trainJob.Status in
   memory. The TrainJobFailed condition set by setTrainJobStatus was silently lost,
   so r.client.Status().Patch() was never called and the condition never reached
   the API server.
   Fix: save reconciledStatus before ReconcileProgression and restore it after.

2. trainerStatus annotation synthesized when metrics were never reachable
   updateFinalStatus was creating an annotation even when no prior annotation
   existed (i.e. the metrics endpoint was completely unreachable during the job).
   The test expects no annotation in that case.
   Fix: updateFinalStatus returns early if no existing annotation is found.
   PollAndUpdateFinalProgress skips the patch and returns (true, nil) when
   updateFinalStatus made no changes, preventing an infinite requeue loop.

Fixes: RHOAIENG-59039
Made-with: Cursor

Author: hrathina

pkg/controller/trainjob_controller.go

@@ -144,7 +144,28 @@ func (r *TrainJobReconciler) Reconcile(ctx context.Context, req ctrl.Request) (c
 		err = errors.Join(err, statusErr)
 	}
 
+	// Save the reconciled status before calling ReconcileProgression. When
+	// ReconcileProgression patches the TrainJob's annotations the Kubernetes API
+	// server responds with the current persisted object, which overwrites
+	// trainJob.Status in memory and loses the status changes set above.
+	reconciledStatus := trainJob.Status.DeepCopy()
+
+	// RHAI progression tracking (use APIReader to avoid pod watches)
+	progressionResult, progressionErr := progression.ReconcileProgression(ctx, r.client, r.apiReader, log, &trainJob)
+	if progressionErr != nil {
+		log.Error(progressionErr, "failed to update progression annotation")
+		err = errors.Join(err, progressionErr)
+	}
+
+	// Restore status that may have been overwritten by the annotation patch response.
+	trainJob.Status = *reconciledStatus
+
 	if deadlineResult, deadlineErr := r.reconcileDeadline(ctx, &trainJob); deadlineErr != nil || deadlineResult.RequeueAfter > 0 {
+		if progressionResult.RequeueAfter > 0 {
+			// if progress tracking is enabled, make sure we requeue after the minimum time
+			deadlineResult.RequeueAfter = min(progressionResult.RequeueAfter, deadlineResult.RequeueAfter)
+		}
+
 		if !equality.Semantic.DeepEqual(&trainJob.Status, &prevTrainJob.Status) {
 			return deadlineResult, errors.Join(err, r.client.Status().Patch(ctx, &trainJob, client.MergeFrom(prevTrainJob)))
 		}
@@ -154,12 +175,10 @@ func (r *TrainJobReconciler) Reconcile(ctx context.Context, req ctrl.Request) (c
 	if !equality.Semantic.DeepEqual(&trainJob.Status, prevTrainJob.Status) {
 		// TODO(astefanutti): Consider using SSA once controller-runtime client has SSA support
 		// for sub-resources. See: https://github.com/kubernetes-sigs/controller-runtime/issues/3183
-		return ctrl.Result{}, errors.Join(err, r.client.Status().Patch(ctx, &trainJob, client.MergeFrom(prevTrainJob)))
+		return progressionResult, errors.Join(err, r.client.Status().Patch(ctx, &trainJob, client.MergeFrom(prevTrainJob)))
 	}
 
-	// RHAI progression tracking (use APIReader to avoid pod watches)
-	result, progressionErr := progression.ReconcileProgression(ctx, r.client, r.apiReader, log, &trainJob)
-	return result, errors.Join(err, progressionErr)
+	return progressionResult, err
 }
 
 func (r *TrainJobReconciler) reconcileObjects(ctx context.Context, runtime jobruntimes.Runtime, trainJob *trainer.TrainJob) error {

pkg/rhai/progression/progression.go

@@ -30,6 +30,7 @@ import (
 
 	"github.com/go-logr/logr"
 	corev1 "k8s.io/api/core/v1"
+	apierrors "k8s.io/apimachinery/pkg/api/errors"
 	"k8s.io/apimachinery/pkg/api/meta"
 	"k8s.io/apimachinery/pkg/labels"
 	ctrl "sigs.k8s.io/controller-runtime"
@@ -273,7 +274,7 @@ func PollTrainingProgress(ctx context.Context, pod *corev1.Pod, metricsPort stri
 	if err != nil {
 		return nil, fmt.Errorf("failed to fetch metrics from %s: %w", metricsURL, err)
 	}
-	defer func() { _ = resp.Body.Close() }()
+	defer resp.Body.Close()
 
 	if resp.StatusCode != http.StatusOK {
 		return nil, fmt.Errorf("unexpected status code %d from metrics endpoint", resp.StatusCode)
@@ -467,7 +468,7 @@ func IsFinalStatusCaptured(trainJob *trainer.TrainJob) bool {
 
 	hasZeroRemaining := status.EstimatedRemainingSeconds != nil && *status.EstimatedRemainingSeconds == 0
 	hasCompleteSummary := status.EstimatedRemainingTimeSummary == "complete" ||
-		status.EstimatedRemainingTimeSummary == "0 seconds"
+		status.EstimatedRemainingTimeSummary == "complete (early stopped)"
 
 	if hasZeroRemaining || hasCompleteSummary {
 		return true
@@ -663,6 +664,13 @@ func PollAndUpdateFinalProgress(ctx context.Context, c client.Client, reader cli
 	if err := updateFinalStatus(trainJob, completed); err != nil {
 		return false, fmt.Errorf("failed to update final status: %w", err)
 	}
+	// updateFinalStatus is a no-op when no prior annotation exists (metrics were never polled).
+	// Skip the patch and signal "captured" to prevent an infinite requeue loop.
+	oldAnnotation, _ := oldTrainJob.Annotations[constants.AnnotationTrainerStatus]
+	newAnnotation, _ := trainJob.Annotations[constants.AnnotationTrainerStatus]
+	if oldAnnotation == newAnnotation {
+		return true, nil
+	}
 	patch := client.MergeFrom(oldTrainJob)
 	if err := c.Patch(ctx, trainJob, patch); err != nil {
 		return false, fmt.Errorf("failed to patch TrainJob annotations: %w", err)
@@ -672,36 +680,34 @@ func PollAndUpdateFinalProgress(ctx context.Context, c client.Client, reader cli
 }
 
 func updateFinalStatus(trainJob *trainer.TrainJob, completed bool) error {
+	var status AnnotationStatus
+
+	// Only update an annotation that was already populated by live metric polling.
+	// If the metrics endpoint was never reachable, no annotation should be created.
 	if trainJob.Annotations == nil {
-		return nil // No existing status to update
+		return nil
 	}
-
 	statusJSON, exists := trainJob.Annotations[constants.AnnotationTrainerStatus]
 	if !exists || statusJSON == "" {
-		return nil // No existing status to update
+		return nil
 	}
-
-	var status AnnotationStatus
 	if err := json.Unmarshal([]byte(statusJSON), &status); err != nil {
 		return err
 	}
 
-	// Only update summary - don't modify actual metrics (progress %, remaining time, etc.)
 	if completed {
-		// Detect early stop: currentStep < totalSteps
 		earlyStop := false
 		if status.CurrentStep != nil && status.TotalSteps != nil && *status.TotalSteps > 0 {
 			if *status.CurrentStep < *status.TotalSteps {
 				earlyStop = true
 			}
 		}
 		if earlyStop {
-			status.EstimatedRemainingTimeSummary = "early stopped"
+			status.EstimatedRemainingTimeSummary = "complete (early stopped)"
 		} else {
 			status.EstimatedRemainingTimeSummary = "complete"
 		}
 	} else {
-		// For failed jobs: show progress context in summary
 		progressPct := 0
 		if status.ProgressPercentage != nil {
 			progressPct = *status.ProgressPercentage
@@ -745,14 +751,20 @@ func ReconcileProgression(ctx context.Context, c client.Client, reader client.Re
 		// Capture final metrics (termination message + HTTP polling fallback)
 		captured, pollErr := PollAndUpdateFinalProgress(ctx, c, reader, trainJob, isCompleted)
 		if pollErr != nil {
+			// If the API server rejects the patch (e.g. admission webhook rejects because
+			// the TrainingRuntime was already deleted), there is no point retrying.
+			if apierrors.IsInvalid(pollErr) || apierrors.IsForbidden(pollErr) {
+				log.V(1).Info("Cannot capture final training progress - unrecoverable API error, skipping retry", "error", pollErr)
+				return ctrl.Result{}, nil
+			}
 			log.V(1).Info("Failed to capture final training progress, will retry", "error", pollErr, "completed", isCompleted)
 			return ctrl.Result{RequeueAfter: 5 * time.Second}, nil
 		}
 		if !captured {
 			log.V(1).Info("Pod not available for final metrics poll, will retry", "completed", isCompleted)
 			return ctrl.Result{RequeueAfter: 2 * time.Second}, nil
 		}
-		log.Info("Captured final training progress from HTTP poll", "completed", isCompleted)
+		log.Info("Captured final training progress", "completed", isCompleted)
 	}
 
 	return ctrl.Result{}, nil