oci: allow to define multiple OCI runtimes #2079

giuseppe · 2019-01-04T10:28:10Z

we can define multiple OCI runtimes that can be chosen with --runtime.

in libpod.conf is possible to specify them with:

 [runtimes]
    foo = [
                 "/usr/bin/foo",
                 "/usr/sbin/foo",
    ]
    bar = [
                 "/usr/bin/foo",
                 "/usr/sbin/foo",
    ]

If the argument to --runtime is an absolute path then it is used directly without any lookup in the configuration.

Closes: #1750

Signed-off-by: Giuseppe Scrivano [email protected]

TomSweeneyRedHat · 2019-01-04T12:59:24Z

libpod.conf

+	     "/sbin/runc",
+	     "/bin/runc",
+	     "/usr/lib/cri-o-runc/sbin/runc"
+]


Is there an extra space before the first entry /usr/bin/runc/?

rhatdan · 2019-01-04T14:05:51Z

Should we add paths for kata? gVisor?

TomSweeneyRedHat · 2019-01-04T14:13:44Z

docs/podman.1.md

@@ -64,7 +64,7 @@ Default state dir is configured in /etc/containers/storage.conf.

 **--runtime**=**value**

-Path to the OCI compatible binary used to run containers
+Name of the OCI runtime as specified in libpod.conf or absolute path to the OCI compatible binary used to run containers.


I think I'd not add the new bit, or maybe just "Absolute path" and the ending period. The names in the conf file are absolute paths anyway. When I first read this, I was looking for some kind of key value, i.e. "runc" vs "/usr/bin/runc"

yes, you can specify an absolute path instead of a runtime name. if it is an absolute path like "/foo/bar" then it is not looked up in the config file but used directly.

TomSweeneyRedHat · 2019-01-04T14:17:07Z

libpod/runtime.go

+				"/sbin/runc",
+				"/bin/runc",
+				"/usr/lib/cri-o-runc/sbin/runc",
+			},


Anyway to use the values from libpod.conf here?

how? This is used only if libpod.conf doesn't override the default specified here

mheon · 2019-01-04T14:35:10Z

@giuseppe Can you add the name of the runtime a container was created with to its config? We don't have to do anything with it yet, but once we flesh this out more I'd like to be able to let containers decide what runtime to use based on that.

mheon · 2019-01-04T14:35:36Z

(If we don't have a name, I think "" is fine - we'll just let the container use whatever the default is)

rh-atomic-bot · 2019-01-07T01:16:02Z

