reconciler: support disconnected clients #12058
Merged
Conversation
This file contains 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
DerekStrickland
force-pushed
the
f-reconciler-disconnected-clients
branch
from
9dde429 to
c7bbf74
Compare
DerekStrickland
commented
Feb 14, 2022
DerekStrickland
commented
Feb 14, 2022
tgross
reviewed
Feb 14, 2022
added 7 commits
February 14, 2022 10:00
DerekStrickland
force-pushed
the
f-reconciler-disconnected-clients
branch
from
4ea42c6 to
6cd52b0
Compare
tgross
reviewed
Feb 14, 2022
There was a problem hiding this comment.
Hi @DerekStrickland! This is looking great for a first pass at some gnarly scheduler code. A bunch of my comments are about docstrings, readability, testing, etc.
I've got a couple of correctness confusions around:
- How we're using
MaxNormScore()incomputeStopByReconnecting - Confusing conditionals in
filterByTaintedwhen tainted nodes are nil that might be obscuring the right behavior - Batch creation in
createTimeoutLaterEvals
DerekStrickland
commented
Feb 15, 2022
Co-authored-by: Tim Gross <[email protected]>
tgross
reviewed
Feb 16, 2022
| // DisconnectTimeout uses the MaxClientDisconnect to compute when the allocation | ||
| // should transition to lost. | ||
| func (a *Allocation) DisconnectTimeout(now time.Time) time.Time { | ||
| tg := a.Job.LookupTaskGroup(a.TaskGroup) |
There was a problem hiding this comment.
Can a.Job ever be nil? I know we set it that way in some circumstances.
There was a problem hiding this comment.
Updated in next push.
tgross
reviewed
Feb 16, 2022
tgross
reviewed
Feb 16, 2022
tgross
reviewed
Feb 16, 2022
tgross
approved these changes
Feb 16, 2022
Co-authored-by: Tim Gross <[email protected]>
DerekStrickland
added a commit
that referenced
this pull request
Apr 5, 2022
* Add merge helper for string maps * structs: add statuses, MaxClientDisconnect, and helper funcs * taintedNodes: Include disconnected nodes * upsertAllocsImpl: don't use existing ClientStatus when upserting unknown * allocSet: update filterByTainted and add delayByMaxClientDisconnect * allocReconciler: support disconnecting and reconnecting allocs * GenericScheduler: upsert unknown and queue reconnecting Co-authored-by: Tim Gross <[email protected]>
DerekStrickland
added a commit
that referenced
this pull request
Apr 6, 2022
* Add merge helper for string maps * structs: add statuses, MaxClientDisconnect, and helper funcs * taintedNodes: Include disconnected nodes * upsertAllocsImpl: don't use existing ClientStatus when upserting unknown * allocSet: update filterByTainted and add delayByMaxClientDisconnect * allocReconciler: support disconnecting and reconnecting allocs * GenericScheduler: upsert unknown and queue reconnecting Co-authored-by: Tim Gross <[email protected]>
|
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. |
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
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.
Description
This PR is a work in progress tracking against the long-running feature branch
f-disconnected-client-handling.It includes:
NodeStatusDisconnected), new client status (AllocClientStatusUnknown), and a new eval trigger (EvalTriggerMaxDisconnectTimeout)taintedNodesfunction to include disconnected nodes as part of the tainted setMaxClientDisconnectfield on theAllocationstruct for tracking how long an alloc is allowed to be disconnectedallocReconcilerchanges to support allocs that can disconnect and reconnect without restartingGenericSchedulerchanges to add disconnects to theNodeAllocationset for upsert with new statusAllocClientStatusUnknownGenericSchedulerchanges to add reconnects toqueuedAllocations.StateStorealloc upsert code so that allocs with the unknown status don't have theirClientStatusoverwritten by the existing allocsClientStatus