Backport of scheduler: don't sort reserved port ranges before adding to bitmap into release/1.10.x (#26725)

Co-authored-by: Tim Gross <[email protected]>

Author: hc-github-team-nomad-core

.changelog/26712.txt

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

nomad/structs/funcs.go

@@ -9,6 +9,7 @@ import (
 	"encoding/binary"
 	"fmt"
 	"math"
+	"slices"
 	"sort"
 	"strconv"
 	"strings"
@@ -485,15 +486,18 @@ func CompareMigrateToken(allocID, nodeSecretID, otherMigrateToken string) bool {
 // port ranges. A port number is a single integer and a port range is two
 // integers separated by a hyphen. As an example the following spec would
 // convert to: ParsePortRanges("10,12-14,16") -> []uint64{10, 12, 13, 14, 16}
+// This function may return duplicates or overlapping ranges, so we limit the
+// maximum number of ports returned to MaxValidPort.
 func ParsePortRanges(spec string) ([]uint64, error) {
+	count := 0
 	parts := strings.Split(spec, ",")
 
 	// Hot path the empty case
 	if len(parts) == 1 && parts[0] == "" {
 		return nil, nil
 	}
 
-	ports := make(map[uint64]struct{})
+	ports := []uint64{}
 	for _, part := range parts {
 		part = strings.TrimSpace(part)
 		rangeParts := strings.Split(part, "-")
@@ -507,11 +511,17 @@ 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{}{}
+				count++
+				if count > MaxValidPort {
+					return nil, fmt.Errorf("maximum of %d ports can be reserved", MaxValidPort)
+				}
+				ports = append(ports, port)
 			}
 		case 2:
 			// We are parsing a range
@@ -526,36 +536,29 @@ 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)
 			}
-
+			count += int(end - start)
+			if count > MaxValidPort {
+				return nil, fmt.Errorf("maximum of %d ports can be reserved", MaxValidPort)
+			}
+			ports = slices.Grow(ports, int(end-start))
 			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,44 @@ 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
+		},
+		{
+			name: "HugeRange",
+			spec: "1-65536,1-65536",
+			err:  "maximum of 65536 ports can be reserved",
+		},
 	}
 
 	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

@@ -3774,11 +3774,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

@@ -7258,53 +7258,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)