-
Notifications
You must be signed in to change notification settings - Fork 2k
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
Fix in-place updates over ineligible nodes #12264
Conversation
Hi @jorgemarey! Sorry for the delay on reviewing this. We had an internal discussion over this about whether or not this was actually the expected behavior, and I think we've come to the conclusion that what you're looking for is correct. I need to review how we handle this case in the scheduler to make sure we're not missing anything, but once I've done so I'll come back to review this PR in detail. Thanks for your patience! |
386978f
to
4be1ce8
Compare
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.
LGTM. I've added a changelog entry and rebased on main so we can ship in the upcoming Nomad 1.3.0
I need to review how we handle this case in the scheduler to make sure we're not missing anything, but once I've done so I'll come back to review this PR in detail.
I've had a chance to follow-up on that. The scheduler removes ineligible nodes from the "stack" when we do feasibility checking in the function readyNodesInDCs
, which in turn calls (*Node) Ready()
and that checks the eligibility. The eligibility of nodes for existing allocations isn't considered at all in the scheduler, which is how we end up with those in-place alloc updates landing on the plan applier as you've noted here.
I'm going to lock this pull request because it has been closed for 120 days ⏳. This helps our maintainers find and focus on the active contributions. |
Fixes #12263