From e9e4358f163b325e3ba9b8af009f1b7fe44bacef Mon Sep 17 00:00:00 2001 From: Tim Gross Date: Tue, 20 Jun 2023 11:23:27 -0400 Subject: [PATCH] scheduler: tolerate having only one dynamic port available 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 --- .changelog/17619.txt | 3 ++ nomad/structs/network.go | 6 ++- nomad/structs/network_test.go | 85 +++++++++++++++++++++++++++++++++-- 3 files changed, 90 insertions(+), 4 deletions(-) create mode 100644 .changelog/17619.txt diff --git a/.changelog/17619.txt b/.changelog/17619.txt new file mode 100644 index 000000000000..8b5aa1375fd5 --- /dev/null +++ b/.changelog/17619.txt @@ -0,0 +1,3 @@ +```release-note:bug +scheduler: Fixed a panic when a node has only one configured dynamic port +``` diff --git a/nomad/structs/network.go b/nomad/structs/network.go index f1cdf774de9e..7374967b2a11 100644 --- a/nomad/structs/network.go +++ b/nomad/structs/network.go @@ -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 } diff --git a/nomad/structs/network_test.go b/nomad/structs/network_test.go index 071e1028c23e..754f8f249716 100644 --- a/nomad/structs/network_test.go +++ b/nomad/structs/network_test.go @@ -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{ @@ -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() @@ -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{ @@ -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), }, }, @@ -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"))