Support NewTerminal & Detached mode (nerdctl -t -d) by yzxiu · Pull Request #1642 · containerd/nerdctl

yzxiu · 2022-12-12T02:56:51Z

1 support run `-t -d`

nerdctal run --name demo -t -d imagename

2 support create `-t`

nerdctl create --name demo -t imagename
nerdctl start demo

3 support start `-a` to attach a terminal container

nerdctl create --name demo -t imagename
nerdctl start -a demo

4 support start/restart the container that run with `-it` (or without `-d`)

nerdctl run --name demo -it imagename
ctrl + c to stop demo
nerdctl start/restart demo

Signed-off-by: yaozhenxiu 946666800@qq.com

AkihiroSuda · 2022-12-12T03:05:59Z

Thanks, but how to test?

yzxiu · 2022-12-12T03:16:14Z

Thanks, but how to test?

This requires building a container that only works properly with -t -d (or -t) modes
I have tested it locally, and I will add some integration test

yzxiu · 2022-12-12T08:17:53Z

@AkihiroSuda PTAL

yzxiu · 2022-12-16T01:58:27Z

@fahedouch @djdongjin PTAL

AkihiroSuda · 2022-12-16T03:10:12Z

+	if runtime.GOOS == "windows" {
+		t.Skip("buildkit is not enabled on windows, this feature may work on windows.")
+	}
+	testutil.DockerIncompatible(t)


Why incompatible?

Why incompatible?

using -d will use the json-file log driver and cause the windows test fail

AkihiroSuda · 2022-12-16T03:10:59Z

