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

core: optimize AllocsFit to minimize calculation #9284

Draft
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

schmichael
Copy link
Member

@schmichael schmichael commented Nov 5, 2020

AllocsFit is called from the plan applier and binpacking iterator to
ensure planned/proposed allocs fit on a node. While it always compares
mem/cpu resources, it does not always need to check devices and
networks.

  • Plan Applier

    • Before this change the plan applier would always check devices and
      networks for collisions.

    • After this change the plan applier will only checks devices and
      networks for collisions if the plan's job contains devices and
      networks. There's no possibility of a collision if there are no new
      device or network reservations.

  • Binpack Iterator

    • Before this change the binpack iterator would never check ports
      or devices because those feasibility checks are handled elsewhere.
      Bandwidth was checked but hardcoded to pass since Nomad 0.12.

    • The behavior has not changed, but the code is much cleaner. Before a
      network index would be passed to AllocsFit, but only its bandwidth
      would be checked. Since bandwidth is deprecated, this check was a
      noop. All of that code is removed so it no longer appears that
      AllocsFit is checking for network collisions when it does not.

TODO

  • Tests!
  • Benchmark
  • NetworkIndex usage could probably still be improved.

AllocsFit is called from the plan applier and binpacking iterator to
ensure planned/proposed allocs fit on a node. While it always compares
mem/cpu resources, it does not always need to check devices and
networks.

- Plan Applier

  - Before this change the plan applier would *always* check devices and
    networks for collisions.

  - After this change the plan applier will only checks devices and
    networks for collisions if the plan's job contains devices and
    networks. There's no possibility of a collision if there are no new
    device or network reservations.

- Binpack Iterator

  - Before this change the binpack iterator would never check ports
    or devices because those feasibility checks are handled elsewhere.
    Bandwidth was checked but hardcoded to pass since Nomad 0.12.

  - The behavior has not changed, but the code is much cleaner. Before a
    network index would be passed to AllocsFit, but only its bandwidth
    would be checked. Since bandwidth is deprecated, this check was a
    noop. All of that code is removed so it no longer appears that
    AllocsFit is checking for network collisions when it does not.
@schmichael schmichael requested a review from nickethier November 5, 2020 23:40
@dadgar
Copy link
Contributor

dadgar commented Nov 7, 2020

@schmichael May be nice to add a benchmark to this so before and after can be compared

Base automatically changed from master to main March 8, 2021 19:25
@hashicorp-cla
Copy link

hashicorp-cla commented Mar 12, 2022

CLA assistant check
All committers have signed the CLA.

@nickethier nickethier removed their request for review June 29, 2023 13:32
@tgross tgross added the stage/needs-rebase This PR needs to be rebased on main before it can be backported to pick up new BPA workflows label May 17, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
stage/needs-rebase This PR needs to be rebased on main before it can be backported to pick up new BPA workflows
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants