search

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

Use systemd mount unit file for extra volume mount in compat mode #245

Closed rhvgoyal closed 7 years ago

rhvgoyal commented 7 years ago

We were using systemd mount unit file for extra volume mount. Then we wanted to extend container-storage-setup for use by other run times and we started doing mounts inline and got rid of dependency on systemd mount unit.

But this does not work for compatibility mode. And reason being that now it is possible that volume device is not up by the time container-storage-setup runs and that means we will not mount it and either try to create new or return.

We can't even wait for device to come up as in comaptibility mode we don't save any metadata. So we don't know if we are running for first time and we should create volume or we are restarting and we should wait for volume.

So go back to creating a systemd mount unit file for mounting extra volumes. Only for compatibility mode. In non-compatibility mode, we need to explicitly activate configuration and that means by that time volume has already been created and that means we can wait for volume.

Signed-off-by: Vivek Goyal vgoyal@redhat.com

rhvgoyal commented 7 years ago

@rhatdan PTAL

rhvgoyal commented 7 years ago

@dustymabe I think this should fix the issue you are seeing

rhvgoyal commented 7 years ago

Fixed a bug and pushed new patch with "set -x". Trying to debug why /var/lib/containers is not mounted.

rhatdan commented 7 years ago

LGTM other then the test failure.

rhvgoyal commented 7 years ago

@cgwalters Made two changes. generating a temp unit file and renaming it. And using systemd daemon-reload.

I am not thinking of moving to using generators for this yet.

PTAL

rhvgoyal commented 7 years ago

Added WARNING to mount unit file.

cgwalters commented 7 years ago

OK, I'll be honest - I haven't yet given this code a full review; the c-s-s code has changed a lot. But it does seem likely to get us to a better place.

@rh-atomic-bot r+ 1b84349

rh-atomic-bot commented 7 years ago

:hourglass: Testing commit 1b84349 with merge 9b77bcb...

rh-atomic-bot commented 7 years ago

:sunny: Test successful - status-redhatci Approved by: cgwalters Pushing 9b77bcb2cba8e272799fa21e2d484e9f6e7c34d0 to master...