Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Backport of scheduling: prevent self-collision in dynamic port network offerings into release/1.3.x #16408

Merged

Conversation

hc-github-team-nomad-core
Copy link
Contributor

Backport

This PR is auto-generated from #16401 to be assessed for backporting due to the inclusion of the label backport/1.3.x.

WARNING automatic cherry-pick of commits failed. Commits will require human attention.

The below text is copied from the body of the original PR.


Fixes #9506 #14850 (and several internally-tracked issues)

When the scheduler tries to find a placement for a new allocation, it iterates over a subset of nodes. For each node, we populate a NetworkIndex bitmap with the ports of all existing allocations and any other allocations already proposed as part of this same evaluation via its SetAllocs method. Then we make an "ask" of the NetworkIndex in AssignPorts for any ports we need and receive an "offer" in return. The offer will include both static ports and any dynamic port assignments.

The AssignPorts method was written to support group networks, and it shares code that selects dynamic ports with the original AssignTaskNetwork code. AssignTaskNetwork can request multiple ports from the bitmap at a time. But AssignPorts requests them one at a time and does not account for possible collisions, and doesn't return an error in that case.

What happens next varies:

  1. If the scheduler doesn't place the allocation on that node, the port conflict is thrown away and there's no problem.
  2. If the node is picked and this is the only allocation (or last allocation), the plan applier will reject the plan when it calls SetAllocs, as we'd expect.
  3. If the node is picked and there are additional allocations in the same eval that iterate over the same node, their call to SetAllocs will detect the impossible state and the node will be rejected. This can have the puzzling behavior where a second task group for the job without any networking at all can hit a port collision error!

It looks like this bug has existed since we implemented group networks, but there are several factors that add up to making the issue rare for many users yet frustratingly frequent for others:

  • You're more likely to hit this bug the more tightly packed your range for dynamic ports is. With 12000 ports in the range by default, many clusters can avoid this for a long time.
  • You're more likely to hit case (3) for jobs with lots of allocations or if a scheduler has to iterate over a large number of nodes, such as with system jobs, jobs with spread blocks, or (sometimes) jobs using unique constraints.

For unlucky combinations of these factors, it's possible that case (3) happens repeatedly, preventing scheduling of a given job until a client state change (ex. restarting the agent so all its allocations are rescheduled elsewhere) re-opens the range of dynamic ports available.

This changeset:

  • Fixes the bug by accounting for collisions in dynamic port selection in AssignPorts.
  • Adds test coverage for AssignPorts, expands coverage of this case for the deprecated AssignTaskNetwork, and tightens the dynamic port range in a scheduler test for spread scheduling to more easily detect this kind of problem in the future.
  • Adds a String() method to Bitmap so that any future "screaming" log lines have a human-readable list of used ports.

@hc-github-team-nomad-core hc-github-team-nomad-core force-pushed the backport/pfnr-networking-fix/terminally-new-sunfish branch 2 times, most recently from 8def6d8 to bcc7f2d Compare March 9, 2023 15:10
@hashicorp-cla
Copy link

hashicorp-cla commented Mar 9, 2023

CLA assistant check
All committers have signed the CLA.

…16401)

When the scheduler tries to find a placement for a new allocation, it iterates
over a subset of nodes. For each node, we populate a `NetworkIndex` bitmap with
the ports of all existing allocations and any other allocations already proposed
as part of this same evaluation via its `SetAllocs` method. Then we make an
"ask" of the `NetworkIndex` in `AssignPorts` for any ports we need and receive
an "offer" in return. The offer will include both static ports and any dynamic
port assignments.

The `AssignPorts` method was written to support group networks, and it shares
code that selects dynamic ports with the original `AssignTaskNetwork`
code. `AssignTaskNetwork` can request multiple ports from the bitmap at a
time. But `AssignPorts` requests them one at a time and does not account for
possible collisions, and doesn't return an error in that case.

What happens next varies:

1. If the scheduler doesn't place the allocation on that node, the port
   conflict is thrown away and there's no problem.
2. If the node is picked and this is the only allocation (or last allocation),
   the plan applier will reject the plan when it calls `SetAllocs`, as we'd expect.
3. If the node is picked and there are additional allocations in the same eval
   that iterate over the same node, their call to `SetAllocs` will detect the
   impossible state and the node will be rejected. This can have the puzzling
   behavior where a second task group for the job without any networking at all
   can hit a port collision error!

It looks like this bug has existed since we implemented group networks, but
there are several factors that add up to making the issue rare for many users
yet frustratingly frequent for others:

* You're more likely to hit this bug the more tightly packed your range for
  dynamic ports is. With 12000 ports in the range by default, many clusters can
  avoid this for a long time.
* You're more likely to hit case (3) for jobs with lots of allocations or if a
  scheduler has to iterate over a large number of nodes, such as with system jobs,
  jobs with `spread` blocks, or (sometimes) jobs using `unique` constraints.

For unlucky combinations of these factors, it's possible that case (3) happens
repeatedly, preventing scheduling of a given job until a client state
change (ex. restarting the agent so all its allocations are rescheduled
elsewhere) re-opens the range of dynamic ports available.

This changeset:

* Fixes the bug by accounting for collisions in dynamic port selection in
  `AssignPorts`.
* Adds test coverage for `AssignPorts`, expands coverage of this case for the
  deprecated `AssignTaskNetwork`, and tightens the dynamic port range in a
  scheduler test for spread scheduling to more easily detect this kind of problem
  in the future.
* Adds a `String()` method to `Bitmap` so that any future "screaming" log lines
  have a human-readable list of used ports.
@tgross tgross force-pushed the backport/pfnr-networking-fix/terminally-new-sunfish branch from 9464006 to d328cdc Compare March 9, 2023 15:13
@tgross tgross merged commit 9185051 into release/1.3.x Mar 9, 2023
@tgross tgross deleted the backport/pfnr-networking-fix/terminally-new-sunfish branch March 9, 2023 15:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants