From c19019e09a545babecf708d63945bff78c7c4b01 Mon Sep 17 00:00:00 2001 From: Watson Yuuma Sato Date: Mon, 31 Aug 2026 12:25:08 +0200 Subject: [PATCH] CMP-4631: Wait for MCPs to observe new config before rescanning After applying dependency-gated remediations, the e2e test's "wait for MachineConfigPools to be updated" gate could return immediately: the MachineConfigPool still reported Updated=True from the *previous* rollout because the Machine Config Operator had not yet reacted to the just-created MachineConfigs. The final rescan then ran while nodes were still rebooting onto staggered MachineConfig generations, so the compliance-operator saw different results per node and aggregated them as INCONSISTENT. usbguard was the most exposed rule set (a two-wave dependency chain), producing 12 spurious INCONSISTENT results across rhcos4-high and rhcos4-stig. Fix, mirroring compliance-operator's WaitForMachinePoolUpdate: - Snapshot each pool's Status.ObservedGeneration before a wave's remediations are applied (snapshotMachineConfigPoolGenerations) and require the pool to advance past that baseline (it has observed a new rendered config) before it can be considered done. A pool that never changes is accepted once a short reaction grace elapses, so genuinely-unchanged pools don't hang. - Tighten isMachineConfigPoolUpdated to also require the targeted rendered config to be realized (Spec.Configuration.Name == Status.Configuration.Name), catching a pool mid-rollout even if machine counts momentarily look settled. Add a unit test for isMachineConfigPoolUpdated covering the mid-rollout case. Co-Authored-By: Claude Opus 4.8 --- helpers/mcp_test.go | 79 ++++++++++++++++++++++++++++++++++++++++++ helpers/utilities.go | 82 ++++++++++++++++++++++++++++++++++++++------ 2 files changed, 151 insertions(+), 10 deletions(-) create mode 100644 helpers/mcp_test.go diff --git a/helpers/mcp_test.go b/helpers/mcp_test.go new file mode 100644 index 00000000..f41e1faa --- /dev/null +++ b/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) + } + }) + } +} diff --git a/helpers/utilities.go b/helpers/utilities.go index cbb975e9..84e3cc04 100644 --- a/helpers/utilities.go +++ b/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.