composefs fixes #2069
composefs fixes #2069
Conversation
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: giuseppe 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 |
|
Is this aiming to fix #2042 ? |
drivers/overlay/overlay.go
Outdated
| if err := fileutils.Exists(mergedDir); err != nil && os.IsNotExist(err) { | ||
| if err := idtools.MkdirAllAs(mergedDir, 0o700, rootUID, rootGID); err != nil && !os.IsExist(err) { |
There was a problem hiding this comment.
I know this code existed before and you're just moving it but...IMO doing stat+mkdir is just unnecessary syscall traffic - we're already checking !os.IsExist from the MkdirAllAs invocation.
(BTW in almost every code base I'm involved with I end up with an EnsureDirectory helper API which is the equivalent of these two things)
There was a problem hiding this comment.
I think we added the outer fileutils.Exists because MkdirAllAs chowns an existing directory, while in this case we don't want to do it when the merged directory exists (it might be in an additional store).
I'll change it to use MkdirAllAndChownNew
| // not a composefs layer, ignore it | ||
| continue | ||
| } | ||
| dir, err := os.MkdirTemp(d.runhome, "composefs-mnt") | ||
| fd, err := openComposefsMount(composefsData) |
There was a problem hiding this comment.
but access directly the files through the mount fd.
Huh...I didn't know one could do that...interesting and quite useful!
There was a problem hiding this comment.
Reminder to myself and anyone interested in more: https://brauner.io/2023/02/28/mounting-into-mount-namespaces.html notes this:
... and with the added benefit that the mount never actually had to appear anywhere in the filesystem hierarchy and thus never had to belong to any mount namespace. This alone is already a very powerful tool but we won’t go into depth today.
Signed-off-by: Giuseppe Scrivano <gscrivan@redhat.com>
this is a preparation patch for the next commit Signed-off-by: Giuseppe Scrivano <gscrivan@redhat.com>
but access directly the files through the mount fd. Signed-off-by: Giuseppe Scrivano <gscrivan@redhat.com>
move the check for a previously mounted "merged" directory before attempting any composefs mount. It prevents mounting the composefs blobs to then throw them away as it reuses the already existing mounted path when possible. Signed-off-by: Giuseppe Scrivano <gscrivan@redhat.com>
bcbeb7d to
57a4177
Compare
Since it fails on |
use MkdirAllAndChownNew instead of checking for the directory existence first and then create it if missing. Signed-off-by: Giuseppe Scrivano <gscrivan@redhat.com>
|
LGTM |
OK, makes sense - that's the kind of thing that I think would be good in one of the commit messages just for future reference for others to avoid needing to backref to the PR: say in f6ca317 or so. But that's just something for future changes |
some improvements for composefs, more details in each patch