feat: add detachKeys option to container start #159
Conversation
coderbirju
force-pushed
the
update-start-options
branch
2 times, most recently
from
5f4ffdd to
27061be
Compare
| "github.com/runfinch/finch-daemon/pkg/errdefs" | ||
| ) | ||
|
|
||
| func (s *service) Restart(ctx context.Context, cid string, timeout time.Duration) error { | ||
| con, err := s.getContainer(ctx, cid) | ||
| func (s *service) Restart(ctx context.Context, containers []string, options types.ContainerRestartOptions) error { |
There was a problem hiding this comment.
Why do we need to change from taking cid string to containers []string?
There was a problem hiding this comment.
Nerdctl currentl accepts container id's in []string format.
I think making our functions take cid in input was the right approach, we can send the cid in the format that nerdctl expects when we make that call.
Starting multiple containers is not a feature that docker supports at the moment - https://docs.docker.com/reference/api/engine/version/v1.43/#tag/Container/operation/ContainerStart
I will update this.
api/handlers/container/container.go
Outdated
| @@ -22,9 +22,9 @@ type Service interface { | |||
| GetPathToFilesInContainer(ctx context.Context, cid string, path string) (string, func(), error) | |||
| Remove(ctx context.Context, cid string, force, removeVolumes bool) error | |||
| Wait(ctx context.Context, cid string, condition string) (code int64, err error) | |||
| Start(ctx context.Context, cid string) error | |||
| Start(ctx context.Context, containers []string, options ncTypes.ContainerStartOptions) error | |||
There was a problem hiding this comment.
Does docker api support starting multiple containers?
There was a problem hiding this comment.
Pasting from another thread
Nerdctl currentl accepts container id's in []string format.
I think making our functions take cid in input was the right approach, we can send the cid in the format that nerdctl expects when we make that call.
Starting multiple containers is not a feature that docker supports at the moment - https://docs.docker.com/reference/api/engine/version/v1.43/#tag/Container/operation/ContainerStart
I will update this.
api/handlers/container/container.go
Outdated
| Stop(ctx context.Context, cid string, timeout *time.Duration) error | ||
| Restart(ctx context.Context, cid string, timeout time.Duration) error | ||
| Restart(ctx context.Context, containers []string, options ncTypes.ContainerRestartOptions) error |
api/handlers/container/restart.go
Outdated
| ctx := namespaces.WithNamespace(r.Context(), h.Config.Namespace) | ||
| err = h.service.Restart(ctx, cid, timeout) | ||
| containers := [1]string{cid} |
There was a problem hiding this comment.
May just pass cid directly, if we are not planning to support multiple containers
| err = h.service.Restart(ctx, cid, timeout) | ||
| containers := [1]string{cid} | ||
| globalOpt := ncTypes.GlobalCommandOptions(*h.Config) | ||
| options := ncTypes.ContainerRestartOptions{ |
There was a problem hiding this comment.
There was a problem hiding this comment.
nerdctl doesn't yet have signal option in restart, I have a WIP PR for that
https://github.com/coderbirju/nerdctl/pull/12/files
| @@ -17,7 +19,26 @@ import ( | |||
| func (h *handler) start(w http.ResponseWriter, r *http.Request) { | |||
| cid := mux.Vars(r)["id"] | |||
| ctx := namespaces.WithNamespace(r.Context(), h.Config.Namespace) | |||
| err := h.service.Start(ctx, cid) | |||
|
|
|||
| detachKeys := r.URL.Query().Get("detachKeys") | |||
There was a problem hiding this comment.
Can we add some validation for possible detachkeys values?
There was a problem hiding this comment.
According to this documentation user can provide an arbitrary sequence of their choice to detached from the container - https://docs.docker.com/reference/cli/docker/container/attach/#detach-keys
I can add some sort of validation to check if the sequence they've provided is in the appropriate format
There was a problem hiding this comment.
Added validation
| options := ncTypes.ContainerStartOptions{ | ||
| GOptions: globalOpt, | ||
| Stdout: devNull, | ||
| Attach: false, |
There was a problem hiding this comment.
nerdctl start supports attach https://github.com/containerd/nerdctl/blob/main/docs/command-reference.md#whale-nerdctl-start. Can we pass it, instead of setting this to False by default.
There was a problem hiding this comment.
Would that be an additional option in container start API?
Docker doesn't support it at the moment - https://docs.docker.com/reference/api/engine/version/v1.43/#tag/Container/operation/ContainerStart
There was a problem hiding this comment.
I see. Looks like docker-cli makes a separate ContainerAttach call for that https://github.com/docker/cli/blob/master/cli/command/container/start.go#L113.
The current implementation looks good to me, but we should follow up later on the end to end Start with attach behavior
coderbirju
force-pushed
the
update-start-options
branch
from
27061be to
1302f27
Compare
Signed-off-by: Arjun Raja Yogidas <arjunry@amazon.com>
coderbirju
force-pushed
the
update-start-options
branch
from
1302f27 to
d8bb17b
Compare
Signed-off-by: Arjun Raja Yogidas <arjunry@amazon.com>
Issue #, if available:
Description of changes:
Testing done:
License Acceptance
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.