Merge pull request kubernetes#91037 from Huang-Wei/prefactor-PreemptExtender

Refactor preemption extender logic and move SchedulerExtender interface to framework pkg

Author: k8s-ci-robot

pkg/scheduler/core/extender.go

@@ -38,55 +38,7 @@ const (
 	DefaultExtenderTimeout = 5 * time.Second
 )
 
-// SchedulerExtender is an interface for external processes to influence scheduling
-// decisions made by Kubernetes. This is typically needed for resources not directly
-// managed by Kubernetes.
-type SchedulerExtender interface {
-	// Name returns a unique name that identifies the extender.
-	Name() string
-
-	// Filter based on extender-implemented predicate functions. The filtered list is
-	// expected to be a subset of the supplied list. failedNodesMap optionally contains
-	// the list of failed nodes and failure reasons.
-	Filter(pod *v1.Pod, nodes []*v1.Node) (filteredNodes []*v1.Node, failedNodesMap extenderv1.FailedNodesMap, err error)
-
-	// Prioritize based on extender-implemented priority functions. The returned scores & weight
-	// are used to compute the weighted score for an extender. The weighted scores are added to
-	// the scores computed by Kubernetes scheduler. The total scores are used to do the host selection.
-	Prioritize(pod *v1.Pod, nodes []*v1.Node) (hostPriorities *extenderv1.HostPriorityList, weight int64, err error)
-
-	// Bind delegates the action of binding a pod to a node to the extender.
-	Bind(binding *v1.Binding) error
-
-	// IsBinder returns whether this extender is configured for the Bind method.
-	IsBinder() bool
-
-	// IsInterested returns true if at least one extended resource requested by
-	// this pod is managed by this extender.
-	IsInterested(pod *v1.Pod) bool
-
-	// ProcessPreemption returns nodes with their victim pods processed by extender based on
-	// 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)
-
-	// SupportsPreemption returns if the scheduler extender support preemption or not.
-	SupportsPreemption() bool
-
-	// IsIgnorable returns true indicates scheduling should not fail when this extender
-	// is unavailable. This gives scheduler ability to fail fast and tolerate non-critical extenders as well.
-	IsIgnorable() bool
-}
-
-// HTTPExtender implements the SchedulerExtender interface.
+// HTTPExtender implements the Extender interface.
 type HTTPExtender struct {
 	extenderURL      string
 	preemptVerb      string
@@ -131,7 +83,7 @@ func makeTransport(config *schedulerapi.Extender) (http.RoundTripper, error) {
 }
 
 // NewHTTPExtender creates an HTTPExtender object.
-func NewHTTPExtender(config *schedulerapi.Extender) (SchedulerExtender, error) {
+func NewHTTPExtender(config *schedulerapi.Extender) (framework.Extender, error) {
 	if config.HTTPTimeout.Nanoseconds() == 0 {
 		config.HTTPTimeout = time.Duration(DefaultExtenderTimeout)
 	}
@@ -212,9 +164,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 +178,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 +195,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 +226,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 +249,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 +262,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.
@@ -352,7 +353,7 @@ func (f *FakeExtender) IsInterested(pod *v1.Pod) bool {
 	return !f.unInterested
 }
 
-var _ SchedulerExtender = &FakeExtender{}
+var _ framework.Extender = &FakeExtender{}
 
 func TestGenericSchedulerWithExtenders(t *testing.T) {
 	tests := []struct {
@@ -574,7 +575,7 @@ func TestGenericSchedulerWithExtenders(t *testing.T) {
 			client := clientsetfake.NewSimpleClientset()
 			informerFactory := informers.NewSharedInformerFactory(client, 0)
 
-			extenders := []SchedulerExtender{}
+			extenders := []framework.Extender{}
 			for ii := range test.extenders {
 				extenders = append(extenders, &test.extenders[ii])
 			}