Move storage options to driver specific entries #432

rhatdan · 2019-09-19T00:21:12Z

Storage options are really driver specific and it is when distributions set defaults, they
should not effect the user if he changes the default driver. By moving the storage options
to be driver specific, we can make sure all drivers only document and support their options.
Signed-off-by: Daniel J Walsh [email protected]

rhatdan · 2019-09-19T00:22:27Z

Fixes: #423

TomSweeneyRedHat · 2019-09-19T11:14:24Z

pkg/config/config.go

+	// MountOpt specifies extra mount options used when mounting
+	MountOpt string `toml:"mountopt"`
+	// Name of the ZFS File system
+	Name string `toml:"name"`


should this be fsname or is it wrong in the manpage?

TomSweeneyRedHat · 2019-09-19T11:15:58Z

storage.conf

+# ignore_chown_errors can be set to allow a non privileged user running with
+# a single UID within a user namespace to run containers. The user can pull
+# and use any image even those with multiple uids.  Note multiple UIDs will be
+# squasheddown to the default uid in the container.  These images will have no


s/squashed down/squasheddown/

rhatdan · 2019-09-21T18:47:56Z

Helps fix containers/buildah#1859
@cevich Ubuntu is being cranky again. I thought this was fixed.

cevich · 2019-09-23T19:20:24Z

Oh this again 😖 I also thought I fixed this...

cevich · 2019-09-23T19:42:45Z

