scheduler: fix a bug where we subtract reserved node resources twice #23386
Conversation
pkazmierczak
added
backport/ent/1.7.x+ent
| @@ -3191,7 +3191,7 @@ func (n *NodeResources) Comparable() *ComparableResources { | |||
| c := &ComparableResources{ | |||
| Flattened: AllocatedTaskResources{ | |||
| Cpu: AllocatedCpuResources{ | |||
| CpuShares: int64(n.Processors.Topology.UsableCompute()), | |||
| CpuShares: int64(n.Processors.Topology.TotalCompute()), | |||
There was a problem hiding this comment.
The comment in the caller in AllocsFit seems to agree that this should be TotalCompute:
// Check that the node resources (after subtracting reserved) are a
// super set of those that are being allocated
available := node.NodeResources.Comparable()
available.Subtract(node.ReservedResources.Comparable())
if superset, dimension := available.Superset(used); !superset {
return false, dimension, used, nil
}But it bothers me that changing this wouldn't also cause some tests to fail. Are we missing tests on AllocsFit that exercise this code path?
There was a problem hiding this comment.
Right, so this is very tricky to test.
I found this bug while chasing AllocsFit and initially just removed the available.Subtract(node.ReservedResources.Comparable()) line, because I thought: ah, this just subtracts what's already been subtracted. It solves the issue of course, but making this change is what breaks a lot of tests in the plan applier and scheduler. The reason is: we mock nodes and their resources. The only way to properly test this is an e2e test, but that's also tricky: we'd have to create a client with reserved resources, run a job that requests all the remaining available resources and make sure it succeeds. To do that, we'd have to manipulate client config in the e2e (that is tricky), and dynamically create the jobspec based on the available resources of the e2e node. Can be done, but it's a lot of work I think. Unless I'm missing something?
There was a problem hiding this comment.
I suspect we could set reserved resources on the mocked nodes on some of those existing scheduler and plan applier tests such that they pass with this change and fail without it.
There was a problem hiding this comment.
ok. finally found the culprit. every node that has reserved resources set by client conf has an additional field OverrideWitholdCompute set in its Topology on "legacy" systems:
nomad/nomad/structs/cpucompat_generic.go
Line 45 in a73965f
This field was missing from the mocked node in our unit tests. After adding it, tests fail without this PR's change.
Fixes a bug in the
nodeResources.Comparablemethod, where CPU resources were accidentally offset with reserved resources, whereas functions that use this field expect total CPU resources.Fixes #23371 and #23314