diff --git a/controllers/topology/cluster_controller.go b/controllers/topology/cluster_controller.go index 050768f04d6b..3c747218e93b 100644 --- a/controllers/topology/cluster_controller.go +++ b/controllers/topology/cluster_controller.go @@ -57,7 +57,7 @@ type ClusterReconciler struct { func (r *ClusterReconciler) SetupWithManager(ctx context.Context, mgr ctrl.Manager, options controller.Options) error { c, err := ctrl.NewControllerManagedBy(mgr). For(&clusterv1.Cluster{}). - Named("cluster/topology"). + Named("topology/cluster"). Watches( &source.Kind{Type: &clusterv1.ClusterClass{}}, handler.EnqueueRequestsFromMapFunc(r.clusterClassToCluster), diff --git a/controllers/topology/internal/templateutil/doc.go b/controllers/topology/internal/templateutil/doc.go deleted file mode 100644 index e26c48cb1036..000000000000 --- a/controllers/topology/internal/templateutil/doc.go +++ /dev/null @@ -1,18 +0,0 @@ -/* -Copyright 2021 The Kubernetes Authors. - -Licensed under the Apache License, Version 2.0 (the "License"); -you may not use this file except in compliance with the License. -You may obtain a copy of the License at - - http://www.apache.org/licenses/LICENSE-2.0 - -Unless required by applicable law or agreed to in writing, software -distributed under the License is distributed on an "AS IS" BASIS, -WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. -See the License for the specific language governing permissions and -limitations under the License. -*/ - -// Package templateutil provides utils to clean up templates during deletion of MachineDeployments and MachineSets. -package templateutil diff --git a/controllers/topology/internal/templateutil/template.go b/controllers/topology/internal/templateutil/template.go deleted file mode 100644 index 521fa16449b1..000000000000 --- a/controllers/topology/internal/templateutil/template.go +++ /dev/null @@ -1,124 +0,0 @@ -/* -Copyright 2021 The Kubernetes Authors. - -Licensed under the Apache License, Version 2.0 (the "License"); -you may not use this file except in compliance with the License. -You may obtain a copy of the License at - - http://www.apache.org/licenses/LICENSE-2.0 - -Unless required by applicable law or agreed to in writing, software -distributed under the License is distributed on an "AS IS" BASIS, -WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. -See the License for the specific language governing permissions and -limitations under the License. -*/ - -package templateutil - -import ( - "context" - "fmt" - - "github.com/pkg/errors" - corev1 "k8s.io/api/core/v1" - apierrors "k8s.io/apimachinery/pkg/api/errors" - "k8s.io/apimachinery/pkg/runtime/schema" - clusterv1 "sigs.k8s.io/cluster-api/api/v1alpha4" - "sigs.k8s.io/cluster-api/controllers/external" - ctrl "sigs.k8s.io/controller-runtime" - "sigs.k8s.io/controller-runtime/pkg/client" -) - -// CalculateTemplatesInUse returns all templates referenced in non-deleting MachineDeployment and MachineSets. -func CalculateTemplatesInUse(md *clusterv1.MachineDeployment, msList []*clusterv1.MachineSet) (map[string]bool, error) { - templatesInUse := map[string]bool{} - - // Templates of the MachineSet are still in use if the MachineSet is not in deleting. - for _, ms := range msList { - if !ms.DeletionTimestamp.IsZero() { - continue - } - - bootstrapRef := ms.Spec.Template.Spec.Bootstrap.ConfigRef - infrastructureRef := &ms.Spec.Template.Spec.InfrastructureRef - if err := addTemplateRef(templatesInUse, bootstrapRef, infrastructureRef); err != nil { - // TODO(sbueringer) use KRef after 5153 has been merged - return nil, errors.Wrapf(err, "failed to add templates of %s/%s to templatesInUse", ms.Kind, ms.Name) - } - } - - // 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 - } - - // 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 { - // TODO(sbueringer) use KRef after 5153 has been merged - return nil, errors.Wrapf(err, "failed to add templates of %s/%s to templatesInUse", md.Kind, md.Name) - } - return templatesInUse, nil -} - -// DeleteTemplateIfUnused deletes the template (ref), if it is not in use (i.e. in templatesInUse). -func DeleteTemplateIfUnused(ctx context.Context, c client.Client, templatesInUse map[string]bool, ref *corev1.ObjectReference) error { - // If ref is nil, do nothing (this can happen, because bootstrap templates are optional). - if ref == nil { - return nil - } - - // TODO(sbueringer) use loggerFrom after 5153 has been merged - // log := loggerFrom(ctx).WithRef(ref) - log := ctrl.LoggerFrom(ctx) - - refID, err := refID(ref) - if err != nil { - return errors.Wrapf(err, "failed to calculate refID") - } - - // If the template is still in use, do nothing. - if templatesInUse[refID] { - return nil - } - - // TODO(sbueringer) use KRef after 5153 has been merged - log.Info(fmt.Sprintf("Deleting %s/%s", ref.Kind, ref.Name)) - if err := external.Delete(ctx, c, ref); err != nil && !apierrors.IsNotFound(err) { - // TODO(sbueringer) use KRef after 5153 has been merged - return errors.Wrapf(err, "failed to delete %s/%s", ref.Kind, ref.Name) - } - return nil -} - -// addTemplateRef adds the refs to the refMap with the refID as key. -func addTemplateRef(refMap map[string]bool, refs ...*corev1.ObjectReference) error { - for _, ref := range refs { - if ref != nil { - refID, err := refID(ref) - if err != nil { - return errors.Wrapf(err, "failed to calculate refID") - } - refMap[refID] = true - } - } - return nil -} - -// refID returns the refID of a ObjectReference in the format: g/k/name. -// Note: We don't include the version as references with different versions should be treated as equal. -func refID(ref *corev1.ObjectReference) (string, error) { - if ref == nil { - return "", nil - } - - gv, err := schema.ParseGroupVersion(ref.APIVersion) - if err != nil { - return "", errors.Wrapf(err, "failed to parse apiVersion %q", ref.APIVersion) - } - - return fmt.Sprintf("%s/%s/%s", gv.Group, ref.Kind, ref.Name), nil -} diff --git a/controllers/topology/internal/templateutil/template_test.go b/controllers/topology/internal/templateutil/template_test.go deleted file mode 100644 index 50cc12911d64..000000000000 --- a/controllers/topology/internal/templateutil/template_test.go +++ /dev/null @@ -1,122 +0,0 @@ -/* -Copyright 2021 The Kubernetes Authors. - -Licensed under the Apache License, Version 2.0 (the "License"); -you may not use this file except in compliance with the License. -You may obtain a copy of the License at - - http://www.apache.org/licenses/LICENSE-2.0 - -Unless required by applicable law or agreed to in writing, software -distributed under the License is distributed on an "AS IS" BASIS, -WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. -See the License for the specific language governing permissions and -limitations under the License. -*/ - -package templateutil - -import ( - "testing" - - . "github.com/onsi/gomega" - "github.com/pkg/errors" - corev1 "k8s.io/api/core/v1" - metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" - clusterv1 "sigs.k8s.io/cluster-api/api/v1alpha4" - "sigs.k8s.io/cluster-api/internal/testtypes" -) - -func TestCalculateTemplatesInUse(t *testing.T) { - t.Run("Calculate templates in use with regular MachineDeployment and MachineSet", func(t *testing.T) { - g := NewWithT(t) - - md := testtypes.NewMachineDeploymentBuilder(metav1.NamespaceDefault, "md"). - WithBootstrapTemplate(testtypes.NewBootstrapTemplateBuilder(metav1.NamespaceDefault, "mdBT").Build()). - WithInfrastructureTemplate(testtypes.NewInfrastructureMachineTemplateBuilder(metav1.NamespaceDefault, "mdIMT").Build()). - Build() - ms := testtypes.NewMachineSetBuilder(metav1.NamespaceDefault, "ms"). - WithBootstrapTemplate(testtypes.NewBootstrapTemplateBuilder(metav1.NamespaceDefault, "msBT").Build()). - WithInfrastructureTemplate(testtypes.NewInfrastructureMachineTemplateBuilder(metav1.NamespaceDefault, "msIMT").Build()). - Build() - - actual, err := CalculateTemplatesInUse(md, []*clusterv1.MachineSet{ms}) - g.Expect(err).ToNot(HaveOccurred()) - g.Expect(actual).To(HaveLen(4)) - - g.Expect(actual).To(HaveKey(mustRefID(md.Spec.Template.Spec.Bootstrap.ConfigRef))) - g.Expect(actual).To(HaveKey(mustRefID(&md.Spec.Template.Spec.InfrastructureRef))) - - g.Expect(actual).To(HaveKey(mustRefID(ms.Spec.Template.Spec.Bootstrap.ConfigRef))) - g.Expect(actual).To(HaveKey(mustRefID(&ms.Spec.Template.Spec.InfrastructureRef))) - }) - - t.Run("Calculate templates in use with MachineDeployment and MachineSet without BootstrapTemplate", func(t *testing.T) { - g := NewWithT(t) - - mdWithoutBootstrapTemplate := testtypes.NewMachineDeploymentBuilder(metav1.NamespaceDefault, "md"). - WithInfrastructureTemplate(testtypes.NewInfrastructureMachineTemplateBuilder(metav1.NamespaceDefault, "mdIMT").Build()). - Build() - msWithoutBootstrapTemplate := testtypes.NewMachineSetBuilder(metav1.NamespaceDefault, "ms"). - WithInfrastructureTemplate(testtypes.NewInfrastructureMachineTemplateBuilder(metav1.NamespaceDefault, "msIMT").Build()). - Build() - - actual, err := CalculateTemplatesInUse(mdWithoutBootstrapTemplate, []*clusterv1.MachineSet{msWithoutBootstrapTemplate}) - g.Expect(err).ToNot(HaveOccurred()) - g.Expect(actual).To(HaveLen(2)) - - g.Expect(actual).To(HaveKey(mustRefID(&mdWithoutBootstrapTemplate.Spec.Template.Spec.InfrastructureRef))) - - g.Expect(actual).To(HaveKey(mustRefID(&msWithoutBootstrapTemplate.Spec.Template.Spec.InfrastructureRef))) - }) - - t.Run("Calculate templates in use with MachineDeployment and MachineSet ignore templates when resources in deleting", func(t *testing.T) { - g := NewWithT(t) - - deletionTimeStamp := metav1.Now() - - mdInDeleting := testtypes.NewMachineDeploymentBuilder(metav1.NamespaceDefault, "md"). - WithInfrastructureTemplate(testtypes.NewInfrastructureMachineTemplateBuilder(metav1.NamespaceDefault, "mdIMT").Build()). - Build() - mdInDeleting.SetDeletionTimestamp(&deletionTimeStamp) - - msInDeleting := testtypes.NewMachineSetBuilder(metav1.NamespaceDefault, "ms"). - WithInfrastructureTemplate(testtypes.NewInfrastructureMachineTemplateBuilder(metav1.NamespaceDefault, "msIMT").Build()). - Build() - msInDeleting.SetDeletionTimestamp(&deletionTimeStamp) - - actual, err := CalculateTemplatesInUse(mdInDeleting, []*clusterv1.MachineSet{msInDeleting}) - g.Expect(err).ToNot(HaveOccurred()) - g.Expect(actual).To(HaveLen(0)) - - g.Expect(actual).ToNot(HaveKey(mustRefID(&mdInDeleting.Spec.Template.Spec.InfrastructureRef))) - - g.Expect(actual).ToNot(HaveKey(mustRefID(&msInDeleting.Spec.Template.Spec.InfrastructureRef))) - }) - - t.Run("Calculate templates in use without MachineDeployment and with MachineSet", func(t *testing.T) { - g := NewWithT(t) - - ms := testtypes.NewMachineSetBuilder(metav1.NamespaceDefault, "ms"). - WithBootstrapTemplate(testtypes.NewBootstrapTemplateBuilder(metav1.NamespaceDefault, "msBT").Build()). - WithInfrastructureTemplate(testtypes.NewInfrastructureMachineTemplateBuilder(metav1.NamespaceDefault, "msIMT").Build()). - Build() - - actual, err := CalculateTemplatesInUse(nil, []*clusterv1.MachineSet{ms}) - g.Expect(err).ToNot(HaveOccurred()) - g.Expect(actual).To(HaveLen(2)) - - g.Expect(actual).To(HaveKey(mustRefID(ms.Spec.Template.Spec.Bootstrap.ConfigRef))) - g.Expect(actual).To(HaveKey(mustRefID(&ms.Spec.Template.Spec.InfrastructureRef))) - }) -} - -// mustRefID returns the refID as calculated by refID, but panics -// if refID returns an error. -func mustRefID(ref *corev1.ObjectReference) string { - refID, err := refID(ref) - if err != nil { - panic(errors.Wrap(err, "failed to calculate refID")) - } - return refID -} diff --git a/controllers/topology/machinedeployment_controller.go b/controllers/topology/machinedeployment_controller.go index 037dab996c09..09462fa6408e 100644 --- a/controllers/topology/machinedeployment_controller.go +++ b/controllers/topology/machinedeployment_controller.go @@ -23,7 +23,6 @@ import ( apierrors "k8s.io/apimachinery/pkg/api/errors" "k8s.io/apimachinery/pkg/types" clusterv1 "sigs.k8s.io/cluster-api/api/v1alpha4" - "sigs.k8s.io/cluster-api/controllers/topology/internal/templateutil" "sigs.k8s.io/cluster-api/util" "sigs.k8s.io/cluster-api/util/annotations" "sigs.k8s.io/cluster-api/util/patch" @@ -40,18 +39,21 @@ import ( // +kubebuilder:rbac:groups=cluster.x-k8s.io,resources=machinesets,verbs=get;list;watch // MachineDeploymentReconciler deletes referenced templates during deletion of topology-owned MachineDeployments. -// The templates are only deleted, if they are not used in other MachineDeployments or MachineSets which are not in deleting, +// The templates are only deleted, if they are not used in other MachineDeployments or MachineSets which are not in deleting state, // i.e. the templates would otherwise be orphaned after the MachineDeployment deletion completes. // Note: To achieve this the cluster topology controller sets a finalizer to hook into the MachineDeployment deletions. type MachineDeploymentReconciler struct { - Client client.Client + Client client.Client + // APIReader is used to list MachineSets directly via the API server to avoid + // race conditions caused by an outdated cache. + APIReader client.Reader WatchFilterValue string } func (r *MachineDeploymentReconciler) SetupWithManager(ctx context.Context, mgr ctrl.Manager, options controller.Options) error { err := ctrl.NewControllerManagedBy(mgr). For(&clusterv1.MachineDeployment{}). - Named("machinedeployment/topology"). + Named("topology/machinedeployment"). WithOptions(options). WithEventFilter(predicates.All(ctrl.LoggerFrom(ctx), predicates.ResourceNotPausedAndHasFilterLabel(ctrl.LoggerFrom(ctx), r.WatchFilterValue), @@ -66,7 +68,7 @@ func (r *MachineDeploymentReconciler) SetupWithManager(ctx context.Context, mgr } // Reconcile deletes referenced templates during deletion of topology-owned MachineDeployments. -// The templates are only deleted, if they are not used in other MachineDeployments or MachineSets which are not in deleting, +// The templates are only deleted, if they are not used in other MachineDeployments or MachineSets which are not in deleting state, // i.e. the templates would otherwise be orphaned after the MachineDeployment deletion completes. // Additional context: // * MachineDeployment deletion: @@ -113,26 +115,26 @@ func (r *MachineDeploymentReconciler) Reconcile(ctx context.Context, req ctrl.Re // 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) { +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}) + msList, err := getMachineSetsForDeployment(ctx, r.APIReader, types.NamespacedName{Namespace: md.Namespace, Name: md.Name}) if err != nil { return ctrl.Result{}, err } - // Calculate which templates are still in use by MachineDeployments or MachineSets which are not in deleting. - templatesInUse, err := templateutil.CalculateTemplatesInUse(md, msList) + // Calculate which templates are still in use by MachineDeployments or MachineSets which are not in deleting state. + templatesInUse, err := calculateTemplatesInUse(md, msList) if 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 { + if err := 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 { + if err := deleteTemplateIfUnused(ctx, r.Client, templatesInUse, ref); err != nil { return ctrl.Result{}, errors.Wrapf(err, "failed to delete infrastructure template for %s", KRef{Obj: md}) } diff --git a/controllers/topology/machinedeployment_controller_test.go b/controllers/topology/machinedeployment_controller_test.go index 0f65a114f49b..3fb591aed469 100644 --- a/controllers/topology/machinedeployment_controller_test.go +++ b/controllers/topology/machinedeployment_controller_test.go @@ -51,7 +51,8 @@ func TestMachineDeploymentReconciler_ReconcileDelete(t *testing.T) { Build() r := &MachineDeploymentReconciler{ - Client: fakeClient, + Client: fakeClient, + APIReader: fakeClient, } _, err := r.reconcileDelete(ctx, md) g.Expect(err).ToNot(HaveOccurred()) @@ -79,7 +80,8 @@ func TestMachineDeploymentReconciler_ReconcileDelete(t *testing.T) { Build() r := &MachineDeploymentReconciler{ - Client: fakeClient, + Client: fakeClient, + APIReader: fakeClient, } _, err := r.reconcileDelete(ctx, mdWithoutBootstrapTemplate) g.Expect(err).ToNot(HaveOccurred()) @@ -108,7 +110,8 @@ func TestMachineDeploymentReconciler_ReconcileDelete(t *testing.T) { Build() r := &MachineDeploymentReconciler{ - Client: fakeClient, + Client: fakeClient, + APIReader: fakeClient, } _, err := r.reconcileDelete(ctx, md) g.Expect(err).ToNot(HaveOccurred()) @@ -129,7 +132,6 @@ func templateExists(fakeClient client.Reader, template *unstructured.Unstructure err := fakeClient.Get(ctx, client.ObjectKeyFromObject(template), obj) if err != nil && !apierrors.IsNotFound(err) { - // This should never happen. panic(errors.Wrapf(err, "failed to get %s/%s", template.GetKind(), template.GetName())) } return err == nil diff --git a/controllers/topology/machineset_controller.go b/controllers/topology/machineset_controller.go index 7765018e4077..5f24827c3cbb 100644 --- a/controllers/topology/machineset_controller.go +++ b/controllers/topology/machineset_controller.go @@ -24,7 +24,6 @@ import ( "k8s.io/apimachinery/pkg/runtime/schema" "k8s.io/apimachinery/pkg/types" clusterv1 "sigs.k8s.io/cluster-api/api/v1alpha4" - "sigs.k8s.io/cluster-api/controllers/topology/internal/templateutil" "sigs.k8s.io/cluster-api/util" "sigs.k8s.io/cluster-api/util/annotations" "sigs.k8s.io/cluster-api/util/patch" @@ -41,18 +40,21 @@ import ( // +kubebuilder:rbac:groups=cluster.x-k8s.io,resources=machinesets;machinesets/finalizers,verbs=get;list;watch;update;patch;delete // MachineSetReconciler deletes referenced templates during deletion of topology-owned MachineSets. -// The templates are only deleted, if they are not used in other MachineDeployments or MachineSets which are not in deleting, +// The templates are only deleted, if they are not used in other MachineDeployments or MachineSets which are not in deleting state, // i.e. the templates would otherwise be orphaned after the MachineSet deletion completes. // Note: To achieve this the reconciler sets a finalizer to hook into the MachineSet deletions. type MachineSetReconciler struct { - Client client.Client + Client client.Client + // APIReader is used to list MachineSets directly via the API server to avoid + // race conditions caused by an outdated cache. + APIReader client.Reader WatchFilterValue string } func (r *MachineSetReconciler) SetupWithManager(ctx context.Context, mgr ctrl.Manager, options controller.Options) error { err := ctrl.NewControllerManagedBy(mgr). For(&clusterv1.MachineSet{}). - Named("machineset/topology"). + Named("topology/machineset"). WithOptions(options). WithEventFilter(predicates.All(ctrl.LoggerFrom(ctx), predicates.ResourceNotPausedAndHasFilterLabel(ctrl.LoggerFrom(ctx), r.WatchFilterValue), @@ -67,7 +69,7 @@ func (r *MachineSetReconciler) SetupWithManager(ctx context.Context, mgr ctrl.Ma } // Reconcile deletes referenced templates during deletion of topology-owned MachineSets. -// The templates are only deleted, if they are not used in other MachineDeployments or MachineSets which are not in deleting, +// The templates are only deleted, if they are not used in other MachineDeployments or MachineSets which are not in deleting state, // i.e. the templates would otherwise be orphaned after the MachineSet deletion completes. // Additional context: // * MachineSet deletion: @@ -124,7 +126,7 @@ func (r *MachineSetReconciler) Reconcile(ctx context.Context, req ctrl.Request) // 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) { +func (r *MachineSetReconciler) reconcileDelete(ctx context.Context, ms *clusterv1.MachineSet) (ctrl.Result, error) { // Gets the name of the MachineDeployment that controls this MachineSet. mdName, err := getMachineDeploymentName(ms) if err != nil { @@ -132,7 +134,7 @@ func (r MachineSetReconciler) reconcileDelete(ctx context.Context, ms *clusterv1 } // Get all the MachineSets for the MachineDeployment. - msList, err := getMachineSetsForDeployment(ctx, r.Client, *mdName) + msList, err := getMachineSetsForDeployment(ctx, r.APIReader, *mdName) if err != nil { return ctrl.Result{}, err } @@ -149,19 +151,19 @@ func (r MachineSetReconciler) reconcileDelete(ctx context.Context, ms *clusterv1 md = nil } - // Calculate which templates are still in use by MachineDeployments or MachineSets which are not in deleting. - templatesInUse, err := templateutil.CalculateTemplatesInUse(md, msList) + // Calculate which templates are still in use by MachineDeployments or MachineSets which are not in deleting state. + templatesInUse, err := calculateTemplatesInUse(md, msList) if 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 { + if err := 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 { + if err := deleteTemplateIfUnused(ctx, r.Client, templatesInUse, ref); err != nil { return ctrl.Result{}, errors.Wrapf(err, "failed to delete infrastructure template for %s", KRef{Obj: ms}) } @@ -186,7 +188,6 @@ func getMachineDeploymentName(ms *clusterv1.MachineSet) (*types.NamespacedName, } gv, err := schema.ParseGroupVersion(ref.APIVersion) if err != nil { - // This should never happen. return nil, errors.Errorf("could not calculate MachineDeployment name for %s: invalid apiVersion %q: %v", KRef{Obj: ms}, ref.APIVersion, err) } diff --git a/controllers/topology/machineset_controller_test.go b/controllers/topology/machineset_controller_test.go index a57ba0ea74a3..6336166638ce 100644 --- a/controllers/topology/machineset_controller_test.go +++ b/controllers/topology/machineset_controller_test.go @@ -60,7 +60,8 @@ func TestMachineSetReconciler_ReconcileDelete(t *testing.T) { Build() r := &MachineSetReconciler{ - Client: fakeClient, + Client: fakeClient, + APIReader: fakeClient, } _, err := r.reconcileDelete(ctx, ms) g.Expect(err).ToNot(HaveOccurred()) @@ -98,7 +99,8 @@ func TestMachineSetReconciler_ReconcileDelete(t *testing.T) { Build() r := &MachineSetReconciler{ - Client: fakeClient, + Client: fakeClient, + APIReader: fakeClient, } _, err := r.reconcileDelete(ctx, msWithoutBootstrapTemplate) g.Expect(err).ToNot(HaveOccurred()) @@ -124,7 +126,8 @@ func TestMachineSetReconciler_ReconcileDelete(t *testing.T) { Build() r := &MachineSetReconciler{ - Client: fakeClient, + Client: fakeClient, + APIReader: fakeClient, } _, err := r.reconcileDelete(ctx, ms) g.Expect(err).ToNot(HaveOccurred()) @@ -169,7 +172,8 @@ func TestMachineSetReconciler_ReconcileDelete(t *testing.T) { Build() r := &MachineSetReconciler{ - Client: fakeClient, + Client: fakeClient, + APIReader: fakeClient, } _, err := r.reconcileDelete(ctx, ms) g.Expect(err).ToNot(HaveOccurred()) diff --git a/controllers/topology/util.go b/controllers/topology/util.go index 7b98f031b414..981ffe900e25 100644 --- a/controllers/topology/util.go +++ b/controllers/topology/util.go @@ -23,7 +23,9 @@ import ( "github.com/go-logr/logr" "github.com/pkg/errors" corev1 "k8s.io/api/core/v1" + apierrors "k8s.io/apimachinery/pkg/api/errors" "k8s.io/apimachinery/pkg/apis/meta/v1/unstructured" + "k8s.io/apimachinery/pkg/runtime/schema" "k8s.io/apimachinery/pkg/types" clusterv1 "sigs.k8s.io/cluster-api/api/v1alpha4" "sigs.k8s.io/cluster-api/controllers/external" @@ -185,7 +187,7 @@ func refToUnstructured(ref *corev1.ObjectReference) *unstructured.Unstructured { } // getMachineSetsForDeployment returns a list of MachineSets associated with a MachineDeployment. -func getMachineSetsForDeployment(ctx context.Context, c client.Client, md types.NamespacedName) ([]*clusterv1.MachineSet, error) { +func getMachineSetsForDeployment(ctx context.Context, c client.Reader, md types.NamespacedName) ([]*clusterv1.MachineSet, error) { // List MachineSets based on the MachineDeployment label. msList := &clusterv1.MachineSetList{} if err := c.List(ctx, msList, @@ -200,3 +202,94 @@ func getMachineSetsForDeployment(ctx context.Context, c client.Client, md types. } return res, nil } + +// calculateTemplatesInUse returns all templates referenced in non-deleting MachineDeployment and MachineSets. +func calculateTemplatesInUse(md *clusterv1.MachineDeployment, msList []*clusterv1.MachineSet) (map[string]bool, error) { + templatesInUse := map[string]bool{} + + // Templates of the MachineSet are still in use if the MachineSet is not in deleting state. + for _, ms := range msList { + if !ms.DeletionTimestamp.IsZero() { + continue + } + + bootstrapRef := ms.Spec.Template.Spec.Bootstrap.ConfigRef + infrastructureRef := &ms.Spec.Template.Spec.InfrastructureRef + if err := addTemplateRef(templatesInUse, bootstrapRef, infrastructureRef); err != nil { + return nil, errors.Wrapf(err, "failed to add templates of %s to templatesInUse", KRef{Obj: ms}) + } + } + + // If MachineDeployment has already been deleted or still exists and is in deleting state, 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 + } + + // 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 { + return nil, errors.Wrapf(err, "failed to add templates of %s to templatesInUse", KRef{Obj: md}) + } + return templatesInUse, nil +} + +// deleteTemplateIfUnused deletes the template (ref), if it is not in use (i.e. in templatesInUse). +func deleteTemplateIfUnused(ctx context.Context, c client.Client, templatesInUse map[string]bool, ref *corev1.ObjectReference) error { + // If ref is nil, do nothing (this can happen, because bootstrap templates are optional). + if ref == nil { + return nil + } + + log := loggerFrom(ctx).WithRef(ref) + + refID, err := templateRefID(ref) + if err != nil { + return errors.Wrapf(err, "failed to calculate templateRefID") + } + + // If the template is still in use, do nothing. + if templatesInUse[refID] { + // TODO(sbueringer) use KRef after 5153 has been merged + log.V(3).Infof("Not deleting %s/%s, because it's still in use", ref.Kind, ref.Name) + return nil + } + + // TODO(sbueringer) use KRef after 5153 has been merged + log.Infof("Deleting %s/%s", ref.Kind, ref.Name) + if err := external.Delete(ctx, c, ref); err != nil && !apierrors.IsNotFound(err) { + // TODO(sbueringer) use KRef after 5153 has been merged + return errors.Wrapf(err, "failed to delete %s/%s", ref.Kind, ref.Name) + } + return nil +} + +// addTemplateRef adds the refs to the refMap with the templateRefID as key. +func addTemplateRef(refMap map[string]bool, refs ...*corev1.ObjectReference) error { + for _, ref := range refs { + if ref != nil { + refID, err := templateRefID(ref) + if err != nil { + return errors.Wrapf(err, "failed to calculate templateRefID") + } + refMap[refID] = true + } + } + return nil +} + +// templateRefID returns the templateRefID of a ObjectReference in the format: g/k/name. +// Note: We don't include the version as references with different versions should be treated as equal. +func templateRefID(ref *corev1.ObjectReference) (string, error) { + if ref == nil { + return "", nil + } + + gv, err := schema.ParseGroupVersion(ref.APIVersion) + if err != nil { + return "", errors.Wrapf(err, "failed to parse apiVersion %q", ref.APIVersion) + } + + return fmt.Sprintf("%s/%s/%s", gv.Group, ref.Kind, ref.Name), nil +} diff --git a/controllers/topology/util_test.go b/controllers/topology/util_test.go index dd4c67ac9373..c0c0ce30d0d4 100644 --- a/controllers/topology/util_test.go +++ b/controllers/topology/util_test.go @@ -19,6 +19,8 @@ package topology import ( "testing" + "github.com/pkg/errors" + clusterv1 "sigs.k8s.io/cluster-api/api/v1alpha4" "sigs.k8s.io/cluster-api/internal/testtypes" "github.com/google/go-cmp/cmp" @@ -114,3 +116,97 @@ func TestGetTemplate(t *testing.T) { }) } } + +func TestCalculateTemplatesInUse(t *testing.T) { + t.Run("Calculate templates in use with regular MachineDeployment and MachineSet", func(t *testing.T) { + g := NewWithT(t) + + md := testtypes.NewMachineDeploymentBuilder(metav1.NamespaceDefault, "md"). + WithBootstrapTemplate(testtypes.NewBootstrapTemplateBuilder(metav1.NamespaceDefault, "mdBT").Build()). + WithInfrastructureTemplate(testtypes.NewInfrastructureMachineTemplateBuilder(metav1.NamespaceDefault, "mdIMT").Build()). + Build() + ms := testtypes.NewMachineSetBuilder(metav1.NamespaceDefault, "ms"). + WithBootstrapTemplate(testtypes.NewBootstrapTemplateBuilder(metav1.NamespaceDefault, "msBT").Build()). + WithInfrastructureTemplate(testtypes.NewInfrastructureMachineTemplateBuilder(metav1.NamespaceDefault, "msIMT").Build()). + Build() + + actual, err := calculateTemplatesInUse(md, []*clusterv1.MachineSet{ms}) + g.Expect(err).ToNot(HaveOccurred()) + g.Expect(actual).To(HaveLen(4)) + + g.Expect(actual).To(HaveKey(mustTemplateRefID(md.Spec.Template.Spec.Bootstrap.ConfigRef))) + g.Expect(actual).To(HaveKey(mustTemplateRefID(&md.Spec.Template.Spec.InfrastructureRef))) + + g.Expect(actual).To(HaveKey(mustTemplateRefID(ms.Spec.Template.Spec.Bootstrap.ConfigRef))) + g.Expect(actual).To(HaveKey(mustTemplateRefID(&ms.Spec.Template.Spec.InfrastructureRef))) + }) + + t.Run("Calculate templates in use with MachineDeployment and MachineSet without BootstrapTemplate", func(t *testing.T) { + g := NewWithT(t) + + mdWithoutBootstrapTemplate := testtypes.NewMachineDeploymentBuilder(metav1.NamespaceDefault, "md"). + WithInfrastructureTemplate(testtypes.NewInfrastructureMachineTemplateBuilder(metav1.NamespaceDefault, "mdIMT").Build()). + Build() + msWithoutBootstrapTemplate := testtypes.NewMachineSetBuilder(metav1.NamespaceDefault, "ms"). + WithInfrastructureTemplate(testtypes.NewInfrastructureMachineTemplateBuilder(metav1.NamespaceDefault, "msIMT").Build()). + Build() + + actual, err := calculateTemplatesInUse(mdWithoutBootstrapTemplate, []*clusterv1.MachineSet{msWithoutBootstrapTemplate}) + g.Expect(err).ToNot(HaveOccurred()) + g.Expect(actual).To(HaveLen(2)) + + g.Expect(actual).To(HaveKey(mustTemplateRefID(&mdWithoutBootstrapTemplate.Spec.Template.Spec.InfrastructureRef))) + + g.Expect(actual).To(HaveKey(mustTemplateRefID(&msWithoutBootstrapTemplate.Spec.Template.Spec.InfrastructureRef))) + }) + + t.Run("Calculate templates in use with MachineDeployment and MachineSet ignore templates when resources in deleting", func(t *testing.T) { + g := NewWithT(t) + + deletionTimeStamp := metav1.Now() + + mdInDeleting := testtypes.NewMachineDeploymentBuilder(metav1.NamespaceDefault, "md"). + WithInfrastructureTemplate(testtypes.NewInfrastructureMachineTemplateBuilder(metav1.NamespaceDefault, "mdIMT").Build()). + Build() + mdInDeleting.SetDeletionTimestamp(&deletionTimeStamp) + + msInDeleting := testtypes.NewMachineSetBuilder(metav1.NamespaceDefault, "ms"). + WithInfrastructureTemplate(testtypes.NewInfrastructureMachineTemplateBuilder(metav1.NamespaceDefault, "msIMT").Build()). + Build() + msInDeleting.SetDeletionTimestamp(&deletionTimeStamp) + + actual, err := calculateTemplatesInUse(mdInDeleting, []*clusterv1.MachineSet{msInDeleting}) + g.Expect(err).ToNot(HaveOccurred()) + g.Expect(actual).To(HaveLen(0)) + + g.Expect(actual).ToNot(HaveKey(mustTemplateRefID(&mdInDeleting.Spec.Template.Spec.InfrastructureRef))) + + g.Expect(actual).ToNot(HaveKey(mustTemplateRefID(&msInDeleting.Spec.Template.Spec.InfrastructureRef))) + }) + + t.Run("Calculate templates in use without MachineDeployment and with MachineSet", func(t *testing.T) { + g := NewWithT(t) + + ms := testtypes.NewMachineSetBuilder(metav1.NamespaceDefault, "ms"). + WithBootstrapTemplate(testtypes.NewBootstrapTemplateBuilder(metav1.NamespaceDefault, "msBT").Build()). + WithInfrastructureTemplate(testtypes.NewInfrastructureMachineTemplateBuilder(metav1.NamespaceDefault, "msIMT").Build()). + Build() + + actual, err := calculateTemplatesInUse(nil, []*clusterv1.MachineSet{ms}) + g.Expect(err).ToNot(HaveOccurred()) + g.Expect(actual).To(HaveLen(2)) + + g.Expect(actual).To(HaveKey(mustTemplateRefID(ms.Spec.Template.Spec.Bootstrap.ConfigRef))) + g.Expect(actual).To(HaveKey(mustTemplateRefID(&ms.Spec.Template.Spec.InfrastructureRef))) + }) +} + +// mustTemplateRefID returns the templateRefID as calculated by templateRefID, but panics +// if templateRefID returns an error. +func mustTemplateRefID(ref *corev1.ObjectReference) string { + refID, err := templateRefID(ref) + if err != nil { + panic(errors.Wrap(err, "failed to calculate templateRefID")) + } + return refID +} diff --git a/main.go b/main.go index f2bef8324b91..c7d3abc0ee88 100644 --- a/main.go +++ b/main.go @@ -287,6 +287,7 @@ func setupReconcilers(ctx context.Context, mgr ctrl.Manager) { if err := (&topology.MachineDeploymentReconciler{ Client: mgr.GetClient(), + APIReader: mgr.GetAPIReader(), WatchFilterValue: watchFilterValue, }).SetupWithManager(ctx, mgr, controller.Options{}); err != nil { setupLog.Error(err, "unable to create controller", "controller", "MachineDeploymentTopology") @@ -295,6 +296,7 @@ func setupReconcilers(ctx context.Context, mgr ctrl.Manager) { if err := (&topology.MachineSetReconciler{ Client: mgr.GetClient(), + APIReader: mgr.GetAPIReader(), WatchFilterValue: watchFilterValue, }).SetupWithManager(ctx, mgr, controller.Options{}); err != nil { setupLog.Error(err, "unable to create controller", "controller", "MachineSetTopology")