From 1efae11d2ecc08a3c1eff73afb22516662229071 Mon Sep 17 00:00:00 2001 From: Chai Bot Date: Fri, 2 Oct 2026 14:13:16 +0000 Subject: [PATCH 1/2] refactor(nodepool): centralize CAPI condition application Return reconcile-local condition results from CAPI so resource reconciliation no longer mutates NodePool conditions directly. Apply accumulated results in the NodePool controller before error handling. Preserve condition order and status updates across early returns and failures. Assisted-by: LLM Signed-off-by: Chai Bot --- .../e2eutilallowlist/e2eutilallowlist_test.go | 56 ++ .../controllers/nodepool/capi.go | 170 ++-- .../controllers/nodepool/capi_test.go | 903 +++++++++++++++++- .../nodepool/nodepool_controller.go | 11 +- .../nodepool/nodepool_controller_test.go | 233 +++++ 5 files changed, 1228 insertions(+), 145 deletions(-) diff --git a/hack/tools/hypershiftlinter/analyzers/e2eutilallowlist/e2eutilallowlist_test.go b/hack/tools/hypershiftlinter/analyzers/e2eutilallowlist/e2eutilallowlist_test.go index f07c0da2a8c1..c96b41f6d86b 100644 --- a/hack/tools/hypershiftlinter/analyzers/e2eutilallowlist/e2eutilallowlist_test.go +++ b/hack/tools/hypershiftlinter/analyzers/e2eutilallowlist/e2eutilallowlist_test.go @@ -7,6 +7,7 @@ import ( "go/types" "os" "path/filepath" + "slices" "strings" "testing" @@ -175,6 +176,61 @@ func TestAllowlist(t *testing.T) { } } +func TestAllowlistOSImageStreamUpgradeScopes(t *testing.T) { + t.Parallel() + const filename = "test/e2e/v2/tests/nodepool_osimagestream_test.go" + file, err := parser.ParseFile(token.NewFileSet(), filepath.Join("../../../../..", filename), nil, 0) + if err != nil { + t.Fatalf("parse OSImageStream upgrade specs: %v", err) + } + wantSymbols := []string{"EventuallyObject", "OSImageStreamPredicate", "Predicate", "UpdateObject", "WaitForReadyNodesByNodePool", "WithInterval", "WithTimeout"} + for _, testCase := range []struct { + name string + function string + }{ + {name: "When the cross-major upgrade spec is renamed, it should retain only its existing utility allowances", function: "NodePoolOSImageStreamCrossMajorUpgradeTest"}, + {name: "When the pinned RHEL9 upgrade spec is renamed, it should retain only its existing utility allowances", function: "NodePoolOSImageStreamPinnedRHEL9UpgradeTest"}, + {name: "When the upgrade spec uses the v2 version waiter, it should remove obsolete condition allowances", function: "NodePoolOSImageStreamUpgradeVerificationTest"}, + } { + t.Run(testCase.name, func(t *testing.T) { + t.Parallel() + var scopes []string + for _, declaration := range file.Decls { + function, ok := declaration.(*ast.FuncDecl) + if !ok || function.Name.Name != testCase.function { + continue + } + ast.Inspect(function.Body, func(node ast.Node) bool { + call, ok := node.(*ast.CallExpr) + if !ok { + return true + } + identifier, ok := call.Fun.(*ast.Ident) + if !ok || identifier.Name != "It" { + return true + } + info := &types.Info{Uses: map[*ast.Ident]types.Object{ + identifier: types.NewFunc(token.NoPos, types.NewPackage("github.com/onsi/ginkgo/v2", "ginkgo"), "It", nil), + }} + scopes = append(scopes, registrationScope(&analysis.Pass{TypesInfo: info}, call, testCase.function, map[string]int{})) + return false + }) + } + if len(scopes) != 1 { + t.Fatalf("%s has %d It registrations, want exactly one", testCase.function, len(scopes)) + } + if got := allowlist[filename][scopes[0]]; !slices.Equal(got, wantSymbols) { + t.Errorf("allowances for current callback %s = %v, want %v", scopes[0], got, wantSymbols) + } + for scope := range allowlist[filename] { + if strings.HasPrefix(scope, testCase.function+"/") && scope != scopes[0] { + t.Errorf("obsolete callback allowance %s remains alongside current callback %s", scope, scopes[0]) + } + } + }) + } +} + func TestMemberName(t *testing.T) { t.Parallel() pkg := types.NewPackage(utilPkgPath, "util") diff --git a/hypershift-operator/controllers/nodepool/capi.go b/hypershift-operator/controllers/nodepool/capi.go index 09364d88771d..4e101e15ab99 100644 --- a/hypershift-operator/controllers/nodepool/capi.go +++ b/hypershift-operator/controllers/nodepool/capi.go @@ -48,6 +48,12 @@ const ( globalPSNodeLabel = "hypershift.openshift.io/nodepool-globalps-enabled" ) +// CAPIResult holds condition information computed during CAPI reconciliation. +// The caller is responsible for setting these on the NodePool status. +type CAPIResult struct { + Conditions []hyperv1.NodePoolCondition +} + // CAPI Knows how to reconcile all the CAPI resources for a unique token. // TODO(alberto): consider stronger decoupling from Token by making it an interface // and let nodepool, hostedcluster, and client be fields of CAPI / interface methods. @@ -86,37 +92,37 @@ func newCAPI(token *Token, capiClusterName string) (*CAPI, error) { }, nil } -func (c *CAPI) Reconcile(ctx context.Context) error { +func (c *CAPI) Reconcile(ctx context.Context) (*CAPIResult, error) { log := ctrl.LoggerFrom(ctx) + result := &CAPIResult{} nodePool := c.nodePool if err := c.cleanupMachineTemplates(ctx, log, nodePool, c.controlplaneNamespace); err != nil { - return err + return result, err } if c.nodePool.Spec.Platform.Type == hyperv1.AWSPlatform { if err := c.reconcileAWSMachines(ctx); err != nil { - return err + return result, err } } // Reconcile (Platform)MachineTemplate. template, err := c.machineTemplateBuilders(ctx) if err != nil { - return err + return result, err } - if result, err := c.ApplyManifest(ctx, c.Client, template); err != nil { - return err + if operationResult, err := c.ApplyManifest(ctx, c.Client, template); err != nil { + return result, err } else { - log.Info("Reconciled Machine template", "result", result) + log.Info("Reconciled Machine template", "result", operationResult) } // Check if platform machine template needs to be updated. targetMachineTemplate := template.GetName() if isUpdatingMachineTemplate(nodePool, targetMachineTemplate) { - // TODO (alberto): deocuple all conditions handling from this file into nodepool_controller.go dedicated function. - SetStatusCondition(&nodePool.Status.Conditions, hyperv1.NodePoolCondition{ + result.Conditions = append(result.Conditions, hyperv1.NodePoolCondition{ Type: hyperv1.NodePoolUpdatingPlatformMachineTemplateConditionType, Status: corev1.ConditionTrue, Reason: hyperv1.AsExpectedReason, @@ -127,7 +133,7 @@ func (c *CAPI) Reconcile(ctx context.Context) error { "current", nodePool.GetAnnotations()[nodePoolAnnotationPlatformMachineTemplate], "target", targetMachineTemplate) } else { - SetStatusCondition(&nodePool.Status.Conditions, hyperv1.NodePoolCondition{ + result.Conditions = append(result.Conditions, hyperv1.NodePoolCondition{ Type: hyperv1.NodePoolUpdatingPlatformMachineTemplateConditionType, Status: corev1.ConditionFalse, Reason: hyperv1.AsExpectedReason, @@ -137,51 +143,59 @@ func (c *CAPI) Reconcile(ctx context.Context) error { if nodePool.Spec.Management.UpgradeType == hyperv1.UpgradeTypeInPlace { ms := c.machineSet() - if result, err := c.reconcileWorkload(ctx, ms, func() error { - return c.reconcileMachineSet( + var conditions []hyperv1.NodePoolCondition + operationResult, err := c.reconcileWorkload(ctx, ms, func() error { + var reconcileErr error + conditions, reconcileErr = c.reconcileMachineSet( ctx, ms, template) - }); err != nil { - return fmt.Errorf("failed to reconcile MachineSet %q: %w", + return reconcileErr + }) + result.Conditions = append(result.Conditions, conditions...) + if err != nil { + return result, fmt.Errorf("failed to reconcile MachineSet %q: %w", client.ObjectKeyFromObject(ms).String(), err) - } else { - log.Info("Reconciled MachineSet", "result", result) } + log.Info("Reconciled MachineSet", "result", operationResult) } if nodePool.Spec.Management.UpgradeType == hyperv1.UpgradeTypeReplace { md := c.machineDeployment() - if result, err := c.reconcileWorkload(ctx, md, func() error { - return c.reconcileMachineDeployment( + var conditions []hyperv1.NodePoolCondition + operationResult, err := c.reconcileWorkload(ctx, md, func() error { + var reconcileErr error + conditions, reconcileErr = c.reconcileMachineDeployment( ctx, log, md, template) - }); err != nil { - return fmt.Errorf("failed to reconcile MachineDeployment %q: %w", + return reconcileErr + }) + result.Conditions = append(result.Conditions, conditions...) + if err != nil { + return result, fmt.Errorf("failed to reconcile MachineDeployment %q: %w", client.ObjectKeyFromObject(md).String(), err) - } else { - log.Info("Reconciled MachineDeployment", "result", result) } + log.Info("Reconciled MachineDeployment", "result", operationResult) } mhc := c.machineHealthCheck() if nodePool.Spec.Management.AutoRepair { - if c := FindStatusCondition(nodePool.Status.Conditions, hyperv1.NodePoolReachedIgnitionEndpoint); c == nil || c.Status != corev1.ConditionTrue { + if cond := FindStatusCondition(nodePool.Status.Conditions, hyperv1.NodePoolReachedIgnitionEndpoint); cond == nil || cond.Status != corev1.ConditionTrue { log.Info("ReachedIgnitionEndpoint is false, MachineHealthCheck won't be created until this is true") - return nil + return result, nil } - if result, err := ctrl.CreateOrUpdate(ctx, c.Client, mhc, func() error { + if operationResult, err := ctrl.CreateOrUpdate(ctx, c.Client, mhc, func() error { return c.reconcileMachineHealthCheck(ctx, mhc) }); err != nil { - return fmt.Errorf("failed to reconcile MachineHealthCheck %q: %w", + return result, fmt.Errorf("failed to reconcile MachineHealthCheck %q: %w", client.ObjectKeyFromObject(mhc).String(), err) } else { - log.Info("Reconciled MachineHealthCheck", "result", result) + log.Info("Reconciled MachineHealthCheck", "result", operationResult) } - SetStatusCondition(&nodePool.Status.Conditions, hyperv1.NodePoolCondition{ + result.Conditions = append(result.Conditions, hyperv1.NodePoolCondition{ Type: hyperv1.NodePoolAutorepairEnabledConditionType, Status: corev1.ConditionTrue, Reason: hyperv1.AsExpectedReason, @@ -192,7 +206,7 @@ func (c *CAPI) Reconcile(ctx context.Context) error { // that auto-repair is blocked because too many machines are unhealthy. if remediationAllowed := findMHCRemediationAllowedCondition(mhc.Status.Conditions); remediationAllowed != nil { if remediationAllowed.Status == metav1.ConditionFalse { - SetStatusCondition(&nodePool.Status.Conditions, hyperv1.NodePoolCondition{ + result.Conditions = append(result.Conditions, hyperv1.NodePoolCondition{ Type: hyperv1.NodePoolReadyConditionType, Status: corev1.ConditionFalse, Reason: remediationAllowed.Reason, @@ -204,14 +218,14 @@ func (c *CAPI) Reconcile(ctx context.Context) error { } else { err := c.Get(ctx, client.ObjectKeyFromObject(mhc), mhc) if err != nil && !apierrors.IsNotFound(err) { - return err + return result, err } if err == nil { if err := c.Delete(ctx, mhc); err != nil && !apierrors.IsNotFound(err) { - return err + return result, err } } - SetStatusCondition(&nodePool.Status.Conditions, hyperv1.NodePoolCondition{ + result.Conditions = append(result.Conditions, hyperv1.NodePoolCondition{ Type: hyperv1.NodePoolAutorepairEnabledConditionType, Status: corev1.ConditionFalse, Reason: hyperv1.AsExpectedReason, @@ -222,28 +236,28 @@ func (c *CAPI) Reconcile(ctx context.Context) error { // Reconcile spot-specific MachineHealthCheck when spot instances are enabled spotMHC := c.spotMachineHealthCheck() if isSpotEnabled(nodePool) { - if result, err := ctrl.CreateOrUpdate(ctx, c.Client, spotMHC, func() error { + if operationResult, err := ctrl.CreateOrUpdate(ctx, c.Client, spotMHC, func() error { return c.reconcileSpotMachineHealthCheck(ctx, spotMHC) }); err != nil { - return fmt.Errorf("failed to reconcile spot MachineHealthCheck %q: %w", + return result, fmt.Errorf("failed to reconcile spot MachineHealthCheck %q: %w", client.ObjectKeyFromObject(spotMHC).String(), err) } else { - log.Info("Reconciled spot MachineHealthCheck", "result", result) + log.Info("Reconciled spot MachineHealthCheck", "result", operationResult) } } else { // Delete spot MHC if spot is not enabled err := c.Get(ctx, client.ObjectKeyFromObject(spotMHC), spotMHC) if err != nil && !apierrors.IsNotFound(err) { - return err + return result, err } if err == nil { if err := c.Delete(ctx, spotMHC); err != nil && !apierrors.IsNotFound(err) { - return err + return result, err } } } - return nil + return result, nil } // reconcileWorkload keeps NodePool rollout tracking consistent with the workload @@ -453,7 +467,7 @@ func deleteMachineHealthCheck(ctx context.Context, c client.Client, mhc *capiv1. func (c *CAPI) reconcileMachineDeployment(ctx context.Context, log logr.Logger, machineDeployment *capiv1.MachineDeployment, machineTemplateCR client.Object, -) error { +) ([]hyperv1.NodePoolCondition, error) { nodePool := c.nodePool capiClusterName := c.capiClusterName @@ -475,7 +489,7 @@ func (c *CAPI) reconcileMachineDeployment(ctx context.Context, log logr.Logger, gvk, err := apiutil.GVKForObject(machineTemplateCR, api.Scheme) if err != nil { - return err + return nil, err } machineDeployment.Spec.Template = capiv1.MachineTemplateSpec{ ObjectMeta: capiv1.ObjectMeta{ @@ -516,7 +530,7 @@ func (c *CAPI) reconcileMachineDeployment(ctx context.Context, log logr.Logger, setMachineDeploymentFailureDomain(c.nodePool, machineDeployment) if err := c.propagateLabelsAndTaintsToMachines(ctx, log, machineDeployment); err != nil { - return err + return nil, err } machineDeployment.Spec.Rollout.Strategy.Type = capiv1.MachineDeploymentRolloutStrategyType(nodePool.Spec.Management.Replace.Strategy) @@ -531,12 +545,10 @@ func (c *CAPI) reconcileMachineDeployment(ctx context.Context, log logr.Logger, setMachineDeploymentReplicas(nodePool, machineDeployment, scaleFromZeroSupported) if updated := c.propagateVersionAndTemplate(log, machineDeployment, machineTemplateCR); updated { - return nil + return nil, nil } - c.reconcileMachineDeploymentStatus(ctx, log, machineDeployment, machineTemplateCR) - - return nil + return c.reconcileMachineDeploymentStatus(ctx, log, machineDeployment, machineTemplateCR), nil } func (c *CAPI) setMachineDeploymentMetadata(machineDeployment *capiv1.MachineDeployment, capiClusterName string) { @@ -757,7 +769,7 @@ func (c *CAPI) propagateVersionAndTemplateToMachineSet(log logr.Logger, machineS return specUpdated } -func (c *CAPI) reconcileMachineDeploymentStatus(ctx context.Context, log logr.Logger, machineDeployment *capiv1.MachineDeployment, machineTemplateCR client.Object) { +func (c *CAPI) reconcileMachineDeploymentStatus(ctx context.Context, log logr.Logger, machineDeployment *capiv1.MachineDeployment, machineTemplateCR client.Object) []hyperv1.NodePoolCondition { nodePool := c.nodePool targetVersion := c.Version() targetConfigHash := c.HashWithoutVersion() @@ -775,7 +787,7 @@ func (c *CAPI) reconcileMachineDeploymentStatus(ctx context.Context, log logr.Lo client.MatchingLabels{capiv1.MachineDeploymentNameLabel: machineDeployment.Name}, ); err != nil { log.Error(err, "failed to list MachineSets for MachineDeployment completion check") - return + return nil } // If the MachineDeployment is now processing we know @@ -835,25 +847,28 @@ func (c *CAPI) reconcileMachineDeploymentStatus(ctx context.Context, log logr.Lo } nodePool.Status.Replicas = ptr.Deref(machineDeployment.Status.AvailableReplicas, 0) - for _, c := range machineDeployment.Status.Conditions { - // In CAPI v1beta2 "Ready" was replaced by "MachinesReady" (True when all machines are ready). - // https://github.com/kubernetes-sigs/cluster-api/issues/3486. - if c.Type == capiv1.MachinesReadyCondition { + return nodePoolReadyConditions(machineDeployment.Status.Conditions, nodePool.Generation) +} + +func nodePoolReadyConditions(conditions []metav1.Condition, observedGeneration int64) []hyperv1.NodePoolCondition { + for _, condition := range conditions { + if condition.Type == capiv1.MachinesReadyCondition { reason := hyperv1.AsExpectedReason - if c.Reason != "" { - reason = c.Reason + if condition.Reason != "" { + reason = condition.Reason } - SetStatusCondition(&nodePool.Status.Conditions, hyperv1.NodePoolCondition{ + return []hyperv1.NodePoolCondition{{ Type: hyperv1.NodePoolReadyConditionType, - Status: corev1.ConditionStatus(c.Status), - ObservedGeneration: nodePool.Generation, - Message: c.Message, + Status: corev1.ConditionStatus(condition.Status), + ObservedGeneration: observedGeneration, + Message: condition.Message, Reason: reason, - }) - break + }} } } + + return nil } func taintsToJSON(taints []hyperv1.Taint) (string, error) { @@ -1083,7 +1098,7 @@ func generateMachineTemplateName(nodePool *hyperv1.NodePool, machineTemplateSpec func (c *CAPI) reconcileMachineSet(ctx context.Context, machineSet *capiv1.MachineSet, machineTemplateCR client.Object, -) error { +) ([]hyperv1.NodePoolCondition, error) { nodePool := c.nodePool capiClusterName := c.capiClusterName @@ -1104,13 +1119,13 @@ func (c *CAPI) reconcileMachineSet(ctx context.Context, gvk, err := apiutil.GVKForObject(machineTemplateCR, api.Scheme) if err != nil { - return err + return nil, err } // Set MaxUnavailable for the inplace upgrader to use maxUnavailable, err := getInPlaceMaxUnavailable(nodePool) if err != nil { - return err + return nil, err } machineSet.Annotations[nodePoolAnnotationMaxUnavailable] = strconv.Itoa(maxUnavailable) @@ -1175,7 +1190,7 @@ func (c *CAPI) reconcileMachineSet(ctx context.Context, // Propagate taints. taintsInJSON, err := taintsToJSON(nodePool.Spec.Taints) if err != nil { - return err + return nil, err } machineSet.Spec.Template.Annotations[nodePoolAnnotationTaints] = taintsInJSON @@ -1183,40 +1198,15 @@ func (c *CAPI) reconcileMachineSet(ctx context.Context, setMachineSetReplicas(nodePool, machineSet, scaleFromZeroSupported) if c.propagateVersionAndTemplateToMachineSet(log, machineSet, machineTemplateCR) { - // Return early during a version/config/MachineTemplate update to persist - // the resource, so the next reconcile loop sees a new Generation. - return nil + return nil, nil } if machineSetInPlaceRolloutIsComplete(machineSet) { c.reconcileMachineSetStatus(log, machineSet, machineTemplateCR) } - // Bubble up AvailableReplicas and Ready condition from MachineSet. nodePool.Status.Replicas = ptr.Deref(machineSet.Status.AvailableReplicas, 0) - for _, cond := range machineSet.Status.Conditions { - // In CAPI v1beta2 "Ready" was replaced by "MachinesReady" (True when all machines are ready). - // https://github.com/kubernetes-sigs/cluster-api/issues/3486. - if cond.Type == capiv1.MachinesReadyCondition { - // this is so api server does not complain - // invalid value: \"\": status.conditions.reason in body should be at least 1 chars long" - reason := hyperv1.AsExpectedReason - if cond.Reason != "" { - reason = cond.Reason - } - - SetStatusCondition(&nodePool.Status.Conditions, hyperv1.NodePoolCondition{ - Type: hyperv1.NodePoolReadyConditionType, - Status: corev1.ConditionStatus(cond.Status), - ObservedGeneration: nodePool.Generation, - Message: cond.Message, - Reason: reason, - }) - break - } - } - - return nil + return nodePoolReadyConditions(machineSet.Status.Conditions, nodePool.Generation), nil } func (c *CAPI) reconcileMachineSetStatus(log logr.Logger, machineSet *capiv1.MachineSet, machineTemplateCR client.Object) { diff --git a/hypershift-operator/controllers/nodepool/capi_test.go b/hypershift-operator/controllers/nodepool/capi_test.go index 4e5c7ee26994..8a3ec89bb330 100644 --- a/hypershift-operator/controllers/nodepool/capi_test.go +++ b/hypershift-operator/controllers/nodepool/capi_test.go @@ -3,6 +3,7 @@ package nodepool import ( "context" "encoding/json" + "errors" "fmt" "testing" "time" @@ -1312,6 +1313,7 @@ func TestReconcileMachineHealthCheck(t *testing.T) { g.Expect(mhc.Spec).To(testutil.MatchExpected(tt.expected.Spec)) }) } + } func TestCAPIReconcile(t *testing.T) { @@ -1629,6 +1631,95 @@ func TestCAPIReconcile(t *testing.T) { }, expectedError: false, }, + { + name: "When auto repair is enabled but ReachedIgnitionEndpoint is not true, it should return early with accumulated conditions", + nodePool: &hyperv1.NodePool{ + ObjectMeta: metav1.ObjectMeta{ + Name: "test-nodepool", + Namespace: "test-namespace", + }, + Spec: hyperv1.NodePoolSpec{ + ClusterName: "test-cluster", + Management: hyperv1.NodePoolManagement{ + UpgradeType: hyperv1.UpgradeTypeReplace, + Replace: &hyperv1.ReplaceUpgrade{ + Strategy: hyperv1.UpgradeStrategyRollingUpdate, + RollingUpdate: &hyperv1.RollingUpdate{ + MaxUnavailable: &maxUnavailable, + MaxSurge: &maxSurge, + }, + }, + AutoRepair: true, + }, + Replicas: ptr.To[int32](3), + Platform: hyperv1.NodePoolPlatform{ + Type: hyperv1.AWSPlatform, + AWS: &hyperv1.AWSNodePoolPlatform{ + AMI: "an-ami", + }, + }, + }, + }, + hostedCluster: &hyperv1.HostedCluster{ + ObjectMeta: metav1.ObjectMeta{Name: "test-cluster", Namespace: "test-namespace"}, + Spec: hyperv1.HostedClusterSpec{ + Platform: hyperv1.PlatformSpec{ + Type: hyperv1.AWSPlatform, + AWS: &hyperv1.AWSPlatformSpec{ + Region: "", + CloudProviderConfig: &hyperv1.AWSCloudProviderConfig{}, + ServiceEndpoints: []hyperv1.AWSServiceEndpoint{}, + RolesRef: hyperv1.AWSRolesRef{}, + ResourceTags: []hyperv1.AWSClusterResourceTag{}, + EndpointAccess: "", + AdditionalAllowedPrincipals: []string{}, + MultiArch: false, + }, + }, + }, + }, + machineSet: &capiv1.MachineSet{ + ObjectMeta: metav1.ObjectMeta{ + Name: "test-machineset", + Namespace: "test-namespace-test-cluster", + Annotations: map[string]string{ + nodePoolAnnotation: "test-namespace/test-nodepool", + }, + }, + Spec: capiv1.MachineSetSpec{ + Template: capiv1.MachineTemplateSpec{ + Spec: capiv1.MachineSpec{ + InfrastructureRef: capiv1.ContractVersionedObjectReference{ + Kind: "AWSMachineTemplate", + APIGroup: "infrastructure.cluster.x-k8s.io", + Name: awsMachineTemplateName, + }, + }, + }, + }, + }, + templates: []client.Object{ + &capiaws.AWSMachineTemplate{ + ObjectMeta: metav1.ObjectMeta{ + Name: "does-not-match-infra-ref-name", + Namespace: "test-namespace-test-cluster", + Annotations: map[string]string{ + nodePoolAnnotation: "test-namespace/test-nodepool", + }, + }, + }, + &capiaws.AWSMachineTemplate{ + ObjectMeta: metav1.ObjectMeta{ + Name: awsMachineTemplateName, + Namespace: "test-namespace-test-cluster", + Annotations: map[string]string{ + nodePoolAnnotation: "test-namespace/test-nodepool", + }, + }, + }, + }, + expectedError: false, + }, // { // name: "error during machine template cleanup", // nodePool: &hyperv1.NodePool{ @@ -1837,6 +1928,7 @@ func TestCAPIReconcile(t *testing.T) { t.Run(tt.name, func(t *testing.T) { t.Parallel() g := NewWithT(t) + tt.nodePool.Generation = 7 c := fake.NewClientBuilder(). WithScheme(api.Scheme). // WithObjectTracker() @@ -1876,11 +1968,65 @@ func TestCAPIReconcile(t *testing.T) { g.Expect(err).NotTo(HaveOccurred()) g.Expect(templateList.Items).To(HaveLen(2)) - err = capi.Reconcile(t.Context()) + capiResult, err := capi.Reconcile(t.Context()) if tt.expectedError { - g.Expect(err).To(HaveOccurred()) + g.Expect(err).To(HaveOccurred(), "CAPI reconciliation should return the expected error") } else { - g.Expect(err).NotTo(HaveOccurred()) + g.Expect(err).NotTo(HaveOccurred(), "failed to reconcile CAPI for the initial workload") + g.Expect(capiResult).NotTo(BeNil(), "CAPI reconciliation should return a nonnil result") + expectedConditions := []hyperv1.NodePoolCondition{{ + Type: hyperv1.NodePoolUpdatingPlatformMachineTemplateConditionType, + Status: corev1.ConditionTrue, + Reason: hyperv1.AsExpectedReason, + Message: fmt.Sprintf("platform machine template update in progress. Target template: %s", awsMachineTemplateName), + ObservedGeneration: tt.nodePool.Generation, + }} + reachedIgnition := FindStatusCondition(tt.nodePool.Status.Conditions, hyperv1.NodePoolReachedIgnitionEndpoint) + if !tt.nodePool.Spec.Management.AutoRepair { + expectedConditions = append(expectedConditions, hyperv1.NodePoolCondition{ + Type: hyperv1.NodePoolAutorepairEnabledConditionType, + Status: corev1.ConditionFalse, + Reason: hyperv1.AsExpectedReason, + ObservedGeneration: tt.nodePool.Generation, + }) + } else if reachedIgnition != nil && reachedIgnition.Status == corev1.ConditionTrue { + expectedConditions = append(expectedConditions, hyperv1.NodePoolCondition{ + Type: hyperv1.NodePoolAutorepairEnabledConditionType, + Status: corev1.ConditionTrue, + Reason: hyperv1.AsExpectedReason, + ObservedGeneration: tt.nodePool.Generation, + }) + } + g.Expect(capiResult.Conditions).To(Equal(expectedConditions), "initial reconciliation should return platform-update and auto-repair conditions in order with the current generation") + if tt.nodePool.Spec.Management.AutoRepair && reachedIgnition == nil { + for _, ignitionTest := range []struct { + name string + status corev1.ConditionStatus + }{ + { + name: "When ReachedIgnitionEndpoint is false, it should preserve prior conditions and skip MHC reconciliation", + status: corev1.ConditionFalse, + }, + { + name: "When ReachedIgnitionEndpoint is unknown, it should preserve prior conditions and skip MHC reconciliation", + status: corev1.ConditionUnknown, + }, + } { + t.Run(ignitionTest.name, func(t *testing.T) { + g := NewWithT(t) + tt.nodePool.Status.Conditions = []hyperv1.NodePoolCondition{{ + Type: hyperv1.NodePoolReachedIgnitionEndpoint, + Status: ignitionTest.status, + }} + repeatedResult, repeatedErr := capi.Reconcile(t.Context()) + g.Expect(repeatedErr).NotTo(HaveOccurred(), "failed to reconcile CAPI while ignition is not reached") + g.Expect(repeatedResult.Conditions).To(Equal(expectedConditions), "repeated reconciliation before ignition should preserve the accumulated conditions") + mhc := &capiv1.MachineHealthCheck{} + err := capi.Client.Get(t.Context(), client.ObjectKey{Namespace: controlpaneNamespace, Name: tt.nodePool.GetName()}, mhc) + g.Expect(apierrors.IsNotFound(err)).To(BeTrue(), "MachineHealthCheck should remain absent until ignition is reached") + }) + } + } // Check that old machine templates are deleted. templateList := &capiaws.AWSMachineTemplateList{} @@ -1938,15 +2084,20 @@ func TestCAPIReconcile(t *testing.T) { } // Check MachineHealthCheck - if tt.nodePool.Spec.Management.AutoRepair { + if tt.nodePool.Spec.Management.AutoRepair && reachedIgnition != nil && reachedIgnition.Status == corev1.ConditionTrue { mhc := &capiv1.MachineHealthCheck{} err = capi.Client.Get(t.Context(), client.ObjectKey{Namespace: controlpaneNamespace, Name: tt.nodePool.GetName()}, mhc) - g.Expect(err).NotTo(HaveOccurred()) - g.Expect(mhc.Spec.ClusterName).To(Equal(capiClusterName)) + g.Expect(err).NotTo(HaveOccurred(), "failed to retrieve the MachineHealthCheck after ignition was reached") + g.Expect(mhc.Spec.ClusterName).To(Equal(capiClusterName), "MachineHealthCheck should target the expected CAPI cluster") + } else if tt.nodePool.Spec.Management.AutoRepair { + // AutoRepair is enabled but ReachedIgnitionEndpoint is not true — early return path. + mhc := &capiv1.MachineHealthCheck{} + err = capi.Client.Get(t.Context(), client.ObjectKey{Namespace: controlpaneNamespace, Name: tt.nodePool.GetName()}, mhc) + g.Expect(apierrors.IsNotFound(err)).To(BeTrue(), "MHC should not be created when ReachedIgnitionEndpoint is not true") } else { mhc := &capiv1.MachineHealthCheck{} - err = capi.Client.Get(t.Context(), client.ObjectKey{Namespace: "test-cp-namespace", Name: tt.nodePool.GetName()}, mhc) - g.Expect(apierrors.IsNotFound(err)).To(BeTrue()) + err = capi.Client.Get(t.Context(), client.ObjectKey{Namespace: controlpaneNamespace, Name: tt.nodePool.GetName()}, mhc) + g.Expect(apierrors.IsNotFound(err)).To(BeTrue(), "MachineHealthCheck should remain absent when auto-repair is disabled") } // Check spot MHC and interruptible label @@ -1989,8 +2140,10 @@ func TestCAPIReconcile(t *testing.T) { g.Expect(err).NotTo(HaveOccurred()) // Re-run reconcile. - err = capi.Reconcile(t.Context()) - g.Expect(err).NotTo(HaveOccurred()) + capiResult, err = capi.Reconcile(t.Context()) + g.Expect(err).NotTo(HaveOccurred(), "failed to reconcile CAPI after MachineDeployment completion") + g.Expect(capiResult).NotTo(BeNil(), "reconciliation after MachineDeployment completion should return a nonnil result") + g.Expect(capiResult.Conditions).To(Equal(expectedConditions), "MachineDeployment completion should preserve the returned condition fields and ordering") // Check for the expected annotations. // TODO(alberto): reconcileMachineDeployment mutate the NodePool with this annotations and status.version. @@ -2008,6 +2161,9 @@ func TestCAPIReconcile(t *testing.T) { } }) } + + t.Run("When an InPlace MachineSet reports readiness repeatedly, it should return exact invocation-local conditions", testCAPIReconcileMachineSetReadyCondition) + t.Run("When a later CAPI operation fails, it should return every condition accumulated before the error", testCAPIReconcileConditionErrorPaths) } // TestCAPIReconcile_machineset is specific for UpgradeTypeInPlace. @@ -2074,6 +2230,7 @@ func TestCAPIReconcile_machineset(t *testing.T) { t.Run(tt.name, func(t *testing.T) { t.Parallel() g := NewWithT(t) + tt.nodePool.Generation = 7 hostedCluster := &hyperv1.HostedCluster{ ObjectMeta: metav1.ObjectMeta{Name: "test-cluster", Namespace: "test-namespace"}, @@ -2158,14 +2315,503 @@ func TestCAPIReconcile_machineset(t *testing.T) { ApplyProvider: upsert.NewApplyProvider(false), } - err := capi.Reconcile(t.Context()) - g.Expect(err).NotTo(HaveOccurred()) + capiResult, err := capi.Reconcile(t.Context()) + g.Expect(err).NotTo(HaveOccurred(), "failed to reconcile the initial InPlace MachineSet") ms := &capiv1.MachineSet{} err = capi.Client.Get(t.Context(), client.ObjectKey{Namespace: controlplaneNamespace, Name: tt.nodePool.GetName()}, ms) - g.Expect(err).NotTo(HaveOccurred()) - g.Expect(ms.Spec.Template.Spec.Deletion.NodeDrainTimeoutSeconds).To(Equal(durationToSeconds(tt.nodePool.Spec.NodeDrainTimeout))) - g.Expect(ms.Spec.Template.Spec.Deletion.NodeVolumeDetachTimeoutSeconds).To(Equal(durationToSeconds(tt.nodePool.Spec.NodeVolumeDetachTimeout))) + g.Expect(err).NotTo(HaveOccurred(), "failed to retrieve the initial InPlace MachineSet") + g.Expect(ms.Spec.Template.Spec.Deletion.NodeDrainTimeoutSeconds).To(Equal(durationToSeconds(tt.nodePool.Spec.NodeDrainTimeout)), "MachineSet should preserve the configured node drain timeout") + g.Expect(ms.Spec.Template.Spec.Deletion.NodeVolumeDetachTimeoutSeconds).To(Equal(durationToSeconds(tt.nodePool.Spec.NodeVolumeDetachTimeout)), "MachineSet should preserve the configured volume detach timeout") + + g.Expect(capiResult).NotTo(BeNil(), "initial MachineSet reconciliation should return a nonnil result") + g.Expect(capiResult.Conditions).To(Equal([]hyperv1.NodePoolCondition{ + { + Type: hyperv1.NodePoolUpdatingPlatformMachineTemplateConditionType, + Status: corev1.ConditionTrue, + Reason: hyperv1.AsExpectedReason, + Message: fmt.Sprintf("platform machine template update in progress. Target template: %s", awsMachineTemplateName), + ObservedGeneration: tt.nodePool.Generation, + }, + { + Type: hyperv1.NodePoolAutorepairEnabledConditionType, + Status: corev1.ConditionFalse, + Reason: hyperv1.AsExpectedReason, + ObservedGeneration: tt.nodePool.Generation, + }, + }), "initial MachineSet reconciliation should return platform-update and disabled auto-repair conditions") + }) + } +} + +func newCAPIReconcileTestCAPI(t *testing.T, nodePool *hyperv1.NodePool, hostedCluster *hyperv1.HostedCluster) (*CAPI, string) { + t.Helper() + + controlPlaneNamespace := manifests.HostedControlPlaneNamespace(hostedCluster.Namespace, hostedCluster.Name) + return &CAPI{ + Token: &Token{ + ConfigGenerator: &ConfigGenerator{ + hostedCluster: hostedCluster, + nodePool: nodePool, + controlplaneNamespace: controlPlaneNamespace, + rolloutConfig: &rolloutConfig{releaseImage: &releaseinfo.ReleaseImage{ + ImageStream: &imageapi.ImageStream{ObjectMeta: metav1.ObjectMeta{Name: "target-version"}}, + }}, + }, + cpoCapabilities: &CPOCapabilities{}, + CreateOrUpdateProvider: upsert.New(false), + }, + capiClusterName: "infra-id", + ApplyProvider: upsert.NewApplyProvider(false), + }, controlPlaneNamespace +} + +func testCAPIReconcileMachineSetReadyCondition(t *testing.T) { + t.Helper() + g := NewWithT(t) + + awsMachineTemplateName := "test-nodepool-28d5cf5a" + nodePool := &hyperv1.NodePool{ + ObjectMeta: metav1.ObjectMeta{ + Name: "test-nodepool", + Namespace: "test-namespace", + Generation: 11, + }, + Spec: hyperv1.NodePoolSpec{ + ClusterName: "test-cluster", + Management: hyperv1.NodePoolManagement{ + UpgradeType: hyperv1.UpgradeTypeInPlace, + InPlace: &hyperv1.InPlaceUpgrade{}, + }, + Replicas: ptr.To[int32](3), + Platform: hyperv1.NodePoolPlatform{ + Type: hyperv1.AWSPlatform, + AWS: &hyperv1.AWSNodePoolPlatform{ + AMI: "an-ami", + }, + }, + }, + } + + hostedCluster := &hyperv1.HostedCluster{ + ObjectMeta: metav1.ObjectMeta{Name: "test-cluster", Namespace: "test-namespace"}, + Spec: hyperv1.HostedClusterSpec{ + Platform: hyperv1.PlatformSpec{ + Type: hyperv1.AWSPlatform, + AWS: &hyperv1.AWSPlatformSpec{ + Region: "", + CloudProviderConfig: &hyperv1.AWSCloudProviderConfig{}, + ServiceEndpoints: []hyperv1.AWSServiceEndpoint{}, + RolesRef: hyperv1.AWSRolesRef{}, + ResourceTags: []hyperv1.AWSClusterResourceTag{}, + EndpointAccess: "", + AdditionalAllowedPrincipals: []string{}, + MultiArch: false, + }, + }, + }, + } + + existingMachineSet := &capiv1.MachineSet{ + ObjectMeta: metav1.ObjectMeta{ + Name: nodePool.GetName(), + Namespace: "test-namespace-test-cluster", + Annotations: map[string]string{ + nodePoolAnnotation: "test-namespace/test-nodepool", + }, + }, + Spec: capiv1.MachineSetSpec{ + Template: capiv1.MachineTemplateSpec{ + Spec: capiv1.MachineSpec{ + InfrastructureRef: capiv1.ContractVersionedObjectReference{ + Kind: "AWSMachineTemplate", + APIGroup: "infrastructure.cluster.x-k8s.io", + Name: awsMachineTemplateName, + }, + }, + }, + }, + } + + templates := []client.Object{ + &capiaws.AWSMachineTemplate{ + ObjectMeta: metav1.ObjectMeta{ + Name: awsMachineTemplateName, + Namespace: "test-namespace-test-cluster", + Annotations: map[string]string{ + nodePoolAnnotation: "test-namespace/test-nodepool", + }, + }, + }, + } + + capi, controlplaneNamespace := newCAPIReconcileTestCAPI(t, nodePool, hostedCluster) + nodePool.Status.Conditions = []hyperv1.NodePoolCondition{{ + Type: hyperv1.NodePoolReadyConditionType, + Status: corev1.ConditionUnknown, + Reason: "PreviousReadiness", + Message: "waiting for reconciliation", + ObservedGeneration: nodePool.Generation - 1, + LastTransitionTime: metav1.NewTime(time.Unix(100, 0)), + }} + originalConditions := append([]hyperv1.NodePoolCondition(nil), nodePool.Status.Conditions...) + capi.Client = fake.NewClientBuilder(). + WithScheme(api.Scheme). + WithObjects(nodePool, existingMachineSet). + WithObjects(templates...). + Build() + + // First reconcile: MachineSet gets updated with user data, version, and config. + _, err := capi.Reconcile(t.Context()) + g.Expect(err).NotTo(HaveOccurred(), "failed to reconcile the initial MachineSet") + + // Update MachineSet status with a ReadyCondition to exercise the condition propagation path. + ms := &capiv1.MachineSet{} + err = capi.Client.Get(t.Context(), client.ObjectKey{Namespace: controlplaneNamespace, Name: nodePool.GetName()}, ms) + g.Expect(err).NotTo(HaveOccurred(), "failed to retrieve the reconciled MachineSet") + + ms.Status.AvailableReplicas = ptr.To[int32](3) + ms.Status.Conditions = append(ms.Status.Conditions, metav1.Condition{ + Type: capiv1.MachinesReadyCondition, + Status: metav1.ConditionTrue, + Reason: "MachinesReady", + Message: "All machines are ready", + }) + err = capi.Client.Update(t.Context(), ms) + g.Expect(err).NotTo(HaveOccurred(), "failed to update MachineSet readiness status") + + // Second reconcile: MachineSet now has matching data, so isUpdating=false + // and the MachinesReady condition from MachineSet status is propagated to CAPIResult. + capiResult, err := capi.Reconcile(t.Context()) + g.Expect(err).NotTo(HaveOccurred(), "failed to reconcile the ready MachineSet") + g.Expect(capiResult).NotTo(BeNil(), "ready MachineSet reconciliation should return a nonnil result") + g.Expect(capiResult.Conditions).To(Equal([]hyperv1.NodePoolCondition{ + { + Type: hyperv1.NodePoolUpdatingPlatformMachineTemplateConditionType, + Status: corev1.ConditionTrue, + Reason: hyperv1.AsExpectedReason, + Message: fmt.Sprintf("platform machine template update in progress. Target template: %s", awsMachineTemplateName), + ObservedGeneration: nodePool.Generation, + }, + { + Type: hyperv1.NodePoolReadyConditionType, + Status: corev1.ConditionTrue, + Reason: "MachinesReady", + Message: "All machines are ready", + ObservedGeneration: nodePool.Generation, + }, + { + Type: hyperv1.NodePoolAutorepairEnabledConditionType, + Status: corev1.ConditionFalse, + Reason: hyperv1.AsExpectedReason, + ObservedGeneration: nodePool.Generation, + }, + }), "ready MachineSet should return ordered platform-update, Ready and auto-repair conditions with the NodePool generation") + + // A third reconcile on the same CAPI instance proves the result is local to each call. + // It also covers the empty-reason fallback and the non-updating platform-template condition. + err = capi.Client.Get(t.Context(), client.ObjectKey{Namespace: controlplaneNamespace, Name: nodePool.GetName()}, ms) + g.Expect(err).NotTo(HaveOccurred(), "failed to retrieve the MachineSet before changing readiness") + ms.Status.Conditions[0].Status = metav1.ConditionFalse + ms.Status.Conditions[0].Reason = "" + ms.Status.Conditions[0].Message = "machines are starting" + g.Expect(capi.Client.Update(t.Context(), ms)).To(Succeed(), "failed to update MachineSet status with an empty readiness reason") + + capiResult, err = capi.Reconcile(t.Context()) + g.Expect(err).NotTo(HaveOccurred(), "failed to reconcile the MachineSet with an empty readiness reason") + g.Expect(capiResult.Conditions).To(Equal([]hyperv1.NodePoolCondition{ + { + Type: hyperv1.NodePoolUpdatingPlatformMachineTemplateConditionType, + Status: corev1.ConditionFalse, + Reason: hyperv1.AsExpectedReason, + ObservedGeneration: nodePool.Generation, + }, + { + Type: hyperv1.NodePoolReadyConditionType, + Status: corev1.ConditionFalse, + Reason: hyperv1.AsExpectedReason, + Message: "machines are starting", + ObservedGeneration: nodePool.Generation, + }, + { + Type: hyperv1.NodePoolAutorepairEnabledConditionType, + Status: corev1.ConditionFalse, + Reason: hyperv1.AsExpectedReason, + ObservedGeneration: nodePool.Generation, + }, + }), "repeated MachineSet reconciliation should return fresh conditions with AsExpected fallback and the updated readiness message") + + ms.Status.Conditions = nil + g.Expect(capi.Client.Update(t.Context(), ms)).To(Succeed(), "failed to remove MachineSet readiness conditions") + capiResult, err = capi.Reconcile(t.Context()) + g.Expect(err).NotTo(HaveOccurred(), "failed to reconcile the MachineSet without MachinesReady") + g.Expect(capiResult).NotTo(BeNil(), "MachineSet reconciliation without readiness should return a nonnil result") + g.Expect(capiResult.Conditions).To(Equal([]hyperv1.NodePoolCondition{ + { + Type: hyperv1.NodePoolUpdatingPlatformMachineTemplateConditionType, + Status: corev1.ConditionFalse, + Reason: hyperv1.AsExpectedReason, + ObservedGeneration: nodePool.Generation, + }, + { + Type: hyperv1.NodePoolAutorepairEnabledConditionType, + Status: corev1.ConditionFalse, + Reason: hyperv1.AsExpectedReason, + ObservedGeneration: nodePool.Generation, + }, + }), "MachineSet without MachinesReady should omit Ready while preserving other condition fields and ordering") + g.Expect(nodePool.Status.Replicas).To(Equal(int32(3)), "MachineSet readiness conversion should preserve available replica propagation") + g.Expect(nodePool.Status.Conditions).To(Equal(originalConditions), "direct CAPI reconciliation should return conditions without mutating existing NodePool conditions") +} + +func testCAPIReconcileConditionErrorPaths(t *testing.T) { + t.Helper() + + const machineTemplateName = "test-nodepool-28d5cf5a" + injectedErr := errors.New("injected reconciliation failure") + + tests := []struct { + name string + upgradeType hyperv1.UpgradeType + autoRepair bool + spot bool + interceptors interceptor.Funcs + expectedTypes []string + }{ + { + name: "When MachineTemplate retrieval fails, it should return an empty result", + upgradeType: hyperv1.UpgradeTypeReplace, + interceptors: interceptor.Funcs{ + Get: func(ctx context.Context, c client.WithWatch, key client.ObjectKey, obj client.Object, opts ...client.GetOption) error { + if _, ok := obj.(*capiaws.AWSMachineTemplate); ok && key.Name == machineTemplateName { + return injectedErr + } + return c.Get(ctx, key, obj, opts...) + }, + }, + }, + { + name: "When MachineDeployment persistence fails after status reconciliation, it should return the accumulated conditions", + upgradeType: hyperv1.UpgradeTypeReplace, + interceptors: interceptor.Funcs{ + Patch: func(ctx context.Context, c client.WithWatch, obj client.Object, patch client.Patch, opts ...client.PatchOption) error { + if _, ok := obj.(*capiv1.MachineDeployment); ok { + return injectedErr + } + return c.Patch(ctx, obj, patch, opts...) + }, + }, + expectedTypes: []string{ + hyperv1.NodePoolUpdatingPlatformMachineTemplateConditionType, + hyperv1.NodePoolReadyConditionType, + }, + }, + { + name: "When MachineSet persistence fails after status reconciliation, it should return the accumulated conditions", + upgradeType: hyperv1.UpgradeTypeInPlace, + interceptors: interceptor.Funcs{ + Patch: func(ctx context.Context, c client.WithWatch, obj client.Object, patch client.Patch, opts ...client.PatchOption) error { + if _, ok := obj.(*capiv1.MachineSet); ok { + return injectedErr + } + return c.Patch(ctx, obj, patch, opts...) + }, + }, + expectedTypes: []string{ + hyperv1.NodePoolUpdatingPlatformMachineTemplateConditionType, + hyperv1.NodePoolReadyConditionType, + }, + }, + { + name: "When normal MachineHealthCheck creation fails, it should preserve readiness without fabricating autorepair success", + upgradeType: hyperv1.UpgradeTypeReplace, + autoRepair: true, + interceptors: interceptor.Funcs{ + Create: func(ctx context.Context, c client.WithWatch, obj client.Object, opts ...client.CreateOption) error { + if mhc, ok := obj.(*capiv1.MachineHealthCheck); ok && mhc.Name == "test-nodepool" { + return injectedErr + } + return c.Create(ctx, obj, opts...) + }, + }, + expectedTypes: []string{ + hyperv1.NodePoolUpdatingPlatformMachineTemplateConditionType, + hyperv1.NodePoolReadyConditionType, + }, + }, + { + name: "When disabled-autorepair MachineHealthCheck retrieval fails, it should not fabricate autorepair disabled", + upgradeType: hyperv1.UpgradeTypeReplace, + interceptors: interceptor.Funcs{ + Get: func(ctx context.Context, c client.WithWatch, key client.ObjectKey, obj client.Object, opts ...client.GetOption) error { + if _, ok := obj.(*capiv1.MachineHealthCheck); ok && key.Name == "test-nodepool" { + return injectedErr + } + return c.Get(ctx, key, obj, opts...) + }, + }, + expectedTypes: []string{ + hyperv1.NodePoolUpdatingPlatformMachineTemplateConditionType, + hyperv1.NodePoolReadyConditionType, + }, + }, + { + name: "When spot MachineHealthCheck creation fails, it should retain earlier readiness and autorepair conditions", + upgradeType: hyperv1.UpgradeTypeReplace, + autoRepair: true, + spot: true, + interceptors: interceptor.Funcs{ + Create: func(ctx context.Context, c client.WithWatch, obj client.Object, opts ...client.CreateOption) error { + if mhc, ok := obj.(*capiv1.MachineHealthCheck); ok && mhc.Name == "test-nodepool-spot" { + return injectedErr + } + return c.Create(ctx, obj, opts...) + }, + }, + expectedTypes: []string{ + hyperv1.NodePoolUpdatingPlatformMachineTemplateConditionType, + hyperv1.NodePoolReadyConditionType, + hyperv1.NodePoolAutorepairEnabledConditionType, + }, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + t.Parallel() + g := NewWithT(t) + + nodePool := &hyperv1.NodePool{ + ObjectMeta: metav1.ObjectMeta{ + Name: "test-nodepool", + Namespace: "test-namespace", + Generation: 9, + Annotations: map[string]string{ + nodePoolAnnotationPlatformMachineTemplate: machineTemplateName, + }, + }, + Spec: hyperv1.NodePoolSpec{ + ClusterName: "test-cluster", + Replicas: ptr.To[int32](3), + Management: hyperv1.NodePoolManagement{ + UpgradeType: tt.upgradeType, + AutoRepair: tt.autoRepair, + }, + Platform: hyperv1.NodePoolPlatform{ + Type: hyperv1.AWSPlatform, + AWS: &hyperv1.AWSNodePoolPlatform{AMI: "an-ami"}, + }, + }, + } + if tt.upgradeType == hyperv1.UpgradeTypeReplace { + nodePool.Spec.Management.Replace = &hyperv1.ReplaceUpgrade{Strategy: hyperv1.UpgradeStrategyRollingUpdate} + } else { + nodePool.Spec.Management.InPlace = &hyperv1.InPlaceUpgrade{} + } + if tt.autoRepair { + nodePool.Status.Conditions = []hyperv1.NodePoolCondition{{ + Type: hyperv1.NodePoolReachedIgnitionEndpoint, + Status: corev1.ConditionTrue, + }} + } + if tt.spot { + nodePool.Annotations[AnnotationEnableSpot] = "true" + } + + hostedCluster := &hyperv1.HostedCluster{ + ObjectMeta: metav1.ObjectMeta{Name: "test-cluster", Namespace: "test-namespace"}, + Spec: hyperv1.HostedClusterSpec{Platform: hyperv1.PlatformSpec{ + Type: hyperv1.AWSPlatform, + AWS: &hyperv1.AWSPlatformSpec{ + CloudProviderConfig: &hyperv1.AWSCloudProviderConfig{}, + }, + }}, + } + capi, controlPlaneNamespace := newCAPIReconcileTestCAPI(t, nodePool, hostedCluster) + + objects := []client.Object{ + nodePool, + &capiaws.AWSMachineTemplate{ObjectMeta: metav1.ObjectMeta{ + Name: machineTemplateName, + Namespace: controlPlaneNamespace, + Annotations: map[string]string{nodePoolAnnotation: client.ObjectKeyFromObject(nodePool).String()}, + }}, + } + readyCondition := metav1.Condition{ + Type: capiv1.MachinesReadyCondition, + Status: metav1.ConditionTrue, + Reason: "MachinesReady", + Message: "all machines are ready", + } + if tt.upgradeType == hyperv1.UpgradeTypeReplace { + objects = append(objects, &capiv1.MachineDeployment{ + ObjectMeta: metav1.ObjectMeta{Name: nodePool.Name, Namespace: controlPlaneNamespace}, + Spec: capiv1.MachineDeploymentSpec{Template: capiv1.MachineTemplateSpec{Spec: capiv1.MachineSpec{ + Bootstrap: capiv1.Bootstrap{DataSecretName: ptr.To(capi.UserDataSecret().Name)}, + InfrastructureRef: capiv1.ContractVersionedObjectReference{Name: machineTemplateName}, + Version: capi.Version(), + }}}, + Status: capiv1.MachineDeploymentStatus{ + AvailableReplicas: ptr.To[int32](3), + Conditions: []metav1.Condition{readyCondition}, + }, + }) + } else { + configVersion := capi.Hash() + objects = append(objects, &capiv1.MachineSet{ + ObjectMeta: metav1.ObjectMeta{ + Name: nodePool.Name, + Namespace: controlPlaneNamespace, + Annotations: map[string]string{ + nodePoolAnnotationCurrentConfigVersion: configVersion, + nodePoolAnnotationTargetConfigVersion: configVersion, + }, + }, + Spec: capiv1.MachineSetSpec{Template: capiv1.MachineTemplateSpec{Spec: capiv1.MachineSpec{ + Bootstrap: capiv1.Bootstrap{DataSecretName: ptr.To(capi.UserDataSecret().Name)}, + InfrastructureRef: capiv1.ContractVersionedObjectReference{Name: machineTemplateName}, + Version: capi.Version(), + }}}, + Status: capiv1.MachineSetStatus{ + AvailableReplicas: ptr.To[int32](3), + Conditions: []metav1.Condition{readyCondition}, + }, + }) + } + if tt.autoRepair && tt.spot { + objects = append(objects, &capiv1.MachineHealthCheck{ + ObjectMeta: metav1.ObjectMeta{Name: nodePool.Name, Namespace: controlPlaneNamespace}, + }) + } + + capi.Client = fake.NewClientBuilder(). + WithScheme(api.Scheme). + WithObjects(objects...). + WithInterceptorFuncs(tt.interceptors). + Build() + + result, err := capi.Reconcile(ctrl.LoggerInto(t.Context(), logr.Discard())) + g.Expect(err).To(MatchError(ContainSubstring(injectedErr.Error())), "CAPI reconciliation should preserve the injected operation error") + g.Expect(result).NotTo(BeNil(), "failed CAPI reconciliation should still return a nonnil result") + g.Expect(result.Conditions).To(HaveLen(len(tt.expectedTypes)), "failed reconciliation should return exactly the conditions computed before the error") + for i, conditionType := range tt.expectedTypes { + expected := hyperv1.NodePoolCondition{ + Type: conditionType, + Reason: hyperv1.AsExpectedReason, + ObservedGeneration: nodePool.Generation, + } + switch conditionType { + case hyperv1.NodePoolUpdatingPlatformMachineTemplateConditionType: + expected.Status = corev1.ConditionFalse + case hyperv1.NodePoolReadyConditionType: + expected.Status = corev1.ConditionTrue + expected.Reason = "MachinesReady" + expected.Message = "all machines are ready" + case hyperv1.NodePoolAutorepairEnabledConditionType: + expected.Status = corev1.ConditionTrue + } + g.Expect(result.Conditions[i]).To(Equal(expected), "condition %d should preserve type, status, reason, message and observed generation before the error", i) + } }) } } @@ -2183,7 +2829,7 @@ func TestGlobalPSManagedLabelOnMachines(t *testing.T) { nodePool *hyperv1.NodePool hostedCluster *hyperv1.HostedCluster objects []client.Object - reconcile func(t *testing.T, capi *CAPI) error + reconcile func(t *testing.T, capi *CAPI) (*CAPIResult, error) expectLabel bool }{ { @@ -2324,10 +2970,10 @@ func TestGlobalPSManagedLabelOnMachines(t *testing.T) { }, }, }, - reconcile: func(t *testing.T, capi *CAPI) error { + reconcile: func(t *testing.T, capi *CAPI) (*CAPIResult, error) { md := &capiv1.MachineDeployment{} if err := capi.Client.Get(t.Context(), client.ObjectKey{Namespace: controlPlaneNamespace, Name: "test-nodepool"}, md); err != nil { - return err + return nil, err } template := &capiazure.AzureMachineTemplate{ ObjectMeta: metav1.ObjectMeta{ @@ -2336,7 +2982,8 @@ func TestGlobalPSManagedLabelOnMachines(t *testing.T) { }, } log := ctrl.LoggerFrom(t.Context()) - return capi.reconcileMachineDeployment(t.Context(), log, md, template) + conditions, err := capi.reconcileMachineDeployment(t.Context(), log, md, template) + return &CAPIResult{Conditions: conditions}, err }, expectLabel: true, }, @@ -2472,10 +3119,10 @@ func TestGlobalPSManagedLabelOnMachines(t *testing.T) { }, // Call reconcileMachineDeployment directly to test the label logic // without requiring full KubeVirt machine template setup. - reconcile: func(t *testing.T, capi *CAPI) error { + reconcile: func(t *testing.T, capi *CAPI) (*CAPIResult, error) { md := &capiv1.MachineDeployment{} if err := capi.Client.Get(t.Context(), client.ObjectKey{Namespace: controlPlaneNamespace, Name: "test-nodepool"}, md); err != nil { - return err + return nil, err } kvTemplate := &capikubevirt.KubevirtMachineTemplate{ ObjectMeta: metav1.ObjectMeta{ @@ -2484,7 +3131,8 @@ func TestGlobalPSManagedLabelOnMachines(t *testing.T) { }, } log := ctrl.LoggerFrom(t.Context()) - return capi.reconcileMachineDeployment(t.Context(), log, md, kvTemplate) + conditions, err := capi.reconcileMachineDeployment(t.Context(), log, md, kvTemplate) + return &CAPIResult{Conditions: conditions}, err }, }, } @@ -2526,12 +3174,12 @@ func TestGlobalPSManagedLabelOnMachines(t *testing.T) { reconcile := tt.reconcile if reconcile == nil { - reconcile = func(t *testing.T, capi *CAPI) error { + reconcile = func(t *testing.T, capi *CAPI) (*CAPIResult, error) { return capi.Reconcile(t.Context()) } } - err := reconcile(t, capi) - g.Expect(err).NotTo(HaveOccurred()) + _, err := reconcile(t, capi) + g.Expect(err).NotTo(HaveOccurred(), "failed to reconcile CAPI while propagating the global pull-secret managed label") globalPSManagedLabelKey := fmt.Sprintf("%s.%s", labelManagedPrefix, globalPSNodeLabel) machineList := &capiv1.MachineList{} @@ -2653,7 +3301,8 @@ func TestReconcileSelectorDropsStaleLabels(t *testing.T) { }, } - g.Expect(capi.reconcileMachineDeployment(t.Context(), log, md, kvTemplate)).To(Succeed()) + _, err := capi.reconcileMachineDeployment(t.Context(), log, md, kvTemplate) + g.Expect(err).ToNot(HaveOccurred(), "failed to reconcile MachineDeployment while removing stale selector labels") assertSubset(g, md.Spec.Selector.MatchLabels, md.Spec.Template.Labels) }) @@ -2671,7 +3320,8 @@ func TestReconcileSelectorDropsStaleLabels(t *testing.T) { }, } - g.Expect(capi.reconcileMachineSet(t.Context(), ms, kvTemplate)).To(Succeed()) + _, err := capi.reconcileMachineSet(t.Context(), ms, kvTemplate) + g.Expect(err).ToNot(HaveOccurred(), "failed to reconcile MachineSet while removing stale selector labels") assertSubset(g, ms.Spec.Selector.MatchLabels, ms.Spec.Template.Labels) }) } @@ -3515,6 +4165,84 @@ func TestPropagateVersionAndTemplateToMachineSet(t *testing.T) { } } +func TestNodePoolReadyConditions(t *testing.T) { + t.Parallel() + testCases := []struct { + name string + conditions []metav1.Condition + expected []hyperv1.NodePoolCondition + }{ + { + name: "When conditions are absent, it should return no Ready condition", + }, + { + name: "When MachinesReady is absent, it should ignore other conditions", + conditions: []metav1.Condition{{ + Type: capiv1.AvailableCondition, + Status: metav1.ConditionTrue, + }}, + }, + { + name: "When machines are ready, it should preserve status reason and message with the NodePool generation", + conditions: []metav1.Condition{{ + Type: capiv1.MachinesReadyCondition, + Status: metav1.ConditionTrue, + Reason: "MachinesReady", + Message: "all machines are ready", + ObservedGeneration: 3, + LastTransitionTime: metav1.NewTime(time.Unix(100, 0)), + }}, + expected: []hyperv1.NodePoolCondition{{ + Type: hyperv1.NodePoolReadyConditionType, + Status: corev1.ConditionTrue, + Reason: "MachinesReady", + Message: "all machines are ready", + ObservedGeneration: 7, + }}, + }, + { + name: "When MachinesReady is false without a reason, it should use AsExpected and preserve the message", + conditions: []metav1.Condition{{ + Type: capiv1.MachinesReadyCondition, + Status: metav1.ConditionFalse, + Message: "machines are starting", + }}, + expected: []hyperv1.NodePoolCondition{{ + Type: hyperv1.NodePoolReadyConditionType, + Status: corev1.ConditionFalse, + Reason: hyperv1.AsExpectedReason, + Message: "machines are starting", + ObservedGeneration: 7, + }}, + }, + { + name: "When MachinesReady is unknown, it should preserve the unknown status and diagnostic fields", + conditions: []metav1.Condition{{ + Type: capiv1.MachinesReadyCondition, + Status: metav1.ConditionUnknown, + Reason: "InspectionPending", + Message: "waiting for machine status", + }}, + expected: []hyperv1.NodePoolCondition{{ + Type: hyperv1.NodePoolReadyConditionType, + Status: corev1.ConditionUnknown, + Reason: "InspectionPending", + Message: "waiting for machine status", + ObservedGeneration: 7, + }}, + }, + } + for _, testCase := range testCases { + t.Run(testCase.name, func(t *testing.T) { + t.Parallel() + testAssertions := NewWithT(t) + originalConditions := append([]metav1.Condition(nil), testCase.conditions...) + testAssertions.Expect(nodePoolReadyConditions(testCase.conditions, 7)).To(Equal(testCase.expected), "Ready conversion should preserve status, reason and message, use the NodePool generation and leave transition time unset") + testAssertions.Expect(testCase.conditions).To(Equal(originalConditions), "Ready conversion should not mutate the source conditions") + }) + } +} + func TestReconcileMachineDeploymentStatus(t *testing.T) { testReconcileStatusAfterReversion(t, hyperv1.UpgradeTypeReplace) testReconcileStatusWithoutBootstrap(t, hyperv1.UpgradeTypeReplace) @@ -3547,7 +4275,8 @@ func TestReconcileMachineDeploymentStatus(t *testing.T) { expectedConfigAnnotation bool expectedTemplateAnnotation bool expectedReadyConditionStatus corev1.ConditionStatus - expectedReadyConditionSet bool + expectedReadyReason string + expectedReadyMessage string }{ { name: "When MachineDeployment is complete, it should update nodePool version and annotations", @@ -3599,7 +4328,7 @@ func TestReconcileMachineDeploymentStatus(t *testing.T) { expectedTemplateAnnotation: false, }, { - name: "When MachineDeployment has Ready condition, it should propagate it to nodePool", + name: "When MachineDeployment has MachinesReady with a reason, it should return the exact NodePool Ready condition", machineDeployment: &capiv1.MachineDeployment{ ObjectMeta: metav1.ObjectMeta{Generation: 2}, Spec: capiv1.MachineDeploymentSpec{ @@ -3622,8 +4351,34 @@ func TestReconcileMachineDeploymentStatus(t *testing.T) { targetVersion: "4.17.0", expectedVersion: "4.16.0", expectedReplicas: 2, - expectedReadyConditionSet: true, expectedReadyConditionStatus: corev1.ConditionTrue, + expectedReadyReason: "SomeReason", + expectedReadyMessage: "all good", + }, + { + name: "When MachineDeployment has MachinesReady without a reason, it should return AsExpected for NodePool Ready", + machineDeployment: &capiv1.MachineDeployment{ + ObjectMeta: metav1.ObjectMeta{Generation: 2}, + Spec: capiv1.MachineDeploymentSpec{ + Replicas: ptr.To[int32](3), + }, + Status: capiv1.MachineDeploymentStatus{ + AvailableReplicas: ptr.To[int32](2), + Conditions: []metav1.Condition{{ + Type: capiv1.MachinesReadyCondition, + Status: metav1.ConditionFalse, + Message: "machines are starting", + }}, + }, + }, + nodePoolVersion: "4.16.0", + nodePoolAnnotations: map[string]string{}, + targetVersion: "4.17.0", + expectedVersion: "4.16.0", + expectedReplicas: 2, + expectedReadyConditionStatus: corev1.ConditionFalse, + expectedReadyReason: hyperv1.AsExpectedReason, + expectedReadyMessage: "machines are starting", }, } @@ -3636,6 +4391,7 @@ func TestReconcileMachineDeploymentStatus(t *testing.T) { Name: "test-np", Namespace: "test-ns", Annotations: tc.nodePoolAnnotations, + Generation: 5, }, Status: hyperv1.NodePoolStatus{ Version: tc.nodePoolVersion, @@ -3686,24 +4442,30 @@ func TestReconcileMachineDeploymentStatus(t *testing.T) { }, } - capi.reconcileMachineDeploymentStatus(context.Background(), logr.Discard(), tc.machineDeployment, templateCR) + conditions := capi.reconcileMachineDeploymentStatus(context.Background(), logr.Discard(), tc.machineDeployment, templateCR) - g.Expect(nodePool.Status.Replicas).To(Equal(tc.expectedReplicas)) - g.Expect(nodePool.Status.Version).To(Equal(tc.expectedVersion)) + g.Expect(nodePool.Status.Replicas).To(Equal(tc.expectedReplicas), "MachineDeployment readiness conversion should preserve available replica propagation") + g.Expect(nodePool.Status.Version).To(Equal(tc.expectedVersion), "MachineDeployment status reconciliation should preserve version advancement semantics") if tc.expectedConfigAnnotation { - g.Expect(nodePool.Annotations).To(HaveKey(nodePoolAnnotationCurrentConfig)) + g.Expect(nodePool.Annotations).To(HaveKey(nodePoolAnnotationCurrentConfig), "completed MachineDeployment should record the current config annotation") } if tc.expectedTemplateAnnotation { g.Expect(nodePool.Annotations).To(HaveKeyWithValue( nodePoolAnnotationPlatformMachineTemplate, "template-name", - )) + ), "completed MachineDeployment should record the current platform machine template") } - if tc.expectedReadyConditionSet { - readyCond := FindStatusCondition(nodePool.Status.Conditions, hyperv1.NodePoolReadyConditionType) - g.Expect(readyCond).ToNot(BeNil()) - g.Expect(readyCond.Status).To(Equal(tc.expectedReadyConditionStatus)) + if tc.expectedReadyReason == "" { + g.Expect(conditions).To(BeNil(), "MachineDeployment without MachinesReady should return no Ready condition") + } else { + g.Expect(conditions).To(Equal([]hyperv1.NodePoolCondition{{ + Type: hyperv1.NodePoolReadyConditionType, + Status: tc.expectedReadyConditionStatus, + Reason: tc.expectedReadyReason, + Message: tc.expectedReadyMessage, + ObservedGeneration: nodePool.Generation, + }}), "MachineDeployment readiness should preserve status and message, use the expected reason and observe the NodePool generation") } }) } @@ -4356,10 +5118,11 @@ func TestPauseUnpauseCycle(t *testing.T) { if tc.upgradeType == hyperv1.UpgradeTypeReplace { g.Expect(c.Get(t.Context(), client.ObjectKeyFromObject(md), md)).To(Succeed()) - g.Expect(capiObj.reconcileMachineDeployment(t.Context(), log, md, template)).To(Succeed()) + _, err := capiObj.reconcileMachineDeployment(t.Context(), log, md, template) + g.Expect(err).ToNot(HaveOccurred(), "failed to reconcile MachineDeployment after unpausing") // Verify pause annotation removed. - g.Expect(md.Annotations).NotTo(HaveKey(capiv1.PausedAnnotation)) + g.Expect(md.Annotations).NotTo(HaveKey(capiv1.PausedAnnotation), "MachineDeployment reconciliation should remove the paused annotation") // Verify replicas clamped within autoscaler bounds. g.Expect(*md.Spec.Replicas).To(Equal(tc.expectedReplicas)) // Verify autoscaler annotations preserved. @@ -4367,10 +5130,11 @@ func TestPauseUnpauseCycle(t *testing.T) { g.Expect(md.Annotations).To(HaveKeyWithValue(autoscalerMaxAnnotation, fmt.Sprintf("%d", tc.autoscalingMax))) } else { g.Expect(c.Get(t.Context(), client.ObjectKeyFromObject(ms), ms)).To(Succeed()) - g.Expect(capiObj.reconcileMachineSet(t.Context(), ms, template)).To(Succeed()) + _, err := capiObj.reconcileMachineSet(t.Context(), ms, template) + g.Expect(err).ToNot(HaveOccurred(), "failed to reconcile MachineSet after unpausing") // Verify pause annotation removed. - g.Expect(ms.Annotations).NotTo(HaveKey(capiv1.PausedAnnotation)) + g.Expect(ms.Annotations).NotTo(HaveKey(capiv1.PausedAnnotation), "MachineSet reconciliation should remove the paused annotation") // Verify replicas clamped within autoscaler bounds. g.Expect(*ms.Spec.Replicas).To(Equal(tc.expectedReplicas)) // Verify autoscaler annotations preserved. @@ -4735,8 +5499,9 @@ func TestMHCRemediationAllowedBubbledUpToReady(t *testing.T) { nodePool := &hyperv1.NodePool{ ObjectMeta: metav1.ObjectMeta{ - Name: "test-nodepool", - Namespace: "test-namespace", + Name: "test-nodepool", + Namespace: "test-namespace", + Generation: 7, }, Spec: hyperv1.NodePoolSpec{ ClusterName: "test-cluster", @@ -4900,15 +5665,51 @@ func TestMHCRemediationAllowedBubbledUpToReady(t *testing.T) { capi.Client = c ctx := ctrl.LoggerInto(t.Context(), logr.Discard()) - err := capi.Reconcile(ctx) - g.Expect(err).NotTo(HaveOccurred()) + result, err := capi.Reconcile(ctx) + g.Expect(err).NotTo(HaveOccurred(), "failed to reconcile CAPI with MachineHealthCheck remediation status") + g.Expect(result).NotTo(BeNil(), "MHC readiness reconciliation should return a nonnil result") + + expectedConditions := []hyperv1.NodePoolCondition{ + { + Type: hyperv1.NodePoolUpdatingPlatformMachineTemplateConditionType, + Status: corev1.ConditionTrue, + Reason: hyperv1.AsExpectedReason, + Message: fmt.Sprintf("platform machine template update in progress. Target template: %s", awsMachineTemplateName), + ObservedGeneration: nodePool.Generation, + }, + { + Type: hyperv1.NodePoolReadyConditionType, + Status: corev1.ConditionTrue, + Reason: hyperv1.AsExpectedReason, + ObservedGeneration: nodePool.Generation, + }, + { + Type: hyperv1.NodePoolAutorepairEnabledConditionType, + Status: corev1.ConditionTrue, + Reason: hyperv1.AsExpectedReason, + ObservedGeneration: nodePool.Generation, + }, + } + if tt.expectedReadyStatus == corev1.ConditionFalse { + expectedConditions = append(expectedConditions, hyperv1.NodePoolCondition{ + Type: hyperv1.NodePoolReadyConditionType, + Status: corev1.ConditionFalse, + Reason: tt.expectedReadyReason, + Message: tt.expectedMessage, + ObservedGeneration: nodePool.Generation, + }) + } + g.Expect(result.Conditions).To(Equal(expectedConditions), "MHC reconciliation should return ordered platform-update, workload-readiness, auto-repair and optional remediation override conditions") + for _, condition := range result.Conditions { + SetStatusCondition(&nodePool.Status.Conditions, condition) + } readyCond := FindStatusCondition(nodePool.Status.Conditions, hyperv1.NodePoolReadyConditionType) - g.Expect(readyCond).NotTo(BeNil()) - g.Expect(readyCond.Status).To(Equal(tt.expectedReadyStatus)) - g.Expect(readyCond.Reason).To(Equal(tt.expectedReadyReason)) + g.Expect(readyCond).NotTo(BeNil(), "applying returned CAPI conditions should produce a Ready condition") + g.Expect(readyCond.Status).To(Equal(tt.expectedReadyStatus), "applied Ready status should reflect MachineHealthCheck remediation precedence") + g.Expect(readyCond.Reason).To(Equal(tt.expectedReadyReason), "applied Ready reason should reflect MachineHealthCheck remediation precedence") if tt.expectedMessage != "" { - g.Expect(readyCond.Message).To(Equal(tt.expectedMessage)) + g.Expect(readyCond.Message).To(Equal(tt.expectedMessage), "applied Ready message should preserve the MachineHealthCheck diagnostic") } }) } diff --git a/hypershift-operator/controllers/nodepool/nodepool_controller.go b/hypershift-operator/controllers/nodepool/nodepool_controller.go index 3ee9ce4318bd..dfe4405c4b29 100644 --- a/hypershift-operator/controllers/nodepool/nodepool_controller.go +++ b/hypershift-operator/controllers/nodepool/nodepool_controller.go @@ -365,9 +365,6 @@ func (r *NodePoolReconciler) reconcile(ctx context.Context, hcluster *hyperv1.Ho r.reachedIgnitionEndpointCondition, r.machineAndNodeConditions, r.validPlatformConfigCondition, - // TODO(alberto): consider moving here: - // NodePoolUpdatingPlatformMachineTemplateConditionType, - // NodePoolAutorepairEnabledConditionType. } for _, f := range signalConditions { result, err := f(ctx, nodePool, hcluster) @@ -503,7 +500,13 @@ func (r *NodePoolReconciler) reconcile(ctx context.Context, hcluster *hyperv1.Ho return ctrl.Result{}, nil } - if err := capi.Reconcile(ctx); err != nil { + capiResult, err := capi.Reconcile(ctx) + if capiResult != nil { + for _, condition := range capiResult.Conditions { + SetStatusCondition(&nodePool.Status.Conditions, condition) + } + } + if err != nil { var notReadyErr *NotReadyError if coreerrors.As(err, ¬ReadyErr) { log.Info("Waiting to create machine template", "message", err.Error()) diff --git a/hypershift-operator/controllers/nodepool/nodepool_controller_test.go b/hypershift-operator/controllers/nodepool/nodepool_controller_test.go index 0bcce55f83ef..1090c5c59e7a 100644 --- a/hypershift-operator/controllers/nodepool/nodepool_controller_test.go +++ b/hypershift-operator/controllers/nodepool/nodepool_controller_test.go @@ -2,10 +2,12 @@ package nodepool import ( "context" + "errors" "fmt" "regexp" "strings" "testing" + "time" . "github.com/onsi/gomega" @@ -33,12 +35,15 @@ import ( "k8s.io/apimachinery/pkg/runtime" "k8s.io/apimachinery/pkg/types" "k8s.io/apimachinery/pkg/util/intstr" + "k8s.io/client-go/tools/record" "k8s.io/utils/ptr" + capiaws "sigs.k8s.io/cluster-api-provider-aws/v2/api/v1beta2" capiv1 "sigs.k8s.io/cluster-api/api/core/v1beta2" ctrl "sigs.k8s.io/controller-runtime" "sigs.k8s.io/controller-runtime/pkg/client" "sigs.k8s.io/controller-runtime/pkg/client/fake" + "sigs.k8s.io/controller-runtime/pkg/client/interceptor" "sigs.k8s.io/controller-runtime/pkg/reconcile" "sigs.k8s.io/yaml" @@ -3875,6 +3880,234 @@ func TestNodePoolReconciler_reconcile(t *testing.T) { } } +func TestNodePoolReconciler_Reconcile(t *testing.T) { + t.Run("When MachineDeployment persistence fails after CAPI computes conditions, it should patch exact ordered conditions and return the error", func(t *testing.T) { + g := NewWithT(t) + injectedErr := errors.New("injected MachineDeployment persistence failure") + const ( + namespace = "myns" + hostedClusterName = "cluster-name" + nodePoolName = "test-nodepool" + ) + + maxUnavailable := intstr.FromInt(0) + maxSurge := intstr.FromInt(1) + nodePool := &hyperv1.NodePool{ + ObjectMeta: metav1.ObjectMeta{ + Name: nodePoolName, + Namespace: namespace, + Generation: 17, + Finalizers: []string{finalizer}, + }, + Spec: hyperv1.NodePoolSpec{ + ClusterName: hostedClusterName, + Replicas: ptr.To[int32](3), + Release: hyperv1.Release{ + Image: "quay.io/openshift-release-dev/ocp-release:4.18.0-x86_64", + }, + Management: hyperv1.NodePoolManagement{ + UpgradeType: hyperv1.UpgradeTypeReplace, + Replace: &hyperv1.ReplaceUpgrade{ + Strategy: hyperv1.UpgradeStrategyRollingUpdate, + RollingUpdate: &hyperv1.RollingUpdate{ + MaxUnavailable: &maxUnavailable, + MaxSurge: &maxSurge, + }, + }, + }, + Platform: hyperv1.NodePoolPlatform{ + Type: hyperv1.AWSPlatform, + AWS: &hyperv1.AWSNodePoolPlatform{ + AMI: "ami-1234567890abcdef0", + }, + }, + }, + Status: hyperv1.NodePoolStatus{ + Version: "4.18.0", + Conditions: []hyperv1.NodePoolCondition{{ + Type: "ExistingUnrelatedCondition", + Status: corev1.ConditionTrue, + Reason: hyperv1.AsExpectedReason, + Message: "preserve this condition", + ObservedGeneration: 16, + LastTransitionTime: metav1.NewTime(time.Unix(100, 0)), + }}, + }, + } + originalCondition := nodePool.Status.Conditions[0] + + hostedCluster := &hyperv1.HostedCluster{ + ObjectMeta: metav1.ObjectMeta{ + Name: hostedClusterName, + Namespace: namespace, + Annotations: map[string]string{ + hyperv1.SkipReleaseImageValidation: "true", + }, + }, + Spec: hyperv1.HostedClusterSpec{ + InfraID: hostedClusterName, + Release: hyperv1.Release{Image: nodePool.Spec.Release.Image}, + PullSecret: corev1.LocalObjectReference{ + Name: "pull-secret", + }, + Platform: hyperv1.PlatformSpec{ + Type: hyperv1.AWSPlatform, + AWS: &hyperv1.AWSPlatformSpec{ + Region: "us-east-1", + EndpointAccess: hyperv1.Public, + CloudProviderConfig: &hyperv1.AWSCloudProviderConfig{}, + }, + }, + Networking: hyperv1.ClusterNetworking{ + ServiceNetwork: []hyperv1.ServiceNetworkEntry{{CIDR: *ipnet.MustParseCIDR("172.30.0.0/16")}}, + }, + }, + Status: hyperv1.HostedClusterStatus{ + IgnitionEndpoint: "ignition.example.com", + KubeConfig: &corev1.LocalObjectReference{Name: "kk"}, + Version: &hyperv1.ClusterVersionStatus{History: []configv1.UpdateHistory{{ + State: configv1.CompletedUpdate, + Version: "4.18.0", + }}}, + Platform: &hyperv1.PlatformStatus{AWS: &hyperv1.AWSPlatformStatus{ + DefaultWorkerSecurityGroupID: "sg-1234567890abcdef0", + }}, + }, + } + + pullSecret, ignitionCA, _, ignitionConfig1, ignitionConfig2, ignitionConfig3 := setupTestObjects() + kubeconfigSecret := &corev1.Secret{ + ObjectMeta: metav1.ObjectMeta{Name: "kk", Namespace: namespace}, + Data: map[string][]byte{"kubeconfig": []byte(`apiVersion: v1 +clusters: +- cluster: + server: https://kubeconfig-host:443 + name: cluster +contexts: +- context: + cluster: cluster + user: "" + name: cluster +current-context: cluster +kind: Config`)}, + } + controlPlaneNamespace := "myns-cluster-name" + machineDeployment := &capiv1.MachineDeployment{ + ObjectMeta: metav1.ObjectMeta{Name: nodePoolName, Namespace: controlPlaneNamespace}, + Spec: capiv1.MachineDeploymentSpec{ + Replicas: ptr.To[int32](3), + Template: capiv1.MachineTemplateSpec{Spec: capiv1.MachineSpec{ + Bootstrap: capiv1.Bootstrap{DataSecretName: ptr.To("previous-user-data")}, + InfrastructureRef: capiv1.ContractVersionedObjectReference{ + APIGroup: "infrastructure.cluster.x-k8s.io", + Kind: "AWSMachineTemplate", + Name: "previous-template", + }, + Version: "4.18.0", + }}, + }, + Status: capiv1.MachineDeploymentStatus{ + AvailableReplicas: ptr.To[int32](3), + Conditions: []metav1.Condition{{ + Type: capiv1.MachinesReadyCondition, + Status: metav1.ConditionTrue, + Reason: "MachinesReady", + Message: "all machines are ready", + }}, + }, + } + + objects := []client.Object{ + nodePool, + hostedCluster, + pullSecret, + ignitionCA, + ignitionConfig1, + ignitionConfig2, + ignitionConfig3, + kubeconfigSecret, + } + testClient := fake.NewClientBuilder(). + WithScheme(api.Scheme). + WithObjects(objects...). + WithStatusSubresource(nodePool). + WithInterceptorFuncs(interceptor.Funcs{ + Patch: func(ctx context.Context, c client.WithWatch, obj client.Object, patch client.Patch, opts ...client.PatchOption) error { + if _, ok := obj.(*capiv1.MachineDeployment); ok { + return injectedErr + } + return c.Patch(ctx, obj, patch, opts...) + }, + }). + Build() + + releaseProvider := &fakereleaseprovider.FakeReleaseProvider{ + Version: "4.18.0", + Components: map[string]string{ + haproxy.HAProxyRouterImageName: "quay.io/openshift-release-dev/ocp-v4.0-art-dev@sha256:1234", + }, + CanonicalComponents: map[string]string{ + haproxy.HAProxyRouterImageName: "quay.io/openshift-release-dev/ocp-v4.0-art-dev@sha256:1234", + }, + } + r := &NodePoolReconciler{ + Client: testClient, + ReleaseProvider: releaseProvider, + HypershiftOperatorImage: "quay.io/hypershift/hypershift-operator:latest", + ImageMetadataProvider: &fakeimagemetadataprovider.FakeRegistryClientImageMetadataProvider{ + Result: &dockerv1client.DockerImageConfig{Config: &docker10.DockerConfig{Labels: map[string]string{}}}, + }, + CreateOrUpdateProvider: upsert.New(false), + recorder: record.NewFakeRecorder(1), + } + token, err := r.token(t.Context(), hostedCluster, nodePool) + g.Expect(err).NotTo(HaveOccurred(), "failed to construct the NodePool token") + capi, err := newCAPI(token, hostedCluster.Spec.InfraID) + g.Expect(err).NotTo(HaveOccurred(), "failed to construct CAPI for the NodePool") + machineTemplate, err := capi.machineTemplateBuilders(t.Context()) + g.Expect(err).NotTo(HaveOccurred(), "failed to build the expected machine template") + machineDeployment.Spec.Template.Spec.Bootstrap.DataSecretName = ptr.To(capi.UserDataSecret().Name) + machineDeployment.Spec.Template.Spec.InfrastructureRef.Name = machineTemplate.GetName() + machineDeployment.Spec.Template.Spec.Version = capi.Version() + g.Expect(testClient.Create(t.Context(), machineDeployment)).To(Succeed(), "failed to create the MachineDeployment") + + result, err := r.Reconcile(t.Context(), reconcile.Request{NamespacedName: client.ObjectKeyFromObject(nodePool)}) + g.Expect(errors.Is(err, injectedErr)).To(BeTrue(), "NodePool reconciliation should preserve the injected MachineDeployment patch error") + g.Expect(err).To(MatchError(fmt.Sprintf("failed to reconcile MachineDeployment %q: %s", client.ObjectKeyFromObject(machineDeployment).String(), injectedErr)), "MachineDeployment reconciliation error should include the workload identity and original failure") + g.Expect(result).To(Equal(ctrl.Result{}), "failed NodePool reconciliation should return an empty result") + + persistedNodePool := &hyperv1.NodePool{} + g.Expect(testClient.Get(t.Context(), client.ObjectKeyFromObject(nodePool), persistedNodePool)).To(Succeed(), "failed to retrieve the NodePool after reconciliation failed") + g.Expect(FindStatusCondition(persistedNodePool.Status.Conditions, originalCondition.Type)).To(Equal(&originalCondition), "patching returned CAPI conditions on error should preserve unrelated condition fields and transition time") + machineTemplates := &capiaws.AWSMachineTemplateList{} + g.Expect(testClient.List(t.Context(), machineTemplates, client.InNamespace(controlPlaneNamespace))).To(Succeed(), "failed to list reconciled machine templates") + g.Expect(machineTemplates.Items).To(HaveLen(1), "reconciliation should persist exactly one machine template before the workload patch error") + + g.Expect(len(persistedNodePool.Status.Conditions)).To(BeNumerically(">=", 2), "persisted NodePool should contain both returned CAPI conditions despite the reconciliation error") + persistedCAPIConditions := append([]hyperv1.NodePoolCondition(nil), persistedNodePool.Status.Conditions[len(persistedNodePool.Status.Conditions)-2:]...) + for i := range persistedCAPIConditions { + g.Expect(persistedCAPIConditions[i].LastTransitionTime.IsZero()).To(BeFalse(), "persisted CAPI condition %d should have a transition time", i) + persistedCAPIConditions[i].LastTransitionTime = metav1.Time{} + } + g.Expect(persistedCAPIConditions).To(Equal([]hyperv1.NodePoolCondition{ + { + Type: hyperv1.NodePoolUpdatingPlatformMachineTemplateConditionType, + Status: corev1.ConditionTrue, + Reason: hyperv1.AsExpectedReason, + Message: fmt.Sprintf("platform machine template update in progress. Target template: %s", machineTemplates.Items[0].Name), + ObservedGeneration: nodePool.Generation, + }, + { + Type: hyperv1.NodePoolReadyConditionType, + Status: corev1.ConditionTrue, + Reason: "MachinesReady", + Message: "all machines are ready", + ObservedGeneration: nodePool.Generation, + }, + }), "NodePool patch-on-error should persist platform-update and Ready conditions in order with their complete fields") + }) +} + func TestEnqueueNodePoolsForCloudConfig(t *testing.T) { t.Parallel() hcNamespace := "clusters" From 5affdb4883e307d7d791243e55bc926812a6504d Mon Sep 17 00:00:00 2001 From: Chai Bot Date: Thu, 29 Oct 2026 02:33:31 +0000 Subject: [PATCH 2/2] fix(nodepool): preserve readiness rollback on workload persistence failure Why: returning workload conditions before persistence succeeds bypasses the NodePool snapshot rollback and publishes readiness for an unapplied change. How: append workload conditions only after successful reconciliation, retain pre-workload conditions on failure, and cover both workload types through the outer reconciler. Restore the location-specific TODO and remove the unrelated OSImageStream test from this PR. Signed-off-by: Chai Bot Assisted-by: LLM --- .../e2eutilallowlist/e2eutilallowlist_test.go | 56 --- .../controllers/nodepool/capi.go | 4 +- .../controllers/nodepool/capi_test.go | 23 +- .../nodepool/nodepool_controller.go | 3 + .../nodepool/nodepool_controller_test.go | 416 +++++++++--------- 5 files changed, 242 insertions(+), 260 deletions(-) diff --git a/hack/tools/hypershiftlinter/analyzers/e2eutilallowlist/e2eutilallowlist_test.go b/hack/tools/hypershiftlinter/analyzers/e2eutilallowlist/e2eutilallowlist_test.go index c96b41f6d86b..f07c0da2a8c1 100644 --- a/hack/tools/hypershiftlinter/analyzers/e2eutilallowlist/e2eutilallowlist_test.go +++ b/hack/tools/hypershiftlinter/analyzers/e2eutilallowlist/e2eutilallowlist_test.go @@ -7,7 +7,6 @@ import ( "go/types" "os" "path/filepath" - "slices" "strings" "testing" @@ -176,61 +175,6 @@ func TestAllowlist(t *testing.T) { } } -func TestAllowlistOSImageStreamUpgradeScopes(t *testing.T) { - t.Parallel() - const filename = "test/e2e/v2/tests/nodepool_osimagestream_test.go" - file, err := parser.ParseFile(token.NewFileSet(), filepath.Join("../../../../..", filename), nil, 0) - if err != nil { - t.Fatalf("parse OSImageStream upgrade specs: %v", err) - } - wantSymbols := []string{"EventuallyObject", "OSImageStreamPredicate", "Predicate", "UpdateObject", "WaitForReadyNodesByNodePool", "WithInterval", "WithTimeout"} - for _, testCase := range []struct { - name string - function string - }{ - {name: "When the cross-major upgrade spec is renamed, it should retain only its existing utility allowances", function: "NodePoolOSImageStreamCrossMajorUpgradeTest"}, - {name: "When the pinned RHEL9 upgrade spec is renamed, it should retain only its existing utility allowances", function: "NodePoolOSImageStreamPinnedRHEL9UpgradeTest"}, - {name: "When the upgrade spec uses the v2 version waiter, it should remove obsolete condition allowances", function: "NodePoolOSImageStreamUpgradeVerificationTest"}, - } { - t.Run(testCase.name, func(t *testing.T) { - t.Parallel() - var scopes []string - for _, declaration := range file.Decls { - function, ok := declaration.(*ast.FuncDecl) - if !ok || function.Name.Name != testCase.function { - continue - } - ast.Inspect(function.Body, func(node ast.Node) bool { - call, ok := node.(*ast.CallExpr) - if !ok { - return true - } - identifier, ok := call.Fun.(*ast.Ident) - if !ok || identifier.Name != "It" { - return true - } - info := &types.Info{Uses: map[*ast.Ident]types.Object{ - identifier: types.NewFunc(token.NoPos, types.NewPackage("github.com/onsi/ginkgo/v2", "ginkgo"), "It", nil), - }} - scopes = append(scopes, registrationScope(&analysis.Pass{TypesInfo: info}, call, testCase.function, map[string]int{})) - return false - }) - } - if len(scopes) != 1 { - t.Fatalf("%s has %d It registrations, want exactly one", testCase.function, len(scopes)) - } - if got := allowlist[filename][scopes[0]]; !slices.Equal(got, wantSymbols) { - t.Errorf("allowances for current callback %s = %v, want %v", scopes[0], got, wantSymbols) - } - for scope := range allowlist[filename] { - if strings.HasPrefix(scope, testCase.function+"/") && scope != scopes[0] { - t.Errorf("obsolete callback allowance %s remains alongside current callback %s", scope, scopes[0]) - } - } - }) - } -} - func TestMemberName(t *testing.T) { t.Parallel() pkg := types.NewPackage(utilPkgPath, "util") diff --git a/hypershift-operator/controllers/nodepool/capi.go b/hypershift-operator/controllers/nodepool/capi.go index 4e101e15ab99..64ad1f3a7f89 100644 --- a/hypershift-operator/controllers/nodepool/capi.go +++ b/hypershift-operator/controllers/nodepool/capi.go @@ -152,11 +152,11 @@ func (c *CAPI) Reconcile(ctx context.Context) (*CAPIResult, error) { template) return reconcileErr }) - result.Conditions = append(result.Conditions, conditions...) if err != nil { return result, fmt.Errorf("failed to reconcile MachineSet %q: %w", client.ObjectKeyFromObject(ms).String(), err) } + result.Conditions = append(result.Conditions, conditions...) log.Info("Reconciled MachineSet", "result", operationResult) } @@ -172,11 +172,11 @@ func (c *CAPI) Reconcile(ctx context.Context) (*CAPIResult, error) { template) return reconcileErr }) - result.Conditions = append(result.Conditions, conditions...) if err != nil { return result, fmt.Errorf("failed to reconcile MachineDeployment %q: %w", client.ObjectKeyFromObject(md).String(), err) } + result.Conditions = append(result.Conditions, conditions...) log.Info("Reconciled MachineDeployment", "result", operationResult) } diff --git a/hypershift-operator/controllers/nodepool/capi_test.go b/hypershift-operator/controllers/nodepool/capi_test.go index 8a3ec89bb330..e45767a29260 100644 --- a/hypershift-operator/controllers/nodepool/capi_test.go +++ b/hypershift-operator/controllers/nodepool/capi_test.go @@ -2591,7 +2591,7 @@ func testCAPIReconcileConditionErrorPaths(t *testing.T) { }, }, { - name: "When MachineDeployment persistence fails after status reconciliation, it should return the accumulated conditions", + name: "When MachineDeployment persistence fails after status reconciliation, it should return only pre-workload conditions", upgradeType: hyperv1.UpgradeTypeReplace, interceptors: interceptor.Funcs{ Patch: func(ctx context.Context, c client.WithWatch, obj client.Object, patch client.Patch, opts ...client.PatchOption) error { @@ -2603,11 +2603,10 @@ func testCAPIReconcileConditionErrorPaths(t *testing.T) { }, expectedTypes: []string{ hyperv1.NodePoolUpdatingPlatformMachineTemplateConditionType, - hyperv1.NodePoolReadyConditionType, }, }, { - name: "When MachineSet persistence fails after status reconciliation, it should return the accumulated conditions", + name: "When MachineSet persistence fails after status reconciliation, it should return only pre-workload conditions", upgradeType: hyperv1.UpgradeTypeInPlace, interceptors: interceptor.Funcs{ Patch: func(ctx context.Context, c client.WithWatch, obj client.Object, patch client.Patch, opts ...client.PatchOption) error { @@ -2619,7 +2618,6 @@ func testCAPIReconcileConditionErrorPaths(t *testing.T) { }, expectedTypes: []string{ hyperv1.NodePoolUpdatingPlatformMachineTemplateConditionType, - hyperv1.NodePoolReadyConditionType, }, }, { @@ -2639,6 +2637,23 @@ func testCAPIReconcileConditionErrorPaths(t *testing.T) { hyperv1.NodePoolReadyConditionType, }, }, + { + name: "When MachineHealthCheck creation fails after MachineSet persistence, it should preserve readiness", + upgradeType: hyperv1.UpgradeTypeInPlace, + autoRepair: true, + interceptors: interceptor.Funcs{ + Create: func(ctx context.Context, c client.WithWatch, obj client.Object, opts ...client.CreateOption) error { + if _, ok := obj.(*capiv1.MachineHealthCheck); ok { + return injectedErr + } + return c.Create(ctx, obj, opts...) + }, + }, + expectedTypes: []string{ + hyperv1.NodePoolUpdatingPlatformMachineTemplateConditionType, + hyperv1.NodePoolReadyConditionType, + }, + }, { name: "When disabled-autorepair MachineHealthCheck retrieval fails, it should not fabricate autorepair disabled", upgradeType: hyperv1.UpgradeTypeReplace, diff --git a/hypershift-operator/controllers/nodepool/nodepool_controller.go b/hypershift-operator/controllers/nodepool/nodepool_controller.go index dfe4405c4b29..532f1b885e92 100644 --- a/hypershift-operator/controllers/nodepool/nodepool_controller.go +++ b/hypershift-operator/controllers/nodepool/nodepool_controller.go @@ -365,6 +365,9 @@ func (r *NodePoolReconciler) reconcile(ctx context.Context, hcluster *hyperv1.Ho r.reachedIgnitionEndpointCondition, r.machineAndNodeConditions, r.validPlatformConfigCondition, + // TODO(alberto): consider moving here: + // NodePoolUpdatingPlatformMachineTemplateConditionType, + // NodePoolAutorepairEnabledConditionType. } for _, f := range signalConditions { result, err := f(ctx, nodePool, hcluster) diff --git a/hypershift-operator/controllers/nodepool/nodepool_controller_test.go b/hypershift-operator/controllers/nodepool/nodepool_controller_test.go index 1090c5c59e7a..6fd2c4fc0528 100644 --- a/hypershift-operator/controllers/nodepool/nodepool_controller_test.go +++ b/hypershift-operator/controllers/nodepool/nodepool_controller_test.go @@ -3881,104 +3881,114 @@ func TestNodePoolReconciler_reconcile(t *testing.T) { } func TestNodePoolReconciler_Reconcile(t *testing.T) { - t.Run("When MachineDeployment persistence fails after CAPI computes conditions, it should patch exact ordered conditions and return the error", func(t *testing.T) { - g := NewWithT(t) - injectedErr := errors.New("injected MachineDeployment persistence failure") - const ( - namespace = "myns" - hostedClusterName = "cluster-name" - nodePoolName = "test-nodepool" - ) - - maxUnavailable := intstr.FromInt(0) - maxSurge := intstr.FromInt(1) - nodePool := &hyperv1.NodePool{ - ObjectMeta: metav1.ObjectMeta{ - Name: nodePoolName, - Namespace: namespace, - Generation: 17, - Finalizers: []string{finalizer}, - }, - Spec: hyperv1.NodePoolSpec{ - ClusterName: hostedClusterName, - Replicas: ptr.To[int32](3), - Release: hyperv1.Release{ - Image: "quay.io/openshift-release-dev/ocp-release:4.18.0-x86_64", + for _, upgradeType := range []hyperv1.UpgradeType{hyperv1.UpgradeTypeReplace, hyperv1.UpgradeTypeInPlace} { + t.Run(fmt.Sprintf("When %s workload persistence fails, it should preserve previous Ready and persist earlier conditions", upgradeType), func(t *testing.T) { + g := NewWithT(t) + injectedErr := errors.New("injected workload persistence failure") + const ( + namespace = "myns" + hostedClusterName = "cluster-name" + nodePoolName = "test-nodepool" + ) + + maxUnavailable := intstr.FromInt(0) + maxSurge := intstr.FromInt(1) + nodePool := &hyperv1.NodePool{ + ObjectMeta: metav1.ObjectMeta{ + Name: nodePoolName, + Namespace: namespace, + Generation: 17, + Finalizers: []string{finalizer}, }, - Management: hyperv1.NodePoolManagement{ - UpgradeType: hyperv1.UpgradeTypeReplace, - Replace: &hyperv1.ReplaceUpgrade{ - Strategy: hyperv1.UpgradeStrategyRollingUpdate, - RollingUpdate: &hyperv1.RollingUpdate{ - MaxUnavailable: &maxUnavailable, - MaxSurge: &maxSurge, + Spec: hyperv1.NodePoolSpec{ + ClusterName: hostedClusterName, + Replicas: ptr.To[int32](5), + Release: hyperv1.Release{ + Image: "quay.io/openshift-release-dev/ocp-release:4.18.0-x86_64", + }, + Management: hyperv1.NodePoolManagement{ + UpgradeType: hyperv1.UpgradeTypeReplace, + Replace: &hyperv1.ReplaceUpgrade{ + Strategy: hyperv1.UpgradeStrategyRollingUpdate, + RollingUpdate: &hyperv1.RollingUpdate{ + MaxUnavailable: &maxUnavailable, + MaxSurge: &maxSurge, + }, }, }, - }, - Platform: hyperv1.NodePoolPlatform{ - Type: hyperv1.AWSPlatform, - AWS: &hyperv1.AWSNodePoolPlatform{ - AMI: "ami-1234567890abcdef0", + Platform: hyperv1.NodePoolPlatform{ + Type: hyperv1.AWSPlatform, + AWS: &hyperv1.AWSNodePoolPlatform{ + AMI: "ami-1234567890abcdef0", + }, }, }, - }, - Status: hyperv1.NodePoolStatus{ - Version: "4.18.0", - Conditions: []hyperv1.NodePoolCondition{{ - Type: "ExistingUnrelatedCondition", - Status: corev1.ConditionTrue, - Reason: hyperv1.AsExpectedReason, - Message: "preserve this condition", - ObservedGeneration: 16, - LastTransitionTime: metav1.NewTime(time.Unix(100, 0)), - }}, - }, - } - originalCondition := nodePool.Status.Conditions[0] - - hostedCluster := &hyperv1.HostedCluster{ - ObjectMeta: metav1.ObjectMeta{ - Name: hostedClusterName, - Namespace: namespace, - Annotations: map[string]string{ - hyperv1.SkipReleaseImageValidation: "true", + Status: hyperv1.NodePoolStatus{ + Version: "4.18.0", + Conditions: []hyperv1.NodePoolCondition{{ + Type: "ExistingUnrelatedCondition", + Status: corev1.ConditionTrue, + Reason: hyperv1.AsExpectedReason, + Message: "preserve this condition", + ObservedGeneration: 16, + LastTransitionTime: metav1.NewTime(time.Unix(100, 0)), + }}, }, - }, - Spec: hyperv1.HostedClusterSpec{ - InfraID: hostedClusterName, - Release: hyperv1.Release{Image: nodePool.Spec.Release.Image}, - PullSecret: corev1.LocalObjectReference{ - Name: "pull-secret", + } + originalCondition := nodePool.Status.Conditions[0] + originalReady := hyperv1.NodePoolCondition{ + Type: hyperv1.NodePoolReadyConditionType, Status: corev1.ConditionFalse, + Reason: "ScalingUp", Message: "waiting for requested replicas", ObservedGeneration: 16, + LastTransitionTime: metav1.NewTime(time.Unix(100, 0)), + } + nodePool.Status.Conditions = append(nodePool.Status.Conditions, originalReady) + if upgradeType == hyperv1.UpgradeTypeInPlace { + nodePool.Spec.Management = hyperv1.NodePoolManagement{UpgradeType: upgradeType, InPlace: &hyperv1.InPlaceUpgrade{}} + } + + hostedCluster := &hyperv1.HostedCluster{ + ObjectMeta: metav1.ObjectMeta{ + Name: hostedClusterName, + Namespace: namespace, + Annotations: map[string]string{ + hyperv1.SkipReleaseImageValidation: "true", + }, }, - Platform: hyperv1.PlatformSpec{ - Type: hyperv1.AWSPlatform, - AWS: &hyperv1.AWSPlatformSpec{ - Region: "us-east-1", - EndpointAccess: hyperv1.Public, - CloudProviderConfig: &hyperv1.AWSCloudProviderConfig{}, + Spec: hyperv1.HostedClusterSpec{ + InfraID: hostedClusterName, + Release: hyperv1.Release{Image: nodePool.Spec.Release.Image}, + PullSecret: corev1.LocalObjectReference{ + Name: "pull-secret", + }, + Platform: hyperv1.PlatformSpec{ + Type: hyperv1.AWSPlatform, + AWS: &hyperv1.AWSPlatformSpec{ + Region: "us-east-1", + EndpointAccess: hyperv1.Public, + CloudProviderConfig: &hyperv1.AWSCloudProviderConfig{}, + }, + }, + Networking: hyperv1.ClusterNetworking{ + ServiceNetwork: []hyperv1.ServiceNetworkEntry{{CIDR: *ipnet.MustParseCIDR("172.30.0.0/16")}}, }, }, - Networking: hyperv1.ClusterNetworking{ - ServiceNetwork: []hyperv1.ServiceNetworkEntry{{CIDR: *ipnet.MustParseCIDR("172.30.0.0/16")}}, + Status: hyperv1.HostedClusterStatus{ + IgnitionEndpoint: "ignition.example.com", + KubeConfig: &corev1.LocalObjectReference{Name: "kk"}, + Version: &hyperv1.ClusterVersionStatus{History: []configv1.UpdateHistory{{ + State: configv1.CompletedUpdate, + Version: "4.18.0", + }}}, + Platform: &hyperv1.PlatformStatus{AWS: &hyperv1.AWSPlatformStatus{ + DefaultWorkerSecurityGroupID: "sg-1234567890abcdef0", + }}, }, - }, - Status: hyperv1.HostedClusterStatus{ - IgnitionEndpoint: "ignition.example.com", - KubeConfig: &corev1.LocalObjectReference{Name: "kk"}, - Version: &hyperv1.ClusterVersionStatus{History: []configv1.UpdateHistory{{ - State: configv1.CompletedUpdate, - Version: "4.18.0", - }}}, - Platform: &hyperv1.PlatformStatus{AWS: &hyperv1.AWSPlatformStatus{ - DefaultWorkerSecurityGroupID: "sg-1234567890abcdef0", - }}, - }, - } + } - pullSecret, ignitionCA, _, ignitionConfig1, ignitionConfig2, ignitionConfig3 := setupTestObjects() - kubeconfigSecret := &corev1.Secret{ - ObjectMeta: metav1.ObjectMeta{Name: "kk", Namespace: namespace}, - Data: map[string][]byte{"kubeconfig": []byte(`apiVersion: v1 + pullSecret, ignitionCA, _, ignitionConfig1, ignitionConfig2, ignitionConfig3 := setupTestObjects() + kubeconfigSecret := &corev1.Secret{ + ObjectMeta: metav1.ObjectMeta{Name: "kk", Namespace: namespace}, + Data: map[string][]byte{"kubeconfig": []byte(`apiVersion: v1 clusters: - cluster: server: https://kubeconfig-host:443 @@ -3990,122 +4000,132 @@ contexts: name: cluster current-context: cluster kind: Config`)}, - } - controlPlaneNamespace := "myns-cluster-name" - machineDeployment := &capiv1.MachineDeployment{ - ObjectMeta: metav1.ObjectMeta{Name: nodePoolName, Namespace: controlPlaneNamespace}, - Spec: capiv1.MachineDeploymentSpec{ - Replicas: ptr.To[int32](3), - Template: capiv1.MachineTemplateSpec{Spec: capiv1.MachineSpec{ - Bootstrap: capiv1.Bootstrap{DataSecretName: ptr.To("previous-user-data")}, - InfrastructureRef: capiv1.ContractVersionedObjectReference{ - APIGroup: "infrastructure.cluster.x-k8s.io", - Kind: "AWSMachineTemplate", - Name: "previous-template", + } + controlPlaneNamespace := "myns-cluster-name" + machineDeployment := &capiv1.MachineDeployment{ + ObjectMeta: metav1.ObjectMeta{Name: nodePoolName, Namespace: controlPlaneNamespace}, + Spec: capiv1.MachineDeploymentSpec{ + Replicas: ptr.To[int32](3), + Template: capiv1.MachineTemplateSpec{Spec: capiv1.MachineSpec{ + Bootstrap: capiv1.Bootstrap{DataSecretName: ptr.To("previous-user-data")}, + InfrastructureRef: capiv1.ContractVersionedObjectReference{ + APIGroup: "infrastructure.cluster.x-k8s.io", + Kind: "AWSMachineTemplate", + Name: "previous-template", + }, + Version: "4.18.0", + }}, + }, + Status: capiv1.MachineDeploymentStatus{ + AvailableReplicas: ptr.To[int32](3), + Conditions: []metav1.Condition{{ + Type: capiv1.MachinesReadyCondition, + Status: metav1.ConditionTrue, + Reason: "MachinesReady", + Message: "all machines are ready", + }}, + }, + } + + objects := []client.Object{ + nodePool, + hostedCluster, + pullSecret, + ignitionCA, + ignitionConfig1, + ignitionConfig2, + ignitionConfig3, + kubeconfigSecret, + } + testClient := fake.NewClientBuilder(). + WithScheme(api.Scheme). + WithObjects(objects...). + WithStatusSubresource(nodePool). + WithInterceptorFuncs(interceptor.Funcs{ + Patch: func(ctx context.Context, c client.WithWatch, obj client.Object, patch client.Patch, opts ...client.PatchOption) error { + switch obj.(type) { + case *capiv1.MachineDeployment, *capiv1.MachineSet: + return injectedErr + } + return c.Patch(ctx, obj, patch, opts...) }, - Version: "4.18.0", - }}, - }, - Status: capiv1.MachineDeploymentStatus{ - AvailableReplicas: ptr.To[int32](3), - Conditions: []metav1.Condition{{ - Type: capiv1.MachinesReadyCondition, - Status: metav1.ConditionTrue, - Reason: "MachinesReady", - Message: "all machines are ready", - }}, - }, - } - - objects := []client.Object{ - nodePool, - hostedCluster, - pullSecret, - ignitionCA, - ignitionConfig1, - ignitionConfig2, - ignitionConfig3, - kubeconfigSecret, - } - testClient := fake.NewClientBuilder(). - WithScheme(api.Scheme). - WithObjects(objects...). - WithStatusSubresource(nodePool). - WithInterceptorFuncs(interceptor.Funcs{ - Patch: func(ctx context.Context, c client.WithWatch, obj client.Object, patch client.Patch, opts ...client.PatchOption) error { - if _, ok := obj.(*capiv1.MachineDeployment); ok { - return injectedErr - } - return c.Patch(ctx, obj, patch, opts...) + }). + Build() + + releaseProvider := &fakereleaseprovider.FakeReleaseProvider{ + Version: "4.18.0", + Components: map[string]string{ + haproxy.HAProxyRouterImageName: "quay.io/openshift-release-dev/ocp-v4.0-art-dev@sha256:1234", }, - }). - Build() - - releaseProvider := &fakereleaseprovider.FakeReleaseProvider{ - Version: "4.18.0", - Components: map[string]string{ - haproxy.HAProxyRouterImageName: "quay.io/openshift-release-dev/ocp-v4.0-art-dev@sha256:1234", - }, - CanonicalComponents: map[string]string{ - haproxy.HAProxyRouterImageName: "quay.io/openshift-release-dev/ocp-v4.0-art-dev@sha256:1234", - }, - } - r := &NodePoolReconciler{ - Client: testClient, - ReleaseProvider: releaseProvider, - HypershiftOperatorImage: "quay.io/hypershift/hypershift-operator:latest", - ImageMetadataProvider: &fakeimagemetadataprovider.FakeRegistryClientImageMetadataProvider{ - Result: &dockerv1client.DockerImageConfig{Config: &docker10.DockerConfig{Labels: map[string]string{}}}, - }, - CreateOrUpdateProvider: upsert.New(false), - recorder: record.NewFakeRecorder(1), - } - token, err := r.token(t.Context(), hostedCluster, nodePool) - g.Expect(err).NotTo(HaveOccurred(), "failed to construct the NodePool token") - capi, err := newCAPI(token, hostedCluster.Spec.InfraID) - g.Expect(err).NotTo(HaveOccurred(), "failed to construct CAPI for the NodePool") - machineTemplate, err := capi.machineTemplateBuilders(t.Context()) - g.Expect(err).NotTo(HaveOccurred(), "failed to build the expected machine template") - machineDeployment.Spec.Template.Spec.Bootstrap.DataSecretName = ptr.To(capi.UserDataSecret().Name) - machineDeployment.Spec.Template.Spec.InfrastructureRef.Name = machineTemplate.GetName() - machineDeployment.Spec.Template.Spec.Version = capi.Version() - g.Expect(testClient.Create(t.Context(), machineDeployment)).To(Succeed(), "failed to create the MachineDeployment") - - result, err := r.Reconcile(t.Context(), reconcile.Request{NamespacedName: client.ObjectKeyFromObject(nodePool)}) - g.Expect(errors.Is(err, injectedErr)).To(BeTrue(), "NodePool reconciliation should preserve the injected MachineDeployment patch error") - g.Expect(err).To(MatchError(fmt.Sprintf("failed to reconcile MachineDeployment %q: %s", client.ObjectKeyFromObject(machineDeployment).String(), injectedErr)), "MachineDeployment reconciliation error should include the workload identity and original failure") - g.Expect(result).To(Equal(ctrl.Result{}), "failed NodePool reconciliation should return an empty result") - - persistedNodePool := &hyperv1.NodePool{} - g.Expect(testClient.Get(t.Context(), client.ObjectKeyFromObject(nodePool), persistedNodePool)).To(Succeed(), "failed to retrieve the NodePool after reconciliation failed") - g.Expect(FindStatusCondition(persistedNodePool.Status.Conditions, originalCondition.Type)).To(Equal(&originalCondition), "patching returned CAPI conditions on error should preserve unrelated condition fields and transition time") - machineTemplates := &capiaws.AWSMachineTemplateList{} - g.Expect(testClient.List(t.Context(), machineTemplates, client.InNamespace(controlPlaneNamespace))).To(Succeed(), "failed to list reconciled machine templates") - g.Expect(machineTemplates.Items).To(HaveLen(1), "reconciliation should persist exactly one machine template before the workload patch error") - - g.Expect(len(persistedNodePool.Status.Conditions)).To(BeNumerically(">=", 2), "persisted NodePool should contain both returned CAPI conditions despite the reconciliation error") - persistedCAPIConditions := append([]hyperv1.NodePoolCondition(nil), persistedNodePool.Status.Conditions[len(persistedNodePool.Status.Conditions)-2:]...) - for i := range persistedCAPIConditions { - g.Expect(persistedCAPIConditions[i].LastTransitionTime.IsZero()).To(BeFalse(), "persisted CAPI condition %d should have a transition time", i) - persistedCAPIConditions[i].LastTransitionTime = metav1.Time{} - } - g.Expect(persistedCAPIConditions).To(Equal([]hyperv1.NodePoolCondition{ - { - Type: hyperv1.NodePoolUpdatingPlatformMachineTemplateConditionType, - Status: corev1.ConditionTrue, + CanonicalComponents: map[string]string{ + haproxy.HAProxyRouterImageName: "quay.io/openshift-release-dev/ocp-v4.0-art-dev@sha256:1234", + }, + } + r := &NodePoolReconciler{ + Client: testClient, + ReleaseProvider: releaseProvider, + HypershiftOperatorImage: "quay.io/hypershift/hypershift-operator:latest", + ImageMetadataProvider: &fakeimagemetadataprovider.FakeRegistryClientImageMetadataProvider{ + Result: &dockerv1client.DockerImageConfig{Config: &docker10.DockerConfig{Labels: map[string]string{}}}, + }, + CreateOrUpdateProvider: upsert.New(false), + recorder: record.NewFakeRecorder(1), + } + token, err := r.token(t.Context(), hostedCluster, nodePool) + g.Expect(err).NotTo(HaveOccurred(), "failed to construct the NodePool token") + capi, err := newCAPI(token, hostedCluster.Spec.InfraID) + g.Expect(err).NotTo(HaveOccurred(), "failed to construct CAPI for the NodePool") + machineTemplate, err := capi.machineTemplateBuilders(t.Context()) + g.Expect(err).NotTo(HaveOccurred(), "failed to build the expected machine template") + machineDeployment.Spec.Template.Spec.Bootstrap.DataSecretName = ptr.To(capi.UserDataSecret().Name) + machineDeployment.Spec.Template.Spec.InfrastructureRef.Name = machineTemplate.GetName() + machineDeployment.Spec.Template.Spec.Version = capi.Version() + var workload client.Object = machineDeployment + if upgradeType == hyperv1.UpgradeTypeInPlace { + workload = &capiv1.MachineSet{ + ObjectMeta: metav1.ObjectMeta{Name: nodePoolName, Namespace: controlPlaneNamespace, Annotations: map[string]string{ + nodePoolAnnotationCurrentConfigVersion: capi.Hash(), nodePoolAnnotationTargetConfigVersion: capi.Hash(), + }}, + Spec: capiv1.MachineSetSpec{Replicas: ptr.To[int32](3), Template: machineDeployment.Spec.Template}, + Status: capiv1.MachineSetStatus{AvailableReplicas: ptr.To[int32](3), Conditions: machineDeployment.Status.Conditions}, + } + } + g.Expect(testClient.Create(t.Context(), workload)).To(Succeed(), "failed to create the workload") + + result, err := r.Reconcile(t.Context(), reconcile.Request{NamespacedName: client.ObjectKeyFromObject(nodePool)}) + g.Expect(errors.Is(err, injectedErr)).To(BeTrue(), "NodePool reconciliation should preserve the injected workload patch error") + g.Expect(err).To(MatchError(ContainSubstring(client.ObjectKeyFromObject(workload).String())), "workload error should include its identity") + g.Expect(testClient.Get(t.Context(), client.ObjectKeyFromObject(workload), workload)).To(Succeed()) + switch persisted := workload.(type) { + case *capiv1.MachineDeployment: + g.Expect(ptr.Deref(persisted.Spec.Replicas, 0)).To(Equal(int32(3))) + case *capiv1.MachineSet: + g.Expect(ptr.Deref(persisted.Spec.Replicas, 0)).To(Equal(int32(3))) + } + g.Expect(result).To(Equal(ctrl.Result{}), "failed NodePool reconciliation should return an empty result") + + persistedNodePool := &hyperv1.NodePool{} + g.Expect(testClient.Get(t.Context(), client.ObjectKeyFromObject(nodePool), persistedNodePool)).To(Succeed(), "failed to retrieve the NodePool after reconciliation failed") + g.Expect(FindStatusCondition(persistedNodePool.Status.Conditions, originalCondition.Type)).To(Equal(&originalCondition), "patching returned CAPI conditions on error should preserve unrelated condition fields and transition time") + machineTemplates := &capiaws.AWSMachineTemplateList{} + g.Expect(testClient.List(t.Context(), machineTemplates, client.InNamespace(controlPlaneNamespace))).To(Succeed(), "failed to list reconciled machine templates") + g.Expect(machineTemplates.Items).To(HaveLen(1), "reconciliation should persist exactly one machine template before the workload patch error") + + g.Expect(persistedNodePool.Generation).To(Equal(int64(17))) + g.Expect(ptr.Deref(persistedNodePool.Spec.Replicas, 0)).To(Equal(int32(5))) + g.Expect(FindStatusCondition(persistedNodePool.Status.Conditions, hyperv1.NodePoolReadyConditionType)).To(Equal(&originalReady), "failed workload persistence must not replace previous Ready") + platformCondition := FindStatusCondition(persistedNodePool.Status.Conditions, hyperv1.NodePoolUpdatingPlatformMachineTemplateConditionType) + g.Expect(platformCondition).NotTo(BeNil()) + g.Expect(platformCondition.LastTransitionTime.IsZero()).To(BeFalse()) + platformCondition.LastTransitionTime = metav1.Time{} + g.Expect(*platformCondition).To(Equal(hyperv1.NodePoolCondition{ + Type: hyperv1.NodePoolUpdatingPlatformMachineTemplateConditionType, Status: corev1.ConditionTrue, Reason: hyperv1.AsExpectedReason, Message: fmt.Sprintf("platform machine template update in progress. Target template: %s", machineTemplates.Items[0].Name), ObservedGeneration: nodePool.Generation, - }, - { - Type: hyperv1.NodePoolReadyConditionType, - Status: corev1.ConditionTrue, - Reason: "MachinesReady", - Message: "all machines are ready", - ObservedGeneration: nodePool.Generation, - }, - }), "NodePool patch-on-error should persist platform-update and Ready conditions in order with their complete fields") - }) + }), "conditions computed before workload persistence must survive failure") + }) + } } func TestEnqueueNodePoolsForCloudConfig(t *testing.T) {