refactor: change template archive extraction to be on provisioner by spikecurtis · Pull Request #9264 · coder/coder

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

-			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

+			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


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


missing Logger here?

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

…-provisioner-filesystem

mtojek

The first review round is done.

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

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

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

 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.

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

+	}
+	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.

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

Conversation

spikecurtis commented Aug 23, 2023

Uh oh!

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

mtojek Aug 23, 2023 • 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!

Uh oh!

Uh oh!

mtojek left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

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!

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!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

mtojek left a comment

Choose a reason for hiding this comment

Uh oh!

matifali commented Aug 24, 2023

Uh oh!

spikecurtis commented Aug 24, 2023

Uh oh!

mtojek left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

mtojek Aug 23, 2023 •

edited

Loading