diff --git a/.changelog/26712.txt b/.changelog/26712.txt new file mode 100644 index 00000000000..ead75eb8d49 --- /dev/null +++ b/.changelog/26712.txt @@ -0,0 +1,3 @@ +```release-note:improvement +scheduling: Improve performance of scheduling when checking reserved ports usage +``` diff --git a/nomad/structs/funcs.go b/nomad/structs/funcs.go index 327ee3ff98e..47c453d8e53 100644 --- a/nomad/structs/funcs.go +++ b/nomad/structs/funcs.go @@ -9,6 +9,7 @@ import ( "encoding/binary" "fmt" "math" + "slices" "sort" "strconv" "strings" @@ -485,7 +486,10 @@ 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 @@ -493,7 +497,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 +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 } diff --git a/nomad/structs/funcs_test.go b/nomad/structs/funcs_test.go index 269e5c8990d..dcecd168467 100644 --- a/nomad/structs/funcs_test.go +++ b/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) + } }) } } diff --git a/nomad/structs/structs.go b/nomad/structs/structs.go index 0ef49b08179..6958d9b8996 100644 --- a/nomad/structs/structs.go +++ b/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. diff --git a/nomad/structs/structs_test.go b/nomad/structs/structs_test.go index 0fd40214531..66b8879e07b 100644 --- a/nomad/structs/structs_test.go +++ b/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)