diff --git a/controlplane/kubeadm/internal/controllers/status.go b/controlplane/kubeadm/internal/controllers/status.go index 73afb280aa32..a4a91886fb89 100644 --- a/controlplane/kubeadm/internal/controllers/status.go +++ b/controlplane/kubeadm/internal/controllers/status.go @@ -230,7 +230,7 @@ func setScalingUpCondition(_ context.Context, kcp *controlplanev1.KubeadmControl if currentReplicas >= desiredReplicas { var message string if missingReferencesMessage != "" { - message = fmt.Sprintf("Scaling up would be blocked %s", missingReferencesMessage) + message = fmt.Sprintf("Scaling up would be blocked because %s", missingReferencesMessage) } v1beta2conditions.Set(kcp, metav1.Condition{ Type: controlplanev1.KubeadmControlPlaneScalingUpV1Beta2Condition, @@ -242,28 +242,21 @@ func setScalingUpCondition(_ context.Context, kcp *controlplanev1.KubeadmControl } message := fmt.Sprintf("Scaling up from %d to %d replicas", currentReplicas, desiredReplicas) - if missingReferencesMessage != "" { - message = fmt.Sprintf("%s is blocked %s", message, missingReferencesMessage) - } - messages := []string{message} - - if preflightChecks.HasDeletingMachine { - messages = append(messages, "waiting for Machine being deleted") - } - if preflightChecks.ControlPlaneComponentsNotHealthy { - messages = append(messages, "waiting for control plane components to be healthy") + additionalMessages := getPreflightMessages(preflightChecks) + if missingReferencesMessage != "" { + additionalMessages = append(additionalMessages, fmt.Sprintf("* %s", missingReferencesMessage)) } - if preflightChecks.EtcdClusterNotHealthy { - messages = append(messages, "waiting for etcd cluster to be healthy") + if len(additionalMessages) > 0 { + message += fmt.Sprintf(" is blocked because:\n%s", strings.Join(additionalMessages, "\n")) } v1beta2conditions.Set(kcp, metav1.Condition{ Type: controlplanev1.KubeadmControlPlaneScalingUpV1Beta2Condition, Status: metav1.ConditionTrue, Reason: controlplanev1.KubeadmControlPlaneScalingUpV1Beta2Reason, - Message: strings.Join(messages, "; "), + Message: message, }) } @@ -292,28 +285,22 @@ func setScalingDownCondition(_ context.Context, kcp *controlplanev1.KubeadmContr return } - messages := []string{fmt.Sprintf("Scaling down from %d to %d replicas", currentReplicas, desiredReplicas)} - if preflightChecks.HasDeletingMachine { - messages = append(messages, "waiting for Machine being deleted") - } + message := fmt.Sprintf("Scaling down from %d to %d replicas", currentReplicas, desiredReplicas) + additionalMessages := getPreflightMessages(preflightChecks) if staleMessage := aggregateStaleMachines(machines); staleMessage != "" { - messages = append(messages, staleMessage) - } - - if preflightChecks.ControlPlaneComponentsNotHealthy { - messages = append(messages, "waiting for control plane components to be healthy") + additionalMessages = append(additionalMessages, fmt.Sprintf("* %s", staleMessage)) } - if preflightChecks.EtcdClusterNotHealthy { - messages = append(messages, "waiting for etcd cluster to be healthy") + if len(additionalMessages) > 0 { + message += fmt.Sprintf(" is blocked because:\n%s", strings.Join(additionalMessages, "\n")) } v1beta2conditions.Set(kcp, metav1.Condition{ Type: controlplanev1.KubeadmControlPlaneScalingDownV1Beta2Condition, Status: metav1.ConditionTrue, Reason: controlplanev1.KubeadmControlPlaneScalingDownV1Beta2Reason, - Message: strings.Join(messages, "; "), + Message: message, }) } @@ -399,7 +386,7 @@ func setMachinesUpToDateCondition(ctx context.Context, kcp *controlplanev1.Kubea func calculateMissingReferencesMessage(kcp *controlplanev1.KubeadmControlPlane, infraMachineTemplateNotFound bool) string { if infraMachineTemplateNotFound { - return fmt.Sprintf("because %s does not exist", kcp.Spec.MachineTemplate.InfrastructureRef.Kind) + return fmt.Sprintf("%s does not exist", kcp.Spec.MachineTemplate.InfrastructureRef.Kind) } return "" } @@ -684,6 +671,22 @@ func minTime(t1, t2 time.Time) time.Time { return t1 } +func getPreflightMessages(preflightChecks internal.PreflightCheckResults) []string { + additionalMessages := []string{} + if preflightChecks.HasDeletingMachine { + additionalMessages = append(additionalMessages, "* waiting for a control plane Machine to complete deletion") + } + + if preflightChecks.ControlPlaneComponentsNotHealthy { + additionalMessages = append(additionalMessages, "* waiting for control plane components to be healthy") + } + + if preflightChecks.EtcdClusterNotHealthy { + additionalMessages = append(additionalMessages, "* waiting for etcd cluster to be healthy") + } + return additionalMessages +} + func aggregateStaleMachines(machines collections.Machines) string { if len(machines) == 0 { return "" diff --git a/controlplane/kubeadm/internal/controllers/status_test.go b/controlplane/kubeadm/internal/controllers/status_test.go index 1e421eb74274..655cf73cc408 100644 --- a/controlplane/kubeadm/internal/controllers/status_test.go +++ b/controlplane/kubeadm/internal/controllers/status_test.go @@ -233,10 +233,11 @@ func Test_setScalingUpCondition(t *testing.T) { InfraMachineTemplateIsNotFound: true, }, expectCondition: metav1.Condition{ - Type: controlplanev1.KubeadmControlPlaneScalingUpV1Beta2Condition, - Status: metav1.ConditionTrue, - Reason: controlplanev1.KubeadmControlPlaneScalingUpV1Beta2Reason, - Message: "Scaling up from 3 to 5 replicas is blocked because AWSTemplate does not exist", + Type: controlplanev1.KubeadmControlPlaneScalingUpV1Beta2Condition, + Status: metav1.ConditionTrue, + Reason: controlplanev1.KubeadmControlPlaneScalingUpV1Beta2Reason, + Message: "Scaling up from 3 to 5 replicas is blocked because:\n" + + "* AWSTemplate does not exist", }, }, { @@ -258,10 +259,13 @@ func Test_setScalingUpCondition(t *testing.T) { }, }, expectCondition: metav1.Condition{ - Type: controlplanev1.KubeadmControlPlaneScalingUpV1Beta2Condition, - Status: metav1.ConditionTrue, - Reason: controlplanev1.KubeadmControlPlaneScalingUpV1Beta2Reason, - Message: "Scaling up from 3 to 5 replicas; waiting for Machine being deleted; waiting for control plane components to be healthy; waiting for etcd cluster to be healthy", + Type: controlplanev1.KubeadmControlPlaneScalingUpV1Beta2Condition, + Status: metav1.ConditionTrue, + Reason: controlplanev1.KubeadmControlPlaneScalingUpV1Beta2Reason, + Message: "Scaling up from 3 to 5 replicas is blocked because:\n" + + "* waiting for a control plane Machine to complete deletion\n" + + "* waiting for control plane components to be healthy\n" + + "* waiting for etcd cluster to be healthy", }, }, } @@ -373,10 +377,11 @@ func Test_setScalingDownCondition(t *testing.T) { ), }, expectCondition: metav1.Condition{ - Type: controlplanev1.KubeadmControlPlaneScalingDownV1Beta2Condition, - Status: metav1.ConditionTrue, - Reason: controlplanev1.KubeadmControlPlaneScalingDownV1Beta2Reason, - Message: "Scaling down from 3 to 1 replicas; Machine m1 is in deletion since more than 30m", + Type: controlplanev1.KubeadmControlPlaneScalingDownV1Beta2Condition, + Status: metav1.ConditionTrue, + Reason: controlplanev1.KubeadmControlPlaneScalingDownV1Beta2Reason, + Message: "Scaling down from 3 to 1 replicas is blocked because:\n" + + "* Machine m1 is in deletion since more than 30m", }, }, { @@ -393,10 +398,11 @@ func Test_setScalingDownCondition(t *testing.T) { ), }, expectCondition: metav1.Condition{ - Type: controlplanev1.KubeadmControlPlaneScalingDownV1Beta2Condition, - Status: metav1.ConditionTrue, - Reason: controlplanev1.KubeadmControlPlaneScalingDownV1Beta2Reason, - Message: "Scaling down from 3 to 1 replicas; Machines m1, m2 are in deletion since more than 30m", + Type: controlplanev1.KubeadmControlPlaneScalingDownV1Beta2Condition, + Status: metav1.ConditionTrue, + Reason: controlplanev1.KubeadmControlPlaneScalingDownV1Beta2Reason, + Message: "Scaling down from 3 to 1 replicas is blocked because:\n" + + "* Machines m1, m2 are in deletion since more than 30m", }, }, { @@ -418,10 +424,13 @@ func Test_setScalingDownCondition(t *testing.T) { }, }, expectCondition: metav1.Condition{ - Type: controlplanev1.KubeadmControlPlaneScalingDownV1Beta2Condition, - Status: metav1.ConditionTrue, - Reason: controlplanev1.KubeadmControlPlaneScalingDownV1Beta2Reason, - Message: "Scaling down from 3 to 1 replicas; waiting for Machine being deleted; waiting for control plane components to be healthy; waiting for etcd cluster to be healthy", + Type: controlplanev1.KubeadmControlPlaneScalingDownV1Beta2Condition, + Status: metav1.ConditionTrue, + Reason: controlplanev1.KubeadmControlPlaneScalingDownV1Beta2Reason, + Message: "Scaling down from 3 to 1 replicas is blocked because:\n" + + "* waiting for a control plane Machine to complete deletion\n" + + "* waiting for control plane components to be healthy\n" + + "* waiting for etcd cluster to be healthy", }, }, } diff --git a/internal/controllers/machineset/machineset_controller.go b/internal/controllers/machineset/machineset_controller.go index 05a2b63544a6..a119c1b8afe3 100644 --- a/internal/controllers/machineset/machineset_controller.go +++ b/internal/controllers/machineset/machineset_controller.go @@ -272,7 +272,7 @@ type scope struct { infrastructureObjectNotFound bool getAndAdoptMachinesForMachineSetSucceeded bool owningMachineDeployment *clusterv1.MachineDeployment - scaleUpPreflightCheckErrMessage string + scaleUpPreflightCheckErrMessages []string reconciliationTime time.Time } @@ -673,15 +673,19 @@ func (r *Reconciler) syncReplicas(ctx context.Context, s *scope) (ctrl.Result, e } } - result, preflightCheckErrMessage, err := r.runPreflightChecks(ctx, cluster, ms, "Scale up") - if err != nil || !result.IsZero() { + preflightCheckErrMessages, err := r.runPreflightChecks(ctx, cluster, ms, "Scale up") + if err != nil || len(preflightCheckErrMessages) > 0 { if err != nil { - // If the error is not nil use that as the message for the condition. - preflightCheckErrMessage = err.Error() + // If err is not nil use that as the preflightCheckErrMessage + preflightCheckErrMessages = append(preflightCheckErrMessages, err.Error()) } - s.scaleUpPreflightCheckErrMessage = preflightCheckErrMessage - conditions.MarkFalse(ms, clusterv1.MachinesCreatedCondition, clusterv1.PreflightCheckFailedReason, clusterv1.ConditionSeverityError, preflightCheckErrMessage) - return result, err + + s.scaleUpPreflightCheckErrMessages = preflightCheckErrMessages + conditions.MarkFalse(ms, clusterv1.MachinesCreatedCondition, clusterv1.PreflightCheckFailedReason, clusterv1.ConditionSeverityError, strings.Join(preflightCheckErrMessages, "; ")) + if err != nil { + return ctrl.Result{}, err + } + return ctrl.Result{RequeueAfter: preflightFailedRequeueAfter}, nil } var ( @@ -1418,31 +1422,39 @@ func (r *Reconciler) reconcileUnhealthyMachines(ctx context.Context, s *scope) ( } // Run preflight checks. - preflightChecksResult, preflightCheckErrMessage, err := r.runPreflightChecks(ctx, cluster, ms, "Machine remediation") - if err != nil { - // If err is not nil use that as the preflightCheckErrMessage - preflightCheckErrMessage = err.Error() - } + preflightCheckErrMessages, err := r.runPreflightChecks(ctx, cluster, ms, "Machine remediation") + if err != nil || len(preflightCheckErrMessages) > 0 { + if err != nil { + // If err is not nil use that as the preflightCheckErrMessage + preflightCheckErrMessages = append(preflightCheckErrMessages, err.Error()) + } + + listMessages := make([]string, len(preflightCheckErrMessages)) + for i, msg := range preflightCheckErrMessages { + listMessages[i] = fmt.Sprintf("* %s", msg) + } - preflightChecksFailed := err != nil || !preflightChecksResult.IsZero() - if preflightChecksFailed { // PreflightChecks did not pass. Update the MachineOwnerRemediated condition on the unhealthy Machines with // WaitingForRemediationReason reason. - if err := patchMachineConditions(ctx, r.Client, machinesToRemediate, metav1.Condition{ + if patchErr := patchMachineConditions(ctx, r.Client, machinesToRemediate, metav1.Condition{ Type: clusterv1.MachineOwnerRemediatedV1Beta2Condition, Status: metav1.ConditionFalse, Reason: clusterv1.MachineSetMachineRemediationDeferredV1Beta2Reason, - Message: preflightCheckErrMessage, + Message: strings.Join(listMessages, "\n"), }, &clusterv1.Condition{ Type: clusterv1.MachineOwnerRemediatedCondition, Status: corev1.ConditionFalse, Reason: clusterv1.WaitingForRemediationReason, Severity: clusterv1.ConditionSeverityWarning, - Message: preflightCheckErrMessage, - }); err != nil { + Message: strings.Join(preflightCheckErrMessages, "; "), + }); patchErr != nil { + return ctrl.Result{}, kerrors.NewAggregate([]error{err, patchErr}) + } + + if err != nil { return ctrl.Result{}, err } - return preflightChecksResult, nil + return ctrl.Result{RequeueAfter: preflightFailedRequeueAfter}, nil } // PreflightChecks passed, so it is safe to remediate unhealthy machines by deleting them. diff --git a/internal/controllers/machineset/machineset_controller_status.go b/internal/controllers/machineset/machineset_controller_status.go index bc383f5d1320..7f08c0fb4125 100644 --- a/internal/controllers/machineset/machineset_controller_status.go +++ b/internal/controllers/machineset/machineset_controller_status.go @@ -19,7 +19,6 @@ package machineset import ( "context" "fmt" - "slices" "sort" "strings" "time" @@ -48,7 +47,7 @@ func (r *Reconciler) updateStatus(ctx context.Context, s *scope) { // Conditions // Update the ScalingUp and ScalingDown condition. - setScalingUpCondition(ctx, s.machineSet, s.machines, s.bootstrapObjectNotFound, s.infrastructureObjectNotFound, s.getAndAdoptMachinesForMachineSetSucceeded, s.scaleUpPreflightCheckErrMessage) + setScalingUpCondition(ctx, s.machineSet, s.machines, s.bootstrapObjectNotFound, s.infrastructureObjectNotFound, s.getAndAdoptMachinesForMachineSetSucceeded, s.scaleUpPreflightCheckErrMessages) setScalingDownCondition(ctx, s.machineSet, s.machines, s.getAndAdoptMachinesForMachineSetSucceeded) // MachinesReady condition: aggregate the Machine's Ready condition. @@ -93,7 +92,7 @@ func setReplicas(_ context.Context, ms *clusterv1.MachineSet, machines []*cluste ms.Status.V1Beta2.UpToDateReplicas = ptr.To(upToDateReplicas) } -func setScalingUpCondition(_ context.Context, ms *clusterv1.MachineSet, machines []*clusterv1.Machine, bootstrapObjectNotFound, infrastructureObjectNotFound, getAndAdoptMachinesForMachineSetSucceeded bool, scaleUpPreflightCheckErrMessage string) { +func setScalingUpCondition(_ context.Context, ms *clusterv1.MachineSet, machines []*clusterv1.Machine, bootstrapObjectNotFound, infrastructureObjectNotFound, getAndAdoptMachinesForMachineSetSucceeded bool, scaleUpPreflightCheckErrMessages []string) { // If we got unexpected errors in listing the machines (this should never happen), surface them. if !getAndAdoptMachinesForMachineSetSucceeded { v1beta2conditions.Set(ms, metav1.Condition{ @@ -140,11 +139,15 @@ func setScalingUpCondition(_ context.Context, ms *clusterv1.MachineSet, machines // Scaling up. message := fmt.Sprintf("Scaling up from %d to %d replicas", currentReplicas, desiredReplicas) - if missingReferencesMessage != "" || scaleUpPreflightCheckErrMessage != "" { - blockMessages := slices.DeleteFunc([]string{missingReferencesMessage, scaleUpPreflightCheckErrMessage}, func(s string) bool { - return s == "" - }) - message += fmt.Sprintf(" is blocked because %s", strings.Join(blockMessages, " and ")) + if missingReferencesMessage != "" || len(scaleUpPreflightCheckErrMessages) > 0 { + listMessages := make([]string, len(scaleUpPreflightCheckErrMessages)) + for i, msg := range scaleUpPreflightCheckErrMessages { + listMessages[i] = fmt.Sprintf("* %s", msg) + } + if missingReferencesMessage != "" { + listMessages = append(listMessages, fmt.Sprintf("* %s", missingReferencesMessage)) + } + message += fmt.Sprintf(" is blocked because:\n%s", strings.Join(listMessages, "\n")) } v1beta2conditions.Set(ms, metav1.Condition{ Type: clusterv1.MachineSetScalingUpV1Beta2Condition, diff --git a/internal/controllers/machineset/machineset_controller_status_test.go b/internal/controllers/machineset/machineset_controller_status_test.go index 043a7c57a05b..16e19f652cf0 100644 --- a/internal/controllers/machineset/machineset_controller_status_test.go +++ b/internal/controllers/machineset/machineset_controller_status_test.go @@ -194,7 +194,7 @@ func Test_setScalingUpCondition(t *testing.T) { bootstrapObjectNotFound bool infrastructureObjectNotFound bool getAndAdoptMachinesForMachineSetSucceeded bool - scaleUpPreflightCheckErrMessage string + scaleUpPreflightCheckErrMessages []string expectCondition metav1.Condition }{ { @@ -281,10 +281,11 @@ func Test_setScalingUpCondition(t *testing.T) { infrastructureObjectNotFound: false, getAndAdoptMachinesForMachineSetSucceeded: true, expectCondition: metav1.Condition{ - Type: clusterv1.MachineSetScalingUpV1Beta2Condition, - Status: metav1.ConditionTrue, - Reason: clusterv1.MachineSetScalingUpV1Beta2Reason, - Message: "Scaling up from 0 to 3 replicas is blocked because KubeadmBootstrapTemplate does not exist", + Type: clusterv1.MachineSetScalingUpV1Beta2Condition, + Status: metav1.ConditionTrue, + Reason: clusterv1.MachineSetScalingUpV1Beta2Reason, + Message: "Scaling up from 0 to 3 replicas is blocked because:\n" + + "* KubeadmBootstrapTemplate does not exist", }, }, { @@ -294,10 +295,11 @@ func Test_setScalingUpCondition(t *testing.T) { infrastructureObjectNotFound: true, getAndAdoptMachinesForMachineSetSucceeded: true, expectCondition: metav1.Condition{ - Type: clusterv1.MachineSetScalingUpV1Beta2Condition, - Status: metav1.ConditionTrue, - Reason: clusterv1.MachineSetScalingUpV1Beta2Reason, - Message: "Scaling up from 0 to 3 replicas is blocked because DockerMachineTemplate does not exist", + Type: clusterv1.MachineSetScalingUpV1Beta2Condition, + Status: metav1.ConditionTrue, + Reason: clusterv1.MachineSetScalingUpV1Beta2Reason, + Message: "Scaling up from 0 to 3 replicas is blocked because:\n" + + "* DockerMachineTemplate does not exist", }, }, { @@ -308,17 +310,14 @@ func Test_setScalingUpCondition(t *testing.T) { getAndAdoptMachinesForMachineSetSucceeded: true, // This preflight check error can happen when a MachineSet is scaling up while the control plane // already has a newer Kubernetes version. - scaleUpPreflightCheckErrMessage: "MachineSet version (1.25.5) and ControlPlane version (1.26.2) " + - "do not conform to kubeadm version skew policy as kubeadm only supports joining with the same " + - "major+minor version as the control plane (\"KubeadmVersionSkew\" preflight check failed)", + scaleUpPreflightCheckErrMessages: []string{"MachineSet version (1.25.5) and ControlPlane version (1.26.2) do not conform to kubeadm version skew policy as kubeadm only supports joining with the same major+minor version as the control plane (\"KubeadmVersionSkew\" preflight check failed)"}, expectCondition: metav1.Condition{ Type: clusterv1.MachineSetScalingUpV1Beta2Condition, Status: metav1.ConditionTrue, Reason: clusterv1.MachineSetScalingUpV1Beta2Reason, - Message: "Scaling up from 0 to 3 replicas is blocked because KubeadmBootstrapTemplate and DockerMachineTemplate " + - "do not exist and MachineSet version (1.25.5) and ControlPlane version (1.26.2) " + - "do not conform to kubeadm version skew policy as kubeadm only supports joining with the same " + - "major+minor version as the control plane (\"KubeadmVersionSkew\" preflight check failed)", + Message: "Scaling up from 0 to 3 replicas is blocked because:\n" + + "* MachineSet version (1.25.5) and ControlPlane version (1.26.2) do not conform to kubeadm version skew policy as kubeadm only supports joining with the same major+minor version as the control plane (\"KubeadmVersionSkew\" preflight check failed)\n" + + "* KubeadmBootstrapTemplate and DockerMachineTemplate do not exist", }, }, { @@ -339,7 +338,7 @@ func Test_setScalingUpCondition(t *testing.T) { t.Run(tt.name, func(t *testing.T) { g := NewWithT(t) - setScalingUpCondition(ctx, tt.ms, tt.machines, tt.bootstrapObjectNotFound, tt.infrastructureObjectNotFound, tt.getAndAdoptMachinesForMachineSetSucceeded, tt.scaleUpPreflightCheckErrMessage) + setScalingUpCondition(ctx, tt.ms, tt.machines, tt.bootstrapObjectNotFound, tt.infrastructureObjectNotFound, tt.getAndAdoptMachinesForMachineSetSucceeded, tt.scaleUpPreflightCheckErrMessages) condition := v1beta2conditions.Get(tt.ms, clusterv1.MachineSetScalingUpV1Beta2Condition) g.Expect(condition).ToNot(BeNil()) diff --git a/internal/controllers/machineset/machineset_controller_test.go b/internal/controllers/machineset/machineset_controller_test.go index 1e937ac0459e..4ba253cf31ea 100644 --- a/internal/controllers/machineset/machineset_controller_test.go +++ b/internal/controllers/machineset/machineset_controller_test.go @@ -1731,7 +1731,7 @@ func TestMachineSetReconciler_reconcileUnhealthyMachines(t *testing.T) { Type: clusterv1.MachineOwnerRemediatedV1Beta2Condition, Status: metav1.ConditionFalse, Reason: clusterv1.MachineSetMachineRemediationDeferredV1Beta2Reason, - Message: "GenericControlPlane default/cp1 is upgrading (\"ControlPlaneIsStable\" preflight check failed)", + Message: "* GenericControlPlane default/cp1 is upgrading (\"ControlPlaneIsStable\" preflight check failed)", }, v1beta2conditions.IgnoreLastTransitionTime(true))) // Verify the healthy machine is not deleted and does not have the OwnerRemediated condition. diff --git a/internal/controllers/machineset/machineset_preflight.go b/internal/controllers/machineset/machineset_preflight.go index 1f446eb96c81..ecb39fb07430 100644 --- a/internal/controllers/machineset/machineset_preflight.go +++ b/internal/controllers/machineset/machineset_preflight.go @@ -47,28 +47,28 @@ const preflightFailedRequeueAfter = 15 * time.Second var minVerKubernetesKubeletVersionSkewThree = semver.MustParse("1.28.0") -func (r *Reconciler) runPreflightChecks(ctx context.Context, cluster *clusterv1.Cluster, ms *clusterv1.MachineSet, action string) (_ ctrl.Result, message string, retErr error) { +func (r *Reconciler) runPreflightChecks(ctx context.Context, cluster *clusterv1.Cluster, ms *clusterv1.MachineSet, action string) ([]string, error) { log := ctrl.LoggerFrom(ctx) // If the MachineSetPreflightChecks feature gate is disabled return early. if !feature.Gates.Enabled(feature.MachineSetPreflightChecks) { - return ctrl.Result{}, "", nil + return nil, nil } skipped := skippedPreflightChecks(ms) // If all the preflight checks are skipped then return early. if skipped.Has(clusterv1.MachineSetPreflightCheckAll) { - return ctrl.Result{}, "", nil + return nil, nil } // If the cluster does not have a control plane reference then there is nothing to do. Return early. if cluster.Spec.ControlPlaneRef == nil { - return ctrl.Result{}, "", nil + return nil, nil } // Get the control plane object. controlPlane, err := external.Get(ctx, r.Client, cluster.Spec.ControlPlaneRef, cluster.Namespace) if err != nil { - return ctrl.Result{}, "", errors.Wrapf(err, "failed to perform %q: failed to perform preflight checks: failed to get ControlPlane %s", action, klog.KRef(cluster.Spec.ControlPlaneRef.Namespace, cluster.Spec.ControlPlaneRef.Name)) + return nil, errors.Wrapf(err, "failed to perform %q: failed to perform preflight checks: failed to get ControlPlane %s", action, klog.KRef(cluster.Spec.ControlPlaneRef.Namespace, cluster.Spec.ControlPlaneRef.Name)) } cpKlogRef := klog.KRef(controlPlane.GetNamespace(), controlPlane.GetName()) @@ -78,13 +78,13 @@ func (r *Reconciler) runPreflightChecks(ctx context.Context, cluster *clusterv1. cpVersion, err := contract.ControlPlane().Version().Get(controlPlane) if err != nil { if errors.Is(err, contract.ErrFieldNotFound) { - return ctrl.Result{}, "", nil + return nil, nil } - return ctrl.Result{}, "", errors.Wrapf(err, "failed to perform %q: failed to perform preflight checks: failed to get the version of ControlPlane %s", action, cpKlogRef) + return nil, errors.Wrapf(err, "failed to perform %q: failed to perform preflight checks: failed to get the version of ControlPlane %s", action, cpKlogRef) } cpSemver, err := semver.ParseTolerant(*cpVersion) if err != nil { - return ctrl.Result{}, "", errors.Wrapf(err, "failed to perform %q: failed to perform preflight checks: failed to parse version %q of ControlPlane %s", action, *cpVersion, cpKlogRef) + return nil, errors.Wrapf(err, "failed to perform %q: failed to perform preflight checks: failed to parse version %q of ControlPlane %s", action, *cpVersion, cpKlogRef) } errList := []error{} @@ -105,7 +105,7 @@ func (r *Reconciler) runPreflightChecks(ctx context.Context, cluster *clusterv1. msVersion := *ms.Spec.Template.Spec.Version msSemver, err := semver.ParseTolerant(msVersion) if err != nil { - return ctrl.Result{}, "", errors.Wrapf(err, "failed to perform %q: failed to perform preflight checks: failed to parse version %q of MachineSet %s", action, msVersion, klog.KObj(ms)) + return nil, errors.Wrapf(err, "failed to perform %q: failed to perform preflight checks: failed to parse version %q of MachineSet %s", action, msVersion, klog.KObj(ms)) } // Run the kubernetes-version skew preflight check. @@ -129,7 +129,7 @@ func (r *Reconciler) runPreflightChecks(ctx context.Context, cluster *clusterv1. } if len(errList) > 0 { - return ctrl.Result{}, "", errors.Wrapf(kerrors.NewAggregate(errList), "failed to perform %q: failed to perform preflight checks", action) + return nil, errors.Wrapf(kerrors.NewAggregate(errList), "failed to perform %q: failed to perform preflight checks", action) } if len(preflightCheckErrs) > 0 { preflightCheckErrStrings := []string{} @@ -137,9 +137,9 @@ func (r *Reconciler) runPreflightChecks(ctx context.Context, cluster *clusterv1. preflightCheckErrStrings = append(preflightCheckErrStrings, *v) } log.Info(fmt.Sprintf("%s on hold because %s. The operation will continue after the preflight check(s) pass", action, strings.Join(preflightCheckErrStrings, "; "))) - return ctrl.Result{RequeueAfter: preflightFailedRequeueAfter}, strings.Join(preflightCheckErrStrings, "; "), nil + return preflightCheckErrStrings, nil } - return ctrl.Result{}, "", nil + return nil, nil } func (r *Reconciler) controlPlaneStablePreflightCheck(controlPlane *unstructured.Unstructured) (preflightCheckErrorMessage, error) { diff --git a/internal/controllers/machineset/machineset_preflight_test.go b/internal/controllers/machineset/machineset_preflight_test.go index 58aca4af4fb9..116e5729d9f8 100644 --- a/internal/controllers/machineset/machineset_preflight_test.go +++ b/internal/controllers/machineset/machineset_preflight_test.go @@ -69,19 +69,19 @@ func TestMachineSetReconciler_runPreflightChecks(t *testing.T) { t.Run("should run preflight checks if the feature gate is enabled", func(t *testing.T) { tests := []struct { - name string - cluster *clusterv1.Cluster - controlPlane *unstructured.Unstructured - machineSet *clusterv1.MachineSet - wantPass bool - wantPreflightCheckErrMessage string - wantErr bool + name string + cluster *clusterv1.Cluster + controlPlane *unstructured.Unstructured + machineSet *clusterv1.MachineSet + wantMessages []string + wantErr bool }{ { - name: "should pass if cluster has no control plane", - cluster: &clusterv1.Cluster{}, - machineSet: &clusterv1.MachineSet{}, - wantPass: true, + name: "should pass if cluster has no control plane", + cluster: &clusterv1.Cluster{}, + machineSet: &clusterv1.MachineSet{}, + wantMessages: nil, + wantErr: false, }, { name: "should pass if the control plane version is not defined", @@ -95,7 +95,8 @@ func TestMachineSetReconciler_runPreflightChecks(t *testing.T) { }, controlPlane: controlPlaneWithNoVersion, machineSet: &clusterv1.MachineSet{}, - wantPass: true, + wantMessages: nil, + wantErr: false, }, { name: "should error if the control plane version is invalid", @@ -109,6 +110,7 @@ func TestMachineSetReconciler_runPreflightChecks(t *testing.T) { }, controlPlane: controlPlaneWithInvalidVersion, machineSet: &clusterv1.MachineSet{}, + wantMessages: nil, wantErr: true, }, { @@ -130,7 +132,8 @@ func TestMachineSetReconciler_runPreflightChecks(t *testing.T) { }, }, }, - wantPass: true, + wantMessages: nil, + wantErr: false, }, { name: "control plane preflight check: should fail if the control plane is provisioning", @@ -142,10 +145,12 @@ func TestMachineSetReconciler_runPreflightChecks(t *testing.T) { ControlPlaneRef: contract.ObjToRef(controlPlaneProvisioning), }, }, - controlPlane: controlPlaneProvisioning, - machineSet: &clusterv1.MachineSet{}, - wantPass: false, - wantPreflightCheckErrMessage: "GenericControlPlane ns1/cp1 is provisioning (\"ControlPlaneIsStable\" preflight check failed)", + controlPlane: controlPlaneProvisioning, + machineSet: &clusterv1.MachineSet{}, + wantMessages: []string{ + "GenericControlPlane ns1/cp1 is provisioning (\"ControlPlaneIsStable\" preflight check failed)", + }, + wantErr: false, }, { name: "control plane preflight check: should fail if the control plane is upgrading", @@ -157,10 +162,12 @@ func TestMachineSetReconciler_runPreflightChecks(t *testing.T) { ControlPlaneRef: contract.ObjToRef(controlPlaneUpgrading), }, }, - controlPlane: controlPlaneUpgrading, - machineSet: &clusterv1.MachineSet{}, - wantPass: false, - wantPreflightCheckErrMessage: "GenericControlPlane ns1/cp1 is upgrading (\"ControlPlaneIsStable\" preflight check failed)", + controlPlane: controlPlaneUpgrading, + machineSet: &clusterv1.MachineSet{}, + wantMessages: []string{ + "GenericControlPlane ns1/cp1 is upgrading (\"ControlPlaneIsStable\" preflight check failed)", + }, + wantErr: false, }, { name: "control plane preflight check: should pass if the control plane is upgrading but the preflight check is skipped", @@ -189,7 +196,8 @@ func TestMachineSetReconciler_runPreflightChecks(t *testing.T) { }, }, }, - wantPass: true, + wantMessages: nil, + wantErr: false, }, { name: "control plane preflight check: should pass if the control plane is stable", @@ -203,7 +211,8 @@ func TestMachineSetReconciler_runPreflightChecks(t *testing.T) { }, controlPlane: controlPlaneStable, machineSet: &clusterv1.MachineSet{}, - wantPass: true, + wantMessages: nil, + wantErr: false, }, { name: "should pass if the machine set version is not defined", @@ -222,7 +231,8 @@ func TestMachineSetReconciler_runPreflightChecks(t *testing.T) { }, Spec: clusterv1.MachineSetSpec{}, }, - wantPass: true, + wantMessages: nil, + wantErr: false, }, { name: "should error if the machine set version is invalid", @@ -247,7 +257,8 @@ func TestMachineSetReconciler_runPreflightChecks(t *testing.T) { }, }, }, - wantErr: true, + wantMessages: nil, + wantErr: true, }, { name: "kubernetes version preflight check: should fail if the machine set minor version is greater than control plane minor version", @@ -272,8 +283,10 @@ func TestMachineSetReconciler_runPreflightChecks(t *testing.T) { }, }, }, - wantPass: false, - wantPreflightCheckErrMessage: "MachineSet version (1.27.0) and ControlPlane version (1.26.2) do not conform to the kubernetes version skew policy as MachineSet version is higher than ControlPlane version (\"KubernetesVersionSkew\" preflight check failed)", + wantMessages: []string{ + "MachineSet version (1.27.0) and ControlPlane version (1.26.2) do not conform to the kubernetes version skew policy as MachineSet version is higher than ControlPlane version (\"KubernetesVersionSkew\" preflight check failed)", + }, + wantErr: false, }, { name: "kubernetes version preflight check: should fail if the machine set minor version is 4 older than control plane minor version for >= v1.28", @@ -298,8 +311,10 @@ func TestMachineSetReconciler_runPreflightChecks(t *testing.T) { }, }, }, - wantPass: false, - wantPreflightCheckErrMessage: "MachineSet version (1.24.0) and ControlPlane version (1.28.0) do not conform to the kubernetes version skew policy as MachineSet version is more than 3 minor versions older than the ControlPlane version (\"KubernetesVersionSkew\" preflight check failed)", + wantMessages: []string{ + "MachineSet version (1.24.0) and ControlPlane version (1.28.0) do not conform to the kubernetes version skew policy as MachineSet version is more than 3 minor versions older than the ControlPlane version (\"KubernetesVersionSkew\" preflight check failed)", + }, + wantErr: false, }, { name: "kubernetes version preflight check: should fail if the machine set minor version is 3 older than control plane minor version for < v1.28", @@ -324,8 +339,10 @@ func TestMachineSetReconciler_runPreflightChecks(t *testing.T) { }, }, }, - wantPass: false, - wantPreflightCheckErrMessage: "MachineSet version (1.23.0) and ControlPlane version (1.26.2) do not conform to the kubernetes version skew policy as MachineSet version is more than 2 minor versions older than the ControlPlane version (\"KubernetesVersionSkew\" preflight check failed)", + wantMessages: []string{ + "MachineSet version (1.23.0) and ControlPlane version (1.26.2) do not conform to the kubernetes version skew policy as MachineSet version is more than 2 minor versions older than the ControlPlane version (\"KubernetesVersionSkew\" preflight check failed)", + }, + wantErr: false, }, { name: "kubernetes version preflight check: should pass if the machine set minor version is greater than control plane minor version but the preflight check is skipped", @@ -353,7 +370,8 @@ func TestMachineSetReconciler_runPreflightChecks(t *testing.T) { }, }, }, - wantPass: true, + wantMessages: nil, + wantErr: false, }, { name: "kubernetes version preflight check: should pass if the machine set minor version and control plane version conform to kubernetes version skew policy >= v1.28", @@ -378,7 +396,8 @@ func TestMachineSetReconciler_runPreflightChecks(t *testing.T) { }, }, }, - wantPass: true, + wantMessages: nil, + wantErr: false, }, { name: "kubernetes version preflight check: should pass if the machine set minor version and control plane version conform to kubernetes version skew policy < v1.28", @@ -403,7 +422,8 @@ func TestMachineSetReconciler_runPreflightChecks(t *testing.T) { }, }, }, - wantPass: true, + wantMessages: nil, + wantErr: false, }, { name: "kubeadm version preflight check: should fail if the machine set version is not equal (major+minor) to control plane version when using kubeadm bootstrap provider", @@ -432,8 +452,10 @@ func TestMachineSetReconciler_runPreflightChecks(t *testing.T) { }, }, }, - wantPass: false, - wantPreflightCheckErrMessage: "MachineSet version (1.25.5) and ControlPlane version (1.26.2) do not conform to kubeadm version skew policy as kubeadm only supports joining with the same major+minor version as the control plane (\"KubeadmVersionSkew\" preflight check failed)", + wantMessages: []string{ + "MachineSet version (1.25.5) and ControlPlane version (1.26.2) do not conform to kubeadm version skew policy as kubeadm only supports joining with the same major+minor version as the control plane (\"KubeadmVersionSkew\" preflight check failed)", + }, + wantErr: false, }, { name: "kubeadm version preflight check: should pass if the machine set is not using kubeadm bootstrap provider", @@ -458,7 +480,8 @@ func TestMachineSetReconciler_runPreflightChecks(t *testing.T) { }, }, }, - wantPass: true, + wantMessages: nil, + wantErr: false, }, { name: "kubeadm version preflight check: should pass if the machine set version and control plane version do not conform to kubeadm version skew policy but the preflight check is skipped", @@ -490,7 +513,8 @@ func TestMachineSetReconciler_runPreflightChecks(t *testing.T) { }, }, }, - wantPass: true, + wantMessages: nil, + wantErr: false, }, { name: "kubeadm version preflight check: should pass if the machine set version and control plane version conform to kubeadm version skew when using kubeadm bootstrap provider", @@ -519,7 +543,8 @@ func TestMachineSetReconciler_runPreflightChecks(t *testing.T) { }, }, }, - wantPass: true, + wantMessages: nil, + wantErr: false, }, { name: "kubeadm version preflight check: should error if the bootstrap ref APIVersion is invalid", @@ -548,7 +573,8 @@ func TestMachineSetReconciler_runPreflightChecks(t *testing.T) { }, }, }, - wantErr: true, + wantMessages: nil, + wantErr: true, }, } @@ -563,14 +589,13 @@ func TestMachineSetReconciler_runPreflightChecks(t *testing.T) { r := &Reconciler{ Client: fakeClient, } - result, preflightCheckErrMessage, err := r.runPreflightChecks(ctx, tt.cluster, tt.machineSet, "") + preflightCheckErrMessage, err := r.runPreflightChecks(ctx, tt.cluster, tt.machineSet, "") if tt.wantErr { g.Expect(err).To(HaveOccurred()) } else { g.Expect(err).ToNot(HaveOccurred()) - g.Expect(result.IsZero()).To(Equal(tt.wantPass)) } - g.Expect(preflightCheckErrMessage).To(BeComparableTo(tt.wantPreflightCheckErrMessage)) + g.Expect(preflightCheckErrMessage).To(BeComparableTo(tt.wantMessages)) }) } }) @@ -606,8 +631,8 @@ func TestMachineSetReconciler_runPreflightChecks(t *testing.T) { } fakeClient := fake.NewClientBuilder().WithObjects(controlPlane).Build() r := &Reconciler{Client: fakeClient} - result, _, err := r.runPreflightChecks(ctx, cluster, machineSet, "") + messages, err := r.runPreflightChecks(ctx, cluster, machineSet, "") g.Expect(err).ToNot(HaveOccurred()) - g.Expect(result.IsZero()).To(BeTrue()) + g.Expect(messages).To(BeNil()) }) }