Skip to content

Commit

Permalink
scheduler: tolerate having only one dynamic port available (#17619)
Browse files Browse the repository at this point in the history
If the dynamic port range for a node is set so that the min is equal to the max,
there's only one port available and this passes config validation. But the
scheduler panics when it tries to pick a random port. Only add the randomness
when there's more than one to pick from.

Adds a test for the behavior but also adjusts the commentary on a couple of the
existing tests that made it seem like this case was already covered if you
didn't look too closely.

Fixes: #17585
  • Loading branch information
tgross committed Jun 20, 2023
1 parent 748aea1 commit f73b454
Show file tree
Hide file tree
Showing 3 changed files with 90 additions and 4 deletions.
3 changes: 3 additions & 0 deletions .changelog/17619.txt
Original file line number Diff line number Diff line change
@@ -0,0 +1,3 @@
```release-note:bug
scheduler: Fixed a panic when a node has only one configured dynamic port
```
6 changes: 5 additions & 1 deletion nomad/structs/network.go
Original file line number Diff line number Diff line change
Expand Up @@ -749,7 +749,11 @@ func getDynamicPortsStochastic(nodeUsed Bitmap, portsInOffer []int, minDynamicPo
return nil, fmt.Errorf("stochastic dynamic port selection failed")
}

randPort := minDynamicPort + rand.Intn(maxDynamicPort-minDynamicPort)
randPort := minDynamicPort
if maxDynamicPort-minDynamicPort > 0 {
randPort = randPort + rand.Intn(maxDynamicPort-minDynamicPort)
}

if nodeUsed != nil && nodeUsed.Check(uint(randPort)) {
goto PICK
}
Expand Down
85 changes: 82 additions & 3 deletions nomad/structs/network_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -362,7 +362,7 @@ func TestNetworkIndex_yieldIP(t *testing.T) {
func TestNetworkIndex_AssignPorts(t *testing.T) {
ci.Parallel(t)

// Create a node that only has one free port
// Create a node that only two free dynamic ports
idx := NewNetworkIndex()
n := &Node{
NodeResources: &NodeResources{
Expand Down Expand Up @@ -424,6 +424,84 @@ func TestNetworkIndex_AssignPorts(t *testing.T) {
must.Between(t, idx.MaxDynamicPort-1, adminPortMapping.Value, idx.MaxDynamicPort)
}

// TestNetworkIndex_AssignPorts_SmallRange exercises assigning ports on group
// networks with small dynamic port ranges configured
func TestNetworkIndex_AssignPortss_SmallRange(t *testing.T) {
ci.Parallel(t)

n := &Node{
NodeResources: &NodeResources{
NodeNetworks: []*NodeNetworkResource{
{
Mode: "host",
Device: "eth0",
Speed: 1000,
Addresses: []NodeNetworkAddress{
{
Alias: "default",
Address: "192.168.0.100",
Family: NodeNetworkAF_IPv4,
},
},
},
},
},
}

testCases := []struct {
name string
min int
max int
ask []Port
expectErr string
}{
{
name: "1 dynamic port avail and 1 port requested",
min: 20000,
max: 20000,
ask: []Port{{"http", 0, 80, "default"}},
expectErr: "",
},
{
name: "1 dynamic port avail and 2 ports requested",
min: 20000,
max: 20000,
ask: []Port{{"http", 0, 80, "default"}, {"admin", 0, 80, "default"}},
expectErr: "dynamic port selection failed",
},
{
name: "2 dynamic ports avail and 2 ports requested",
min: 20000,
max: 20001,
ask: []Port{{"http", 0, 80, "default"}, {"admin", 0, 80, "default"}},
expectErr: "",
},
}

for _, tc := range testCases {

idx := NewNetworkIndex()
idx.MinDynamicPort = tc.min
idx.MaxDynamicPort = tc.max
idx.SetNode(n)

ask := &NetworkResource{DynamicPorts: tc.ask}
offer, err := idx.AssignPorts(ask)
if tc.expectErr != "" {
must.EqError(t, err, tc.expectErr)
} else {
must.NoError(t, err)
must.NotNil(t, offer, must.Sprint("did not get an offer"))

for _, port := range tc.ask {
_, ok := offer.Get(port.Label)
must.True(t, ok)
}
}
}

}

func TestNetworkIndex_AssignTaskNetwork(t *testing.T) {
ci.Parallel(t)
idx := NewNetworkIndex()
Expand Down Expand Up @@ -531,7 +609,7 @@ func TestNetworkIndex_AssignTaskNetwork(t *testing.T) {
func TestNetworkIndex_AssignTaskNetwork_Dynamic_Contention(t *testing.T) {
ci.Parallel(t)

// Create a node that only has one free port
// Create a node that only has two free dynamic ports
idx := NewNetworkIndex()
n := &Node{
NodeResources: &NodeResources{
Expand All @@ -546,6 +624,7 @@ func TestNetworkIndex_AssignTaskNetwork_Dynamic_Contention(t *testing.T) {
},
ReservedResources: &NodeReservedResources{
Networks: NodeReservedNetworkResources{
// leave only 2 available ports
ReservedHostPorts: fmt.Sprintf("%d-%d", idx.MinDynamicPort, idx.MaxDynamicPort-2),
},
},
Expand All @@ -561,7 +640,7 @@ func TestNetworkIndex_AssignTaskNetwork_Dynamic_Contention(t *testing.T) {
must.NoError(t, err)
must.NotNil(t, offer, must.Sprint("did not get an offer"))
must.Eq(t, "192.168.0.100", offer.IP)
must.Len(t, 2, offer.DynamicPorts, must.Sprint("There should be one dynamic ports"))
must.Len(t, 2, offer.DynamicPorts, must.Sprint("There should be two dynamic ports"))

must.NotEq(t, offer.DynamicPorts[0].Value, offer.DynamicPorts[1].Value,
must.Sprint("assigned dynamic ports must not conflict"))
Expand Down

0 comments on commit f73b454

Please sign in to comment.