Revert commit that breaks podman backwards compatibility

[Luap99]

Revert "overlay: do not recreate "merged" dir"

This reverts commit https://github.com/containers/storage/commit/de38d80855503aac0c46de79b121588cb4d57966.

Podman upgrade tests are faiing because of this[1]. A start of container created with a previous version is now failing because of the missing merged dir.

A very simple reproducer is to run a container with a older versin then update to one with this commit and try to start the container again: podman run --name test quay.io/libpod/testimage:20240123 echo hi <-- update podman to a storage version with the reverted commit podman start -a test <-- now failing while trying to mount the container

[1] https://api.cirrus-ci.com/v1/artifact/task/5918496639418368/html/upgrade_test-podman-fedora-39-root-host.log.html

[Luap99]

@giuseppe @mtrmac @rhatdan PTAL

I haven't looked closely at #1827 but this likely means we need to reopen that one. Breaking backwards compatibility by no longer creating the merged dir seems to be not an option.

If you want to propose a better fix instead of reverting this is fine for me but I like to add that we a short one time for the vendor dance.

[giuseppe]

let me take a look

[giuseppe]

the failure I see is something broken in the Podman test:

    # umount, and make sure files are gone
    run_podman umount $external_cid
    if [ -d "$mount_path" ]; then
        # Under VFS, mountpoint always exists even despite umount
        if [[ "$(podman_storage_driver)" != "vfs" ]]; then
            die "'podman umount' did not umount $mount_path"
        fi
    fi

The directory can be present, and not be a mountpoint

[giuseppe]

a better test would be findmnt "$mount_path" >/dev/null; if it succeeds the path is a mountpoint

[giuseppe]

then we don't even need to worry about the vfs special case

diff --git a/test/system/060-mount.bats b/test/system/060-mount.bats
index fbc925a47..0a7123c40 100644
--- a/test/system/060-mount.bats
+++ b/test/system/060-mount.bats
@@ -251,11 +251,8 @@ EOF

     # umount, and make sure files are gone
     run_podman umount $external_cid
-    if [ -d "$mount_path" ]; then
-        # Under VFS, mountpoint always exists even despite umount
-        if [[ "$(podman_storage_driver)" != "vfs" ]]; then
-            die "'podman umount' did not umount $mount_path"
-        fi
+    if test \! findmnt "$mount_path" >/dev/null ; then
+        die "'podman umount' did not umount $mount_path"
     fi
     buildah rm $external_cid
 }

[Luap99]

Yeah the first failure did indeed seems a bit odd to me as well but I didn't took to closely at the test. We should certainly fix the test for this then, however the second one is a real problem as it breaks all old podman containers.

[edsantiago]

There is nothing special about that "Under VFS" code: it's only a few months old, from #20161, and it was my (ignorant) way of dealing with a difference between VFS and overlay. IOW I wrote that code as a way to deal with the situation as I saw it, not because I knew what I was doing.

[giuseppe]

we can probably revert 12dd0774a75a6dcb9b685fd5f697132e9ea04e06, it should be a no-op if the directory exists

[Luap99]

Ack, let me drop the fist revert then.

[giuseppe]

There is nothing special about that "Under VFS" code: it's only a few months old, from #20161, and it was my (ignorant) way of dealing with a difference between VFS and overlay. IOW I wrote that code as a way to deal with the situation as I saw it, not because I knew what I was doing.

to be fair, that was confusing in c/storage. We are still treating vfs differently, and the lack of a mountpoint for vfs is the root cause for most/(all?) the flakes we see in the Podman CI when vfs is used.

[openshift-ci[bot]]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: giuseppe, Luap99

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: - ~~[OWNERS](https://github.com/containers/storage/blob/main/OWNERS)~~ [Luap99,giuseppe] Approvers can indicate their approval by writing `/approve` in a comment Approvers can cancel approval by writing `/approve cancel` in a comment

[Luap99]

I created https://github.com/containers/podman/pull/21875 in podman to fix the test.

[rhatdan]

LGTM