[lxc-devel] [PATCH] Add support for checkpoint and restore via CRIU

[Tycho Andersen]

Hi Stéphane,

On Wed, Aug 20, 2014 at 05:19:14PM -0500, Stéphane Graber wrote:
> On Wed, Aug 20, 2014 at 02:31:05PM -0500, Tycho Andersen wrote:
> >  
> > +# criu
> > +AC_ARG_ENABLE([criu],
> > +	[AC_HELP_STRING([--enable-criu], [enable checkpoint/restore support [default=auto]])],
> > +	[], [enable_criu=auto])
> > +
> > +if test "x$enable_criu" = "xauto" ; then
> > +	AC_CHECK_PROG([CRIU_CHECK], [criu], [yes], [no], "$PATH:/sbin:/bin:/usr/sbin:/usr/bin:/usr/local/sbin")
> > +	if test "x$CRIU_CHECK" = "xyes" ; then
> > +		enable_criu=yes
> > +	fi
> > +fi
> > +
> > +AM_CONDITIONAL([ENABLE_CRIU], [test "x$enable_criu" = "xyes"])
> > +AC_PATH_TOOL([CRIU_PATH], [criu], [no])
> > +if test "x$enable_criu" = "xyes" ; then
> > +	if test "x$CRIU_PATH" = "xno" ; then
> > +		AC_MSG_ERROR([Could not find criu])
> > +	fi
> > +	AC_DEFINE_UNQUOTED([CRIU_PATH], "$CRIU_PATH", [Criu path])
> > +fi
> > +
> 
> So why are you doing this?
> 
> To have this check pass, a packager would have to build-depend on criu
> even though the build process doesn't use criu at any point.
> 
> 
> Also, I know we may have been setting a bad example in the past and I'd
> love to see this corrected in the future, but in general, it's a really
> bad practice to hardcode path to binaries at build time.
> 
> One simple example of why that's bad is that distros which still do care
> about /usr separation, like Ubuntu, regularly move binaries between
> /usr/bin and /bin depending on what needs to be available for early
> boot.
> 
> Obviously, this is unlikely to affect criu, but say that for whatever
> reaosn, criu was considered critical for early boot, we'd then move it
> from /usr/sbin to /sbin which would break LXC until someone uploads an
> (otherwise pointless) no change rebuild to have the hardcode path
> updated.

Mostly because hallyn asked me to :). I guess it is to prevent path
attacks, but I can remove it if you guys want.

> >  # cgmanager
> >  AC_ARG_ENABLE([cgmanager],
> >  	[AC_HELP_STRING([--enable-cgmanager], [enable cgmanager support [default=auto]])],
> > @@ -652,6 +673,7 @@ AC_CONFIG_FILES([
> >  	doc/lxc-autostart.sgml
> >  	doc/lxc-cgroup.sgml
> >  	doc/lxc-checkconfig.sgml
> > +	doc/lxc-checkpoint.sgml
> >  	doc/lxc-clone.sgml
> >  	doc/lxc-config.sgml
> >  	doc/lxc-console.sgml
> > @@ -780,6 +802,7 @@ Environment:
> >   - GnuTLS: $enable_gnutls
> >   - Bash integration: $enable_bash
> >   - Openvswitch: $enable_ovs
> > + - CRIU: $CRIU_PATH
> 
> I guess with the above comment, this one can go too?
> 
> Or if criu is just turned into an enable/disable kind of parameter
> (defaulting to disable as not all distros have it), then we probably
> would want to show a simple yes/no at this point.

Yep, agreed. Right now it is the path to criu or "no".

> > +static int my_checker(const struct lxc_arguments *args)
> > +{
> > +	if (do_restore && stop) {
> > +		lxc_error(args, "-s not compatible with -r.");
> > +		return -1;
> > +
> > +	} else if (daemonize_set) {
> > +		lxc_error(args, "-d/-F not compatible with -r.");
> 
> Don't you mean do_restore && daemonize_set here?

Ah, actually I mean !do_restore && daemonize_set, but yes, that logic
is wrong.

I will make the rest of the changes and re-post once we decide what to
do about the autoconf bit.

Tycho