A try at try-overlay

[cgwalters]

/cc @lsm5 too

[rhatdan]

@cgwalters We are not planning on using this other then for Docker. Not planning on using it for CRI-O.

[cgwalters]

What is the plan for CRI-O? Just overlay by default?

[rhatdan]

Overlay by default and have admin configure the setup if he wants to modify it. The main reason for docker-storage-setup originally was to setup docker to use devicemapper on loopback devices so it would work out of the box. We did not have a nice default at that time like Overlay. We added some additional features, but when we looked at making this work for other tools like CRI-O, Buildah, kpod, skopeo things started to fall apart. So @rhvgoyal and I decided in the end it was better to document how to setup alternative storage.

[cgwalters]

One deep issue of course is that AH starts docker.service by default. I regret that now...but we're somewhat locked into it.

[rhatdan]

Yes I would love to remove all container runtimes from AH, and allow users to pick and choose which CR they want to use. Customers I talked to, really want AH to be shrunk, where they are talking about the number of packages in the default install.

[rhvgoyal]

So if STORAGE_DRIVER=auto, we can't allow users to specify any driver specific options. If user does not know what driver will be selected, how can they specific driver specific options. So we will have to put logic which checks if there are any driver specific options in /etc/sysconfig/docker-storage-setup and error out if that's the case.

[cgwalters]

So if STORAGE_DRIVER=auto, we can't allow users to specify any driver specific options.

Or perhaps instead assume if the admin specifies devmapper-specific options, use that by default?

[rhvgoyal]

Our current default is devicemapper as specificed in /usr/share/container-storage-setup/container-storage-setup. I am assuming that now you are going to change this to STORAGE_DRIVER=auto instead. That is a user by default will get STORAGE_DRIVER=auto, until and unless user has overridden it.

I am not a big fan of assuming user wants devicemapper. What will we do if user has specified conflicting options. Say some for devicemapper and some for overlay.

[cgwalters]

Yeah, though auto will (should) only take effect for new installs. But what we do need to think about is the case where an admin is writing to the config via e.g. cloud-init/ansible/etc., and hasn't specified STORAGE_DRIVER.

I'd say we have to handle this case correctly; right now the default on CentOS AH for example is:

# Edit this file to override any configuration options specified in
# /usr/share/container-storage-setup/container-storage-setup.
#
# For more details refer to "man container-storage-setup"
CONTAINER_THINPOOL=docker-pool

If the user changes that to e.g. CONTAINER_THINPOOL=my-awesome-thinpool and that's it, we need to auto-select devicemapper right?

As far as specifying options for both devmapper and overlay, I'd personally be fine if we just picked arbitrarily...

[cgwalters]

OK, pushed a total rework of this ⬆️. I haven't tested it, but see whether the high level looks better to you?

[cgwalters]

One final step I didn't take in this PR is to change the default to auto. That's obviously a small additional patch; I just wanted to separate the infrastructure from the default change. But the goal is to make that switch, so review with that in mind.

[cgwalters]

OK, pushed fixups ⬆️, and tested - things seem to work well for me, now we just need to change STORAGE_DRIVER=auto in our downstreams.

[cgwalters]

https://src.fedoraproject.org/rpms/docker/pull-request/3

[rhvgoyal]

Right now I see 3 patches. With two patches with same subject. And there are no changelogs in top two patches. Can we get patches broken down properly and with proper changelogs. We have discussed a lot about our rationale of why we are doing certain thing. It will be good of changelogs are verbose.

[cgwalters]

The latter two are fixup! - they get auto-squashed by homu on merge.

[cgwalters]

The idea behind using fixup! is you can easily see what changed between last review, without littering the final merge with "fix comments" type patches.

[cgwalters]

📞 ?

[rhvgoyal]

I was waiting for you to update patch. Did not realize you already updated it. I will test it tomorrow and look little more closely. In the mean time please add another test case for non-compatibility mode.

[rhvgoyal]

And last, I am still concerned that we have not defined the semantics of what happens if user specifies STORAGE_DRIVER=auto, and there are other config options present in the file.

Current semantics seem to be that existing options will be used if option belongs to selected driver. That means a user will be to specify options belonging to all drivers because user does not know which driver will be selected.

For example, Specifying CONTAINER_THINPOOL is mandatory now. So a config file will look.

STORAGE_DRIVER=auto CONTAINER_THINPOOL=foo

And we might end up using "overlay2".

This looks pretty odd and hacky to me.

[rhvgoyal]

We probably are making this thing more complicated, then it really has to be. Thinking purely from upstream point of view, how about if we switch to overlay2 as default graph driver. At run time we do bunch of checks to make sure overlay is supported.

• /var/ supports ftype=1 (for xfs)
• Create a temporary overlay2 mount point with multiple lower directories to make sure kernel supports multiple lower dirs.

And if that works, overlayfs will most likely work. And we don't have to introduce STORAGE_DRIVER=auto.

Now possible issues with this are.

• After an upgrade, default storage driver will change to overlay2 (from devicemapper) and that can be a surprise to people. This will not break existing storage configurations but if people reset storage and restart docker, then they will be using overlay2 instead. Is that a problem?

We could put some logic to detect if devicemapper specific options are present in /etc/sysconfig/docker-storage-setup, and error out saying default storage driver is overlay2 and CONTAINER_THINPOOL does not make sense and force user to make some changes.

• It will take affect on older kernels too. (Don't know what can we do about it).

But overall, this seems much simpler than going into the mechanics of supporting STORAGE_DRIVER=auto.

[rhvgoyal]

@rhatdan WDYT?

[rhvgoyal]

One problem with dynamic checks is it will require some temp directories to be created. May be we can write something in /tmp/ or /var/tmp/ and test if overlay mount is supported or not.

[rhvgoyal]

That way upstream does not have to deal with kernel version checks. And if something does not work, then user will have to change storage driver. Well current default also does not guarantee that it works on all machines. It requires extra space and a volume group and many cloud images don't have that by default.

In fact overlay2 as default probably will work fine on many cloud images which don't have LVM and does not require extra disk/space out of box, which is kind of nice.

[rhatdan]

Makes sense to me. I would prefer that you do the checking for overlay on /run rather then /tmp, to prevent potential attacks from users.

[cgwalters]

Hmm, maybe it actually would just work to change the default. Existing installs will be fine. The only case where it'll break is using old install media (e.g. RHEL 7.1 ISO) to kickstart a 7.4 tree. But IMO those people will just have to deal with it.

So...OK, on one hand we need to get this right, on the other hand we don't have a ton of time to pick a choice and start testing/documenting it. I can take a shot at rewriting this patch again, but if you have a strong opinion on how it's done, do you want to take it and I'll review?

[rhvgoyal]

@rhatdan is /run writable for most the use cases? What about /run inside container?

[cgwalters]

Obsoleted by https://github.com/projectatomic/container-storage-setup/pull/263

[rhatdan]

/run would be writable when the process is run as root. /run inside of a container would have the same chance of being read/only as /tmp or /var/tmp.