Re: [lxc-devel] [PATCH] Add remove_snapshots_entry() (rebased - v2)

[Christian Brauner]

On Tue, Sep 15, 2015 at 12:57:26AM +, Serge Hallyn wrote:
> Quoting Christian Brauner (christianvanbrau...@gmail.com):
> > On Mon, Sep 14, 2015 at 08:51:08PM +, Serge Hallyn wrote:
> > > Quoting Christian Brauner (christianvanbrau...@gmail.com):
> > > > On Mon, Sep 14, 2015 at 08:31:59PM +, Serge Hallyn wrote:
> > > > > Quoting Christian Brauner (christianvanbrau...@gmail.com):
> > > > > > On Mon, Sep 14, 2015 at 07:27:06PM +, Serge Hallyn wrote:
> > > > > > > Quoting Christian Brauner (christianvanbrau...@gmail.com):
> > > > > > > > On Mon, Sep 14, 2015 at 04:33:05PM +, Serge Hallyn wrote:
> > > > > > > > > Quoting Serge Hallyn (serge.hal...@ubuntu.com):
> > > > > > > > > > Quoting Christian Brauner (christianvanbrau...@gmail.com):
> > > > > > > > > > > On Mon, Sep 14, 2015 at 02:50:39PM +, Serge Hallyn 
> > > > > > > > > > > wrote:
> > > > > > > > > > > > Quoting Christian Brauner 
> > > > > > > > > > > > (christianvanbrau...@gmail.com):
> > > > > > > > > > > > > When creating ephemeral containers that have the 
> > > > > > > > > > > > > option lxc.ephemeral = 1 set
> > > > > > > > > > > > > in their config, they will be destroyed on shutdown. 
> > > > > > > > > > > > > As they are simple overlay
> > > > > > > > > > > > > clones of an existing container they should be 
> > > > > > > > > > > > > registered in the lxc_snapshots
> > > > > > > > > > > > > file of the original container to stay consistent and 
> > > > > > > > > > > > > adhere to the
> > > > > > > > > > > > > expectancies of the users. Most of all, it ensure 
> > > > > > > > > > > > > that we cannot remove a
> > > > > > > > > > > > > container that has clones, even if they are just 
> > > > > > > > > > > > > ephemeral snapshot-clones. The
> > > > > > > > > > > > > function adds further consistency because 
> > > > > > > > > > > > > remove_snapshots_entry() ensures that
> > > > > > > > > > > > > ephemeral clone-snapshots deregister themselves from 
> > > > > > > > > > > > > the lxc_snapshots file
> > > > > > > > > > > > > when they are destroyed.
> > > > > > > > > > > > > 
> > > > > > > > > > > > > POSSIBLE GLITCH:
> > > > > > > > > > > > > I was thinking hard about racing conditions and 
> > > > > > > > > > > > > concurrent acces on the
> > > > > > > > > > > > > lxc_snapshots file when lxc-destroy is called on the 
> > > > > > > > > > > > > container while we
> > > > > > > > > > > > > shutdown then container from inside. Here is what my 
> > > > > > > > > > > > > thoughts are so far:
> > > > > > > > > > > > > 
> > > > > > > > > > > > > There should be no racing condition when lxc-destroy 
> > > > > > > > > > > > > including all snapshots is
> > > > > > > > > > > > 
> > > > > > > > > > > > Note that lxcapi_destroy_with_snapshots() deletes the 
> > > > > > > > > > > > *snapshots*, not the
> > > > > > > > > > > > snapshot clones.  This is an unfortunate naming clash 
> > > > > > > > > > > > (which we could try
> > > > > > > > > > > > to correct henceforth but we need good names :), but 
> > > > > > > > > > > > they are different.
> > > > > > > > > > > > So anything under /var/lib/lxc/$container/snaps will be 
> > > > > > > > > > > > deleted.  But if
> > > > > > > > > > > > you've created an overlayfs clone, then containers 
> > > > > > > > > > > > listed in
> > > > > > > > > > > > /var/lib/lxc/$container/lxc_snapshots will not be 
> > > > > > > > > > > > deleted. There is no
> > > > > > > > > > > > API call or program to automatically deleted those 
> > > > > > > > > > > > right now.
> > > > > > > > > > > 
> > > > > > > > > > > I think you are partially wrong here. I was not thinking 
> > > > > > > > > > > about problems created
> > > > > > > > > > > by an API-call but by the lxc-destroy exectuable. A quick 
> > > > > > > > > > > walkthrough: With the
> > > > > > > > > > 
> > > > > > > > > > D'oh.  Yeah, you'll need to mutex that somehow.
> > > > > > > > > 
> > > > > > > > > If you want help up-front with the design, please let me 
> > > > > > > > > know.  If you
> > > > > > > > > aren't sure what the current container_disk_lock() and 
> > > > > > > > > container_mem_lock()
> > > > > > > > > do, please shout.  (they are explained in a LOCKING comment 
> > > > > > > > > above
> > > > > > > > > lxc_container_free() in src/lxc/lxccontainer.c)
> > > > > > > > > 
> > > > > > > > > The easiest thing to do mght be to disk_lock the container in 
> > > > > > > > > lxc_destroy.c,
> > > > > > > > > then make the mod_rdep() helper which you use in 
> > > > > > > > > lxc_destroy.c be a _locked
> > > > > > > > > variant (to avoid deadlock).  So mod_rdep() would turn into 
> > > > > > > > > something like:
> > > > > > > > > 
> > > > > > > > > static bool mod_rdep(struct lxc_container *c0, struct 
> > > > > > > > > lxc_container *c, bool inc)
> > > > > > > > > {
> > > > > > > > >   bool ret;
> > > > > > > > >   if (container_disk_lock(c0))
> > > > > > > > >   return false;
> > > > > > > > >   ret = 

Re: [lxc-devel] [PATCH] Add remove_snapshots_entry() (rebased - v2)

[Serge Hallyn]

Quoting Christian Brauner (christianvanbrau...@gmail.com):
> On Tue, Sep 15, 2015 at 12:57:26AM +, Serge Hallyn wrote:
> > Quoting Christian Brauner (christianvanbrau...@gmail.com):
> > > The easiest way to achieve this would be to #include lxccontainer.h in 
> > > utils.h
> > > because we need mod_all_rdep() multiple times in lxccontainer.c and 
> > > start.c and
> > > mod_all_rdep() needs struct lxc_container declared. Unless I'm missing 
> > > something
> > > terribly obvious. Fine with that? Or rather some extern trickery? Or other
> > > ideas?
> > 
> > Does it work if you just define mod_all_rdeps as non-static in 
> > lxccontainer.c
> > and then, in start.c, put
> > 
> > struct lxc_container;
> > extern void mod_all_rdeps(struct lxc_container *c, bool inc);
> > 
> > then use that in your fn?
> 
> Yes, that was what I meant when I said "use some extern trickery" :) If you're

Oh sorry I missed that :)

> fine with that we can easily do it this way. We would then in start.c have:
> 
> struct lxc_container;
> extern void mod_all_rdeps(struct lxc_container *c, bool inc);
> 
> /* Lots of code */
> 
> static void lxc_destroy_container_on_signal(struct lxc_handler 
> *handler,
>   const char *name)
> {
>   char destroy[MAXPATHLEN];
>   bool bret = true;
>   int ret = 0;
>   struct lxc_container *c;
>   if (handler->conf && handler->conf->rootfs.path && 
> handler->conf->rootfs.mount) {
>   bret = do_destroy_container(handler->conf);
>   if (!bret) {
>   ERROR("Error destroying rootfs for %s", name);
>   return;
>   }
>   }
>   INFO("Destroyed rootfs for %s", name);
> 
>   ret = snprintf(destroy, MAXPATHLEN, "%s/%s", handler->lxcpath, 
> name);
>   if (ret < 0 || ret >= MAXPATHLEN) {
>   ERROR("Error printing path for %s", name);
>   ERROR("Error destroying directory for %s", name);
>   return;
>   }
> 
> /* Relevant part start */
> 
>   c = lxc_container_new(name, handler->lxcpath);
>   if (c) {
>   if (container_disk_lock(c)) {
>   INFO("Could not update lxc_snapshots file");
>   lxc_container_put(c);
>   } else {
>   mod_all_rdeps(c, false);
>   container_disk_unlock(c);
>   lxc_container_put(c);
>   }
>   }
> 
> /* Relevant part end */
> 
>   if (am_unpriv())
>   ret = userns_exec_1(handler->conf, 
> lxc_rmdir_onedev_wrapper, destroy);
>   else
>   ret = lxc_rmdir_onedev(destroy, NULL);
> 
>   if (ret < 0) {
>   ERROR("Error destroying directory for %s", name);
>   return;
>   }
>   INFO("Destroyed directory for %s", name);
> }
> 
> I could either just update this patch and resend it as v4 or update the whole
> series of patches I sent so far, including this one. Whatever you find more
> convenient.

The rest of the patches have already been acked and i dont 'want to confuse
Stéphane, so let's go with a new v4.

thanks!

-serge
___
lxc-devel mailing list
lxc-devel@lists.linuxcontainers.org
http://lists.linuxcontainers.org/listinfo/lxc-devel

Re: [lxc-devel] [PATCH] Add remove_snapshots_entry() (rebased - v2)

[Serge Hallyn]

Quoting Christian Brauner (christianvanbrau...@gmail.com):
> When creating ephemeral containers that have the option lxc.ephemeral = 1 set
> in their config, they will be destroyed on shutdown. As they are simple 
> overlay
> clones of an existing container they should be registered in the lxc_snapshots
> file of the original container to stay consistent and adhere to the
> expectancies of the users. Most of all, it ensure that we cannot remove a
> container that has clones, even if they are just ephemeral snapshot-clones. 
> The
> function adds further consistency because remove_snapshots_entry() ensures 
> that
> ephemeral clone-snapshots deregister themselves from the lxc_snapshots file
> when they are destroyed.
> 
> POSSIBLE GLITCH:
> I was thinking hard about racing conditions and concurrent acces on the
> lxc_snapshots file when lxc-destroy is called on the container while we
> shutdown then container from inside. Here is what my thoughts are so far:
> 
> There should be no racing condition when lxc-destroy including all snapshots 
> is

Note that lxcapi_destroy_with_snapshots() deletes the *snapshots*, not the
snapshot clones.  This is an unfortunate naming clash (which we could try
to correct henceforth but we need good names :), but they are different.
So anything under /var/lib/lxc/$container/snaps will be deleted.  But if
you've created an overlayfs clone, then containers listed in
/var/lib/lxc/$container/lxc_snapshots will not be deleted.  There is no
API call or program to automatically deleted those right now.  (I don't
think we want to write one, but a program to show which snapshots exist
would be good).

