scheduler: prevent extra reschedules from invalid states - #28445
Merged
Merged
Conversation
tgross
added
type/bug
theme/scheduling
theme/restart/reschedule
theme/disconnect
backport/ent/1.10.x+ent
While investigating a customer incident, we uncovered several states within the scheduler that should be impossible inputs, but that the scheduler handled badly and created potentially many extra allocations on successive evals. Note that these issues don't appear to be reachable bugs in current versions of Nomad. Place guardrails around these states: * Ensure that the `updateByReschedulable` only allows force-rescheduling for client-terminal allocations. This is currently guarded in the RPC handler but not in Raft. We're currently protected from write skew by making either the client view or server view canonical for a given field. * Ensure that only valid `AllocState` entries are added to a stopped allocation, to ensure there's no code path where we can reschedule an unknown allocation multiple times by having it no longer show as unknown when we check for reconnect. Ref: https://hashicorp.atlassian.net/browse/NMD-1681
tgross
force-pushed
the
b-extraneous-reschedules
branch
from
August 20, 2026 19:44
5f3c190 to
0685d19
Compare
tgross
commented
Aug 20, 2026
Comment on lines
+1
to
+3
| ```release-note:improvement | ||
| scheduler: Improved robustness of reschedule logic for invalid states | ||
| ``` |
Member
Author
There was a problem hiding this comment.
Note to reviewers: I'm only calling this "improvement" because I don't have a concrete reachable user-facing bug to describe. It's a bug for sure internally, so that's why I'm marking it for backports. 🤷
8 tasks
tgross
added a commit
that referenced
this pull request
Aug 21, 2026
In #19937 we attempted to fix a potential state store corruption by copying allocations we were upserting before canonicalizing them. But while I was working on #28445 I noticed that we weren't actually updating the slice that we'd then insert into the state store, which means we're throwing away the canonicalized version. This was obscured by several state store tests that were asserting that the allocation being passed to `UpsertPlanResults` was equal to the one we return from `AllocByID`. This is obviously incorrect because we modify new allocations coming in from the plan quite a bit. Rework these tests to clarify their intent and reduce duplication between tests. Ref: #19937 Ref: #28445
tgross
marked this pull request as ready for review
August 21, 2026 01:24
jrasell
approved these changes
Aug 21, 2026
Merged
8 tasks
tgross
added a commit
that referenced
this pull request
Aug 31, 2026
In #19937 we attempted to fix a potential state store corruption by copying allocations we were upserting before canonicalizing them. But while I was working on #28445 I noticed that we weren't actually updating the slice that we'd then insert into the state store, which means we're throwing away the canonicalized version. This was obscured by several state store tests that were asserting that the allocation being passed to `UpsertPlanResults` was equal to the one we return from `AllocByID`. This is obviously incorrect because we modify new allocations coming in from the plan quite a bit. Rework these tests to clarify their intent and reduce duplication between tests. Ref: #19937 Ref: #28445
tgross
added a commit
that referenced
this pull request
Sep 1, 2026
In #19937 we attempted to fix a potential state store corruption by copying allocations we were upserting before canonicalizing them. But while I was working on #28445 I noticed that we weren't actually updating the slice that we'd then insert into the state store, which means we're throwing away the canonicalized version. As it turns out, the original reasoning behind #19937 was faulty and we shouldn't have been copying at all. These allocations are always coming from the `Plan.Submit` RPC and not from the state store. And we send the mutated version back in the response to the RPC. This means that we haven't been upgrading allocations or canonicalizing them on plan submit, but in practice it also hasn't mattered. Canonicalizing populates missing resources from deprecated resources, for the case where we have pre-0.11 schedulers running. And it recursively canonicalizes the `Job`, which comes over the wire as nil in the plan and gets populated on the leader anyways. So fixing this has no production impact today but keeps our behavior correct the next time we need to upgrade allocations. Ref: #19937 Ref: #28445 Ref: https://hashicorp.atlassian.net/browse/NMD-1681
tgross
added a commit
that referenced
this pull request
Sep 2, 2026
In #19937 we attempted to fix a potential state store corruption by copying allocations we were upserting before canonicalizing them. But while I was working on #28445 I noticed that we weren't actually updating the slice that we'd then insert into the state store, which means we're throwing away the canonicalized version. As it turns out, the original reasoning behind #19937 was faulty and we shouldn't have been copying at all. These allocations are always coming from the `Plan.Submit` RPC and not from the state store. And we send the mutated version back in the response to the RPC. This means that we haven't been upgrading allocations or canonicalizing them on plan submit, but in practice it also hasn't mattered. Canonicalizing populates missing resources from deprecated resources, for the case where we have pre-0.11 schedulers running. And it recursively canonicalizes the `Job`, which comes over the wire as nil in the plan and gets populated on the leader anyways. So fixing this has no production impact today but keeps our behavior correct the next time we need to upgrade allocations. Ref: #19937 Ref: #28445 Ref: https://hashicorp.atlassian.net/browse/NMD-1681
Merged
8 tasks
schmichael
pushed a commit
to vercel/nomad
that referenced
this pull request
Sep 25, 2026
…8445) While investigating a customer incident, we uncovered several states within the scheduler that should be impossible inputs, but that the scheduler handled badly and created potentially many extra allocations on successive evals. Note that these issues don't appear to be reachable bugs in current versions of Nomad. Place guardrails around these states: * Ensure that the `updateByReschedulable` only allows force-rescheduling for client-terminal allocations. This is currently guarded in the RPC handler but not in Raft. We're currently protected from write skew by making either the client view or server view canonical for a given field. * Ensure that only valid `AllocState` entries are added to a stopped allocation, to ensure there's no code path where we can reschedule an unknown allocation multiple times by having it no longer show as unknown when we check for reconnect. Ref: https://hashicorp.atlassian.net/browse/NMD-1681
schmichael
pushed a commit
to vercel/nomad
that referenced
this pull request
Sep 25, 2026
…rp#28447) In hashicorp#19937 we attempted to fix a potential state store corruption by copying allocations we were upserting before canonicalizing them. But while I was working on hashicorp#28445 I noticed that we weren't actually updating the slice that we'd then insert into the state store, which means we're throwing away the canonicalized version. As it turns out, the original reasoning behind hashicorp#19937 was faulty and we shouldn't have been copying at all. These allocations are always coming from the `Plan.Submit` RPC and not from the state store. And we send the mutated version back in the response to the RPC. This means that we haven't been upgrading allocations or canonicalizing them on plan submit, but in practice it also hasn't mattered. Canonicalizing populates missing resources from deprecated resources, for the case where we have pre-0.11 schedulers running. And it recursively canonicalizes the `Job`, which comes over the wire as nil in the plan and gets populated on the leader anyways. So fixing this has no production impact today but keeps our behavior correct the next time we need to upgrade allocations. Ref: hashicorp#19937 Ref: hashicorp#28445 Ref: https://hashicorp.atlassian.net/browse/NMD-1681
This branch was successfully deployed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
While investigating a customer incident, we uncovered several states within the scheduler that should be impossible inputs, but that the scheduler handled badly and created potentially many extra allocations on successive evals. Note that these issues don't appear to be reachable bugs in current versions of Nomad.
Place guardrails around these states:
Ensure that the
updateByReschedulableonly allows force-rescheduling for client-terminal allocations. This is currently guarded in the RPC handler but not in Raft. We're currently protected from write skew by making either the client view or server view canonical for a given field.Ensure that only valid
AllocStateentries are added to a stopped allocation, to ensure there's no code path where we can reschedule an unknown allocation multiple times by having it no longer show as unknown when we check for reconnect.Ref: https://hashicorp.atlassian.net/browse/NMD-1681
Contributor Checklist
changelog entry using the
make clcommand.ensure regressions will be caught.
Reviewer Checklist
backporting document.
in the majority of situations. The main exceptions are long-lived feature branches or merges where
history should be preserved.
within the public repository.
Changes to Security Controls
Are there any changes to security controls (access controls, encryption, logging) in this pull request? If so, explain.