+
+// this container only works properly with -t -d (or -t) modes
+const TerminalContainerDockerfile = `
+FROM golang:latest as builder


Please pin the golang version and go.sum.

Also, can we avoid using golang here?

Please pin the golang version and go.sum.

Also, can we avoid using golang here?

We can package the container in advance and just provide the image name, what do you think?

I'd prefer to minimize dependency on third party images unless it is unavoidable

I thought of an easier way, just run the container directly (bash or /bin/sh), this requires -t to run properly,
no need to build image anymore

AkihiroSuda · 2022-12-16T03:11:52Z

~~Let me merge #1639 before this, sorry~~

djdongjin · 2022-12-16T03:29:50Z

+		}
+		return nil
+	})
+	base.Cmd("container", "rm", "-f", containerName).AssertOK()


This line can be a defer and moved up to before or after line 506, so that the rm -f containerName is always called.

Right now if either 507 or 517 fails, the container won't be cleaned.

Here are actually two little tests, with "-t" and without "-t", so the container must be deleted before running again.
Perhaps use different name? or test separately?

I think you can use different names and defer both rm -f containerName calls.

Same for the test in cmd/nerdctl/create_linux_test.go

I think you can use different names and defer both rm -f containerName calls.

Same for the test in cmd/nerdctl/create_linux_test.go

Great, thanks

djdongjin · 2022-12-17T00:52:29Z

 	assert.Check(t, strings.Contains(log, "bar"))
 }
+
+func TestNewTerminalDetachedMode(t *testing.T) {


This test failed on windows. Not sure if it's due to some issue or not supported on windows.

Also suggest to include Run in the func name, something like TestRunWithTtyAndDetached.

This test failed on windows. Not sure if it's due to some issue or not supported on windows.

~~it seems that the error location is different every time~~

This test failed on windows. Not sure if it's due to some issue or not supported on windows.

using -d will use the json-file log driver and cause the windows test fail

AkihiroSuda · 2022-12-17T14:57:09Z

+	defer base.Cmd("container", "rm", "-f", withTerminalContainerName).AssertOK()
+	withTerminalContainer := base.InspectContainer(withTerminalContainerName)
+	assert.Equal(base.T, "running", withTerminalContainer.State.Status)
+	assert.Equal(base.T, true, withTerminalContainer.State.Running)


The test has to verify that tty is allocated when -t is specified.

Probably you can just test stty

$ docker run -d --name without-t alpine stty f6ee65e70c512ddba7846f595f3dc5ce0cdb72110e9f901d1661e05f0a8a1372 $ docker logs without-t stty: standard input: Not a tty $ docker container inspect without-t | jq .[0].State.ExitCode 1

$ docker run -td --name with-t alpine stty 5b7bf5c0b3b07af1ccbe2c049aa34841ea635e10c0b493e056201e16dd38de03 $ docker logs with-t speed 38400 baud; line = 0; intr = ^C; quit = ^\; erase = ^?; kill = ^U; eof = ^D; eol = <undef>; eol2 = <undef>; swtch = <undef>; start = ^Q; stop = ^S; susp = ^Z; rprnt = ^R; werase = ^W; lnext = ^V; flush = ^O; min = 1; time = 0; -brkint -imaxbel $ docker container inspect with-t | jq .[0].State.ExitCode 0

The test has to verify that tty is allocated when -t is specified.

Probably you can just test stty

$ docker run -d --name without-t alpine stty f6ee65e70c512ddba7846f595f3dc5ce0cdb72110e9f901d1661e05f0a8a1372 $ docker logs without-t stty: standard input: Not a tty $ docker container inspect without-t | jq .[0].State.ExitCode 1

$ docker run -td --name with-t alpine stty 5b7bf5c0b3b07af1ccbe2c049aa34841ea635e10c0b493e056201e16dd38de03 $ docker logs with-t speed 38400 baud; line = 0; intr = ^C; quit = ^\; erase = ^?; kill = ^U; eof = ^D; eol = <undef>; eol2 = <undef>; swtch = <undef>; start = ^Q; stop = ^S; susp = ^Z; rprnt = ^R; werase = ^W; lnext = ^V; flush = ^O; min = 1; time = 0; -brkint -imaxbel $ docker container inspect with-t | jq .[0].State.ExitCode 0

done

Signed-off-by: yaozhenxiu <946666800@qq.com>

AkihiroSuda

Thanks

yzxiu changed the title ~~Fix: Support NewTerminal & Detached mode (nerdctl -t -d ...)~~ Support NewTerminal & Detached mode (nerdctl -t -d ...) Dec 12, 2022

yzxiu force-pushed the support-newterminal_detached_mode branch 4 times, most recently from f907bf5 to 5438991 Compare December 12, 2022 07:53

yzxiu force-pushed the support-newterminal_detached_mode branch 2 times, most recently from 1df53f0 to af918c0 Compare December 12, 2022 09:06

yzxiu changed the title ~~Support NewTerminal & Detached mode (nerdctl -t -d ...)~~ Support NewTerminal & Detached mode (nerdctl -t -d) Dec 12, 2022

djdongjin mentioned this pull request Dec 15, 2022

Fix flag -t needs -i to be specified together restriction in compose run #1604

Open

AkihiroSuda reviewed Dec 16, 2022

View reviewed changes

Comment thread cmd/nerdctl/run_test.go Outdated

AkihiroSuda reviewed Dec 16, 2022

View reviewed changes

Comment thread cmd/nerdctl/run_test.go Outdated

djdongjin reviewed Dec 16, 2022

View reviewed changes

AkihiroSuda added this to the v1.2.0 (tentative) milestone Dec 16, 2022

yzxiu force-pushed the support-newterminal_detached_mode branch from 333e9ed to e0ef207 Compare December 16, 2022 19:04

djdongjin reviewed Dec 17, 2022

View reviewed changes

yzxiu force-pushed the support-newterminal_detached_mode branch 8 times, most recently from 5099881 to 61e4f19 Compare December 17, 2022 08:53

yzxiu force-pushed the support-newterminal_detached_mode branch from 61e4f19 to 09b37d5 Compare December 17, 2022 09:51

yzxiu requested a review from AkihiroSuda December 17, 2022 14:50

AkihiroSuda reviewed Dec 17, 2022

View reviewed changes

Support NewTerminal & Detached mode

c67b102

Signed-off-by: yaozhenxiu <946666800@qq.com>

yzxiu force-pushed the support-newterminal_detached_mode branch from e45ce4d to c67b102 Compare December 17, 2022 16:14

AkihiroSuda approved these changes Dec 20, 2022

View reviewed changes

AkihiroSuda merged commit 32fae2d into containerd:main Dec 20, 2022

ningziwen mentioned this pull request Dec 21, 2022

Support nerdctl logs for the containers running without -d #1657

Closed

fahedouch mentioned this pull request Dec 23, 2022

fix can't start with -it flag #950

Closed

Conversation

yzxiu commented Dec 12, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

1 support run -t -d

2 support create -t

3 support start -a to attach a terminal container

4 support start/restart the container that run with -it (or without -d)

Uh oh!

AkihiroSuda commented Dec 12, 2022

Uh oh!

yzxiu commented Dec 12, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

yzxiu commented Dec 12, 2022

Uh oh!

yzxiu commented Dec 16, 2022

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!

yzxiu Dec 16, 2022 • 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!

yzxiu Dec 16, 2022 • 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!

Uh oh!

Uh oh!

AkihiroSuda commented Dec 16, 2022 • 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!

yzxiu Dec 16, 2022 • 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!

djdongjin Dec 17, 2022 • 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!

yzxiu Dec 17, 2022 • 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!

AkihiroSuda left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

yzxiu commented Dec 12, 2022 •

edited

Loading

1 support run `-t -d`

2 support create `-t`

3 support start `-a` to attach a terminal container

4 support start/restart the container that run with `-it` (or without `-d`)

yzxiu commented Dec 12, 2022 •

edited

Loading

yzxiu Dec 16, 2022 •

edited

Loading

yzxiu Dec 16, 2022 •

edited

Loading

AkihiroSuda commented Dec 16, 2022 •

edited

Loading

yzxiu Dec 16, 2022 •

edited

Loading

djdongjin Dec 17, 2022 •

edited

Loading

yzxiu Dec 17, 2022 •

edited

Loading