[lxc-devel] [PATCH] c/r: rework external mountpoint handling v2

[Tycho Andersen]

On Wed, Apr 15, 2015 at 04:19:54PM +0000, Serge Hallyn wrote:
> Quoting Tycho Andersen (tycho.andersen at canonical.com):
> > On Wed, Apr 15, 2015 at 03:48:10PM +0000, Serge Hallyn wrote:
> > > Quoting Tycho Andersen (tycho.andersen at canonical.com):
> > > > CRIU now supports autodetection of external mounts via the --ext-mount-map auto
> > > > --enable-external-sharing --enable-external-masters options, so we don't need
> > > > to explicitly pass the cgmanager mount or any of the mounts from the config.
> > > > This also means that lxcfs mounts (since they are bind mounts from outside the
> > > > container) are autodetected, meaning that c/r of containers using lxcfs works.
> > > > 
> > > > A further advantage of this patch is that it addresses some of the ugliness
> > > > that was in the exec_criu() function. There are other criu options that will
> > > > allow us to trim this even further, though.
> > > > 
> > > > Finally, with --enable-external-masters, criu understands slave mounts in the
> > > > container with shared mounts in the peer group that are outside the namespace.
> > > > This allows containers on a systemd host to be dumped and restored correctly.
> > > > 
> > > > However, these options have just landed in criu trunk today, and the next
> > > > tagged release will be 1.6 on June 1, so we should avoid merging this into any
> > > 
> > > Ouch, June 1, that's a ways away.
> > > 
> > > > stable releases until then.
> > > 
> > > Should there be a lxc_check_criu_version() fn, updated in cases like these,
> > > which ensures that criu is new enough version for the lxc version?
> > 
> > Yes, I was thinking about this, the only question is how to detect it;
> > I can fork() and just pass --version, but then on every checkpoint or
> > restore we have an extra fork() in the critical path. Unfortunately, I
> 
> The critical path of checkpoint/restore?  Isn't that as speedy as mollasses
> now?

A fair point :)

> You could also cache the result in a global variable (-1 by default)
> so only the first lxcapi_checkpoint or lxcapi_restart will invoke the
> penalty.

Right, but in the case of e.g. checkpoint it will only ever be called
once, because as soon as you invoke checkpoint the monitor process
dies. Anyway, I think I'll just check it in the critical path. When
one extra fork is a performance bottleneck for migration I'll probably
be rotting away peacefully in a pine box somewhere :)

Tycho

> > think that may be our best option right now. If you're ok with the
> > forks (I am), I can send a patch for a version check.
> > 
> > > Given the heavy criu development I do NOT think we should fallback to
> > > different behavior on older versions - just require the minversion we
> > > need and fail with a nice informational message otherwise.
> > 
> > Yep, agreed.
> > 
> > > > v2: remount / as private before bind mounting the container's directory for
> > > 
> > > It's probably a whacky enough case that lxc+criu would fail anyway, but
> > > it *is* possible for / to not be a mountpoint.  Init could chroot you
> > > instead of pivot-rooting.  In that case you'd have to bind-mount / onto
> > > itself before doing the MS_PRIVATE remount.
> > > 
> > > I'm also kind of surprised it worked without a MS_REMOUNT flag, but <shrug>
> > 
> > Yes, in fact criu doesn't pass MS_REMOUNT most of the time, nor does
> > LXC when re-mounting / for startup.
> > 
> > > >     criu. The problem here is that if / is mounted as shared, even if we
> > > >     unshare() the /var/lib/lxc/rootfs mountpoint propagates outside of our
> > > >     mount namespace, which is bad, since we don't want to leak mounts. In
> > > >     particular, this leak confuses criu the second time it goes to checkpoint
> > > >     the container.
> > > > 
> > > > Signed-off-by: Tycho Andersen <tycho.andersen at canonical.com>
> > > 
> > > Acked-by: Serge E. Hallyn <serge.hallyn at ubuntu.com>
> > 
> > Thanks, although there is a v3 of this patch that I see you replied
> > to, I'll try and address those concerns there.
> > 
> > > But where do we keep this?  We need a github cronjob to post merge request
> > > on a given date.
> > 
> > I was thinking we could just merge it into master since we have the
> > stable-1.1 branch that we're tagging 1.1 stable release from.
> > 
> > Tycho
> > _______________________________________________
> > lxc-devel mailing list
> > lxc-devel at lists.linuxcontainers.org
> > http://lists.linuxcontainers.org/listinfo/lxc-devel
> _______________________________________________
> lxc-devel mailing list
> lxc-devel at lists.linuxcontainers.org
> http://lists.linuxcontainers.org/listinfo/lxc-devel