Fix ImageBuildDegraded Status updates

After an image was built and rolled out successfully,
and we triggered a new build, the ImageBuildDegraded
status wasn't updating correctly as it kept focusing on
the successful build and not the build currently active.
Fix the logic to be able to keep track of the active
build instead and update the status accordingly.

Signed-off-by: Urvashi <[email protected]>

Author: umohnani8

pkg/controller/build/reconciler.go

@@ -263,56 +263,20 @@ func (b *buildReconciler) updateMachineOSBuild(ctx context.Context, old, current
 	}
 
 	if !oldState.IsBuildFailure() && curState.IsBuildFailure() {
-		klog.Infof("MachineOSBuild %s failed, leaving ephemeral objects in place for inspection and setting BuildDegraded condition", current.Name)
-
-		// Before setting BuildDegraded, check if another MachineOSBuild is active or if this one has been superseded
-		mosbList, err := b.getMachineOSBuildsForMachineOSConfig(mosc)
-		if err != nil {
-			return fmt.Errorf("could not get MachineOSBuilds for MachineOSConfig %q: %w", mosc.Name, err)
-		}
-
-		// Check if there are any newer or active builds that would supersede this failure
-		hasActiveBuild := false
-		isCurrentBuildStale := false
-		for _, mosb := range mosbList {
-			// Skip the current failed build
-			if mosb.Name == current.Name {
-				continue
-			}
-			mosbState := ctrlcommon.NewMachineOSBuildState(mosb)
-			// Check if there's an active build (building, prepared, or succeeded)
-			if mosbState.IsBuilding() || mosbState.IsBuildPrepared() || mosbState.IsBuildSuccess() {
-				hasActiveBuild = true
-				klog.Infof("Found active MachineOSBuild %s, skipping BuildDegraded condition for failed build %s", mosb.Name, current.Name)
-				break
-			}
-		}
-
-		// Also check if the failed MachineOSBuild is no longer referenced by the MachineOSConfig
-		if mosc.Status.CurrentImagePullSpec != "" && !isMachineOSBuildCurrentForMachineOSConfigWithPullspec(mosc, current) {
-			isCurrentBuildStale = true
-			klog.Infof("Failed MachineOSBuild %s is no longer current for MachineOSConfig %s, skipping BuildDegraded condition", current.Name, mosc.Name)
-		}
-
-		// Only set BuildDegraded if there are no active builds and this build is still current
-		if hasActiveBuild || isCurrentBuildStale {
-			klog.Infof("Skipping BuildDegraded condition for failed MachineOSBuild %s (hasActiveBuild=%v, isStale=%v)", current.Name, hasActiveBuild, isCurrentBuildStale)
-			return nil
-		}
+		klog.Infof("MachineOSBuild %s failed, leaving ephemeral objects in place for inspection", current.Name)
 
 		mcp, err := b.machineConfigPoolLister.Get(mosc.Spec.MachineConfigPool.Name)
 		if err != nil {
 			return fmt.Errorf("could not get MachineConfigPool from MachineOSConfig %q: %w", mosc.Name, err)
 		}
 
-		// Set BuildDegraded condition
-		buildError := getBuildErrorFromMOSB(current)
-		return b.syncBuildFailureStatus(ctx, mcp, buildError, current.Name)
+		// Always update ImageBuildDegraded condition based on current active build status
+		return b.updateImageBuildDegradedCondition(ctx, mcp, mosc)
 	}
 
 	// If the build was successful, clean up the build objects and propagate the
 	// final image pushspec onto the MachineOSConfig object.