☔ The latest upstream changes (presumably #2090) made this pull request unmergeable. Please resolve the merge conflicts.

giuseppe · 2019-01-07T07:46:51Z

@giuseppe Can you add the name of the runtime a container was created with to its config? We don't have to do anything with it yet, but once we flesh this out more I'd like to be able to let containers decide what runtime to use based on that.

added a patch to store it to the container configuration.

giuseppe · 2019-01-07T07:49:16Z

Should we add paths for kata? gVisor?

I don't see them in Fedora yet

rhatdan · 2019-01-07T13:06:59Z

I think @lsm5 started looking at packaging them. But not sure if it went forward.

lsm5 · 2019-01-07T13:20:46Z

most of the packages have been added (in rawhide, and built for others but not submitted yet) but there's still some kernel config changes that we need to get resolved.

giuseppe · 2019-01-08T08:30:17Z

I think @lsm5 started looking at packaging them. But not sure if it went forward.

If you are fine with it, let's add them once we have on Fedora and we can test them

mheon · 2019-01-10T15:00:10Z

/approve
Not including in 1.0 branch

openshift-ci-robot · 2019-01-10T15:00:19Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: mheon

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

~~OWNERS~~ [mheon]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

libpod/runtime.go

pkg/spec/createconfig.go

rh-atomic-bot · 2019-01-11T10:26:12Z

☔ The latest upstream changes (presumably #2105) made this pull request unmergeable. Please resolve the merge conflicts.

rhatdan · 2019-01-11T13:36:45Z

Needs rebase.

vrothberg

Overall LGTM, just a minor potential issue regarding absolute paths and keys in the map.

vrothberg · 2019-01-14T08:48:44Z

Overall LGTM, just a minor potential issue regarding absolute paths and keys in the map.

Looks like I missed creating the comment. My concern was that a user could specify an absolute path as a key in the libpod.conf but apparently, keys cannot contain a '/' in toml.

This deprecates the libpod.conf variable of `runtime_path=`, and now has `runtimes=`, like a map for naming the runtime, preparing for a `--runtime` flag to `podman run` (i.e. runc, kata, etc.) Reference: containers#1750 Signed-off-by: Vincent Batts <[email protected]>

we can define multiple OCI runtimes that can be chosen with --runtime. in libpod.conf is possible to specify them with: [runtimes] foo = [ "/usr/bin/foo", "/usr/sbin/foo", ] bar = [ "/usr/bin/foo", "/usr/sbin/foo", ] If the argument to --runtime is an absolute path then it is used directly without any lookup in the configuration. Closes: containers#1750 Signed-off-by: Giuseppe Scrivano <[email protected]>

Signed-off-by: Giuseppe Scrivano <[email protected]>

giuseppe · 2019-01-14T09:24:24Z

Overall LGTM, just a minor potential issue regarding absolute paths and keys in the map.

the config map should not contain any '/'.

When the specified runtime starts with '/' then it is treated differently and no lookup is done

rhatdan · 2019-01-14T11:29:44Z

libpod/runtime.go

 		foundRuntime = true
-		runtime.ociRuntimePath = path
-		break
+		runtime.ociRuntimePath = OCIRuntimePath{Name: filepath.Base(runtime.config.OCIRuntime), Paths: []string{runtime.config.OCIRuntime}}


Should we check here if the path exists and return an error?

we check for it later, if I try:

$ podman --runtime /foo/bar run alpine echo hi error creating libpod runtime: could not find a working binary (configured options: [/foo/bar]): invalid argument

rhatdan · 2019-01-16T11:31:22Z

/lgtm

openshift-ci-robot requested review from mheon and vrothberg January 4, 2019 10:28

openshift-ci-robot added the size/L label Jan 4, 2019

giuseppe mentioned this pull request Jan 4, 2019

--runtime= flag #1750

Closed

giuseppe force-pushed the multiple-runtimes branch 2 times, most recently from 56a29fb to 47ef665 Compare January 4, 2019 12:27

TomSweeneyRedHat reviewed Jan 4, 2019

View reviewed changes

giuseppe force-pushed the multiple-runtimes branch from 47ef665 to 888bc99 Compare January 4, 2019 15:49

openshift-ci-robot added size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. and removed size/L labels Jan 4, 2019

openshift-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jan 7, 2019

giuseppe force-pushed the multiple-runtimes branch from 888bc99 to 87587a4 Compare January 7, 2019 07:28

openshift-ci-robot added size/L and removed needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. labels Jan 7, 2019

giuseppe force-pushed the multiple-runtimes branch from 4590b55 to 3523887 Compare January 9, 2019 10:58

openshift-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jan 10, 2019

mheon reviewed Jan 10, 2019

View reviewed changes

libpod/runtime.go Outdated Show resolved Hide resolved

mheon reviewed Jan 10, 2019

View reviewed changes

pkg/spec/createconfig.go Outdated Show resolved Hide resolved

openshift-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jan 13, 2019

vrothberg reviewed Jan 14, 2019

View reviewed changes

vbatts and others added 3 commits January 14, 2019 10:03

config: store the runtime used to create each container

3b37101

Signed-off-by: Giuseppe Scrivano <[email protected]>

giuseppe force-pushed the multiple-runtimes branch from 3523887 to 3b37101 Compare January 14, 2019 09:22

openshift-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jan 14, 2019

rhatdan reviewed Jan 14, 2019

View reviewed changes

openshift-ci-robot assigned rhatdan Jan 16, 2019

openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Jan 16, 2019

openshift-merge-robot merged commit d868321 into containers:master Jan 16, 2019

github-actions bot added the locked - please file new issue/PR Assist humans wanting to comment on an old issue or PR with locked comments. label Sep 27, 2023

github-actions bot locked as resolved and limited conversation to collaborators Sep 27, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

oci: allow to define multiple OCI runtimes #2079

oci: allow to define multiple OCI runtimes #2079

giuseppe commented Jan 4, 2019 •

edited

Loading

TomSweeneyRedHat Jan 4, 2019

rhatdan commented Jan 4, 2019

TomSweeneyRedHat Jan 4, 2019

giuseppe Jan 4, 2019

TomSweeneyRedHat Jan 4, 2019

giuseppe Jan 7, 2019

mheon commented Jan 4, 2019

mheon commented Jan 4, 2019

rh-atomic-bot commented Jan 7, 2019

giuseppe commented Jan 7, 2019

giuseppe commented Jan 7, 2019

rhatdan commented Jan 7, 2019

lsm5 commented Jan 7, 2019

giuseppe commented Jan 8, 2019

mheon commented Jan 10, 2019

openshift-ci-robot commented Jan 10, 2019

rh-atomic-bot commented Jan 11, 2019

rhatdan commented Jan 11, 2019

vrothberg left a comment •

edited

Loading

vrothberg commented Jan 14, 2019

giuseppe commented Jan 14, 2019

rhatdan Jan 14, 2019

giuseppe Jan 16, 2019

rhatdan commented Jan 16, 2019

oci: allow to define multiple OCI runtimes #2079

oci: allow to define multiple OCI runtimes #2079

Conversation

giuseppe commented Jan 4, 2019 • edited Loading

TomSweeneyRedHat Jan 4, 2019

Choose a reason for hiding this comment

rhatdan commented Jan 4, 2019

TomSweeneyRedHat Jan 4, 2019

Choose a reason for hiding this comment

giuseppe Jan 4, 2019

Choose a reason for hiding this comment

TomSweeneyRedHat Jan 4, 2019

Choose a reason for hiding this comment

giuseppe Jan 7, 2019

Choose a reason for hiding this comment

mheon commented Jan 4, 2019

mheon commented Jan 4, 2019

rh-atomic-bot commented Jan 7, 2019

giuseppe commented Jan 7, 2019

giuseppe commented Jan 7, 2019

rhatdan commented Jan 7, 2019

lsm5 commented Jan 7, 2019

giuseppe commented Jan 8, 2019

mheon commented Jan 10, 2019

openshift-ci-robot commented Jan 10, 2019

rh-atomic-bot commented Jan 11, 2019

rhatdan commented Jan 11, 2019

vrothberg left a comment • edited Loading

Choose a reason for hiding this comment

vrothberg commented Jan 14, 2019

giuseppe commented Jan 14, 2019

rhatdan Jan 14, 2019

Choose a reason for hiding this comment

giuseppe Jan 16, 2019

Choose a reason for hiding this comment

rhatdan commented Jan 16, 2019

giuseppe commented Jan 4, 2019 •

edited

Loading

vrothberg left a comment •

edited

Loading