Skip to content

Commit

Permalink
core: merge reserved_ports into host_networks (#13651)
Browse files Browse the repository at this point in the history
Fixes #13505

This fixes #13505 by treating reserved_ports like we treat a lot of jobspec settings: merging settings from more global stanzas (client.reserved.reserved_ports) "down" into more specific stanzas (client.host_networks[].reserved_ports).

As discussed in #13505 there are other options, and since it's totally broken right now we have some flexibility:

Treat overlapping reserved_ports on addresses as invalid and refuse to start agents. However, I'm not sure there's a cohesive model we want to publish right now since so much 0.9-0.12 compat code still exists! We would have to explain to folks that if their -network-interface and host_network addresses overlapped, they could only specify reserved_ports in one place or the other?! It gets ugly.
Use the global client.reserved.reserved_ports value as the default and treat host_network[].reserverd_ports as overrides. My first suggestion in the issue, but @groggemans made me realize the addresses on the agent's interface (as configured by -network-interface) may overlap with host_networks, so you'd need to remove the global reserved_ports from addresses shared with a shared network?! This seemed really confusing and subtle for users to me.
So I think "merging down" creates the most expressive yet understandable approach. I've played around with it a bit, and it doesn't seem too surprising. The only frustrating part is how difficult it is to observe the available addresses and ports on a node! However that's a job for another PR.
  • Loading branch information
schmichael committed Jul 12, 2022
1 parent 7b55f7a commit f998a2b
Show file tree
Hide file tree
Showing 12 changed files with 495 additions and 204 deletions.
3 changes: 3 additions & 0 deletions .changelog/13651.txt
Original file line number Diff line number Diff line change
@@ -0,0 +1,3 @@
```release-note:bug
core: Fixed a bug where reserved ports on multiple node networks would be treated as a collision. `client.reserved.reserved_ports` is now merged into each `host_network`'s reserved ports instead of being treated as a collision.
```
1 change: 1 addition & 0 deletions command/agent/command.go
Original file line number Diff line number Diff line change
Expand Up @@ -399,6 +399,7 @@ func (c *Command) IsValidConfig(config, cmdConfig *Config) bool {
}

for _, hn := range config.Client.HostNetworks {
// Ensure port range is valid
if _, err := structs.ParsePortRanges(hn.ReservedPorts); err != nil {
c.Ui.Error(fmt.Sprintf("host_network[%q].reserved_ports %q invalid: %v",
hn.Name, hn.ReservedPorts, err))
Expand Down
11 changes: 9 additions & 2 deletions nomad/structs/funcs.go
Original file line number Diff line number Diff line change
Expand Up @@ -207,8 +207,11 @@ func AllocsFit(node *Node, allocs []*Allocation, netIdx *NetworkIndex, checkDevi
netIdx = NewNetworkIndex()
defer netIdx.Release()

if collision, reason := netIdx.SetNode(node); collision {
return false, fmt.Sprintf("reserved node port collision: %v", reason), used, nil
if err := netIdx.SetNode(node); err != nil {
// To maintain backward compatibility with when SetNode
// returned collision+reason like AddAllocs, return
// this as a reason instead of an error.
return false, fmt.Sprintf("reserved node port collision: %v", err), used, nil
}
if collision, reason := netIdx.AddAllocs(allocs); collision {
return false, fmt.Sprintf("reserved alloc port collision: %v", reason), used, nil
Expand Down Expand Up @@ -530,6 +533,10 @@ func ParsePortRanges(spec string) ([]uint64, error) {
if err != nil {
return nil, err
}

if port > MaxValidPort {
return nil, fmt.Errorf("port must be < %d but found %d", MaxValidPort, port)
}
ports[port] = struct{}{}
}
case 2:
Expand Down
Loading

0 comments on commit f998a2b

Please sign in to comment.