Actuator patches

pkg/controller/machineset/controller.go

@@ -19,6 +19,7 @@ package machineset
 import (
 	"context"
 	"fmt"
+	"sort"
 	"strings"
 	"sync"
 	"time"
@@ -151,6 +152,12 @@ 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 := &machinev1beta1.MachineList{}
 
 	err = r.Client.List(context.Background(), client.InNamespace(machineSet.Namespace), allMachines)
@@ -159,7 +166,8 @@ func (r *ReconcileMachineSet) Reconcile(request reconcile.Request) (reconcile.Re
 	}
 
 	// Filter out irrelevant machines (deleting/mismatch labels) and claim orphaned machines.
-	var filteredMachines []*machinev1beta1.Machine
+	var machineNames []string
+	machineSetMachines := make(map[string]*machinev1beta1.Machine)
 	for idx := range allMachines.Items {
 		machine := &allMachines.Items[idx]
 		if shouldExcludeMachine(machineSet, machine) {
@@ -172,7 +180,15 @@ func (r *ReconcileMachineSet) Reconcile(request reconcile.Request) (reconcile.Re
 				continue
 			}
 		}
-		filteredMachines = append(filteredMachines, machine)
+		machineNames = append(machineNames, machine.Name)
+		machineSetMachines[machine.Name] = machine
+	}
+	// sort the filteredMachines from the oldest to the youngest
+	sort.Strings(machineNames)
+
+	var filteredMachines []*machinev1beta1.Machine
+	for _, machineName := range machineNames {
+		filteredMachines = append(filteredMachines, machineSetMachines[machineName])
 	}
 
 	syncErr := r.syncReplicas(machineSet, filteredMachines)

pkg/controller/machineset/delete_policy.go

@@ -31,10 +31,17 @@ const (
 
 type deletePriorityFunc func(machine *v1beta1.Machine) deletePriority
 
+// machineDeleteAnnotationKey annotates machines to be delete among first ones
+var machineDeleteAnnotationKey = "machine.openshift.io/cluster-api-delete-machine"
+
 func simpleDeletePriority(machine *v1beta1.Machine) deletePriority {
 	if machine.DeletionTimestamp != nil && !machine.DeletionTimestamp.IsZero() {
 		return mustDelete
 	}
+	if _, exists := machine.Annotations[machineDeleteAnnotationKey]; exists {
+		return mustDelete
+	}
+
 	if machine.Status.ErrorReason != nil || machine.Status.ErrorMessage != nil {
 		return betterDelete
 	}

pkg/controller/machineset/machineset_controller_test.go

@@ -23,7 +23,7 @@ import (
 	"golang.org/x/net/context"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 	"k8s.io/apimachinery/pkg/types"
-	clusterv1beta1 "sigs.k8s.io/cluster-api/pkg/apis/machine/v1beta1"
+	machinev1beta1 "sigs.k8s.io/cluster-api/pkg/apis/machine/v1beta1"
 	"sigs.k8s.io/controller-runtime/pkg/client"
 	"sigs.k8s.io/controller-runtime/pkg/manager"
 	"sigs.k8s.io/controller-runtime/pkg/reconcile"
@@ -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 := &clusterv1beta1.MachineSet{
-		ObjectMeta: metav1.ObjectMeta{Name: "foo", Namespace: "default"},
-		Spec: clusterv1beta1.MachineSetSpec{
-			Replicas: &replicas,
-			Template: clusterv1beta1.MachineTemplateSpec{
-				Spec: clusterv1beta1.MachineSpec{
-					Versions: clusterv1beta1.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        *machinev1beta1.MachineSet
+		expectedRequest reconcile.Request
+		verifyFnc       func()
+	}{
+		{
+			name: "Refuse invalid machineset (with invalid matching labels)",
+			instance: &machinev1beta1.MachineSet{
+				ObjectMeta: metav1.ObjectMeta{Name: "invalidfoo", Namespace: "default"},
+				Spec: machinev1beta1.MachineSetSpec{
+					Replicas: &replicas,
+					Selector: metav1.LabelSelector{
+						MatchLabels: map[string]string{"foo": "bar"},
+					},
+					Template: machinev1beta1.MachineTemplateSpec{
+						ObjectMeta: metav1.ObjectMeta{
+							Labels: map[string]string{"foo": "bar2"},
+						},
+						Spec: machinev1beta1.MachineSpec{
+							Versions: machinev1beta1.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: &machinev1beta1.MachineSet{
+				ObjectMeta: metav1.ObjectMeta{Name: "foo", Namespace: "default"},
+				Spec: machinev1beta1.MachineSetSpec{
+					Replicas: &replicas,
+					Selector: metav1.LabelSelector{
+						MatchLabels: labels,
+					},
+					Template: machinev1beta1.MachineTemplateSpec{
+						ObjectMeta: metav1.ObjectMeta{
+							Labels: labels,
+						},
+						Spec: machinev1beta1.MachineSpec{
+							Versions: machinev1beta1.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 := &machinev1beta1.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 := &clusterv1beta1.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) {