refactor: change template archive extraction to be on provisioner #9264

spikecurtis · 2023-08-23T09:31:46Z

Refactors provisionersdk protocol to transmit template archive and has the provisioner extract the archive, not the daemon.

The new protocol introduces a "Session" concept so that we only transmit the archive once, and then use it for Parse/Plan (template import) and Plan/Apply (workspace build) operations, which are the mainline use of the provisioner.

Easiest to review commit by commit, as the protocol refactor touches a lot of files.

Note that the provisionersdk protocol is entirely within the same process, so there are no back compatibility concerns about protocol changes.

Signed-off-by: Spike Curtis <spike@coder.com>

mtojek

just a comment on the PR size

mtojek · 2023-08-23T10:40:59Z

cli/agent_test.go

-			ProvisionApply: []*proto.Provision_Response{{
-				Type: &proto.Provision_Response_Complete{
-					Complete: &proto.Provision_Complete{
+			ProvisionApply: []*proto.Response{{


I can see that this PR does a lot of renaming. Is it possible to extract them to a separate PR? Maybe it could shrink this PR a bit.

It's not really possible to put in a separate PR, since the protocol changes are fundamental, and without renames nothing builds.

I've split it out into different commits to reduce the noise

I admit that I focus mainly on changes in the CLI package like:

echo.ProvisionComplete -> echo.ApplyComplete

proto.Provision_Response -> proto.Response

completeWithAgent()

but if these are strongly coupled with the protocol, then please ignore the comment.

johnstcn · 2023-08-23T10:40:58Z

provisionersdk/session.go

+			if err != nil {
+				return xerrors.Errorf("mkdir %q: %w", headerPath, err)
+			}
+			s.Logger.Debug(context.Background(), "extracted directory", slog.F("path", headerPath))


suggestion: include mode in msg

johnstcn · 2023-08-23T10:46:44Z

cli/server.go

@@ -1304,7 +1303,10 @@ func newProvisionerDaemon(
 			defer wg.Done()
 			defer cancel()

-			err := echo.Serve(ctx, afero.NewOsFs(), &provisionersdk.ServeOptions{Listener: echoServer})
+			err := echo.Serve(ctx, &provisionersdk.ServeOptions{
+				Listener:      echoServer,


missing Logger here?

coderd/coderdtest/coderdtest.go

coderd/workspacebuilds_test.go

Signed-off-by: Spike Curtis <spike@coder.com>

…-provisioner-filesystem

mtojek

The first review round is done.

Makefile

coderd/workspacebuilds_test.go

provisionersdk/proto/provisioner.proto

mtojek · 2023-08-23T13:53:36Z

provisionersdk/session.go

+		stream: stream,
+		server: p.server,
+	}
+	sessDir := fmt.Sprintf("Session%p", s)


why %p? isn't it better to link it with (PID+something/id)? if the process dies, it would be easier to find it.

We need something unique to the Session, for the lifetime of the Session. The pointer is a simple and easy choice.

Each provisioner needs to be run with a unique work directory so that they don't trample on each others caches, so there is no need to qualify the pointer with a PID.

The PID might make things easier to find... if you know the PID, which is rarely the case after you have a crash.

Point in favour of PID: the PID will at least show up in dmesg in case of an OOMKill.

provisionersdk/proto/provisioner.proto

mtojek · 2023-08-23T14:05:49Z

provisionersdk/proto/provisioner.proto

    }
 }

 service Provisioner {
-    rpc Parse(Parse.Request) returns (stream Parse.Response);
-    rpc Provision(stream Provision.Request) returns (stream Provision.Response);
+    // Session represents provisioning a single template import or workspace.  The daemon always sends Config followed


double-check: do we need to update our Coder docs? If not, then it might be nice to visualize these changes.

I don't believe we need to update any docs --- this interface is entirely within the provisioner daemon for now.

One thing it does affect is the format of messages on the Echo provisioner, which technically you could install in a real system. But, its only for testing and we don't publicly document it today (nor do I think we should!).

Actually, I was thinking about updating this page accordingly, but it can be performed in a followup.

Yeah, this PR doesn't change the architecture, but it is true that a subsequent PR will allow the provisioner to be separate from the provisioner daemon, which would be an architecture change.

provisionersdk/session.go

mtojek · 2023-08-23T14:11:41Z

provisionersdk/session.go

+	}
+	sessDir := fmt.Sprintf("Session%p", s)
+	s.WorkDirectory = filepath.Join(p.opts.WorkDirectory, sessDir)
+	err := os.MkdirAll(s.WorkDirectory, 0o700)


Thinking loud: what are the chances that s.WorkDirectory exists, and contains a malicious/old/buggy .tf file, which will be included in the terraform provisioning process?

I'm asking as os.MkdirAll doesn't complain that the directory exists.

I guess there is some chance of this from a restarted process, or failed cleanup. I'll add a call to os.RemoveAll() before.

provisionersdk/session.go

Signed-off-by: Spike Curtis <spike@coder.com>

…-provisioner-filesystem

mtojek

LGTM 👍

I will leave the decision about session ID (UUID vs. timestamp vs. pointer) to you as this seems to be rather a lower risk.

Signed-off-by: Spike Curtis <spike@coder.com>

…-provisioner-filesystem

Signed-off-by: Spike Curtis <spike@coder.com>

matifali · 2023-08-24T12:11:13Z

Could this also solve #9246 @mafredri?

spikecurtis · 2023-08-24T13:10:32Z

Could this also solve #9246 @mafredri?

I don't think so. That's an issue with the cache / downloading terraform providers. (This PR is about Coder provisioners).

mtojek

👍

…-provisioner-filesystem

This reverts commit 9762158.

spikecurtis added 4 commits August 23, 2023 08:17

refactor provisionersdk protocol

935719c

Signed-off-by: Spike Curtis <spike@coder.com>

refactor provisioners to use new protocol

a29592a

Signed-off-by: Spike Curtis <spike@coder.com>

refactor provisionerd to use new protocol

1f0efb4

Signed-off-by: Spike Curtis <spike@coder.com>

refactor tests & proto renames

2be6cf0

spikecurtis requested review from johnstcn and mtojek August 23, 2023 09:31

github-actions bot assigned spikecurtis Aug 23, 2023

Fixes from self-review

39d6065

Signed-off-by: Spike Curtis <spike@coder.com>

mtojek reviewed Aug 23, 2023

View reviewed changes

spikecurtis changed the title ~~refactor: changes template archive extraction to be on provisioner~~ refactor: change template archive extraction to be on provisioner Aug 23, 2023

johnstcn approved these changes Aug 23, 2023

View reviewed changes

spikecurtis added 4 commits August 23, 2023 11:12

appease fmt & link

e39b8a2

Signed-off-by: Spike Curtis <spike@coder.com>

code review fixes & e2e fixes

d92608e

Signed-off-by: Spike Curtis <spike@coder.com>

More fmt

0412773

Signed-off-by: Spike Curtis <spike@coder.com>

Merge branch 'main' of https://github.com/coder/coder into spike/9165…

83deae6

…-provisioner-filesystem

mtojek reviewed Aug 23, 2023

View reviewed changes

spikecurtis added 2 commits August 24, 2023 06:08

Code review fixes

d1a9b2a

Signed-off-by: Spike Curtis <spike@coder.com>

Merge branch 'main' of https://github.com/coder/coder into spike/9165…

8d7261f

…-provisioner-filesystem

mtojek approved these changes Aug 24, 2023

View reviewed changes

spikecurtis added 4 commits August 24, 2023 07:09

new gen; use uuid for session workdir

ef1820f

Signed-off-by: Spike Curtis <spike@coder.com>

Revert nix-based gen CI task until dogfood is on nix

96e1e64

Signed-off-by: Spike Curtis <spike@coder.com>

Merge branch 'main' of https://github.com/coder/coder into spike/9165…

2805dda

…-provisioner-filesystem

revert deleting dogfood Docker stuff

9762158

Signed-off-by: Spike Curtis <spike@coder.com>

mtojek approved these changes Aug 24, 2023

View reviewed changes

spikecurtis added 2 commits August 25, 2023 05:38

Merge branch 'main' of https://github.com/coder/coder into spike/9165…

cf8a48b

…-provisioner-filesystem

Revert "revert deleting dogfood Docker stuff"

3d7b6a8

This reverts commit 9762158.

spikecurtis enabled auto-merge (squash) August 25, 2023 06:03

spikecurtis merged commit 60d5002 into main Aug 25, 2023

spikecurtis deleted the spike/9165-provisioner-filesystem branch August 25, 2023 06:10

github-actions bot locked and limited conversation to collaborators Aug 25, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

refactor: change template archive extraction to be on provisioner #9264

refactor: change template archive extraction to be on provisioner #9264

spikecurtis commented Aug 23, 2023

mtojek left a comment

mtojek Aug 23, 2023

spikecurtis Aug 23, 2023

mtojek Aug 23, 2023 •

edited

Loading

johnstcn Aug 23, 2023

johnstcn Aug 23, 2023

mtojek left a comment

mtojek Aug 23, 2023

spikecurtis Aug 24, 2023

johnstcn Aug 24, 2023

mtojek Aug 23, 2023

spikecurtis Aug 24, 2023

mtojek Aug 24, 2023

spikecurtis Aug 24, 2023

mtojek Aug 23, 2023

spikecurtis Aug 24, 2023

mtojek left a comment

matifali commented Aug 24, 2023

spikecurtis commented Aug 24, 2023

mtojek left a comment

refactor: change template archive extraction to be on provisioner #9264

refactor: change template archive extraction to be on provisioner #9264

Conversation

spikecurtis commented Aug 23, 2023

mtojek left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mtojek Aug 23, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mtojek left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mtojek left a comment

Choose a reason for hiding this comment

matifali commented Aug 24, 2023

spikecurtis commented Aug 24, 2023

mtojek left a comment

Choose a reason for hiding this comment

mtojek Aug 23, 2023 •

edited

Loading