Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

feat: support for --sig-proxy in run #3043

Merged
merged 1 commit into from
Jun 24, 2024
Merged

feat: support for --sig-proxy in run #3043

merged 1 commit into from
Jun 24, 2024

Conversation

CodeChanning
Copy link
Contributor

@CodeChanning CodeChanning commented May 30, 2024 •
edited
Loading

adds support for the --sig-proxy flag in nerdctl run and nerdctl attach

Address part of #3039

--sig-proxy expected behavior:

  • if true, CTRL+C will send a SIGINT to the container
  • if false, CTRL+C will send a SIGKILL to the container

context:

"To stop a container, use CTRL-c. This key sequence sends SIGKILL to the container. If --sig-proxy is true (the default),CTRL-c sends a SIGINT to the container. If the container was run with -i and -t, you can detach from a container and leave it running using the CTRL-p CTRL-q key sequence."

src: docker attach documentation

@apostasie
Copy link
Contributor

@CodeChanning this would be nice to have - thanks!
Are you willing to carry this through?

@CodeChanning
Copy link
Contributor Author

Contributor

Yes, I'm aiming to finish this change

@CodeChanning CodeChanning changed the title Support for --sig-proxy in run Support for --sig-proxy in run and attach Jun 3, 2024
@CodeChanning CodeChanning changed the title Support for --sig-proxy in run and attach feat: support for --sig-proxy in run and attach Jun 4, 2024
@CodeChanning CodeChanning force-pushed the sig-proxy_run branch 3 times, most recently from 720827b to 90995f9 Compare June 12, 2024 22:39
@CodeChanning CodeChanning changed the title feat: support for --sig-proxy in run and attach feat: support for --sig-proxy in run Jun 12, 2024
@CodeChanning CodeChanning marked this pull request as ready for review June 12, 2024 22:45
@CodeChanning CodeChanning force-pushed the sig-proxy_run branch 2 times, most recently from 13d0882 to 8bee996 Compare June 12, 2024 23:45
@CodeChanning
Copy link
Contributor Author

Hi @apostasie, I believe my changes should be good for run --sig-proxy support. Both rootful and rootless tests work fine for me locally. Do you have insight as to why the rootless tests are timing out?

@apostasie
Copy link
Contributor

Hi @apostasie, I believe my changes should be good for run --sig-proxy support. Both rootful and rootless tests work fine for me locally. Do you have insight as to why the rootless tests are timing out?

Nothing obvious top of the head - I'll look into it and see if I can help.

sondavidb
sondavidb reviewed Jun 17, 2024
View reviewed changes
cmd/nerdctl/container_run.go Outdated Show resolved Hide resolved
cmd/nerdctl/container_run_linux_test.go Outdated Show resolved Hide resolved
Shubhranshu153
Shubhranshu153 reviewed Jun 17, 2024
View reviewed changes
cmd/nerdctl/container_run.go Outdated Show resolved Hide resolved
pkg/api/types/container_types.go Outdated Show resolved Hide resolved
@CodeChanning CodeChanning force-pushed the sig-proxy_run branch 3 times, most recently from 13355c5 to 23dedaa Compare June 21, 2024 22:40
AkihiroSuda
AkihiroSuda reviewed Jun 24, 2024
View reviewed changes
docs/command-reference.md
@@ -388,7 +388,7 @@ IPFS flags:
Unimplemented `docker run` flags:
`--attach`, `--blkio-weight-device`, `--cpu-rt-*`, `--device-*`,
`--disable-content-trust`, `--domainname`, `--expose`, `--health-*`, `--isolation`, `--no-healthcheck`,
`--link*`, `--publish-all`, `--sig-proxy`, `--storage-opt`,
`--link*`, `--publish-all`, `--storage-opt`,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you add the usage of sig-proxy?

Also, please squash the commits

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍🏾
Added it

@CodeChanning CodeChanning force-pushed the sig-proxy_run branch 3 times, most recently from 00601c1 to a44deac Compare June 24, 2024 22:04
@CodeChanning
feat: support for --sig-proxy in run
ae27768 
Signed-off-by: CodeChanning <chxgaddy@amazon.com>
AkihiroSuda
AkihiroSuda approved these changes Jun 24, 2024
View reviewed changes
Copy link
Member

@AkihiroSuda AkihiroSuda left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks

@AkihiroSuda AkihiroSuda merged commit a2a21b4 into containerd:main Jun 24, 2024
22 checks passed
@AkihiroSuda AkihiroSuda added this to the v2.0.0 milestone Jun 24, 2024
@CodeChanning CodeChanning mentioned this pull request Jun 25, 2024
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@AkihiroSuda AkihiroSuda AkihiroSuda approved these changes

@apostasie apostasie Awaiting requested review from apostasie

@sondavidb sondavidb Awaiting requested review from sondavidb

@Shubhranshu153 Shubhranshu153 Awaiting requested review from Shubhranshu153

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
v2.0.0
Development

Successfully merging this pull request may close these issues.
5 participants
@CodeChanning @apostasie @AkihiroSuda @Shubhranshu153 @sondavidb