...oh right yes, ubuntu-1904-disco-v20190514 is a base-image (essentially as-is straight from Canonical and google) and broken-by-default. So the only "fix" in play for this PR is the timeout/retry loop (which apparently isn't long enough). It's possible to increase this timeout...but that also means waiting longer (like 5-10 minutes for a full system-update)

There's another (more through) fix in #408 which uses the libpod VM cache-images. Those have all these *&!J_$#ing background package-update services disabled. That PR is blocked on a testing-bug, though I'll try rebasing/re-testing again to try and shove it forward...

cevich · 2019-09-24T16:05:11Z

...nope still failing. The best short-term fix here is to bump up the "short" timeout value:

diff --git a/contrib/cirrus/lib.sh b/contrib/cirrus/lib.sh
index fbd3ba15c..a8a5b8138 100755
--- a/contrib/cirrus/lib.sh
+++ b/contrib/cirrus/lib.sh
@@ -62,7 +62,7 @@ export DEBIAN_FRONTEND=noninteractive
 # Short-cut for frequently used base command
 export SUDOAPTGET='sudo -E apt-get -q --yes'
 # Short list of packages or quick-running command
-SHORT_APTGET="timeout_attempt_delay_command 24s 5 30s $SUDOAPTGET"
+SHORT_APTGET="timeout_attempt_delay_command 120s 5 60s $SUDOAPTGET"
 # Long list / long-running command
 LONG_APTGET="timeout_attempt_delay_command 300s 5 60s $SUDOAPTGET"

Though TBH it may need to be even longer. What's likely happening, is this (stooooopid) systemd service is running at boot, and trying to upgrade all the packages on the system. Then, if there's any kind of networking slowdown or the repo. is busy, it can hold that lock for quite a while.

Before you ask, I already researched the safety of killing the damn thing...general sentiment was it's not, and could end up corrupting the database 😢 Outright disabling the dagumnit services in the image is the ideal fix (PR #408) but is blocked by unit-test breakage I have not been able to figure out (issue #419).

cevich · 2019-09-24T17:27:49Z

OMG it's worse 😭 after poking the hornet's nest, there's also packaging crap Ubuntu does ON BOOT which has to be turned off separately. Will the madness never end! (base-image fix for this is tricky, since that requires booting the image - not done by design for base images). I'm working on a fix for this that will hit the libpod cache images, and come here via #408 (eventually).

cevich · 2019-09-24T17:28:56Z

Meaning: For this PR, the best we can do in short-order is the timeout suggestion above.

rhatdan · 2019-09-27T05:48:18Z

@nalind @giuseppe @vrothberg @TomSweeneyRedHat PTAL

vrothberg

We really need tests for this new behavior and the commit/PR should document in greater detail what changes, how users can migrate to /use the new behavior and if we remain backward compatible.

Do we remain backwards compatible? Some users may depend on the previous behavior.

vrothberg · 2019-09-30T11:59:37Z

store.go

@@ -3394,6 +3350,83 @@ func ReloadConfigurationFile(configFile string, storeOptions *StoreOptions) {
 	if os.Getenv("STORAGE_DRIVER") != "" {
 		storeOptions.GraphDriverName = os.Getenv("STORAGE_DRIVER")
 	}
+	switch storeOptions.GraphDriverName {


Please create driver-specific functions for extracting the storage options. This will hopefully make the code easier to read, document and maintain. Currently, the switch table is too big for my eyes to easily follow what's going on.

This is tough to do, since the driver has not been created yet. It would be nice if the known options were stored in the drivers themselves though.

vrothberg · 2019-09-30T12:00:36Z

store.go

+		if config.Storage.Options.Thinpool.XfsNoSpaceMaxRetries != "" {
+			storeOptions.GraphDriverOptions = append(storeOptions.GraphDriverOptions, fmt.Sprintf("dm.xfs_nospace_max_retries=%s", config.Storage.Options.Thinpool.XfsNoSpaceMaxRetries))
+		}
+	case "overlay":


Empty new line missing before the new case.

vrothberg · 2019-09-30T12:03:09Z

docs/containers-storage.conf.5.md

+
+  Comma separated list of default options to be used to mount container images.  Suggested value "nodev".
+
+### STORAGE OPTIONS FOR BTRFS TABLE


BTRFS is already mentioned above. Let's remove this entry here.

vrothberg · 2019-09-30T12:04:48Z

docs/containers-storage.conf.5.md

+The `storage.options.aufs` table supports the following options:
+
+**mountopt**=""
+


Drop empty line.

vrothberg · 2019-09-30T12:05:06Z

docs/containers-storage.conf.5.md

@@ -155,6 +154,10 @@ Specifies the min free space percent in a thin pool required for new device crea

 Specifies extra mkfs arguments to be used when creating the base device.

+**mountopt**=""
+


Drop empty line.

vrothberg · 2019-09-30T12:05:11Z

docs/containers-storage.conf.5.md

+**mountopt**=""
+
+  Comma separated list of default options to be used to mount container images.  Suggested value "nodev".
+
 **use_deferred_deletion**=""



Drop empty line.

vrothberg · 2019-09-30T12:05:17Z

docs/containers-storage.conf.5.md

+      mount_program = "/usr/bin/fuse-overlayfs"
+
+**mountopt**=""
+


Drop empty line.

vrothberg · 2019-09-30T12:05:23Z

docs/containers-storage.conf.5.md

+  File System name for the zfs driver
+
+**mountopt**=""
+


Drop empty line.

vrothberg · 2019-09-30T12:06:15Z

docs/containers-storage.conf.5.md

+The `storage.options.overlay` table supports the following options:
+
+**ignore_chown_errors** = "true|False"
+  ignore_chown_errors can be set to allow a non privileged user running with a  single UID within a user namespace to run containers. The user can pull and use any image even those with multiple uids.  Note multiple UIDs will be squashed down to the default uid in the container.  These images will have no separation between the users in the container. Only supported for the overlay and vfs drivers.


The last sentence suggests that we also need a doc entry and checks for vfs.

pkg/config/config.go

vrothberg · 2019-10-04T09:42:19Z

@rhatdan, do we remain backwards compatible with this PR? I didn't test it yet.

rhatdan · 2019-10-04T10:39:28Z

Yes, and I added tests to make sure.

rhatdan · 2019-10-04T11:20:03Z

Although I did find a couple of more backwards compatibility issues. Should be fixed now.

TomSweeneyRedHat · 2019-10-04T12:18:11Z

docs/containers-storage.conf.5.md

-  Default Copy On Write (COW) container storage driver
-  Valid drivers are "overlay", "vfs", "devmapper", "aufs", "btrfs", and "zfs"
-  Some drivers (for example, "zfs", "btrfs", and "aufs") may not work if your kernel lacks support for the filesystem
+  Default Copy On Write (COW) container storage driver. Valid drivers are "overlay", "vfs", "devmapper", "aufs", "btrfs", and "zfs". Some drivers (for example, "zfs", "btrfs", and "aufs") may not work if your kernel lacks support for the filesystem


missing end period

There are a lot of missed periods. Fixed.

TomSweeneyRedHat · 2019-10-04T12:25:09Z

docs/containers-storage.conf.5.md

-     container to UID 1668442479 outside.  UID 1 will be mapped to 1668442480.
-     UID 2 will be mapped to 1668442481, etc, for the next 65533 UIDs in
-     Succession.
+  These mappings tell the container engines to map UID 0 inside of the container to UID 1668442479 outside.  UID 1 will be mapped to 1668442480. UID 2 will be mapped to 1668442481, etc, for the next 65533 UIDs in Succession.


s/succession/Succession/

TomSweeneyRedHat · 2019-10-04T12:31:24Z

docs/containers-storage.conf.5.md


-**use_deferred_deletion**=""
+**size**=""
+  Maximum size of a container image.   This flag can be used to set quota on the size of container images.


What size? Gb, Mb, Kb, Bytes? Can you specify 120M for 120MB say?

TomSweeneyRedHat · 2019-10-04T12:31:47Z

docs/containers-storage.conf.5.md


 **mkfsarg**=""
+  Specifies extra mkfs arguments to be used when creating the base device.


list of accepted values or pointer to a man page with them?

I will pass on this one, This is an old section, and I think this differs depending on the underlying file system.

TomSweeneyRedHat · 2019-10-04T12:32:02Z

docs/containers-storage.conf.5.md


-Specifies extra mkfs arguments to be used when creating the base device.
+**mountopt**=""
+  Comma separated list of default options to be used to mount container images.  Suggested value "nodev".


ditto list of accepted values.

Pointed them off to mount man page.

TomSweeneyRedHat · 2019-10-04T12:32:57Z

docs/containers-storage.conf.5.md


-### STORAGE OPTIONS FOR THINPOOL TABLE
+**mountopt**=""
+  Comma separated list of default options to be used to mount container images.  Suggested value "nodev".


accepted values?

TomSweeneyRedHat · 2019-10-04T12:33:08Z

docs/containers-storage.conf.5.md


-**autoextend_threshold**=""
+**size**=""
+  Maximum size of a container image.   This flag can be used to set quota on the size of container images.


accepted values?

TomSweeneyRedHat · 2019-10-04T12:33:34Z

docs/containers-storage.conf.5.md


 **fs**="xfs"
-
-Specifies the filesystem type to use for the base device. (default: xfs)
+  Specifies the filesystem type to use for the base device. (default: xfs)


accepted values

TomSweeneyRedHat · 2019-10-04T12:35:22Z

docs/containers-storage.conf.5.md

+
+**size**=""
+  Maximum size of a container image.   This flag can be used to set quota on the size of container images.
+


accepted values

TomSweeneyRedHat · 2019-10-04T12:35:30Z

docs/containers-storage.conf.5.md

+  mount_program = "/usr/bin/fuse-overlayfs"
+
+**mountopt**=""
+  Comma separated list of default options to be used to mount container images.  Suggested value "nodev".


accepted values

TomSweeneyRedHat · 2019-10-04T12:36:27Z

docs/containers-storage.conf.5.md

-Specifies the maximum number of retries XFS should attempt to complete IO when ENOSPC (no space) error is returned by underlying storage device. (default: 0, which means to try continuously.)
+The `storage.options.overlay` table supports the following options:
+
+**ignore_chown_errors** = "true|False"


What's the default? Did you mean to upper case "False" here?

TomSweeneyRedHat · 2019-10-04T12:36:50Z

docs/containers-storage.conf.5.md

+  File System name for the zfs driver
+
+**mountopt**=""
+  Comma separated list of default options to be used to mount container images.  Suggested value "nodev".


accepted values

TomSweeneyRedHat · 2019-10-04T12:36:59Z

docs/containers-storage.conf.5.md

+  Comma separated list of default options to be used to mount container images.  Suggested value "nodev".
+
+**size**=""
+  Maximum size of a container image.   This flag can be used to set quota on the size of container images.


accepted values

TomSweeneyRedHat · 2019-10-04T12:37:27Z

docs/containers-storage.conf.5.md


 ## SELINUX LABELING

 When running on an SELinux system, if you move the containers storage graphroot directory, you must make sure the labeling is correct.

-Tell SELinux about the new containers storage by setting up an equivalence record.
-This tells SELinux to label content under the new path, as if it was stored
+Tell SELinux about the new containers storage by setting up an equivalence record. This tells SELinux to label content under the new path, as if it was stored


need ending period

There already is on the next line.

TomSweeneyRedHat

I may have missed some, but please add verbiage with either examples or talk about the accepted values for the options in the man pages.

rhatdan · 2019-10-04T20:13:41Z

Ok I have updated and fixed all found issues. Added more info in man pages. I think this is ready to merge.

rhatdan · 2019-10-04T20:14:09Z

@nalind @TomSweeneyRedHat @giuseppe @vrothberg PTAL

TomSweeneyRedHat · 2019-10-04T20:31:44Z

LGTM, would like a head nod from others.

Storage options are really driver specific and it is when distributions set defaults, they should not effect the user if he changes the default driver. By moving the storage options to be driver specific, we can make sure all drivers only document and support their options. With this patch we will continue to support the global mountopt but the driver specific version will override the global mountopt. Signed-off-by: Daniel J Walsh <[email protected]>

rhatdan · 2019-10-11T20:31:57Z

@nalind @giuseppe @vrothberg PTAL

vrothberg · 2019-10-14T13:35:18Z

Testing it now locally

vrothberg

LGTM

Note that this justifies a minor version bump for the next release (i.e., v1.14.0).

rhatdan changed the title ~~Storageopts~~ Move storage options to driver specific entries Sep 19, 2019

rhatdan force-pushed the storageopts branch from 55a7a7a to a713ac4 Compare September 19, 2019 00:28

TomSweeneyRedHat reviewed Sep 19, 2019

View reviewed changes

rhatdan force-pushed the storageopts branch from a713ac4 to 66c60ae Compare September 20, 2019 04:57

cevich mentioned this pull request Sep 24, 2019

cirrus: Add bash-completion support containers/podman#4028

Merged

rhatdan force-pushed the storageopts branch from 66c60ae to 3a0b468 Compare September 25, 2019 07:00

vrothberg reviewed Sep 30, 2019

View reviewed changes

rhatdan force-pushed the storageopts branch 3 times, most recently from 66444c7 to 1833eef Compare October 4, 2019 09:03

rhatdan force-pushed the storageopts branch from 1833eef to c57e001 Compare October 4, 2019 11:19

TomSweeneyRedHat reviewed Oct 4, 2019

View reviewed changes

rhatdan force-pushed the storageopts branch 3 times, most recently from 907b0d4 to fc23a24 Compare October 4, 2019 14:23

rhatdan force-pushed the storageopts branch from fc23a24 to ff125a5 Compare October 11, 2019 20:27

vrothberg reviewed Oct 14, 2019

View reviewed changes

vrothberg merged commit 9efd327 into containers:master Oct 14, 2019

gartnera mentioned this pull request Mar 1, 2020

additionalimagestores config option ignored #544

Closed

kwilczynski mentioned this pull request Nov 5, 2024

OCPBUGS-44995: Update the storage.conf configuration file template openshift/machine-config-operator#4681

Merged


		Comma separated list of default options to be used to mount container images. Suggested value "nodev".

		### STORAGE OPTIONS FOR BTRFS TABLE

		The `storage.options.aufs` table supports the following options:

		mountopt=""

		@@ -155,6 +154,10 @@ Specifies the min free space percent in a thin pool required for new device crea

		Specifies extra mkfs arguments to be used when creating the base device.

		mountopt=""


		mkfsarg=""
		Specifies extra mkfs arguments to be used when creating the base device.


		size=""
		Maximum size of a container image. This flag can be used to set quota on the size of container images.

Move storage options to driver specific entries #432

Move storage options to driver specific entries #432

Conversation

rhatdan commented Sep 19, 2019 • edited Loading

rhatdan commented Sep 19, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rhatdan commented Sep 21, 2019

cevich commented Sep 23, 2019

cevich commented Sep 23, 2019

cevich commented Sep 24, 2019

cevich commented Sep 24, 2019

cevich commented Sep 24, 2019

rhatdan commented Sep 27, 2019

vrothberg left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vrothberg commented Oct 4, 2019

rhatdan commented Oct 4, 2019

rhatdan commented Oct 4, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

TomSweeneyRedHat left a comment

Choose a reason for hiding this comment

rhatdan commented Oct 4, 2019

rhatdan commented Oct 4, 2019

TomSweeneyRedHat commented Oct 4, 2019

rhatdan commented Oct 11, 2019

vrothberg commented Oct 14, 2019

vrothberg left a comment

Choose a reason for hiding this comment

rhatdan commented Sep 19, 2019 •

edited

Loading