cleanup: use string instead of v1.Node as key of nodeToVictims

Author: Huang-Wei

pkg/scheduler/core/extender.go

@@ -69,14 +69,13 @@ type SchedulerExtender interface {
 	// given:
 	//   1. Pod to schedule
 	//   2. Candidate nodes and victim pods (nodeToVictims) generated by previous scheduling process.
-	//   3. nodeNameToInfo to restore v1.Node from node name if extender cache is enabled.
 	// The possible changes made by extender may include:
 	//   1. Subset of given candidate nodes after preemption phase of extender.
 	//   2. A different set of victim pod for every given candidate node after preemption phase of extender.
 	ProcessPreemption(
 		pod *v1.Pod,
-		nodeToVictims map[*v1.Node]*extenderv1.Victims,
-		nodeInfos framework.NodeInfoLister) (map[*v1.Node]*extenderv1.Victims, error)
+		nodeToVictims map[string]*extenderv1.Victims,
+		nodeInfos framework.NodeInfoLister) (map[string]*extenderv1.Victims, error)
 
 	// SupportsPreemption returns if the scheduler extender support preemption or not.
 	SupportsPreemption() bool
@@ -212,9 +211,9 @@ func (h *HTTPExtender) SupportsPreemption() bool {
 // ProcessPreemption returns filtered candidate nodes and victims after running preemption logic in extender.
 func (h *HTTPExtender) ProcessPreemption(
 	pod *v1.Pod,
-	nodeToVictims map[*v1.Node]*extenderv1.Victims,
+	nodeNameToVictims map[string]*extenderv1.Victims,
 	nodeInfos framework.NodeInfoLister,
-) (map[*v1.Node]*extenderv1.Victims, error) {
+) (map[string]*extenderv1.Victims, error) {
 	var (
 		result extenderv1.ExtenderPreemptionResult
 		args   *extenderv1.ExtenderPreemptionArgs
@@ -226,13 +225,12 @@ func (h *HTTPExtender) ProcessPreemption(
 
 	if h.nodeCacheCapable {
 		// If extender has cached node info, pass NodeNameToMetaVictims in args.
-		nodeNameToMetaVictims := convertToNodeNameToMetaVictims(nodeToVictims)
+		nodeNameToMetaVictims := convertToNodeNameToMetaVictims(nodeNameToVictims)
 		args = &extenderv1.ExtenderPreemptionArgs{
 			Pod:                   pod,
 			NodeNameToMetaVictims: nodeNameToMetaVictims,
 		}
 	} else {
-		nodeNameToVictims := convertToNodeNameToVictims(nodeToVictims)
 		args = &extenderv1.ExtenderPreemptionArgs{
 			Pod:               pod,
 			NodeNameToVictims: nodeNameToVictims,
@@ -244,22 +242,22 @@ func (h *HTTPExtender) ProcessPreemption(
 	}
 
 	// Extender will always return NodeNameToMetaVictims.
-	// So let's convert it to NodeToVictims by using NodeNameToInfo.
-	newNodeToVictims, err := h.convertToNodeToVictims(result.NodeNameToMetaVictims, nodeInfos)
+	// So let's convert it to NodeNameToVictims by using <nodeInfos>.
+	newNodeNameToVictims, err := h.convertToNodeNameToVictims(result.NodeNameToMetaVictims, nodeInfos)
 	if err != nil {
 		return nil, err
 	}
-	// Do not override nodeToVictims
-	return newNodeToVictims, nil
+	// Do not override <nodeNameToVictims>.
+	return newNodeNameToVictims, nil
 }
 
-// convertToNodeToVictims converts "nodeNameToMetaVictims" from object identifiers,
+// convertToNodeNameToVictims converts "nodeNameToMetaVictims" from object identifiers,
 // such as UIDs and names, to object pointers.
-func (h *HTTPExtender) convertToNodeToVictims(
+func (h *HTTPExtender) convertToNodeNameToVictims(
 	nodeNameToMetaVictims map[string]*extenderv1.MetaVictims,
 	nodeInfos framework.NodeInfoLister,
-) (map[*v1.Node]*extenderv1.Victims, error) {
-	nodeToVictims := map[*v1.Node]*extenderv1.Victims{}
+) (map[string]*extenderv1.Victims, error) {
+	nodeNameToVictims := map[string]*extenderv1.Victims{}
 	for nodeName, metaVictims := range nodeNameToMetaVictims {
 		nodeInfo, err := nodeInfos.Get(nodeName)
 		if err != nil {
@@ -275,9 +273,9 @@ func (h *HTTPExtender) convertToNodeToVictims(
 			}
 			victims.Pods = append(victims.Pods, pod)
 		}
-		nodeToVictims[nodeInfo.Node()] = victims
+		nodeNameToVictims[nodeName] = victims
 	}
-	return nodeToVictims, nil
+	return nodeNameToVictims, nil
 }
 
 // convertPodUIDToPod returns v1.Pod object for given MetaPod and node info.
@@ -298,10 +296,10 @@ func (h *HTTPExtender) convertPodUIDToPod(
 
 // convertToNodeNameToMetaVictims converts from struct type to meta types.
 func convertToNodeNameToMetaVictims(
-	nodeToVictims map[*v1.Node]*extenderv1.Victims,
+	nodeNameToVictims map[string]*extenderv1.Victims,
 ) map[string]*extenderv1.MetaVictims {
-	nodeNameToVictims := map[string]*extenderv1.MetaVictims{}
-	for node, victims := range nodeToVictims {
+	nodeNameToMetaVictims := map[string]*extenderv1.MetaVictims{}
+	for node, victims := range nodeNameToVictims {
 		metaVictims := &extenderv1.MetaVictims{
 			Pods: []*extenderv1.MetaPod{},
 		}
@@ -311,20 +309,9 @@ func convertToNodeNameToMetaVictims(
 			}
 			metaVictims.Pods = append(metaVictims.Pods, metaPod)
 		}
-		nodeNameToVictims[node.GetName()] = metaVictims
-	}
-	return nodeNameToVictims
-}
-
-// convertToNodeNameToVictims converts from node type to node name as key.
-func convertToNodeNameToVictims(
-	nodeToVictims map[*v1.Node]*extenderv1.Victims,
-) map[string]*extenderv1.Victims {
-	nodeNameToVictims := map[string]*extenderv1.Victims{}
-	for node, victims := range nodeToVictims {
-		nodeNameToVictims[node.GetName()] = victims
+		nodeNameToMetaVictims[node] = metaVictims
 	}
-	return nodeNameToVictims
+	return nodeNameToMetaVictims
 }
 
 // Filter based on extender implemented predicate functions. The filtered list is

pkg/scheduler/core/extender_test.go

@@ -159,36 +159,37 @@ func (f *FakeExtender) SupportsPreemption() bool {
 
 func (f *FakeExtender) ProcessPreemption(
 	pod *v1.Pod,
-	nodeToVictims map[*v1.Node]*extenderv1.Victims,
+	nodeNameToVictims map[string]*extenderv1.Victims,
 	nodeInfos framework.NodeInfoLister,
-) (map[*v1.Node]*extenderv1.Victims, error) {
-	nodeToVictimsCopy := map[*v1.Node]*extenderv1.Victims{}
-	// We don't want to change the original nodeToVictims
-	for k, v := range nodeToVictims {
+) (map[string]*extenderv1.Victims, error) {
+	nodeNameToVictimsCopy := map[string]*extenderv1.Victims{}
+	// We don't want to change the original nodeNameToVictims
+	for k, v := range nodeNameToVictims {
 		// In real world implementation, extender's user should have their own way to get node object
 		// by name if needed (e.g. query kube-apiserver etc).
 		//
 		// For test purpose, we just use node from parameters directly.
-		nodeToVictimsCopy[k] = v
+		nodeNameToVictimsCopy[k] = v
 	}
 
-	for node, victims := range nodeToVictimsCopy {
+	for nodeName, victims := range nodeNameToVictimsCopy {
 		// Try to do preemption on extender side.
-		extenderVictimPods, extendernPDBViolations, fits, err := f.selectVictimsOnNodeByExtender(pod, node)
+		nodeInfo, _ := nodeInfos.Get(nodeName)
+		extenderVictimPods, extenderPDBViolations, fits, err := f.selectVictimsOnNodeByExtender(pod, nodeInfo.Node())
 		if err != nil {
 			return nil, err
 		}
 		// If it's unfit after extender's preemption, this node is unresolvable by preemption overall,
 		// let's remove it from potential preemption nodes.
 		if !fits {
-			delete(nodeToVictimsCopy, node)
+			delete(nodeNameToVictimsCopy, nodeName)
 		} else {
 			// Append new victims to original victims
-			nodeToVictimsCopy[node].Pods = append(victims.Pods, extenderVictimPods...)
-			nodeToVictimsCopy[node].NumPDBViolations = victims.NumPDBViolations + int64(extendernPDBViolations)
+			nodeNameToVictimsCopy[nodeName].Pods = append(victims.Pods, extenderVictimPods...)
+			nodeNameToVictimsCopy[nodeName].NumPDBViolations = victims.NumPDBViolations + int64(extenderPDBViolations)
 		}
 	}
-	return nodeToVictimsCopy, nil
+	return nodeNameToVictimsCopy, nil
 }
 
 // selectVictimsOnNodeByExtender checks the given nodes->pods map with predicates on extender's side.

pkg/scheduler/core/generic_scheduler.go

@@ -105,7 +105,7 @@ type ScheduleAlgorithm interface {
 	// the pod by preempting lower priority pods if possible.
 	// It returns the node where preemption happened, a list of preempted pods, a
 	// list of pods whose nominated node name should be removed, and error if any.
-	Preempt(context.Context, *profile.Profile, *framework.CycleState, *v1.Pod, error) (selectedNode *v1.Node, preemptedPods []*v1.Pod, cleanupNominatedPods []*v1.Pod, err error)
+	Preempt(context.Context, *profile.Profile, *framework.CycleState, *v1.Pod, error) (selectedNode string, preemptedPods []*v1.Pod, cleanupNominatedPods []*v1.Pod, err error)
 	// Extenders returns a slice of extender config. This is exposed for
 	// testing.
 	Extenders() []SchedulerExtender
@@ -251,74 +251,74 @@ func (g *genericScheduler) selectHost(nodeScoreList framework.NodeScoreList) (st
 // other pods with the same priority. The nominated pod prevents other pods from
 // using the nominated resources and the nominated pod could take a long time
 // before it is retried after many other pending pods.
-func (g *genericScheduler) Preempt(ctx context.Context, prof *profile.Profile, state *framework.CycleState, pod *v1.Pod, scheduleErr error) (*v1.Node, []*v1.Pod, []*v1.Pod, error) {
+func (g *genericScheduler) Preempt(ctx context.Context, prof *profile.Profile, state *framework.CycleState, pod *v1.Pod, scheduleErr error) (string, []*v1.Pod, []*v1.Pod, error) {
 	// Scheduler may return various types of errors. Consider preemption only if
 	// the error is of type FitError.
 	fitError, ok := scheduleErr.(*FitError)
 	if !ok || fitError == nil {
-		return nil, nil, nil, nil
+		return "", nil, nil, nil
 	}
 	if !podEligibleToPreemptOthers(pod, g.nodeInfoSnapshot.NodeInfos()) {
 		klog.V(5).Infof("Pod %v/%v is not eligible for more preemption.", pod.Namespace, pod.Name)
-		return nil, nil, nil, nil
+		return "", nil, nil, nil
 	}
 	allNodes, err := g.nodeInfoSnapshot.NodeInfos().List()
 	if err != nil {
-		return nil, nil, nil, err
+		return "", nil, nil, err
 	}
 	if len(allNodes) == 0 {
-		return nil, nil, nil, ErrNoNodesAvailable
+		return "", nil, nil, ErrNoNodesAvailable
 	}
 	potentialNodes := nodesWherePreemptionMightHelp(allNodes, fitError)
 	if len(potentialNodes) == 0 {
 		klog.V(3).Infof("Preemption will not help schedule pod %v/%v on any node.", pod.Namespace, pod.Name)
 		// In this case, we should clean-up any existing nominated node name of the pod.
-		return nil, nil, []*v1.Pod{pod}, nil
+		return "", nil, []*v1.Pod{pod}, nil
 	}
 	var pdbs []*policy.PodDisruptionBudget
 	if g.pdbLister != nil {
 		pdbs, err = g.pdbLister.List(labels.Everything())
 		if err != nil {
-			return nil, nil, nil, err
+			return "", nil, nil, err
 		}
 	}
-	nodeToVictims, err := g.selectNodesForPreemption(ctx, prof, state, pod, potentialNodes, pdbs)
+	nodeNameToVictims, err := g.selectNodesForPreemption(ctx, prof, state, pod, potentialNodes, pdbs)
 	if err != nil {
-		return nil, nil, nil, err
+		return "", nil, nil, err
 	}
 
-	// We will only check nodeToVictims with extenders that support preemption.
+	// We will only check nodeNameToVictims with extenders that support preemption.
 	// Extenders which do not support preemption may later prevent preemptor from being scheduled on the nominated
 	// node. In that case, scheduler will find a different host for the preemptor in subsequent scheduling cycles.
-	nodeToVictims, err = g.processPreemptionWithExtenders(pod, nodeToVictims)
+	nodeNameToVictims, err = g.processPreemptionWithExtenders(pod, nodeNameToVictims)
 	if err != nil {
-		return nil, nil, nil, err
+		return "", nil, nil, err
 	}
 
-	candidateNode := pickOneNodeForPreemption(nodeToVictims)
-	if candidateNode == nil {
-		return nil, nil, nil, nil
+	candidateNode := pickOneNodeForPreemption(nodeNameToVictims)
+	if len(candidateNode) == 0 {
+		return "", nil, nil, nil
 	}
 
 	// Lower priority pods nominated to run on this node, may no longer fit on
 	// this node. So, we should remove their nomination. Removing their
 	// nomination updates these pods and moves them to the active queue. It
 	// lets scheduler find another place for them.
-	nominatedPods := g.getLowerPriorityNominatedPods(pod, candidateNode.Name)
-	return candidateNode, nodeToVictims[candidateNode].Pods, nominatedPods, nil
+	nominatedPods := g.getLowerPriorityNominatedPods(pod, candidateNode)
+	return candidateNode, nodeNameToVictims[candidateNode].Pods, nominatedPods, nil
 }
 
 // processPreemptionWithExtenders processes preemption with extenders
 func (g *genericScheduler) processPreemptionWithExtenders(
 	pod *v1.Pod,
-	nodeToVictims map[*v1.Node]*extenderv1.Victims,
-) (map[*v1.Node]*extenderv1.Victims, error) {
-	if len(nodeToVictims) > 0 {
+	nodeNameToVictims map[string]*extenderv1.Victims,
+) (map[string]*extenderv1.Victims, error) {
+	if len(nodeNameToVictims) > 0 {
 		for _, extender := range g.extenders {
 			if extender.SupportsPreemption() && extender.IsInterested(pod) {
-				newNodeToVictims, err := extender.ProcessPreemption(
+				newNodeNameToVictims, err := extender.ProcessPreemption(
 					pod,
-					nodeToVictims,
+					nodeNameToVictims,
 					g.nodeInfoSnapshot.NodeInfos(),
 				)
 				if err != nil {
@@ -330,19 +330,19 @@ func (g *genericScheduler) processPreemptionWithExtenders(
 					return nil, err
 				}
 
-				// Replace nodeToVictims with new result after preemption. So the
+				// Replace nodeNameToVictims with new result after preemption. So the
 				// rest of extenders can continue use it as parameter.
-				nodeToVictims = newNodeToVictims
+				nodeNameToVictims = newNodeNameToVictims
 
 				// If node list becomes empty, no preemption can happen regardless of other extenders.
-				if len(nodeToVictims) == 0 {
+				if len(nodeNameToVictims) == 0 {
 					break
 				}
 			}
 		}
 	}
 
-	return nodeToVictims, nil
+	return nodeNameToVictims, nil
 }
 
 // getLowerPriorityNominatedPods returns pods whose priority is smaller than the
@@ -719,12 +719,12 @@ func (g *genericScheduler) prioritizeNodes(
 // 6. If there are still ties, the first such node is picked (sort of randomly).
 // The 'minNodes1' and 'minNodes2' are being reused here to save the memory
 // allocation and garbage collection time.
-func pickOneNodeForPreemption(nodesToVictims map[*v1.Node]*extenderv1.Victims) *v1.Node {
+func pickOneNodeForPreemption(nodesToVictims map[string]*extenderv1.Victims) string {
 	if len(nodesToVictims) == 0 {
-		return nil
+		return ""
 	}
 	minNumPDBViolatingPods := int64(math.MaxInt32)
-	var minNodes1 []*v1.Node
+	var minNodes1 []string
 	lenNodes1 := 0
 	for node, victims := range nodesToVictims {
 		if len(victims.Pods) == 0 {
@@ -752,7 +752,7 @@ func pickOneNodeForPreemption(nodesToVictims map[*v1.Node]*extenderv1.Victims) *
 	// There are more than one node with minimum number PDB violating pods. Find
 	// the one with minimum highest priority victim.
 	minHighestPriority := int32(math.MaxInt32)
-	var minNodes2 = make([]*v1.Node, lenNodes1)
+	var minNodes2 = make([]string, lenNodes1)
 	lenNodes2 := 0
 	for i := 0; i < lenNodes1; i++ {
 		node := minNodes1[i]
@@ -855,8 +855,8 @@ func (g *genericScheduler) selectNodesForPreemption(
 	pod *v1.Pod,
 	potentialNodes []*framework.NodeInfo,
 	pdbs []*policy.PodDisruptionBudget,
-) (map[*v1.Node]*extenderv1.Victims, error) {
-	nodeToVictims := map[*v1.Node]*extenderv1.Victims{}
+) (map[string]*extenderv1.Victims, error) {
+	nodeNameToVictims := map[string]*extenderv1.Victims{}
 	var resultLock sync.Mutex
 
 	checkNode := func(i int) {
@@ -869,12 +869,12 @@ func (g *genericScheduler) selectNodesForPreemption(
 				Pods:             pods,
 				NumPDBViolations: int64(numPDBViolations),
 			}
-			nodeToVictims[potentialNodes[i].Node()] = &victims
+			nodeNameToVictims[potentialNodes[i].Node().Name] = &victims
 			resultLock.Unlock()
 		}
 	}
 	parallelize.Until(ctx, len(potentialNodes), checkNode)
-	return nodeToVictims, nil
+	return nodeNameToVictims, nil
 }
 
 // filterPodsWithPDBViolation groups the given "pods" into two groups of "violatingPods"