feat: add 'nerdctl container attach' #2108

davidhsingyuchen · 2023-03-16T20:44:59Z

Summary

PR is unblocked by #2128. After this PR, for a container started in attached mode, a user can detach from it (enabled by #2128) and then re-attach to it using nerdctl attach (enabled by this PR).

However, since "dual logging" is not implemented yet, containers spun up in detached mode, namely via nerdctl run -d or nerdctl start (without -a), cannot be nerdctl attached, which is mentioned in nerdctl attach --help and command-reference.md. To be specific, for a container started in detached mode, cio.LogURI instead of cio.NewCreator is used, which means the FIFOs that store the container's stdin/stdout/stderr and will be used by cio.NewAttach won't be there.

Notes

ContainerAttachOptions.GOptions is not used by container.Attach at the moment, but I still added it for consistency across commands.
To keep this PR as simple as possible, only the bare minimum CLI parameters are supported in it. In other words, out of all the parameters supported by docker attach, only --detach-keys is supported, and both --no-stdin and sig-proxy are not supported yet. We can add support for them in follow-up PRs.
cio.Terminal is not really used by cio.NewAttach (i.e., whether the container is with a terminal is already decided when the container is created/started), so I omit the logic to decide if it should be passed to cio.NewAttach.
Currently only one attach session is supported because only one reader can be attached to a task under containerd's current design. Supporting multiple attach sessions at nerdctl side implies quite a bit complexity, so maybe we can leave it to another PR, or even consider doing it at containerd side.

davidhsingyuchen · 2023-06-12T22:50:28Z

@AkihiroSuda PTAL, thanks! Some jobs seemed to be cancelled for no obvious reason.

AkihiroSuda · 2023-06-13T05:22:33Z

.gitignore

@@ -7,3 +7,6 @@ _output

 # vagrant
 /.vagrant
+
+# IDE
+.vscode


This should be in your local git config

Thanks for the pointer! TIL 😄

AkihiroSuda · 2023-06-13T05:23:59Z

cmd/nerdctl/container_attach.go

+		Args: cobra.ExactArgs(1),
+		Short: "Attach stdin, stdout, and stderr to a running container.\n\nCaveats:\n\n" +
+			"- Currently only one attach session is allowed.\n" +
+			"- Before [dual logging](https://github.com/containerd/nerdctl/issues/1946#issuecomment-1443649307) is implemented, " +


Suggested change

"- Before [dual logging](https://github.com/containerd/nerdctl/issues/1946#issuecomment-1443649307) is implemented, " +

"- Until dual logging (issue #1946) is implemented, " +

AkihiroSuda · 2023-06-13T05:24:42Z

cmd/nerdctl/container_attach_linux_test.go

