[buildd-tools-devel] Filesystem union v3

[Roger Leigh]

On Wed, Jul 01, 2009 at 05:57:14PM +0200, Jan-Marek Glogowski wrote:
> On Wed, 1 Jul 2009, Roger Leigh wrote:
> 
> > What is the pipe used for in sbuild-session; it's not clear to me what
> > the purpose is.
> 
> As all setup script are started in a sub-process, I'm using the pipe to
> send the failing script name from this process, so on cleanup / stop it
> just runs the executed ones.

What is the purpose of the sub-process?  I'm not sure I see what
additional benefit it provides compared with just forking the
child processes off the main process, which would save the need for
the pipe since the parent would already know which script failed.

> For the original patches [02-08] that's true. All the following patches do
> the same as the new patches in master and the fs-union branch. Just real
> chroots inherit from sbuild::chroot.

OK, that's great.

> > * I've fixed up the session flag and environment usage.
> >
> > * In order to support unions on block-devices, this necessitated
> >   splitting block-device into a base class from which both
> >   block-device and lvm-snapshot could inherit, so lvm-snapshot
> >   doesn't get affected at all by union stuff.
> >
> > * Union filesystem support is configurable with SBUILD_FEATURE_UNION.
> >
> > So, what needs doing now:
> >
> > * I missed out the chroot_loopback testcase you added, so this
> >   still needs adding.

This is now done.

> > * I missed out UUID support and the setup script changes for that.
> >   I'm not sure what the purpose of this is, and why we need it.
> 
> This isn't needed anymore. Every session now has an own underlay
> directory. I used the UUID to create a common underlay directory for
> loopback chroots in mounts/.

OK, I'll not apply the UUID-related changes in this case.

> > * I also missed the run_parts changes; these can also be added.
> 
> From my FSU v3 mail:
> 
> > The current code reruns the failing script. The fs-union code doesn't
> > depend on this feature, as this is just a fix for the problem with
> > loopback chroots without fs-union, where the stop-skript 15killprocs
> > killed the running chroot when starting a second session, which failed
> > and killed the first session.

This is somewhere where we can improve on our use of locking.
We could use a mechanism such as POSIX semaphores to prevent cleanup
when it is already in use.

In the long term, I'd like to look at making schroot use a
client-server architecture so that chroot access is centrally
managed, and these issues can be solved cleanly.

> > * The patches you just posted very likely contain bits I didn't
> >   merge due to either just missing them by accident or
> >   deliberately choosing not to merge them (such as the UUID
> >   stuff).  It would be great if you could go through and see
> >   if we are missing anything that still needs adding.
> 
> There are some additional cleanups in my FSU v3 patchset:
> 
> - Remove "plain" from all scripts and abort 00check, just in case
>   someone mixes old schroot with new scripts

Done.

> - Install to debian/tmp to make dh_prep work correctly

Not done.  Simply removing debian/install after dh_prep is
sufficient.  debian/tmp is an optional directory, and we don't
make use of it (it has historical uses we don't want).

> - Remove the backup header template produced by bootstrap, when run twice

Done.

> - The loopback chroot test, but without the UUID stuff

Done.

> - If accepted as a fix / workaround for the loopback without fs-union
>   problem the run-parts patch

I'll probably merge it, but I would like to get what's merged
to work correctly first, and then we can merge the missing bits.
As I mentioned above, I'm not sure I understand the need for
running in a separate process, so I'd like to know about that
first.

> - Reusing loopback devices for the same chroots - not sure if there is a
>   limit, but reusing is simple

Which bit of the patch is this one?  Are there any race conditions
when it comes to freeing the loopback device?

> - Protecting set_run_setup_scripts

Not applied, but that's just an oversight.  I'll fix that now.

> - The private member chroot::session_id is never set, but used for
>   get_keyfile_name() - one reason, why session files currently fail. Why
>   are there two session_ids in chroot and chroot_session, anyway?

This was a bug.  Originally, the session ID was only set in
chroot_session, and never in the chroot.  However, because the chroot
needs to know the session ID, I've recently moved it into the chroot.

Long term, I would like to move the functionality of chroot_session
into chroot.  This is needed in order to support non-chroot
virtualisation such as KVM/Xen where a mechanism other than
chroot()/fork()/exec() is needed to run commands inside the
virtual environment.

> I just built and tested master with test loopback union.
> 
> chroot -b -c loop runs the scripts, but doesn't create the session file at
> all; /var/lib/schroot/session is empty.

This was due to missing the session file creation in setup_lock;
that's now fixed.

If you take a look at the current git, it should have most things
now merged, with a few exceptions.  I'd like to get the merged
changes stabilised and tested before making any further major
changes.


Currently, the testsuite fails due to some issue with apparent
memory corruption.  I've attached the diagnostic patch I'm using
to track down the cause, but I'm rather confused at the moment:

(gdb) frame 3
#3  0x0000000000457a82 in sbuild::chroot_source::get_session_flags (
    this=0xaecf50) at sbuild-chroot-source.cc:142
142       assert(this->get_source() == false);
(gdb) print this->is_source
$9 = false
(gdb) print this->get_source()
$10 = 96

Since get_source() just returns this->is_source, there's something
bizarre going on which causes the tests to fail since it thinks
it's a source chroot when it's not, so the session flags are all
screwed up.

The this pointer looks valid, and the member looks OK, so this
is a bit odd!

valgrind warns about uninitialised variables, but gcc does not,
and it's not clear what/where the variables are it's warning about!

If anyone has any ideas, please shout!


Regards,
Roger

-- 
  .''`.  Roger Leigh
 : :' :  Debian GNU/Linux             http://people.debian.org/~rleigh/
 `. `'   Printing on GNU/Linux?       http://gutenprint.sourceforge.net/
   `-    GPG Public Key: 0x25BFB848   Please GPG sign your mail.