do not merge: WIP provisionerd by kylecarbs · Pull Request #84 · coder/coder

kylecarbs · 2022-01-29T16:03:36Z

Creates the provisionerd service that interfaces with
coderd to process provision jobs!

This modifies a prior migration which is typically forbidden, but because we're pre-production deployment I felt grouping would be helpful to future contributors. This adds database functions that are required for the provisioner daemon and job queue logic.

Adds a projectparameter package to compute build-time project values for a provided scope. This package will be used to return which variables are being used for a build, and can visually indicate the hierarchy to a user.

Provisionerd communicates with coderd over a multiplexed WebSocket serving dRPC. This adds a roughly accurate protocol definition. It shares definitions with "provisioner.proto" for simple interop with provisioners!

Creates the provisionerd service that interfaces with coderd to process provision jobs!

codecov · 2022-01-29T16:05:40Z

Codecov Report

Merging #84 (82a5750) into main (ac617e1) will increase coverage by 2.66%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##             main      #84      +/-   ##
==========================================
+ Coverage   72.06%   74.72%   +2.66%     
==========================================
  Files          91       53      -38     
  Lines        3751      550    -3201     
  Branches       59       59              
==========================================
- Hits         2703      411    -2292     
+ Misses        829      127     -702     
+ Partials      219       12     -207

Flag	Coverage Δ
unittest-go-macos-latest	`?`
unittest-go-ubuntu-latest	`?`
unittest-go-windows-latest	`?`
unittest-js	`74.72% <ø> (ø)`

Impacted Files	Coverage Δ
coderd/coderd.go
coderd/coderdtest/coderdtest.go
coderd/projects.go
coderd/workspaces.go
codersdk/projects.go
codersdk/workspaces.go
peerbroker/listen.go
provisionersdk/serve.go
provisionersdk/transport.go
peer/channel.go
... and 28 more

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 ac617e1...82a5750. Read the comment docs.

bryphe-coder · 2022-01-29T21:56:06Z

codersdk/provisionerd.go

+		}
+		return nil, readBodyAsError(res)
+	}
+	session, err := yamux.Client(websocket.NetConn(context.Background(), conn, websocket.MessageBinary), nil)


Just curious, why is this using context.Background() instead of ctx?

bryphe-coder · 2022-01-29T22:50:34Z

coderd/coderd.go

 		})
 		r.Post("/login", users.loginWithPassword)
 		r.Post("/logout", users.logout)
+		r.Get("/provisionerd", provisionerd.listen)


At very first glance, I thought this was an endpoint the browser would listen to for updates over a socket - but after reading the PR, I realized this is for the provisionerd process to establish a socket connection with coderd.

It might be helpful to have a comment to this effect, like:

// Endpoint that facilitates connection with running `provisionerd` processes

Do you think we should change the endpoint? I genuinely wasn't sure what to call this!

coderd/provisionerd.go

bryphe-coder · 2022-01-29T22:58:03Z

provisionerd/provisionerd.go

+		if a.isClosed() {
+			return
+		}


Hmm, why is this needed if we have this right below:

