From 69ce798b090517c8577ff4c7e0d3f9b789f153ce Mon Sep 17 00:00:00 2001 From: killianmuldoon Date: Tue, 7 Mar 2023 12:16:45 +0000 Subject: [PATCH] Rename md parameters for consistency Signed-off-by: killianmuldoon --- .../client/alpha/machinedeployment.go | 8 +- .../client/alpha/rollout_rollbacker.go | 10 +-- .../machinedeployment_controller.go | 74 +++++++++---------- .../machinedeployment_rolling.go | 16 ++-- .../machinedeployment_rollout_ondelete.go | 16 ++-- .../machinedeployment_sync.go | 54 +++++++------- .../machinedeployment/mdutil/util.go | 12 +-- .../machinedeployment/mdutil/util_test.go | 28 +++---- 8 files changed, 109 insertions(+), 109 deletions(-) diff --git a/cmd/clusterctl/client/alpha/machinedeployment.go b/cmd/clusterctl/client/alpha/machinedeployment.go index f5e66d21c5e0..d4466c511330 100644 --- a/cmd/clusterctl/client/alpha/machinedeployment.go +++ b/cmd/clusterctl/client/alpha/machinedeployment.go @@ -118,7 +118,7 @@ func findMachineDeploymentRevision(toRevision int64, allMSs []*clusterv1.Machine } // getMachineSetsForDeployment returns a list of MachineSets associated with a MachineDeployment. -func getMachineSetsForDeployment(proxy cluster.Proxy, d *clusterv1.MachineDeployment) ([]*clusterv1.MachineSet, error) { +func getMachineSetsForDeployment(proxy cluster.Proxy, md *clusterv1.MachineDeployment) ([]*clusterv1.MachineSet, error) { log := logf.Log c, err := proxy.NewClient() if err != nil { @@ -126,7 +126,7 @@ func getMachineSetsForDeployment(proxy cluster.Proxy, d *clusterv1.MachineDeploy } // List all MachineSets to find those we own but that no longer match our selector. machineSets := &clusterv1.MachineSetList{} - if err := c.List(ctx, machineSets, client.InNamespace(d.Namespace)); err != nil { + if err := c.List(ctx, machineSets, client.InNamespace(md.Namespace)); err != nil { return nil, err } @@ -135,12 +135,12 @@ func getMachineSetsForDeployment(proxy cluster.Proxy, d *clusterv1.MachineDeploy ms := &machineSets.Items[idx] // Skip this MachineSet if its controller ref is not pointing to this MachineDeployment - if !metav1.IsControlledBy(ms, d) { + if !metav1.IsControlledBy(ms, md) { log.V(5).Info("Skipping MachineSet, controller ref does not match MachineDeployment", "machineset", ms.Name) continue } - selector, err := metav1.LabelSelectorAsSelector(&d.Spec.Selector) + selector, err := metav1.LabelSelectorAsSelector(&md.Spec.Selector) if err != nil { log.V(5).Info("Skipping MachineSet, failed to get label selector from spec selector", "machineset", ms.Name) continue diff --git a/cmd/clusterctl/client/alpha/rollout_rollbacker.go b/cmd/clusterctl/client/alpha/rollout_rollbacker.go index d2c7e69dfa7f..d9832e7dbee6 100644 --- a/cmd/clusterctl/client/alpha/rollout_rollbacker.go +++ b/cmd/clusterctl/client/alpha/rollout_rollbacker.go @@ -47,7 +47,7 @@ func (r *rollout) ObjectRollbacker(proxy cluster.Proxy, ref corev1.ObjectReferen } // rollbackMachineDeployment will rollback to a previous MachineSet revision used by this MachineDeployment. -func rollbackMachineDeployment(proxy cluster.Proxy, d *clusterv1.MachineDeployment, toRevision int64) error { +func rollbackMachineDeployment(proxy cluster.Proxy, md *clusterv1.MachineDeployment, toRevision int64) error { log := logf.Log c, err := proxy.NewClient() if err != nil { @@ -57,7 +57,7 @@ func rollbackMachineDeployment(proxy cluster.Proxy, d *clusterv1.MachineDeployme if toRevision < 0 { return errors.Errorf("revision number cannot be negative: %v", toRevision) } - msList, err := getMachineSetsForDeployment(proxy, d) + msList, err := getMachineSetsForDeployment(proxy, md) if err != nil { return err } @@ -67,7 +67,7 @@ func rollbackMachineDeployment(proxy cluster.Proxy, d *clusterv1.MachineDeployme return err } log.V(7).Info("Found revision", "revision", msForRevision) - patchHelper, err := patch.NewHelper(d, c) + patchHelper, err := patch.NewHelper(md, c) if err != nil { return err } @@ -75,6 +75,6 @@ func rollbackMachineDeployment(proxy cluster.Proxy, d *clusterv1.MachineDeployme revMSTemplate := *msForRevision.Spec.Template.DeepCopy() delete(revMSTemplate.Labels, clusterv1.MachineDeploymentUniqueLabel) - d.Spec.Template = revMSTemplate - return patchHelper.Patch(ctx, d) + md.Spec.Template = revMSTemplate + return patchHelper.Patch(ctx, md) } diff --git a/internal/controllers/machinedeployment/machinedeployment_controller.go b/internal/controllers/machinedeployment/machinedeployment_controller.go index 20a212497d44..7e193299d61b 100644 --- a/internal/controllers/machinedeployment/machinedeployment_controller.go +++ b/internal/controllers/machinedeployment/machinedeployment_controller.go @@ -172,9 +172,9 @@ func (r *Reconciler) Reconcile(ctx context.Context, req ctrl.Request) (_ ctrl.Re return result, err } -func patchMachineDeployment(ctx context.Context, patchHelper *patch.Helper, d *clusterv1.MachineDeployment, options ...patch.Option) error { +func patchMachineDeployment(ctx context.Context, patchHelper *patch.Helper, md *clusterv1.MachineDeployment, options ...patch.Option) error { // Always update the readyCondition by summarizing the state of other conditions. - conditions.SetSummary(d, + conditions.SetSummary(md, conditions.WithConditions( clusterv1.MachineDeploymentAvailableCondition, ), @@ -187,29 +187,29 @@ func patchMachineDeployment(ctx context.Context, patchHelper *patch.Helper, d *c clusterv1.MachineDeploymentAvailableCondition, }}, ) - return patchHelper.Patch(ctx, d, options...) + return patchHelper.Patch(ctx, md, options...) } -func (r *Reconciler) reconcile(ctx context.Context, cluster *clusterv1.Cluster, d *clusterv1.MachineDeployment) (ctrl.Result, error) { +func (r *Reconciler) reconcile(ctx context.Context, cluster *clusterv1.Cluster, md *clusterv1.MachineDeployment) (ctrl.Result, error) { log := ctrl.LoggerFrom(ctx) log.V(4).Info("Reconcile MachineDeployment") // Reconcile and retrieve the Cluster object. - if d.Labels == nil { - d.Labels = make(map[string]string) + if md.Labels == nil { + md.Labels = make(map[string]string) } - if d.Spec.Selector.MatchLabels == nil { - d.Spec.Selector.MatchLabels = make(map[string]string) + if md.Spec.Selector.MatchLabels == nil { + md.Spec.Selector.MatchLabels = make(map[string]string) } - if d.Spec.Template.Labels == nil { - d.Spec.Template.Labels = make(map[string]string) + if md.Spec.Template.Labels == nil { + md.Spec.Template.Labels = make(map[string]string) } - d.Labels[clusterv1.ClusterNameLabel] = d.Spec.ClusterName + md.Labels[clusterv1.ClusterNameLabel] = md.Spec.ClusterName // Set the MachineDeployment as directly owned by the Cluster (if not already present). - if r.shouldAdopt(d) { - d.OwnerReferences = util.EnsureOwnerRef(d.OwnerReferences, metav1.OwnerReference{ + if r.shouldAdopt(md) { + md.OwnerReferences = util.EnsureOwnerRef(md.OwnerReferences, metav1.OwnerReference{ APIVersion: clusterv1.GroupVersion.String(), Kind: "Cluster", Name: cluster.Name, @@ -219,17 +219,17 @@ func (r *Reconciler) reconcile(ctx context.Context, cluster *clusterv1.Cluster, } // Make sure to reconcile the external infrastructure reference. - if err := reconcileExternalTemplateReference(ctx, r.Client, cluster, &d.Spec.Template.Spec.InfrastructureRef); err != nil { + if err := reconcileExternalTemplateReference(ctx, r.Client, cluster, &md.Spec.Template.Spec.InfrastructureRef); err != nil { return ctrl.Result{}, err } // Make sure to reconcile the external bootstrap reference, if any. - if d.Spec.Template.Spec.Bootstrap.ConfigRef != nil { - if err := reconcileExternalTemplateReference(ctx, r.Client, cluster, d.Spec.Template.Spec.Bootstrap.ConfigRef); err != nil { + if md.Spec.Template.Spec.Bootstrap.ConfigRef != nil { + if err := reconcileExternalTemplateReference(ctx, r.Client, cluster, md.Spec.Template.Spec.Bootstrap.ConfigRef); err != nil { return ctrl.Result{}, err } } - msList, err := r.getMachineSetsForDeployment(ctx, d) + msList, err := r.getMachineSetsForDeployment(ctx, md) if err != nil { return ctrl.Result{}, err } @@ -241,7 +241,7 @@ func (r *Reconciler) reconcile(ctx context.Context, cluster *clusterv1.Cluster, // to all MachineSets created by a MachineDeployment or if a user manually removed the label. for idx := range msList { machineSet := msList[idx] - if name, ok := machineSet.Labels[clusterv1.MachineDeploymentNameLabel]; ok && name == d.Name { + if name, ok := machineSet.Labels[clusterv1.MachineDeploymentNameLabel]; ok && name == md.Name { continue } @@ -249,7 +249,7 @@ func (r *Reconciler) reconcile(ctx context.Context, cluster *clusterv1.Cluster, if err != nil { return ctrl.Result{}, errors.Wrapf(err, "failed to apply %s label to MachineSet %q", clusterv1.MachineDeploymentNameLabel, machineSet.Name) } - machineSet.Labels[clusterv1.MachineDeploymentNameLabel] = d.Name + machineSet.Labels[clusterv1.MachineDeploymentNameLabel] = md.Name if err := helper.Patch(ctx, machineSet); err != nil { return ctrl.Result{}, errors.Wrapf(err, "failed to apply %s label to MachineSet %q", clusterv1.MachineDeploymentNameLabel, machineSet.Name) } @@ -268,35 +268,35 @@ func (r *Reconciler) reconcile(ctx context.Context, cluster *clusterv1.Cluster, } } - if d.Spec.Paused { - return ctrl.Result{}, r.sync(ctx, d, msList) + if md.Spec.Paused { + return ctrl.Result{}, r.sync(ctx, md, msList) } - if d.Spec.Strategy == nil { + if md.Spec.Strategy == nil { return ctrl.Result{}, errors.Errorf("missing MachineDeployment strategy") } - if d.Spec.Strategy.Type == clusterv1.RollingUpdateMachineDeploymentStrategyType { - if d.Spec.Strategy.RollingUpdate == nil { - return ctrl.Result{}, errors.Errorf("missing MachineDeployment settings for strategy type: %s", d.Spec.Strategy.Type) + if md.Spec.Strategy.Type == clusterv1.RollingUpdateMachineDeploymentStrategyType { + if md.Spec.Strategy.RollingUpdate == nil { + return ctrl.Result{}, errors.Errorf("missing MachineDeployment settings for strategy type: %s", md.Spec.Strategy.Type) } - return ctrl.Result{}, r.rolloutRolling(ctx, d, msList) + return ctrl.Result{}, r.rolloutRolling(ctx, md, msList) } - if d.Spec.Strategy.Type == clusterv1.OnDeleteMachineDeploymentStrategyType { - return ctrl.Result{}, r.rolloutOnDelete(ctx, d, msList) + if md.Spec.Strategy.Type == clusterv1.OnDeleteMachineDeploymentStrategyType { + return ctrl.Result{}, r.rolloutOnDelete(ctx, md, msList) } - return ctrl.Result{}, errors.Errorf("unexpected deployment strategy type: %s", d.Spec.Strategy.Type) + return ctrl.Result{}, errors.Errorf("unexpected deployment strategy type: %s", md.Spec.Strategy.Type) } // getMachineSetsForDeployment returns a list of MachineSets associated with a MachineDeployment. -func (r *Reconciler) getMachineSetsForDeployment(ctx context.Context, d *clusterv1.MachineDeployment) ([]*clusterv1.MachineSet, error) { +func (r *Reconciler) getMachineSetsForDeployment(ctx context.Context, md *clusterv1.MachineDeployment) ([]*clusterv1.MachineSet, error) { log := ctrl.LoggerFrom(ctx) // List all MachineSets to find those we own but that no longer match our selector. machineSets := &clusterv1.MachineSetList{} - if err := r.Client.List(ctx, machineSets, client.InNamespace(d.Namespace)); err != nil { + if err := r.Client.List(ctx, machineSets, client.InNamespace(md.Namespace)); err != nil { return nil, err } @@ -304,7 +304,7 @@ func (r *Reconciler) getMachineSetsForDeployment(ctx context.Context, d *cluster for idx := range machineSets.Items { ms := &machineSets.Items[idx] log.WithValues("MachineSet", klog.KObj(ms)) - selector, err := metav1.LabelSelectorAsSelector(&d.Spec.Selector) + selector, err := metav1.LabelSelectorAsSelector(&md.Spec.Selector) if err != nil { log.Error(err, "Skipping MachineSet, failed to get label selector from spec selector") continue @@ -317,23 +317,23 @@ func (r *Reconciler) getMachineSetsForDeployment(ctx context.Context, d *cluster } // Skip this MachineSet unless either selector matches or it has a controller ref pointing to this MachineDeployment - if !selector.Matches(labels.Set(ms.Labels)) && !metav1.IsControlledBy(ms, d) { + if !selector.Matches(labels.Set(ms.Labels)) && !metav1.IsControlledBy(ms, md) { log.V(4).Info("Skipping MachineSet, label mismatch") continue } // Attempt to adopt MachineSet if it meets previous conditions and it has no controller references. if metav1.GetControllerOf(ms) == nil { - if err := r.adoptOrphan(ctx, d, ms); err != nil { + if err := r.adoptOrphan(ctx, md, ms); err != nil { log.Error(err, "Failed to adopt MachineSet into MachineDeployment") - r.recorder.Eventf(d, corev1.EventTypeWarning, "FailedAdopt", "Failed to adopt MachineSet %q: %v", ms.Name, err) + r.recorder.Eventf(md, corev1.EventTypeWarning, "FailedAdopt", "Failed to adopt MachineSet %q: %v", ms.Name, err) continue } log.Info("Adopted MachineSet into MachineDeployment") - r.recorder.Eventf(d, corev1.EventTypeNormal, "SuccessfulAdopt", "Adopted MachineSet %q", ms.Name) + r.recorder.Eventf(md, corev1.EventTypeNormal, "SuccessfulAdopt", "Adopted MachineSet %q", ms.Name) } - if !metav1.IsControlledBy(ms, d) { + if !metav1.IsControlledBy(ms, md) { continue } diff --git a/internal/controllers/machinedeployment/machinedeployment_rolling.go b/internal/controllers/machinedeployment/machinedeployment_rolling.go index a40dd5c2fe6c..ffc7acc19943 100644 --- a/internal/controllers/machinedeployment/machinedeployment_rolling.go +++ b/internal/controllers/machinedeployment/machinedeployment_rolling.go @@ -30,8 +30,8 @@ import ( ) // rolloutRolling implements the logic for rolling a new MachineSet. -func (r *Reconciler) rolloutRolling(ctx context.Context, d *clusterv1.MachineDeployment, msList []*clusterv1.MachineSet) error { - newMS, oldMSs, err := r.getAllMachineSetsAndSyncRevision(ctx, d, msList, true) +func (r *Reconciler) rolloutRolling(ctx context.Context, md *clusterv1.MachineDeployment, msList []*clusterv1.MachineSet) error { + newMS, oldMSs, err := r.getAllMachineSetsAndSyncRevision(ctx, md, msList, true) if err != nil { return err } @@ -46,25 +46,25 @@ func (r *Reconciler) rolloutRolling(ctx context.Context, d *clusterv1.MachineDep allMSs := append(oldMSs, newMS) // Scale up, if we can. - if err := r.reconcileNewMachineSet(ctx, allMSs, newMS, d); err != nil { + if err := r.reconcileNewMachineSet(ctx, allMSs, newMS, md); err != nil { return err } - if err := r.syncDeploymentStatus(allMSs, newMS, d); err != nil { + if err := r.syncDeploymentStatus(allMSs, newMS, md); err != nil { return err } // Scale down, if we can. - if err := r.reconcileOldMachineSets(ctx, allMSs, oldMSs, newMS, d); err != nil { + if err := r.reconcileOldMachineSets(ctx, allMSs, oldMSs, newMS, md); err != nil { return err } - if err := r.syncDeploymentStatus(allMSs, newMS, d); err != nil { + if err := r.syncDeploymentStatus(allMSs, newMS, md); err != nil { return err } - if mdutil.DeploymentComplete(d, &d.Status) { - if err := r.cleanupDeployment(ctx, oldMSs, d); err != nil { + if mdutil.DeploymentComplete(md, &md.Status) { + if err := r.cleanupDeployment(ctx, oldMSs, md); err != nil { return err } } diff --git a/internal/controllers/machinedeployment/machinedeployment_rollout_ondelete.go b/internal/controllers/machinedeployment/machinedeployment_rollout_ondelete.go index dbe9a9a3b340..107d65e506d9 100644 --- a/internal/controllers/machinedeployment/machinedeployment_rollout_ondelete.go +++ b/internal/controllers/machinedeployment/machinedeployment_rollout_ondelete.go @@ -32,8 +32,8 @@ import ( ) // rolloutOnDelete implements the logic for the OnDelete MachineDeploymentStrategyType. -func (r *Reconciler) rolloutOnDelete(ctx context.Context, d *clusterv1.MachineDeployment, msList []*clusterv1.MachineSet) error { - newMS, oldMSs, err := r.getAllMachineSetsAndSyncRevision(ctx, d, msList, true) +func (r *Reconciler) rolloutOnDelete(ctx context.Context, md *clusterv1.MachineDeployment, msList []*clusterv1.MachineSet) error { + newMS, oldMSs, err := r.getAllMachineSetsAndSyncRevision(ctx, md, msList, true) if err != nil { return err } @@ -48,25 +48,25 @@ func (r *Reconciler) rolloutOnDelete(ctx context.Context, d *clusterv1.MachineDe allMSs := append(oldMSs, newMS) // Scale up, if we can. - if err := r.reconcileNewMachineSetOnDelete(ctx, allMSs, newMS, d); err != nil { + if err := r.reconcileNewMachineSetOnDelete(ctx, allMSs, newMS, md); err != nil { return err } - if err := r.syncDeploymentStatus(allMSs, newMS, d); err != nil { + if err := r.syncDeploymentStatus(allMSs, newMS, md); err != nil { return err } // Scale down, if we can. - if err := r.reconcileOldMachineSetsOnDelete(ctx, oldMSs, allMSs, d); err != nil { + if err := r.reconcileOldMachineSetsOnDelete(ctx, oldMSs, allMSs, md); err != nil { return err } - if err := r.syncDeploymentStatus(allMSs, newMS, d); err != nil { + if err := r.syncDeploymentStatus(allMSs, newMS, md); err != nil { return err } - if mdutil.DeploymentComplete(d, &d.Status) { - if err := r.cleanupDeployment(ctx, oldMSs, d); err != nil { + if mdutil.DeploymentComplete(md, &md.Status) { + if err := r.cleanupDeployment(ctx, oldMSs, md); err != nil { return err } } diff --git a/internal/controllers/machinedeployment/machinedeployment_sync.go b/internal/controllers/machinedeployment/machinedeployment_sync.go index 8c05d997b790..9bc3640bca4f 100644 --- a/internal/controllers/machinedeployment/machinedeployment_sync.go +++ b/internal/controllers/machinedeployment/machinedeployment_sync.go @@ -49,13 +49,13 @@ import ( // sync is responsible for reconciling deployments on scaling events or when they // are paused. -func (r *Reconciler) sync(ctx context.Context, d *clusterv1.MachineDeployment, msList []*clusterv1.MachineSet) error { - newMS, oldMSs, err := r.getAllMachineSetsAndSyncRevision(ctx, d, msList, false) +func (r *Reconciler) sync(ctx context.Context, md *clusterv1.MachineDeployment, msList []*clusterv1.MachineSet) error { + newMS, oldMSs, err := r.getAllMachineSetsAndSyncRevision(ctx, md, msList, false) if err != nil { return err } - if err := r.scale(ctx, d, newMS, oldMSs); err != nil { + if err := r.scale(ctx, md, newMS, oldMSs); err != nil { // If we get an error while trying to scale, the deployment will be requeued // so we can abort this resync return err @@ -65,7 +65,7 @@ func (r *Reconciler) sync(ctx context.Context, d *clusterv1.MachineDeployment, m // // TODO: Clean up the deployment when it's paused and no rollback is in flight. // allMSs := append(oldMSs, newMS) - return r.syncDeploymentStatus(allMSs, newMS, d) + return r.syncDeploymentStatus(allMSs, newMS, md) } // getAllMachineSetsAndSyncRevision returns all the machine sets for the provided deployment (new and all old), with new MS's and deployment's revision updated. @@ -80,11 +80,11 @@ func (r *Reconciler) sync(ctx context.Context, d *clusterv1.MachineDeployment, m // // Note that currently the deployment controller is using caches to avoid querying the server for reads. // This may lead to stale reads of machine sets, thus incorrect deployment status. -func (r *Reconciler) getAllMachineSetsAndSyncRevision(ctx context.Context, d *clusterv1.MachineDeployment, msList []*clusterv1.MachineSet, createIfNotExisted bool) (*clusterv1.MachineSet, []*clusterv1.MachineSet, error) { - _, allOldMSs := mdutil.FindOldMachineSets(d, msList) +func (r *Reconciler) getAllMachineSetsAndSyncRevision(ctx context.Context, md *clusterv1.MachineDeployment, msList []*clusterv1.MachineSet, createIfNotExisted bool) (*clusterv1.MachineSet, []*clusterv1.MachineSet, error) { + _, allOldMSs := mdutil.FindOldMachineSets(md, msList) // Get new machine set with the updated revision number - newMS, err := r.getNewMachineSet(ctx, d, msList, allOldMSs, createIfNotExisted) + newMS, err := r.getNewMachineSet(ctx, md, msList, allOldMSs, createIfNotExisted) if err != nil { return nil, nil, err } @@ -95,7 +95,7 @@ func (r *Reconciler) getAllMachineSetsAndSyncRevision(ctx context.Context, d *cl // Returns a MachineSet that matches the intent of the given MachineDeployment. // If there does not exist such a MachineSet and createIfNotExisted is true, create a new MachineSet. // If there is already such a MachineSet, update it to propagate in-place mutable fields from the MachineDeployment. -func (r *Reconciler) getNewMachineSet(ctx context.Context, d *clusterv1.MachineDeployment, msList, oldMSs []*clusterv1.MachineSet, createIfNotExists bool) (*clusterv1.MachineSet, error) { +func (r *Reconciler) getNewMachineSet(ctx context.Context, md *clusterv1.MachineDeployment, msList, oldMSs []*clusterv1.MachineSet, createIfNotExists bool) (*clusterv1.MachineSet, error) { // Try to find a MachineSet which matches the MachineDeployments intent, while ignore diffs between // the in-place mutable fields. // If we find a matching MachineSet we just update it to propagate any changes to the in-place mutable @@ -103,19 +103,19 @@ func (r *Reconciler) getNewMachineSet(ctx context.Context, d *clusterv1.MachineD // If we don't find a matching MachineSet, we need a rollout and thus create a new MachineSet. // Note: The in-place mutable fields can be just updated inline, because they do not affect the actual machines // themselves (i.e. the infrastructure and the software running on the Machines not the Machine object). - matchingMS := mdutil.FindNewMachineSet(d, msList) + matchingMS := mdutil.FindNewMachineSet(md, msList) // If there is a MachineSet that matches the intent of the MachineDeployment, update the MachineSet // to propagate all in-place mutable fields from MachineDeployment to the MachineSet. if matchingMS != nil { - updatedMS, err := r.updateMachineSet(ctx, d, matchingMS, oldMSs) + updatedMS, err := r.updateMachineSet(ctx, md, matchingMS, oldMSs) if err != nil { return nil, err } // Ensure MachineDeployment has the latest MachineSet revision in its revision annotation. - err = r.updateMachineDeployment(ctx, d, func(innerDeployment *clusterv1.MachineDeployment) { - mdutil.SetDeploymentRevision(d, updatedMS.Annotations[clusterv1.RevisionAnnotation]) + err = r.updateMachineDeployment(ctx, md, func(innerDeployment *clusterv1.MachineDeployment) { + mdutil.SetDeploymentRevision(md, updatedMS.Annotations[clusterv1.RevisionAnnotation]) }) if err != nil { return nil, errors.Wrap(err, "failed to update revision annotation on MachineDeployment") @@ -128,14 +128,14 @@ func (r *Reconciler) getNewMachineSet(ctx context.Context, d *clusterv1.MachineD } // Create a new MachineSet and wait until the new MachineSet exists in the cache. - newMS, err := r.createMachineSetAndWait(ctx, d, oldMSs) + newMS, err := r.createMachineSetAndWait(ctx, md, oldMSs) if err != nil { return nil, err } // Ensure MachineDeployment has the latest MachineSet revision in its revision annotation. - err = r.updateMachineDeployment(ctx, d, func(innerDeployment *clusterv1.MachineDeployment) { - mdutil.SetDeploymentRevision(d, newMS.Annotations[clusterv1.RevisionAnnotation]) + err = r.updateMachineDeployment(ctx, md, func(innerDeployment *clusterv1.MachineDeployment) { + mdutil.SetDeploymentRevision(md, newMS.Annotations[clusterv1.RevisionAnnotation]) }) if err != nil { return nil, errors.Wrap(err, "failed to update revision annotation on MachineDeployment") @@ -483,17 +483,17 @@ func (r *Reconciler) scale(ctx context.Context, deployment *clusterv1.MachineDep } // syncDeploymentStatus checks if the status is up-to-date and sync it if necessary. -func (r *Reconciler) syncDeploymentStatus(allMSs []*clusterv1.MachineSet, newMS *clusterv1.MachineSet, d *clusterv1.MachineDeployment) error { - d.Status = calculateStatus(allMSs, newMS, d) +func (r *Reconciler) syncDeploymentStatus(allMSs []*clusterv1.MachineSet, newMS *clusterv1.MachineSet, md *clusterv1.MachineDeployment) error { + md.Status = calculateStatus(allMSs, newMS, md) - // minReplicasNeeded will be equal to d.Spec.Replicas when the strategy is not RollingUpdateMachineDeploymentStrategyType. - minReplicasNeeded := *(d.Spec.Replicas) - mdutil.MaxUnavailable(*d) + // minReplicasNeeded will be equal to md.Spec.Replicas when the strategy is not RollingUpdateMachineDeploymentStrategyType. + minReplicasNeeded := *(md.Spec.Replicas) - mdutil.MaxUnavailable(*md) - if d.Status.AvailableReplicas >= minReplicasNeeded { + if md.Status.AvailableReplicas >= minReplicasNeeded { // NOTE: The structure of calculateStatus() does not allow us to update the machinedeployment directly, we can only update the status obj it returns. Ideally, we should change calculateStatus() --> updateStatus() to be consistent with the rest of the code base, until then, we update conditions here. - conditions.MarkTrue(d, clusterv1.MachineDeploymentAvailableCondition) + conditions.MarkTrue(md, clusterv1.MachineDeploymentAvailableCondition) } else { - conditions.MarkFalse(d, clusterv1.MachineDeploymentAvailableCondition, clusterv1.WaitingForAvailableMachinesReason, clusterv1.ConditionSeverityWarning, "Minimum availability requires %d replicas, current %d available", minReplicasNeeded, d.Status.AvailableReplicas) + conditions.MarkFalse(md, clusterv1.MachineDeploymentAvailableCondition, clusterv1.WaitingForAvailableMachinesReason, clusterv1.ConditionSeverityWarning, "Minimum availability requires %d replicas, current %d available", minReplicasNeeded, md.Status.AvailableReplicas) } return nil } @@ -641,15 +641,15 @@ func (r *Reconciler) cleanupDeployment(ctx context.Context, oldMSs []*clusterv1. return nil } -func (r *Reconciler) updateMachineDeployment(ctx context.Context, d *clusterv1.MachineDeployment, modify func(*clusterv1.MachineDeployment)) error { - return updateMachineDeployment(ctx, r.Client, d, modify) +func (r *Reconciler) updateMachineDeployment(ctx context.Context, md *clusterv1.MachineDeployment, modify func(*clusterv1.MachineDeployment)) error { + return updateMachineDeployment(ctx, r.Client, md, modify) } // We have this as standalone variant to be able to use it from the tests. -func updateMachineDeployment(ctx context.Context, c client.Client, d *clusterv1.MachineDeployment, modify func(*clusterv1.MachineDeployment)) error { - mdObjectKey := util.ObjectKey(d) +func updateMachineDeployment(ctx context.Context, c client.Client, md *clusterv1.MachineDeployment, modify func(*clusterv1.MachineDeployment)) error { + mdObjectKey := util.ObjectKey(md) return retry.RetryOnConflict(retry.DefaultBackoff, func() error { - // Note: We intentionally don't re-use the passed in MachineDeployment d here as that would + // Note: We intentionally don't re-use the passed in MachineDeployment md here as that would // overwrite any local changes we might have previously made to the MachineDeployment with the version // we get here from the apiserver. md := &clusterv1.MachineDeployment{} diff --git a/internal/controllers/machinedeployment/mdutil/util.go b/internal/controllers/machinedeployment/mdutil/util.go index c4957454153c..b8e672cd908d 100644 --- a/internal/controllers/machinedeployment/mdutil/util.go +++ b/internal/controllers/machinedeployment/mdutil/util.go @@ -322,12 +322,12 @@ func MaxSurge(deployment clusterv1.MachineDeployment) int32 { // GetProportion will estimate the proportion for the provided machine set using 1. the current size // of the parent deployment, 2. the replica count that needs be added on the machine sets of the // deployment, and 3. the total replicas added in the machine sets of the deployment so far. -func GetProportion(ms *clusterv1.MachineSet, d clusterv1.MachineDeployment, deploymentReplicasToAdd, deploymentReplicasAdded int32, logger logr.Logger) int32 { +func GetProportion(ms *clusterv1.MachineSet, md clusterv1.MachineDeployment, deploymentReplicasToAdd, deploymentReplicasAdded int32, logger logr.Logger) int32 { if ms == nil || *(ms.Spec.Replicas) == 0 || deploymentReplicasToAdd == 0 || deploymentReplicasToAdd == deploymentReplicasAdded { return int32(0) } - msFraction := getMachineSetFraction(*ms, d, logger) + msFraction := getMachineSetFraction(*ms, md, logger) allowed := deploymentReplicasToAdd - deploymentReplicasAdded if deploymentReplicasToAdd > 0 { @@ -344,20 +344,20 @@ func GetProportion(ms *clusterv1.MachineSet, d clusterv1.MachineDeployment, depl // getMachineSetFraction estimates the fraction of replicas a machine set can have in // 1. a scaling event during a rollout or 2. when scaling a paused deployment. -func getMachineSetFraction(ms clusterv1.MachineSet, d clusterv1.MachineDeployment, logger logr.Logger) int32 { +func getMachineSetFraction(ms clusterv1.MachineSet, md clusterv1.MachineDeployment, logger logr.Logger) int32 { // If we are scaling down to zero then the fraction of this machine set is its whole size (negative) - if *(d.Spec.Replicas) == int32(0) { + if *(md.Spec.Replicas) == int32(0) { return -*(ms.Spec.Replicas) } - deploymentReplicas := *(d.Spec.Replicas) + MaxSurge(d) + deploymentReplicas := *(md.Spec.Replicas) + MaxSurge(md) annotatedReplicas, ok := getMaxReplicasAnnotation(&ms, logger) if !ok { // If we cannot find the annotation then fallback to the current deployment size. Note that this // will not be an accurate proportion estimation in case other machine sets have different values // which means that the deployment was scaled at some point but we at least will stay in limits // due to the min-max comparisons in getProportion. - annotatedReplicas = d.Status.Replicas + annotatedReplicas = md.Status.Replicas } // We should never proportionally scale up from zero which means ms.spec.replicas and annotatedReplicas diff --git a/internal/controllers/machinedeployment/mdutil/util_test.go b/internal/controllers/machinedeployment/mdutil/util_test.go index 049f82d2751b..5329b1a65c0c 100644 --- a/internal/controllers/machinedeployment/mdutil/util_test.go +++ b/internal/controllers/machinedeployment/mdutil/util_test.go @@ -36,26 +36,26 @@ import ( clusterv1 "sigs.k8s.io/cluster-api/api/v1beta1" ) -func newDControllerRef(d *clusterv1.MachineDeployment) *metav1.OwnerReference { +func newDControllerRef(md *clusterv1.MachineDeployment) *metav1.OwnerReference { isController := true return &metav1.OwnerReference{ APIVersion: "clusters/v1alpha", Kind: "MachineDeployment", - Name: d.GetName(), - UID: d.GetUID(), + Name: md.GetName(), + UID: md.GetUID(), Controller: &isController, } } // generateMS creates a machine set, with the input deployment's template as its template. -func generateMS(deployment clusterv1.MachineDeployment) clusterv1.MachineSet { - template := deployment.Spec.Template.DeepCopy() +func generateMS(md clusterv1.MachineDeployment) clusterv1.MachineSet { + template := md.Spec.Template.DeepCopy() return clusterv1.MachineSet{ ObjectMeta: metav1.ObjectMeta{ UID: randomUID(), Name: names.SimpleNameGenerator.GenerateName("machineset"), Labels: template.Labels, - OwnerReferences: []metav1.OwnerReference{*newDControllerRef(&deployment)}, + OwnerReferences: []metav1.OwnerReference{*newDControllerRef(&md)}, }, Spec: clusterv1.MachineSetSpec{ Replicas: new(int32), @@ -617,32 +617,32 @@ func TestDeploymentComplete(t *testing.T) { tests := []struct { name string - d *clusterv1.MachineDeployment + md *clusterv1.MachineDeployment expected bool }{ { name: "not complete: min but not all machines become available", - d: deployment(5, 5, 5, 4, 1, 0), + md: deployment(5, 5, 5, 4, 1, 0), expected: false, }, { name: "not complete: min availability is not honored", - d: deployment(5, 5, 5, 3, 1, 0), + md: deployment(5, 5, 5, 3, 1, 0), expected: false, }, { name: "complete", - d: deployment(5, 5, 5, 5, 0, 0), + md: deployment(5, 5, 5, 5, 0, 0), expected: true, }, { name: "not complete: all machines are available but not updated", - d: deployment(5, 5, 4, 5, 0, 0), + md: deployment(5, 5, 4, 5, 0, 0), expected: false, }, { @@ -650,13 +650,13 @@ func TestDeploymentComplete(t *testing.T) { // old machine set: spec.replicas=1, status.replicas=1, status.availableReplicas=1 // new machine set: spec.replicas=1, status.replicas=1, status.availableReplicas=0 - d: deployment(1, 2, 1, 1, 0, 1), + md: deployment(1, 2, 1, 1, 0, 1), expected: false, }, { name: "not complete: one replica deployment never comes up", - d: deployment(1, 1, 1, 0, 1, 1), + md: deployment(1, 1, 1, 0, 1, 1), expected: false, }, } @@ -665,7 +665,7 @@ func TestDeploymentComplete(t *testing.T) { t.Run(test.name, func(t *testing.T) { g := NewWithT(t) - g.Expect(DeploymentComplete(test.d, &test.d.Status)).To(Equal(test.expected)) + g.Expect(DeploymentComplete(test.md, &test.md.Status)).To(Equal(test.expected)) }) } }