options: keep using prior drivers if found

[flouthoc]

There is no need for vfs to be the default storage driver since kernel
>= 5.13 supports overlay natively however there is use-case for users
who don't had any configs and they started using vfs in a default manner following check is a hack to keep buildah and podman working for such users.

See: #1571 for prior discussions.

[flouthoc]

@nalind @giuseppe PTAL , this is a hack to prevent breaking old users, was not able to think of any other cheap way to prevent this break.

[nalind]

There's a whole scanPriorDrivers() function in drivers/drivers.go. Can we find a way to refrain from bypassing it?

[flouthoc]

There's a whole scanPriorDrivers() function in drivers/drivers.go. Can we find a way to refrain from bypassing it?

Ah it does a similar check, I think its a good idea if we can find a way so c/storage does not bypass it while checking config. Let me take a look.

[giuseppe]

we may need to tweak scanPriorDrivers() though as it has an explicit check to skip vfs, looking at the only caller of the function it should be fine to just drop the driver != "vfs" check

[flouthoc]

When I was looking at this is seems this part was always ignored by buildah or podman https://github.com/containers/storage/blob/main/drivers/driver.go#L345, since config itself defaults to a DriverName so New( will never receive empty driver string.

[flouthoc]

@giuseppe @nalind PTAL, it seems modifying behavior of New( could again break API users, so instead I think adding a similar check in types is safer way. Which will be only triggered if no selection of driver was set by user and c/storage is trying to find a default driver, in such case it will first check if any prior driver was set.

[rhatdan]

@giuseppe @nalind PTAL

[flouthoc]

cc @edsantiago (for info) I think this will unblock buildah vendor.

[giuseppe]

LGTM

[nalind]

types/options.go will need to add _ "github.com/containers/storage/drivers/register" to its list of imports, otherwise its first call to ScanPriorDrivers() appears to be iterating through the map of registered drivers before that map is populated. LGTM once that's changed.

[rhatdan]

LGTM

[Luap99]

This change is causing a huge bloat for rootlessport (size increase is 3 MB) in podman. Making storage/types depend on the drivers causes a new gigantic transitive dependency chain.

But more importantly this now causes build issues because it tries to build the devmapper driver even on systems without the device-mapper-devel installed, of course the correct thing is to use exclude_graphdriver_devicemapper but that wasn't necessary before which means likely other users who only depend on github.com/containers/storage/types will run into the same problem.

And to clarify rootlessport does not depend on c/storage/types directly. It only needs c/common/pkg/config but this package depends on c/storage/types which means everyone depending only on c/common/pkg/config and not the storage drivers will not suffer a almost 3 MB bloat in binary size.

Is there any way we can move this check into another package? Shouldn't this be decided in driver.New() and not in types?

[rhatdan]

@flouthoc PTAL

We should have never allowed the change in types/options.go

Lets revert this change and rework it.

@@ -10,6 +10,8 @@ import (
	"time"

	"github.com/BurntSushi/toml"
+	drivers "github.com/containers/storage/drivers"
+	_ "github.com/containers/storage/drivers/register"
	cfg "github.com/containers/storage/pkg/config"
	"github.com/containers/storage/pkg/idtools"
	"github.com/sirupsen/logrus"

[flouthoc]

@rhatdan We can move ScanPriorDrivers to a different pkg , that should resolve this.

[flouthoc]

I think moving entire check to Drivers.New( would break the API which will return empty driver when no driver is selected in the config and if we default to only vfs then we will have to hack Drivers.New to support native overlay again.

A simpler solution could be to create a function similar to ScanPriorDrivers for types/options which will only work on text filtering from a list of drivers ( e.g []string{"overlay", "vfs" ... } ) instead of going through a map of registered drivers.

This will remove types/options.go dependency on drivers and drivers can still keep using older code.

@containers/storage-maintainers @Luap99 WDYT ?

[Luap99]

I think moving entire check to Drivers.New( would break the API which will return empty driver when no driver is selected in the config and if we default to only vfs then we will have to hack Drivers.New to support native overlay again.

I cannot follow that statement? Looking at drivers.New() that function tries all drivers before it errors out when no driver works.

[flouthoc]

@Luap99 @containers/storage-maintainers This is patch which i want to propose 0c6fcf4 on my branch https://github.com/flouthoc/storage/tree/move_scan_prior

[flouthoc]

Nvm I think there is another simpler patch suggested by @giuseppe , d7a6662 on the branch https://github.com/flouthoc/storage/tree/use-readdir-prior, this should do the trick as well.

[flouthoc]

@Luap99 Following PR should address this #1637