-	// Also clear BuildDegraded condition if it was set due to a previously failed build
+	// Also update BuildDegraded condition based on current active build status
 	if !oldState.IsBuildSuccess() && curState.IsBuildSuccess() {
 		klog.Infof("MachineOSBuild %s succeeded, cleaning up all ephemeral objects used for the build", current.Name)
 
@@ -321,9 +285,9 @@ func (b *buildReconciler) updateMachineOSBuild(ctx context.Context, old, current
 			return fmt.Errorf("could not get MachineConfigPool from MachineOSConfig %q: %w", mosc.Name, err)
 		}
 
-		// Clear BuildDegraded condition if set
-		if err := b.syncBuildSuccessStatus(ctx, mcp); err != nil {
-			klog.Errorf("Failed to clear BuildDegraded condition for pool %s: %v", mcp.Name, err)
+		// Update BuildDegraded condition based on current active build status
+		if err := b.updateImageBuildDegradedCondition(ctx, mcp, mosc); err != nil {
+			klog.Errorf("Failed to update ImageBuildDegraded condition for pool %s: %v", mcp.Name, err)
 		}
 
 		// Clean up ephemeral objects
@@ -369,6 +333,18 @@ func (b *buildReconciler) updateMachineOSConfigStatus(ctx context.Context, mosc
 		klog.Infof("Updated annotations on MachineOSConfig %q", mosc.Name)
 
 		mosc = updatedMosc
+
+		// When annotations are updated (new build starts), also update observedGeneration
+		// to signal that the controller is processing the current generation
+		if mosc.Status.ObservedGeneration != mosc.GetGeneration() {
+			mosc.Status.ObservedGeneration = mosc.GetGeneration()
+			_, err = b.mcfgclient.MachineconfigurationV1().MachineOSConfigs().UpdateStatus(ctx, mosc, metav1.UpdateOptions{})
+			if err != nil {
+				klog.Errorf("Failed to update observedGeneration on MachineOSConfig %q: %v", mosc.Name, err)
+			} else {
+				klog.Infof("Updated observedGeneration on MachineOSConfig %q to %d", mosc.Name, mosc.GetGeneration())
+			}
+		}
 	}
 
 	// Skip the status update if digest image pushspec hasn't been set yet.
@@ -457,14 +433,14 @@ func (b *buildReconciler) startBuild(ctx context.Context, mosb *mcfgv1.MachineOS
 		return fmt.Errorf("could not update MachineOSConfig %q status for MachineOSBuild %q: %w", mosc.Name, mosb.Name, err)
 	}
 
-	// Initialize BuildDegraded condition to False when build starts
+	// Update BuildDegraded condition based on current active build status when build starts
 	mcp, err := b.machineConfigPoolLister.Get(mosc.Spec.MachineConfigPool.Name)
 	if err != nil {
 		return fmt.Errorf("could not get MachineConfigPool from MachineOSConfig %q: %w", mosc.Name, err)
 	}
 
-	if err := b.initializeBuildDegradedCondition(ctx, mcp); err != nil {
-		klog.Errorf("Failed to initialize BuildDegraded condition for pool %s: %v", mcp.Name, err)
+	if err := b.updateImageBuildDegradedCondition(ctx, mcp, mosc); err != nil {
+		klog.Errorf("Failed to update ImageBuildDegraded condition for pool %s: %v", mcp.Name, err)
 	}
 
 	return nil
@@ -1633,3 +1609,79 @@ func (b *buildReconciler) syncBuildFailureStatus(ctx context.Context, pool *mcfg
 	}
 	return buildErr
 }
+
+// getCurrentBuild finds the currently active (most relevant) build from a list of MachineOSBuilds
+// Priority: 1) Currently referenced by MOSC, 2) Building/Prepared, 3) Most recent
+func (b *buildReconciler) getCurrentBuild(mosc *mcfgv1.MachineOSConfig, mosbList []*mcfgv1.MachineOSBuild) *mcfgv1.MachineOSBuild {
+	var activeBuild *mcfgv1.MachineOSBuild
+	var mostRecentBuild *mcfgv1.MachineOSBuild
+
+	// First, look for the build currently referenced by the MachineOSConfig
+	for _, mosb := range mosbList {
+		if isMachineOSBuildCurrentForMachineOSConfig(mosc, mosb) {
+			activeBuild = mosb
+			break
+		}
+	}
+
+	// If no current build found, look for active builds (building/prepared)
+	if activeBuild == nil {
+		for _, mosb := range mosbList {
+			mosbState := ctrlcommon.NewMachineOSBuildState(mosb)
+			if mosbState.IsBuilding() || mosbState.IsBuildPrepared() {
+				activeBuild = mosb
+				break
+			}
+		}
+	}
+
+	// Keep track of the most recent build regardless
+	for _, mosb := range mosbList {
+		if mostRecentBuild == nil || mosb.CreationTimestamp.After(mostRecentBuild.CreationTimestamp.Time) {
+			mostRecentBuild = mosb
+		}
+	}
+
+	// If still no active build, use the most recent one
+	if activeBuild == nil {
+		activeBuild = mostRecentBuild
+	}
+
+	return activeBuild
+}
+
+// updateImageBuildDegradedCondition examines all MachineOSBuilds for the MachineOSConfig
+// and sets the ImageBuildDegraded condition based on the status of the currently active build
+func (b *buildReconciler) updateImageBuildDegradedCondition(ctx context.Context, pool *mcfgv1.MachineConfigPool, mosc *mcfgv1.MachineOSConfig) error {
+	mosbList, err := b.getMachineOSBuildsForMachineOSConfig(mosc)
+	if err != nil {
+		return fmt.Errorf("could not get MachineOSBuilds for MachineOSConfig %q: %w", mosc.Name, err)
+	}
+
+	// Find the currently active build
+	activeBuild := b.getCurrentBuild(mosc, mosbList)
+
+	// If no builds exist at all, clear any existing BuildDegraded condition
+	if activeBuild == nil {
+		return b.syncBuildSuccessStatus(ctx, pool)
+	}
+
+	// Update condition based on the active build's status
+	activeState := ctrlcommon.NewMachineOSBuildState(activeBuild)
+
+	switch {
+	case activeState.IsBuildFailure():
+		// Set BuildDegraded=True for failed builds
+		buildError := getBuildErrorFromMOSB(activeBuild)
+		return b.syncBuildFailureStatus(ctx, pool, buildError, activeBuild.Name)
+	case activeState.IsBuildSuccess():
+		// Clear BuildDegraded=False for successful builds
+		return b.syncBuildSuccessStatus(ctx, pool)
+	case activeState.IsBuilding(), activeState.IsBuildPrepared():
+		// Clear BuildDegraded=False for builds in progress (allow retry after previous failure)
+		return b.initializeBuildDegradedCondition(ctx, pool)
+	}
+
+	// For any other states, don't change the condition
+	return nil
+}

pkg/controller/node/status.go

@@ -247,7 +247,8 @@ func (ctrl *Controller) calculateStatus(mcns []*mcfgv1.MachineConfigNode, cconfi
 	// 	  if the pool is paused or true if the pool is not paused and the PIS is not degraded
 	allUpdated := updatedMachineCount == totalMachineCount &&
 		readyMachineCount == totalMachineCount &&
-		unavailableMachineCount == 0
+		unavailableMachineCount == 0 &&
+		!isLayeredPoolBuilding(isLayeredPool, mosc, mosb)
 	if allUpdated {
 		//TODO: update api to only have one condition regarding status of update.
 		updatedMsg := fmt.Sprintf("All nodes are updated with %s", getPoolUpdateLine(pool, mosc, isLayeredPool))
@@ -504,3 +505,35 @@ func isPinnedImageSetsInProgressForPool(pool *mcfgv1.MachineConfigPool) bool {
 	}
 	return false
 }
+
+// isLayeredPoolBuilding checks if a layered pool has an active build or failed build that would
+// make nodes not truly "updated" even if they have the current machine config
+func isLayeredPoolBuilding(isLayeredPool bool, mosc *mcfgv1.MachineOSConfig, mosb *mcfgv1.MachineOSBuild) bool {
+	if !isLayeredPool || mosc == nil || mosb == nil {
+		return false
+	}
+
+	mosbState := ctrlcommon.NewMachineOSBuildState(mosb)
+
+	// Check if there's an active build (building or prepared)
+	if mosbState.IsBuilding() || mosbState.IsBuildPrepared() {
+		return true
+	}
+
+	// Check if there's a failed build - this means the update attempt failed
+	// so nodes should not be considered "updated"
+	if mosbState.IsBuildFailure() {
+		return true
+	}
+
+	// Check if there's a successful build that nodes haven't applied yet
+	// This happens when a build completes but the MOSC status hasn't been updated
+	// or nodes haven't picked up the new image yet
+	if mosbState.IsBuildSuccess() && mosb.Status.DigestedImagePushSpec != "" {
+		// If the successful build's image differs from what MOSC thinks is current,
+		// then nodes are not truly updated to the latest successful build
+		return string(mosb.Status.DigestedImagePushSpec) != string(mosc.Status.CurrentImagePullSpec)
+	}
+
+	return false
+}

test/e2e-ocl/onclusterlayering_test.go

@@ -1448,11 +1448,148 @@ func TestImageBuildDegradedOnFailureAndClearedOnBuildStart(t *testing.T) {
 	moscChangeMosb := buildrequest.NewMachineOSBuildFromAPIOrDie(ctx, cs.GetKubeclient(), updated, mcp)
 
 	// Wait for the second build to start
-	waitForBuildToStart(t, cs, moscChangeMosb)
+	secondMosb := waitForBuildToStart(t, cs, moscChangeMosb)
+	t.Logf("Second build started successfully: %s", secondMosb.Name)
 
 	// Wait for and verify ImageBuildDegraded condition is False after the new build starts.
+	// The condition should be cleared when the build starts.
 	degradedCondition = waitForImageBuildDegradedCondition(ctx, t, cs, layeredMCPName, corev1.ConditionFalse)
 	require.NotNil(t, degradedCondition, "ImageBuildDegraded condition should still be present")
 	assert.Equal(t, string(mcfgv1.MachineConfigPoolBuilding), degradedCondition.Reason, "ImageBuildDegraded reason should be Building")
-	t.Logf("ImageBuildDegraded condition correctly cleared to False with message: %s", degradedCondition.Message)
+	t.Logf("ImageBuildDegraded condition correctly cleared to False when build started with message: %s", degradedCondition.Message)
+
+	// Wait for the second build to complete successfully
+	finishedBuild := waitForBuildToComplete(t, cs, secondMosb)
+	t.Logf("Second build completed successfully: %s", finishedBuild.Name)
+
+	// Wait for the MachineOSConfig to get the new pullspec, which indicates full reconciliation
+	waitForMOSCToGetNewPullspec(ctx, t, cs, mosc.Name, string(finishedBuild.Status.DigestedImagePushSpec))
+
+	// Wait for and verify ImageBuildDegraded condition is False with reason BuildSucceeded
+	degradedCondition = waitForImageBuildDegradedCondition(ctx, t, cs, layeredMCPName, corev1.ConditionFalse)
+	require.NotNil(t, degradedCondition, "ImageBuildDegraded condition should still be present")
+	assert.Equal(t, string(mcfgv1.MachineConfigPoolBuildSuccess), degradedCondition.Reason, "ImageBuildDegraded reason should be BuildSuccess")
+	t.Logf("ImageBuildDegraded condition correctly set to False when build succeeded with message: %s", degradedCondition.Message)
+
+	// Verify MCP status is correct after successful build and full reconciliation
+	successMcp, err := cs.MachineconfigurationV1Interface.MachineConfigPools().Get(ctx, layeredMCPName, metav1.GetOptions{})
+	require.NoError(t, err)
+
+	// After successful build completion and full reconciliation, MCP should show:
+	// Updated=True, Updating=False, Degraded=False, ImageBuildDegraded=False
+	kubeassert.Eventually().MachineConfigPoolReachesState(successMcp, func(mcp *mcfgv1.MachineConfigPool, err error) (bool, error) {
+		if err != nil {
+			return false, err
+		}
+		// Return false (keep polling) if conditions don't match expected state
+		if !apihelpers.IsMachineConfigPoolConditionTrue(mcp.Status.Conditions, mcfgv1.MachineConfigPoolUpdated) ||
+			apihelpers.IsMachineConfigPoolConditionTrue(mcp.Status.Conditions, mcfgv1.MachineConfigPoolUpdating) ||
+			apihelpers.IsMachineConfigPoolConditionTrue(mcp.Status.Conditions, mcfgv1.MachineConfigPoolDegraded) ||
+			apihelpers.IsMachineConfigPoolConditionTrue(mcp.Status.Conditions, mcfgv1.MachineConfigPoolImageBuildDegraded) {
+			return false, nil
+		}
+
+		// Return true when expected state is reached
+		t.Logf("MCP status after successful build - Updated: %v, Updating: %v, Degraded: %v, ImageBuildDegraded: %v",
+			apihelpers.IsMachineConfigPoolConditionTrue(mcp.Status.Conditions, mcfgv1.MachineConfigPoolUpdated),
+			apihelpers.IsMachineConfigPoolConditionTrue(mcp.Status.Conditions, mcfgv1.MachineConfigPoolUpdating),
+			apihelpers.IsMachineConfigPoolConditionTrue(mcp.Status.Conditions, mcfgv1.MachineConfigPoolDegraded),
+			apihelpers.IsMachineConfigPoolConditionTrue(mcp.Status.Conditions, mcfgv1.MachineConfigPoolImageBuildDegraded))
+
+		return true, nil
+	}, "MCP should reach correct state after successful build (Updated=True, Updating=False, Degraded=False, ImageBuildDegraded=False)")
+
+	// Now trigger another build to test MCP status transitions when a new build starts
+	t.Logf("Triggering a third build to test MCP status transitions")
+
+	// Modify the containerfile slightly to trigger a new build
+	apiMosc, err = cs.MachineconfigurationV1Interface.MachineOSConfigs().Get(ctx, mosc.Name, metav1.GetOptions{})
+	require.NoError(t, err)
+
+	// Add a comment to the containerfile to change it and trigger a new build
+	modifiedDockerfile := cowsayDockerfile + "\n# Comment to trigger new build"
+	apiMosc.Spec.Containerfile = []mcfgv1.MachineOSContainerfile{
+		{
+			ContainerfileArch: mcfgv1.NoArch,
+			Content:           modifiedDockerfile,
+		},
+	}
+
+	updated, err = cs.MachineconfigurationV1Interface.MachineOSConfigs().Update(ctx, apiMosc, metav1.UpdateOptions{})
+	require.NoError(t, err)
+
+	t.Logf("Modified containerfile, waiting for third build to start")
+
+	// Get the updated MCP to compute the new build
+	mcp, err = cs.MachineconfigurationV1Interface.MachineConfigPools().Get(ctx, layeredMCPName, metav1.GetOptions{})
+	require.NoError(t, err)
+
+	// Compute the new MachineOSBuild name for the third build
+	thirdMoscMosb := buildrequest.NewMachineOSBuildFromAPIOrDie(ctx, cs.GetKubeclient(), updated, mcp)
+
+	// Wait for the third build to start
+	thirdMosb := waitForBuildToStart(t, cs, thirdMoscMosb)
+	t.Logf("Third build started: %s", thirdMosb.Name)
+
+	// Verify MCP status during active build:
+	// Updated=False, Updating=True, Degraded=False, ImageBuildDegraded=False
+	buildingMcp, err := cs.MachineconfigurationV1Interface.MachineConfigPools().Get(ctx, layeredMCPName, metav1.GetOptions{})
+	require.NoError(t, err)
+
+	kubeassert.Eventually().MachineConfigPoolReachesState(buildingMcp, func(mcp *mcfgv1.MachineConfigPool, err error) (bool, error) {
+		if err != nil {
+			return false, err
+		}
+		// During build, MCP should show: Updated=False, Updating=True
+		if apihelpers.IsMachineConfigPoolConditionTrue(mcp.Status.Conditions, mcfgv1.MachineConfigPoolUpdated) ||
+			!apihelpers.IsMachineConfigPoolConditionTrue(mcp.Status.Conditions, mcfgv1.MachineConfigPoolUpdating) ||
+			apihelpers.IsMachineConfigPoolConditionTrue(mcp.Status.Conditions, mcfgv1.MachineConfigPoolDegraded) ||
+			apihelpers.IsMachineConfigPoolConditionTrue(mcp.Status.Conditions, mcfgv1.MachineConfigPoolImageBuildDegraded) {
+			return false, nil
+		}
+
+		t.Logf("MCP status during active build - Updated: %v, Updating: %v, Degraded: %v, ImageBuildDegraded: %v",
+			apihelpers.IsMachineConfigPoolConditionTrue(mcp.Status.Conditions, mcfgv1.MachineConfigPoolUpdated),
+			apihelpers.IsMachineConfigPoolConditionTrue(mcp.Status.Conditions, mcfgv1.MachineConfigPoolUpdating),
+			apihelpers.IsMachineConfigPoolConditionTrue(mcp.Status.Conditions, mcfgv1.MachineConfigPoolDegraded),
+			apihelpers.IsMachineConfigPoolConditionTrue(mcp.Status.Conditions, mcfgv1.MachineConfigPoolImageBuildDegraded))
+
+		return true, nil
+	}, "MCP should reach correct state during active build (Updated=False, Updating=True, Degraded=False, ImageBuildDegraded=False)")
+
+	// Wait for the third build to complete successfully
+	finalBuild := waitForBuildToComplete(t, cs, thirdMosb)
+	t.Logf("Third build completed successfully: %s", finalBuild.Name)
+
+	// Wait for the MachineOSConfig to get the new pullspec, which indicates full reconciliation
+	waitForMOSCToGetNewPullspec(ctx, t, cs, mosc.Name, string(finalBuild.Status.DigestedImagePushSpec))
+
+	// Final verification: MCP status should return to:
+	// Updated=True, Updating=False, Degraded=False, ImageBuildDegraded=False
+	finalMcp, err := cs.MachineconfigurationV1Interface.MachineConfigPools().Get(ctx, layeredMCPName, metav1.GetOptions{})
+	require.NoError(t, err)
+
+	kubeassert.Eventually().MachineConfigPoolReachesState(finalMcp, func(mcp *mcfgv1.MachineConfigPool, err error) (bool, error) {
+		if err != nil {
+			return false, err
+		}
+		// Return false (keep polling) if conditions don't match expected state
+		if !apihelpers.IsMachineConfigPoolConditionTrue(mcp.Status.Conditions, mcfgv1.MachineConfigPoolUpdated) ||
+			apihelpers.IsMachineConfigPoolConditionTrue(mcp.Status.Conditions, mcfgv1.MachineConfigPoolUpdating) ||
+			apihelpers.IsMachineConfigPoolConditionTrue(mcp.Status.Conditions, mcfgv1.MachineConfigPoolDegraded) ||
+			apihelpers.IsMachineConfigPoolConditionTrue(mcp.Status.Conditions, mcfgv1.MachineConfigPoolImageBuildDegraded) {
+			return false, nil
+		}
+
+		// Return true when expected state is reached
+		t.Logf("Final MCP status after third build completion - Updated: %v, Updating: %v, Degraded: %v, ImageBuildDegraded: %v",
+			apihelpers.IsMachineConfigPoolConditionTrue(mcp.Status.Conditions, mcfgv1.MachineConfigPoolUpdated),
+			apihelpers.IsMachineConfigPoolConditionTrue(mcp.Status.Conditions, mcfgv1.MachineConfigPoolUpdating),
+			apihelpers.IsMachineConfigPoolConditionTrue(mcp.Status.Conditions, mcfgv1.MachineConfigPoolDegraded),
+			apihelpers.IsMachineConfigPoolConditionTrue(mcp.Status.Conditions, mcfgv1.MachineConfigPoolImageBuildDegraded))
+
+		return true, nil
+	}, "MCP should return to correct state after final build completion (Updated=True, Updating=False, Degraded=False, ImageBuildDegraded=False)")
+
+	t.Logf("All MCP status transitions verified successfully across build failure, success, and subsequent new build")
 }