diff --git a/hypershift-operator/controllers/nodepool/capi.go b/hypershift-operator/controllers/nodepool/capi.go index 09364d88771d..64ad1f3a7f89 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 + }) + 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) } + result.Conditions = append(result.Conditions, conditions...) + 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 + }) + 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) } + result.Conditions = append(result.Conditions, conditions...) + 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..e45767a29260 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,518 @@ 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 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 { + if _, ok := obj.(*capiv1.MachineDeployment); ok { + return injectedErr + } + return c.Patch(ctx, obj, patch, opts...) + }, + }, + expectedTypes: []string{ + hyperv1.NodePoolUpdatingPlatformMachineTemplateConditionType, + }, + }, + { + 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 { + if _, ok := obj.(*capiv1.MachineSet); ok { + return injectedErr + } + return c.Patch(ctx, obj, patch, opts...) + }, + }, + expectedTypes: []string{ + hyperv1.NodePoolUpdatingPlatformMachineTemplateConditionType, + }, + }, + { + 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 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, + 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 +2844,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 +2985,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 +2997,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 +3134,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 +3146,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 +3189,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 +3316,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 +3335,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 +4180,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 +4290,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 +4343,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 +4366,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 +4406,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 +4457,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 +5133,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 +5145,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 +5514,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 +5680,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..532f1b885e92 100644 --- a/hypershift-operator/controllers/nodepool/nodepool_controller.go +++ b/hypershift-operator/controllers/nodepool/nodepool_controller.go @@ -503,7 +503,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..57e8c98cd26b 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,261 @@ func TestNodePoolReconciler_reconcile(t *testing.T) { } } +func TestNodePoolReconciler_Reconcile(t *testing.T) { + for _, testCase := range []struct { + name string + upgradeType hyperv1.UpgradeType + }{ + {name: "When Replace workload persistence fails, it should preserve previous Ready and persist earlier conditions", upgradeType: hyperv1.UpgradeTypeReplace}, + {name: "When InPlace workload persistence fails, it should preserve previous Ready and persist earlier conditions", upgradeType: hyperv1.UpgradeTypeInPlace}, + } { + t.Run(testCase.name, func(t *testing.T) { + upgradeType := testCase.upgradeType + 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}, + }, + 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", + }, + }, + }, + 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] + 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", + }, + }, + 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 { + switch obj.(type) { + case *capiv1.MachineDeployment, *capiv1.MachineSet: + return injectedErr + } + return c.Patch(ctx, obj, patch, opts...) + }, + }). + Build() + + releaseProvider := &fakereleaseprovider.FakeReleaseProvider{ + Version: "4.18.0", + Components: map[string]string{ + "haproxy-router": "quay.io/openshift-release-dev/ocp-v4.0-art-dev@sha256:1234", + }, + CanonicalComponents: map[string]string{ + "haproxy-router": "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, + }), "conditions computed before workload persistence must survive failure") + }) + } +} + func TestEnqueueNodePoolsForCloudConfig(t *testing.T) { t.Parallel() hcNamespace := "clusters"