monitor: Enable to exec into the container #1626

ktock · 2023-02-14T12:26:43Z

Related to #1104

PR6
Add exec command to execute new processes in the same debug container. All processes are attachable as described in the previous section.

exec execs a new command in the container

$ BUILDX_EXPERIMENTAL=1 /tmp/out/buildx build --invoke sh /tmp/ctx
...
(buildx) exec ps
Process "6zqrfs121s8fqo097mjf7j1t4" started. Press Ctrl-a-c to switch to that process.
(buildx) Switched IO


PID   USER     TIME  COMMAND
    1 root      0:00 sh
    7 root      0:00 ps
Switched IO
(buildx) exec ls
Process "57c0vhpkpo621nla15b2pj8oh" started. Press Ctrl-a-c to switch to that process.
(buildx) Switched IO


bin     etc     hello2  home    root    tmp     var
dev     hello   hello3  proc    sys     usr
Switched IO
(buildx) exec sh
Process "njnu3unnfb5zf5omot93yh6ap" started. Press Ctrl-a-c to switch to that process.
(buildx) Switched IO


/ # 
/ # ps
PID   USER     TIME  COMMAND
    1 root      0:00 sh
    8 root      0:00 sh
   14 root      0:00 ps

attach attaches IO to the specified process.

$ BUILDX_EXPERIMENTAL=1 /tmp/out/buildx build --invoke sh /tmp/ctx
...
Interactive container was restarted with process "tmb96qnbc2q7xfjhochczysga". Press Ctrl-a-c to switch to the new container
(buildx) exec sh
Process "ooybngbcn0nnz584i8xvgbnii" started. Press Ctrl-a-c to switch to that process.
<C-a-c>
(buildx) exec sh
Process "xy51w8j7lzll5cduwfwdm7w50" started. Press Ctrl-a-c to switch to that process.
<C-a-c>
(buildx) exec sh
Process "yx8of20854f0jkd1ga3d9odan" started. Press Ctrl-a-c to switch to that process.
<C-a-c>
(buildx) ps
PID				CURRENT_SESSION	COMMAND
ooybngbcn0nnz584i8xvgbnii	false		[sh]
tmb96qnbc2q7xfjhochczysga	false		[sh]
xy51w8j7lzll5cduwfwdm7w50	false		[sh]
yx8of20854f0jkd1ga3d9odan	true		[sh]
(buildx) attach ooybngbcn0nnz584i8xvgbnii
Switched IO
Attached to process "ooybngbcn0nnz584i8xvgbnii". Press Ctrl-a-c to switch to the new container

/ # ps
PID   USER     TIME  COMMAND
    1 root      0:00 sh
    7 root      0:00 sh
   13 root      0:00 sh
   19 root      0:00 sh
   25 root      0:00 ps
/ # echo $$
7
/ # Switched IO
(buildx) attach tmb96qnbc2q7xfjhochczysga
Switched IO
Attached to process "tmb96qnbc2q7xfjhochczysga". Press Ctrl-a-c to switch to the new container

/ # echo $$
1
/ # Switched IO
(buildx) attach xy51w8j7lzll5cduwfwdm7w50
Switched IO
Attached to process "xy51w8j7lzll5cduwfwdm7w50". Press Ctrl-a-c to switch to the new container

/ # echo $$
13
/ # Switched IO
(buildx) attach yx8of20854f0jkd1ga3d9odan
Switched IO
Attached to process "yx8of20854f0jkd1ga3d9odan". Press Ctrl-a-c to switch to the new container

/ # echo $$
19

note

This commit includes a change to docker-container driver to always include init process in the driver container. This is for making sure to clean up exited processes created by BuildKit's container API.

jedevc

Shared some initial thoughts - but I think generally, it works! 🎉 I'll definitely make sure that we don't stall on this PR as we did on #1296 😢😢

