[0.1] set OwnerRef in more cases (#1207)

* Add HasOwner function

* Adopt conditionally based on owner

Author: liztio

pkg/controller/machine/machine_controller.go

@@ -26,6 +26,7 @@ import (
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 	"k8s.io/apimachinery/pkg/runtime"
 	"k8s.io/klog"
+	"sigs.k8s.io/cluster-api/pkg/apis/cluster/v1alpha1"
 	clusterv1 "sigs.k8s.io/cluster-api/pkg/apis/cluster/v1alpha1"
 	controllerError "sigs.k8s.io/cluster-api/pkg/controller/error"
 	"sigs.k8s.io/cluster-api/pkg/controller/remote"
@@ -122,7 +123,8 @@ func (r *ReconcileMachine) Reconcile(request reconcile.Request) (reconcile.Resul
 	}
 
 	// Set the ownerRef with foreground deletion if there is a linked cluster.
-	if cluster != nil && len(m.OwnerReferences) == 0 {
+	if cluster != nil && shouldAdopt(m) {
+		klog.Infof("Cluster %s/%s is adopting Machine %s", cluster.Namespace, cluster.Name, m.Name)
 		blockOwnerDeletion := true
 		m.OwnerReferences = append(m.OwnerReferences, metav1.OwnerReference{
 			APIVersion:         cluster.APIVersion,
@@ -384,3 +386,7 @@ func (r *ReconcileMachine) getMachinesInCluster(ctx context.Context, namespace,
 
 	return machines, nil
 }
+
+func shouldAdopt(m *v1alpha1.Machine) bool {
+	return !util.HasOwner(m.OwnerReferences, v1alpha1.SchemeGroupVersion.String(), []string{"MachineSet", "MachineDeployment", "Cluster"})
+}

pkg/controller/machinedeployment/machinedeployment_controller.go

@@ -173,7 +173,8 @@ func (r *ReconcileMachineDeployment) reconcile(ctx context.Context, d *v1alpha1.
 	}
 
 	// Set the ownerRef with foreground deletion if there is a linked cluster.
-	if cluster != nil && len(d.OwnerReferences) == 0 {
+	if cluster != nil && shouldAdopt(d) {
+		klog.Infof("Cluster %s/%s is adopting MachineDeployment %s", cluster.Namespace, cluster.Name, d.Name)
 		blockOwnerDeletion := true
 		d.OwnerReferences = append(d.OwnerReferences, metav1.OwnerReference{
 			APIVersion:         cluster.APIVersion,
@@ -412,3 +413,7 @@ func (r *ReconcileMachineDeployment) MachineSetToDeployments(o handler.MapObject
 
 	return result
 }
+
+func shouldAdopt(md *v1alpha1.MachineDeployment) bool {
+	return !util.HasOwner(md.OwnerReferences, v1alpha1.SchemeGroupVersion.String(), []string{"Cluster"})
+}

pkg/controller/machineset/machineset_controller.go

@@ -31,6 +31,7 @@ import (
 	"k8s.io/apimachinery/pkg/runtime"
 	"k8s.io/client-go/tools/record"
 	"k8s.io/klog"
+	"sigs.k8s.io/cluster-api/pkg/apis/cluster/v1alpha1"
 	clusterv1alpha1 "sigs.k8s.io/cluster-api/pkg/apis/cluster/v1alpha1"
 	"sigs.k8s.io/cluster-api/pkg/util"
 	"sigs.k8s.io/controller-runtime/pkg/client"
@@ -170,7 +171,8 @@ func (r *ReconcileMachineSet) reconcile(ctx context.Context, machineSet *cluster
 	}
 
 	// Set the ownerRef with foreground deletion if there is a linked cluster.
-	if cluster != nil && len(machineSet.OwnerReferences) == 0 {
+	if cluster != nil && shouldAdopt(machineSet) {
+		klog.Infof("Cluster %s/%s is adopting MachineSet %s", cluster.Namespace, cluster.Name, machineSet.Name)
 		blockOwnerDeletion := true
 		machineSet.OwnerReferences = append(machineSet.OwnerReferences, metav1.OwnerReference{
 			APIVersion:         cluster.APIVersion,
@@ -494,3 +496,7 @@ func (r *ReconcileMachineSet) MachineToMachineSets(o handler.MapObject) []reconc
 
 	return result
 }
+
+func shouldAdopt(ms *v1alpha1.MachineSet) bool {
+	return !util.HasOwner(ms.OwnerReferences, v1alpha1.SchemeGroupVersion.String(), []string{"MachineDeployment", "Cluster"})
+}

pkg/util/BUILD.bazel

@@ -27,6 +27,7 @@ go_test(
     srcs = ["util_test.go"],
     embed = [":go_default_library"],
     deps = [
+        "//pkg/apis/cluster/v1alpha1:go_default_library",
         "//vendor/k8s.io/apimachinery/pkg/apis/meta/v1:go_default_library",
         "//vendor/k8s.io/apimachinery/pkg/types:go_default_library",
     ],

pkg/util/util.go

@@ -326,3 +326,19 @@ func PointsTo(refs []metav1.OwnerReference, target *metav1.ObjectMeta) bool {
 
 	return false
 }
+
+// HasOwner checks if any of the references in the past list match the given apiVersion and one of the given kinds
+func HasOwner(refList []metav1.OwnerReference, apiVersion string, kinds []string) bool {
+	kMap := make(map[string]bool)
+	for _, kind := range kinds {
+		kMap[kind] = true
+	}
+
+	for _, mr := range refList {
+		if mr.APIVersion == apiVersion && kMap[mr.Kind] {
+			return true
+		}
+	}
+
+	return false
+}

pkg/util/util_test.go

@@ -23,6 +23,7 @@ import (
 
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 	"k8s.io/apimachinery/pkg/types"
+	"sigs.k8s.io/cluster-api/pkg/apis/cluster/v1alpha1"
 )
 
 const validCluster = `
@@ -390,3 +391,82 @@ func TestPointsTo(t *testing.T) {
 		})
 	}
 }
+
+func TestHasOwner(t *testing.T) {
+
+	tests := []struct {
+		name     string
+		refList  []metav1.OwnerReference
+		expected bool
+	}{
+		{
+			name: "no ownership",
+		},
+		{
+			name: "owned by cluster",
+			refList: []metav1.OwnerReference{
+				{
+					Kind:       "Cluster",
+					APIVersion: v1alpha1.SchemeGroupVersion.String(),
+				},
+			},
+			expected: true,
+		},
+
+		{
+			name: "owned by something else",
+			refList: []metav1.OwnerReference{
+				{
+					Kind:       "Pod",
+					APIVersion: "v1",
+				},
+				{
+					Kind:       "Deployment",
+					APIVersion: "apps/v1",
+				},
+			},
+		},
+		{
+			name: "owner by a deployment",
+			refList: []metav1.OwnerReference{
+				{
+					Kind:       "MachineDeployment",
+					APIVersion: v1alpha1.SchemeGroupVersion.String(),
+				},
+			},
+			expected: true,
+		},
+		{
+			name: "right kind, wrong apiversion",
+			refList: []metav1.OwnerReference{
+				{
+					Kind:       "MachineDeployment",
+					APIVersion: "wrong/v2",
+				},
+			},
+		},
+		{
+
+			name: "right apiversion, wrong kind",
+			refList: []metav1.OwnerReference{
+				{
+					Kind:       "Machine",
+					APIVersion: v1alpha1.SchemeGroupVersion.String(),
+				},
+			},
+		},
+	}
+
+	for _, test := range tests {
+		t.Run(test.name, func(t *testing.T) {
+			result := HasOwner(
+				test.refList,
+				v1alpha1.SchemeGroupVersion.String(),
+				[]string{"MachineDeployment", "Cluster"},
+			)
+			if test.expected != result {
+				t.Errorf("expected hasOwner to be %v, got %v", test.expected, result)
+			}
+		})
+	}
+}