diff --git a/pkg/scheduler/framework/plugins/nodeports/node_ports.go b/pkg/scheduler/framework/plugins/nodeports/node_ports.go index 3566bfab0c9..2f2c9932077 100644 --- a/pkg/scheduler/framework/plugins/nodeports/node_ports.go +++ b/pkg/scheduler/framework/plugins/nodeports/node_ports.go @@ -136,15 +136,13 @@ func (pl *NodePorts) isSchedulableAfterPodDeleted(logger klog.Logger, pod *v1.Po return fwk.QueueSkip, nil } - // Construct a fake NodeInfo that only has the deleted Pod. - // If we can schedule `pod` to this fake node, it means that `pod` and the deleted pod don't have any common port(s). + // Verify that `pod` and the deleted pod don't have any common port(s). // So, deleting that pod couldn't make `pod` schedulable. - usedPorts := make(fwk.HostPortInfo, len(ports)) + portsInUse := make(fwk.HostPortInfo, len(ports)) for _, p := range ports { - usedPorts.Add(p.HostIP, string(p.Protocol), p.HostPort) + portsInUse.Add(p.HostIP, string(p.Protocol), p.HostPort) } - nodeInfo := framework.NodeInfo{UsedPorts: usedPorts} - if Fits(pod, &nodeInfo) { + if fitsPorts(util.GetHostPorts(pod), portsInUse) { logger.V(4).Info("the deleted pod and the target pod don't have any common port(s), returning QueueSkip as deleting this Pod won't make the Pod schedulable", "pod", klog.KObj(pod), "deletedPod", klog.KObj(deletedPod)) return fwk.QueueSkip, nil } @@ -160,7 +158,7 @@ func (pl *NodePorts) Filter(ctx context.Context, cycleState fwk.CycleState, pod return fwk.AsStatus(err) } - fits := fitsPorts(wantPorts, nodeInfo) + fits := fitsPorts(wantPorts, nodeInfo.GetUsedPorts()) if !fits { return fwk.NewStatus(fwk.Unschedulable, ErrReason) } @@ -168,16 +166,16 @@ func (pl *NodePorts) Filter(ctx context.Context, cycleState fwk.CycleState, pod return nil } -// Fits checks if the pod fits the node. +// Fits checks if the pod has any ports conflicting with nodeInfo's ports. +// It returns true if there are no conflicts (which means that pod fits the node), otherwise false. func Fits(pod *v1.Pod, nodeInfo fwk.NodeInfo) bool { - return fitsPorts(util.GetHostPorts(pod), nodeInfo) + return fitsPorts(util.GetHostPorts(pod), nodeInfo.GetUsedPorts()) } -func fitsPorts(wantPorts []v1.ContainerPort, nodeInfo fwk.NodeInfo) bool { - // try to see whether existingPorts and wantPorts will conflict or not - existingPorts := nodeInfo.GetUsedPorts() +func fitsPorts(wantPorts []v1.ContainerPort, portsInUse fwk.HostPortInfo) bool { + // try to see whether portsInUse and wantPorts will conflict or not for _, cp := range wantPorts { - if existingPorts.CheckConflict(cp.HostIP, string(cp.Protocol), cp.HostPort) { + if portsInUse.CheckConflict(cp.HostIP, string(cp.Protocol), cp.HostPort) { return false } } diff --git a/pkg/scheduler/framework/plugins/nodeports/node_ports_test.go b/pkg/scheduler/framework/plugins/nodeports/node_ports_test.go index e31a365eb01..c7f1786d8a9 100644 --- a/pkg/scheduler/framework/plugins/nodeports/node_ports_test.go +++ b/pkg/scheduler/framework/plugins/nodeports/node_ports_test.go @@ -48,6 +48,10 @@ func newPod(host string, hostPortInfos ...string) *v1.Pod { return st.MakePod().Node(host).ContainerPort(networkPorts).Obj() } +func newNodeInfo(hostPortInfos ...string) fwk.NodeInfo { + return framework.NewNodeInfo(newPod("p1", hostPortInfos...)) +} + func TestNodePorts(t *testing.T) { tests := []struct { pod *v1.Pod @@ -270,3 +274,67 @@ func Test_isSchedulableAfterPodDeleted(t *testing.T) { }) } } + +// This test is similar to TestHostPortInfo_Check in k8s.io/kube-scheduler/framework/types_test.go, +// but it tests a smaller set of cases. We could consider using a fake HostPortInfo to verify the logic +// of Fits, instead of the logic of CheckConflict. +func TestFits(t *testing.T) { + tests := []struct { + desc string + pod *v1.Pod + existing fwk.NodeInfo + expect bool + }{ + { + desc: "non-conflicting ports", + pod: newPod("p", "TCP/127.0.0.1/80"), + existing: newNodeInfo("TCP/127.0.0.1/9090"), + expect: true, + }, + { + desc: "multiple non-conflicting ports", + pod: newPod("p", + "TCP/127.0.0.1/80", + "TCP/127.0.1.1/80", + "TCP/127.0.1.1/90", + "TCP/127.0.1.1/100"), + existing: newNodeInfo( + "TCP/127.0.0.1/9090", + "TCP/127.0.0.1/9191", + "TCP/127.0.1.1/8080"), + expect: true, + }, + { + desc: "same ports on different protocols", + pod: newPod("m1", "TCP/127.0.0.1/80"), + existing: newNodeInfo("UDP/127.0.0.1/80"), + expect: true, + }, + { + desc: "conflicting ports", + pod: newPod("m1", "TCP/127.0.0.1/80"), + existing: newNodeInfo("TCP/127.0.0.1/80"), + expect: false, + }, + { + desc: "multiple ports, some conflicting", + pod: newPod("p", + "TCP/127.0.0.1/80", + "TCP/127.0.1.1/80", + "TCP/127.0.1.1/90", + "TCP/127.0.1.1/100"), + existing: newNodeInfo( + "TCP/127.0.0.1/9090", + "TCP/127.0.1.1/90", + "TCP/127.0.1.1/8080"), + expect: false, + }, + } + for _, test := range tests { + t.Run(test.desc, func(t *testing.T) { + if Fits(test.pod, test.existing) != test.expect { + t.Errorf("expected %t; got %t", test.expect, !test.expect) + } + }) + } +}