Aside from the technical things, we definitely need to make sure we revisit the DX here (probably after all of #1104 is completed) - the combination of commands we have some weird confusions, like kill kills the server instead of processes spawned with exec and seen with ps. I gave some suggestions in a comment on session lifetimes, so we may actually want to get rid of the explicit kill command in the server at some point (or maybe just rename it if we really want it).

controller/local/controller.go

controller/remote/server.go

build/build.go

jedevc · 2023-02-15T15:20:57Z

build/build.go

-			}
-			close(doneCh)
-		}()
+func (c *Container) IsUnavailable() bool {


Why do we need to have a separate method to mark this as unavailable? Atm, I think what we'll get is a separate container every time all the processes in that container terminate? e.g.:

Launching interactive container. Press Ctrl-a-c to switch to monitor console Interactive container was restarted with process "7vs69sixlm1osw0jywealy8j3". Press Ctrl-a-c to switch to the new container Switched IO Switched IO / # / # touch x / # ls bin etc lib mnt proc run srv tmp var x dev home media opt root sbin sys usr work / # Switched IO (buildx) exec sh Process "fa8khvxhu0xkre6lqvpr4pz8i" started. Press Ctrl-a-c to switch to that process. (buildx) Switched IO / # / # ls bin etc lib mnt proc run srv tmp var dev home media opt root sbin sys usr work / #

I think that we could simplify this by making sure that the lifetime of the container corresponds to the lifetime of the debugging session, or until the user calls rollback (so this might actually play weirdly with 56b9e78 from #1620 and leak resources forever).

I think we need to work out exactly what the lifetime semantics of the debugging session should be, but generally I think we should follow:

Processes are terminated up 1. when they exit, or 2. when their parent session terminates

A session should terminate 1. when it is explicitly terminated by the user, 2. when its parent controller is terminated (for local, when the calling process ends, for remote, when the server terminates), or 3. after a deadline during which no user interaction has occurred

The deadline is useful, so that a user can run a build, watch it fail, and then connect to the session to investigate and invoke, without needing to re-trigger the build.

The remote server should terminate itself when 1. the user requests it to using kill, or 2. it has no more sessions (so that after the deadlines of all the sessions have expired, we can cleanly exit).

We can refine these over time, but TL;DR I think the right call here is to not clean up the container when all the processes are terminated.

(also we seem to get a strange newline from stdin in the code example above, I'm not sure where it comes from 🤔)

Overall SGTM. Fixed not to mark it as unavailable except 2 cases:

1: When PID1 exits. Currently PID1 is the first command executed by the user in the container (e.g. command specified by --invoke). In the future, maybe we can introduce a special PID1 process (e.g. tini?) for keeping the container available even when all user-specified commands exit.

2: When one of the processes returns non-zero exit code. In this case, BuildKit's container API kills the container. To fix this behaviour, we need to fix BuildKit first so we should do this as a long-term following up work.

jedevc · 2023-02-15T15:28:38Z

monitor/monitor.go

+						curRef = ref
+					}
+					fmt.Fprintf(stdout, "Attached to process %q. Press Ctrl-a-c to switch to the new container\n", id)
+				case "exec":


Probably something for a follow-up, but I've noticed that this switch statement keeps growing and getting more and more indented 😢

We should probably try and work out how to get all of this functionality into separate methods on the monitor struct, to try and make it a little easier to have modular functionality.

Thank you for the suggestion. It can be a large refactoring so let's work in a following-up PR.

controller/pb/controller.proto

build/build.go

ktock · 2023-02-16T09:54:49Z

Thanks for the comments. Fixed the patch based on the comments.
Now processes are managed on the server side so monitor can switch IO among processes in different sessions without killing processes.

exec a process in a session:

(buildx) exec ps
Process "lg4atdbxa3al3l7l42lgo5sq3" started. Press Ctrl-a-c to switch to that process.
(buildx) Switched IO
PID   USER     TIME  COMMAND
    1 root      0:00 sh
    6 root      0:00 ps
Switched IO
(buildx) exec sh
Process "nwkw4tpg1zngwu1mkz600yrfe" started. Press Ctrl-a-c to switch to that process.
(buildx) Switched IO
/ # ps
PID   USER     TIME  COMMAND
    1 root      0:00 sh
   11 root      0:00 sh
   17 root      0:00 ps
/ # echo $$
11

attach to another process in the same session:

(buildx) ps
PID				CURRENT_SESSION	COMMAND
zncafsibzg3i6zriaviwsmzxw	false		[sh]
nwkw4tpg1zngwu1mkz600yrfe	true		[sh]
(buildx) Switched IO

/ # Switched IO
(buildx) attach zncafsibzg3i6zriaviwsmzxw
Attached to process "zncafsibzg3i6zriaviwsmzxw". Press Ctrl-a-c to switch to the new container
(buildx) Switched IO

/ # echo $$
1

attach to another process in another session:
The following attaches to process 3mqxm55ww9bcguidmkwmh96u2 in another session 1vo8nbia21t6rprjjtrmph3rv.

(buildx) list
ID				CURRENT_SESSION
1vo8nbia21t6rprjjtrmph3rv	false
y0vvv1ta2bkpgt8osm0yhpzbd	true
(buildx) attach 1vo8nbia21t6rprjjtrmph3rv
Attached to process "". Press Ctrl-a-c to switch to the new container
(buildx) ps
PID				CURRENT_SESSION	COMMAND
3mqxm55ww9bcguidmkwmh96u2	false		[sh]
(buildx) attach 3mqxm55ww9bcguidmkwmh96u2
Attached to process "3mqxm55ww9bcguidmkwmh96u2". Press Ctrl-a-c to switch to the new container
(buildx) Switched IO

/ # echo $$
1

If other session already attached to this process, this will be automatically detached.

attach back to the original process in the original session (process nwkw4tpg1zngwu1mkz600yrfe in y0vvv1ta2bkpgt8osm0yhpzbd):

(buildx) list
ID				CURRENT_SESSION
1vo8nbia21t6rprjjtrmph3rv	true
y0vvv1ta2bkpgt8osm0yhpzbd	false
(buildx) attach y0vvv1ta2bkpgt8osm0yhpzbd
Attached to process "". Press Ctrl-a-c to switch to the new container
(buildx) ps
PID				CURRENT_SESSION	COMMAND
zncafsibzg3i6zriaviwsmzxw	false		[sh]
nwkw4tpg1zngwu1mkz600yrfe	false		[sh]
(buildx) attach nwkw4tpg1zngwu1mkz600yrfe
Attached to process "nwkw4tpg1zngwu1mkz600yrfe". Press Ctrl-a-c to switch to the new container
(buildx) Switched IO

/ # echo $$
11

jedevc

Thanks @ktock ❤️ This already looks a lot cleaner to me, just had a few more minor comments.

I noticed you also opened #1640, which seems to complete #1104? I'll take a look at that one asap as well, but ideally before moving forward past that into the next steps, we can dive into the TODOs left in the code as well as tidy up the lifetimes stuff, and clean up the UX before going too much further? I don't want to block merging this or any of the other debug PRs in, but I do want to make sure we have a strong foundation before trying to add too much more functionality (like breakpoints, etc).

build/invoke.go

jedevc · 2023-02-21T11:35:37Z

build/invoke.go

+}
+
+func (c *Container) Exec(ctx context.Context, cfg *InvokeConfig) error {
+	killCh := make(chan struct{})


Noticed this here, but also in some other places: I'd prefer to use contexts instead of channels for the purpose of sending cancel signals.

When ctx passed in here is done, then we'd send the kill signal, but I'm not sure why we can't just listen for ctx.Done in exec.

In general, I think it makes sense to use channels for cases where we explicitly need low-level concurrent primitives, or where we need to send messages between routines, but we should use contexts wherever possible to handle cancelling, for consistency with the rest of the codebase (in buildx and in buildkit).

build/invoke.go

jedevc · 2023-02-21T11:47:09Z

controller/control/controller.go

+	// Invoke starts an IO session into the specified process.
+	// If pid doesn't matche to any running processes, it starts a new process with the specified config.
+	// If there is no container running or InvokeConfig.Rollback is speicfied, the process will start in a newly created container.
+	// NOTE: If needed, in the future, we can split this API into three APIs (NewContainer, NewProcess and Attach).


I think I prefer the suggested semantics over the current Invoke call - at the moment, it's a bit like Invoke does a bit of everything.

That said, I think it's good for now, but it would good to see a follow-up for this 😄

Thanks for the suggestion. Let's work on the refactor on this API in the following-up PRs.

controller/processes/processes.go

jedevc · 2023-02-21T12:01:14Z

controller/remote/server.go

+	statusChan  chan *client.SolveStatus
+	result      *build.ResultContext
+	inputPipe   *io.PipeWriter
+	cancelBuild func()


Is running multiple builds in a single session a supported scenario? If not (I'm fairly sure this is the case), we need to explicitly handle this and error out, if yes, then we need to make sure that subsequent builds don't lose the previous cancel handle.

No it isn't. Fixed to return an error if a build happens during another ongoing.

monitor/monitor.go

jedevc · 2023-02-21T12:18:30Z

monitor/monitor.go

+					plist, err := c.ListProcesses(ctx, curRef)
+					if err != nil {
+						fmt.Println("cannot list process:", err)
+						continue
+					}


At some point, as a follow-up it would be nice if we could use the actual PIDs in the container instead of the random identity.NewIDs - at the moment BuildKit doesn't expose that information though, so we'd need to make changes (probably in StartedMessage).

Thanks for the suggestion. SGTM.

monitor/monitor.go

ktock · 2023-02-22T03:39:41Z

@jedevc Thank you for the review. Fixed the patch based on the comments,

ktock · 2023-02-22T03:56:47Z

I noticed you also opened #1640, which seems to complete #1104? I'll take a look at that one asap as well, but ideally before moving forward past that into the next steps, we can dive into the TODOs left in the code as well as tidy up the lifetimes stuff, and clean up the UX before going too much further? I don't want to block merging this or any of the other debug PRs in, but I do want to make sure we have a strong foundation before trying to add too much more functionality (like breakpoints, etc).

SGTM 👍

jedevc · 2023-03-02T09:31:06Z

build/invoke.go

+)
+
+// InvokeConfig is configuration for a process to run in the container.
+type InvokeConfig struct {


I think we should remove this struct, and instead use controllerapi.InvokeConfig, it would be good to avoid having multiple duplicate struct. The main difference seems to be that we obviously can't attach io objects to it, we can pass those as separate args (maybe using ioset.In to stick them together).

Note on having the duplicate User and NoUser fields to represent an optional value (which if we were using a modern version of protobuf, we could use the optional field in https://github.com/protocolbuffers/protobuf/blob/main/docs/field_presence.md). I guess there's not reason we have to use gogo proto for buildx like we do for buildkit (we don't care about version compatibility), we could just switch to using upstream? https://pkg.go.dev/google.golang.org/protobuf. Definitely follow-up material 🎉 cc @crazy-max since you've done protobuf investigations.

Thank you for the comment. Removed InvokeConfig.

controller/local/controller.go

jedevc · 2023-03-02T09:43:38Z

These should just be nits now, any major changes we can do in follow-ups 👍

Signed-off-by: Kohei Tokunaga <[email protected]>

jedevc

LGTM, happy to address the unresolved comments as follow-ups later.

PTAL @tonistiigi 😄

ktock · 2023-03-07T13:51:39Z

@tonistiigi PTAL 🙏

jedevc added the area/debug label Feb 14, 2023

jedevc requested review from jedevc and tonistiigi February 14, 2023 12:30

jedevc reviewed Feb 15, 2023

View reviewed changes

ktock force-pushed the monitor-exec branch 2 times, most recently from d5b6705 to 6e45206 Compare February 16, 2023 09:54

jedevc mentioned this pull request Feb 21, 2023

Remote controller should clean up after itself #1641

Open

jedevc reviewed Feb 21, 2023

View reviewed changes

ktock force-pushed the monitor-exec branch from 6e45206 to 81b0454 Compare February 22, 2023 03:29

ktock force-pushed the monitor-exec branch from 81b0454 to 3287347 Compare February 24, 2023 11:38

ktock requested a review from jedevc February 28, 2023 10:48

jedevc reviewed Mar 2, 2023

View reviewed changes

monitor: Enable to exec into the container

e8f55a3

Signed-off-by: Kohei Tokunaga <[email protected]>

ktock force-pushed the monitor-exec branch from 3287347 to e8f55a3 Compare March 2, 2023 10:30

jedevc approved these changes Mar 2, 2023

View reviewed changes

tonistiigi merged commit c5ce08b into docker:master Mar 7, 2023

ktock deleted the monitor-exec branch March 8, 2023 01:06

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

monitor: Enable to exec into the container #1626

monitor: Enable to exec into the container #1626

ktock commented Feb 14, 2023

jedevc left a comment

jedevc Feb 15, 2023

jedevc Feb 15, 2023

ktock Feb 16, 2023

jedevc Feb 15, 2023

ktock Feb 16, 2023

ktock commented Feb 16, 2023

jedevc left a comment

jedevc Feb 21, 2023

ktock Feb 22, 2023

jedevc Feb 21, 2023

ktock Feb 22, 2023

jedevc Feb 21, 2023

ktock Feb 22, 2023

jedevc Feb 21, 2023

ktock Feb 22, 2023

ktock commented Feb 22, 2023

ktock commented Feb 22, 2023

jedevc Mar 2, 2023

ktock Mar 2, 2023

jedevc commented Mar 2, 2023

jedevc left a comment

ktock commented Mar 7, 2023

monitor: Enable to exec into the container #1626

monitor: Enable to exec into the container #1626

Conversation

ktock commented Feb 14, 2023

note

jedevc 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

ktock commented Feb 16, 2023

jedevc 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

ktock commented Feb 22, 2023

ktock commented Feb 22, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jedevc commented Mar 2, 2023

jedevc left a comment

Choose a reason for hiding this comment

ktock commented Mar 7, 2023