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

[WIP]Fix the --runtime option of container create to support path #2411

Draft
wants to merge 1 commit into
base: main
Choose a base branch
from
Draft

[WIP]Fix the --runtime option of container create to support path #2411

wants to merge 1 commit into from

Conversation

yankay
Copy link
Contributor

@yankay yankay commented Aug 2, 2023 •
edited
Loading

Description

Unlike ctr , the nerdctl does not support the --runtime flag while creating a container with path

Steps to reproduce the issue

# Copy runc to another path
cp  /usr/local/bin/containerd-shim-runc-v2 /tmp/haha

# create a container with the path
nerdctl container create --runtime /tmp/haha docker.m.daocloud.io/library/nginx:alpine

ctr -n k8s.io container ls
CONTAINER                                                           IMAGE                                        RUNTIME
8e7e104ba64f9aa8d320fe1972d4d62634d920151612c4f0e0fc0fb12b51ace6    docker.m.daocloud.io/library/nginx:alpine    io.containerd.runc.v2

After the fix.

# create a container with the path
root@kay201:~/oss/nerdctl# ./_output/nerdctl container create --runtime /tmp/haha docker.m.daocloud.io/library/nginx:alpine
ctr -n k8s.io container ls
95fadeb4c7e1aadf13698fa4abb1c8d34b16e23444fe9c37beb84a97cb31037f
CONTAINER                                                           IMAGE                                        RUNTIME
95fadeb4c7e1aadf13698fa4abb1c8d34b16e23444fe9c37beb84a97cb31037f    docker.m.daocloud.io/library/nginx:alpine    /tmp/haha

The runtime flag with path can be configed successfully.

And it can run sucessfuly:

root@kay201:~/oss/nerdctl# ./_output/nerdctl run --runtime /tmp/haha docker.m.daocloud.io/library/nginx:alpine
/docker-entrypoint.sh: /docker-entrypoint.d/ is not empty, will attempt to perform configuration
/docker-entrypoint.sh: Looking for shell scripts in /docker-entrypoint.d/
/docker-entrypoint.sh: Launching /docker-entrypoint.d/10-listen-on-ipv6-by-default.sh
10-listen-on-ipv6-by-default.sh: info: Getting the checksum of /etc/nginx/conf.d/default.conf

AkihiroSuda
AkihiroSuda requested changes Aug 2, 2023
View reviewed changes
Copy link
Member

@AkihiroSuda AkihiroSuda left a comment •
edited
Loading

Choose a reason for hiding this comment

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

This doesn't work for --runtime=/usr/local/bin/crun, --runtime=runsc, etc.

