Make the priority for picking the storage driver configurable #1460

dcermak · 2023-01-03T16:16:31Z

This fixes #1457

I have tested this locally by vendoring the specific PR into podman and then setting the following ~/.config/containers/storage.conf:

[storage]

driver_priority = [
                "zfs",
                "btrfs",
                "overlay"
]

podman then tried zfs (but failed as I don't have any zfs partitions) and picked btrfs as the next viable candidate.

Removing the setting causes podman to correctly pick up overlay as the driver.

rhatdan · 2023-01-03T16:35:58Z

Please update the man page and make it clear the priority list is only used if graphdriver name is NOT specified.

dcermak · 2023-01-03T16:46:16Z

@rhatdan done

docs/containers-storage.conf.5.md

rhatdan · 2023-01-03T18:03:31Z

One issue in the man page, once this is fixed, LGTM
@vrothberg PTAL

dcermak · 2023-01-04T08:25:23Z

One question about this change though: when invoking podman now, I always get the following error:

ERRO[0000] The storage 'driver' option must be set in /home/dan/.config/containers/storage.conf to guarantee proper operation

which is also suboptimal from a UX perspective. Does it really need to be an error? The driver is after all set in the store itself.

vrothberg · 2023-01-04T08:47:15Z

One question about this change though: when invoking podman now, I always get the following error:
ERRO[0000] The storage 'driver' option must be set in /home/dan/.config/containers/storage.conf to guarantee proper operation
which is also suboptimal from a UX perspective. Does it really need to be an error? The driver is after all set in the store itself.

@giuseppe @rhatdan WDYT? I am surprised as well. Shouldn't Podman / storage be smart enough to pick a default driver if none is set?

drivers/driver.go

types/options.go

vrothberg · 2023-01-04T08:50:19Z

docs/containers-storage.conf.5.md

@@ -59,6 +59,11 @@ A common use case for this field is to provide a local storage directory when us
  container storage run dir (default: "/run/containers/storage")
 Default directory to store all temporary writable content created by container storage programs. The rootless runroot path supports environment variable substitutions (ie. `$HOME/containers/storage`)

+**driver_priority**=[]
+  Priority list for the storage drivers that will be tried in case no driver is set.


"will be tried" is a bit vague. Please be specific when a certain driver will be used and when not.

Does it make more sense now?

dcermak · 2023-01-04T08:54:46Z

One question about this change though: when invoking podman now, I always get the following error:
ERRO[0000] The storage 'driver' option must be set in /home/dan/.config/containers/storage.conf to guarantee proper operation
which is also suboptimal from a UX perspective. Does it really need to be an error? The driver is after all set in the store itself.
@giuseppe @rhatdan WDYT? I am surprised as well. Shouldn't Podman / storage be smart enough to pick a default driver if none is set?

The error comes from here fyi:

storage/types/options.go

Line 384 in 46e9455

    
           logrus.Errorf("The storage 'driver' option must be set in %s to guarantee proper operation", configFile)

and I suspect it has been there for a while (git blame unfortunately only goes back to a commit from 2021)

giuseppe

could we keep the error only when DriverPriority is empty (and maybe change it to a warning)?

drivers/driver.go

rhatdan · 2023-01-04T12:25:55Z

I believe at some point we forced users to specify their driver, because they kept on failing over to VFS in rootless mode, and then complained when they ran out of disk space in their homedir.

Forcing them to specify overlay and failing, caused us to know to have them install fuse-overlayfs before the kernel supported rootless overlay.

Now that rootless overlay will work for almost everyone the overlay default should work, and we might not need to require them to enter any longer.

rhatdan · 2023-01-05T10:56:39Z

@dcermak Would like to get this in as we are about to release a new version of containers/storage. Are you able to address the comments?

dcermak · 2023-01-05T11:38:17Z

could we keep the error only when DriverPriority is empty (and maybe change it to a warning)?

Given @rhatdan's comment for the historical context, I'd honestly suggest to remove this error completely. WDYT?

@dcermak Would like to get this in as we are about to release a new version of containers/storage. Are you able to address the comments?

Yes, I'd like to get this is in soon.

This fixes containers#1457 Co-authored-by: Valentin Rothberg <vrothberg@redhat.com> Signed-off-by: Dan Čermák <dcermak@suse.com>

rhatdan · 2023-01-05T11:52:18Z

Could we do the check iff driver is not specified and priority is not specified, then fail.

dcermak · 2023-01-05T13:53:07Z

Could we do the check iff driver is not specified and priority is not specified, then fail.

We do have an internal priority list as a fallback and also in New we try more or less every driver until one of them works:

storage/drivers/driver.go

Line 332 in fc91849

func New(name string, config Options) (Driver, error) {

Thus I don't consider that warning really valid or is there a case where the user really should define a driver?

rhatdan · 2023-01-05T15:51:34Z

I am not crazy about the internal list, since it is not easily understood by humans, so I would rather fail and tell them (or the distribution, specify one rather then having it randomly chosen potentially based on which software they have installe.

If the user or distribution specifies a search list of the driver then we should not warn.

Currently we would display an error when the user does not specify a `driver` in their config file. This has been present for historical reasons mostly to prevent users from accidentally getting the vfs driver (containers#1460 (comment)). Now that most systems support the overlay driver natively, we can reduce this to a warning and only warn about it if the driver_priority list is unset. If it is provided, then clearly the user or the distribution wanted for c/storage to pick a driver itself and the warning would be only confusing to users. Signed-off-by: Dan Čermák <dcermak@suse.com>

dcermak · 2023-01-06T07:27:45Z

@rhatdan Makes sense, I've adjusted the code accordingly, modified the error to be just a warning and tweaked the message as well to sound less scary.

Currently we would display an error when the user does not specify a `driver` in their config file. This has been present for historical reasons mostly to prevent users from accidentally getting the vfs driver (containers#1460 (comment)). Now that most systems support the overlay driver natively, we can reduce this to a warning and only warn about it if the driver_priority list is unset. If it is provided, then clearly the user or the distribution wanted for c/storage to pick a driver itself and the warning would be only confusing to users. Signed-off-by: Dan Čermák <dcermak@suse.com>

rhatdan · 2023-01-06T13:31:49Z

LGTM
@vrothberg @giuseppe PTAL

giuseppe

LGTM

This is an amend to containers#1460 That PR was not adressing the case when the system wide config had the driver_priority option configured and the user had no config file of their own. Then getRootlessStorageOpts would be called and it would override the graph driver to "vfs". With this commit we only override the graph driver if driver priority is empty. Otherwise we propagate the driver priority into the storage options, so that the driver autodetection works as expected. Signed-off-by: Dan Čermák <dcermak@suse.com>

This is an amend to containers#1460 That PR was not addressing the case when the system wide config had the driver_priority option configured and the user had no config file of their own. Then `getRootlessStorageOpts` would be called and it would override the graph driver to "vfs". With this commit we only override the graph driver if driver priority is empty. Otherwise we propagate the driver priority into the storage options, so that the driver autodetection works as expected. Signed-off-by: Dan Čermák <dcermak@suse.com>

dcermak force-pushed the configurable-driver-priority branch from b67adf5 to ba9c182 Compare January 3, 2023 16:21

dcermak force-pushed the configurable-driver-priority branch from ba9c182 to 3d35b47 Compare January 3, 2023 16:45

rhatdan reviewed Jan 3, 2023

View reviewed changes

docs/containers-storage.conf.5.md Outdated Show resolved Hide resolved

dcermak force-pushed the configurable-driver-priority branch from 3d35b47 to 92ddbc9 Compare January 3, 2023 23:54

vrothberg reviewed Jan 4, 2023

View reviewed changes

dcermak force-pushed the configurable-driver-priority branch 2 times, most recently from 6e1ef87 to bee865b Compare January 4, 2023 09:09

giuseppe reviewed Jan 4, 2023

View reviewed changes

drivers/driver.go Outdated Show resolved Hide resolved

Make the priority for picking the storage driver configurable

881ac48

This fixes containers#1457 Co-authored-by: Valentin Rothberg <vrothberg@redhat.com> Signed-off-by: Dan Čermák <dcermak@suse.com>

dcermak force-pushed the configurable-driver-priority branch from bee865b to 881ac48 Compare January 5, 2023 11:38

dcermak force-pushed the configurable-driver-priority branch from fbe44c0 to b2b2fef Compare January 6, 2023 07:43

giuseppe approved these changes Jan 6, 2023

View reviewed changes

rhatdan merged commit 90d0b54 into containers:main Jan 6, 2023

dcermak mentioned this pull request Jan 13, 2023

Only override the graphdriver to vfs if the priority is unset #1468

Merged

dcermak mentioned this pull request Jan 16, 2023

Refactor storage.conf loading #1469

Open

Luap99 mentioned this pull request Apr 14, 2023

podman build: rootless build takes up a lot of disk space #1570

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Make the priority for picking the storage driver configurable #1460

Make the priority for picking the storage driver configurable #1460

dcermak commented Jan 3, 2023

rhatdan commented Jan 3, 2023

dcermak commented Jan 3, 2023

rhatdan commented Jan 3, 2023

dcermak commented Jan 4, 2023

vrothberg commented Jan 4, 2023

vrothberg Jan 4, 2023

dcermak Jan 4, 2023

dcermak commented Jan 4, 2023

giuseppe left a comment

rhatdan commented Jan 4, 2023

rhatdan commented Jan 5, 2023

dcermak commented Jan 5, 2023 •

edited

rhatdan commented Jan 5, 2023

dcermak commented Jan 5, 2023

rhatdan commented Jan 5, 2023

dcermak commented Jan 6, 2023

rhatdan commented Jan 6, 2023

giuseppe left a comment

Make the priority for picking the storage driver configurable #1460

Make the priority for picking the storage driver configurable #1460

Conversation

dcermak commented Jan 3, 2023

rhatdan commented Jan 3, 2023

dcermak commented Jan 3, 2023

rhatdan commented Jan 3, 2023

dcermak commented Jan 4, 2023

vrothberg commented Jan 4, 2023

vrothberg Jan 4, 2023

Choose a reason for hiding this comment

dcermak Jan 4, 2023

Choose a reason for hiding this comment

dcermak commented Jan 4, 2023

giuseppe left a comment

Choose a reason for hiding this comment

rhatdan commented Jan 4, 2023

rhatdan commented Jan 5, 2023

dcermak commented Jan 5, 2023 • edited

rhatdan commented Jan 5, 2023

dcermak commented Jan 5, 2023

rhatdan commented Jan 5, 2023

dcermak commented Jan 6, 2023

rhatdan commented Jan 6, 2023

giuseppe left a comment

Choose a reason for hiding this comment

dcermak commented Jan 5, 2023 •

edited