CMP-4631: Wait for MachineConfigPools to observe new config before rescanning

helpers/mcp_test.go

@@ -0,0 +1,79 @@
+package helpers
+
+import (
+	"testing"
+
+	mcfgv1 "github.com/openshift/machine-config-operator/pkg/apis/machineconfiguration.openshift.io/v1"
+	corev1 "k8s.io/api/core/v1"
+)
+
+// mcp builds a MachineConfigPool with the given machine counts and rendered
+// config names for status/spec.
+func mcp(
+	machineCount, updated, unavailable, degraded int32,
+	specConfig, statusConfig string,
+) *mcfgv1.MachineConfigPool {
+	return &mcfgv1.MachineConfigPool{
+		Spec: mcfgv1.MachineConfigPoolSpec{
+			Configuration: mcfgv1.MachineConfigPoolStatusConfiguration{
+				ObjectReference: corev1.ObjectReference{Name: specConfig},
+			},
+		},
+		Status: mcfgv1.MachineConfigPoolStatus{
+			MachineCount:            machineCount,
+			UpdatedMachineCount:     updated,
+			UnavailableMachineCount: unavailable,
+			DegradedMachineCount:    degraded,
+			Configuration: mcfgv1.MachineConfigPoolStatusConfiguration{
+				ObjectReference: corev1.ObjectReference{Name: statusConfig},
+			},
+		},
+	}
+}
+
+// TestIsMachineConfigPoolUpdated covers the completion predicate, including the
+// CMP-4631 addition that a pool mid-rollout (its targeted rendered config not
+// yet realized in status) is not reported as updated even when the machine
+// counts momentarily look settled.
+func TestIsMachineConfigPoolUpdated(t *testing.T) {
+	const rendered = "rendered-worker-abc"
+	tests := []struct {
+		name string
+		pool *mcfgv1.MachineConfigPool
+		want bool
+	}{
+		{
+			name: "fully converged and config realized",
+			pool: mcp(1, 1, 0, 0, rendered, rendered),
+			want: true,
+		},
+		{
+			name: "mid-rollout: spec config not yet realized in status",
+			pool: mcp(3, 3, 0, 0, "rendered-worker-new", "rendered-worker-old"),
+			want: false,
+		},
+		{
+			name: "not all machines updated",
+			pool: mcp(3, 2, 1, 0, rendered, rendered),
+			want: false,
+		},
+		{
+			name: "a machine is unavailable",
+			pool: mcp(3, 3, 1, 0, rendered, rendered),
+			want: false,
+		},
+		{
+			name: "a machine is degraded",
+			pool: mcp(3, 3, 0, 1, rendered, rendered),
+			want: false,
+		},
+	}
+
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			if got := isMachineConfigPoolUpdated(tt.pool); got != tt.want {
+				t.Errorf("isMachineConfigPoolUpdated() = %v, want %v", got, tt.want)
+			}
+		})
+	}
+}

helpers/utilities.go

