diff --git a/hypershift-operator/controllers/nodepool/capi.go b/hypershift-operator/controllers/nodepool/capi.go index c14504b8f0b9..8e44e58d63b4 100644 --- a/hypershift-operator/controllers/nodepool/capi.go +++ b/hypershift-operator/controllers/nodepool/capi.go @@ -46,6 +46,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. @@ -54,6 +60,7 @@ type CAPI struct { capiClusterName string scaleFromZeroPlatform hyperv1.PlatformType upsert.ApplyProvider + conditions []hyperv1.NodePoolCondition } // hasStatusCapacity checks if a machine template has Status.Capacity populated @@ -84,28 +91,28 @@ 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) nodePool := c.nodePool if err := c.cleanupMachineTemplates(ctx, log, nodePool, c.controlplaneNamespace); err != nil { - return err + return nil, err } if c.nodePool.Spec.Platform.Type == hyperv1.AWSPlatform { if err := c.reconcileAWSMachines(ctx); err != nil { - return err + return nil, err } } // Reconcile (Platform)MachineTemplate. template, err := c.machineTemplateBuilders(ctx) if err != nil { - return err + return nil, err } if result, err := c.ApplyManifest(ctx, c.Client, template); err != nil { - return err + return nil, err } else { log.Info("Reconciled Machine template", "result", result) } @@ -113,8 +120,7 @@ func (c *CAPI) Reconcile(ctx context.Context) error { // 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{ + c.conditions = append(c.conditions, hyperv1.NodePoolCondition{ Type: hyperv1.NodePoolUpdatingPlatformMachineTemplateConditionType, Status: corev1.ConditionTrue, Reason: hyperv1.AsExpectedReason, @@ -125,7 +131,7 @@ func (c *CAPI) Reconcile(ctx context.Context) error { "current", nodePool.GetAnnotations()[nodePoolAnnotationPlatformMachineTemplate], "target", targetMachineTemplate) } else { - SetStatusCondition(&nodePool.Status.Conditions, hyperv1.NodePoolCondition{ + c.conditions = append(c.conditions, hyperv1.NodePoolCondition{ Type: hyperv1.NodePoolUpdatingPlatformMachineTemplateConditionType, Status: corev1.ConditionFalse, Reason: hyperv1.AsExpectedReason, @@ -141,7 +147,7 @@ func (c *CAPI) Reconcile(ctx context.Context) error { ms, template) }); err != nil { - return fmt.Errorf("failed to reconcile MachineSet %q: %w", + return nil, fmt.Errorf("failed to reconcile MachineSet %q: %w", client.ObjectKeyFromObject(ms).String(), err) } else { log.Info("Reconciled MachineSet", "result", result) @@ -157,7 +163,7 @@ func (c *CAPI) Reconcile(ctx context.Context) error { md, template) }); err != nil { - return fmt.Errorf("failed to reconcile MachineDeployment %q: %w", + return nil, fmt.Errorf("failed to reconcile MachineDeployment %q: %w", client.ObjectKeyFromObject(md).String(), err) } else { log.Info("Reconciled MachineDeployment", "result", result) @@ -166,20 +172,20 @@ func (c *CAPI) Reconcile(ctx context.Context) error { 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 &CAPIResult{Conditions: c.conditions}, nil } if result, 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 nil, fmt.Errorf("failed to reconcile MachineHealthCheck %q: %w", client.ObjectKeyFromObject(mhc).String(), err) } else { log.Info("Reconciled MachineHealthCheck", "result", result) } - SetStatusCondition(&nodePool.Status.Conditions, hyperv1.NodePoolCondition{ + c.conditions = append(c.conditions, hyperv1.NodePoolCondition{ Type: hyperv1.NodePoolAutorepairEnabledConditionType, Status: corev1.ConditionTrue, Reason: hyperv1.AsExpectedReason, @@ -188,14 +194,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 nil, err } if err == nil { if err := c.Delete(ctx, mhc); err != nil && !apierrors.IsNotFound(err) { - return err + return nil, err } } - SetStatusCondition(&nodePool.Status.Conditions, hyperv1.NodePoolCondition{ + c.conditions = append(c.conditions, hyperv1.NodePoolCondition{ Type: hyperv1.NodePoolAutorepairEnabledConditionType, Status: corev1.ConditionFalse, Reason: hyperv1.AsExpectedReason, @@ -209,7 +215,7 @@ func (c *CAPI) Reconcile(ctx context.Context) error { if result, 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 nil, fmt.Errorf("failed to reconcile spot MachineHealthCheck %q: %w", client.ObjectKeyFromObject(spotMHC).String(), err) } else { log.Info("Reconciled spot MachineHealthCheck", "result", result) @@ -218,16 +224,16 @@ func (c *CAPI) Reconcile(ctx context.Context) error { // 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 nil, err } if err == nil { if err := c.Delete(ctx, spotMHC); err != nil && !apierrors.IsNotFound(err) { - return err + return nil, err } } } - return nil + return &CAPIResult{Conditions: c.conditions}, nil } func (c *CAPI) cleanupMachineTemplates(ctx context.Context, log logr.Logger, nodePool *hyperv1.NodePool, controlPlaneNamespace string) error { @@ -653,7 +659,7 @@ func (c *CAPI) reconcileMachineDeploymentStatus(log logr.Logger, machineDeployme if cond.Reason != "" { reason = cond.Reason } - SetStatusCondition(&nodePool.Status.Conditions, hyperv1.NodePoolCondition{ + c.conditions = append(c.conditions, hyperv1.NodePoolCondition{ Type: hyperv1.NodePoolReadyConditionType, Status: cond.Status, ObservedGeneration: nodePool.Generation, @@ -1047,22 +1053,22 @@ func (c *CAPI) reconcileMachineSet(ctx context.Context, // Bubble up AvailableReplicas and Ready condition from MachineSet. nodePool.Status.Replicas = machineSet.Status.AvailableReplicas - for _, c := range machineSet.Status.Conditions { + for _, cond := range machineSet.Status.Conditions { // This condition should aggregate and summarize readiness from underlying MachineSets and Machines // https://github.com/kubernetes-sigs/cluster-api/issues/3486. - if c.Type == capiv1.ReadyCondition { + if cond.Type == capiv1.ReadyCondition { // 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 c.Reason != "" { - reason = c.Reason + if cond.Reason != "" { + reason = cond.Reason } - SetStatusCondition(&nodePool.Status.Conditions, hyperv1.NodePoolCondition{ + c.conditions = append(c.conditions, hyperv1.NodePoolCondition{ Type: hyperv1.NodePoolReadyConditionType, - Status: c.Status, + Status: cond.Status, ObservedGeneration: nodePool.Generation, - Message: c.Message, + Message: cond.Message, Reason: reason, }) break diff --git a/hypershift-operator/controllers/nodepool/capi_test.go b/hypershift-operator/controllers/nodepool/capi_test.go index 28c166d11516..69dbe1919059 100644 --- a/hypershift-operator/controllers/nodepool/capi_test.go +++ b/hypershift-operator/controllers/nodepool/capi_test.go @@ -1632,6 +1632,96 @@ 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.AWSResourceTag{}, + 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: corev1.ObjectReference{ + Kind: "AWSMachineTemplate", + APIVersion: "infrastructure.cluster.x-k8s.io/v1beta2", + Namespace: "test-namespace-test-cluster", + 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{ @@ -1881,11 +1971,13 @@ 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()) } else { g.Expect(err).NotTo(HaveOccurred()) + g.Expect(capiResult).NotTo(BeNil()) + g.Expect(capiResult.Conditions).NotTo(BeEmpty()) // Check that old machine templates are deleted. templateList := &capiaws.AWSMachineTemplateList{} @@ -1943,16 +2035,54 @@ func TestCAPIReconcile(t *testing.T) { g.Expect(md.Annotations).To(HaveKeyWithValue(autoscalerMinAnnotation, "0")) } + // Validate that UpdatingPlatformMachineTemplate condition is present in CAPIResult. + foundUpdatingCondition := false + for _, cond := range capiResult.Conditions { + if cond.Type == hyperv1.NodePoolUpdatingPlatformMachineTemplateConditionType { + foundUpdatingCondition = true + break + } + } + g.Expect(foundUpdatingCondition).To(BeTrue(), "expected UpdatingPlatformMachineTemplate condition in CAPIResult") + // Check MachineHealthCheck - if tt.nodePool.Spec.Management.AutoRepair { + reachedIgnition := FindStatusCondition(tt.nodePool.Status.Conditions, hyperv1.NodePoolReachedIgnitionEndpoint) + 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)) + + // Validate autorepair enabled condition is present. + foundAutorepair := false + for _, cond := range capiResult.Conditions { + if cond.Type == hyperv1.NodePoolAutorepairEnabledConditionType { + g.Expect(cond.Status).To(Equal(corev1.ConditionTrue)) + foundAutorepair = true + break + } + } + g.Expect(foundAutorepair).To(BeTrue(), "expected AutorepairEnabled condition in CAPIResult") + } 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()) + + // Validate autorepair disabled condition is present. + foundAutorepair := false + for _, cond := range capiResult.Conditions { + if cond.Type == hyperv1.NodePoolAutorepairEnabledConditionType { + g.Expect(cond.Status).To(Equal(corev1.ConditionFalse)) + foundAutorepair = true + break + } + } + g.Expect(foundAutorepair).To(BeTrue(), "expected AutorepairEnabled=False condition in CAPIResult") } // Check spot MHC and interruptible label @@ -1995,8 +2125,10 @@ func TestCAPIReconcile(t *testing.T) { g.Expect(err).NotTo(HaveOccurred()) // Re-run reconcile. - err = capi.Reconcile(t.Context()) + capiResult, err = capi.Reconcile(t.Context()) g.Expect(err).NotTo(HaveOccurred()) + g.Expect(capiResult).NotTo(BeNil()) + g.Expect(capiResult.Conditions).NotTo(BeEmpty()) // Check for the expected annotations. // TODO(alberto): reconcileMachineDeployment mutate the NodePool with this annotations and status.version. @@ -2165,7 +2297,7 @@ func TestCAPIReconcile_machineset(t *testing.T) { ApplyProvider: upsert.NewApplyProvider(false), } - err := capi.Reconcile(t.Context()) + capiResult, err := capi.Reconcile(t.Context()) g.Expect(err).NotTo(HaveOccurred()) ms := &capiv1.MachineSet{} @@ -2173,10 +2305,167 @@ func TestCAPIReconcile_machineset(t *testing.T) { g.Expect(err).NotTo(HaveOccurred()) g.Expect(ms.Spec.Template.Spec.NodeDrainTimeout).To(Equal(tt.nodePool.Spec.NodeDrainTimeout)) g.Expect(ms.Spec.Template.Spec.NodeVolumeDetachTimeout).To(Equal(tt.nodePool.Spec.NodeVolumeDetachTimeout)) + + g.Expect(capiResult).NotTo(BeNil()) + g.Expect(capiResult.Conditions).NotTo(BeEmpty()) }) } } +func TestCAPIReconcile_machineset_readyCondition(t *testing.T) { + t.Parallel() + g := NewWithT(t) + + awsMachineTemplateName := "test-nodepool-28d5cf5a" + capiClusterName := "infra-id" + + nodePool := &hyperv1.NodePool{ + ObjectMeta: metav1.ObjectMeta{ + Name: "test-nodepool", + Namespace: "test-namespace", + }, + 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.AWSResourceTag{}, + 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: corev1.ObjectReference{ + Kind: "AWSMachineTemplate", + APIVersion: "infrastructure.cluster.x-k8s.io/v1beta2", + Namespace: "test-namespace-test-cluster", + 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", + }, + }, + }, + } + + c := fake.NewClientBuilder(). + WithScheme(api.Scheme). + WithObjects(nodePool, existingMachineSet). + WithObjects(templates...). + Build() + + controlplaneNamespace := manifests.HostedControlPlaneNamespace(hostedCluster.Namespace, hostedCluster.Name) + capi := &CAPI{ + Token: &Token{ + ConfigGenerator: &ConfigGenerator{ + Client: c, + 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: capiClusterName, + ApplyProvider: upsert.NewApplyProvider(false), + } + + // First reconcile: MachineSet gets updated with user data, version, and config. + _, err := capi.Reconcile(t.Context()) + g.Expect(err).NotTo(HaveOccurred()) + + // 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()) + + ms.Status.AvailableReplicas = 3 + ms.Status.Conditions = append(ms.Status.Conditions, capiv1.Condition{ + Type: capiv1.ReadyCondition, + Status: corev1.ConditionTrue, + Reason: "MachinesReady", + Message: "All machines are ready", + }) + err = capi.Client.Update(t.Context(), ms) + g.Expect(err).NotTo(HaveOccurred()) + + // Reset accumulated conditions for the second reconcile. + capi.conditions = nil + + // Second reconcile: MachineSet now has matching data, so isUpdating=false + // and the ReadyCondition from MachineSet status is propagated to CAPIResult. + capiResult, err := capi.Reconcile(t.Context()) + g.Expect(err).NotTo(HaveOccurred()) + g.Expect(capiResult).NotTo(BeNil()) + g.Expect(capiResult.Conditions).NotTo(BeEmpty()) + + foundReadyCondition := false + for _, cond := range capiResult.Conditions { + if cond.Type == hyperv1.NodePoolReadyConditionType { + g.Expect(cond.Status).To(Equal(corev1.ConditionTrue)) + g.Expect(cond.Reason).To(Equal("MachinesReady")) + g.Expect(cond.Message).To(Equal("All machines are ready")) + foundReadyCondition = true + break + } + } + g.Expect(foundReadyCondition).To(BeTrue(), "expected NodePoolReady condition propagated from MachineSet ReadyCondition") +} + func TestGlobalPSManagedLabelOnMachines(t *testing.T) { t.Parallel() maxUnavailable := intstr.FromInt(0) @@ -2190,7 +2479,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 }{ { @@ -2332,10 +2621,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{ @@ -2344,7 +2633,7 @@ func TestGlobalPSManagedLabelOnMachines(t *testing.T) { }, } log := ctrl.LoggerFrom(t.Context()) - return capi.reconcileMachineDeployment(t.Context(), log, md, template) + return &CAPIResult{Conditions: capi.conditions}, capi.reconcileMachineDeployment(t.Context(), log, md, template) }, expectLabel: true, }, @@ -2481,10 +2770,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{ @@ -2493,7 +2782,7 @@ func TestGlobalPSManagedLabelOnMachines(t *testing.T) { }, } log := ctrl.LoggerFrom(t.Context()) - return capi.reconcileMachineDeployment(t.Context(), log, md, kvTemplate) + return &CAPIResult{Conditions: capi.conditions}, capi.reconcileMachineDeployment(t.Context(), log, md, kvTemplate) }, }, } @@ -2535,11 +2824,11 @@ 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) + _, err := reconcile(t, capi) g.Expect(err).NotTo(HaveOccurred()) globalPSManagedLabelKey := fmt.Sprintf("%s.%s", labelManagedPrefix, globalPSNodeLabel) @@ -3063,6 +3352,9 @@ func TestReconcileMachineDeploymentStatus(t *testing.T) { } capi.reconcileMachineDeploymentStatus(logr.Discard(), tc.machineDeployment, templateCR) + for _, cond := range capi.conditions { + SetStatusCondition(&nodePool.Status.Conditions, cond) + } g.Expect(nodePool.Status.Replicas).To(Equal(tc.expectedReplicas)) g.Expect(nodePool.Status.Version).To(Equal(tc.expectedVersion)) diff --git a/hypershift-operator/controllers/nodepool/nodepool_controller.go b/hypershift-operator/controllers/nodepool/nodepool_controller.go index aab79922eefd..ee8b85fd5dc5 100644 --- a/hypershift-operator/controllers/nodepool/nodepool_controller.go +++ b/hypershift-operator/controllers/nodepool/nodepool_controller.go @@ -315,9 +315,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) @@ -429,7 +426,8 @@ 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 err != nil { var notReadyErr *NotReadyError if coreerrors.As(err, ¬ReadyErr) { log.Info("Waiting to create machine template", "message", err.Error()) @@ -437,6 +435,9 @@ func (r *NodePoolReconciler) reconcile(ctx context.Context, hcluster *hyperv1.Ho } return ctrl.Result{}, err } + for _, condition := range capiResult.Conditions { + SetStatusCondition(&nodePool.Status.Conditions, condition) + } // Set scale-from-zero annotations if provider is configured and platform is supported // This works for both Replace (MachineDeployment) and InPlace (MachineSet) upgrade types