Reuse hasMatchingLabels

This let the machineSet reconciler reuse the logic from hasMatchingLabels. It also fix getMachineSetsForMachine to return error appropriately to let the caller ignore them or not.
kubernetes-sigs · May 31, 2021 · c1ebebe · c1ebebe
1 parent f5dec18
commit c1ebebe
Show file tree

Hide file tree

Showing 3 changed files with 16 additions and 145 deletions.
diff --git a/controllers/machine_helpers_test.go b/controllers/machine_helpers_test.go
@@ -24,7 +24,7 @@ import (
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 )
 
-func TestMachineHealthCheckHasMatchingLabels(t *testing.T) {
+func TestHasMatchingLabels(t *testing.T) {
 	testCases := []struct {
 		name     string
 		selector metav1.LabelSelector
@@ -33,28 +33,24 @@ func TestMachineHealthCheckHasMatchingLabels(t *testing.T) {
 	}{
 		{
 			name: "selector matches labels",
-
 			selector: metav1.LabelSelector{
 				MatchLabels: map[string]string{
 					"foo": "bar",
 				},
 			},
-
 			labels: map[string]string{
-				"foo": "bar",
+				"foo":  "bar",
+				"more": "labels",
 			},
-
 			expected: true,
 		},
 		{
 			name: "selector does not match labels",
-
 			selector: metav1.LabelSelector{
 				MatchLabels: map[string]string{
 					"foo": "bar",
 				},
 			},
-
 			labels: map[string]string{
 				"no": "match",
 			},
@@ -67,7 +63,7 @@ func TestMachineHealthCheckHasMatchingLabels(t *testing.T) {
 			expected: false,
 		},
 		{
-			name: "seelctor is invalid",
+			name: "selector is invalid",
 			selector: metav1.LabelSelector{
 				MatchLabels: map[string]string{
 					"foo": "bar",

diff --git a/controllers/machineset_controller.go b/controllers/machineset_controller.go
@@ -519,6 +519,7 @@ func (r *MachineSetReconciler) waitForMachineDeletion(ctx context.Context, machi
 // MachineToMachineSets is a handler.ToRequestsFunc to be used to enqeue requests for reconciliation
 // for MachineSets that might adopt an orphaned Machine.
 func (r *MachineSetReconciler) MachineToMachineSets(o client.Object) []ctrl.Request {
+	log := ctrl.LoggerFrom(context.TODO(), "object", client.ObjectKeyFromObject(o))
 	result := []ctrl.Request{}
 
 	m, ok := o.(*clusterv1.Machine)
@@ -534,7 +535,11 @@ func (r *MachineSetReconciler) MachineToMachineSets(o client.Object) []ctrl.Requ
 		}
 	}
 
-	mss := r.getMachineSetsForMachine(context.TODO(), m)
+	mss, err := r.getMachineSetsForMachine(context.TODO(), m)
+	if err != nil {
+		log.Error(err, "Failed getting MachineSets for Machine")
+		return nil
+	}
 	if len(mss) == 0 {
 		return nil
 	}
@@ -547,53 +552,25 @@ func (r *MachineSetReconciler) MachineToMachineSets(o client.Object) []ctrl.Requ
 	return result
 }
 
-func (r *MachineSetReconciler) getMachineSetsForMachine(ctx context.Context, m *clusterv1.Machine) []*clusterv1.MachineSet {
-	log := ctrl.LoggerFrom(ctx, "machine", m.Name)
-
+func (r *MachineSetReconciler) getMachineSetsForMachine(ctx context.Context, m *clusterv1.Machine) ([]*clusterv1.MachineSet, error) {
 	if len(m.Labels) == 0 {
-		log.Info("No machine sets found because it has no labels")
-		return nil
+		return nil, fmt.Errorf("machine %v has no labels", client.ObjectKeyFromObject(m))
 	}
 
 	msList := &clusterv1.MachineSetList{}
-	err := r.Client.List(ctx, msList, client.InNamespace(m.Namespace))
-	if err != nil {
-		log.Error(err, "Failed to list machine sets")
-		return nil
+	if err := r.Client.List(ctx, msList, client.InNamespace(m.Namespace)); err != nil {
+		return nil, fmt.Errorf("failed to list MachineSets: %w", err)
 	}
 
 	var mss []*clusterv1.MachineSet
 	for idx := range msList.Items {
 		ms := &msList.Items[idx]
-		if r.hasMatchingLabels(ctx, ms, m) {
+		if hasMatchingLabels(ms.Spec.Selector, m.Labels) {
 			mss = append(mss, ms)
 		}
 	}
 
-	return mss
-}
-
-func (r *MachineSetReconciler) hasMatchingLabels(ctx context.Context, machineSet *clusterv1.MachineSet, machine *clusterv1.Machine) bool {
-	log := ctrl.LoggerFrom(ctx, "machine", machine.Name)
-
-	selector, err := metav1.LabelSelectorAsSelector(&machineSet.Spec.Selector)
-	if err != nil {
-		log.Error(err, "Unable to convert selector")
-		return false
-	}
-
-	// If a deployment with a nil or empty selector creeps in, it should match nothing, not everything.
-	if selector.Empty() {
-		log.V(2).Info("Machineset has empty selector")
-		return false
-	}
-
-	if !selector.Matches(labels.Set(machine.Labels)) {
-		log.V(4).Info("Machine has mismatch labels")
-		return false
-	}
-
-	return true
+	return mss, nil
 }
 
 func (r *MachineSetReconciler) shouldAdopt(ms *clusterv1.MachineSet) bool {

diff --git a/controllers/machineset_controller_test.go b/controllers/machineset_controller_test.go
@@ -690,108 +690,6 @@ func TestAdoptOrphan(t *testing.T) {
 	}
 }
 
-func TestHasMatchingLabels(t *testing.T) {
-	r := &MachineSetReconciler{}
-
-	testCases := []struct {
-		name       string
-		machineSet clusterv1.MachineSet
-		machine    clusterv1.Machine
-		expected   bool
-	}{
-		{
-			name: "machine set and machine have matching labels",
-			machineSet: clusterv1.MachineSet{
-				Spec: clusterv1.MachineSetSpec{
-					Selector: metav1.LabelSelector{
-						MatchLabels: map[string]string{
-							"foo": "bar",
-						},
-					},
-				},
-			},
-			machine: clusterv1.Machine{
-				ObjectMeta: metav1.ObjectMeta{
-					Name: "matchSelector",
-					Labels: map[string]string{
-						"foo": "bar",
-					},
-				},
-			},
-			expected: true,
-		},
-		{
-			name: "machine set and machine do not have matching labels",
-			machineSet: clusterv1.MachineSet{
-				Spec: clusterv1.MachineSetSpec{
-					Selector: metav1.LabelSelector{
-						MatchLabels: map[string]string{
-							"foo": "bar",
-						},
-					},
-				},
-			},
-			machine: clusterv1.Machine{
-				ObjectMeta: metav1.ObjectMeta{
-					Name: "doesNotMatchSelector",
-					Labels: map[string]string{
-						"no": "match",
-					},
-				},
-			},
-			expected: false,
-		},
-		{
-			name: "machine set has empty selector",
-			machineSet: clusterv1.MachineSet{
-				Spec: clusterv1.MachineSetSpec{
-					Selector: metav1.LabelSelector{},
-				},
-			},
-			machine: clusterv1.Machine{
-				ObjectMeta: metav1.ObjectMeta{
-					Name: "doesNotMatter",
-				},
-			},
-			expected: false,
-		},
-		{
-			name: "machine set has bad selector",
-			machineSet: clusterv1.MachineSet{
-				Spec: clusterv1.MachineSetSpec{
-					Selector: metav1.LabelSelector{
-						MatchLabels: map[string]string{
-							"foo": "bar",
-						},
-						MatchExpressions: []metav1.LabelSelectorRequirement{
-							{
-								Operator: "bad-operator",
-							},
-						},
-					},
-				},
-			},
-			machine: clusterv1.Machine{
-				ObjectMeta: metav1.ObjectMeta{
-					Name: "match",
-					Labels: map[string]string{
-						"foo": "bar",
-					},
-				},
-			},
-			expected: false,
-		},
-	}
-
-	for _, tc := range testCases {
-		t.Run(tc.name, func(t *testing.T) {
-			g := NewWithT(t)
-			got := r.hasMatchingLabels(ctx, &tc.machineSet, &tc.machine)
-			g.Expect(got).To(Equal(tc.expected))
-		})
-	}
-}
-
 func newMachineSet(name, cluster string) *clusterv1.MachineSet {
 	var replicas int32
 	return &clusterv1.MachineSet{