NotificationsYou must be signed in to change notification settings
Fork1k
Star11.2k

do not merge: WIP provisionerd#84

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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Jump to bottom

Closed

kylecarbs wants to merge21 commits intomainfromprovisionerservice

Closed

do not merge: WIP provisionerd#84

kylecarbs wants to merge21 commits intomainfromprovisionerservice

Conversation

Copy link

Member

kylecarbs commentedJan 29, 2022•
edited
Loading

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

kylecarbs added5 commits

January 29, 2022 15:24

feat: Add parameter and jobs database schema

48527f7

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

feat: Compute project build parameters

c7c7388

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

Fix terraform provisioner

ace6248

feat: Add provisionerd protobuf definitions

2bd0c42

Provisionerd communicates with coderd over a multiplexedWebSocket serving dRPC. This adds a roughly accurate protocoldefinition.It shares definitions with "provisioner.proto" for simpleinterop with provisioners!

feat: Add provisionerd service

d878c13

Creates the provisionerd service that interfaces withcoderd to process provision jobs!

kylecarbs requested a review frombryphe-coder

January 29, 2022 16:03

kylecarbs self-assigned this

Jan 29, 2022

kylecarbs changed the base branch frommain toprovisionerdproto

January 29, 2022 16:04

Copy link

codecovbot commentedJan 29, 2022•
edited
Loading

Codecov Report

Merging#84 (82a5750) intomain (ac617e1) willincrease coverage by2.66%.
The diff coverage isn/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
... and28 more

Continue to review full report at Codecov.

Legend -Click here to learn more
Δ = absolute <relative> (impact),ø = not affected,? = missing data
Powered byCodecov. Last updateac617e1...82a5750. Read thecomment docs.

kylecarbs added3 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

codersdk/provisionerd.go Outdated

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

Copy link

Contributor

bryphe-coderJan 29, 2022

Choose a reason for hiding this comment

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

Just curious, why is this usingcontext.Background() instead ofctx?

bryphe-coder reviewed

Jan 29, 2022

View reviewed changes

coderd/coderd.go Outdated

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

Copy link

Contributor

bryphe-coderJan 29, 2022•
edited
Loading

Choose a reason for hiding this comment

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

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 theprovisionerd process to establish a socket connection withcoderd.

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

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

Copy link

MemberAuthor

kylecarbsJan 30, 2022

Choose a reason for hiding this comment

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

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

bryphe-coder reviewed

Jan 29, 2022

View reviewed changes

coderd/provisionerd.goShow resolvedHide resolved

bryphe-coder reviewed

Jan 29, 2022

View reviewed changes

provisionerd/provisionerd.go Outdated

Comment on lines 99 to 101

		ifa.isClosed() {
		return
		}

Copy link

Contributor

bryphe-coderJan 29, 2022•
edited
Loading

Choose a reason for hiding this comment

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

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 reviewed

Jan 29, 2022

View reviewed changes

provisionerd/provisionerd.go Outdated

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

Copy link

Contributor

bryphe-coderJan 29, 2022•
edited
Loading

Choose a reason for hiding this comment

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

Should this be using the

a.activeJobMutex.Lock()defera.activeJobMutex.Unlock()

pattern?

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

Copy link

Contributor

bryphe-coderJan 29, 2022

Choose a reason for hiding this comment

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

I just we call intocancelActiveJob 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 reviewed

Jan 29, 2022

View reviewed changes

coderd/provisionerd.go

		returnnil,xerrors.Errorf("unmarshal job data: %w",err)
		}

		// Validate that all parameters send from the provisioner daemon

Copy link

Contributor

bryphe-coderJan 29, 2022

Choose a reason for hiding this comment

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

Suggested change

	// Validate that all parameterssend from the provisioner daemon
	// Validate that all parameterssent from the provisioner daemon

bryphe-coder reviewed

Jan 29, 2022

View reviewed changes

codersdk/provisionerd.go Outdated

Comment on lines 40 to 42

		// 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.

Copy link

Contributor

bryphe-coderJan 29, 2022

Choose a reason for hiding this comment

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

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

Copy link

jawnsyJan 31, 2022

Choose a reason for hiding this comment

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

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

Copy link

MemberAuthor

kylecarbsJan 31, 2022

Choose a reason for hiding this comment

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

gRPC multiplexes with HTTP/2. So although itkinda 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 reviewed

Jan 29, 2022

View reviewed changes

provisionerd/provisionerd.go Outdated


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

Copy link

Contributor

bryphe-coderJan 29, 2022

Choose a reason for hiding this comment

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

Thanks for all the logging in this path

bryphe-coder approved these changes

Jan 29, 2022

View reviewed changes

Copy link

Contributor

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.

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

bryphe-coder reviewed

Jan 29, 2022

View reviewed changes

provisionerd/provisionerd_test.go Outdated

Comment on lines 53 to 56

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

Copy link

Contributor

bryphe-coderJan 29, 2022

Choose a reason for hiding this comment

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

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

Copy link

Contributor

bryphe-coderJan 29, 2022

Choose a reason for hiding this comment

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

The test cases are helpful in understanding the API!

bryphe-coder reviewed

Jan 29, 2022

View reviewed changes

provisionerd/provisionerd_test.go Outdated

Comment on lines 90 to 95

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

Copy link

Contributor

bryphe-coderJan 29, 2022

Choose a reason for hiding this comment

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

Is this test actually running against terraform?

bryphe-coder reviewed

Jan 29, 2022

View reviewed changes

provisionerd/provisionerd_test.go Outdated

Comment on lines 45 to 49

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

Copy link

Contributor

bryphe-coderJan 29, 2022

Choose a reason for hiding this comment

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

Nice 😄

Add logging to provision jobs

ce4a9fb

Copy link

MemberAuthor

kylecarbs commentedJan 29, 2022

@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!

Base automatically changed fromprovisionerdproto tomain

January 29, 2022 23:52

kylecarbs added2 commits

January 30, 2022 00:01

Merge branch 'main' into provisionerservice

f2fcc25

Fix linting

666529e

kylecarbs force-pushed theprovisionerservice branch from6440b6b to919572aCompare

January 30, 2022 01:37

kylecarbs force-pushed theprovisionerservice branch from919572a to69003f4Compare

January 30, 2022 03:06

Rename variables

ab7fbec

kylecarbs force-pushed theprovisionerservice branch from69003f4 toab7fbecCompare

January 30, 2022 03:06

kylecarbs added8 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 theprovisionerservice branch from48abd93 to73bc4e6Compare

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

Labels

None yet

3 participants

Movatterモバイル変換

do not merge: WIP provisionerd#84

do not merge: WIP provisionerd#84

Uh oh!

Conversation

kylecarbs commentedJan 29, 2022• editedLoading Uh oh!There was an error while loading.Please reload this page.

Uh oh!

Uh oh!

codecovbot commentedJan 29, 2022• editedLoading 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-coderJan 29, 2022• editedLoading 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-coderJan 29, 2022• editedLoading Uh oh!There was an error while loading.Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bryphe-coderJan 29, 2022• editedLoading 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 commentedJan 29, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

kylecarbs commentedJan 29, 2022•
edited
Loading

codecovbot commentedJan 29, 2022•
edited
Loading

bryphe-coderJan 29, 2022•
edited
Loading

bryphe-coderJan 29, 2022•
edited
Loading

bryphe-coderJan 29, 2022•
edited
Loading