-
Notifications
You must be signed in to change notification settings - Fork 1.9k
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
scheduler/reconcile: set FollowupEvalID on lost stop_after_client_disconnect #8105
Conversation
@schmichael The |
scheduler/reconcile.go
Outdated
@@ -910,17 +923,14 @@ func (a *allocReconciler) handleDelayedReschedulesImpl(rescheduleLater []*delaye | |||
} | |||
|
|||
// Create in-place updates for every alloc ID that needs to be updated with its follow up eval ID | |||
for allocID, evalID := range allocIDToFollowupEvalID { | |||
if createUpdates { | |||
if createUpdates { |
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 change just reverts the previous commit
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.
provisional lgtm - but I'll do more manual testing to understand that flow more.
alloc: alloc, | ||
clientStatus: clientStatus, | ||
statusDescription: statusDescription, | ||
followupEvalID: followupEvals[alloc.ID], |
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.
Noticed that handleDelayedReschedulesImpl
may return nil. Does function need to handle nil followupEvals
? Also, maybe markDelayedStop
is a better descriptive name for this function?
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, I do need a nil check there. I re(de?)factored handleDelayedReschedulesImpl
, leaving just the two functions designed to be called: handleDelayedReschedules
and handleDelayedLost
in 314fffd.
scheduler/reconcile.go
Outdated
@@ -304,12 +304,17 @@ func (a *allocReconciler) markStop(allocs allocSet, clientStatus, statusDescript | |||
// markDelayed does markStop, but optionally includes a FollowupEvalID so that we can update | |||
// the stopped alloc with its delayed rescheduling evalID | |||
func (a *allocReconciler) markDelayed(allocs allocSet, clientStatus, statusDescription string, followupEvals map[string]string) { | |||
var e string |
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.
I think this will be easier to reason about if the e
is inside the loop. I'd fear that if an alloc doesn't have a follow up eval associated with it (not sure if that's expected, or due to a bug), the result will contain follow up evals of a previous element in list.
Also, an alternative to initialize followUpEvals
to an empty map if nil, and you avoid the conditionals inside the loop.
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.
I think empty initialization is correct, you're right about assigning the wrong followupEval
to the allocation, only some allocations will have keys in the map. thanks, fixed.
…connect (#8105) * scheduler/reconcile: set FollowupEvalID on lost stop_after_client_disconnect * scheduler/reconcile: thread follupEvalIDs through to results.stop * scheduler/reconcile: comment typo * nomad/_test: correct arguments for plan.AppendStoppedAlloc * scheduler/reconcile: avoid nil, cleanup handleDelayed(Lost|Reschedules)
…connect (#8105) (#8138) * scheduler/reconcile: set FollowupEvalID on lost stop_after_client_disconnect * scheduler/reconcile: thread follupEvalIDs through to results.stop * scheduler/reconcile: comment typo * nomad/_test: correct arguments for plan.AppendStoppedAlloc * scheduler/reconcile: avoid nil, cleanup handleDelayed(Lost|Reschedules)
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. |
FollowupEvalID
is used by the CLI to display the delayed eval. #8098