support for storage for rootless containers #936
Conversation
|
tests are probably failing for opencontainers/runc#1816 |
d56bed4 to
ad2b673
Compare
|
@rhatdan the patch for runc got merged (b222ea4469dd5e304f3f6cf7a2482860285639a2), could we get an updated build? |
d4d1a8f to
8b3bf64
Compare
|
☔ The latest upstream changes (presumably 4b4de5d) made this pull request unmergeable. Please resolve the merge conflicts. |
19083c3 to
d67feb3
Compare
|
☔ The latest upstream changes (presumably 16ea659) made this pull request unmergeable. Please resolve the merge conflicts. |
9d8f9cf to
a8b8614
Compare
|
This is currently ready for review. The updated runc is used only on Travis right now (and tests are happy there). Everywhere else, we need a newer runc |
pkg/rootless/rootless.go
Outdated
| func BecomeRootInUserNS() (bool, error) { | ||
| runtime.LockOSThread() | ||
|
|
||
| if os.Getuid() == 0 || os.Getenv("_LIBPOD_USERNS_CONFIGURED") != "" { |
There was a problem hiding this comment.
Should this happen before LockOSThread()? Seems like it doesn't need to be guaranteed to run in a single thread, and this should be the typical case, so we can save some time by not calling LockOSThread() in this case
Having different runc versions in different places makes me jumpy. Is there something that's blocking us bumping to a single new version of runc in all of our locations ( The global user namespace approach you have here is just mapping a single user and group to 0 (e.g. you're not using |
|
@wking, there are multiple users mapped when the user has additional uids/gids defined in I think we are not vendoring runc but using the version from the host, that is why we are dealing with different versions on different systems :-( |
|
I've kept the implementation in a new pkg so that it can be more easily re-used. @nalind could this be useful for Buildah? |
c5f6028 to
b1e5b52
Compare
When running podman as non root user always create an userNS and let the OCI runtime use it. Signed-off-by: Giuseppe Scrivano <gscrivan@redhat.com>
Signed-off-by: Giuseppe Scrivano <gscrivan@redhat.com>
Additional groups are not allowed in an userNS. Signed-off-by: Giuseppe Scrivano <gscrivan@redhat.com>
Signed-off-by: Giuseppe Scrivano <gscrivan@redhat.com>
If the file exists, use it to read the configuration. Signed-off-by: Giuseppe Scrivano <gscrivan@redhat.com>
c4f0a9d to
b1228c9
Compare
cmd/podman/libpodruntime/runtime.go
Outdated
| if dataDir != "" { | ||
| opts.GraphRoot = filepath.Join(dataDir, "containers", "storage") | ||
| } else { | ||
| home := os.Getenv("HOME") | ||
| if home == "" { | ||
| return opts, fmt.Errorf("HOME not specified") | ||
| } | ||
| opts.GraphRoot = filepath.Join(home, ".containers", "storage") | ||
| opts.GraphRoot = filepath.Join(home, ".local", "share", "containers", "storage") |
There was a problem hiding this comment.
The spec has:
If
$XDG_DATA_HOMEis either not set or empty, a default equal to$HOME/.local/shareshould be used.
A simpler implementation of that would be:
dataDir := os.Getenv("XDG_DATA_HOME")
if dataDir == "" {
home := os.Getenv("HOME")
if home == "" {
return opts, fmt.Errorf("neither XDG_DATA_HOME nor HOME was set non-empty")
}
dataDir = filepath.Join(home, ".local", "share")
}
// then append to dataDir to construct GraphRootThis sort of generic XDG support is something that you can get from XDG libraries as well. For example, here.
Once you have dataDir populated, I think we should be using a single containers-storage instead of two directories. Again from the spec:
Other specifications may reference this specification by specifying the location of a data file as
$XDG_DATA_DIRS/subdir/filename. This implies that:
Such file should be installed to
$datadir/subdir/filenamewith$datadirdefaulting to/usr/share.A user specific version of the data file may be created in
$XDG_DATA_HOME/subdir/filename, taking into account the default value for$XDG_DATA_HOMEif$XDG_DATA_HOMEis not set.Lookups of the data file should search for
./subdir/filenamerelative to all base directories specified by$XDG_DATA_HOMEand$XDG_DATA_DIRS. If an environment variable is either not set or empty, its default value as defined by this specification should be used instead.
containers-storage would be the subdir specific to the containers/storage library. I don't see a need to group all of the container data under a containers/ directory, because it makes managing that shared directory more complicated. dataDir will still be a shared directory potentially consumed by several packages, so sharing directories is obviously something that can work, but I think containers/storage is unnecessarily deep.
Finally, the chained-lookup point at the end of the XDG quote above is something that we'd have to approach by patching containers/storage itself. If we end up patching containers/storage to support the XDG spec (which seems useful to me), we probably won't need any code here. We can just leave GraphRoot empty, and let containers/storage pull it in from the XDG variables and its internal subdir/filename, etc. defaults.
There was a problem hiding this comment.
Lets open a PR for containers/storage and see what @nalind thinks, but for now, we can handle this here, and just get this PR merged, when it is ready.
There was a problem hiding this comment.
I am in favour of keeping the two levels containers/storage as we also have containers/atomic
There was a problem hiding this comment.
... but for now, we can handle this here, and just get this PR merged, when it is ready.
The dataDir unification in my earlier comment (which DRYs up the current two lines with "containers", "storage") can happen in this PR without blocking on upstream containers/storage work.
|
Yes lets keep them separate directories. |
Signed-off-by: Giuseppe Scrivano <gscrivan@redhat.com>
Signed-off-by: Giuseppe Scrivano <gscrivan@redhat.com>
b1228c9 to
2bc4eb0
Compare
|
I addressed all the comments above, is there anything more blocking it? |
|
Now that #1001 is in, I have no more objections. LGTM. |
|
📌 Commit 2bc4eb0 has been approved by |
Signed-off-by: Giuseppe Scrivano <gscrivan@redhat.com> Closes: #936 Approved by: rhatdan
Additional groups are not allowed in an userNS. Signed-off-by: Giuseppe Scrivano <gscrivan@redhat.com> Closes: #936 Approved by: rhatdan
Signed-off-by: Giuseppe Scrivano <gscrivan@redhat.com> Closes: #936 Approved by: rhatdan
If the file exists, use it to read the configuration. Signed-off-by: Giuseppe Scrivano <gscrivan@redhat.com> Closes: #936 Approved by: rhatdan
Signed-off-by: Giuseppe Scrivano <gscrivan@redhat.com> Closes: #936 Approved by: rhatdan
Signed-off-by: Giuseppe Scrivano <gscrivan@redhat.com> Closes: #936 Approved by: rhatdan
|
☀️ Test successful - status-papr |
|
thanks a lot @giuseppe ! |
continuation of: #871