MachineSet: Prefer to delete non-Running machines during scale down

See: https://bugzilla.redhat.com/1903733
openshift · Dec 3, 2020 · 3ceba01 · 3ceba01
1 parent 71aeab4
commit 3ceba01
Show file tree

Hide file tree

Showing 2 changed files with 43 additions and 19 deletions.
diff --git a/pkg/controller/machineset/delete_policy.go b/pkg/controller/machineset/delete_policy.go
@@ -35,6 +35,7 @@ const (
 
 	mustDelete    deletePriority = 100.0
 	betterDelete  deletePriority = 50.0
+	preferDelete  deletePriority = 40.0
 	couldDelete   deletePriority = 20.0
 	mustNotDelete deletePriority = 0.0
 
@@ -87,6 +88,10 @@ func randomDeletePolicy(machine *v1beta1.Machine) deletePriority {
 	if machine.Status.ErrorReason != nil || machine.Status.ErrorMessage != nil {
 		return betterDelete
 	}
+	// The machine doesn't have a Node yet, and therefore isn't running any workloads
+	if machine.Status.NodeRef == nil {
+		return preferDelete
+	}
 	return couldDelete
 }
 

diff --git a/pkg/controller/machineset/delete_policy_test.go b/pkg/controller/machineset/delete_policy_test.go
@@ -21,6 +21,7 @@ import (
 	"testing"
 
 	"github.com/openshift/machine-api-operator/pkg/apis/machine/v1beta1"
+	corev1 "k8s.io/api/core/v1"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 )
 
@@ -30,6 +31,8 @@ func TestMachineToDelete(t *testing.T) {
 	mustDeleteMachine := &v1beta1.Machine{ObjectMeta: metav1.ObjectMeta{DeletionTimestamp: &now}}
 	betterDeleteMachine := &v1beta1.Machine{Status: v1beta1.MachineStatus{ErrorMessage: &msg}}
 	deleteMeMachine := &v1beta1.Machine{ObjectMeta: metav1.ObjectMeta{Annotations: map[string]string{DeleteNodeAnnotation: "yes"}}}
+	runningMachine := &v1beta1.Machine{Status: v1beta1.MachineStatus{NodeRef: &corev1.ObjectReference{}}}
+	notYetRunningMachine := &v1beta1.Machine{}
 
 	tests := []struct {
 		desc     string
@@ -41,38 +44,38 @@ func TestMachineToDelete(t *testing.T) {
 			desc: "func=randomDeletePolicy, diff=0",
 			diff: 0,
 			machines: []*v1beta1.Machine{
-				{},
+				runningMachine,
 			},
 			expect: []*v1beta1.Machine{},
 		},
 		{
 			desc: "func=randomDeletePolicy, diff>len(machines)",
 			diff: 2,
 			machines: []*v1beta1.Machine{
-				{},
+				runningMachine,
 			},
 			expect: []*v1beta1.Machine{
-				{},
+				runningMachine,
 			},
 		},
 		{
 			desc: "func=randomDeletePolicy, diff>betterDelete",
 			diff: 2,
 			machines: []*v1beta1.Machine{
-				{},
+				runningMachine,
 				betterDeleteMachine,
-				{},
+				runningMachine,
 			},
 			expect: []*v1beta1.Machine{
 				betterDeleteMachine,
-				{},
+				runningMachine,
 			},
 		},
 		{
 			desc: "func=randomDeletePolicy, diff<betterDelete",
 			diff: 2,
 			machines: []*v1beta1.Machine{
-				{},
+				runningMachine,
 				betterDeleteMachine,
 				betterDeleteMachine,
 				betterDeleteMachine,
@@ -86,7 +89,7 @@ func TestMachineToDelete(t *testing.T) {
 			desc: "func=randomDeletePolicy, diff<=mustDelete",
 			diff: 2,
 			machines: []*v1beta1.Machine{
-				{},
+				runningMachine,
 				mustDeleteMachine,
 				betterDeleteMachine,
 				mustDeleteMachine,
@@ -100,9 +103,9 @@ func TestMachineToDelete(t *testing.T) {
 			desc: "func=randomDeletePolicy, diff<=mustDelete+betterDelete",
 			diff: 2,
 			machines: []*v1beta1.Machine{
-				{},
+				runningMachine,
 				mustDeleteMachine,
-				{},
+				runningMachine,
 				betterDeleteMachine,
 			},
 			expect: []*v1beta1.Machine{
@@ -114,40 +117,56 @@ func TestMachineToDelete(t *testing.T) {
 			desc: "func=randomDeletePolicy, diff<=mustDelete+betterDelete+couldDelete",
 			diff: 2,
 			machines: []*v1beta1.Machine{
-				{},
+				runningMachine,
 				mustDeleteMachine,
-				{},
+				runningMachine,
 			},
 			expect: []*v1beta1.Machine{
 				mustDeleteMachine,
-				{},
+				runningMachine,
 			},
 		},
 		{
 			desc: "func=randomDeletePolicy, diff>betterDelete",
 			diff: 2,
 			machines: []*v1beta1.Machine{
-				{},
+				runningMachine,
 				betterDeleteMachine,
-				{},
+				runningMachine,
 			},
 			expect: []*v1beta1.Machine{
 				betterDeleteMachine,
-				{},
+				runningMachine,
 			},
 		},
 		{
 			desc: "func=randomDeletePolicy, annotated, diff=1",
 			diff: 1,
 			machines: []*v1beta1.Machine{
-				{},
+				runningMachine,
 				deleteMeMachine,
-				{},
+				runningMachine,
 			},
 			expect: []*v1beta1.Machine{
 				deleteMeMachine,
 			},
-		}}
+		},
+		{
+			desc: "func=randomDeletePolicy, delete non-running hosts first",
+			diff: 3,
+			machines: []*v1beta1.Machine{
+				runningMachine,
+				notYetRunningMachine,
+				deleteMeMachine,
+				betterDeleteMachine,
+			},
+			expect: []*v1beta1.Machine{
+				deleteMeMachine,
+				betterDeleteMachine,
+				notYetRunningMachine,
+			},
+		},
+	}
 
 	for _, test := range tests {
 		result := getMachinesToDeletePrioritized(test.machines, test.diff, randomDeletePolicy)