Prefer ready and reachable control plane nodes

Author: ingvagabund

pkg/cli/admin/mustgather/mustgather.go

@@ -10,6 +10,7 @@ import (
 	"os"
 	"path"
 	"regexp"
+	"sort"
 	"strconv"
 	"strings"
 	"sync"
@@ -39,6 +40,7 @@ import (
 	"k8s.io/kubectl/pkg/util/templates"
 	admissionapi "k8s.io/pod-security-admission/api"
 	"k8s.io/utils/exec"
+	utilptr "k8s.io/utils/ptr"
 
 	configclient "github.com/openshift/client-go/config/clientset/versioned"
 	imagev1client "github.com/openshift/client-go/image/clientset/versioned/typed/image/v1"
@@ -51,9 +53,10 @@ import (
 )
 
 const (
-	gatherContainerName = "gather"
-	unreachableTaintKey = "node.kubernetes.io/unreachable"
-	notReadyTaintKey    = "node.kubernetes.io/not-ready"
+	gatherContainerName       = "gather"
+	unreachableTaintKey       = "node.kubernetes.io/unreachable"
+	notReadyTaintKey          = "node.kubernetes.io/not-ready"
+	controlPlaneNodeRoleLabel = "node-role.kubernetes.io/control-plane"
 )
 
 var (
@@ -101,23 +104,9 @@ sleep 5
 done`
 )
 
-var (
-	tolerationNotReady = corev1.Toleration{
-		Key:      "node.kubernetes.io/not-ready",
-		Operator: corev1.TolerationOpExists,
-		Effect:   corev1.TaintEffectNoSchedule,
-	}
-
-	tolerationMasterNoSchedule = corev1.Toleration{
-		Key:      "node-role.kubernetes.io/master",
-		Operator: corev1.TolerationOpExists,
-		Effect:   corev1.TaintEffectNoSchedule,
-	}
-)
-
-// buildNodeSelector builds a node selector from the provided node names.
-func buildNodeAffinity(nodeNames []string) *corev1.Affinity {
-	if len(nodeNames) == 0 {
+// buildNodeAffinity builds a node affinity from the provided node hostnames.
+func buildNodeAffinity(nodeHostnames []string) *corev1.Affinity {
+	if len(nodeHostnames) == 0 {
 		return nil
 	}
 	return &corev1.Affinity{
@@ -129,7 +118,7 @@ func buildNodeAffinity(nodeNames []string) *corev1.Affinity {
 							{
 								Key:      "kubernetes.io/hostname",
 								Operator: corev1.NodeSelectorOpIn,
-								Values:   nodeNames,
+								Values:   nodeHostnames,
 							},
 						},
 					},
@@ -444,40 +433,124 @@ func (o *MustGatherOptions) Validate() error {
 	return nil
 }
 
-// prioritizeHealthyNodes returns a preferred node to run the must-gather pod on, and a fallback node if no preferred node is found.
-func prioritizeHealthyNodes(nodes *corev1.NodeList) (preferred *corev1.Node, fallback *corev1.Node) {
-	var fallbackNode *corev1.Node
-	var latestHeartbeat time.Time
+func getNodeLastHeartbeatTime(node corev1.Node) *metav1.Time {
+	for _, cond := range node.Status.Conditions {
+		if cond.Type == corev1.NodeReady {
+			if !cond.LastHeartbeatTime.IsZero() {
+				return utilptr.To[metav1.Time](cond.LastHeartbeatTime)
+			}
+			return nil
+		}
+	}
+	return nil
+}
+
+func isNodeReadyByCondition(node corev1.Node) bool {
+	for _, cond := range node.Status.Conditions {
+		if cond.Type == corev1.NodeReady && cond.Status == corev1.ConditionTrue {
+			return true
+		}
+	}
+	return false
+}
+
+func isNodeReadyAndReachableByTaint(node corev1.Node) bool {
+	for _, taint := range node.Spec.Taints {
+		if taint.Key == unreachableTaintKey || taint.Key == notReadyTaintKey {
+			return false
+		}
+	}
+	return true
+}
 
+// getCandidateNodeNames identifies suitable nodes for constructing a list of
+// node names for a node affinity expression.
+func getCandidateNodeNames(nodes *corev1.NodeList, hasMaster bool) []string {
+	// Identify ready and reacheable nodes
+	var controlPlaneNodes, allControlPlaneNodes, workerNodes, unschedulableNodes, remainingNodes, selectedNodes []corev1.Node
 	for _, node := range nodes.Items {
-		var hasUnhealthyTaint bool
-		for _, taint := range node.Spec.Taints {
-			if taint.Key == unreachableTaintKey || taint.Key == notReadyTaintKey {
-				hasUnhealthyTaint = true
-				break
-			}
+		if _, ok := node.Labels[controlPlaneNodeRoleLabel]; ok {
+			allControlPlaneNodes = append(allControlPlaneNodes, node)
+		}
+		if !isNodeReadyByCondition(node) || !isNodeReadyAndReachableByTaint(node) {
+			remainingNodes = append(remainingNodes, node)
+			continue
+		}
+		if node.Spec.Unschedulable {
+			unschedulableNodes = append(unschedulableNodes, node)
+			continue
+		}
+		if _, ok := node.Labels[controlPlaneNodeRoleLabel]; ok {
+			controlPlaneNodes = append(controlPlaneNodes, node)
+		} else {
+			workerNodes = append(workerNodes, node)
 		}
+	}
 
-		// Look at heartbeat time for readiness hint
-		for _, cond := range node.Status.Conditions {
-			if cond.Type == corev1.NodeReady && cond.Status == corev1.ConditionTrue {
-				// Check if heartbeat is recent (less than 2m old)
-				if time.Since(cond.LastHeartbeatTime.Time) < 2*time.Minute {
-					if !hasUnhealthyTaint {
-						return &node, nil // return immediately on good candidate
-					}
-				}
-				break
-			}
+	// INFO(ingvagabund): a single target node should be enough. Yet,
+	// it might help to provide more to allow the scheduler choose a more
+	// suitable node based on the cluster scheduling capabilities.
+
+	// hasMaster cause the must-gather pod to set a node selector targeting all
+	// control plane node. So there's no point of processing other than control
+	// plane nodes
+	if hasMaster {
+		if len(controlPlaneNodes) > 0 {
+			selectedNodes = controlPlaneNodes
+		} else {
+			selectedNodes = allControlPlaneNodes
+		}
+	} else {
+		// For hypershift case
+
+		// Order of preference:
+		// - ready and reachable control plane nodes first
+		// - then ready and reachable worker nodes
+		// - then ready and reachable unschedulable nodes
+		// - then any other node
+		selectedNodes = controlPlaneNodes // this will be very likely empty for hypershift
+		if len(selectedNodes) == 0 {
+			selectedNodes = workerNodes
+		}
+		if len(selectedNodes) == 0 {
+			// unschedulable nodes might still be better candidates than the remaining
+			// not ready and not reacheable nodes
+			selectedNodes = unschedulableNodes
+		}
+		if len(selectedNodes) == 0 {
+			// whatever is left for the last resort
+			selectedNodes = remainingNodes
+		}
+	}
+
+	// Sort nodes based on the cond.LastHeartbeatTime.Time to prefer nodes that
+	// reported their status most recently
+	sort.SliceStable(selectedNodes, func(i, j int) bool {
+		iTime := getNodeLastHeartbeatTime(selectedNodes[i])
+		jTime := getNodeLastHeartbeatTime(selectedNodes[j])
+		// iTime has no effect since all is sorted right
+		if jTime == nil {
+			return true
+		}
+		if iTime == nil {
+			return false
 		}
+		return jTime.Before(iTime)
+	})
 
-		if fallbackNode == nil || node.CreationTimestamp.Time.After(latestHeartbeat) {
-			fallbackNode = &node
-			latestHeartbeat = node.CreationTimestamp.Time
+	// Limit the number of nodes to 10 at most (choosen randomly) to provide a sane
+	// list of nodes
+	nodeNames := []string{}
+	var nodeNamesSize = 0
+	for _, n := range selectedNodes {
+		nodeNames = append(nodeNames, n.Name)
+		nodeNamesSize++
+		if nodeNamesSize >= 10 {
+			break
 		}
 	}
 
-	return nil, fallbackNode
+	return nodeNames
 }
 
 // Run creates and runs a must-gather pod
@@ -536,59 +609,25 @@ func (o *MustGatherOptions) Run() error {
 		defer cleanupNamespace()
 	}
 
-	nodes, err := o.Client.CoreV1().Nodes().List(context.TODO(), metav1.ListOptions{})
+	// Prefer to run in master if there's any but don't be explicit otherwise.
+	// This enables the command to run by default in hypershift where there's no masters.
+	nodes, err := o.Client.CoreV1().Nodes().List(context.TODO(), metav1.ListOptions{
+		LabelSelector: o.NodeSelector,
+	})
 	if err != nil {
 		return err
 	}
 
-	var cpNodes, workerNodes, unschedulableNodes []corev1.Node
-	for _, node := range nodes.Items {
-		isReady := false
-		for _, cond := range node.Status.Conditions {
-			if cond.Type == corev1.NodeReady && cond.Status == corev1.ConditionTrue {
-				isReady = true
-				break
-			}
-		}
-		if !isReady {
-			continue
-		}
-		if node.Spec.Unschedulable {
-			unschedulableNodes = append(unschedulableNodes, node)
-			continue
-		}
-		if _, ok := node.Labels["node-role.kubernetes.io/control-plane"]; ok {
-			cpNodes = append(cpNodes, node)
-		} else {
-			workerNodes = append(workerNodes, node)
-		}
-	}
-
-	selectedNodes := cpNodes
-	if len(selectedNodes) == 0 {
-		selectedNodes = workerNodes
-	}
-	if len(selectedNodes) == 0 {
-		selectedNodes = unschedulableNodes
-	}
-
-	var nodeNames []string
-	for _, n := range selectedNodes {
-		nodeNames = append(nodeNames, n.Name)
-	}
-
-	affinity := buildNodeAffinity(nodeNames)
-	// If we have no nodes, we cannot run the must-gather pod.
-
-	// Determine if there is a master node
-	hasMaster := false
+	var hasMaster bool
 	for _, node := range nodes.Items {
 		if _, ok := node.Labels["node-role.kubernetes.io/master"]; ok {
 			hasMaster = true
 			break
 		}
 	}
 
+	affinity := buildNodeAffinity(getCandidateNodeNames(nodes, hasMaster))
+
 	// ... and create must-gather pod(s)
 	var pods []*corev1.Pod
 	for _, image := range o.Images {
@@ -1057,31 +1096,6 @@ func (o *MustGatherOptions) newPod(node, image string, hasMaster bool, affinity
 	cleanedSourceDir := path.Clean(o.SourceDir)
 	volumeUsageChecker := fmt.Sprintf(volumeUsageCheckerScript, cleanedSourceDir, cleanedSourceDir, o.VolumePercentage, o.VolumePercentage, executedCommand)
 
-	// Define taints we do not want to tolerate (can be extended with user input in the future)
-	excludedTaints := []corev1.Taint{
-		{Key: unreachableTaintKey, Effect: corev1.TaintEffectNoExecute},
-		{Key: notReadyTaintKey, Effect: corev1.TaintEffectNoSchedule},
-	}
-
-	// Define candidate tolerations we may want to apply
-	candidateTolerations := []corev1.Toleration{tolerationNotReady}
-	if node == "" && hasMaster {
-		candidateTolerations = append(candidateTolerations, tolerationMasterNoSchedule)
-	}
-
-	// Build the toleration list by only adding tolerations that do NOT tolerate excluded taints
-	filteredTolerations := make([]corev1.Toleration, 0, len(candidateTolerations))
-TolerationLoop:
-	for _, tol := range candidateTolerations {
-		for _, excluded := range excludedTaints {
-			if tol.ToleratesTaint(&excluded) {
-				// Skip this toleration if it tolerates an excluded taint
-				continue TolerationLoop
-			}
-		}
-		filteredTolerations = append(filteredTolerations, tol)
-	}
-
 	ret := &corev1.Pod{
 		ObjectMeta: metav1.ObjectMeta{
 			GenerateName: "must-gather-",