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.

Sign up for GitHub

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

scheduler: fix a bug where we subtract reserved node resources twice #23386

Merged
merged 4 commits into from
Jun 21, 2024
Merged

scheduler: fix a bug where we subtract reserved node resources twice #23386

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
a0a7173
scheduler: fix a bug where we subtract reserved node resources twice
pkazmierczak Jun 19, 2024
e0dcac3
nodeResources.Comparable() should in fact return total resources
pkazmierczak Jun 19, 2024
10131ea
cl
pkazmierczak Jun 19, 2024
a73965f
mocked Node correction
pkazmierczak Jun 21, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 3 additions & 0 deletions .changelog/23386.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,3 @@
```release-note:bug
scheduler: Fix a bug where reserved resources are not calculated correctly
```
1 change: 1 addition & 0 deletions nomad/structs/funcs_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -105,6 +105,7 @@ func node2k() *Node {
Grade: numalib.Performance,
BaseSpeed: 1000,
}},
OverrideWitholdCompute: 1000, // set by client reserved field
},
},
Memory: NodeMemoryResources{
Expand Down
2 changes: 1 addition & 1 deletion nomad/structs/structs.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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()),
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

OverrideWitholdCompute: withheld,

This field was missing from the mocked node in our unit tests. After adding it, tests fail without this PR's change.

ReservedCores: reservableCores,
},
Memory: AllocatedMemoryResources{
Expand Down
Loading