-
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
server: stop after client disconnect #7939
Conversation
Meta map[string]string | ||
Services []*Service | ||
ShutdownDelay *time.Duration `mapstructure:"shutdown_delay"` | ||
StopAfterClientDisconnect *time.Duration `mapstructure:"stop_after_client_disconnect"` |
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 is the only new field, the rest are whitespace differences
3d65429
to
97cf331
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.
Mostly LGTM. The tests are great!
If we can get rid of that stringly-typed field I think this would be solid.
return true | ||
} | ||
|
||
// WaitClientStop uses the reschedule delay mechanism to block rescheduling until |
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 forget whether this was in the RFC or not, but either way this is a good catch. We would have had some really wild behavior without this.
Also, like how this API gives us a time.Time
rather than ticking the clock over; not having to wait in tests is 👍
nomad/structs/structs.go
Outdated
@@ -6512,6 +6512,13 @@ func (t *Template) Warnings() error { | |||
return mErr.ErrorOrNil() | |||
} | |||
|
|||
// AllocState records a single event that changes the state of the whole allocation | |||
type AllocState struct { | |||
Field 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.
There doesn't seem to be any value in use for Field
other than "ClientStatus"
. I imagine this was intended for future extensibility. But given we'll always control the values, I think it'd be better to have it be an enum-style const so that we have type-checking around this field?
1. a global AllocStates to track status changes with timestamps. We need this to track the time at which the alloc became lost originally. 2. ShouldClientStop() and WaitClientStop() to actually do the math
This was setup to only update allocs to lost if the DesiredStatus had already been set by the scheduler. It seems like the intention was to update the status from any non-terminal state, and not all lost allocs have been marked stop or evict by now
7b7c3f4
to
a53af87
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'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. |
stop after client disconnected
part 2, the scheduling side. See also #7793
before marking the node as down
safe to just treat them as lost
closes #2185