Skip to content

Commit

Permalink
fix review findings
Browse files Browse the repository at this point in the history
  • Loading branch information
@sbueringer
sbueringer committed Sep 8, 2021
1 parent e0ab8ec commit 9601c4e
Show file tree
Hide file tree
Showing 5 changed files with 174 additions and 87 deletions.
4 changes: 2 additions & 2 deletions controllers/topology/internal/templateutil/template.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -48,13 +48,13 @@ func CalculateTemplatesInUse(md *clusterv1.MachineDeployment, msList []*clusterv
}
}

// MachineDeployment has already been deleted or still exists and is in deleting. In both cases there
// If MachineDeployment has already been deleted or still exists and is in deleting, then there
// are no templates referenced in the MachineDeployment which are still in use, so let's return here.
if md == nil || !md.DeletionTimestamp.IsZero() {
return templatesInUse, nil
}

// Templates of the MachineDeployment are still in use if the MachineDeployment is not in deleting.
// Otherwise, the templates of the MachineDeployment are still in use.
bootstrapRef := md.Spec.Template.Spec.Bootstrap.ConfigRef
infrastructureRef := &md.Spec.Template.Spec.InfrastructureRef
if err := addTemplateRef(templatesInUse, bootstrapRef, infrastructureRef); err != nil {
Expand Down
32 changes: 10 additions & 22 deletions controllers/topology/machinedeployment_controller.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -111,6 +111,8 @@ func (r *MachineDeploymentReconciler) Reconcile(ctx context.Context, req ctrl.Re
return ctrl.Result{}, nil
}

// reconcileDelete deletes templates referenced in a MachineDeployment, if the templates are not used by other
// MachineDeployments or MachineSets.
func (r MachineDeploymentReconciler) reconcileDelete(ctx context.Context, md *clusterv1.MachineDeployment) (ctrl.Result, error) {
// Get the corresponding MachineSets.
msList, err := getMachineSetsForDeployment(ctx, r.Client, types.NamespacedName{Namespace: md.Namespace, Name: md.Name})
Expand All @@ -124,9 +126,14 @@ func (r MachineDeploymentReconciler) reconcileDelete(ctx context.Context, md *cl
return ctrl.Result{}, err
}

// Delete unused templates of the MachineDeployment.
if err := r.deleteUnusedMachineDeploymentTemplates(ctx, templatesInUse, md); err != nil {
return ctrl.Result{}, err
// Delete unused templates.
ref := md.Spec.Template.Spec.Bootstrap.ConfigRef
if err := templateutil.DeleteTemplateIfUnused(ctx, r.Client, templatesInUse, ref); err != nil {
return ctrl.Result{}, errors.Wrapf(err, "failed to delete bootstrap template for %s", KRef{Obj: md})
}
ref = &md.Spec.Template.Spec.InfrastructureRef
if err := templateutil.DeleteTemplateIfUnused(ctx, r.Client, templatesInUse, ref); err != nil {
return ctrl.Result{}, errors.Wrapf(err, "failed to delete infrastructure template for %s", KRef{Obj: md})
}

// Remove the finalizer so the MachineDeployment can be garbage collected by Kubernetes.
Expand All @@ -141,22 +148,3 @@ func (r MachineDeploymentReconciler) reconcileDelete(ctx context.Context, md *cl

return ctrl.Result{}, nil
}

// deleteUnusedMachineDeploymentTemplates deletes templates referenced in MachineDeployments, if
// the templates are not used by other MachineDeployments or MachineSets (i.e. they are not in templatesInUse).
// Note: We don't care about MachineSets, because the MachineSet deletion is triggered based
// on owner references by Kubernetes *after* we remove the finalizer from the
// MachineDeployment and the MachineDeployment has been garbage collected by Kubernetes.
func (r *MachineDeploymentReconciler) deleteUnusedMachineDeploymentTemplates(ctx context.Context, templatesInUse map[string]bool, md *clusterv1.MachineDeployment) error {
ref := md.Spec.Template.Spec.Bootstrap.ConfigRef
if err := templateutil.DeleteTemplateIfUnused(ctx, r.Client, templatesInUse, ref); err != nil {
return errors.Wrapf(err, "failed to delete bootstrap template for %s", KRef{Obj: md})
}

ref = &md.Spec.Template.Spec.InfrastructureRef
if err := templateutil.DeleteTemplateIfUnused(ctx, r.Client, templatesInUse, ref); err != nil {
return errors.Wrapf(err, "failed to delete infrastructure template for %s", KRef{Obj: md})
}

return nil
}
62 changes: 42 additions & 20 deletions controllers/topology/machinedeployment_controller_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -24,24 +24,23 @@ import (
apierrors "k8s.io/apimachinery/pkg/api/errors"
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
"k8s.io/apimachinery/pkg/apis/meta/v1/unstructured"
"sigs.k8s.io/cluster-api/controllers/topology/internal/templateutil"
clusterv1 "sigs.k8s.io/cluster-api/api/v1alpha4"
"sigs.k8s.io/cluster-api/internal/testtypes"
"sigs.k8s.io/controller-runtime/pkg/client"
"sigs.k8s.io/controller-runtime/pkg/client/fake"
"sigs.k8s.io/controller-runtime/pkg/controller/controllerutil"
)

func TestDeleteUnusedMachineDeploymentTemplates(t *testing.T) {
func TestMachineDeploymentReconciler_ReconcileDelete(t *testing.T) {
deletionTimeStamp := metav1.Now()

mdBT := testtypes.NewBootstrapTemplateBuilder(metav1.NamespaceDefault, "mdBT").Build()
mdIMT := testtypes.NewInfrastructureMachineTemplateBuilder(metav1.NamespaceDefault, "mdIMT").Build()
md := testtypes.NewMachineDeploymentBuilder(metav1.NamespaceDefault, "md").
WithBootstrapTemplate(mdBT).
WithInfrastructureTemplate(mdIMT).
Build()

mdWithoutBootstrapTemplateIMT := testtypes.NewInfrastructureMachineTemplateBuilder(metav1.NamespaceDefault, "mdWithoutBootstrapTemplateIMT").Build()
mdWithoutBootstrapTemplate := testtypes.NewMachineDeploymentBuilder(metav1.NamespaceDefault, "mdWithoutBootstrapTemplate").
WithInfrastructureTemplate(mdWithoutBootstrapTemplateIMT).
Build()
md.SetDeletionTimestamp(&deletionTimeStamp)

t.Run("Should delete templates of a MachineDeployment", func(t *testing.T) {
g := NewWithT(t)
Expand All @@ -54,49 +53,72 @@ func TestDeleteUnusedMachineDeploymentTemplates(t *testing.T) {
r := &MachineDeploymentReconciler{
Client: fakeClient,
}
err := r.deleteUnusedMachineDeploymentTemplates(ctx, map[string]bool{}, md)
_, err := r.reconcileDelete(ctx, md)
g.Expect(err).ToNot(HaveOccurred())

afterMD := &clusterv1.MachineDeployment{}
g.Expect(fakeClient.Get(ctx, client.ObjectKeyFromObject(md), afterMD)).To(Succeed())

g.Expect(controllerutil.ContainsFinalizer(afterMD, clusterv1.MachineDeploymentTopologyFinalizer)).To(BeFalse())
g.Expect(templateExists(fakeClient, mdBT)).To(BeFalse())
g.Expect(templateExists(fakeClient, mdIMT)).To(BeFalse())
})

t.Run("Should not delete templates of a MachineDeployment when they are still in use", func(t *testing.T) {
t.Run("Should delete infra template of a MachineDeployment without a bootstrap template", func(t *testing.T) {
g := NewWithT(t)

mdWithoutBootstrapTemplateIMT := testtypes.NewInfrastructureMachineTemplateBuilder(metav1.NamespaceDefault, "mdWithoutBootstrapTemplateIMT").Build()
mdWithoutBootstrapTemplate := testtypes.NewMachineDeploymentBuilder(metav1.NamespaceDefault, "mdWithoutBootstrapTemplate").
WithInfrastructureTemplate(mdWithoutBootstrapTemplateIMT).
Build()
mdWithoutBootstrapTemplate.SetDeletionTimestamp(&deletionTimeStamp)

fakeClient := fake.NewClientBuilder().
WithScheme(fakeScheme).
WithObjects(md, mdBT, mdIMT).
WithObjects(mdWithoutBootstrapTemplate, mdWithoutBootstrapTemplateIMT).
Build()

templatesInUse, err := templateutil.CalculateTemplatesInUse(md, nil)
g.Expect(err).ToNot(HaveOccurred())

r := &MachineDeploymentReconciler{
Client: fakeClient,
}
err = r.deleteUnusedMachineDeploymentTemplates(ctx, templatesInUse, md)
_, err := r.reconcileDelete(ctx, mdWithoutBootstrapTemplate)
g.Expect(err).ToNot(HaveOccurred())

g.Expect(templateExists(fakeClient, mdBT)).To(BeTrue())
g.Expect(templateExists(fakeClient, mdIMT)).To(BeTrue())
afterMD := &clusterv1.MachineDeployment{}
g.Expect(fakeClient.Get(ctx, client.ObjectKeyFromObject(mdWithoutBootstrapTemplate), afterMD)).To(Succeed())

g.Expect(controllerutil.ContainsFinalizer(afterMD, clusterv1.MachineDeploymentTopologyFinalizer)).To(BeFalse())
g.Expect(templateExists(fakeClient, mdWithoutBootstrapTemplateIMT)).To(BeFalse())
})

t.Run("Should delete infra template of a MachineDeployment without a bootstrap template", func(t *testing.T) {
t.Run("Should not delete templates of a MachineDeployment when they are still in use in a MachineSet", func(t *testing.T) {
g := NewWithT(t)

ms := testtypes.NewMachineSetBuilder(md.Namespace, "ms").
WithBootstrapTemplate(mdBT).
WithInfrastructureTemplate(mdIMT).
WithLabels(map[string]string{
clusterv1.MachineDeploymentLabelName: md.Name,
}).
Build()

fakeClient := fake.NewClientBuilder().
WithScheme(fakeScheme).
WithObjects(mdWithoutBootstrapTemplate, mdWithoutBootstrapTemplateIMT).
WithObjects(md, ms, mdBT, mdIMT).
Build()

r := &MachineDeploymentReconciler{
Client: fakeClient,
}
err := r.deleteUnusedMachineDeploymentTemplates(ctx, map[string]bool{}, mdWithoutBootstrapTemplate)
_, err := r.reconcileDelete(ctx, md)
g.Expect(err).ToNot(HaveOccurred())

g.Expect(templateExists(fakeClient, mdWithoutBootstrapTemplateIMT)).To(BeFalse())
afterMD := &clusterv1.MachineDeployment{}
g.Expect(fakeClient.Get(ctx, client.ObjectKeyFromObject(md), afterMD)).To(Succeed())

g.Expect(controllerutil.ContainsFinalizer(afterMD, clusterv1.MachineDeploymentTopologyFinalizer)).To(BeFalse())
g.Expect(templateExists(fakeClient, mdBT)).To(BeTrue())
g.Expect(templateExists(fakeClient, mdIMT)).To(BeTrue())
})
}

Expand Down
45 changes: 18 additions & 27 deletions controllers/topology/machineset_controller.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -122,14 +122,16 @@ func (r *MachineSetReconciler) Reconcile(ctx context.Context, req ctrl.Request)
return ctrl.Result{}, nil
}

// reconcileDelete deletes templates referenced in a MachineSet, if the templates are not used by other
// MachineDeployments or MachineSets.
func (r MachineSetReconciler) reconcileDelete(ctx context.Context, ms *clusterv1.MachineSet) (ctrl.Result, error) {
// Calculate the name of the corresponding MachineDeployment.
mdName, err := calculateMachineDeploymentName(ms)
// Gets the name of the MachineDeployment that controls this MachineSet.
mdName, err := getMachineDeploymentName(ms)
if err != nil {
return ctrl.Result{}, err
}

// Get the corresponding MachineSets.
// Get all the MachineSets for the MachineDeployment.
msList, err := getMachineSetsForDeployment(ctx, r.Client, *mdName)
if err != nil {
return ctrl.Result{}, err
Expand All @@ -153,9 +155,14 @@ func (r MachineSetReconciler) reconcileDelete(ctx context.Context, ms *clusterv1
return ctrl.Result{}, err
}

// Delete unused templates of the MachineSet.
if err := r.deleteUnusedMachineSetTemplates(ctx, templatesInUse, ms); err != nil {
return ctrl.Result{}, err
// Delete unused templates.
ref := ms.Spec.Template.Spec.Bootstrap.ConfigRef
if err := templateutil.DeleteTemplateIfUnused(ctx, r.Client, templatesInUse, ref); err != nil {
return ctrl.Result{}, errors.Wrapf(err, "failed to delete bootstrap template for %s", KRef{Obj: ms})
}
ref = &ms.Spec.Template.Spec.InfrastructureRef
if err := templateutil.DeleteTemplateIfUnused(ctx, r.Client, templatesInUse, ref); err != nil {
return ctrl.Result{}, errors.Wrapf(err, "failed to delete infrastructure template for %s", KRef{Obj: ms})
}

// Remove the finalizer so the MachineSet can be garbage collected by Kubernetes.
Expand All @@ -171,8 +178,8 @@ func (r MachineSetReconciler) reconcileDelete(ctx context.Context, ms *clusterv1
return ctrl.Result{}, nil
}

// calculateMachineDeploymentName calculates the MachineDeployment name based on owner references.
func calculateMachineDeploymentName(ms *clusterv1.MachineSet) (*types.NamespacedName, error) {
// getMachineDeploymentName calculates the MachineDeployment name based on owner references.
func getMachineDeploymentName(ms *clusterv1.MachineSet) (*types.NamespacedName, error) {
for _, ref := range ms.OwnerReferences {
if ref.Kind != "MachineDeployment" {
continue
Expand All @@ -188,24 +195,8 @@ func calculateMachineDeploymentName(ms *clusterv1.MachineSet) (*types.Namespaced
}
}

// Note: Once we set an owner reference to a MachineDeployment in a MachineSet it stays there
// and is not deleted when the MachineDeployment is deleted. So we assume there's something wrong,
// if we couldn't find a MachineDeployment owner reference.
return nil, errors.Errorf("could not calculate MachineDeployment name for %s", KRef{Obj: ms})
}

// deleteUnusedMachineSetTemplates deletes templates referenced in MachineSets, if:
// the templates are not used by other MachineDeployments or MachineSets (i.e. they are not in templatesInUse).
// Note: We don't care about Machines, because the Machine deletion is triggered based
// on owner references by Kubernetes *after* we remove the finalizer from the
// MachineSet and the MachineSet has been garbage collected by Kubernetes.
func (r *MachineSetReconciler) deleteUnusedMachineSetTemplates(ctx context.Context, templatesInUse map[string]bool, ms *clusterv1.MachineSet) error {
ref := ms.Spec.Template.Spec.Bootstrap.ConfigRef
if err := templateutil.DeleteTemplateIfUnused(ctx, r.Client, templatesInUse, ref); err != nil {
return errors.Wrapf(err, "failed to delete bootstrap template for %s", KRef{Obj: ms})
}

ref = &ms.Spec.Template.Spec.InfrastructureRef
if err := templateutil.DeleteTemplateIfUnused(ctx, r.Client, templatesInUse, ref); err != nil {
return errors.Wrapf(err, "failed to delete infrastructure template for %s", KRef{Obj: ms})
}

return nil
}
Loading

0 comments on commit 9601c4e

Please sign in to comment.