This would ignore all kinds of errors; is there a way to restrict this to chown errors only?

if !inAdditionalStore { MkdirAllAs } else { MkdirAll } seems to be an option. I don’t immediately know about the Rmdir one.

Or, really, if the store is supposed to be read-only, then why change it at all? Test that the directories exist, and fail if they don’t.

But if the merged directory is supposed to be created on a Get and always deleted on a Put, I can’t see how that can possibly work as truly read-only additional image store: a “legitimate” image store won’t have the merged directory, and we won’t be able to create it. Am I missing something?

the correct test would be to check whether the directory has the ContainersOverrideXattr xattr set and assume fuse-overlayfs is used, in this case the mode specified in the xattr will be used, no matter what is the mode on the directory itself. But do we care to make it so specific and not just try in any case?

If you prefer, I can rewrite it as:

if !inAdditionalStore { MkdirAllAs } else { // do nothing, just assume it exists }

are you ok with it?

(Warning: I don’t understand the full feature, I’m going just by the bug report and the PR.)

Yes, the talk about “read-only additional image store” would lead to basically what you propose. I don’t feel too strongly about the “test that the directories exist” part. But then I saw the Remove of merged — and I don’t understand how that could work. I assume it does actually work for you — so I’m missing something trivial.

Yes, the talk about “read-only additional image store” would lead to basically what you propose. I don’t feel too strongly about the “test that the directories exist” part. But then I saw the Remove of merged — and I don’t understand how that could work. I assume it does actually work for you — so I’m missing something trivial.

I am not even sure why we need to rm the mount directory, we could just drop that code IMO, so we don't have to worry whether the store is read-only or writeable

Git blame points at #390 .

And I’m still interested in understanding how this works in the first place, in #1828 (review) . (Admittedly I didn’t try that in practice.)

And I’m still interested in understanding how this works in the first place, in #1828 (review) . (Admittedly I didn’t try that in practice.)

the question is: should we fail when the directory in the additional image store (which is supposed to be R/O) doesn't exist but the user has enough privileges to create it? Or should we try to create it anyway?

	if err := idtools.MkdirAs(mergedDir, 0o700, rootUID, rootGID); err != nil && !os.IsExist(err) {
		if !inAdditionalStore {
			return "", err
		}
		// if it is in an additional store, do not fail if the directory already exists
		if _, err2 := os.Stat(mergedDir); err2 != nil {
			return "", err
		}
	}

Oh I don’t have a strong opinion on that design question.

I’m stuck at an earlier phase: If the user doesn’t have enough privileges, I understand mergedDir should not exist most of the time. So how does this work when we can’t create it, and it doesn’t exist? … and if it doesn’t, the primary goal of this PR can’t be achieved at all AFAICT.

I understand mergedDir should not exist most of the time.

if you just perform a "pull" then the directory exists, at least until you try to mount/unmount it.

Maybe we can replace the rmdir with a rename("merged.tmp", "merged")? It is an atomic operation with empty directories

if you just perform a "pull" then the directory exists, at least until you try to mount/unmount it.

OK I’ll need to look into the situation in more detail, then.

Maybe we can replace the rmdir with a rename

Does that retain the originally-wanted benefit of handling leaked mounts?

``

Does that retain the originally-wanted benefit of handling leaked mounts?

I think it does, although after started looking into it, I am not sure we need to overcomplicate the code for such a corner case where an image is mounted/unmounted on an additional image store

*shrug* the original thing smells like a workaround, and it seems no-one cared enough to document in detail what it is working around (or maybe it is not known at all).

The trouble is that when creating the image, we don’t know that the store is going to later be used as an additional one.

One entirely different possibility might be to create the mounts somewhere else, not in the read-only additional store. That’s rather more disruptive.

But really I need to understand what happens on pull, first. Hopefully tomorrow. For now, I’m just confused.

[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

I've replaced the Rmdir with a Rename so we can safely assume the merged directory is never removed.

Differently the diffDir errors are ignored, it might have the wrong ownership but should not really matter with additional image stores since we are already expecting the files in the additional image store owned by a different user to have different ownership.

comments addressed ⬆️

LGTM
@mtrmac @nalind @saschagrunert @vrothberg PTAL

LGTM
but definitely want a @mtrmac or @nalind head nod

there are two reviews, can we get this merged?

/lgtm

/cherry-pick release-1.42

@nalind: #1828 failed to apply on top of branch "release-1.42":

Applying: overlay: replace rmdir with rename
Using index info to reconstruct a base tree...
M	drivers/overlay/overlay.go
Falling back to patching base and 3-way merge...
Auto-merging drivers/overlay/overlay.go
CONFLICT (content): Merge conflict in drivers/overlay/overlay.go
error: Failed to merge in the changes.
hint: Use 'git am --show-current-patch=diff' to see the failed patch
Patch failed at 0001 overlay: replace rmdir with rename
When you have resolved this problem, run "git am --continue".
If you prefer to skip this patch, run "git am --skip" instead.
To restore the original branch and stop patching, run "git am --abort".

/cherry-pick release-1.45

@nalind: #1828 failed to apply on top of branch "release-1.45":

Applying: overlay: replace rmdir with rename
Using index info to reconstruct a base tree...
M	drivers/overlay/overlay.go
Falling back to patching base and 3-way merge...
Auto-merging drivers/overlay/overlay.go
CONFLICT (content): Merge conflict in drivers/overlay/overlay.go
error: Failed to merge in the changes.
hint: Use 'git am --show-current-patch=diff' to see the failed patch
Patch failed at 0001 overlay: replace rmdir with rename
When you have resolved this problem, run "git am --continue".
If you prefer to skip this patch, run "git am --skip" instead.
To restore the original branch and stop patching, run "git am --abort".

Addresses: https://issues.redhat.com/browse/RHEL-32589