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.

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

--attach in create command #1805

Open
ningziwen opened this issue Jan 4, 2023 · 3 comments · May be fixed by #3951
Open

--attach in create command #1805

ningziwen opened this issue Jan 4, 2023 · 3 comments · May be fixed by #3951
Assignees

Comments

@ningziwen
Copy link
Contributor

ningziwen commented Jan 4, 2023

What is the problem you're trying to solve

Hi, --attach flag is in docker create but not in nerdctl create.

I'd like to contribute this change in nerdctl but I found --attach is not mentioned in nerdctl create documentation as "unimplemented flag". So before I make the change, just want to confirm that --attach is an expected feature in nerdctl create and there is no implicit reason of not having --attach in nerdctl create intentionally.

Describe the solution you'd like

Support --attach in nerdctl create.

Additional context

https://docs.docker.com/engine/reference/commandline/create/#options
https://github.com/containerd/nerdctl#whale-blue_square-nerdctl-create

@manugupt1
Copy link
Contributor

Sure! thanks for offering. Please feel to work on it.

@ningziwen
Copy link
Contributor Author

ningziwen commented Feb 5, 2023

The "container" and "task" below are referring to containerd concept: https://github.com/containerd/containerd/blob/main/docs/getting-started.md#creating-a-running-task

--attach comes into effect in when creating new task today. https://github.com/containerd/nerdctl/blob/main/pkg/taskutil/taskutil.go#L43

nerdctl start passes the --attach flag when creating new task.
nerdctl run does not support --attach flag and hardcoded it to false when creating new task. It should be easy to support --attach in nerdctl run. I may also complete it in a separated PR.
nerdctl create only creates container without task. That requires persistently storing --attach info in container.

The additional --attach information will be added to container struct in containerd and stored in container store.

If --attach is also included in nerdctl start for this container, the task will be created with the one in nerdctl start. Otherwise, it will use the one in container store.

The only benefit I can think of adding --attach in nerdctl create is to make it more docker-compatible. The functionality seems already covered by --attach flag in nerdctl create.

Do you think it is good to add --attach in nerdctl create with the above approach? Or is it better to save the duplicate functionality even it is not Docker-compatible? Let me know the tenets you prefer to follow, or if you have better ideas or concerns.

cc @AkihiroSuda as you reviewed the previous PR to add --attach in nerdctl start

@djdongjin
Copy link
Member

The additional --attach information will be added to container struct in containerd and stored in container store.

Do you mean add a new field to container struct or utilize existing fields? I think you can utilize Labels for this (or maybe Extensions? I don't know much about Extensions but from its comment Extensions stores client-specified metadata). You can add a label for flagA during create and check its value during start.

The only benefit I can think of adding --attach in nerdctl create is to make it more docker-compatible. The functionality seems already covered by --attach flag in nerdctl create.

That's also what I thought (since nerdctl create+nerdctl start -a = nerdctl create -a + nerdctl start). I don't have strong preference on whether or not to have the feature. The above label method should not add much code anyway :)

@swagatbora90 swagatbora90 linked a pull request Feb 28, 2025 that will close this issue
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants