[lxc-devel] [PATCH] [RFC] snapshots: move snapshot directory (v3)

[Serge Hallyn]

Quoting Stéphane Graber (stgraber at ubuntu.com):
> On Sun, May 25, 2014 at 12:26:12AM -0400, S.Çağlar Onur wrote:
> > On Sat, May 24, 2014 at 11:37 PM, Serge Hallyn <serge.hallyn at ubuntu.com> wrote:
> > > Quoting S.Çağlar Onur (caglar at 10ur.org):
> > >> Hey Serge,
> > >>
> > >> On Sat, May 24, 2014 at 8:15 PM, Serge Hallyn <serge.hallyn at ubuntu.com> wrote:
> > >> > Quoting Serge Hallyn (serge.hallyn at ubuntu.com):
> > >> >> Quoting Serge Hallyn (serge.hallyn at ubuntu.com):
> > >> >> > Originally we kept snapshots under /var/lib/lxcsnaps.  If a
> > >> >> > separate btrfs is mounted at /var/lib/lxc, then we can't
> > >> >> > make btrfs snapshots under /var/lib/lxcsnaps.
> > >> >> >
> > >> >> > This patch moves the default directory to /var/lib/lxc/c/snaps.
> > >> >> > If /var/lib/lxcsnaps already exists, then use that.
> > >> >> >
> > >> >> > If we are deleting a container which has snapshots, we currently
> > >> >> > will delete the container itself and its rootfs, but not its
> > >> >> > snapshots.  This could be confusing for the user, and there is
> > >> >> > no option to c->destroy() to ask for different behavior.  So
> > >> >> > currently a user would have to delete all snapshots first, then
> > >> >> > delete the container.  Ideas for better handling this would be
> > >> >> > welcome, but we don't want to change the current api, so while
> > >> >> > adding a new c->destroy_full() would be ok, adding a flags
> > >> >> > argument to c->destroy(c, flags) is not.
> > >> >>
> > >> >> I'm thinking that
> > >> >>
> > >> >>       c->snapshot_destroy(c, NULL);
> > >> >>
> > >> >> should tell lxc to remove all snapshots.  So then at least we can
> > >> >>
> > >> >>       c->snapshot_destroy(c, NULL);
> > >> >>       c->destroy(c);
> > >> >>       lxc_container_put(c);
> > >> >>
> > >> >> as a way of making sure we delete the whole thing.
> > >> >
> > >> > I'm working on a full patch to go about it this way.
> > >>
> > >> Sorry for being late in the game but I'm wondering would be make more
> > >> sense to introduce a new method called lxcapi_snapshot_destroy_all to
> > >> do lxcapi_snapshot_list followed by a loop to call
> > >> lxcapi_snapshot_destroy instead of overloading the snapname parameter?
> > >
> > > Hm, we could do that.  Note that since snapshots are always called
> > > snap%d, 'ALL' can't realistically be in use unless the user has
> > > manually created a container into the snapshot path.
> > >
> > > I don't have strong feelings either way.
> > 
> > I have some mixed feelings against the ALL method in v4 :) Personally,
> > if we have to pick either a magic parameter (referring passing the
> > NULL as snapname) or passing a magic string (referring passing the
> > "ALL" string as snapname) my preference would be the magic parameter.
> > Providing the intent via a pre-defined string sounds trouble some to
> > me.
> > 
> > I guess you chose the later so that one can pass ALL to lxc-snapshot
> > utility but this behavior could be implemented via new parameter
> > called --all/-a (eg; lxc-snapshot -name xyz -d --all)
> > 
> > What do you think?
> 
> 
> FWIW, I also tend to dislike magic strings, so NULL seems better to me
> than 'ALL'.

'NULL' as an alias for 'all' seems too magic.

So it sounds like a new c->destroy_full(c) or c->destroy_all_snapshots(c)
(or both, with the former deleting the container as well) is best.

If someone wants to implement that on top of my v4 I won't be offended :)
Otherwise I'll get to it sometime next week.

-serge