Skip to content
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

fix: Simplify provisionerd job acquire #158

Merged
merged 2 commits into from
Feb 4, 2022
Merged

fix: Simplify provisionerd job acquire #158

merged 2 commits into from
Feb 4, 2022

Conversation

kylecarbs
Copy link
Member

This uses a simple channel to detect whether a
job is running or not, and moves all cancels
to be in goroutines.

@kylecarbs kylecarbs self-assigned this Feb 3, 2022
@codecov
Copy link

codecov bot commented Feb 3, 2022

Codecov Report

Merging #158 (62022ae) into main (7884b43) will increase coverage by 0.30%.
The diff coverage is 80.17%.

Impacted file tree graph

@@            Coverage Diff             @@
##             main     #158      +/-   ##
==========================================
+ Coverage   67.57%   67.88%   +0.30%     
==========================================
  Files         101      101              
  Lines        5101     5125      +24     
  Branches       68       68              
==========================================
+ Hits         3447     3479      +32     
+ Misses       1345     1339       -6     
+ Partials      309      307       -2     
Flag Coverage Δ
unittest-go-macos-latest 64.73% <78.44%> (+0.47%) ⬆️
unittest-go-ubuntu-latest 66.31% <73.27%> (-0.09%) ⬇️
unittest-go-windows-latest 63.84% <74.13%> (+0.01%) ⬆️
unittest-js 64.92% <ø> (ø)
Impacted Files Coverage Δ
coderd/workspacehistory.go 53.96% <0.00%> (-1.17%) ⬇️
provisionerd/provisionerd.go 74.27% <80.41%> (+1.63%) ⬆️
coderd/coderd.go 92.95% <100.00%> (+0.10%) ⬆️
coderd/coderdtest/coderdtest.go 100.00% <100.00%> (ø)
coderd/provisionerdaemons.go 49.49% <100.00%> (+2.77%) ⬆️
coderd/provisioners.go 71.05% <100.00%> (ø)
peer/conn.go 78.46% <0.00%> (-1.29%) ⬇️
peerbroker/dial.go 80.95% <0.00%> (+4.76%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 7884b43...62022ae. Read the comment docs.

@kylecarbs kylecarbs force-pushed the provisionrace branch 5 times, most recently from 75def89 to a99ce12 Compare February 3, 2022 22:51
@@ -12,7 +12,7 @@ type ProvisionerJobStatus string

// Completed returns whether the job is still processing.
func (p ProvisionerJobStatus) Completed() bool {
return p == ProvisionerJobStatusSucceeded || p == ProvisionerJobStatusFailed
return p == ProvisionerJobStatusSucceeded || p == ProvisionerJobStatusFailed || p == ProvisionerJobStatusCancelled
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good catch!

@kylecarbs kylecarbs force-pushed the provisionrace branch 2 times, most recently from 5054d16 to 32a065e Compare February 4, 2022 00:16
This uses a simple channel to detect whether a
job is running or not, and moves all cancels
to be in goroutines.
Copy link
Contributor

@bryphe-coder bryphe-coder left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks like some green is showing up!

Changes look good to me 👍

acquiredJobGroup sync.WaitGroup
jobID string
jobMutex sync.Mutex
jobRunning chan struct{}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would a sync.WaitGroup be a better fit here? I saw some tricky races with the initialization of the acquiredJobDone channel here: https://github.com/coder/coder/pull/148/files (specifically - the channel could be created while it was being closed or waited on).

I think a sync.WaitGroup, along with an atomic.Bool to track the running state, could accomplish the same thing

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Answered in slack - sync.WaitGroup has the same issues with the channel approach

@kylecarbs kylecarbs merged commit c65850b into main Feb 4, 2022
@kylecarbs kylecarbs deleted the provisionrace branch February 4, 2022 01:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants