feat: support '--detach-keys' for 'nerdctl (run|start)' #2128

davidhsingyuchen · 2023-03-23T19:43:58Z

Control Flow

Detect if the detach keys are present in stdin.
After detecting the detach keys, cancel the current I/O operations. This means that the underlying I/O FIFOs should be left intact, and containerd should keep writing to/reading from those FIFOs, but nerdctl should stop writing to/reading from them. It seems that cio.IO.Cancel() should be used for this, but there's an issue: IO.Cancel() becomes a no-op if the FIFOs are already opened containerd#8326.
After starting the task, we need to call IO.Wait, and after it returns, we need to see if the container exits or it's just the user detaching from the container by checking the state of the container.

Notes

It may be better to support nerdctl exec --detach-keys in a follow-up PR so that we can minimize the changes needed in this PR as the control flow of it is a bit different from that of run|start.

TODOs

Before this PR can be merged, I need to:

Revert the temporarily use the containerd feature branch commit and use the released containerd version.

davidhsingyuchen · 2023-03-27T16:59:10Z

@AkihiroSuda PTAL, thanks!

go.mod

pkg/consoleutil/detach.go

fahedouch · 2023-03-29T10:25:02Z

pkg/consoleutil/detach.go

+	if ds.closed {
+		return 0, syscall.EBADF
+	}


not sure if this condition is relevant here (is it possible to read from already closed (detached) stdin ?)

I think you're right. It turned out IO.Cancel() doesn't really close the FIFOs (containerd/containerd#8326), so if this check is removed, closer() will keep being called, and the debug logs (i.e., "Cancelling IO" and "IO cancelled") will overflow my screen. As a result, when term.EscapeError is detected, I'm returning a non-nil error for now , and I added a TODO to return a nil error afrer the issue is fixed and before this PR can be merged.

pkg/taskutil/taskutil.go

fahedouch · 2023-03-29T12:52:46Z

pkg/consoleutil/detach.go

+	closed bool
+}
+
+// NewDetachableStdin returns a new TTY proxy reader


NewDetachableStdin returns a new detachableStdin which uses a TTY proxy reader to read specified detach keys from stdin, in which case closer will be called

Hmm, I'm not sure if we want to mention the underlying unexported struct here, maybe the users of this function shouldn't need to know that implementation detail?

LGTM, but just one thing was confusing me : NewDetachableStdin returns a new TTY proxy reader. In fact the NewDetachableStdin is not returning a TTY proxy reader but it is wrapping a a TTY proxy reader

Ah you're right! Updated the comment accordingly, plz let me know if it works for you :)

fahedouch · 2023-03-29T13:03:53Z

After detecting the detach keys, cancel the current I/O operations. This means that the underlying I/O FIFOs should be left intact, and containerd should keep writing to/reading from those FIFOs, but nerdctl should stop writing to/reading from them. It seems that cio.IO.Cancel() should work, but I'm not 100% sure due to the nil IO() thing below.

Cancel will aborts all current io operations ( from nerdctl and containerd). but nerdctl is fully controlling the io of the task (nerdctl is creating task with specefic io) , so I am wondering why not making task.CloseIO() ( this will abort io and clean fifos) , I also think that we no longer use the exsiting fifos after detach

davidhsingyuchen · 2023-03-29T17:18:54Z

After detecting the detach keys, cancel the current I/O operations. This means that the underlying I/O FIFOs should be left intact, and containerd should keep writing to/reading from those FIFOs, but nerdctl should stop writing to/reading from them. It seems that cio.IO.Cancel() should work, but I'm not 100% sure due to the nil IO() thing below.

Cancel will aborts all current io operations ( from nerdctl and containerd). but nerdctl is fully controlling the io of the task (nerdctl is creating task with specefic io) , so I am wondering why not making task.CloseIO() ( this will abort io and clean fifos) , I also think that we no longer use the exsiting fifos after detach

After we detach from a container, we should be able to reattach to it again. Docker:

➜  ~ docker attach test
/ #
/ #
/ # read escape sequence
➜  ~ docker attach test
/ #
/ #
/ # read escape sequence

cmd/nerdctl/container_run_test.go

davidhsingyuchen · 2023-03-30T20:10:46Z

@fahedouch Thanks a lot for the quick review! In the new revision, besides addressing the comments, I also made it print read detach keys when detaching from the container so that it's clearer to the users what's happening, and it's also aligned to UX of docker. Also updated e2e tests accordingly.

davidhsingyuchen · 2023-03-31T02:34:16Z

The latest revision: If I replace containerd/containerd in nerdctl's go.mod with the version from containerd/containerd#8334, it seems to be working.