select { case <-a.closed: return

?

Is it for performance, or just to be explicit?

bryphe-coder · 2022-01-29T22:59:55Z

provisionerd/provisionerd.go

+func (a *API) acquireJob() {
+	a.opts.Logger.Debug(context.Background(), "acquiring new job")
+	var err error
+	a.activeJobMutex.Lock()


Should this be using the

a.activeJobMutex.Lock() defer a.activeJobMutex.Unlock()

pattern?

I see some cases below where we potentially modify activeJob again, that isn't protected by the mutex - so I'd be worried about a potential race (like Line 147/148)

I just we call into cancelActiveJob later, which also acquires the mutex - go probably behaves like C, where it's an error to grab the mutex twice.

In that case - maybe we need to wrap Lines 147-150 in a mutex too?

bryphe-coder · 2022-01-29T23:02:59Z

coderd/provisionerd.go

+			return nil, xerrors.Errorf("unmarshal job data: %w", err)
+		}
+
+		// Validate that all parameters send from the provisioner daemon


Suggested change

// Validate that all parameters send from the provisioner daemon

// Validate that all parameters sent from the provisioner daemon

bryphe-coder · 2022-01-29T23:04:37Z

codersdk/provisionerd.go

+// dRPC is a single-stream protocol by design. It's intended to operate
+// a single HTTP-request per invocation. This multiplexes the WebSocket
+// using yamux to enable multiple streams to function on a single connection.


Thanks for the detailed comment; I wasn't sure why we were using yamux and this helps answer it 👍

if we're going to add this kind of multiplexing, why not use gRPC, which supports this kind of multiplexing?

gRPC multiplexes with HTTP/2. So although it kinda supports multiplexing, we'd have to proxy an HTTP/2 server over this WebSocket. I also had difficulty finding examples of gRPC over a WebSocket.

A benefit of the dRPC library is the simple abstraction. We could easily split the requests out into separate connections if we needed.

Happy to elaborate further!

bryphe-coder · 2022-01-29T23:05:36Z

provisionerd/provisionerd.go

+
+	switch jobType := a.activeJob.Type.(type) {
+	case *proto.AcquiredJob_ProjectImport_:
+		a.opts.Logger.Debug(context.Background(), "acquired job is project import",


Thanks for all the logging in this path

bryphe-coder

Exciting to see this all come together! Looks good to me, just some questions / thoughts

bryphe-coder · 2022-01-29T23:10:44Z

provisionerd/provisionerd_test.go

+		projectHistory, err := client.CreateProjectHistory(context.Background(), user.Organization, project.Name, coderd.CreateProjectVersionRequest{
+			StorageMethod: database.ProjectStorageMethodInlineArchive,
+			StorageSource: buffer.Bytes(),
+		})


I'm wondering how this flow will work on in the UI, to create new projects w/ the collateral necessary for the provisioner

The test cases are helpful in understanding the API!

bryphe-coder · 2022-01-29T23:11:58Z

provisionerd/provisionerd_test.go

+			err := terraform.Serve(ctx, &terraform.ServeOptions{
+				ServeOptions: &provisionersdk.ServeOptions{
+					Transport: serverPipe,
+				},
+			})
+			require.NoError(t, err)


Is this test actually running against terraform?

bryphe-coder · 2022-01-29T23:12:42Z

provisionerd/provisionerd_test.go

+		content := `resource "null_resource" "hi" {}`
+		err := writer.WriteHeader(&tar.Header{
+			Name: "main.tf",
+			Size: int64(len(content)),
+		})


kylecarbs · 2022-01-29T23:35:00Z

@bryphe-coder I shoulda made this a draft!

I'm refactoring a ton of this code actively, so I'll have to tag you again. Most of the testing and logic was scaffold a few hours ago, but the entire system is pretty much hooked up.

I'll need to add a few HTTP APIs, the workspace agent, and then I think we have the triangle!

kylecarbs added 5 commits January 29, 2022 15:24

feat: Compute project build parameters

c7c7388

Adds a projectparameter package to compute build-time project values for a provided scope. This package will be used to return which variables are being used for a build, and can visually indicate the hierarchy to a user.

Fix terraform provisioner

ace6248

feat: Add provisionerd protobuf definitions

2bd0c42

Provisionerd communicates with coderd over a multiplexed WebSocket serving dRPC. This adds a roughly accurate protocol definition. It shares definitions with "provisioner.proto" for simple interop with provisioners!

feat: Add provisionerd service

d878c13

Creates the provisionerd service that interfaces with coderd to process provision jobs!

kylecarbs requested a review from bryphe-coder January 29, 2022 16:03

kylecarbs self-assigned this Jan 29, 2022

kylecarbs changed the base branch from main to provisionerdproto January 29, 2022 16:04

kylecarbs added 3 commits January 29, 2022 17:21

Improve provisioner testing

1e8c421

Add support for completing a job

bc8c0e0

Use fork of terraform-exec for JSON output

5d16f2a

bryphe-coder reviewed Jan 29, 2022

View reviewed changes

coderd/provisionerd.go Show resolved Hide resolved

bryphe-coder reviewed Jan 29, 2022

View reviewed changes

bryphe-coder approved these changes Jan 29, 2022

View reviewed changes

bryphe-coder reviewed Jan 29, 2022

View reviewed changes

Add logging to provision jobs

ce4a9fb

Base automatically changed from provisionerdproto to main January 29, 2022 23:52

kylecarbs added 2 commits January 30, 2022 00:01

Merge branch 'main' into provisionerservice

f2fcc25

Fix linting

666529e

kylecarbs force-pushed the provisionerservice branch from 6440b6b to 919572a Compare January 30, 2022 01:37

kylecarbs force-pushed the provisionerservice branch from 919572a to 69003f4 Compare January 30, 2022 03:06

Rename variables

ab7fbec

kylecarbs force-pushed the provisionerservice branch from 69003f4 to ab7fbec Compare January 30, 2022 03:06

kylecarbs added 8 commits January 31, 2022 02:21

Merge branch 'main' into provisionerservice

342a9de

Add new query functions for storing project history logs

c79653e

Add queries for workspace logs

1f4dfc7

add workspace and project history parameters

ba3ce57

Update protobufs for logs

c01860d

Log streaming

c440af4

Refactor provisionerd tests to disconnect from coderd

b495991

Merge branch 'main' into provisionerservice

73bc4e6

kylecarbs force-pushed the provisionerservice branch from 48abd93 to 73bc4e6 Compare February 1, 2022 04:44

Merge branch 'main' into provisionerservice

82a5750

kylecarbs changed the title ~~feat: Add provisionerd service~~ do not merge: WIP provisionerd Feb 1, 2022

kylecarbs closed this Feb 4, 2022

kylecarbs deleted the provisionerservice branch February 4, 2022 18:11

	// Validate that all parameters send from the provisioner daemon
	// Validate that all parameters sent from the provisioner daemon

Conversation

kylecarbs commented Jan 29, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

codecov bot commented Jan 29, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bryphe-coder Jan 29, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

bryphe-coder Jan 29, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bryphe-coder Jan 29, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bryphe-coder left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kylecarbs commented Jan 29, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

kylecarbs commented Jan 29, 2022 •

edited

Loading

codecov bot commented Jan 29, 2022 •

edited

Loading

bryphe-coder Jan 29, 2022 •

edited

Loading

bryphe-coder Jan 29, 2022 •

edited

Loading

bryphe-coder Jan 29, 2022 •

edited

Loading