update gateway to add ability to run and exec into containers by coryb · Pull Request #1627 · moby/buildkit

coryb · 2020-08-05T16:24:18Z

more updates for #749, this pr adds the gateway client and server implementation with proto updates.

Here is a sequence diagram for the grpc message flow:

coryb · 2020-08-05T16:26:41Z

+	}
+}
+
+func addDefaultEnvvar(env []string, k, v string) []string {


this was copied from ops/exec.go, we can move it to a common util package if desired, was not sure where to put it.

coryb · 2020-08-05T17:09:16Z

The cross compile failed 3 times with this error:

#116 FROM docker.io/tonistiigi/binfmt:buildkit
#116 sha256:90fda517d0186166992332ac17ed66c6e1f3c0b78be072165351cc2aea215ca9
#116 ERROR: pull access denied, repository does not exist or may require authorization: server message: invalid_token: authorization failed

#33 FROM docker.io/tonistiigi/binfmt:buildkit
#33 sha256:d4959a5a882759bba7601923e8ae2aa875841b7cad47faec82eb0b059698c6a3
#33 ERROR: pull access denied, repository does not exist or may require authorization: server message: invalid_token: authorization failed

not sure what this issue could be from.

tonistiigi

Could we get some tests for this please 🤞

For the CI error, I think it is token expiring showing up after #1622 . I switched CI to v0.7.2 that should unblock this (but introduces another issue). I'll try to get a proper fix soon but requires custom containerd authorizer implementation.

hinshun

Found a few potential race conditions, I may find more but submitting this for now.

coryb · 2020-08-05T23:43:58Z

@tonistiigi not sure how to create tests for this one. I could not find any unit tests for the gateway, and the integration tests (ie ./client/build_test.go) I think will require modifications to the gateway.Client interface (suggested some changes here). Do you have an example how I might test these changes without the gateway.Client changes? Otherwise, it seems like I could either add tests for this along with the next PR for client changes or try to update this PR with client changes + tests. Other thoughts?

coryb · 2020-08-06T21:01:46Z

After reading Tonis' comment here I realized I have the starting message out of order. I kept thinking the started channel was to indicate IO ready to receive, but it really means container is ready to exec into if necessary. I need to reorder the Started message and delay sending any async Fd messages until the Started has been sent. Will update ...

tonistiigi · 2020-08-06T22:41:59Z

@coryb Sorry, got confused as well. We do not need a started channel to signal if container is ready for exec, we know that from the create() returning. But that only works if create does not contain meta/process-info. This PR looks correct in that aspect but the client-side proposal didn't.

coryb · 2020-08-06T23:19:27Z

I see, yeah. There is still an unlikely race with my code since the server returns the StartedMessage in parallel with executor.Run. So if client receives StartedMessage and then immediately sends another InitMessage (which should call executor.Exec) then the second InitMessage might result int "container not found" because pid1 might not have updated the running map yet due to the race. It should be pretty easy to fix this.

coryb · 2020-08-11T20:46:13Z

Converted to Draft until I can get the client side of this integrated as per discussion in #749

coryb · 2020-08-31T21:02:00Z

Sorry for the delay, life has not gone to plan for a few weeks now. I have reset the PR to include the client code and added some tests. Here is a gist for sample main:
https://gist.github.com/coryb/e9a74902f172c57cefe258c525735b94

Some issues that came up I am not sure how to deal with. In the sample main, the status displayCh is not synchronized with the container startup so output can get messed up then Stdin goes into Raw mode. It does not cause problems, just looks ugly. Ideally we would be able to flush the displayCh before going into Raw mode.

Another odd issue I am not sure how to deal with is that the integration tests fail under worker=oci-rootless. Runc is getting a panic: panic: cannot statfs cgroup root when run in rootless mode. For now I skip the tests in rootless mode.

coryb · 2020-08-31T21:12:46Z

-
-// ExitError will be returned from Run and Exec when the container process exits with
-// a non-zero exit code.
-type ExitError struct {


moved ExitError to errdefs package to avoid circular package dependencies

Signed-off-by: Cory Bennett <cbennett@netflix.com>

hinshun

I reviewed a few times before, only small nit for now.

hinshun · 2020-08-31T21:30:06Z

+
+	lbf.ctrsMu.Lock()
+	// ensure we are not clobbering a dup container id request
+	if _, ok := lbf.ctrs[in.ContainerID]; ok {


Should we check this in the beginning of this function instead?

I think to move it up we would have to put most of the function under the mutex lock. It is isolate at the bottom after the container object has been created so that we can check for duplication at the same time we do the assignment and to minimize the timing and code run under the lock. If we move the dup check up, we would either have to check again anyway inside the lock (since concurrency is allowed) or lock before we create the container which could take time with creating volumes via the cache manager.

I have tweaked the code a bit to tighten up the release and unlock handling, does that look better?

Signed-off-by: Cory Bennett <cbennett@netflix.com>

tonistiigi · 2020-09-03T01:18:08Z

Another odd issue I am not sure how to deal with is that the integration tests fail under worker=oci-rootless. Runc is getting a panic: panic: cannot statfs cgroup root when run in rootless mode. For now I skip the tests in rootless mode.****

@AkihiroSuda any ideas?

tonistiigi · 2020-09-03T01:21:37Z

Haven't reviewed yet but one thing I noticed was that there was no new api cap. Client-side should also detect missing cap.

I assume integration to ExecState is planned as a follow-up?

AkihiroSuda · 2020-09-03T01:37:09Z

+func testClientGatewayContainerPID1Exit(t *testing.T, sb integration.Sandbox) {
+	if sb.Rootless() {
+		// TODO fix this
+		// We get `panic: cannot statfs cgroup root` when running this test


Could you open an issue in runc repo

Why does it appear in exec and not in regular builds?

Created issue, with stacktrace from runc: opencontainers/runc#2573

/sys/fs/cgroup isn't mounted?

It looks like the /sys mounts are trimmed out here

It appears to work if we bind mount in /sys, but not sure is this is the right thing to do here. After diffing the config.json from runc spec and runc spec --rootless. I added this to that ToRootless function and it worked for my tests, but not sure if there are other complications:

spec.Mounts = append(spec.Mounts, specs.Mount{ Destination: "/sys", Type: "none", Source: "/sys", Options: []string{"rbind", "nosuid", "noexec", "nodev", "ro"}, })

Does this seem like the right solution?

Sorry I didn't notice this comment.

{"rbind", "ro"} doesn't make submounts recursively read-only and can result in container breakout.
So we should mount /sys and /sys/fs/cgroup separately as {"bind", "ro"}.

In the long term, we should fix opencontainers/runc#2573

Okay, I have updated with this: 4b51fbd

It seems to work, hopefully does not cause other issues.

This change ended up causing several other integration tests to fail with oci-rootless, like: TestIntegration/TestDefaultEnvWithArgs so I am going to revert it. I am leaning towards leaving my "skip rootless" hack in place for this PR and then investigating/fixing the rootless + /sys issue later.

…d.Solve Signed-off-by: Cory Bennett <cbennett@netflix.com>

…be per process Signed-off-by: Cory Bennett <cbennett@netflix.com>

Signed-off-by: Cory Bennett <cbennett@netflix.com>

coryb · 2020-10-02T07:43:57Z

Sorry for the delays, I was out on a much needed vacation for almost 2 weeks.

I think I have most of the items fixed. 2 open issue:

not sure where we should be defaulting Cwd: update gateway to add ability to run and exec into containers #1627 (comment)
not sure how to proceed with runc-rootless: update gateway to add ability to run and exec into containers #1627 (comment)
(should we deal with the necessary /sys bind mounts in another PR?)

AkihiroSuda · 2020-10-02T07:51:57Z

Sorry I didn't notice your question, please take a look at #1627 (comment)

Signed-off-by: Cory Bennett <cbennett@netflix.com>

coryb · 2020-10-02T21:18:24Z

I think this PR is ready now. There are several follow-up issues that I will create to track:

add pty creation logic in runc executor (work left over from return exit status from executor Run/Exec #1619)
add (*ExecState).ToExecProcess wrapper
mount /sys in runc-rootless and enable gateway exec tests for rootless

tonistiigi · 2020-10-02T21:28:11Z

-					},
-					exit.Error,
-				)
+				if exit.Code == containerd.UnknownExitStatus {


Don't quite understand this special case handling. Why isn't this just

err = grpcerrors.FromGRPC(status.ErrorProto(exit.Error) if exit.Code != containerd.UnknownExitStatus { err = &errdefs.ExitError{ExitCode: exit.Code, Err: err} }

Yeah, that makes more sense. exit.Error is an *rpc.Status so has to be converted to *spb.Status first for status.ErrorProto, unless I am missing some other helper func that already does this? Anyway, updated per your outline here: 398593b

Signed-off-by: Cory Bennett <cbennett@netflix.com>

tonistiigi · 2021-10-14T07:07:39Z

@coryb I started to try to use this and discovered the signal part was left unimplemented

buildkit/frontend/gateway/client/client.go

Line 78 in ba673bb

// TODO Signal(ctx context.Context, sig os.Signal)

. That unfortunately makes it quite unusable for anything interactive (and means we still need to wait for another buildkit release for everyone 😢 ). Do you have plans to finish it?

coryb · 2021-10-15T20:56:17Z

Hey @tonistiigi, I will try to look into it next week. We have been using the gateway exec functionality fairly extensively for debugging failing builds, not having signals has caused a few annoyances for us but generally it has been very usable for us. The interactive terminal allows things like ctrl-c to go through, so just curious where the missing signals are causing problems for you.

tonistiigi · 2021-10-15T20:59:25Z

The interactive terminal allows things like ctrl-c to go through, so just curious where the missing signals are causing problems for you.

Hmm. It did not work for me.

coryb · 2021-10-15T21:40:21Z

@tonistiigi not sure what is wrong. I have a crud client main that will exec into a container and run bash with interactive tty here: https://gist.github.com/coryb/8d733b7f1f2398a7b85a640d1f38fdc0

If you go run main.go it will get you a prompt (formatting likely off with the default progress output). You can then run tools like top and ctrl-c out of it. At least that worked for me.

tonistiigi · 2021-10-16T01:30:38Z

@coryb That indeed seems to work in most cases. Need to test more.

coryb commented Aug 5, 2020

View reviewed changes

tonistiigi reviewed Aug 5, 2020

View reviewed changes

Comment thread solver/pb/ops.proto Outdated

Comment thread frontend/gateway/pb/gateway.proto Outdated

hinshun reviewed Aug 5, 2020

View reviewed changes

Comment thread frontend/gateway/gateway.go Outdated

Comment thread frontend/gateway/gateway.go Outdated

Comment thread frontend/gateway/gateway.go Outdated

Comment thread frontend/gateway/gateway.go Outdated

AkihiroSuda reviewed Aug 6, 2020

View reviewed changes

Comment thread frontend/gateway/pb/gateway.proto Outdated

coryb force-pushed the gateway-exec-proto branch from bb7adb8 to 5962481 Compare August 6, 2020 17:34

hinshun reviewed Aug 6, 2020

View reviewed changes

Comment thread frontend/gateway/gateway.go Outdated

Comment thread frontend/gateway/gateway.go Outdated

Comment thread frontend/gateway/gateway.go Outdated

coryb force-pushed the gateway-exec-proto branch from f6744d5 to 1732086 Compare August 6, 2020 18:50

coryb force-pushed the gateway-exec-proto branch from 57d512b to b90aaee Compare August 7, 2020 07:14

coryb mentioned this pull request Aug 7, 2020

Add Exec to the gateway API. #749

Closed

coryb marked this pull request as draft August 11, 2020 20:44

coryb force-pushed the gateway-exec-proto branch from ef6b41a to 14ffe33 Compare August 17, 2020 21:46

coryb force-pushed the gateway-exec-proto branch 2 times, most recently from 3448537 to df7d944 Compare August 31, 2020 20:30

coryb marked this pull request as ready for review August 31, 2020 20:54

coryb commented Aug 31, 2020

View reviewed changes

update gateway to add ability to run and exec into containers

355e937

Signed-off-by: Cory Bennett <cbennett@netflix.com>

coryb force-pushed the gateway-exec-proto branch from df7d944 to 355e937 Compare August 31, 2020 21:14

hinshun reviewed Aug 31, 2020

View reviewed changes

cleanup release on error logic for gateway NewContainer

8df17c8

Signed-off-by: Cory Bennett <cbennett@netflix.com>

coryb force-pushed the gateway-exec-proto branch from c4223b4 to 8df17c8 Compare September 1, 2020 22:45

AkihiroSuda reviewed Sep 3, 2020

View reviewed changes

remove SessonManager from FrontendLLBBridge, adding as arg to Fronten…

03e1c19

…d.Solve Signed-off-by: Cory Bennett <cbennett@netflix.com>

coryb force-pushed the gateway-exec-proto branch from c09d63d to 03e1c19 Compare October 2, 2020 01:59

coryb added 2 commits October 2, 2020 05:36

Move SecurityMode to InitMessage from NewContainer message so it can …

ea3bc24

…be per process Signed-off-by: Cory Bennett <cbennett@netflix.com>

remove unnecessary MountManager interface

095a919

Signed-off-by: Cory Bennett <cbennett@netflix.com>

coryb force-pushed the gateway-exec-proto branch from 349a0fd to 095a919 Compare October 2, 2020 06:25

coryb added 3 commits October 2, 2020 06:33

fix unnecessary MetadataStore rename

c04aeca

Signed-off-by: Cory Bennett <cbennett@netflix.com>

fix linting error

e824c5e

Signed-off-by: Cory Bennett <cbennett@netflix.com>

use grpc typed error for ExitMessage error

d1a14ae

Signed-off-by: Cory Bennett <cbennett@netflix.com>

coryb force-pushed the gateway-exec-proto branch from 5845b3c to d1a14ae Compare October 2, 2020 07:37

coryb force-pushed the gateway-exec-proto branch from 4b51fbd to d1a14ae Compare October 2, 2020 17:56

Default Cwd to "/"

2f0738c

Signed-off-by: Cory Bennett <cbennett@netflix.com>

tonistiigi reviewed Oct 2, 2020

View reviewed changes

Simplify rpc exec exit error handling

398593b

Signed-off-by: Cory Bennett <cbennett@netflix.com>

tonistiigi approved these changes Oct 2, 2020

View reviewed changes

hinshun approved these changes Oct 3, 2020

View reviewed changes

hinshun requested a review from AkihiroSuda October 3, 2020 06:01

AkihiroSuda approved these changes Oct 4, 2020

View reviewed changes

AkihiroSuda merged commit 78f3e86 into moby:master Oct 4, 2020

This was referenced Oct 4, 2020

gateway exec: add (*ExecState).ToExecProcess wrapper #1715

Open

gateway exec: mount /sys and /sys/fs/cgroup into rootless runc #1716

Closed

[fix #1716] mount /sys and /sys/fs/cgroup for rootless #1718

Closed

tonistiigi mentioned this pull request Jan 18, 2022

gateway: exec API should support sending signals to processes #2566

Closed

Conversation

coryb commented Aug 5, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

coryb commented Aug 5, 2020

Uh oh!

tonistiigi left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

hinshun left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

coryb commented Aug 5, 2020

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

coryb commented Aug 6, 2020

Uh oh!

tonistiigi commented Aug 6, 2020

Uh oh!

coryb commented Aug 6, 2020

Uh oh!

coryb commented Aug 11, 2020

Uh oh!

coryb commented Aug 31, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

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

tonistiigi commented Sep 3, 2020

Uh oh!

tonistiigi commented Sep 3, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tonistiigi Sep 3, 2020 • 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!

Choose a reason for hiding this comment

Uh oh!

coryb commented Oct 2, 2020

Uh oh!

AkihiroSuda commented Oct 2, 2020

Uh oh!

coryb commented Oct 2, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tonistiigi commented Oct 14, 2021

coryb commented Aug 5, 2020 •

edited

Loading

tonistiigi Sep 3, 2020 •

edited

Loading