-
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
drainv2: Job Watcher Testing #3961
Conversation
Chan must be buffered to avoid skipping triggering altogether Also made timing in a test a bit more lenient
They must be synchronous or they'll spin in a tight loop
75c206d
to
d360047
Compare
d360047
to
d18d2d7
Compare
nomad/drainer/watch_jobs.go
Outdated
@@ -90,21 +90,28 @@ func NewDrainingJobWatcher(ctx context.Context, limiter *rate.Limiter, state *st | |||
} | |||
|
|||
// RegisterJob marks the given job as draining and adds it to being watched. | |||
func (w *drainingJobWatcher) RegisterJob(job structs.JobNs) { | |||
func (w *drainingJobWatcher) RegisterJobs(jobs []structs.JobNs) { |
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.
We should switch this to NamespacedID. I didn't see it originally. We should add the String() method tho
nomad/drainer/watch_jobs.go
Outdated
@@ -184,7 +190,7 @@ func (w *drainingJobWatcher) watch() { | |||
|
|||
// Lookup the job | |||
job, err := w.state.JobByID(nil, jns.Namespace, jns.ID) | |||
if err != nil { | |||
if err != nil || job == nil { |
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.
Hmm if the job is nil shouldn't we do a deregisterJob
. That would be the job stop purge case
nomad/drainer/watch_jobs.go
Outdated
@@ -390,10 +397,13 @@ func handleTaskGroup(snap *state.StateSnapshot, tg *structs.TaskGroup, | |||
numToDrain := healthy - thresholdCount | |||
numToDrain = helper.IntMin(len(drainable), numToDrain) | |||
if numToDrain <= 0 { | |||
fmt.Printf("------- Not draining any allocs\n") | |||
fmt.Printf("------- Not draining any allocs: drainable:%d healthy:%d thresholdCount:%d\n", |
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.
Remove these at this point? If still being used ignore
nomad/drainer/watch_jobs_test.go
Outdated
|
||
// assertOps asserts how many allocs should be drained and migrated. | ||
// The drains and migrations - if any - are returned. | ||
assertOps := func(drained, migrated int) (drains *DrainRequest, migrations []*structs.Allocation) { |
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.
Pull this out into a helper
Also drop the New func as it's easy to swap the order of arguments since they're both strings.
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. |
No description provided.