Validate machineset before reconciling

pkg/controller/machineset/controller.go

@@ -150,6 +150,13 @@ func (r *ReconcileMachineSet) Reconcile(request reconcile.Request) (reconcile.Re
 	}
 
 	klog.V(4).Infof("Reconcile machineset %v", machineSet.Name)
+
+	if errList := machineSet.Validate(); len(errList) > 0 {
+		err := fmt.Errorf("%q machineset validation failed: %v", machineSet.Name, errList.ToAggregate().Error())
+		klog.Error(err)
+		return reconcile.Result{}, err
+	}
+
 	allMachines := &clusterv1alpha1.MachineList{}
 
 	err = r.Client.List(context.Background(), client.InNamespace(machineSet.Namespace), allMachines)

pkg/controller/machineset/machineset_controller_test.go

@@ -36,19 +36,6 @@ var expectedRequest = reconcile.Request{NamespacedName: types.NamespacedName{Nam
 const timeout = time.Second * 5
 
 func TestReconcile(t *testing.T) {
-	replicas := int32(2)
-	instance := &clusterv1alpha1.MachineSet{
-		ObjectMeta: metav1.ObjectMeta{Name: "foo", Namespace: "default"},
-		Spec: clusterv1alpha1.MachineSetSpec{
-			Replicas: &replicas,
-			Template: clusterv1alpha1.MachineTemplateSpec{
-				Spec: clusterv1alpha1.MachineSpec{
-					Versions: clusterv1alpha1.MachineVersionInfo{Kubelet: "1.10.3"},
-				},
-			},
-		},
-	}
-
 	// Setup the Manager and Controller.  Wrap the Controller Reconcile function so it writes each request to a
 	// channel when it is finished.
 	mgr, err := manager.New(cfg, manager.Options{})
@@ -64,61 +51,142 @@ func TestReconcile(t *testing.T) {
 	}
 	defer close(StartTestManager(mgr, t))
 
-	// Create the MachineSet object and expect Reconcile to be called and the Machines to be created.
-	if err := c.Create(context.TODO(), instance); err != nil {
-		t.Errorf("error creating instance: %v", err)
-	}
-	defer c.Delete(context.TODO(), instance)
-	select {
-	case recv := <-requests:
-		if recv != expectedRequest {
-			t.Error("received request does not match expected request")
-		}
-	case <-time.After(timeout):
-		t.Error("timed out waiting for request")
+	replicas := int32(2)
+	labels := map[string]string{"foo": "bar"}
+
+	testCases := []struct {
+		name            string
+		instance        *clusterv1alpha1.MachineSet
+		expectedRequest reconcile.Request
+		verifyFnc       func()
+	}{
+		{
+			name: "Refuse invalid machineset (with invalid matching labels)",
+			instance: &clusterv1alpha1.MachineSet{
+				ObjectMeta: metav1.ObjectMeta{Name: "invalidfoo", Namespace: "default"},
+				Spec: clusterv1alpha1.MachineSetSpec{
+					Replicas: &replicas,
+					Selector: metav1.LabelSelector{
+						MatchLabels: map[string]string{"foo": "bar"},
+					},
+					Template: clusterv1alpha1.MachineTemplateSpec{
+						ObjectMeta: metav1.ObjectMeta{
+							Labels: map[string]string{"foo": "bar2"},
+						},
+						Spec: clusterv1alpha1.MachineSpec{
+							Versions: clusterv1alpha1.MachineVersionInfo{Kubelet: "1.10.3"},
+						},
+					},
+				},
+			},
+			expectedRequest: reconcile.Request{NamespacedName: types.NamespacedName{Name: "invalidfoo", Namespace: "default"}},
+			verifyFnc: func() {
+				// expecting machineset validation error
+				if _, err := r.Reconcile(reconcile.Request{
+					NamespacedName: types.NamespacedName{Name: "invalidfoo", Namespace: "default"},
+				}); err == nil {
+					t.Errorf("expected validation error did not occur")
+				}
+			},
+		},
+		{
+			name: "Create the MachineSet object and expect Reconcile to be called and the Machines to be created",
+			instance: &clusterv1alpha1.MachineSet{
+				ObjectMeta: metav1.ObjectMeta{Name: "foo", Namespace: "default"},
+				Spec: clusterv1alpha1.MachineSetSpec{
+					Replicas: &replicas,
+					Selector: metav1.LabelSelector{
+						MatchLabels: labels,
+					},
+					Template: clusterv1alpha1.MachineTemplateSpec{
+						ObjectMeta: metav1.ObjectMeta{
+							Labels: labels,
+						},
+						Spec: clusterv1alpha1.MachineSpec{
+							Versions: clusterv1alpha1.MachineVersionInfo{Kubelet: "1.10.3"},
+						},
+					},
+				},
+			},
+			expectedRequest: reconcile.Request{NamespacedName: types.NamespacedName{Name: "foo", Namespace: "default"}},
+			// Verify machines are created and recreated after deletion
+			verifyFnc: func() {
+				machines := &clusterv1alpha1.MachineList{}
+
+				// TODO(joshuarubin) there seems to be a race here. If expectInt sleeps
+				// briefly, even 10ms, the number of replicas is 4 and not 2 as expected
+				expectInt(t, int(replicas), func(ctx context.Context) int {
+					if err := c.List(ctx, &client.ListOptions{}, machines); err != nil {
+						return -1
+					}
+					return len(machines.Items)
+				})
+
+				// Verify that each machine has the desired kubelet version.
+				for _, m := range machines.Items {
+					if k := m.Spec.Versions.Kubelet; k != "1.10.3" {
+						t.Errorf("kubelet was %q not '1.10.3'", k)
+					}
+				}
+
+				// Delete a Machine and expect Reconcile to be called to replace it.
+				m := machines.Items[0]
+				if err := c.Delete(context.TODO(), &m); err != nil {
+					t.Errorf("error deleting machine: %v", err)
+				}
+				select {
+				case recv := <-requests:
+					if recv != expectedRequest {
+						t.Error("received request does not match expected request")
+					}
+				case <-time.After(timeout):
+					t.Error("timed out waiting for request")
+				}
+
+				// TODO (robertbailey): Figure out why the control loop isn't working as expected.
+				/*
+					g.Eventually(func() int {
+						if err := c.List(context.TODO(), &client.ListOptions{}, machines); err != nil {
+							return -1
+						}
+						return len(machines.Items)
+					}, timeout).Should(gomega.BeEquivalentTo(replicas))
+				*/
+			},
+		},
 	}
 
-	machines := &clusterv1alpha1.MachineList{}
-
-	// TODO(joshuarubin) there seems to be a race here. If expectInt sleeps
-	// briefly, even 10ms, the number of replicas is 4 and not 2 as expected
-	expectInt(t, int(replicas), func(ctx context.Context) int {
-		if err := c.List(ctx, &client.ListOptions{}, machines); err != nil {
-			return -1
-		}
-		return len(machines.Items)
-	})
+	for _, tc := range testCases {
+		t.Logf("Running %q testcase", tc.name)
+		func() {
+			if err := c.Create(context.TODO(), tc.instance); err != nil {
+				t.Errorf("error creating instance: %v", err)
+			}
 
-	// Verify that each machine has the desired kubelet version.
-	for _, m := range machines.Items {
-		if k := m.Spec.Versions.Kubelet; k != "1.10.3" {
-			t.Errorf("kubelet was %q not '1.10.3'", k)
-		}
-	}
+			defer func() {
+				c.Delete(context.TODO(), tc.instance)
+				select {
+				case recv := <-requests:
+					if recv != tc.expectedRequest {
+						t.Error("received request does not match expected request")
+					}
+				case <-time.After(timeout):
+					t.Error("timed out waiting for request")
+				}
+			}()
+
+			select {
+			case recv := <-requests:
+				if recv != tc.expectedRequest {
+					t.Error("received request does not match expected request")
+				}
+			case <-time.After(timeout):
+				t.Error("timed out waiting for request")
+			}
 
-	// Delete a Machine and expect Reconcile to be called to replace it.
-	m := machines.Items[0]
-	if err := c.Delete(context.TODO(), &m); err != nil {
-		t.Errorf("error deleting machine: %v", err)
+			tc.verifyFnc()
+		}()
 	}
-	select {
-	case recv := <-requests:
-		if recv != expectedRequest {
-			t.Error("received request does not match expected request")
-		}
-	case <-time.After(timeout):
-		t.Error("timed out waiting for request")
-	}
-
-	// TODO (robertbailey): Figure out why the control loop isn't working as expected.
-	/*
-		g.Eventually(func() int {
-			if err := c.List(context.TODO(), &client.ListOptions{}, machines); err != nil {
-				return -1
-			}
-			return len(machines.Items)
-		}, timeout).Should(gomega.BeEquivalentTo(replicas))
-	*/
 }
 
 func expectInt(t *testing.T, expect int, fn func(context.Context) int) {