Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

🐛 [WIP] Update MachineSet and MachineDeployment ownerRef versions on reconcile #7509

Closed
wants to merge 1 commit into from
Closed

🐛 [WIP] Update MachineSet and MachineDeployment ownerRef versions on reconcile #7509

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
91 changes: 51 additions & 40 deletions internal/controllers/machinedeployment/machinedeployment_controller.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -165,9 +165,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 {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: I would highly recommend to split out a general m=>md rename into a separate PR next time.

Just helps to de-obfuscate the actual changes in this PR and keeps everything nicely separated

(just resolve for this PR)

// Always update the readyCondition by summarizing the state of other conditions.
conditions.SetSummary(d,
conditions.SetSummary(md,
conditions.WithConditions(
clusterv1.MachineDeploymentAvailableCondition,
),
Expand All @@ -180,28 +180,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.ClusterLabelName] = d.Spec.ClusterName
md.Labels[clusterv1.ClusterLabelName] = md.Spec.ClusterName

if r.shouldAdopt(d) {
d.OwnerReferences = util.EnsureOwnerRef(d.OwnerReferences, metav1.OwnerReference{
// Check to ensure the owner reference on the Cluster exists and that it is of the current version.
if r.shouldUpdateOwnerReference(md) {
md.OwnerReferences = util.EnsureOwnerRef(md.OwnerReferences, metav1.OwnerReference{
APIVersion: clusterv1.GroupVersion.String(),
Kind: "Cluster",
Name: cluster.Name,
Expand All @@ -211,58 +212,58 @@ func (r *Reconciler) reconcile(ctx context.Context, cluster *clusterv1.Cluster,
}

// Make sure to reconcile the external infrastructure reference.
if err := reconcileExternalTemplateReference(ctx, r.Client, r.APIReader, cluster, &d.Spec.Template.Spec.InfrastructureRef); err != nil {
if err := reconcileExternalTemplateReference(ctx, r.Client, r.APIReader, 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, r.APIReader, cluster, d.Spec.Template.Spec.Bootstrap.ConfigRef); err != nil {
if md.Spec.Template.Spec.Bootstrap.ConfigRef != nil {
if err := reconcileExternalTemplateReference(ctx, r.Client, r.APIReader, 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
}

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
}

filtered := make([]*clusterv1.MachineSet, 0, len(machineSets.Items))
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
Expand All @@ -275,37 +276,47 @@ 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 machine 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 ControllerReference is nil, adopt the machineSet by updating its controller ref.
if err := r.updateControllerRef(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
}

// Ensure that the version of the controllerReference is up-to-date.
if metav1.GetControllerOf(ms).APIVersion != md.APIVersion {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Are sure the md.APIVersion is set? (afaik per default it's not in CR / client-go)

I think we should use the same source of truth as updateControllerReference (which is machineDeploymentKind)

err := r.updateControllerRef(ctx, md, ms)
if err != nil {
log.Error(err, "Failed to update Machine controller reference.")
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wonder if we should return the error in this case.

In general this request should work anyway, right?

continue
}
}

filtered = append(filtered, ms)
}

return filtered, nil
}

// adoptOrphan sets the MachineDeployment as a controller OwnerReference to the MachineSet.
func (r *Reconciler) adoptOrphan(ctx context.Context, deployment *clusterv1.MachineDeployment, machineSet *clusterv1.MachineSet) error {
// updateControllerRef sets the MachineDeployment as a controller OwnerReference to the MachineSet.
func (r *Reconciler) updateControllerRef(ctx context.Context, deployment *clusterv1.MachineDeployment, machineSet *clusterv1.MachineSet) error {
patch := client.MergeFrom(machineSet.DeepCopy())
newRef := *metav1.NewControllerRef(deployment, machineDeploymentKind)
machineSet.OwnerReferences = append(machineSet.OwnerReferences, newRef)
machineSet.OwnerReferences = util.EnsureOwnerRef(machineSet.OwnerReferences, newRef)
return r.Client.Patch(ctx, machineSet, patch)
}

Expand Down Expand Up @@ -373,8 +384,8 @@ func (r *Reconciler) MachineSetToDeployments(o client.Object) []ctrl.Request {
return result
}

func (r *Reconciler) shouldAdopt(md *clusterv1.MachineDeployment) bool {
return !util.HasOwner(md.OwnerReferences, clusterv1.GroupVersion.String(), []string{"Cluster"})
func (r *Reconciler) shouldUpdateOwnerReference(md *clusterv1.MachineDeployment) bool {
return !util.HasOwnerWithSameVersion(md.OwnerReferences, clusterv1.GroupVersion.String(), []string{"Cluster"})
}

func reconcileExternalTemplateReference(ctx context.Context, c client.Client, apiReader client.Reader, cluster *clusterv1.Cluster, ref *corev1.ObjectReference) error {
Expand Down
14 changes: 7 additions & 7 deletions internal/controllers/machinedeployment/machinedeployment_controller_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -244,7 +244,7 @@ func TestMachineDeploymentReconciler(t *testing.T) {
//
secondMachineSet := machineSets.Items[0]
t.Log("Scaling the MachineDeployment to 3 replicas")
modifyFunc := func(d *clusterv1.MachineDeployment) { d.Spec.Replicas = pointer.Int32(3) }
modifyFunc := func(md *clusterv1.MachineDeployment) { md.Spec.Replicas = pointer.Int32(3) }
g.Expect(updateMachineDeployment(ctx, env, deployment, modifyFunc)).To(Succeed())
g.Eventually(func() int {
key := client.ObjectKey{Name: secondMachineSet.Name, Namespace: secondMachineSet.Namespace}
Expand All @@ -258,7 +258,7 @@ func TestMachineDeploymentReconciler(t *testing.T) {
// Update a MachineDeployment, expect Reconcile to be called and a new MachineSet to appear.
//
t.Log("Setting a label on the MachineDeployment")
modifyFunc = func(d *clusterv1.MachineDeployment) { d.Spec.Template.Labels["updated"] = "true" }
modifyFunc = func(md *clusterv1.MachineDeployment) { md.Spec.Template.Labels["updated"] = "true" }
g.Expect(updateMachineDeployment(ctx, env, deployment, modifyFunc)).To(Succeed())
g.Eventually(func() int {
if err := env.List(ctx, machineSets, msListOpts...); err != nil {
Expand All @@ -268,8 +268,8 @@ func TestMachineDeploymentReconciler(t *testing.T) {
}, timeout).Should(BeEquivalentTo(2))

t.Log("Updating deletePolicy on the MachineDeployment")
modifyFunc = func(d *clusterv1.MachineDeployment) {
d.Spec.Strategy.RollingUpdate.DeletePolicy = pointer.String("Newest")
modifyFunc = func(md *clusterv1.MachineDeployment) {
md.Spec.Strategy.RollingUpdate.DeletePolicy = pointer.String("Newest")
}
g.Expect(updateMachineDeployment(ctx, env, deployment, modifyFunc)).To(Succeed())
g.Eventually(func() string {
Expand Down Expand Up @@ -348,9 +348,9 @@ func TestMachineDeploymentReconciler(t *testing.T) {
}

t.Log("Updating MachineDeployment label")
modifyFunc = func(d *clusterv1.MachineDeployment) {
d.Spec.Selector.MatchLabels = newLabels
d.Spec.Template.Labels = newLabels
modifyFunc = func(md *clusterv1.MachineDeployment) {
md.Spec.Selector.MatchLabels = newLabels
md.Spec.Template.Labels = newLabels
}
g.Expect(updateMachineDeployment(ctx, env, deployment, modifyFunc)).To(Succeed())

Expand Down
16 changes: 8 additions & 8 deletions internal/controllers/machinedeployment/machinedeployment_rolling.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
}
Expand All @@ -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
}
}
Expand Down
16 changes: 8 additions & 8 deletions internal/controllers/machinedeployment/machinedeployment_rollout_ondelete.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
}
Expand All @@ -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
}
}
Expand Down
Loading