davidhyc@lima-dev:/Users/davidhyc/dev/containerd/nerdctl$ ./_output/nerdctl run -it --name detach busybox
/ #
/ # INFO[0001] read detach keys
davidhyc@lima-dev:/Users/davidhyc/dev/containerd/nerdctl$ nerdctl ps -a
CONTAINER ID    IMAGE                               COMMAND    CREATED          STATUS    PORTS    NAMES
fd3b77abf607    docker.io/library/busybox:latest    "sh"       4 seconds ago    Up                 detach

fahedouch · 2023-05-07T16:17:13Z

@davidhsingyuchen

It seems that cio.IO.Cancel() should be used for this, but there's an issue: containerd/containerd#8326.

please see

AkihiroSuda · 2023-05-18T01:27:37Z

Needs rebase

davidhsingyuchen · 2023-05-18T17:38:19Z

@AkihiroSuda Thank you for the reminder. Could you help take a look at containerd/containerd#8326 and/or containerd/containerd#8334? I'll rebase the feature branch after the blocker is resolved, thanks!

davidhsingyuchen · 2023-06-07T00:38:27Z

@fahedouch PTAL, thanks! The failed jobs seem to be flaky as all the jobs pass at least once:

AkihiroSuda · 2023-06-08T17:01:55Z

go.mod

