review fixes

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),

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})
 	}
 

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