fix: Allow provisionerd to cleanup acquired job #159
Conversation
| if p.isClosed() { | ||
| return | ||
| } |
There was a problem hiding this comment.
So in this case, if provisionerd is killed - we let the job run if it was just acquired?
There was a problem hiding this comment.
Seems like it could make it to runJob at the bottom - would we want it to cancelActiveJob in this case?
There was a problem hiding this comment.
I changed it so cancel or acquire can run at once, so it would properly cancel now!
Codecov Report
@@ Coverage Diff @@
## main #159 +/- ##
==========================================
- Coverage 67.47% 67.14% -0.34%
==========================================
Files 103 103
Lines 5132 5120 -12
Branches 68 68
==========================================
- Hits 3463 3438 -25
- Misses 1359 1371 +12
- Partials 310 311 +1
Continue to review full report at Codecov.
|
kylecarbs
force-pushed
the
provisionerdebug
branch
2 times, most recently
from
97f31fa
to
942795f
Compare
provisionerd/provisionerd.go
Outdated
| @@ -67,26 +66,28 @@ func New(clientDialer Dialer, opts *Options) io.Closer { | |||
| type provisionerDaemon struct { | |||
| opts *Options | |||
|
|
|||
| mutex sync.Mutex | |||
There was a problem hiding this comment.
Cool, I like that you consolidated the closeMutex and jobMutex into a single one - seems simpler!
There was a problem hiding this comment.
I'd suggest adding a comment to state when this lock needs to be acquired, and what it's protecting against, so that people know when to acquire/release this lock
kylecarbs
force-pushed
the
provisionerdebug
branch
2 times, most recently
from
7b5ca01
to
9985ff3
Compare
provisionerd/provisionerd.go
Outdated
| close(p.jobRunning) | ||
| }() | ||
| // It's safe to cast this ProvisionerType. This data is coming directly from coderd. | ||
| provisioner, hasProvisioner := p.opts.Provisioners[job.Provisioner] | ||
| if !hasProvisioner { | ||
| go p.cancelActiveJob(fmt.Sprintf("provisioner %q not registered", job.Provisioner)) | ||
| go p.lockCancelActiveJob(fmt.Sprintf("provisioner %q not registered", job.Provisioner)) |
There was a problem hiding this comment.
given all the call sites that immediately invoke fmt.Sprintf, what do you think about adding a variant (e.g. p.lockCancelActiveJobf(format string, args ...interface{})) that can do this?
kylecarbs
force-pushed
the
provisionerdebug
branch
6 times, most recently
from
1792a01
to
c548133
Compare
If a job is acquired from the database, then provisionerd was killed, the job would be left in an idle state where it was technically in-progress.
kylecarbs
force-pushed
the
provisionerdebug
branch
from
c548133
to
71c6670
Compare
If a job is acquired from the database, then provisionerd was
killed, the job would be left in an idle state where it was
technically in-progress.