Merge pull request kubernetes-sigs#10780 from sbueringer/pr-improve-m…

…dms-topo-reconciler 🐛 MD/MS topo reconciler: only add finalizer for owned MD/MS
mesosphere · Jun 18, 2024 · 54c8e46 · 54c8e46
2 parents 1cbe864 + 1932242
commit 54c8e46
Show file tree

Hide file tree

Showing 4 changed files with 35 additions and 2 deletions.
diff --git a/internal/controllers/topology/machinedeployment/machinedeployment_controller.go b/internal/controllers/topology/machinedeployment/machinedeployment_controller.go
@@ -18,6 +18,7 @@ package machinedeployment
 
 import (
 	"context"
+	"fmt"
 
 	"github.com/pkg/errors"
 	apierrors "k8s.io/apimachinery/pkg/api/errors"
@@ -35,6 +36,7 @@ import (
 	tlog "sigs.k8s.io/cluster-api/internal/log"
 	"sigs.k8s.io/cluster-api/util"
 	"sigs.k8s.io/cluster-api/util/annotations"
+	"sigs.k8s.io/cluster-api/util/labels"
 	"sigs.k8s.io/cluster-api/util/patch"
 	"sigs.k8s.io/cluster-api/util/predicates"
 )
@@ -133,6 +135,12 @@ func (r *Reconciler) Reconcile(ctx context.Context, req ctrl.Request) (_ ctrl.Re
 		return ctrl.Result{}, nil
 	}
 
+	// Return early if the MachineDeployment is not topology owned.
+	if !labels.IsTopologyOwned(md) {
+		log.Info(fmt.Sprintf("Reconciliation is skipped because the MachineDeployment does not have the %q label", clusterv1.ClusterTopologyOwnedLabel))
+		return ctrl.Result{}, nil
+	}
+
 	// Create a patch helper to add or remove the finalizer from the MachineDeployment.
 	patchHelper, err := patch.NewHelper(md, r.Client)
 	if err != nil {

diff --git a/internal/controllers/topology/machinedeployment/machinedeployment_controller_test.go b/internal/controllers/topology/machinedeployment/machinedeployment_controller_test.go
@@ -41,9 +41,14 @@ func TestMachineDeploymentTopologyFinalizer(t *testing.T) {
 	mdBuilder := builder.MachineDeployment(metav1.NamespaceDefault, "md").
 		WithClusterName("fake-cluster").
 		WithBootstrapTemplate(mdBT).
-		WithInfrastructureTemplate(mdIMT)
-
+		WithInfrastructureTemplate(mdIMT).
+		WithLabels(map[string]string{
+			clusterv1.ClusterTopologyOwnedLabel: "",
+		})
 	md := mdBuilder.Build()
+
+	mdWithoutTopologyOwnedLabel := md.DeepCopy()
+	delete(mdWithoutTopologyOwnedLabel.Labels, clusterv1.ClusterTopologyOwnedLabel)
 	mdWithFinalizer := mdBuilder.Build()
 	mdWithFinalizer.Finalizers = []string{clusterv1.MachineDeploymentTopologyFinalizer}
 
@@ -64,6 +69,11 @@ func TestMachineDeploymentTopologyFinalizer(t *testing.T) {
 			md:              mdWithFinalizer,
 			expectFinalizer: true,
 		},
+		{
+			name:            "should not add ClusterTopology finalizer on MachineDeployment without topology owned label",
+			md:              mdWithoutTopologyOwnedLabel,
+			expectFinalizer: false,
+		},
 	}
 
 	for _, tc := range testCases {

diff --git a/internal/controllers/topology/machineset/machineset_controller.go b/internal/controllers/topology/machineset/machineset_controller.go
@@ -18,6 +18,7 @@ package machineset
 
 import (
 	"context"
+	"fmt"
 
 	"github.com/pkg/errors"
 	apierrors "k8s.io/apimachinery/pkg/api/errors"
@@ -36,6 +37,7 @@ import (
 	tlog "sigs.k8s.io/cluster-api/internal/log"
 	"sigs.k8s.io/cluster-api/util"
 	"sigs.k8s.io/cluster-api/util/annotations"
+	"sigs.k8s.io/cluster-api/util/labels"
 	clog "sigs.k8s.io/cluster-api/util/log"
 	"sigs.k8s.io/cluster-api/util/patch"
 	"sigs.k8s.io/cluster-api/util/predicates"
@@ -140,6 +142,12 @@ func (r *Reconciler) Reconcile(ctx context.Context, req ctrl.Request) (_ ctrl.Re
 		return ctrl.Result{}, nil
 	}
 
+	// Return early if the MachineSet is not topology owned.
+	if !labels.IsTopologyOwned(ms) {
+		log.Info(fmt.Sprintf("Reconciliation is skipped because the MachineSet does not have the %q label", clusterv1.ClusterTopologyOwnedLabel))
+		return ctrl.Result{}, nil
+	}
+
 	// Create a patch helper to add or remove the finalizer from the MachineSet.
 	patchHelper, err := patch.NewHelper(ms, r.Client)
 	if err != nil {

diff --git a/internal/controllers/topology/machineset/machineset_controller_test.go b/internal/controllers/topology/machineset/machineset_controller_test.go
@@ -58,6 +58,8 @@ func TestMachineSetTopologyFinalizer(t *testing.T) {
 		})
 
 	ms := msBuilder.Build()
+	msWithoutTopologyOwnedLabel := ms.DeepCopy()
+	delete(msWithoutTopologyOwnedLabel.Labels, clusterv1.ClusterTopologyOwnedLabel)
 	msWithFinalizer := msBuilder.Build()
 	msWithFinalizer.Finalizers = []string{clusterv1.MachineSetTopologyFinalizer}
 
@@ -78,6 +80,11 @@ func TestMachineSetTopologyFinalizer(t *testing.T) {
 			ms:              msWithFinalizer,
 			expectFinalizer: true,
 		},
+		{
+			name:            "should not add ClusterTopology finalizer on MachineSet without topology owned label",
+			ms:              msWithoutTopologyOwnedLabel,
+			expectFinalizer: false,
+		},
 	}
 
 	for _, tc := range testCases {