networking: don't sort reserved port ranges before adding to bitmap

During a large volume dispatch load test, I discovered that a lot of the total
scheduling time is being spent calling `structs.ParsePortRanges` repeatedly, in
order to parse the reserved ports configuration of the node (ex. converting
`"80,8000-8001"` to `[]int{80, 8000, 8001}`). A close examination of the
profiles shows that the bulk of the time is being spent hashing the keys for the
map of ports we use for de-duplication, and then sorting the resulting slice.

The `(*NetworkIndex) SetNode` method that calls the offending `ParsePortRanges`
merges all the ports into the `UsedPorts` map of bitmaps at scheduling
time. Which means the consumer of the slice is already de-duplicating and
doesn't care about the order. The only other caller of `ParsePortRanges` is when
we validate the configuration file, and that throws away the slice entirely.

By skipping de-duplication and not sorting, we can cut down the runtime of this
function by 30x and memory usage by 3x.

Ref: https://github.com/hashicorp/nomad/blob/v1.10.4/nomad/structs/network.go#L201
Fixes: #26654

Author: tgross

.changelog/26712.txt

@@ -0,0 +1,3 @@
+```release-note:improvement
+scheduling: Improve performance of scheduling when checking reserved ports usage
+```

nomad/structs/funcs.go

@@ -493,7 +493,7 @@ func ParsePortRanges(spec string) ([]uint64, error) {
 		return nil, nil
 	}
 
-	ports := make(map[uint64]struct{})
+	ports := []uint64{}
 	for _, part := range parts {
 		part = strings.TrimSpace(part)
 		rangeParts := strings.Split(part, "-")
@@ -507,11 +507,13 @@ func ParsePortRanges(spec string) ([]uint64, error) {
 				if err != nil {
 					return nil, err
 				}
-
+				if port == 0 {
+					return nil, fmt.Errorf("port must be > 0")
+				}
 				if port > MaxValidPort {
 					return nil, fmt.Errorf("port must be < %d but found %d", MaxValidPort, port)
 				}
-				ports[port] = struct{}{}
+				ports = append(ports, port)
 			}
 		case 2:
 			// We are parsing a range
@@ -526,36 +528,25 @@ func ParsePortRanges(spec string) ([]uint64, error) {
 			}
 
 			if end < start {
-				return nil, fmt.Errorf("invalid range: starting value (%v) less than ending (%v) value", end, start)
+				return nil, fmt.Errorf("invalid range: ending value (%v) less than starting (%v) value", end, start)
 			}
 
 			// Full range validation is below but prevent creating
 			// arbitrarily large arrays here
+			if start == 0 {
+				return nil, fmt.Errorf("port must be > 0")
+			}
 			if end > MaxValidPort {
 				return nil, fmt.Errorf("port must be < %d but found %d", MaxValidPort, end)
 			}
 
 			for i := start; i <= end; i++ {
-				ports[i] = struct{}{}
+				ports = append(ports, i)
 			}
 		default:
 			return nil, fmt.Errorf("can only parse single port numbers or port ranges (ex. 80,100-120,150)")
 		}
 	}
 
-	var results []uint64
-	for port := range ports {
-		if port == 0 {
-			return nil, fmt.Errorf("port must be > 0")
-		}
-		if port > MaxValidPort {
-			return nil, fmt.Errorf("port must be < %d but found %d", MaxValidPort, port)
-		}
-		results = append(results, port)
-	}
-
-	sort.Slice(results, func(i, j int) bool {
-		return results[i] < results[j]
-	})
-	return results, nil
+	return ports, nil
 }

nomad/structs/funcs_test.go

@@ -966,14 +966,16 @@ func TestVaultNamespaceSet(t *testing.T) {
 	require.ElementsMatch(t, expected, got)
 }
 
-// TestParsePortRanges asserts ParsePortRanges errors on invalid port ranges.
+// TestParsePortRanges asserts ParsePortRanges errors on invalid port ranges and
+// returns the expected values
 func TestParsePortRanges(t *testing.T) {
 	ci.Parallel(t)
 
 	cases := []struct {
-		name string
-		spec string
-		err  string
+		name   string
+		spec   string
+		expect []uint64
+		err    string
 	}{
 		{
 			name: "UnmatchedDash",
@@ -995,14 +997,39 @@ func TestParsePortRanges(t *testing.T) {
 			spec: "9223372036854775807", // (2**63)-1
 			err:  "port must be < 65536 but found 9223372036854775807",
 		},
+		{
+			name:   "OverlappingRanges",
+			spec:   "1-3,2-4",
+			expect: []uint64{1, 2, 3, 2, 3, 4}, // we don't care about dupes
+		},
+		{
+			name: "ReversedRange",
+			spec: "3-1",
+			err:  "invalid range: ending value (1) less than starting (3) value",
+		},
+		{
+			name: "ZeroRange",
+			spec: "0-1",
+			err:  "port must be > 0",
+		},
+		{
+			name:   "OverlappingOutOfOrderRanges",
+			spec:   "2-4,1-3",
+			expect: []uint64{2, 3, 4, 1, 2, 3}, // we don't care about dupes
+		},
 	}
 
 	for i := range cases {
 		tc := cases[i]
 		t.Run(tc.name, func(t *testing.T) {
 			results, err := ParsePortRanges(tc.spec)
-			require.Nil(t, results)
-			require.EqualError(t, err, tc.err)
+			if tc.err == "" {
+				must.NoError(t, err)
+				must.Eq(t, tc.expect, results)
+			} else {
+				must.Nil(t, results)
+				must.EqError(t, err, tc.err)
+			}
 		})
 	}
 }

nomad/structs/structs.go

@@ -3739,11 +3739,6 @@ type NodeReservedNetworkResources struct {
 	ReservedHostPorts string
 }
 
-// ParseReservedHostPorts returns the reserved host ports.
-func (n *NodeReservedNetworkResources) ParseReservedHostPorts() ([]uint64, error) {
-	return ParsePortRanges(n.ReservedHostPorts)
-}
-
 // AllocatedResources is the set of resources to be used by an allocation.
 type AllocatedResources struct {
 	// Tasks is a mapping of task name to the resources for the task.

nomad/structs/structs_test.go

@@ -7283,53 +7283,6 @@ func TestSpread_Validate(t *testing.T) {
 	}
 }
 
-func TestNodeReservedNetworkResources_ParseReserved(t *testing.T) {
-	ci.Parallel(t)
-
-	require := require.New(t)
-	cases := []struct {
-		Input  string
-		Parsed []uint64
-		Err    bool
-	}{
-		{
-			"1,2,3",
-			[]uint64{1, 2, 3},
-			false,
-		},
-		{
-			"3,1,2,1,2,3,1-3",
-			[]uint64{1, 2, 3},
-			false,
-		},
-		{
-			"3-1",
-			nil,
-			true,
-		},
-		{
-			"1-3,2-4",
-			[]uint64{1, 2, 3, 4},
-			false,
-		},
-		{
-			"1-3,4,5-5,6,7,8-10",
-			[]uint64{1, 2, 3, 4, 5, 6, 7, 8, 9, 10},
-			false,
-		},
-	}
-
-	for i, tc := range cases {
-		r := &NodeReservedNetworkResources{ReservedHostPorts: tc.Input}
-		out, err := r.ParseReservedHostPorts()
-		if (err != nil) != tc.Err {
-			t.Fatalf("test case %d: %v", i, err)
-		}
-
-		require.Equal(out, tc.Parsed)
-	}
-}
-
 func TestMultiregion_CopyCanonicalize(t *testing.T) {
 	ci.Parallel(t)