@yankay yankay changed the title Fix the --runtime option of container create is ignored [WIP]Fix the --runtime option of container create is ignored Aug 3, 2023
@yankay yankay closed this Aug 4, 2023
@yankay yankay reopened this Aug 4, 2023
@yankay yankay force-pushed the fix-runtime-option-is-ignored branch 4 times, most recently from a1dfb02 to e7b91a5 Compare August 4, 2023 10:06
fahedouch
fahedouch reviewed Aug 6, 2023
View reviewed changes
pkg/cmd/container/run_runtime.go Outdated
if !strings.HasPrefix(runtimeStr, "io.containerd.runc.") {
if cgroupManager == "systemd" {
logrus.Warnf("cannot set cgroup manager to %q for runtime %q", cgroupManager, runtimeStr)
}
runtimeOpts = nil
}
} else {
} else if filepath.IsAbs(runtimeStr) {
// runtimeStr is a runc binary
Copy link
Member

Choose a reason for hiding this comment

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

why runtimeStr is a runc binary here?

Copy link
Member

Choose a reason for hiding this comment

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

To support --runtime=/usr/local/bin/crun, --runtime=runsc, etc.
(runc-compatible third-party runtime binaries are called "runc binary" here)

Copy link
Member

Choose a reason for hiding this comment

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

--runtime=runsc is not absolute path

@yankay yankay force-pushed the fix-runtime-option-is-ignored branch 4 times, most recently from 5218242 to 2c8aa16 Compare August 7, 2023 06:40
@fahedouch
Copy link
Member

test is failing

test target: "nerdctl"
=== RUN   TestComposeUpDetailedError
    compose_up_test.go:54: assertion failed: 
        Command:  /usr/local/bin/nerdctl --namespace=nerdctl-test compose -f /tmp/nerdctl-compose-test3932546953/docker-compose.yaml up -d
        ExitCode: 0
        Stdout:   
        Stderr:   time="2023-08-07T07:04:15Z" level=info msg="Creating network nerdctl-compose-test3932546953_default"
        time="2023-08-07T07:04:15Z" level=info msg="Ensuring image ghcr.io/stargz-containers/alpine:3.13-org"
        time="2023-08-07T07:04:15Z" level=info msg="Creating container nerdctl-compose-test3932546953-foo-1"
        
        
        Failures:
        ExitCode was 0 expected 1
        Expected stderr to contain "failed to resolve runtime path: invalid runtime name invalid"

@yankay yankay force-pushed the fix-runtime-option-is-ignored branch from 2c8aa16 to 88a0813 Compare August 7, 2023 07:56
@yankay yankay changed the title [WIP]Fix the --runtime option of container create is ignored [WIP ]Fix the --runtime option of container create to support path Aug 7, 2023
@yankay yankay force-pushed the fix-runtime-option-is-ignored branch from 88a0813 to 9bbe3ea Compare August 7, 2023 09:44
@yankay yankay changed the title [WIP ]Fix the --runtime option of container create to support path [WIP]Fix the --runtime option of container create to support path Aug 7, 2023
@yankay yankay force-pushed the fix-runtime-option-is-ignored branch 3 times, most recently from d78d883 to ef8f1c9 Compare August 8, 2023 08:41
@fahedouch
Copy link
Member

do we have the same behavior as docker ?

@AkihiroSuda
Copy link
Member

do we have the same behavior as docker ?

No, Docker needs explicit configuration in daemon.json.

For nerdctl, we support strings like

  • “io.containerd.runc.v2”
  • “runc”, “crun”, “runsc” etc.
  • “/usr/local/bin/runc” etc.

AkihiroSuda
AkihiroSuda reviewed Aug 8, 2023
View reviewed changes
pkg/cmd/container/run_runtime.go
@@ -47,6 +48,9 @@ func generateRuntimeCOpts(cgroupManager, runtimeStr string) ([]containerd.NewCon
}
runtimeOpts = nil
}
} else if filepath.IsAbs(runtimeStr) {
// runtimeStr is absolute path to runtime binary
Copy link
Member

Choose a reason for hiding this comment

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

Runc-compatible binary should be assumed unless the binary name is composed of reversed domains

Copy link
Member

Choose a reason for hiding this comment

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

We can also consider “CSV” values like “type=runc,path=foo”, “type=containerd,name=com.example.foo.v1”, “type=containerd,path=containerd-shim-foo-v1”

@AkihiroSuda AkihiroSuda marked this pull request as draft August 8, 2023 22:48
@fahedouch
Copy link
Member

fahedouch commented Aug 9, 2023 •
edited
Loading

do we have the same behavior as docker ?

No, Docker needs explicit configuration in daemon.json.

For nerdctl, we support strings like

  • “io.containerd.runc.v2”
  • “runc”, “crun”, “runsc” etc.
  • “/usr/local/bin/runc” etc.

I mean this behavior

root@kay200:~# docker container create --runtime com.example.sample.v1 docker.m.daocloud.io/nginx:alpine
Error response from daemon: Unknown runtime specified com.example.sample.v1

@fahedouch
Copy link
Member

@yankay PTAL

@apostasie
Copy link
Contributor

hey @yankay !

Wondering if this is still something you are interested in carrying through?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@AkihiroSuda AkihiroSuda AkihiroSuda requested changes

@fahedouch fahedouch fahedouch left review comments

Requested changes must be addressed to merge this pull request.

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@yankay @fahedouch @AkihiroSuda @apostasie