@@ -1199,8 +1199,19 @@ func ApplyRemediationsWithDependencies(tc *testConfig.TestConfig, c dynclient.Cl
 		iteration++
 		log.Printf("Starting remediation iteration %d for suite %s", iteration, suiteName)
 
+		// Snapshot the MachineConfigPool generations *before* this wave's
+		// remediations are applied. WaitForMachineConfigPoolsUpdated uses this
+		// baseline to tell "already converged from the previous rollout" apart
+		// from "the MCO has not yet reacted to the MachineConfigs we just
+		// created" -- the race that otherwise lets the rescan run mid-reboot and
+		// produce spurious INCONSISTENT results (CMP-4631).
+		mcpBaseline, err := snapshotMachineConfigPoolGenerations(c)
+		if err != nil {
+			return fmt.Errorf("failed to snapshot MachineConfigPool generations in iteration %d: %w", iteration, err)
+		}
+
 		// Wait for remediations to be applied
-		err := WaitForRemediationsToBeApplied(tc, c, suiteName)
+		err = WaitForRemediationsToBeApplied(tc, c, suiteName)
 		if err != nil {
 			return fmt.Errorf("failed during remediation iteration %d: %w", iteration, err)
 		}
@@ -1218,7 +1229,7 @@ func ApplyRemediationsWithDependencies(tc *testConfig.TestConfig, c dynclient.Cl
 			log.Printf("Performing final rescan to get accurate results after all remediations")
 
 			// Wait for MachineConfigPools to be updated after final remediations
-			err = WaitForMachineConfigPoolsUpdated(tc, c)
+			err = WaitForMachineConfigPoolsUpdated(tc, c, mcpBaseline)
 			if err != nil {
 				return fmt.Errorf("failed to wait for MachineConfigPools after final remediations: %w", err)
 			}
@@ -1242,7 +1253,7 @@ func ApplyRemediationsWithDependencies(tc *testConfig.TestConfig, c dynclient.Cl
 		log.Printf("Found %d remediations with missing dependencies, triggering rescan", missingDepCount)
 
 		// Wait for MachineConfigPools to be updated after remediations
-		err = WaitForMachineConfigPoolsUpdated(tc, c)
+		err = WaitForMachineConfigPoolsUpdated(tc, c, mcpBaseline)
 		if err != nil {
 			return fmt.Errorf("failed to wait for MachineConfigPools in iteration %d: %w", iteration, err)
 		}
@@ -1478,8 +1489,40 @@ func RescanComplianceSuite(tc *testConfig.TestConfig, c dynclient.Client, suiteN
 	return nil
 }
 
-// WaitForMachineConfigPoolsUpdated waits for all MachineConfigPools to be fully updated.
-func WaitForMachineConfigPoolsUpdated(tc *testConfig.TestConfig, c dynclient.Client) error {
+// mcpReactionGrace is how long we allow the Machine Config Operator to react to
+// newly-applied MachineConfigs (render a new config and flip the pool to
+// Updating) before we trust a pool that has *not* advanced its generation as
+// having no pending work. Bridging this window is what stops the final rescan
+// from running while nodes are still rebooting onto staggered MachineConfig
+// generations (CMP-4631). The MCO normally reacts within a minute; the grace is
+// only actually paid when a pool genuinely has nothing to roll out.
+const mcpReactionGrace = 5 * time.Minute
+
+// snapshotMachineConfigPoolGenerations records each MachineConfigPool's
+// observed generation. Captured before remediations are applied, it lets
+// WaitForMachineConfigPoolsUpdated detect that a pool has observed a *new*
+// rendered configuration (its generation advances) rather than mistaking the
+// previous rollout's Updated=True state for completion of the new one.
+func snapshotMachineConfigPoolGenerations(c dynclient.Client) (map[string]int64, error) {
+	mcpList := &mcfgv1.MachineConfigPoolList{}
+	if err := c.List(goctx.TODO(), mcpList); err != nil {
+		return nil, fmt.Errorf("failed to list MachineConfigPools: %w", err)
+	}
+	gens := make(map[string]int64, len(mcpList.Items))
+	for i := range mcpList.Items {
+		gens[mcpList.Items[i].Name] = mcpList.Items[i].Status.ObservedGeneration
+	}
+	return gens, nil
+}
+
+// WaitForMachineConfigPoolsUpdated waits for all MachineConfigPools to be fully
+// updated. baseline is the per-pool observed generation captured before the
+// current wave of remediations was applied (see
+// snapshotMachineConfigPoolGenerations); a pool is only considered done once it
+// has both observed the new configuration (generation advanced past baseline)
+// and finished rolling it out, or the reaction grace has elapsed for a pool
+// that never changed.
+func WaitForMachineConfigPoolsUpdated(tc *testConfig.TestConfig, c dynclient.Client, baseline map[string]int64) error {
 	// Get all MachineConfigPools
 	mcpList := &mcfgv1.MachineConfigPoolList{}
 	err := c.List(goctx.TODO(), mcpList)
@@ -1493,6 +1536,7 @@ func WaitForMachineConfigPoolsUpdated(tc *testConfig.TestConfig, c dynclient.Cli
 	}
 
 	log.Printf("Waiting for %d MachineConfigPools to be fully updated", len(mcpList.Items))
+	start := time.Now()
 
 	// Wait for all MCPs to be updated
 	bo := backoff.WithMaxRetries(backoff.NewConstantBackOff(tc.APIPollInterval), 720) // 60 minutes max
@@ -1507,13 +1551,26 @@ func WaitForMachineConfigPoolsUpdated(tc *testConfig.TestConfig, c dynclient.Cli
 				return fmt.Errorf("failed to get MachineConfigPool %s: %w", mcpList.Items[i].Name, err)
 			}
 
-			// Check if the pool is fully updated
-			if !isMachineConfigPoolUpdated(currentMCP) {
-				pendingPools = append(pendingPools, fmt.Sprintf("%s (Updated: %d/%d, Unavailable: %d)",
+			// The pool has observed the new rendered config once its generation
+			// advances past the pre-remediation baseline. A pool that has not
+			// yet reacted is held pending until the grace elapses, so we never
+			// mistake the previous rollout's Updated=True for completion of this
+			// one.
+			reacted := currentMCP.Status.ObservedGeneration > baseline[currentMCP.Name]
+			converged := isMachineConfigPoolUpdated(currentMCP)
+			done := converged && (reacted || time.Since(start) >= mcpReactionGrace)
+
+			if !done {
+				pendingPools = append(pendingPools, fmt.Sprintf(
+					"%s (Updated: %d/%d, Unavailable: %d, gen %d->%d, reacted=%t, converged=%t)",
 					currentMCP.Name,
 					currentMCP.Status.UpdatedMachineCount,
 					currentMCP.Status.MachineCount,
-					currentMCP.Status.UnavailableMachineCount))
+					currentMCP.Status.UnavailableMachineCount,
+					baseline[currentMCP.Name],
+					currentMCP.Status.ObservedGeneration,
+					reacted,
+					converged))
 			}
 		}
 
@@ -1564,9 +1621,14 @@ func isMachineConfigPoolUpdated(mcp *mcfgv1.MachineConfigPool) bool {
 	// 1. All machines are updated (UpdatedMachineCount == MachineCount)
 	// 2. No machines are unavailable (UnavailableMachineCount == 0)
 	// 3. No machines are degraded (DegradedMachineCount == 0)
+	// 4. The targeted rendered config (spec) has been fully realized (status).
+	//    While the MCO is rolling a new config these names differ, so this
+	//    catches a pool that is mid-rollout even if the machine counts briefly
+	//    look settled.
 	return mcp.Status.UpdatedMachineCount == mcp.Status.MachineCount &&
 		mcp.Status.UnavailableMachineCount == 0 &&
-		mcp.Status.DegradedMachineCount == 0
+		mcp.Status.DegradedMachineCount == 0 &&
+		mcp.Spec.Configuration.Name == mcp.Status.Configuration.Name
 }
 
 // getBoolString converts a boolean to a string for logging.