+)
+
+// skipAttachForDocker should be called by attach-related tests that assert 'read detach keys' in stdout.
+func skipAttachForDocker(t *testing.T) {


Needs t.Helper()

AkihiroSuda · 2023-06-13T05:25:42Z

cmd/nerdctl/container_attach_linux_test.go

+func prepareContainerToAttach(base *testutil.Base, containerName string) {
+	opts := []func(*testutil.Cmd){
+		testutil.WithStdin(testutil.NewDelayOnceReader(bytes.NewReader(
+			[]byte{16, 17}, // https://www.physics.udel.edu/~watson/scen103/ascii.html


Please add a comment to explain the key codes (C-p C-q)

davidhsingyuchen · 2023-06-19T21:14:57Z

@AkihiroSuda All the comments should have been addressed, thanks!

AkihiroSuda · 2023-07-30T07:49:19Z

Sorry needs rebase

davidhsingyuchen · 2023-07-31T17:07:42Z

@AkihiroSuda done!

ktock · 2023-08-01T00:05:50Z

cmd/nerdctl/container_attach.go

+		Short: "Attach stdin, stdout, and stderr to a running container.\n\nCaveats:\n\n" +
+			"- Currently only one attach session is allowed.\n" +
+			"- Until dual logging (issue #1946) is implemented, " +
+			"a container that is spun up by either `nerdctl run -d` or `nerdctl start` (without `--attach`) cannot be attached to.",


Can we use multi-line quote?

`aaa bbb`

ktock · 2023-08-01T00:09:05Z

cmd/nerdctl/container_attach.go

+		Use:  "attach [flags] CONTAINER",
+		Args: cobra.ExactArgs(1),
+		Short: "Attach stdin, stdout, and stderr to a running container.\n\nCaveats:\n\n" +
+			"- Currently only one attach session is allowed.\n" +


Should mention that what happens when the second session tries to attach.

ktock · 2023-08-01T00:13:35Z

cmd/nerdctl/container_attach.go

+		Short: "Attach stdin, stdout, and stderr to a running container.\n\nCaveats:\n\n" +
+			"- Currently only one attach session is allowed.\n" +
+			"- Until dual logging (issue #1946) is implemented, " +
+			"a container that is spun up by either `nerdctl run -d` or `nerdctl start` (without `--attach`) cannot be attached to.",


Should list some examples of containers allowed to be attached.

ktock · 2023-08-01T00:14:07Z

docs/command-reference.md

+Caveats:
+
+- Currently only one attach session is allowed.
+- Until dual logging (issue #1946) is implemented, a container that is spun up by either `nerdctl run -d` or `nerdctl start` (without `--attach`) cannot be attached to.


Can we have examples of containers allowed to be attached?

Signed-off-by: Hsing-Yu (David) Chen <davidhsingyuchen@gmail.com>

AkihiroSuda

Thanks

AkihiroSuda · 2024-03-13T07:37:36Z

This seems currently broken

failed to attach to the container: failed to open stdout fifo: error creating fifo binary:///usr/local/bin/nerdctl?_NERDCTL_INTERNAL_LOGGING=%2Fvar%2Flib%2Fnerdctl%2F1935db59: no such file or directory #2877

davidhsingyuchen force-pushed the add-attach branch from f69e123 to db9f425 Compare March 20, 2023 18:31

davidhsingyuchen force-pushed the add-attach branch 2 times, most recently from 64efeff to 4441ce2 Compare June 12, 2023 21:48

davidhsingyuchen closed this Jun 12, 2023

davidhsingyuchen reopened this Jun 12, 2023

davidhsingyuchen changed the title ~~[WIP] feat: add 'nerdctl container attach'~~ feat: add 'nerdctl container attach' Jun 12, 2023

davidhsingyuchen marked this pull request as ready for review June 12, 2023 22:50

AkihiroSuda added this to the v1.5.0 milestone Jun 13, 2023

AkihiroSuda reviewed Jun 13, 2023

View reviewed changes

davidhsingyuchen force-pushed the add-attach branch from 4441ce2 to 359af70 Compare June 13, 2023 17:07

davidhsingyuchen requested a review from AkihiroSuda June 13, 2023 17:08

AkihiroSuda requested a review from ktock July 25, 2023 18:51

AkihiroSuda modified the milestones: v1.5.0, v1.5.1 (tentative) Jul 30, 2023

davidhsingyuchen force-pushed the add-attach branch from 48a17ee to 9549b3e Compare July 31, 2023 16:50

ktock reviewed Aug 1, 2023

View reviewed changes

feat: add 'nerdctl container attach'

f0140a5

Signed-off-by: Hsing-Yu (David) Chen <davidhsingyuchen@gmail.com>

davidhsingyuchen force-pushed the add-attach branch from 9549b3e to f0140a5 Compare August 1, 2023 17:30

davidhsingyuchen requested a review from ktock August 1, 2023 17:31

ktock approved these changes Aug 2, 2023

View reviewed changes

AkihiroSuda approved these changes Aug 2, 2023

View reviewed changes

AkihiroSuda merged commit 9cd00de into containerd:main Aug 2, 2023
20 of 21 checks passed

davidhsingyuchen deleted the add-attach branch August 2, 2023 16:53

AkihiroSuda mentioned this pull request Mar 13, 2024

add attach command #744

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: add 'nerdctl container attach' #2108

feat: add 'nerdctl container attach' #2108

davidhsingyuchen commented Mar 16, 2023 •

edited

Loading

davidhsingyuchen commented Jun 12, 2023

AkihiroSuda Jun 13, 2023

davidhsingyuchen Jun 13, 2023

AkihiroSuda Jun 13, 2023

AkihiroSuda Jun 13, 2023

AkihiroSuda Jun 13, 2023

davidhsingyuchen commented Jun 19, 2023

AkihiroSuda commented Jul 30, 2023

davidhsingyuchen commented Jul 31, 2023

ktock Aug 1, 2023

ktock Aug 1, 2023

ktock Aug 1, 2023

ktock Aug 1, 2023

AkihiroSuda left a comment

AkihiroSuda commented Mar 13, 2024

	"- Before [dual logging](https://github.com/containerd/nerdctl/issues/1946#issuecomment-1443649307) is implemented, " +
	"- Until dual logging (issue #1946) is implemented, " +

feat: add 'nerdctl container attach' #2108

feat: add 'nerdctl container attach' #2108

Conversation

davidhsingyuchen commented Mar 16, 2023 • edited Loading

Summary

Notes

davidhsingyuchen commented Jun 12, 2023

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

davidhsingyuchen commented Jun 19, 2023

AkihiroSuda commented Jul 30, 2023

davidhsingyuchen commented Jul 31, 2023

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

AkihiroSuda left a comment

Choose a reason for hiding this comment

AkihiroSuda commented Mar 13, 2024

davidhsingyuchen commented Mar 16, 2023 •

edited

Loading