-
Notifications
You must be signed in to change notification settings - Fork 2k
scheduler: reschedule tracker dropped if follow-up fails placement #27129
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
Conversation
c0c1ec4 to
389c765
Compare
In #12319 we attempted to fix a bug where the reschedule tracker could be dropped if the scheduler couldn't place the replacement allocation. This would result in the eventual replacement being reschedulable more than the job author's policy intended. In #5602 we introduced plan normalization, where we reduced the size of the required Raft log entry for plan apply by dropping the job spec from the plan. This required backwards compatibility shims that we intended to remove in Nomad 0.11. It's been long impossible to upgrade to any currently supported version of Nomad from that far back, so I attempted to remove these backwards compatibility shims. But in doing so, this uncovered that the #12319 fix was incorrect. The scheduler test harness used the old code paths, which did not normalize the plans. With normalized plans, we end up dropping the reschedule tracker. This changeset fixes the bug by ensuring that a rescheduled allocation that cannot be placed is not marked with `DesiredStatus: stop`, to match the behavior we see when an evaluation fires before the `reschedule.delay` window expires. This ensures that the plan applier doesn't clear the reschedule tracker because the allocation is terminal. I've also removed the backwards compatibility shims and version checks for plan normalization, and fixed a few test incorrect assertions revealed by the fix. Ref: #12319 Ref: #5602
389c765 to
79e48b6
Compare
allisonlarson
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This lgtm! I was able to follow the removal of the shims, and then the fix for the rescheduling bugs. I appreciate the comments.
| // reschedule and updates the plan to annotate its reschedule tracker and to | ||
| // move it out of the stop list and into the update list so that we don't drop | ||
| // tracking information in the plan applier | ||
| func markFailedToReschedule(plan *structs.Plan, original *structs.Allocation, job *structs.Job) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for this comment! It's really helpful for my understanding
In #12319 we attempted to fix a bug where the reschedule tracker could be dropped if the scheduler couldn't place the replacement allocation. This would result in the eventual replacement being reschedulable more than the job author's policy intended.
In #5602 we introduced plan normalization, where we reduced the size of the required Raft log entry for plan apply by dropping the job spec from the plan. This required backwards compatibility shims that we intended to remove in Nomad 0.11. It's been long impossible to upgrade to any currently supported version of Nomad from that far back, so I attempted to remove these backwards compatibility shims. But in doing so, this uncovered that the #12319 fix was incorrect. The scheduler test harness used the old code paths, which did not normalize the plans. With normalized plans, we end up dropping the reschedule tracker.
This changeset fixes the bug by ensuring that a rescheduled allocation that cannot be placed is not marked with
DesiredStatus: stop, to match the behavior we see when an evaluation fires before thereschedule.delaywindow expires. This ensures that the plan applier doesn't clear the reschedule tracker because the allocation is terminal.I've also removed the backwards compatibility shims and version checks for plan normalization, and fixed a few test incorrect assertions revealed by the fix.
Ref: #12319
Ref: #5602
Testing
To setup a test, run a single node with the node meta
role=foo:Run the following job:
jobspec
Test steps
desired=run)nomad job eval exampleand get no new allocation, as expected.nomad node meta apply -node-id $nodeid role=barto make the node infeasiblenomad job eval exampleand expect no allocAt this point, running
mainwill have the failed alloc showingdesired=stopeven though it hasn't been replaced. The patch here will show it asdesired=runstill and the reschedule tracker'sLastRechedulefield will show "no placement":Next unblock the eval via
nomad node meta apply -node-id $nodeid role=foo. If you runnomad alloc status -json $allocid | jq .RescheduleTrackeryou'll see whether the replacement has a reschedule tracker. Onmainit will be null, whereas with this patch you'll see something like:Contributor Checklist
changelog entry using the
make clcommand.ensure regressions will be caught.
and job configuration, please update the Nomad product documentation, which is stored in the
web-unified-docsrepo. Refer to theweb-unified-docscontributor guide for docs guidelines.Please also consider whether the change requires notes within the upgrade
guide. If you would like help with the docs, tag the
nomad-docsteam in this PR.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.