@@ -57,12 +58,14 @@ require (
 	golang.org/x/term v0.8.0
 	golang.org/x/text v0.9.0
 	gopkg.in/yaml.v3 v3.0.1
+	gotest.tools v2.2.0+incompatible
 	gotest.tools/v3 v3.4.0


Can we avoid mixing up v2 and v3?

AkihiroSuda · 2023-06-08T17:02:22Z

go.mod

@@ -37,6 +37,7 @@ require (
 	github.com/mitchellh/mapstructure v1.5.0
 	github.com/moby/sys/mount v0.3.3
 	github.com/moby/sys/signal v0.7.0
+	github.com/moby/term v0.0.0-20221205130635-1aeaba878587


Can we use https://github.com/moby/term/releases/tag/v0.5.0

Control flow: 1. Detect if the detach keys are present in stdin. 2. After detecting the detach keys, cancel the current I/O operations by calling IO.Cancel(). This means that the underlying I/O FIFOs should be left intact, and containerd should keep writing to/reading from those FIFOs, but nerdctl should stop writing to/reading from them. 3. After starting the task, we need to call IO.Wait(), and after it returns, we need to see if the container exits or it's just the user detaching from the container by checking the state of the container. Signed-off-by: Hsing-Yu (David) Chen <davidhsingyuchen@gmail.com>

davidhsingyuchen · 2023-06-08T17:15:05Z

@AkihiroSuda Thanks for the reminder. Both are fixed.

AkihiroSuda

Thanks

davidhsingyuchen mentioned this pull request Mar 23, 2023

feat: add 'nerdctl container attach' #2108

Merged

davidhsingyuchen changed the title ~~[WIP] feat: support '--detach-keys' for 'nerdctl (run|start|exec)'~~ [WIP, but PTAL] feat: support '--detach-keys' for 'nerdctl (run|start|exec)' Mar 28, 2023

AkihiroSuda requested review from fahedouch and yuchanns March 28, 2023 17:07

davidhsingyuchen force-pushed the detach-keys branch from 1b2898a to e99afb1 Compare March 28, 2023 17:16

fahedouch reviewed Mar 29, 2023

View reviewed changes

davidhsingyuchen mentioned this pull request Mar 29, 2023

IO.Cancel() becomes a no-op if the FIFOs are already opened containerd/containerd#8326

Closed

davidhsingyuchen force-pushed the detach-keys branch 5 times, most recently from 1305692 to 585588c Compare March 29, 2023 20:50

davidhsingyuchen changed the title ~~[WIP, but PTAL] feat: support '--detach-keys' for 'nerdctl (run|start|exec)'~~ [WIP, but PTAL] feat: support '--detach-keys' for 'nerdctl (run|start)' Mar 29, 2023

davidhsingyuchen force-pushed the detach-keys branch from 585588c to d81485a Compare March 29, 2023 21:05

davidhsingyuchen requested review from fahedouch and removed request for yuchanns March 29, 2023 21:09

davidhsingyuchen force-pushed the detach-keys branch 2 times, most recently from 8047c15 to cce73c3 Compare March 29, 2023 23:29

fahedouch reviewed Mar 30, 2023

View reviewed changes

cmd/nerdctl/container_run_test.go Outdated Show resolved Hide resolved

cmd/nerdctl/container_run_test.go Outdated Show resolved Hide resolved

cmd/nerdctl/container_run_test.go Outdated Show resolved Hide resolved

fahedouch modified the milestones: v1.3.0, v1.4.0 (tentative) Mar 30, 2023

davidhsingyuchen force-pushed the detach-keys branch from cce73c3 to cc33276 Compare March 30, 2023 19:05

davidhsingyuchen requested a review from fahedouch March 30, 2023 19:45

davidhsingyuchen force-pushed the detach-keys branch from cc33276 to 4314ef7 Compare March 31, 2023 02:32

davidhsingyuchen force-pushed the detach-keys branch from a723f80 to 2a7d60c Compare April 4, 2023 17:33

AkihiroSuda marked this pull request as draft April 4, 2023 17:33

AkihiroSuda modified the milestones: v1.3.1, v1.4.0 (tentative) Apr 13, 2023

fahedouch mentioned this pull request May 9, 2023

improve container Task comment containerd/containerd#8503

Merged

AkihiroSuda modified the milestones: v1.4.0, v1.4.1 (tentative) May 18, 2023

davidhsingyuchen mentioned this pull request Jun 5, 2023

build(deps): bump github.com/containerd/containerd from 1.7.1 to 1.7.2 #2274

Merged

davidhsingyuchen force-pushed the detach-keys branch 3 times, most recently from a0635be to 8e25eae Compare June 6, 2023 23:33

davidhsingyuchen closed this Jun 7, 2023

davidhsingyuchen reopened this Jun 7, 2023

davidhsingyuchen changed the title ~~[WIP, but PTAL] feat: support '--detach-keys' for 'nerdctl (run|start)'~~ feat: support '--detach-keys' for 'nerdctl (run|start)' Jun 7, 2023

davidhsingyuchen marked this pull request as ready for review June 7, 2023 00:37

davidhsingyuchen requested a review from fahedouch June 7, 2023 00:39

fahedouch approved these changes Jun 8, 2023

View reviewed changes

AkihiroSuda reviewed Jun 8, 2023

View reviewed changes

davidhsingyuchen force-pushed the detach-keys branch from 8e25eae to ec49140 Compare June 8, 2023 17:14

AkihiroSuda approved these changes Jun 8, 2023

View reviewed changes

fahedouch merged commit 8c08e1e into containerd:main Jun 11, 2023
21 checks passed

davidhsingyuchen deleted the detach-keys branch June 12, 2023 16:48

fahedouch mentioned this pull request Jul 9, 2023

nerdctl does not log stdout if there is no newline #2313

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: support '--detach-keys' for 'nerdctl (run|start)' #2128

feat: support '--detach-keys' for 'nerdctl (run|start)' #2128

davidhsingyuchen commented Mar 23, 2023 •

edited

davidhsingyuchen commented Mar 27, 2023

fahedouch Mar 29, 2023 •

edited

davidhsingyuchen Mar 29, 2023

fahedouch Mar 29, 2023

davidhsingyuchen Mar 29, 2023

fahedouch Mar 30, 2023 •

edited

davidhsingyuchen Mar 30, 2023

fahedouch commented Mar 29, 2023 •

edited

davidhsingyuchen commented Mar 29, 2023

davidhsingyuchen commented Mar 30, 2023

davidhsingyuchen commented Mar 31, 2023 •

edited

fahedouch commented May 7, 2023

AkihiroSuda commented May 18, 2023

davidhsingyuchen commented May 18, 2023

davidhsingyuchen commented Jun 7, 2023

AkihiroSuda Jun 8, 2023

AkihiroSuda Jun 8, 2023

davidhsingyuchen commented Jun 8, 2023

AkihiroSuda left a comment

feat: support '--detach-keys' for 'nerdctl (run|start)' #2128

feat: support '--detach-keys' for 'nerdctl (run|start)' #2128

Conversation

davidhsingyuchen commented Mar 23, 2023 • edited

Control Flow

Notes

TODOs

davidhsingyuchen commented Mar 27, 2023

fahedouch Mar 29, 2023 • edited

Choose a reason for hiding this comment

davidhsingyuchen Mar 29, 2023

Choose a reason for hiding this comment

fahedouch Mar 29, 2023

Choose a reason for hiding this comment

davidhsingyuchen Mar 29, 2023

Choose a reason for hiding this comment

fahedouch Mar 30, 2023 • edited

Choose a reason for hiding this comment

davidhsingyuchen Mar 30, 2023

Choose a reason for hiding this comment

fahedouch commented Mar 29, 2023 • edited

davidhsingyuchen commented Mar 29, 2023

davidhsingyuchen commented Mar 30, 2023

davidhsingyuchen commented Mar 31, 2023 • edited

fahedouch commented May 7, 2023

AkihiroSuda commented May 18, 2023

davidhsingyuchen commented May 18, 2023

davidhsingyuchen commented Jun 7, 2023

AkihiroSuda Jun 8, 2023

Choose a reason for hiding this comment

AkihiroSuda Jun 8, 2023

Choose a reason for hiding this comment

davidhsingyuchen commented Jun 8, 2023

AkihiroSuda left a comment

Choose a reason for hiding this comment

davidhsingyuchen commented Mar 23, 2023 •

edited

fahedouch Mar 29, 2023 •

edited

fahedouch Mar 30, 2023 •

edited

fahedouch commented Mar 29, 2023 •

edited

davidhsingyuchen commented Mar 31, 2023 •

edited