Pick default OCI Runtime from containers.conf #2926
Conversation
|
@containers/podman-maintainers PTAL |
util/types.go
Outdated
| DefaultRuntime = "runc" | ||
| // DefaultRuntime should no longer be used. | ||
| // Default runtime should come from containers.conf | ||
| DefaultRuntime = "crun" |
There was a problem hiding this comment.
I think this should be "runc" for now. It's safe for Fedora but who knows in which packages this ends up.
There was a problem hiding this comment.
I actually think we should remove this altogether or maybe return "". This field should never be used. Basically it would only ever be used if containers/common had a serious issue, where their was a coding mistake in the code, which has already gone through multiple layers of testing.
There was a problem hiding this comment.
But to get it through, I changed it back. Also fixed the message if containers.conf is screwed up to Warnf level.
util/util.go
Outdated
| return DefaultRuntime | ||
| conf, err := config.Default() | ||
| if err != nil { | ||
| logrus.Debugf("Error loading container config when searching for local runtime.") |
There was a problem hiding this comment.
| logrus.Debugf("Error loading container config when searching for local runtime.") | |
| logrus.Debugf("Error loading container config when searching for local runtime: %v", err) |
I wonder whether this error should be fatal. I didn't check the code but I would assume that we don't get an error if the config does not exist ... but if there's some real config/syntax/permission error here, then it would be very hard to figure out.
| conf, _ := config.Default() | ||
| defaultRuntime := conf.Engine.OCIRuntime | ||
| runtime := Runtime() | ||
| if runtime != defaultRuntime { |
There was a problem hiding this comment.
Maybe this is better?
if runtime != defaultRuntime {
t.Fatalf("expected %v, got %v", runtime, defaultRuntime)
}
than
if runtime != defaultRuntime {
t.Fatalf("expected %v, got %v", "crun", defaultRuntime)
}
Another thing, in the commit message "but Buildah is still falling
back to use crun " => "but Buildah is still falling
back to use runc"
|
Everything commented on was merged and fixed. |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: rhatdan, saschagrunert 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:
Approvers can indicate their approval by writing |
|
@rhatdan needs a rebase :( |
Currently we have a weird situation where the user sets the default runtime in his containers.conf for podman but Buildah is still falling back to use runc because it was hard coded as the default for Buildah. I would like to remove this default, but that would theoretically break the API promise of Buildah. This should fix containers/podman#8893 Signed-off-by: Daniel J Walsh <[email protected]>
|
New changes are detected. LGTM label has been removed. |
|
LGTM |
Currently we have a weird situation where the user sets the default
runtime in his containers.conf for podman but Buildah is still falling
back to use runc because it was hard coded as the default for Buildah.
I would like to remove this default, but that would theoretically break
the API promise of Buildah.
This should fix containers/podman#8893
Signed-off-by: Daniel J Walsh [email protected]
What type of PR is this?
What this PR does / why we need it:
How to verify it
Which issue(s) this PR fixes:
Special notes for your reviewer:
Does this PR introduce a user-facing change?