feat: Add provisionerdaemon to coderd #141
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
These will be used for streaming logs, checking status, and other operations related to workspace and project history.
Nesting all structs behind their respective structures is leaky, and promotes naming conflicts between handlers. Our HTTP routes cannot have conflicts, so neither should function naming.
Codecov Report
@@ Coverage Diff @@
## main #141 +/- ##
==========================================
+ Coverage 64.92% 67.43% +2.50%
==========================================
Files 55 101 +46
Lines 633 5098 +4465
Branches 68 68
==========================================
+ Hits 411 3438 +3027
- Misses 210 1349 +1139
- Partials 12 311 +299
Continue to review full report at Codecov.
|
kylecarbs
force-pushed
the
provisionerdaemon
branch
from
6606e01
to
0a1623f
Compare
bryphe-coder
reviewed
Feb 2, 2022
bryphe-coder
reviewed
Feb 2, 2022
Comment on lines
+29
to
+31
| CancelledAt *time.Time `json:"canceled_at,omitempty"` | ||
| CompletedAt *time.Time `json:"completed_at,omitempty"` |
There was a problem hiding this comment.
Curious why both CancelledAt and CompletedAt are needed? I'm thinking there could be just FinishedAt - and it would be the completion time on success, and cancel time if cancelled.
bryphe-coder
reviewed
Feb 2, 2022
| ProvisionerJobStatusPending ProvisionerJobStatus = "pending" | ||
| ProvisionerJobStatusRunning ProvisionerJobStatus = "running" | ||
| ProvisionerJobStatusSucceeded ProvisionerJobStatus = "succeeded" | ||
| ProvisionerJobStatusFailed ProvisionerJobStatus = "failed" |
There was a problem hiding this comment.
Should there be a state for cancelled?
There was a problem hiding this comment.
I suppose I'm not sure. I felt like failed would represent cancelled, but I suppose that's a different state. I'm going to add it in!
bryphe-coder
reviewed
Feb 2, 2022
bryphe-coder
reviewed
Feb 2, 2022
| WorkspaceID uuid.UUID `json:"workspace_id"` | ||
| ProjectHistoryID uuid.UUID `json:"project_history_id"` | ||
| BeforeID uuid.UUID `json:"before_id"` | ||
| AfterID uuid.UUID `json:"after_id"` | ||
| Transition database.WorkspaceTransition `json:"transition"` | ||
| Initiator string `json:"initiator"` | ||
| Provision ProvisionerJob `json:"provision"` |
There was a problem hiding this comment.
Cool, looks like this type w/ Provision now has most of the stuff I need to power the workspaces timeline in my mock!
bryphe-coder
reviewed
Feb 2, 2022
| @@ -22,13 +23,13 @@ type WorkspaceHistory struct { | |||
| ID uuid.UUID `json:"id"` | |||
| CreatedAt time.Time `json:"created_at"` | |||
| UpdatedAt time.Time `json:"updated_at"` | |||
| CompletedAt time.Time `json:"completed_at"` | |||
| WorkspaceID uuid.UUID `json:"workspace_id"` | |||
| ProjectHistoryID uuid.UUID `json:"project_history_id"` | |||
| BeforeID uuid.UUID `json:"before_id"` | |||
| AfterID uuid.UUID `json:"after_id"` | |||
| Transition database.WorkspaceTransition `json:"transition"` | |||
| Initiator string `json:"initiator"` | |||
There was a problem hiding this comment.
Not an issue with this PR, but I worry that this schema will make it difficult for a user to change names. That username-change will have to percolate through all these places we're storing their name.
There was a problem hiding this comment.
This is a user ID. It's not very clear, so I should add a comment.
We can't use the uuid.UUID type, because we have to support v1 users.
bryphe-coder
approved these changes
Feb 2, 2022
kylecarbs
force-pushed
the
provisionerdaemon
branch
3 times, most recently
from
cb2733b
to
3e1e60c
Compare
kylecarbs
force-pushed
the
provisionerdaemon
branch
7 times, most recently
from
7eb5519
to
319f640
Compare
kylecarbs
force-pushed
the
provisionerdaemon
branch
7 times, most recently
from
53802fb
to
f26bdbb
Compare
kylecarbs
force-pushed
the
provisionerdaemon
branch
from
f26bdbb
to
2a76b60
Compare
There is a race condition in the interop between the websocket and `dRPC`: https://github.com/coder/coder/runs/5038545709?check_suite_focus=true#step:7:117 - it seems both the websocket and dRPC feel like they own the `byte[]` being sent between them. This can lead to data races, in which both `dRPC` and the websocket are writing. This is just tracking some experimentation to fix that race condition ## Run results: ## - Run 1: peer test failure - Run 2: peer test failure - Run 3: `TestWorkspaceHistory/CreateHistory` - https://github.com/coder/coder/runs/5040858460?check_suite_focus=true#step:8:45 ``` status code 412: The provided project history is running. Wait for it to complete importing!` ``` - Run 4: `TestWorkspaceHistory/CreateHistory` - https://github.com/coder/coder/runs/5040957999?check_suite_focus=true#step:7:176 ``` workspacehistory_test.go:122: Error Trace: workspacehistory_test.go:122 Error: Condition never satisfied Test: TestWorkspaceHistory/CreateHistory ``` - Run 5: peer failure - Run 6: Pass ✅ - Run 7: Peer failure ## Open Questions: ## ### Is `dRPC` or `websocket` at fault for the data race? It looks like this condition is specifically happening when `dRPC` decides to [`SendError`]). This constructs a new byte payload from [`MarshalError`](https://github.com/storj/drpc/blob/f6e369438f636b47ee788095d3fc13062ffbd019/drpcwire/error.go#L15) - so `dRPC` has created this buffer and owns it. From `dRPC`'s perspective, the callstack looks like this: - [`sendPacket`](https://github.com/storj/drpc/blob/f6e369438f636b47ee788095d3fc13062ffbd019/drpcstream/stream.go#L253) - [`writeFrame`](https://github.com/storj/drpc/blob/f6e369438f636b47ee788095d3fc13062ffbd019/drpcwire/writer.go#L65) - [`AppendFrame`](https://github.com/storj/drpc/blob/f6e369438f636b47ee788095d3fc13062ffbd019/drpcwire/packet.go#L128) - with finally the data race happening here: ```go // AppendFrame appends a marshaled form of the frame to the provided buffer. func AppendFrame(buf []byte, fr Frame) []byte { ... out := buf out = append(out, control). // <--------- ``` This should be fine, since `dPRC` create this buffer, and is taking the byte buffer constructed from `MarshalError` and tacking a bunch of headers on it to create a proper frame. Once `dRPC` is done writing, it _hangs onto the buffer and resets it here__: https://github.com/storj/drpc/blob/f6e369438f636b47ee788095d3fc13062ffbd019/drpcwire/writer.go#L73 However... the websocket implementation, once it gets the buffer, it runs a `statelessDeflate` [here](https://github.com/nhooyr/websocket/blob/8dee580a7f74cf1713400307b4eee514b927870f/write.go#L180), which compresses the buffer on the fly. This functionality actually [mutates the buffer in place](https://github.com/klauspost/compress/blob/a1a9cfc821f00faf2f5231beaa96244344d50391/flate/stateless.go#L94), which is where get our race. In the case where the `byte[]` aren't being manipulated anywhere else, this compress-in-place operation would be safe, and that's probably the case for most over-the-wire usages. In this case, though, where we're plumbing `dRPC` -> websocket, they both are manipulating it (`dRPC` is reusing the buffer for the next `write`, and `websocket` is compressing on the fly). ### Why does cloning on `Read` fail? Get a bunch of errors like: ``` 2022/02/02 19:26:10 [WARN] yamux: frame for missing stream: Vsn:0 Type:0 Flags:0 StreamID:0 Length:0 2022/02/02 19:26:25 [ERR] yamux: Failed to read header: unexpected EOF 2022/02/02 19:26:25 [ERR] yamux: Failed to read header: unexpected EOF 2022/02/02 19:26:25 [WARN] yamux: frame for missing stream: Vsn:0 Type:0 Flags:0 StreamID:0 Length:0 ``` # UPDATE: We decided we could disable websocket compression, which would avoid the race because the in-place `deflate` operaton would no longer be run. Trying that out now: - Run 1: ✅ - Run 2: https://github.com/coder/coder/runs/5042645522?check_suite_focus=true#step:8:338 - Run 3: ✅ - Run 4: https://github.com/coder/coder/runs/5042988758?check_suite_focus=true#step:7:168 - Run 5: ✅
Found another data race while running the tests: https://github.com/coder/coder/runs/5044320845?check_suite_focus=true#step:7:83 __Issue:__ There is a race in the p.acquiredJobDone chan - in particular, there can be a case where we're waiting on the channel to finish (in close) with <-p.acquiredJobDone, but in parallel, an acquireJob could've been started, which would create a new channel for p.acquiredJobDone. There is a similar race in `close(..)`ing the channel, which also came up in test runs. __Fix:__ Instead of recreating the channel everytime, we can use `sync.WaitGroup` to accomplish the same functionality - a semaphore to make close wait for the current job to wrap up.
This is an attempted fix for failures like: https://github.com/coder/coder/runs/5043435263?check_suite_focus=true#step:7:32 Looking at the timing of the test: ``` t.go:56: 2022-02-02 21:33:21.964 [DEBUG] (terraform-provisioner) <provision.go:139> ran apply t.go:56: 2022-02-02 21:33:21.991 [DEBUG] (provisionerd) <provisionerd.go:162> skipping acquire; job is already running t.go:56: 2022-02-02 21:33:22.050 [DEBUG] (provisionerd) <provisionerd.go:162> skipping acquire; job is already running t.go:56: 2022-02-02 21:33:22.090 [DEBUG] (provisionerd) <provisionerd.go:162> skipping acquire; job is already running t.go:56: 2022-02-02 21:33:22.140 [DEBUG] (provisionerd) <provisionerd.go:162> skipping acquire; job is already running t.go:56: 2022-02-02 21:33:22.195 [DEBUG] (provisionerd) <provisionerd.go:162> skipping acquire; job is already running t.go:56: 2022-02-02 21:33:22.240 [DEBUG] (provisionerd) <provisionerd.go:162> skipping acquire; job is already running workspacehistory_test.go:122: Error Trace: workspacehistory_test.go:122 Error: Condition never satisfied Test: TestWorkspaceHistory/CreateHistory ``` It appears that the `terraform apply` job had just finished - with less than a second to spare until our `require.Eventually` completes - but there's still work to be done (ie, collecting the state files). So my suspicion is that terraform might, in some cases, exceed our 5s timeout. Note that in the setup for this test - there is a similar project history wait that waits for 15s, so I borrowed that here. In the future - we can look at potentially using a simple echo provider to exercise this in the unit test, in a way that is more reliable in terms of timing. I'll log an issue to track that.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Integrates provisionerd into coderd. This modifies workspace history tests to provision resources with Terraform, and ensure the result was successful.