projectatomic / container-storage-setup

Service to set up storage for Docker and other container systems
Apache License 2.0
153 stars 77 forks source link

Switch to overlay2 as default graph driver for docker #263

Closed rhvgoyal closed 6 years ago

rhvgoyal commented 6 years ago

Switch to overlay2 as default graph driver for docker. For rest, it remains unaffected.

This series adds extra checks to make sure system supports overlay2 otherwise container-storage-setup will exit with error code.

We will need to make sure that docker service has Requires= dependency on container-storage-setup instead of Wants= dependency. Otherwise docker will use loop devices which we probably don't want.

rhvgoyal commented 6 years ago

@cgwalters @rhatdan PTAL

Don't merge it yet. I need to do more testing in various configurations to make sure things are working. Right now I am just looking for comments and see if this is an acceptable solution or not.

rhvgoyal commented 6 years ago

@cgwalters took care of your comments. Also added a patch to bump up version to 0.9.0

cgwalters commented 6 years ago

Hm, so I was testing this with a dev CentOS AH vagrant box (ostree admin unlock, rsync the updated c-s-s from git in) then editing STORAGE_DRIVER=overlay2 in /etc/sysconfig/docker-storage-setup, but systemctl start docker-storage-setup fails with:

Nov 13 18:13:48 localhost.localdomain container-storage-setup[2441]: ERROR: Storage is already configured with devicemapper driver. Can't configure it with overlay2 driver...

So I think we'll need to make it not an error. One option perhaps is to change the default in /usr, and have the new config file be empty. Then we detect the already configured case, as distinct from the default in /usr?

cgwalters commented 6 years ago

To be fair/clear actually this was also a problem with my auto series I think.

cgwalters commented 6 years ago

We'll be getting the new config file because (at least for ostree-based systems, I didn't test a yum-managed host), we haven't modified the file:

# rpm-ostree status
State: idle
Deployments:
● centos-atomic-host:centos-atomic-host/7/x86_64/standard
                Version: 7.1708 (2017-09-15 15:32:30)
                 Commit: 33b4f0442242a06096ffeffadcd9655905a41fbd11f36cd6f33ee0d974fdb2a8
           GPGSignature: 1 signature
                         Signature made Fri 15 Sep 2017 05:17:39 PM UTC using RSA key ID F17E745691BA8335
                         Good signature from "CentOS Atomic SIG <security@centos.org>"
# ostree admin config-diff|grep -i docker
M    sysconfig/docker-storage
A    lvm/profile/atomicos--docker-pool-extend.profile
A    docker/key.json
# 
rhvgoyal commented 6 years ago

why are we getting new /etc/sysconfig/docker-storage-setup over upgrade?

cgwalters commented 6 years ago

Because it's not modified, so we get the new version.

rhvgoyal commented 6 years ago

Oh, you put it in manually. Then it explains it. Docker is already configured with devicemapper and one can't switch to overlay2. That's intended.

User will have to reset existing storage first and then retry.

cgwalters commented 6 years ago

I put it in manually, but that's simulating the same effect that a "clean upgrade" would. Basically if i've booted and am running RHELAH 7.4.2 with docker running OK on devicemapper, and I haven't touched the config file, then upgrade, I don't want docker-storage-setup.service to start failing trying to change me to overlay2 right?

Now this is mostly cosmetic but i'd like not to have systemd units fail on start.

cgwalters commented 6 years ago

What I'm arguing here is we create a distinction between "STORAGE_DRIVER set in /etc/sysconfig/" versus "STORAGE_DRIVER set in /usr/share/container-storage-setup/container-storage-setup". The default in /etc becomes empty (aside from comments). Then we don't error out if the system is already configured and no STORAGE_DRIVER is set in /etc.

(Actually we need to change the default from devicemapper in the default config anyways right?)

rhvgoyal commented 6 years ago

I was avoiding changing default in /usr/share/container-storage-setup/container-storage-setup because it will be a big surprise for existing users. They will upgrade package, reset storage and re-run docker and this time they will get overlay2 (instead of devicemapper). So if their systems are already setup and working with devicemapper, they certainly don't want to switch to overlay2 automatically.

And it will also have this problem which you are seeing right now.

cgwalters commented 6 years ago

I can't think of a way we can avoid "reset storage, rerun docker" not giving overlay2 without introducing some other state somewhere, like a /var/lib/container-storage-setup.done stamp file. And that'd be even more annoying for people who do want to switch the default. Under what scenarios are you thinking someone would want to completely blow away all of their containers but not switch to overlay?

rhvgoyal commented 6 years ago

Openshift people often blow up all their containers and restart node. They have already setup extra disks so that devicemapper volume is carved out properly. I think they will be pretty upset if after grade they suddenly start getting overlayfs on rootfs (which is smaller) and not big enough to store their containers. (Which was working previous to upgrade).

cgwalters commented 6 years ago

But do they already have STORAGE_DRIVER=devicemapper in /etc/sysconfig/docker-storage-setup?

rhvgoyal commented 6 years ago

I don't think so. They just work with container-storage-setup defaults and I don't think we shipped STORAGE_DRIVER=devicemapper. So may be some people have specified this explicitly, but anybody who is using defaults will have issues.

cgwalters commented 6 years ago

Basically I think people who want to retain devicemapper across storage resets are going to have to learn to change the config file. We're trying to change the default right? So anyone who wants not-default has to edit config files.

rhvgoyal commented 6 years ago

@cgwalters I have converted Fatal message into Info message and exited with exit code 0, if storage is already configured and STORAGE_DRIVER is a different value.

So only side affect now will be extra Info message

rhvgoyal commented 6 years ago

And of course, after reset overlay2 will be set (if user did not touch /etc/sysconfig/docker-storage-setup)

cgwalters commented 6 years ago

Yep, works for me in a quick test. 🚅

@rh-atomic-bot r+ d7c58ef

rh-atomic-bot commented 6 years ago

:zap: Test exempted: pull fully rebased and already tested.