Merge pull request #7841 from pmendelski/force-ds-fix2

Force system-node-critical daemon-sets in node group templates

Author: k8s-ci-robot

cluster-autoscaler/processors/nodeinfosprovider/mixed_nodeinfos_processor.go

@@ -184,5 +184,12 @@ func isNodeGoodTemplateCandidate(node *apiv1.Node, now time.Time) bool {
 	ready, lastTransitionTime, _ := kube_util.GetReadinessState(node)
 	stable := lastTransitionTime.Add(stabilizationDelay).Before(now)
 	schedulable := !node.Spec.Unschedulable
-	return ready && stable && schedulable
+	toBeDeleted := false
+	for _, taint := range node.Spec.Taints {
+		if taint.Key == taints.ToBeDeletedTaint {
+			toBeDeleted = true
+			break
+		}
+	}
+	return ready && stable && schedulable && !toBeDeleted
 }

cluster-autoscaler/processors/nodeinfosprovider/mixed_nodeinfos_processor_test.go

@@ -17,6 +17,7 @@ limitations under the License.
 package nodeinfosprovider
 
 import (
+	"fmt"
 	"testing"
 	"time"
 
@@ -50,14 +51,19 @@ func TestGetNodeInfosForGroups(t *testing.T) {
 	SetNodeReadyState(unready4, false, now)
 	justReady5 := BuildTestNode("n5", 5000, 5000)
 	SetNodeReadyState(justReady5, true, now)
+	readyToBeDeleted6 := BuildTestNode("n6", 2000, 2000)
+	SetNodeReadyState(readyToBeDeleted6, true, now.Add(-2*time.Minute))
+	setToBeDeletedTaint(readyToBeDeleted6)
+	ready7 := BuildTestNode("n7", 6000, 6000)
+	SetNodeReadyState(ready7, true, now.Add(-2*time.Minute))
 
 	tn := BuildTestNode("tn", 5000, 5000)
 	tni := framework.NewTestNodeInfo(tn)
 
 	// Cloud provider with TemplateNodeInfo implemented.
 	provider1 := testprovider.NewTestAutoprovisioningCloudProvider(
 		nil, nil, nil, nil, nil,
-		map[string]*framework.NodeInfo{"ng3": tni, "ng4": tni, "ng5": tni})
+		map[string]*framework.NodeInfo{"ng3": tni, "ng4": tni, "ng5": tni, "ng6": tni})
 	provider1.AddNodeGroup("ng1", 1, 10, 1) // Nodegroup with ready node.
 	provider1.AddNode("ng1", ready1)
 	provider1.AddNodeGroup("ng2", 1, 10, 1) // Nodegroup with ready and unready node.
@@ -68,15 +74,18 @@ func TestGetNodeInfosForGroups(t *testing.T) {
 	provider1.AddNodeGroup("ng4", 0, 1000, 0) // Nodegroup without nodes.
 	provider1.AddNodeGroup("ng5", 1, 10, 1)   // Nodegroup with node that recently became ready.
 	provider1.AddNode("ng5", justReady5)
+	provider1.AddNodeGroup("ng6", 1, 10, 1) // Nodegroup with to be deleted node
+	provider1.AddNode("ng6", readyToBeDeleted6)
+	provider1.AddNode("ng6", ready7)
 
 	// Cloud provider with TemplateNodeInfo not implemented.
 	provider2 := testprovider.NewTestAutoprovisioningCloudProvider(nil, nil, nil, nil, nil, nil)
-	provider2.AddNodeGroup("ng6", 1, 10, 1) // Nodegroup without nodes.
+	provider2.AddNodeGroup("ng7", 1, 10, 1) // Nodegroup without nodes.
 
 	podLister := kube_util.NewTestPodLister([]*apiv1.Pod{})
 	registry := kube_util.NewListerRegistry(nil, nil, podLister, nil, nil, nil, nil, nil, nil)
 
-	nodes := []*apiv1.Node{justReady5, unready4, unready3, ready2, ready1}
+	nodes := []*apiv1.Node{justReady5, unready4, unready3, ready2, ready1, ready7, readyToBeDeleted6}
 	snapshot := testsnapshot.NewTestSnapshotOrDie(t)
 	err := snapshot.SetClusterState(nodes, nil, drasnapshot.Snapshot{})
 	assert.NoError(t, err)
@@ -90,7 +99,7 @@ func TestGetNodeInfosForGroups(t *testing.T) {
 	}
 	res, err := NewMixedTemplateNodeInfoProvider(&cacheTtl, false).Process(&ctx, nodes, []*appsv1.DaemonSet{}, taints.TaintConfig{}, now)
 	assert.NoError(t, err)
-	assert.Equal(t, 5, len(res))
+	assert.Equal(t, 6, len(res))
 	info, found := res["ng1"]
 	assert.True(t, found)
 	assertEqualNodeCapacities(t, ready1, info.Node())
@@ -106,6 +115,9 @@ func TestGetNodeInfosForGroups(t *testing.T) {
 	info, found = res["ng5"]
 	assert.True(t, found)
 	assertEqualNodeCapacities(t, tn, info.Node())
+	info, found = res["ng6"]
+	assert.True(t, found)
+	assertEqualNodeCapacities(t, ready7, info.Node())
 
 	// Test for a nodegroup without nodes and TemplateNodeInfo not implemented by cloud proivder
 	ctx = context.AutoscalingContext{
@@ -314,3 +326,11 @@ func getNodeResource(node *apiv1.Node, resource apiv1.ResourceName) int64 {
 
 	return nodeCapacityValue
 }
+
+func setToBeDeletedTaint(node *apiv1.Node) {
+	node.Spec.Taints = append(node.Spec.Taints, apiv1.Taint{
+		Key:    taints.ToBeDeletedTaint,
+		Value:  fmt.Sprint(time.Now().Unix()),
+		Effect: apiv1.TaintEffectNoSchedule,
+	})
+}

cluster-autoscaler/simulator/node_info_utils.go

@@ -148,23 +148,34 @@ func podsExpectedOnFreshNode(sanitizedExampleNodeInfo *framework.NodeInfo, daemo
 	// TODO(DRA): Figure out how to make this work for DS pods using DRA. Currently such pods would get force-added to the
 	// ClusterSnapshot, but the ResourceClaims reflecting their DRA usage on the Node wouldn't. So CA would be overestimating
 	// available DRA resources on the Node.
-	if forceDaemonSets {
-		var pendingDS []*appsv1.DaemonSet
-		for _, ds := range daemonsets {
-			if !runningDS[ds.UID] {
-				pendingDS = append(pendingDS, ds)
-			}
+	var pendingDS []*appsv1.DaemonSet
+	for _, ds := range daemonsets {
+		if !runningDS[ds.UID] {
+			pendingDS = append(pendingDS, ds)
 		}
-		// The provided nodeInfo has to have taints properly sanitized, or this won't work correctly.
-		daemonPods, err := daemonset.GetDaemonSetPodsForNode(sanitizedExampleNodeInfo, pendingDS)
-		if err != nil {
-			return nil, err
-		}
-		for _, pod := range daemonPods {
-			// There's technically no need to sanitize these pods since they're created from scratch, but
-			// it's nice to have the same suffix for all names in one sanitized NodeInfo when debugging.
-			result = append(result, &framework.PodInfo{Pod: createSanitizedPod(pod.Pod, sanitizedExampleNodeInfo.Node().Name, nameSuffix)})
+	}
+	// The provided nodeInfo has to have taints properly sanitized, or this won't work correctly.
+	daemonPods, err := daemonset.GetDaemonSetPodsForNode(sanitizedExampleNodeInfo, pendingDS)
+	if err != nil {
+		return nil, err
+	}
+	for _, pod := range daemonPods {
+		if !forceDaemonSets && !isPreemptingSystemNodeCritical(pod) {
+			continue
 		}
+		// There's technically no need to sanitize these pods since they're created from scratch, but
+		// it's nice to have the same suffix for all names in one sanitized NodeInfo when debugging.
+		result = append(result, &framework.PodInfo{Pod: createSanitizedPod(pod.Pod, sanitizedExampleNodeInfo.Node().Name, nameSuffix)})
 	}
 	return result, nil
 }
+
+func isPreemptingSystemNodeCritical(pod *framework.PodInfo) bool {
+	if pod.Spec.PriorityClassName != labels.SystemNodeCriticalLabel {
+		return false
+	}
+	if pod.Spec.PreemptionPolicy != nil && *pod.Spec.PreemptionPolicy != apiv1.PreemptLowerPriority {
+		return false
+	}
+	return true
+}

cluster-autoscaler/simulator/node_info_utils_test.go

@@ -31,13 +31,15 @@ import (
 	resourceapi "k8s.io/api/resource/v1beta1"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 	"k8s.io/apimachinery/pkg/types"
+	"k8s.io/kubernetes/pkg/controller/daemon"
+
 	"k8s.io/autoscaler/cluster-autoscaler/config"
 	drautils "k8s.io/autoscaler/cluster-autoscaler/simulator/dynamicresources/utils"
 	"k8s.io/autoscaler/cluster-autoscaler/simulator/framework"
 	"k8s.io/autoscaler/cluster-autoscaler/utils/errors"
+	"k8s.io/autoscaler/cluster-autoscaler/utils/labels"
 	"k8s.io/autoscaler/cluster-autoscaler/utils/taints"
 	. "k8s.io/autoscaler/cluster-autoscaler/utils/test"
-	"k8s.io/kubernetes/pkg/controller/daemon"
 )
 
 var (
@@ -69,7 +71,21 @@ var (
 			},
 		},
 	}
-	testDaemonSets = []*appsv1.DaemonSet{ds1, ds2, ds3}
+	ds4 = &appsv1.DaemonSet{
+		ObjectMeta: metav1.ObjectMeta{
+			Name:      "ds4",
+			Namespace: "ds4-namespace",
+			UID:       types.UID("ds4"),
+		},
+		Spec: appsv1.DaemonSetSpec{
+			Template: apiv1.PodTemplateSpec{
+				Spec: apiv1.PodSpec{
+					PriorityClassName: labels.SystemNodeCriticalLabel,
+				},
+			},
+		},
+	}
+	testDaemonSets = []*appsv1.DaemonSet{ds1, ds2, ds3, ds4}
 )
 
 func TestSanitizedTemplateNodeInfoFromNodeGroup(t *testing.T) {
@@ -98,6 +114,7 @@ func TestSanitizedTemplateNodeInfoFromNodeGroup(t *testing.T) {
 			wantPods: []*apiv1.Pod{
 				buildDSPod(ds1, "n"),
 				buildDSPod(ds2, "n"),
+				buildDSPod(ds4, "n"),
 			},
 		},
 		{
@@ -116,6 +133,7 @@ func TestSanitizedTemplateNodeInfoFromNodeGroup(t *testing.T) {
 				SetMirrorPodSpec(BuildScheduledTestPod("p3", 100, 1, "n")),
 				buildDSPod(ds1, "n"),
 				buildDSPod(ds2, "n"),
+				buildDSPod(ds4, "n"),
 			},
 		},
 	} {
@@ -208,6 +226,7 @@ func TestSanitizedTemplateNodeInfoFromNodeInfo(t *testing.T) {
 			daemonSets: testDaemonSets,
 			wantPods: []*apiv1.Pod{
 				buildDSPod(ds1, "n"),
+				buildDSPod(ds4, "n"),
 			},
 		},
 		{
@@ -232,6 +251,7 @@ func TestSanitizedTemplateNodeInfoFromNodeInfo(t *testing.T) {
 			wantPods: []*apiv1.Pod{
 				buildDSPod(ds1, "n"),
 				buildDSPod(ds2, "n"),
+				buildDSPod(ds4, "n"),
 			},
 		},
 		{
@@ -248,6 +268,7 @@ func TestSanitizedTemplateNodeInfoFromNodeInfo(t *testing.T) {
 			wantPods: []*apiv1.Pod{
 				SetMirrorPodSpec(BuildScheduledTestPod("p3", 100, 1, "n")),
 				buildDSPod(ds1, "n"),
+				buildDSPod(ds4, "n"),
 			},
 		},
 		{
@@ -266,6 +287,7 @@ func TestSanitizedTemplateNodeInfoFromNodeInfo(t *testing.T) {
 				SetMirrorPodSpec(BuildScheduledTestPod("p3", 100, 1, "n")),
 				buildDSPod(ds1, "n"),
 				buildDSPod(ds2, "n"),
+				buildDSPod(ds4, "n"),
 			},
 		},
 	}

cluster-autoscaler/utils/labels/labels.go

@@ -25,6 +25,11 @@ import (
 	"k8s.io/apimachinery/pkg/api/resource"
 )
 
+const (
+	// SystemNodeCriticalLabel is a label that marks critical pods with the highest priority.
+	SystemNodeCriticalLabel = "system-node-critical"
+)
+
 var (
 	// cpu amount used for account pods that don't specify cpu requests
 	defaultMinCPU        = *resource.NewMilliQuantity(50, resource.DecimalSI)