HACK: storage: overlay: only unmount storage home if needed

[intelfx]

Do not attempt to unmount storage home directory if we were not supposed
to mount or remount it at all. This is never useful and may also break
certain (unsupported) setups.

Fixes: containers/podman#27012

[packit-as-a-service]

Packit jobs failed. @containers/packit-build please check.

[packit-as-a-service]

Packit jobs failed. @containers/packit-build please check.

[Luap99]

I don't think creating submounts within the storage driver directories is something we would ever support directly
cc @giuseppe @mtrmac @nalind

[giuseppe]

I don't think creating submounts within the storage driver directories is something we would ever support directly
cc @giuseppe @mtrmac @nalind

yeah that is not something we want to deal with, it is the same comment I've left on the linked issue

[mtrmac]

Thirded, the layout of containers storage is an implementation detail with no external promises, and external modifications to that directory structure are not supportable. (If nothing else, we have vague plans to move towards composefs-like layer backends.)

[intelfx]

@Luap99, @mtrmac et al.:

(Duplicating the comment I just left on the issue for visibility)

The reason to do this is to punt the container images (== conceptually discardable data) onto a storage medium with different characteristics than the rest of the graphroot.

(E.g.: compressed, deduplicated, less redundant, etc.)

Can you suggest any alternatives?

---

Thirded, the layout of containers storage is an implementation detail with no external promises, and external modifications to that directory structure are not supportable. (If nothing else, we have vague plans to move towards composefs-like layer backends.)

Absolutely, I'm not asking to commit to a specific layout of the graphroot or the storage home. But even if there is no commitment to support anything, not breaking a pre-existing system configuration would have been nice.

[giuseppe]

For that use case, you could use an additional image store so that the images are stored on a separate store/path

[mtrmac]

The reason to do this is to punt the container images (== conceptually discardable data) onto a storage medium with different characteristics than the rest of the graphroot.

As opposed to what “rest” of the graph root? Layers don’t exist only in the graph root, so this seems, at least, imprecisely targeted.

Guessing, is the goal to preserve the RW container contents? The two might be possible to separate using the imagestore option (although that was originally intended to make container contents less durably stored than layers, IIRC, so that might need careful review).

(Well, and then there’s the elephant in the room, where layers and containers and … have dependencies, so if the precious container data is preserved, but a parent layer is blown away, I suppose the data is recoverable, but it requires completely manual recovery steps because no tools support such a situation.)

[intelfx]

As opposed to what “rest” of the graph root?

Volumes, primarily.

Guessing, is the goal to preserve the RW container contents?

Nope, I understand in the model of an OCI container these are also expendable; I'm not trying to say changing that is feasible or desirable (and is not my goal anyway).

[intelfx]

@giuseppe

For that use case, you could use an additional image store so that the images are stored on a separate store/path

Okay, yeah, that seems to work. As noted in passing by @mtrmac, this is only about the images (RO layers); the RW container contents stay in the graphroot, but I guess I can live with these semantics.

---

Still, would you consider accepting the updated patch with much clearer purpose? This seems like a reasonable thing to do regardless of the whatever unsupported configuration I was trying to conjure.

[mtrmac]

I do think there is some scope for improvement / proper cleanup here.

• This does not look exactly correct: In some situations the existing code does a remount; entirely unmounting the directory is not an opposite of that
• I’m generally worried about error reporting / handling here. AFAICS currently any unmounting errors are propagated to the Podman top level and reported there, so… either no-one ever uses a skipMountHome configuration and these are bugs, or they are an important error reporting mechanism.

I suppose this is where I remember my policy that “drivers/overlay is too complex and I won’t be reviewing any PRs that add new options or new code paths”, and defer to others that understand this.

[giuseppe]

so the expectation here, from the user perspective, is to manually set skip_mount_home but manually do the mount. That is not something we will support.

Without the mount home, we would have never ended up in this situation though.

So if someone is not using skip_mount_home, but then we change the setting, we will start leaking the mount. I don't think that is desiderable.

• either no-one ever uses a skipMountHome configuration and these are bugs, or they are an important error reporting mechanism.

the EINVAL error is ignored by unmount.

[intelfx]

so the expectation here, from the user perspective, is to manually set skip_mount_home but manually do the mount. That is not something we will support.

I'm not asking to support "manually doing the mount"; you made it clear that it is an unsupported configuration.

However, if skip_mount_home is set, then podman does not attempt to mount or remount the dir in question — from which it follows that it should not try to unmount it, either. Is this reasoning wrong?

So if someone is not using skip_mount_home, but then we change the setting, we will start leaking the mount. I don't think that is desiderable.

How so?

[mtrmac]

the EINVAL error is ignored by unmount.

Doh, thank you!

[giuseppe]

I'm not asking to support "manually doing the mount"; you made it clear that it is an unsupported configuration.

However, if skip_mount_home is set, then podman does not attempt to mount or remount the dir in question — from which it follows that it should not try to unmount it, either. Is this reasoning wrong?

but the setting could be changed while a Podman is running, and it will be efffective from the next time.

e.g. you've a container running, and there is the mount on the overlay directory. While that is running, you switch the flag to skip_mount_home=true, next time the cleanup up runs, we will leak the initial overlay directory, while now we don't