(Actually, there seems to be a bug right now - The sequence:

lxc-create -t download -n w1 -- -d ubuntu -r wily -a amd64
lxc-clone -s -B overlayfs -o w1 -n w2
lxc-snapshot -n w2
lxc-snapshot -n w2 -r snap0

does not result in /var/lib/lxc/w2/snap0 being deleted, so a subsequent

lxc-destroy -n w2

is refused.

Do you have time to either look into that, or raise an issue on github
about it?

> called and the container in question is running, lxc-destroy will simply fail
> with the warning that the clone is still running but lxc-destroy won't touch
> the lxc_snapshots file. The offending container will then exit and delete the
> container entry. lxc-destroy can then be called again and will delete the
> remaining containers.
> 
> The strange case seems to be when we create another clone-snapshot while
> another one shuts down. Does someone have any arguments against this way of

This should be fine, because mod_rdep does a container_disk_lock(c0).  So
the inc and dec will be mutually exclusive.

> implementing it? Do we expect trouble? Do we need flocks in start.c and
> lxccontainer.c?
> 
> Christian Brauner (1):
>   Add remove_snapshots_entry()
> 
>  src/lxc/start.c | 123 
> 
>  1 file changed, 123 insertions(+)
> 
> -- 
> 2.5.1
> 
___
lxc-devel mailing list
lxc-devel@lists.linuxcontainers.org
http://lists.linuxcontainers.org/listinfo/lxc-devel

Re: [lxc-devel] [PATCH] Add remove_snapshots_entry() (rebased - v2)

[Serge Hallyn]

Quoting Christian Brauner (christianvanbrau...@gmail.com):
> When creating ephemeral containers that have the option lxc.ephemeral = 1 set
> in their config, they will be destroyed on shutdown. As they are simple 
> overlay
> clones of an existing container they should be registered in the lxc_snapshots
> file of the original container to stay consistent and adhere to the
> expectancies of the users. Most of all, it ensure that we cannot remove a
> container that has clones, even if they are just ephemeral snapshot-clones. 
> The
> function adds further consistency because remove_snapshots_entry() ensures 
> that
> ephemeral clone-snapshots deregister themselves from the lxc_snapshots file
> when they are destroyed.
> 
> Signed-off-by: Christian Brauner 
> ---
>  src/lxc/start.c | 123 
> 
>  1 file changed, 123 insertions(+)
> 
> diff --git a/src/lxc/start.c b/src/lxc/start.c
> index 8fe08a1..f58d9a4 100644
> --- a/src/lxc/start.c
> +++ b/src/lxc/start.c
> @@ -33,6 +33,7 @@
>  #include 
>  #include 
>  #include 
> +#include 
>  #include 
>  #include 
>  #include 
> @@ -83,6 +84,7 @@ const struct ns_info ns_info[LXC_NS_MAX] = {
>   [LXC_NS_NET] = {"net", CLONE_NEWNET}
>  };
>  
> +static void remove_snapshot_entry(struct lxc_handler *handler, const char 
> *name);
>  static bool do_destroy_container(struct lxc_conf *conf);
>  static int lxc_rmdir_onedev_wrapper(void *data);
>  static void lxc_destroy_container_on_signal(struct lxc_handler *handler,
> @@ -1322,6 +1324,8 @@ static void lxc_destroy_container_on_signal(struct 
> lxc_handler *handler,
>   return;
>   }
>  
> + remove_snapshot_entry(handler, name);

There's probably a reason I haven't considered - but shouldn't this
really be using mod_all_rdeps(c, false)?

Well, technically mod_all_rdeps takes a lxc_container which you don't have
here.  So mod_all_rdeps() and remove_snapshot_entry() should be sharing a
common fn which takes lxcpath and lxcname and does the bulk of the work.

> +
>   if (am_unpriv())
>   ret = userns_exec_1(handler->conf, lxc_rmdir_onedev_wrapper, 
> destroy);
>   else
> @@ -1349,3 +1353,122 @@ static bool do_destroy_container(struct lxc_conf 
> *conf) {
>  return bdev_destroy(conf);
>  }
>  
> +static void remove_snapshot_entry(struct lxc_handler *handler, const char 
> *name)
> +{
> + char rdepfile[MAXPATHLEN];
> + char path[MAXPATHLEN];
> + char newpath[MAXPATHLEN];
> + char *base = NULL;
> + char *buf = NULL;
> + char *del = NULL;
> + char *scratch = NULL;
> + char *origpath = NULL;
> + char *origname = NULL;
> + int ret = 0;
> + int bytes = 0;
> + int fd = -1;
> + size_t len = 0;
> + struct stat fbuf;
> +
> + ret = snprintf(rdepfile, MAXPATHLEN, "%s/%s/lxc_rdepends", 
> handler->lxcpath, name);
> + if (ret < 0 || ret >= MAXPATHLEN)
> + return;
> +
> + fd = open(rdepfile, O_RDONLY | O_CLOEXEC);
> + if (fd < 0)
> + return;
> +
> + ret = fstat(fd, );
> + if (ret < 0) {
> + close(fd);
> + return;
> + }
> +
> + base = calloc(fbuf.st_size + 1, sizeof(char));
> + if (!base) {
> + close(fd);
> + return;
> + }
> +
> + ret = read(fd, (void *)base, fbuf.st_size);
> + if (ret <= 0) {
> + free(base);
> + close(fd);
> + return;
> + }
> +
> + origpath = strtok_r(base, "\n", );
> + if (!origpath) {
> + free(base);
> + close(fd);
> + return;
> + }
> +
> + origname = strtok_r(NULL, "\n", );
> + if (!origname) {
> + free(base);
> + close(fd);
> + return;
> + }
> +
> + close(fd);
> +
> + ret = snprintf(path, MAXPATHLEN, "%s/%s/lxc_snapshots", origpath, 
> origname);
> + if (ret < 0 || ret >= MAXPATHLEN) {
> + free(base);
> + return;
> + }
> + free(base);
> +
> + ret = snprintf(newpath, MAXPATHLEN, "%s\n%s\n", handler->lxcpath, name);
> + if (ret < 0 || ret >= MAXPATHLEN)
> + return;
> +
> + fd = open(path, O_RDWR | O_CLOEXEC);
> + if (fd < 0)
> + return;
> +
> + ret = fstat(fd, );
> + if (ret < 0) {
> + close(fd);
> + return;
> + }
> +
> + if (fbuf.st_size != 0) {
> + /* write terminating \0-byte to file */
> + if (pwrite(fd, "", 1, fbuf.st_size) <= 0) {
> + close(fd);
> + return;
> + }
> +
> + buf = mmap(NULL, fbuf.st_size + 1, PROT_READ | PROT_WRITE, 
> MAP_SHARED, fd, 0);
> + if (buf == MAP_FAILED) {
> + SYSERROR("Failed to create mapping %s", path);
> + close(fd);
> + return;
> + }
> +
> + len = 

Re: [lxc-devel] [PATCH] Add remove_snapshots_entry() (rebased - v2)

[Christian Brauner]

On Mon, Sep 14, 2015 at 03:03:58PM +, Serge Hallyn wrote:
> Quoting Christian Brauner (christianvanbrau...@gmail.com):
> > When creating ephemeral containers that have the option lxc.ephemeral = 1 
> > set
> > in their config, they will be destroyed on shutdown. As they are simple 
> > overlay
> > clones of an existing container they should be registered in the 
> > lxc_snapshots
> > file of the original container to stay consistent and adhere to the
> > expectancies of the users. Most of all, it ensure that we cannot remove a
> > container that has clones, even if they are just ephemeral snapshot-clones. 
> > The
> > function adds further consistency because remove_snapshots_entry() ensures 
> > that
> > ephemeral clone-snapshots deregister themselves from the lxc_snapshots file
> > when they are destroyed.
> > 
> > Signed-off-by: Christian Brauner 
> > ---
> >  src/lxc/start.c | 123 
> > 
> >  1 file changed, 123 insertions(+)
> > 
> > diff --git a/src/lxc/start.c b/src/lxc/start.c
> > index 8fe08a1..f58d9a4 100644
> > --- a/src/lxc/start.c
> > +++ b/src/lxc/start.c
> > @@ -33,6 +33,7 @@
> >  #include 
> >  #include 
> >  #include 
> > +#include 
> >  #include 
> >  #include 
> >  #include 
> > @@ -83,6 +84,7 @@ const struct ns_info ns_info[LXC_NS_MAX] = {
> > [LXC_NS_NET] = {"net", CLONE_NEWNET}
> >  };
> >  
> > +static void remove_snapshot_entry(struct lxc_handler *handler, const char 
> > *name);
> >  static bool do_destroy_container(struct lxc_conf *conf);
> >  static int lxc_rmdir_onedev_wrapper(void *data);
> >  static void lxc_destroy_container_on_signal(struct lxc_handler *handler,
> > @@ -1322,6 +1324,8 @@ static void lxc_destroy_container_on_signal(struct 
> > lxc_handler *handler,
> > return;
> > }
> >  
> > +   remove_snapshot_entry(handler, name);
> 
> There's probably a reason I haven't considered - but shouldn't this
> really be using mod_all_rdeps(c, false)?
> 
> Well, technically mod_all_rdeps takes a lxc_container which you don't have
> here.  So mod_all_rdeps() and remove_snapshot_entry() should be sharing a
> common fn which takes lxcpath and lxcname and does the bulk of the work.

There's only one reason, I didn't want to make a function public without your
permission. :) I'll rewrite and resend the patch.

> 
> > +
> > if (am_unpriv())
> > ret = userns_exec_1(handler->conf, lxc_rmdir_onedev_wrapper, 
> > destroy);
> > else
> > @@ -1349,3 +1353,122 @@ static bool do_destroy_container(struct lxc_conf 
> > *conf) {
> >  return bdev_destroy(conf);
> >  }
> >  
> > +static void remove_snapshot_entry(struct lxc_handler *handler, const char 
> > *name)
> > +{
> > +   char rdepfile[MAXPATHLEN];
> > +   char path[MAXPATHLEN];
> > +   char newpath[MAXPATHLEN];
> > +   char *base = NULL;
> > +   char *buf = NULL;
> > +   char *del = NULL;
> > +   char *scratch = NULL;
> > +   char *origpath = NULL;
> > +   char *origname = NULL;
> > +   int ret = 0;
> > +   int bytes = 0;
> > +   int fd = -1;
> > +   size_t len = 0;
> > +   struct stat fbuf;
> > +
> > +   ret = snprintf(rdepfile, MAXPATHLEN, "%s/%s/lxc_rdepends", 
> > handler->lxcpath, name);
> > +   if (ret < 0 || ret >= MAXPATHLEN)
> > +   return;
> > +
> > +   fd = open(rdepfile, O_RDONLY | O_CLOEXEC);
> > +   if (fd < 0)
> > +   return;
> > +
> > +   ret = fstat(fd, );
> > +   if (ret < 0) {
> > +   close(fd);
> > +   return;
> > +   }
> > +
> > +   base = calloc(fbuf.st_size + 1, sizeof(char));
> > +   if (!base) {
> > +   close(fd);
> > +   return;
> > +   }
> > +
> > +   ret = read(fd, (void *)base, fbuf.st_size);
> > +   if (ret <= 0) {
> > +   free(base);
> > +   close(fd);
> > +   return;
> > +   }
> > +
> > +   origpath = strtok_r(base, "\n", );
> > +   if (!origpath) {
> > +   free(base);
> > +   close(fd);
> > +   return;
> > +   }
> > +
> > +   origname = strtok_r(NULL, "\n", );
> > +   if (!origname) {
> > +   free(base);
> > +   close(fd);
> > +   return;
> > +   }
> > +
> > +   close(fd);
> > +
> > +   ret = snprintf(path, MAXPATHLEN, "%s/%s/lxc_snapshots", origpath, 
> > origname);
> > +   if (ret < 0 || ret >= MAXPATHLEN) {
> > +   free(base);
> > +   return;
> > +   }
> > +   free(base);
> > +
> > +   ret = snprintf(newpath, MAXPATHLEN, "%s\n%s\n", handler->lxcpath, name);
> > +   if (ret < 0 || ret >= MAXPATHLEN)
> > +   return;
> > +
> > +   fd = open(path, O_RDWR | O_CLOEXEC);
> > +   if (fd < 0)
> > +   return;
> > +
> > +   ret = fstat(fd, );
> > +   if (ret < 0) {
> > +   close(fd);
> > +   return;
> > +   }
> > +
> > +   if (fbuf.st_size != 0) {
> > +   /* write terminating \0-byte to file */
> > +   if (pwrite(fd, "", 1, fbuf.st_size) <= 0) {
> > +   close(fd);
> > +

Re: [lxc-devel] [PATCH] Add remove_snapshots_entry() (rebased - v2)

[Christian Brauner]

On Mon, Sep 14, 2015 at 02:50:39PM +, Serge Hallyn wrote:
> Quoting Christian Brauner (christianvanbrau...@gmail.com):
> > When creating ephemeral containers that have the option lxc.ephemeral = 1 
> > set
> > in their config, they will be destroyed on shutdown. As they are simple 
> > overlay
> > clones of an existing container they should be registered in the 
> > lxc_snapshots
> > file of the original container to stay consistent and adhere to the
> > expectancies of the users. Most of all, it ensure that we cannot remove a
> > container that has clones, even if they are just ephemeral snapshot-clones. 
> > The
> > function adds further consistency because remove_snapshots_entry() ensures 
> > that
> > ephemeral clone-snapshots deregister themselves from the lxc_snapshots file
> > when they are destroyed.
> > 
> > POSSIBLE GLITCH:
> > I was thinking hard about racing conditions and concurrent acces on the
> > lxc_snapshots file when lxc-destroy is called on the container while we
> > shutdown then container from inside. Here is what my thoughts are so far:
> > 
> > There should be no racing condition when lxc-destroy including all 
> > snapshots is
> 
> Note that lxcapi_destroy_with_snapshots() deletes the *snapshots*, not the
> snapshot clones.  This is an unfortunate naming clash (which we could try
> to correct henceforth but we need good names :), but they are different.
> So anything under /var/lib/lxc/$container/snaps will be deleted.  But if
> you've created an overlayfs clone, then containers listed in
> /var/lib/lxc/$container/lxc_snapshots will not be deleted. There is no
> API call or program to automatically deleted those right now.

I think you are partially wrong here. I was not thinking about problems created
by an API-call but by the lxc-destroy exectuable. A quick walkthrough: With the
recent patches by me to lxc-destroy when I do:

lxc-clone -s -B overlayfs -o w1 -n w2

the snapshot-clone will be registered in the lxc_snapshots file and when I call

lxc-destroy -n w1 -s

the original container including all it's snapshot-clones will be destroyed and
deleted from the lxc_snapshots file. This is done by having lxc-destroy open the
lxc_snapshots file of the original container, reading in all snapshot-clone
entries in and deleting them.

> think we want to write one, but a program to show which snapshots exist
> would be good).
> 
> (Actually, there seems to be a bug right now - The sequence:
> 
>   lxc-create -t download -n w1 -- -d ubuntu -r wily -a amd64
>   lxc-clone -s -B overlayfs -o w1 -n w2
>   lxc-snapshot -n w2
>   lxc-snapshot -n w2 -r snap0
> 
> does not result in /var/lib/lxc/w2/snap0 being deleted, so a subsequent
> 
>   lxc-destroy -n w2
> 
> is refused.
> 
> Do you have time to either look into that, or raise an issue on github
> about it?

I'll take a look tonight. Don't know how fast I can fix this.

> 
> > called and the container in question is running, lxc-destroy will simply 
> > fail
> > with the warning that the clone is still running but lxc-destroy won't touch
> > the lxc_snapshots file. The offending container will then exit and delete 
> > the
> > container entry. lxc-destroy can then be called again and will delete the
> > remaining containers.
> > 
> > The strange case seems to be when we create another clone-snapshot while
> > another one shuts down. Does someone have any arguments against this way of
> 
> This should be fine, because mod_rdep does a container_disk_lock(c0).  So
> the inc and dec will be mutually exclusive.
> 
> > implementing it? Do we expect trouble? Do we need flocks in start.c and
> > lxccontainer.c?
> > 
> > Christian Brauner (1):
> >   Add remove_snapshots_entry()
> > 
> >  src/lxc/start.c | 123 
> > 
> >  1 file changed, 123 insertions(+)
> > 
> > -- 
> > 2.5.1
> > 


signature.asc
Description: PGP signature
___
lxc-devel mailing list
lxc-devel@lists.linuxcontainers.org
http://lists.linuxcontainers.org/listinfo/lxc-devel

Re: [lxc-devel] [PATCH] Add remove_snapshots_entry() (rebased - v2)

[Serge Hallyn]

Quoting Christian Brauner (christianvanbrau...@gmail.com):
> On Mon, Sep 14, 2015 at 02:50:39PM +, Serge Hallyn wrote:
> > Quoting Christian Brauner (christianvanbrau...@gmail.com):
> > > When creating ephemeral containers that have the option lxc.ephemeral = 1 
> > > set
> > > in their config, they will be destroyed on shutdown. As they are simple 
> > > overlay
> > > clones of an existing container they should be registered in the 
> > > lxc_snapshots
> > > file of the original container to stay consistent and adhere to the
> > > expectancies of the users. Most of all, it ensure that we cannot remove a
> > > container that has clones, even if they are just ephemeral 
> > > snapshot-clones. The
> > > function adds further consistency because remove_snapshots_entry() 
> > > ensures that
> > > ephemeral clone-snapshots deregister themselves from the lxc_snapshots 
> > > file
> > > when they are destroyed.
> > > 
> > > POSSIBLE GLITCH:
> > > I was thinking hard about racing conditions and concurrent acces on the
> > > lxc_snapshots file when lxc-destroy is called on the container while we
> > > shutdown then container from inside. Here is what my thoughts are so far:
> > > 
> > > There should be no racing condition when lxc-destroy including all 
> > > snapshots is
> > 
> > Note that lxcapi_destroy_with_snapshots() deletes the *snapshots*, not the
> > snapshot clones.  This is an unfortunate naming clash (which we could try
> > to correct henceforth but we need good names :), but they are different.
> > So anything under /var/lib/lxc/$container/snaps will be deleted.  But if
> > you've created an overlayfs clone, then containers listed in
> > /var/lib/lxc/$container/lxc_snapshots will not be deleted.  There is no
> > API call or program to automatically deleted those right now.  (I don't
> > think we want to write one, but a program to show which snapshots exist
> > would be good).
> > 
> > (Actually, there seems to be a bug right now - The sequence:
> > 
> > lxc-create -t download -n w1 -- -d ubuntu -r wily -a amd64
> > lxc-clone -s -B overlayfs -o w1 -n w2
> > lxc-snapshot -n w2
> > lxc-snapshot -n w2 -r snap0
> > 
> > does not result in /var/lib/lxc/w2/snap0 being deleted, so a subsequent
> > 
> > lxc-destroy -n w2
> > 
> > is refused.
> 
> Has the bug been introduced by changes I made. It does not look like it as 
> this

D'oh, sorry, I was testing wrongly.  I was thinking '-r' meant
remove.

Please ignore :)

-serge
___
lxc-devel mailing list
lxc-devel@lists.linuxcontainers.org
http://lists.linuxcontainers.org/listinfo/lxc-devel

Re: [lxc-devel] [PATCH] Add remove_snapshots_entry() (rebased - v2)

[Christian Brauner]

On Mon, Sep 14, 2015 at 02:50:39PM +, Serge Hallyn wrote:
> Quoting Christian Brauner (christianvanbrau...@gmail.com):
> > When creating ephemeral containers that have the option lxc.ephemeral = 1 
> > set
> > in their config, they will be destroyed on shutdown. As they are simple 
> > overlay
> > clones of an existing container they should be registered in the 
> > lxc_snapshots
> > file of the original container to stay consistent and adhere to the
> > expectancies of the users. Most of all, it ensure that we cannot remove a
> > container that has clones, even if they are just ephemeral snapshot-clones. 
> > The
> > function adds further consistency because remove_snapshots_entry() ensures 
> > that
> > ephemeral clone-snapshots deregister themselves from the lxc_snapshots file
> > when they are destroyed.
> > 
> > POSSIBLE GLITCH:
> > I was thinking hard about racing conditions and concurrent acces on the
> > lxc_snapshots file when lxc-destroy is called on the container while we
> > shutdown then container from inside. Here is what my thoughts are so far:
> > 
> > There should be no racing condition when lxc-destroy including all 
> > snapshots is
> 
> Note that lxcapi_destroy_with_snapshots() deletes the *snapshots*, not the
> snapshot clones.  This is an unfortunate naming clash (which we could try
> to correct henceforth but we need good names :), but they are different.
> So anything under /var/lib/lxc/$container/snaps will be deleted.  But if
> you've created an overlayfs clone, then containers listed in
> /var/lib/lxc/$container/lxc_snapshots will not be deleted.  There is no
> API call or program to automatically deleted those right now.  (I don't
> think we want to write one, but a program to show which snapshots exist
> would be good).
> 
> (Actually, there seems to be a bug right now - The sequence:
> 
>   lxc-create -t download -n w1 -- -d ubuntu -r wily -a amd64
>   lxc-clone -s -B overlayfs -o w1 -n w2
>   lxc-snapshot -n w2
>   lxc-snapshot -n w2 -r snap0
> 
> does not result in /var/lib/lxc/w2/snap0 being deleted, so a subsequent
> 
>   lxc-destroy -n w2
> 
> is refused.

Has the bug been introduced by changes I made. It does not look like it as this
is also the behavior in stable-1.1. So what you want is to have the snapshot
deleted after having restored from it once? That seems not what we want. Btw, if
one would really like to have w2 removed there is always the option to do a
simple

lxc-destroy -n w2 -s

which means remove including all snapshots...

> 
> Do you have time to either look into that, or raise an issue on github
> about it?
> 
> > called and the container in question is running, lxc-destroy will simply 
> > fail
> > with the warning that the clone is still running but lxc-destroy won't touch
> > the lxc_snapshots file. The offending container will then exit and delete 
> > the
> > container entry. lxc-destroy can then be called again and will delete the
> > remaining containers.
> > 
> > The strange case seems to be when we create another clone-snapshot while
> > another one shuts down. Does someone have any arguments against this way of
> 
> This should be fine, because mod_rdep does a container_disk_lock(c0).  So
> the inc and dec will be mutually exclusive.
> 
> > implementing it? Do we expect trouble? Do we need flocks in start.c and
> > lxccontainer.c?
> > 
> > Christian Brauner (1):
> >   Add remove_snapshots_entry()
> > 
> >  src/lxc/start.c | 123 
> > 
> >  1 file changed, 123 insertions(+)
> > 
> > -- 
> > 2.5.1
> > 


signature.asc
Description: PGP signature
___
lxc-devel mailing list
lxc-devel@lists.linuxcontainers.org
http://lists.linuxcontainers.org/listinfo/lxc-devel

Re: [lxc-devel] [PATCH] Add remove_snapshots_entry() (rebased - v2)

[Christian Brauner]

On Mon, Sep 14, 2015 at 08:51:08PM +, Serge Hallyn wrote:
> Quoting Christian Brauner (christianvanbrau...@gmail.com):
> > On Mon, Sep 14, 2015 at 08:31:59PM +, Serge Hallyn wrote:
> > > Quoting Christian Brauner (christianvanbrau...@gmail.com):
> > > > On Mon, Sep 14, 2015 at 07:27:06PM +, Serge Hallyn wrote:
> > > > > Quoting Christian Brauner (christianvanbrau...@gmail.com):
> > > > > > On Mon, Sep 14, 2015 at 04:33:05PM +, Serge Hallyn wrote:
> > > > > > > Quoting Serge Hallyn (serge.hal...@ubuntu.com):
> > > > > > > > Quoting Christian Brauner (christianvanbrau...@gmail.com):
> > > > > > > > > On Mon, Sep 14, 2015 at 02:50:39PM +, Serge Hallyn wrote:
> > > > > > > > > > Quoting Christian Brauner (christianvanbrau...@gmail.com):
> > > > > > > > > > > When creating ephemeral containers that have the option 
> > > > > > > > > > > lxc.ephemeral = 1 set
> > > > > > > > > > > in their config, they will be destroyed on shutdown. As 
> > > > > > > > > > > they are simple overlay
> > > > > > > > > > > clones of an existing container they should be registered 
> > > > > > > > > > > in the lxc_snapshots
> > > > > > > > > > > file of the original container to stay consistent and 
> > > > > > > > > > > adhere to the
> > > > > > > > > > > expectancies of the users. Most of all, it ensure that we 
> > > > > > > > > > > cannot remove a
> > > > > > > > > > > container that has clones, even if they are just 
> > > > > > > > > > > ephemeral snapshot-clones. The
> > > > > > > > > > > function adds further consistency because 
> > > > > > > > > > > remove_snapshots_entry() ensures that
> > > > > > > > > > > ephemeral clone-snapshots deregister themselves from the 
> > > > > > > > > > > lxc_snapshots file
> > > > > > > > > > > when they are destroyed.
> > > > > > > > > > > 
> > > > > > > > > > > POSSIBLE GLITCH:
> > > > > > > > > > > I was thinking hard about racing conditions and 
> > > > > > > > > > > concurrent acces on the
> > > > > > > > > > > lxc_snapshots file when lxc-destroy is called on the 
> > > > > > > > > > > container while we
> > > > > > > > > > > shutdown then container from inside. Here is what my 
> > > > > > > > > > > thoughts are so far:
> > > > > > > > > > > 
> > > > > > > > > > > There should be no racing condition when lxc-destroy 
> > > > > > > > > > > including all snapshots is
> > > > > > > > > > 
> > > > > > > > > > Note that lxcapi_destroy_with_snapshots() deletes the 
> > > > > > > > > > *snapshots*, not the
> > > > > > > > > > snapshot clones.  This is an unfortunate naming clash 
> > > > > > > > > > (which we could try
> > > > > > > > > > to correct henceforth but we need good names :), but they 
> > > > > > > > > > are different.
> > > > > > > > > > So anything under /var/lib/lxc/$container/snaps will be 
> > > > > > > > > > deleted.  But if
> > > > > > > > > > you've created an overlayfs clone, then containers listed in
> > > > > > > > > > /var/lib/lxc/$container/lxc_snapshots will not be deleted. 
> > > > > > > > > > There is no
> > > > > > > > > > API call or program to automatically deleted those right 
> > > > > > > > > > now.
> > > > > > > > > 
> > > > > > > > > I think you are partially wrong here. I was not thinking 
> > > > > > > > > about problems created
> > > > > > > > > by an API-call but by the lxc-destroy exectuable. A quick 
> > > > > > > > > walkthrough: With the
> > > > > > > > 
> > > > > > > > D'oh.  Yeah, you'll need to mutex that somehow.
> > > > > > > 
> > > > > > > If you want help up-front with the design, please let me know.  
> > > > > > > If you
> > > > > > > aren't sure what the current container_disk_lock() and 
> > > > > > > container_mem_lock()
> > > > > > > do, please shout.  (they are explained in a LOCKING comment above
> > > > > > > lxc_container_free() in src/lxc/lxccontainer.c)
> > > > > > > 
> > > > > > > The easiest thing to do mght be to disk_lock the container in 
> > > > > > > lxc_destroy.c,
> > > > > > > then make the mod_rdep() helper which you use in lxc_destroy.c be 
> > > > > > > a _locked
> > > > > > > variant (to avoid deadlock).  So mod_rdep() would turn into 
> > > > > > > something like:
> > > > > > > 
> > > > > > > static bool mod_rdep(struct lxc_container *c0, struct 
> > > > > > > lxc_container *c, bool inc)
> > > > > > > {
> > > > > > >   bool ret;
> > > > > > >   if (container_disk_lock(c0))
> > > > > > >   return false;
> > > > > > >   ret = mod_rdep_locked(c0->name, c0->lxcpath, c->name, 
> > > > > > > c->lxcpath);
> > > > > > >   container_disk_unlock(c0);
> > > > > > >   return ret;
> > > > > > > }
> > > > > > > 
> > > > > > > -serge
> > > > > > 
> > > > > > Thanks, this is really nice! One question:
> > > > > > - I'll take it that we want to make mod_rdep() public. mod_rdep() 
> > > > > > will be used
> > > > > >   in lxc_destroy.c, lxccontainer.c and start.c. Problem is that in 
> > > > > > start.c we do
> > > > > >   not have a container to pass into mod_rdep(). Do you want me to 
> > > > > > 

Re: [lxc-devel] [PATCH] Add remove_snapshots_entry() (rebased - v2)

[Serge Hallyn]

Quoting Christian Brauner (christianvanbrau...@gmail.com):
> On Mon, Sep 14, 2015 at 08:31:59PM +, Serge Hallyn wrote:
> > Quoting Christian Brauner (christianvanbrau...@gmail.com):
> > > On Mon, Sep 14, 2015 at 07:27:06PM +, Serge Hallyn wrote:
> > > > Quoting Christian Brauner (christianvanbrau...@gmail.com):
> > > > > On Mon, Sep 14, 2015 at 04:33:05PM +, Serge Hallyn wrote:
> > > > > > Quoting Serge Hallyn (serge.hal...@ubuntu.com):
> > > > > > > Quoting Christian Brauner (christianvanbrau...@gmail.com):
> > > > > > > > On Mon, Sep 14, 2015 at 02:50:39PM +, Serge Hallyn wrote:
> > > > > > > > > Quoting Christian Brauner (christianvanbrau...@gmail.com):
> > > > > > > > > > When creating ephemeral containers that have the option 
> > > > > > > > > > lxc.ephemeral = 1 set
> > > > > > > > > > in their config, they will be destroyed on shutdown. As 
> > > > > > > > > > they are simple overlay
> > > > > > > > > > clones of an existing container they should be registered 
> > > > > > > > > > in the lxc_snapshots
> > > > > > > > > > file of the original container to stay consistent and 
> > > > > > > > > > adhere to the
> > > > > > > > > > expectancies of the users. Most of all, it ensure that we 
> > > > > > > > > > cannot remove a
> > > > > > > > > > container that has clones, even if they are just ephemeral 
> > > > > > > > > > snapshot-clones. The
> > > > > > > > > > function adds further consistency because 
> > > > > > > > > > remove_snapshots_entry() ensures that
> > > > > > > > > > ephemeral clone-snapshots deregister themselves from the 
> > > > > > > > > > lxc_snapshots file
> > > > > > > > > > when they are destroyed.
> > > > > > > > > > 
> > > > > > > > > > POSSIBLE GLITCH:
> > > > > > > > > > I was thinking hard about racing conditions and concurrent 
> > > > > > > > > > acces on the
> > > > > > > > > > lxc_snapshots file when lxc-destroy is called on the 
> > > > > > > > > > container while we
> > > > > > > > > > shutdown then container from inside. Here is what my 
> > > > > > > > > > thoughts are so far:
> > > > > > > > > > 
> > > > > > > > > > There should be no racing condition when lxc-destroy 
> > > > > > > > > > including all snapshots is
> > > > > > > > > 
> > > > > > > > > Note that lxcapi_destroy_with_snapshots() deletes the 
> > > > > > > > > *snapshots*, not the
> > > > > > > > > snapshot clones.  This is an unfortunate naming clash (which 
> > > > > > > > > we could try
> > > > > > > > > to correct henceforth but we need good names :), but they are 
> > > > > > > > > different.
> > > > > > > > > So anything under /var/lib/lxc/$container/snaps will be 
> > > > > > > > > deleted.  But if
> > > > > > > > > you've created an overlayfs clone, then containers listed in
> > > > > > > > > /var/lib/lxc/$container/lxc_snapshots will not be deleted. 
> > > > > > > > > There is no
> > > > > > > > > API call or program to automatically deleted those right now.
> > > > > > > > 
> > > > > > > > I think you are partially wrong here. I was not thinking about 
> > > > > > > > problems created
> > > > > > > > by an API-call but by the lxc-destroy exectuable. A quick 
> > > > > > > > walkthrough: With the
> > > > > > > 
> > > > > > > D'oh.  Yeah, you'll need to mutex that somehow.
> > > > > > 
> > > > > > If you want help up-front with the design, please let me know.  If 
> > > > > > you
> > > > > > aren't sure what the current container_disk_lock() and 
> > > > > > container_mem_lock()
> > > > > > do, please shout.  (they are explained in a LOCKING comment above
> > > > > > lxc_container_free() in src/lxc/lxccontainer.c)
> > > > > > 
> > > > > > The easiest thing to do mght be to disk_lock the container in 
> > > > > > lxc_destroy.c,
> > > > > > then make the mod_rdep() helper which you use in lxc_destroy.c be a 
> > > > > > _locked
> > > > > > variant (to avoid deadlock).  So mod_rdep() would turn into 
> > > > > > something like:
> > > > > > 
> > > > > > static bool mod_rdep(struct lxc_container *c0, struct lxc_container 
> > > > > > *c, bool inc)
> > > > > > {
> > > > > > bool ret;
> > > > > > if (container_disk_lock(c0))
> > > > > > return false;
> > > > > > ret = mod_rdep_locked(c0->name, c0->lxcpath, c->name, 
> > > > > > c->lxcpath);
> > > > > > container_disk_unlock(c0);
> > > > > > return ret;
> > > > > > }
> > > > > > 
> > > > > > -serge
> > > > > 
> > > > > Thanks, this is really nice! One question:
> > > > > - I'll take it that we want to make mod_rdep() public. mod_rdep() 
> > > > > will be used
> > > > >   in lxc_destroy.c, lxccontainer.c and start.c. Problem is that in 
> > > > > start.c we do
> > > > >   not have a container to pass into mod_rdep(). Do you want me to 
> > > > > rewrite
> > > > >   mod_rdep() to take in lxcpath and lxcname? If so, could we still use
> > > > 
> > > > Ok, I'm not running on all cylinders, sorry.
> > > > 
> > > > You don't want mod_rdep, you want mod_all_rdeps.  So yes export that,
> > > > make a struct 

Re: [lxc-devel] [PATCH] Add remove_snapshots_entry() (rebased - v2)

[Christian Brauner]

On Mon, Sep 14, 2015 at 07:27:06PM +, Serge Hallyn wrote:
> Quoting Christian Brauner (christianvanbrau...@gmail.com):
> > On Mon, Sep 14, 2015 at 04:33:05PM +, Serge Hallyn wrote:
> > > Quoting Serge Hallyn (serge.hal...@ubuntu.com):
> > > > Quoting Christian Brauner (christianvanbrau...@gmail.com):
> > > > > On Mon, Sep 14, 2015 at 02:50:39PM +, Serge Hallyn wrote:
> > > > > > Quoting Christian Brauner (christianvanbrau...@gmail.com):
> > > > > > > When creating ephemeral containers that have the option 
> > > > > > > lxc.ephemeral = 1 set
> > > > > > > in their config, they will be destroyed on shutdown. As they are 
> > > > > > > simple overlay
> > > > > > > clones of an existing container they should be registered in the 
> > > > > > > lxc_snapshots
> > > > > > > file of the original container to stay consistent and adhere to 
> > > > > > > the
> > > > > > > expectancies of the users. Most of all, it ensure that we cannot 
> > > > > > > remove a
> > > > > > > container that has clones, even if they are just ephemeral 
> > > > > > > snapshot-clones. The
> > > > > > > function adds further consistency because 
> > > > > > > remove_snapshots_entry() ensures that
> > > > > > > ephemeral clone-snapshots deregister themselves from the 
> > > > > > > lxc_snapshots file
> > > > > > > when they are destroyed.
> > > > > > > 
> > > > > > > POSSIBLE GLITCH:
> > > > > > > I was thinking hard about racing conditions and concurrent acces 
> > > > > > > on the
> > > > > > > lxc_snapshots file when lxc-destroy is called on the container 
> > > > > > > while we
> > > > > > > shutdown then container from inside. Here is what my thoughts are 
> > > > > > > so far:
> > > > > > > 
> > > > > > > There should be no racing condition when lxc-destroy including 
> > > > > > > all snapshots is
> > > > > > 
> > > > > > Note that lxcapi_destroy_with_snapshots() deletes the *snapshots*, 
> > > > > > not the
> > > > > > snapshot clones.  This is an unfortunate naming clash (which we 
> > > > > > could try
> > > > > > to correct henceforth but we need good names :), but they are 
> > > > > > different.
> > > > > > So anything under /var/lib/lxc/$container/snaps will be deleted.  
> > > > > > But if
> > > > > > you've created an overlayfs clone, then containers listed in
> > > > > > /var/lib/lxc/$container/lxc_snapshots will not be deleted. There is 
> > > > > > no
> > > > > > API call or program to automatically deleted those right now.
> > > > > 
> > > > > I think you are partially wrong here. I was not thinking about 
> > > > > problems created
> > > > > by an API-call but by the lxc-destroy exectuable. A quick 
> > > > > walkthrough: With the
> > > > 
> > > > D'oh.  Yeah, you'll need to mutex that somehow.
> > > 
> > > If you want help up-front with the design, please let me know.  If you
> > > aren't sure what the current container_disk_lock() and 
> > > container_mem_lock()
> > > do, please shout.  (they are explained in a LOCKING comment above
> > > lxc_container_free() in src/lxc/lxccontainer.c)
> > > 
> > > The easiest thing to do mght be to disk_lock the container in 
> > > lxc_destroy.c,
> > > then make the mod_rdep() helper which you use in lxc_destroy.c be a 
> > > _locked
> > > variant (to avoid deadlock).  So mod_rdep() would turn into something 
> > > like:
> > > 
> > > static bool mod_rdep(struct lxc_container *c0, struct lxc_container *c, 
> > > bool inc)
> > > {
> > >   bool ret;
> > >   if (container_disk_lock(c0))
> > >   return false;
> > >   ret = mod_rdep_locked(c0->name, c0->lxcpath, c->name, c->lxcpath);
> > >   container_disk_unlock(c0);
> > >   return ret;
> > > }
> > > 
> > > -serge
> > 
> > Thanks, this is really nice! One question:
> > - I'll take it that we want to make mod_rdep() public. mod_rdep() will be 
> > used
> >   in lxc_destroy.c, lxccontainer.c and start.c. Problem is that in start.c 
> > we do
> >   not have a container to pass into mod_rdep(). Do you want me to rewrite
> >   mod_rdep() to take in lxcpath and lxcname? If so, could we still use
> 
> Ok, I'm not running on all cylinders, sorry.
> 
> You don't want mod_rdep, you want mod_all_rdeps.  So yes export that,
> make a struct lxc_container, lock it, and pass that to mod_all_rdeps
> which will dothe mod_rdeps for you.

Ok, so in start.c we can actually use lxc_container new. I could modify my:

static void lxc_destroy_container_on_signal(struct lxc_handler *handler,
const char *name)
{
char destroy[MAXPATHLEN];
bool bret = true;
int ret = 0;
if (handler->conf && handler->conf->rootfs.path && 
handler->conf->rootfs.mount) {
bret = do_destroy_container(handler->conf);
if (!bret) {
ERROR("Error destroying rootfs for %s", name);
return;
}
}
 

Re: [lxc-devel] [PATCH] Add remove_snapshots_entry() (rebased - v2)

[Serge Hallyn]

Quoting Christian Brauner (christianvanbrau...@gmail.com):
> On Mon, Sep 14, 2015 at 07:27:06PM +, Serge Hallyn wrote:
> > Quoting Christian Brauner (christianvanbrau...@gmail.com):
> > > On Mon, Sep 14, 2015 at 04:33:05PM +, Serge Hallyn wrote:
> > > > Quoting Serge Hallyn (serge.hal...@ubuntu.com):
> > > > > Quoting Christian Brauner (christianvanbrau...@gmail.com):
> > > > > > On Mon, Sep 14, 2015 at 02:50:39PM +, Serge Hallyn wrote:
> > > > > > > Quoting Christian Brauner (christianvanbrau...@gmail.com):
> > > > > > > > When creating ephemeral containers that have the option 
> > > > > > > > lxc.ephemeral = 1 set
> > > > > > > > in their config, they will be destroyed on shutdown. As they 
> > > > > > > > are simple overlay
> > > > > > > > clones of an existing container they should be registered in 
> > > > > > > > the lxc_snapshots
> > > > > > > > file of the original container to stay consistent and adhere to 
> > > > > > > > the
> > > > > > > > expectancies of the users. Most of all, it ensure that we 
> > > > > > > > cannot remove a
> > > > > > > > container that has clones, even if they are just ephemeral 
> > > > > > > > snapshot-clones. The
> > > > > > > > function adds further consistency because 
> > > > > > > > remove_snapshots_entry() ensures that
> > > > > > > > ephemeral clone-snapshots deregister themselves from the 
> > > > > > > > lxc_snapshots file
> > > > > > > > when they are destroyed.
> > > > > > > > 
> > > > > > > > POSSIBLE GLITCH:
> > > > > > > > I was thinking hard about racing conditions and concurrent 
> > > > > > > > acces on the
> > > > > > > > lxc_snapshots file when lxc-destroy is called on the container 
> > > > > > > > while we
> > > > > > > > shutdown then container from inside. Here is what my thoughts 
> > > > > > > > are so far:
> > > > > > > > 
> > > > > > > > There should be no racing condition when lxc-destroy including 
> > > > > > > > all snapshots is
> > > > > > > 
> > > > > > > Note that lxcapi_destroy_with_snapshots() deletes the 
> > > > > > > *snapshots*, not the
> > > > > > > snapshot clones.  This is an unfortunate naming clash (which we 
> > > > > > > could try
> > > > > > > to correct henceforth but we need good names :), but they are 
> > > > > > > different.
> > > > > > > So anything under /var/lib/lxc/$container/snaps will be deleted.  
> > > > > > > But if
> > > > > > > you've created an overlayfs clone, then containers listed in
> > > > > > > /var/lib/lxc/$container/lxc_snapshots will not be deleted. There 
> > > > > > > is no
> > > > > > > API call or program to automatically deleted those right now.
> > > > > > 
> > > > > > I think you are partially wrong here. I was not thinking about 
> > > > > > problems created
> > > > > > by an API-call but by the lxc-destroy exectuable. A quick 
> > > > > > walkthrough: With the
> > > > > 
> > > > > D'oh.  Yeah, you'll need to mutex that somehow.
> > > > 
> > > > If you want help up-front with the design, please let me know.  If you
> > > > aren't sure what the current container_disk_lock() and 
> > > > container_mem_lock()
> > > > do, please shout.  (they are explained in a LOCKING comment above
> > > > lxc_container_free() in src/lxc/lxccontainer.c)
> > > > 
> > > > The easiest thing to do mght be to disk_lock the container in 
> > > > lxc_destroy.c,
> > > > then make the mod_rdep() helper which you use in lxc_destroy.c be a 
> > > > _locked
> > > > variant (to avoid deadlock).  So mod_rdep() would turn into something 
> > > > like:
> > > > 
> > > > static bool mod_rdep(struct lxc_container *c0, struct lxc_container *c, 
> > > > bool inc)
> > > > {
> > > > bool ret;
> > > > if (container_disk_lock(c0))
> > > > return false;
> > > > ret = mod_rdep_locked(c0->name, c0->lxcpath, c->name, 
> > > > c->lxcpath);
> > > > container_disk_unlock(c0);
> > > > return ret;
> > > > }
> > > > 
> > > > -serge
> > > 
> > > Thanks, this is really nice! One question:
> > > - I'll take it that we want to make mod_rdep() public. mod_rdep() will be 
> > > used
> > >   in lxc_destroy.c, lxccontainer.c and start.c. Problem is that in 
> > > start.c we do
> > >   not have a container to pass into mod_rdep(). Do you want me to rewrite
> > >   mod_rdep() to take in lxcpath and lxcname? If so, could we still use
> > 
> > Ok, I'm not running on all cylinders, sorry.
> > 
> > You don't want mod_rdep, you want mod_all_rdeps.  So yes export that,
> > make a struct lxc_container, lock it, and pass that to mod_all_rdeps
> > which will dothe mod_rdeps for you.
> 
> Ok, so in start.c we can actually use lxc_container new. I could modify my:

Yeah I think that looks good,

> static void lxc_destroy_container_on_signal(struct lxc_handler 
> *handler,
> const char *name)
> {
>   char destroy[MAXPATHLEN];
>   bool bret = true;
>   int ret = 0;
>   if 

Re: [lxc-devel] [PATCH] Add remove_snapshots_entry() (rebased - v2)

[Christian Brauner]

On Mon, Sep 14, 2015 at 08:31:59PM +, Serge Hallyn wrote:
> Quoting Christian Brauner (christianvanbrau...@gmail.com):
> > On Mon, Sep 14, 2015 at 07:27:06PM +, Serge Hallyn wrote:
> > > Quoting Christian Brauner (christianvanbrau...@gmail.com):
> > > > On Mon, Sep 14, 2015 at 04:33:05PM +, Serge Hallyn wrote:
> > > > > Quoting Serge Hallyn (serge.hal...@ubuntu.com):
> > > > > > Quoting Christian Brauner (christianvanbrau...@gmail.com):
> > > > > > > On Mon, Sep 14, 2015 at 02:50:39PM +, Serge Hallyn wrote:
> > > > > > > > Quoting Christian Brauner (christianvanbrau...@gmail.com):
> > > > > > > > > When creating ephemeral containers that have the option 
> > > > > > > > > lxc.ephemeral = 1 set
> > > > > > > > > in their config, they will be destroyed on shutdown. As they 
> > > > > > > > > are simple overlay
> > > > > > > > > clones of an existing container they should be registered in 
> > > > > > > > > the lxc_snapshots
> > > > > > > > > file of the original container to stay consistent and adhere 
> > > > > > > > > to the
> > > > > > > > > expectancies of the users. Most of all, it ensure that we 
> > > > > > > > > cannot remove a
> > > > > > > > > container that has clones, even if they are just ephemeral 
> > > > > > > > > snapshot-clones. The
> > > > > > > > > function adds further consistency because 
> > > > > > > > > remove_snapshots_entry() ensures that
> > > > > > > > > ephemeral clone-snapshots deregister themselves from the 
> > > > > > > > > lxc_snapshots file
> > > > > > > > > when they are destroyed.
> > > > > > > > > 
> > > > > > > > > POSSIBLE GLITCH:
> > > > > > > > > I was thinking hard about racing conditions and concurrent 
> > > > > > > > > acces on the
> > > > > > > > > lxc_snapshots file when lxc-destroy is called on the 
> > > > > > > > > container while we
> > > > > > > > > shutdown then container from inside. Here is what my thoughts 
> > > > > > > > > are so far:
> > > > > > > > > 
> > > > > > > > > There should be no racing condition when lxc-destroy 
> > > > > > > > > including all snapshots is
> > > > > > > > 
> > > > > > > > Note that lxcapi_destroy_with_snapshots() deletes the 
> > > > > > > > *snapshots*, not the
> > > > > > > > snapshot clones.  This is an unfortunate naming clash (which we 
> > > > > > > > could try
> > > > > > > > to correct henceforth but we need good names :), but they are 
> > > > > > > > different.
> > > > > > > > So anything under /var/lib/lxc/$container/snaps will be 
> > > > > > > > deleted.  But if
> > > > > > > > you've created an overlayfs clone, then containers listed in
> > > > > > > > /var/lib/lxc/$container/lxc_snapshots will not be deleted. 
> > > > > > > > There is no
> > > > > > > > API call or program to automatically deleted those right now.
> > > > > > > 
> > > > > > > I think you are partially wrong here. I was not thinking about 
> > > > > > > problems created
> > > > > > > by an API-call but by the lxc-destroy exectuable. A quick 
> > > > > > > walkthrough: With the
> > > > > > 
> > > > > > D'oh.  Yeah, you'll need to mutex that somehow.
> > > > > 
> > > > > If you want help up-front with the design, please let me know.  If you
> > > > > aren't sure what the current container_disk_lock() and 
> > > > > container_mem_lock()
> > > > > do, please shout.  (they are explained in a LOCKING comment above
> > > > > lxc_container_free() in src/lxc/lxccontainer.c)
> > > > > 
> > > > > The easiest thing to do mght be to disk_lock the container in 
> > > > > lxc_destroy.c,
> > > > > then make the mod_rdep() helper which you use in lxc_destroy.c be a 
> > > > > _locked
> > > > > variant (to avoid deadlock).  So mod_rdep() would turn into something 
> > > > > like:
> > > > > 
> > > > > static bool mod_rdep(struct lxc_container *c0, struct lxc_container 
> > > > > *c, bool inc)
> > > > > {
> > > > >   bool ret;
> > > > >   if (container_disk_lock(c0))
> > > > >   return false;
> > > > >   ret = mod_rdep_locked(c0->name, c0->lxcpath, c->name, 
> > > > > c->lxcpath);
> > > > >   container_disk_unlock(c0);
> > > > >   return ret;
> > > > > }
> > > > > 
> > > > > -serge
> > > > 
> > > > Thanks, this is really nice! One question:
> > > > - I'll take it that we want to make mod_rdep() public. mod_rdep() will 
> > > > be used
> > > >   in lxc_destroy.c, lxccontainer.c and start.c. Problem is that in 
> > > > start.c we do
> > > >   not have a container to pass into mod_rdep(). Do you want me to 
> > > > rewrite
> > > >   mod_rdep() to take in lxcpath and lxcname? If so, could we still use
> > > 
> > > Ok, I'm not running on all cylinders, sorry.
> > > 
> > > You don't want mod_rdep, you want mod_all_rdeps.  So yes export that,
> > > make a struct lxc_container, lock it, and pass that to mod_all_rdeps
> > > which will dothe mod_rdeps for you.
> > 
> > Ok, so in start.c we can actually use lxc_container new. I could modify my:
> 
> Yeah I think that looks good,

Then one final question:
Should we make 

Re: [lxc-devel] [PATCH] Add remove_snapshots_entry() (rebased - v2)

[Christian Brauner]

On Mon, Sep 14, 2015 at 10:54:34PM +, Serge Hallyn wrote:
> Does it help if we simply define c->delete_with_snapshot_clones(), and have
> src/lxc/destroy.c use that?  Then we can contain mod_all_rdeps to being a
> static function in src/lxc/lxccontainer.c  If not, remind me where else we
> would've needed the mod_all_rdeps?

lxc_destroy.c does not call mod_all_rdeps() directly at all. It reads in the
lxc_snapshots file from the container all at once and finds each container
listed in it and passes it to c->destroy(c). So we should be good regarding
locks on this side. Here is the relevant bit from lxc_destroy.c (omitting the
reading in part of the lxc_snapshots file):

while ((lxcpath = strtok_r(!counter ? buf : NULL, "\n", 
))) {
if (!(lxcname = strtok_r(NULL, "\n", )))
break;
c1 = lxc_container_new(lxcname, lxcpath);
if (!c1)
goto next;
if (!c1->destroy(c1)) {
fprintf(stderr, "Destroying snapshot %s of %s 
failed\n", lxcname, my_args.name);
lxc_container_put(c1);
free(buf);
return -1;
}
lxc_container_put(c1);
next:
counter++;
}

What I was worried about is the implementation in start.c:

When we have ephemeral containers that delete themselves we need to make sure
that they update the lxc_snapshots file before they die. One way to achieve this
is to have a mod_all_rdeps() version or something similar in start.c.
Specifically modifying this function to update the lxc_snapshots file:

static void lxc_destroy_container_on_signal(struct lxc_handler *handler,
const char *name)
{
char destroy[MAXPATHLEN];
bool bret = true;
int ret = 0;
struct lxc_container *c;
if (handler->conf && handler->conf->rootfs.path && 
handler->conf->rootfs.mount) {
bret = do_destroy_container(handler->conf);
if (!bret) {
ERROR("Error destroying rootfs for %s", name);
return;
}
}
INFO("Destroyed rootfs for %s", name);

ret = snprintf(destroy, MAXPATHLEN, "%s/%s", handler->lxcpath, 
name);
if (ret < 0 || ret >= MAXPATHLEN) {
ERROR("Error printing path for %s", name);
ERROR("Error destroying directory for %s", name);
return;
}

c = lxc_container_new(name, handler->lxcpath);
if (c) {
if (container_disk_lock(c)) {
INFO("Could not update lxc_snapshots file");
lxc_container_put(c);
} else {
mod_all_rdeps(c, false);
container_disk_unlock(c);
lxc_container_put(c);
}
}

Right here we should update the lxc_snapshots file. Making mod_all_rdeps() is
the option you suggested. We can either have a special function for start.c or
make one of the functions mod_all_rdeps() or mod_rdep() public. Making
mod_rdep() or mod_all_rdeps() public involves binding in the lxccontainer.h file
into place we might not want them. So the easiest thing seems to be to have a
mod_rdep() function specific to start.c which takes in a lxc_container struct
and protects it with container_disk_lock(). Or do you have another idea?

if (am_unpriv())
ret = userns_exec_1(handler->conf, 
lxc_rmdir_onedev_wrapper, destroy);
else
ret = lxc_rmdir_onedev(destroy, NULL);

if (ret < 0) {
ERROR("Error destroying directory for %s", name);
return;
}
INFO("Destroyed directory for %s", name);
}


signature.asc
Description: PGP signature
___
lxc-devel mailing list
lxc-devel@lists.linuxcontainers.org
http://lists.linuxcontainers.org/listinfo/lxc-devel

Re: [lxc-devel] [PATCH] Add remove_snapshots_entry() (rebased - v2)

[Serge Hallyn]

Quoting Serge Hallyn (serge.hal...@ubuntu.com):
> Quoting Christian Brauner (christianvanbrau...@gmail.com):
> > On Mon, Sep 14, 2015 at 10:54:34PM +, Serge Hallyn wrote:
> > > Does it help if we simply define c->delete_with_snapshot_clones(), and 
> > > have
> > > src/lxc/destroy.c use that?  Then we can contain mod_all_rdeps to being a
> > > static function in src/lxc/lxccontainer.c  If not, remind me where else we
> > > would've needed the mod_all_rdeps?
> > 
> > lxc_destroy.c does not call mod_all_rdeps() directly at all. It reads in the
> > lxc_snapshots file from the container all at once and finds each container
> > listed in it and passes it to c->destroy(c). So we should be good regarding
> > locks on this side. Here is the relevant bit from lxc_destroy.c (omitting 
> > the
> > reading in part of the lxc_snapshots file):
> > 
> > while ((lxcpath = strtok_r(!counter ? buf : NULL, "\n", 
> > ))) {
> > if (!(lxcname = strtok_r(NULL, "\n", )))
> > break;
> > c1 = lxc_container_new(lxcname, lxcpath);
> > if (!c1)
> > goto next;
> > if (!c1->destroy(c1)) {
> > fprintf(stderr, "Destroying snapshot %s of %s 
> > failed\n", lxcname, my_args.name);
> > lxc_container_put(c1);
> > free(buf);
> > return -1;
> > }
> > lxc_container_put(c1);
> > next:
> > counter++;
> > }
> > 
> > What I was worried about is the implementation in start.c:
> 
> Oh, right.
> 
> All right let's go back to your original patch.  I'd like to avoid,
> if we end up changing the file format again, having to chase down
> all the places where lxc_depends and rdepends are updated.  So let's
> come up with a small internal api for getting and updating those.
> We don't have to do that immediately, so I'll go ahead and re-review
> your patch (and presumably ack it).

Sigh.  The race.

Yeah, please just add a helper in lxcontainer.h which takes lxcname
and lxcpath and does the mod_all_rdeps.

thanks :)

-serge
___
lxc-devel mailing list
lxc-devel@lists.linuxcontainers.org
http://lists.linuxcontainers.org/listinfo/lxc-devel

Re: [lxc-devel] [PATCH] Add remove_snapshots_entry() (rebased - v2)

[Serge Hallyn]

Quoting Christian Brauner (christianvanbrau...@gmail.com):
> On Mon, Sep 14, 2015 at 10:54:34PM +, Serge Hallyn wrote:
> > Does it help if we simply define c->delete_with_snapshot_clones(), and have
> > src/lxc/destroy.c use that?  Then we can contain mod_all_rdeps to being a
> > static function in src/lxc/lxccontainer.c  If not, remind me where else we
> > would've needed the mod_all_rdeps?
> 
> lxc_destroy.c does not call mod_all_rdeps() directly at all. It reads in the
> lxc_snapshots file from the container all at once and finds each container
> listed in it and passes it to c->destroy(c). So we should be good regarding
> locks on this side. Here is the relevant bit from lxc_destroy.c (omitting the
> reading in part of the lxc_snapshots file):
> 
>   while ((lxcpath = strtok_r(!counter ? buf : NULL, "\n", 
> ))) {
>   if (!(lxcname = strtok_r(NULL, "\n", )))
>   break;
>   c1 = lxc_container_new(lxcname, lxcpath);
>   if (!c1)
>   goto next;
>   if (!c1->destroy(c1)) {
>   fprintf(stderr, "Destroying snapshot %s of %s 
> failed\n", lxcname, my_args.name);
>   lxc_container_put(c1);
>   free(buf);
>   return -1;
>   }
>   lxc_container_put(c1);
> next:
>   counter++;
>   }
> 
> What I was worried about is the implementation in start.c:

Oh, right.

All right let's go back to your original patch.  I'd like to avoid,
if we end up changing the file format again, having to chase down
all the places where lxc_depends and rdepends are updated.  So let's
come up with a small internal api for getting and updating those.
We don't have to do that immediately, so I'll go ahead and re-review
your patch (and presumably ack it).

thanks, Christian.
-serge
___
lxc-devel mailing list
lxc-devel@lists.linuxcontainers.org
http://lists.linuxcontainers.org/listinfo/lxc-devel

Re: [lxc-devel] [PATCH] Add remove_snapshots_entry() (rebased - v2)

[Christian Brauner]

On Mon, Sep 14, 2015 at 07:27:06PM +, Serge Hallyn wrote:
> Quoting Christian Brauner (christianvanbrau...@gmail.com):
> > On Mon, Sep 14, 2015 at 04:33:05PM +, Serge Hallyn wrote:
> > > Quoting Serge Hallyn (serge.hal...@ubuntu.com):
> > > > Quoting Christian Brauner (christianvanbrau...@gmail.com):
> > > > > On Mon, Sep 14, 2015 at 02:50:39PM +, Serge Hallyn wrote:
> > > > > > Quoting Christian Brauner (christianvanbrau...@gmail.com):
> > > > > > > When creating ephemeral containers that have the option 
> > > > > > > lxc.ephemeral = 1 set
> > > > > > > in their config, they will be destroyed on shutdown. As they are 
> > > > > > > simple overlay
> > > > > > > clones of an existing container they should be registered in the 
> > > > > > > lxc_snapshots
> > > > > > > file of the original container to stay consistent and adhere to 
> > > > > > > the
> > > > > > > expectancies of the users. Most of all, it ensure that we cannot 
> > > > > > > remove a
> > > > > > > container that has clones, even if they are just ephemeral 
> > > > > > > snapshot-clones. The
> > > > > > > function adds further consistency because 
> > > > > > > remove_snapshots_entry() ensures that
> > > > > > > ephemeral clone-snapshots deregister themselves from the 
> > > > > > > lxc_snapshots file
> > > > > > > when they are destroyed.
> > > > > > > 
> > > > > > > POSSIBLE GLITCH:
> > > > > > > I was thinking hard about racing conditions and concurrent acces 
> > > > > > > on the
> > > > > > > lxc_snapshots file when lxc-destroy is called on the container 
> > > > > > > while we
> > > > > > > shutdown then container from inside. Here is what my thoughts are 
> > > > > > > so far:
> > > > > > > 
> > > > > > > There should be no racing condition when lxc-destroy including 
> > > > > > > all snapshots is
> > > > > > 
> > > > > > Note that lxcapi_destroy_with_snapshots() deletes the *snapshots*, 
> > > > > > not the
> > > > > > snapshot clones.  This is an unfortunate naming clash (which we 
> > > > > > could try
> > > > > > to correct henceforth but we need good names :), but they are 
> > > > > > different.
> > > > > > So anything under /var/lib/lxc/$container/snaps will be deleted.  
> > > > > > But if
> > > > > > you've created an overlayfs clone, then containers listed in
> > > > > > /var/lib/lxc/$container/lxc_snapshots will not be deleted. There is 
> > > > > > no
> > > > > > API call or program to automatically deleted those right now.
> > > > > 
> > > > > I think you are partially wrong here. I was not thinking about 
> > > > > problems created
> > > > > by an API-call but by the lxc-destroy exectuable. A quick 
> > > > > walkthrough: With the
> > > > 
> > > > D'oh.  Yeah, you'll need to mutex that somehow.
> > > 
> > > If you want help up-front with the design, please let me know.  If you
> > > aren't sure what the current container_disk_lock() and 
> > > container_mem_lock()
> > > do, please shout.  (they are explained in a LOCKING comment above
> > > lxc_container_free() in src/lxc/lxccontainer.c)
> > > 
> > > The easiest thing to do mght be to disk_lock the container in 
> > > lxc_destroy.c,
> > > then make the mod_rdep() helper which you use in lxc_destroy.c be a 
> > > _locked
> > > variant (to avoid deadlock).  So mod_rdep() would turn into something 
> > > like:
> > > 
> > > static bool mod_rdep(struct lxc_container *c0, struct lxc_container *c, 
> > > bool inc)
> > > {
> > >   bool ret;
> > >   if (container_disk_lock(c0))
> > >   return false;
> > >   ret = mod_rdep_locked(c0->name, c0->lxcpath, c->name, c->lxcpath);
> > >   container_disk_unlock(c0);
> > >   return ret;
> > > }
> > > 
> > > -serge
> > 
> > Thanks, this is really nice! One question:
> > - I'll take it that we want to make mod_rdep() public. mod_rdep() will be 
> > used
> >   in lxc_destroy.c, lxccontainer.c and start.c. Problem is that in start.c 
> > we do
> >   not have a container to pass into mod_rdep(). Do you want me to rewrite
> >   mod_rdep() to take in lxcpath and lxcname? If so, could we still use
> 
> Ok, I'm not running on all cylinders, sorry.
> 
> You don't want mod_rdep, you want mod_all_rdeps.  So yes export that,
> make a struct lxc_container, lock it, and pass that to mod_all_rdeps
> which will dothe mod_rdeps for you.

Ha, no worries. :) Ok, just to be clear, where do we fit mod_all_rdeps() in with
start.c? We do not have a struct lxc_container in start.c only struct
lxc_handler. I assume there must be some reason for this.

> 
> >   disk_lock() and mem_lock by e.g. calling lxc_container_new(lxcname, 
> > lxcpath)
> >   and then calling disk_lock() or mem_lock() after to protect the container?
> 
> 


signature.asc
Description: PGP signature
___
lxc-devel mailing list
lxc-devel@lists.linuxcontainers.org
http://lists.linuxcontainers.org/listinfo/lxc-devel

Re: [lxc-devel] [PATCH] Add remove_snapshots_entry() (rebased - v2)

[Christian Brauner]

On Mon, Sep 14, 2015 at 04:33:05PM +, Serge Hallyn wrote:
> Quoting Serge Hallyn (serge.hal...@ubuntu.com):
> > Quoting Christian Brauner (christianvanbrau...@gmail.com):
> > > On Mon, Sep 14, 2015 at 02:50:39PM +, Serge Hallyn wrote:
> > > > Quoting Christian Brauner (christianvanbrau...@gmail.com):
> > > > > When creating ephemeral containers that have the option lxc.ephemeral 
> > > > > = 1 set
> > > > > in their config, they will be destroyed on shutdown. As they are 
> > > > > simple overlay
> > > > > clones of an existing container they should be registered in the 
> > > > > lxc_snapshots
> > > > > file of the original container to stay consistent and adhere to the
> > > > > expectancies of the users. Most of all, it ensure that we cannot 
> > > > > remove a
> > > > > container that has clones, even if they are just ephemeral 
> > > > > snapshot-clones. The
> > > > > function adds further consistency because remove_snapshots_entry() 
> > > > > ensures that
> > > > > ephemeral clone-snapshots deregister themselves from the 
> > > > > lxc_snapshots file
> > > > > when they are destroyed.
> > > > > 
> > > > > POSSIBLE GLITCH:
> > > > > I was thinking hard about racing conditions and concurrent acces on 
> > > > > the
> > > > > lxc_snapshots file when lxc-destroy is called on the container while 
> > > > > we
> > > > > shutdown then container from inside. Here is what my thoughts are so 
> > > > > far:
> > > > > 
> > > > > There should be no racing condition when lxc-destroy including all 
> > > > > snapshots is
> > > > 
> > > > Note that lxcapi_destroy_with_snapshots() deletes the *snapshots*, not 
> > > > the
> > > > snapshot clones.  This is an unfortunate naming clash (which we could 
> > > > try
> > > > to correct henceforth but we need good names :), but they are different.
> > > > So anything under /var/lib/lxc/$container/snaps will be deleted.  But if
> > > > you've created an overlayfs clone, then containers listed in
> > > > /var/lib/lxc/$container/lxc_snapshots will not be deleted. There is no
> > > > API call or program to automatically deleted those right now.
> > > 
> > > I think you are partially wrong here. I was not thinking about problems 
> > > created
> > > by an API-call but by the lxc-destroy exectuable. A quick walkthrough: 
> > > With the
> > 
> > D'oh.  Yeah, you'll need to mutex that somehow.
> 
> If you want help up-front with the design, please let me know.  If you
> aren't sure what the current container_disk_lock() and container_mem_lock()
> do, please shout.  (they are explained in a LOCKING comment above
> lxc_container_free() in src/lxc/lxccontainer.c)
> 
> The easiest thing to do mght be to disk_lock the container in lxc_destroy.c,
> then make the mod_rdep() helper which you use in lxc_destroy.c be a _locked
> variant (to avoid deadlock).  So mod_rdep() would turn into something like:
> 
> static bool mod_rdep(struct lxc_container *c0, struct lxc_container *c, bool 
> inc)
> {
>   bool ret;
>   if (container_disk_lock(c0))
>   return false;
>   ret = mod_rdep_locked(c0->name, c0->lxcpath, c->name, c->lxcpath);
>   container_disk_unlock(c0);
>   return ret;
> }
> 
> -serge

Thanks, this is really nice! One question:
- I'll take it that we want to make mod_rdep() public. mod_rdep() will be used
  in lxc_destroy.c, lxccontainer.c and start.c. Problem is that in start.c we do
  not have a container to pass into mod_rdep(). Do you want me to rewrite
  mod_rdep() to take in lxcpath and lxcname? If so, could we still use
  disk_lock() and mem_lock by e.g. calling lxc_container_new(lxcname, lxcpath)
  and then calling disk_lock() or mem_lock() after to protect the container?



signature.asc
Description: PGP signature
___
lxc-devel mailing list
lxc-devel@lists.linuxcontainers.org
http://lists.linuxcontainers.org/listinfo/lxc-devel

Re: [lxc-devel] [PATCH] Add remove_snapshots_entry() (rebased - v2)

[Serge Hallyn]

Quoting Christian Brauner (christianvanbrau...@gmail.com):
> On Mon, Sep 14, 2015 at 04:33:05PM +, Serge Hallyn wrote:
> > Quoting Serge Hallyn (serge.hal...@ubuntu.com):
> > > Quoting Christian Brauner (christianvanbrau...@gmail.com):
> > > > On Mon, Sep 14, 2015 at 02:50:39PM +, Serge Hallyn wrote:
> > > > > Quoting Christian Brauner (christianvanbrau...@gmail.com):
> > > > > > When creating ephemeral containers that have the option 
> > > > > > lxc.ephemeral = 1 set
> > > > > > in their config, they will be destroyed on shutdown. As they are 
> > > > > > simple overlay
> > > > > > clones of an existing container they should be registered in the 
> > > > > > lxc_snapshots
> > > > > > file of the original container to stay consistent and adhere to the
> > > > > > expectancies of the users. Most of all, it ensure that we cannot 
> > > > > > remove a
> > > > > > container that has clones, even if they are just ephemeral 
> > > > > > snapshot-clones. The
> > > > > > function adds further consistency because remove_snapshots_entry() 
> > > > > > ensures that
> > > > > > ephemeral clone-snapshots deregister themselves from the 
> > > > > > lxc_snapshots file
> > > > > > when they are destroyed.
> > > > > > 
> > > > > > POSSIBLE GLITCH:
> > > > > > I was thinking hard about racing conditions and concurrent acces on 
> > > > > > the
> > > > > > lxc_snapshots file when lxc-destroy is called on the container 
> > > > > > while we
> > > > > > shutdown then container from inside. Here is what my thoughts are 
> > > > > > so far:
> > > > > > 
> > > > > > There should be no racing condition when lxc-destroy including all 
> > > > > > snapshots is
> > > > > 
> > > > > Note that lxcapi_destroy_with_snapshots() deletes the *snapshots*, 
> > > > > not the
> > > > > snapshot clones.  This is an unfortunate naming clash (which we could 
> > > > > try
> > > > > to correct henceforth but we need good names :), but they are 
> > > > > different.
> > > > > So anything under /var/lib/lxc/$container/snaps will be deleted.  But 
> > > > > if
> > > > > you've created an overlayfs clone, then containers listed in
> > > > > /var/lib/lxc/$container/lxc_snapshots will not be deleted. There is no
> > > > > API call or program to automatically deleted those right now.
> > > > 
> > > > I think you are partially wrong here. I was not thinking about problems 
> > > > created
> > > > by an API-call but by the lxc-destroy exectuable. A quick walkthrough: 
> > > > With the
> > > 
> > > D'oh.  Yeah, you'll need to mutex that somehow.
> > 
> > If you want help up-front with the design, please let me know.  If you
> > aren't sure what the current container_disk_lock() and container_mem_lock()
> > do, please shout.  (they are explained in a LOCKING comment above
> > lxc_container_free() in src/lxc/lxccontainer.c)
> > 
> > The easiest thing to do mght be to disk_lock the container in lxc_destroy.c,
> > then make the mod_rdep() helper which you use in lxc_destroy.c be a _locked
> > variant (to avoid deadlock).  So mod_rdep() would turn into something like:
> > 
> > static bool mod_rdep(struct lxc_container *c0, struct lxc_container *c, 
> > bool inc)
> > {
> > bool ret;
> > if (container_disk_lock(c0))
> > return false;
> > ret = mod_rdep_locked(c0->name, c0->lxcpath, c->name, c->lxcpath);
> > container_disk_unlock(c0);
> > return ret;
> > }
> > 
> > -serge
> 
> Thanks, this is really nice! One question:
> - I'll take it that we want to make mod_rdep() public. mod_rdep() will be used
>   in lxc_destroy.c, lxccontainer.c and start.c. Problem is that in start.c we 
> do
>   not have a container to pass into mod_rdep(). Do you want me to rewrite
>   mod_rdep() to take in lxcpath and lxcname? If so, could we still use

Ok, I'm not running on all cylinders, sorry.

You don't want mod_rdep, you want mod_all_rdeps.  So yes export that,
make a struct lxc_container, lock it, and pass that to mod_all_rdeps
which will dothe mod_rdeps for you.

>   disk_lock() and mem_lock by e.g. calling lxc_container_new(lxcname, lxcpath)
>   and then calling disk_lock() or mem_lock() after to protect the container?


___
lxc-devel mailing list
lxc-devel@lists.linuxcontainers.org
http://lists.linuxcontainers.org/listinfo/lxc-devel

[lxc-devel] [PATCH] Add remove_snapshots_entry()

[Christian Brauner]

When creating ephemeral containers that have the option lxc.ephemeral = 1 set
in their config, they will be destroyed on shutdown. As they are simple overlay
clones of an existing container they should be registered in the lxc_snapshots
file of the original container to stay consistent and adhere to the
expectancies of the users. Most of all, it ensure that we cannot remove a
container that has clones, even if they are just ephemeral snapshot-clones. The
function adds further consistency because remove_snapshots_entry() ensures that
ephemeral clone-snapshots deregister themselves from the lxc_snapshots file
when they are destroyed.

POSSIBLE GLITCH:
I was thinking hard about racing conditions and concurrent acces on the
lxc_snapshots file when lxc-destroy is called on the container while we
shutdown then container from inside. Here is what my thoughts are so far:

There should be no racing condition when lxc-destroy including all snapshots is
called and the container in question is running, lxc-destroy will simply fail
with the warning that the clone is still running but lxc-destroy won't touch
the lxc_snapshots file. The offending container will then exit and delete the
container entry. lxc-destroy can then be called again and will delete the
remaining containers.

The strange case seems to be when we create another clone-snapshot while
another one shuts down. Does someone have any arguments against this way of
implementing it? Do we expect trouble? Do we need flocks in start.c and
lxccontainer.c?

(Another step for providing a consistent rewrite of lxc-clone +
lxc-start-ephemeral.)

Christian Brauner (1):
  Add remove_snapshots_entry()

 src/lxc/start.c | 43 ---
 1 file changed, 20 insertions(+), 23 deletions(-)

-- 
2.5.1

___
lxc-devel mailing list
lxc-devel@lists.linuxcontainers.org
http://lists.linuxcontainers.org/listinfo/lxc-devel

[lxc-devel] [PATCH] Add remove_snapshots_entry()

[Christian Brauner]

When creating ephemeral containers that have the option lxc.ephemeral = 1 set
in their config, they will be destroyed on shutdown. As they are simple overlay
clones of an existing container they should be registered in the lxc_snapshots
file of the original container to stay consistent and adhere to the
expectancies of the users. Most of all, it ensure that we cannot remove a
container that has clones, even if they are just ephemeral snapshot-clones. The
function adds further consistency because remove_snapshots_entry() ensures that
ephemeral clone-snapshots deregister themselves from the lxc_snapshots file
when they are destroyed.

Signed-off-by: Christian Brauner 
---
 src/lxc/start.c | 43 ---
 1 file changed, 20 insertions(+), 23 deletions(-)

diff --git a/src/lxc/start.c b/src/lxc/start.c
index 93c039c..f58d9a4 100644
--- a/src/lxc/start.c
+++ b/src/lxc/start.c
@@ -84,7 +84,7 @@ const struct ns_info ns_info[LXC_NS_MAX] = {
[LXC_NS_NET] = {"net", CLONE_NEWNET}
 };
 
-static void update_clone_snapshots(struct lxc_handler *handler, const char 
*name);
+static void remove_snapshot_entry(struct lxc_handler *handler, const char 
*name);
 static bool do_destroy_container(struct lxc_conf *conf);
 static int lxc_rmdir_onedev_wrapper(void *data);
 static void lxc_destroy_container_on_signal(struct lxc_handler *handler,
@@ -1324,7 +1324,7 @@ static void lxc_destroy_container_on_signal(struct 
lxc_handler *handler,
return;
}
 
-   update_clone_snapshots(handler, name);
+   remove_snapshot_entry(handler, name);
 
if (am_unpriv())
ret = userns_exec_1(handler->conf, lxc_rmdir_onedev_wrapper, 
destroy);
@@ -1353,7 +1353,7 @@ static bool do_destroy_container(struct lxc_conf *conf) {
 return bdev_destroy(conf);
 }
 
-static void update_clone_snapshots(struct lxc_handler *handler, const char 
*name)
+static void remove_snapshot_entry(struct lxc_handler *handler, const char 
*name)
 {
char rdepfile[MAXPATHLEN];
char path[MAXPATHLEN];
@@ -1365,9 +1365,9 @@ static void update_clone_snapshots(struct lxc_handler 
*handler, const char *name
char *origpath = NULL;
char *origname = NULL;
int ret = 0;
+   int bytes = 0;
int fd = -1;
size_t len = 0;
-   size_t difflen = 0;
struct stat fbuf;
 
ret = snprintf(rdepfile, MAXPATHLEN, "%s/%s/lxc_rdepends", 
handler->lxcpath, name);
@@ -1435,7 +1435,13 @@ static void update_clone_snapshots(struct lxc_handler 
*handler, const char *name
}
 
if (fbuf.st_size != 0) {
-   buf = mmap(NULL, fbuf.st_size, PROT_READ | PROT_WRITE, 
MAP_SHARED, fd, 0);
+   /* write terminating \0-byte to file */
+   if (pwrite(fd, "", 1, fbuf.st_size) <= 0) {
+   close(fd);
+   return;
+   }
+
+   buf = mmap(NULL, fbuf.st_size + 1, PROT_READ | PROT_WRITE, 
MAP_SHARED, fd, 0);
if (buf == MAP_FAILED) {
SYSERROR("Failed to create mapping %s", path);
close(fd);
@@ -1443,25 +1449,16 @@ static void update_clone_snapshots(struct lxc_handler 
*handler, const char *name
}
 
len = strlen(newpath);
+   while ((del = strstr((char *)buf, newpath))) {
+   memmove(del, del + len, strlen(del) - len + 1);
+   bytes += len;
+   }
 
-   /* mmap()ed memory is only \0-terminated when it is not
-* a multiple of a pagesize. Hence, we'll use memmem(). */
-   if ((del = memmem(buf, fbuf.st_size, newpath, len))) {
-   /* remove container entry */
-   if (del != buf + fbuf.st_size - len) {
-   difflen = fbuf.st_size - (del - buf);
-   memmove(del, del + len, strnlen(del, difflen) - 
len);
-   }
-
-   munmap(buf, fbuf.st_size);
-
-   if (ftruncate(fd, fbuf.st_size - len) < 0) {
-   SYSERROR("Failed to truncate file %s", path);
-   close(fd);
-   return;
-   }
-   } else {
-   munmap(buf, fbuf.st_size);
+   munmap(buf, fbuf.st_size + 1);
+   if (ftruncate(fd, fbuf.st_size - bytes) < 0) {
+   SYSERROR("Failed to truncate file %s", path);
+   close(fd);
+   return;
}
}
 
-- 
2.5.1

___
lxc-devel mailing list
lxc-devel@lists.linuxcontainers.org
http://lists.linuxcontainers.org/listinfo/lxc-devel

[lxc-devel] [PATCH] Add remove_snapshots_entry() (rebased - v2)

[Christian Brauner]

When creating ephemeral containers that have the option lxc.ephemeral = 1 set
in their config, they will be destroyed on shutdown. As they are simple overlay
clones of an existing container they should be registered in the lxc_snapshots
file of the original container to stay consistent and adhere to the
expectancies of the users. Most of all, it ensure that we cannot remove a
container that has clones, even if they are just ephemeral snapshot-clones. The
function adds further consistency because remove_snapshots_entry() ensures that
ephemeral clone-snapshots deregister themselves from the lxc_snapshots file
when they are destroyed.

POSSIBLE GLITCH:
I was thinking hard about racing conditions and concurrent acces on the
lxc_snapshots file when lxc-destroy is called on the container while we
shutdown then container from inside. Here is what my thoughts are so far:

There should be no racing condition when lxc-destroy including all snapshots is
called and the container in question is running, lxc-destroy will simply fail
with the warning that the clone is still running but lxc-destroy won't touch
the lxc_snapshots file. The offending container will then exit and delete the
container entry. lxc-destroy can then be called again and will delete the
remaining containers.

The strange case seems to be when we create another clone-snapshot while
another one shuts down. Does someone have any arguments against this way of
implementing it? Do we expect trouble? Do we need flocks in start.c and
lxccontainer.c?

Christian Brauner (1):
  Add remove_snapshots_entry()

 src/lxc/start.c | 123 
 1 file changed, 123 insertions(+)

-- 
2.5.1

___
lxc-devel mailing list
lxc-devel@lists.linuxcontainers.org
http://lists.linuxcontainers.org/listinfo/lxc-devel

[lxc-devel] [PATCH] Add remove_snapshots_entry() (rebased - v2)

[Christian Brauner]

When creating ephemeral containers that have the option lxc.ephemeral = 1 set
in their config, they will be destroyed on shutdown. As they are simple overlay
clones of an existing container they should be registered in the lxc_snapshots
file of the original container to stay consistent and adhere to the
expectancies of the users. Most of all, it ensure that we cannot remove a
container that has clones, even if they are just ephemeral snapshot-clones. The
function adds further consistency because remove_snapshots_entry() ensures that
ephemeral clone-snapshots deregister themselves from the lxc_snapshots file
when they are destroyed.

Signed-off-by: Christian Brauner 
---
 src/lxc/start.c | 123 
 1 file changed, 123 insertions(+)

diff --git a/src/lxc/start.c b/src/lxc/start.c
index 8fe08a1..f58d9a4 100644
--- a/src/lxc/start.c
+++ b/src/lxc/start.c
@@ -33,6 +33,7 @@
 #include 
 #include 
 #include 
+#include 
 #include 
 #include 
 #include 
@@ -83,6 +84,7 @@ const struct ns_info ns_info[LXC_NS_MAX] = {
[LXC_NS_NET] = {"net", CLONE_NEWNET}
 };
 
+static void remove_snapshot_entry(struct lxc_handler *handler, const char 
*name);
 static bool do_destroy_container(struct lxc_conf *conf);
 static int lxc_rmdir_onedev_wrapper(void *data);
 static void lxc_destroy_container_on_signal(struct lxc_handler *handler,
@@ -1322,6 +1324,8 @@ static void lxc_destroy_container_on_signal(struct 
lxc_handler *handler,
return;
}
 
+   remove_snapshot_entry(handler, name);
+
if (am_unpriv())
ret = userns_exec_1(handler->conf, lxc_rmdir_onedev_wrapper, 
destroy);
else
@@ -1349,3 +1353,122 @@ static bool do_destroy_container(struct lxc_conf *conf) 
{
 return bdev_destroy(conf);
 }
 
+static void remove_snapshot_entry(struct lxc_handler *handler, const char 
*name)
+{
+   char rdepfile[MAXPATHLEN];
+   char path[MAXPATHLEN];
+   char newpath[MAXPATHLEN];
+   char *base = NULL;
+   char *buf = NULL;
+   char *del = NULL;
+   char *scratch = NULL;
+   char *origpath = NULL;
+   char *origname = NULL;
+   int ret = 0;
+   int bytes = 0;
+   int fd = -1;
+   size_t len = 0;
+   struct stat fbuf;
+
+   ret = snprintf(rdepfile, MAXPATHLEN, "%s/%s/lxc_rdepends", 
handler->lxcpath, name);
+   if (ret < 0 || ret >= MAXPATHLEN)
+   return;
+
+   fd = open(rdepfile, O_RDONLY | O_CLOEXEC);
+   if (fd < 0)
+   return;
+
+   ret = fstat(fd, );
+   if (ret < 0) {
+   close(fd);
+   return;
+   }
+
+   base = calloc(fbuf.st_size + 1, sizeof(char));
+   if (!base) {
+   close(fd);
+   return;
+   }
+
+   ret = read(fd, (void *)base, fbuf.st_size);
+   if (ret <= 0) {
+   free(base);
+   close(fd);
+   return;
+   }
+
+   origpath = strtok_r(base, "\n", );
+   if (!origpath) {
+   free(base);
+   close(fd);
+   return;
+   }
+
+   origname = strtok_r(NULL, "\n", );
+   if (!origname) {
+   free(base);
+   close(fd);
+   return;
+   }
+
+   close(fd);
+
+   ret = snprintf(path, MAXPATHLEN, "%s/%s/lxc_snapshots", origpath, 
origname);
+   if (ret < 0 || ret >= MAXPATHLEN) {
+   free(base);
+   return;
+   }
+   free(base);
+
+   ret = snprintf(newpath, MAXPATHLEN, "%s\n%s\n", handler->lxcpath, name);
+   if (ret < 0 || ret >= MAXPATHLEN)
+   return;
+
+   fd = open(path, O_RDWR | O_CLOEXEC);
+   if (fd < 0)
+   return;
+
+   ret = fstat(fd, );
+   if (ret < 0) {
+   close(fd);
+   return;
+   }
+
+   if (fbuf.st_size != 0) {
+   /* write terminating \0-byte to file */
+   if (pwrite(fd, "", 1, fbuf.st_size) <= 0) {
+   close(fd);
+   return;
+   }
+
+   buf = mmap(NULL, fbuf.st_size + 1, PROT_READ | PROT_WRITE, 
MAP_SHARED, fd, 0);
+   if (buf == MAP_FAILED) {
+   SYSERROR("Failed to create mapping %s", path);
+   close(fd);
+   return;
+   }
+
+   len = strlen(newpath);
+   while ((del = strstr((char *)buf, newpath))) {
+   memmove(del, del + len, strlen(del) - len + 1);
+   bytes += len;
+   }
+
+   munmap(buf, fbuf.st_size + 1);
+   if (ftruncate(fd, fbuf.st_size - bytes) < 0) {
+   SYSERROR("Failed to truncate file %s", path);
+   close(fd);
+   return;
+   }
+   }
+
+   close(fd);
+
+   /* If the