On 12/12/14 14:55, Richard Maw wrote:
On Fri, Dec 12, 2014 at 02:22:21PM +0000, Sam Thursfield wrote:
> On 12/12/14 14:14, Richard Maw wrote:
>> On Fri, Dec 12, 2014 at 02:08:35PM +0000, Sam Thursfield wrote:
>>> On 12/12/14 13:46, Richard Maw wrote:
>>>> morphlib/stagingarea.py | 7 ++++++-
>>>> 1 file changed, 6 insertions(+), 1 deletion(-)
>>>> diff --git a/morphlib/stagingarea.py b/morphlib/stagingarea.py
>>>> index b676d4d..5810bf3 100644
>>>> --- a/morphlib/stagingarea.py
>>>> +++ b/morphlib/stagingarea.py
>>>> @@ -274,9 +274,14 @@ class StagingArea(object):
>>>> binds = ()
>>>> + if self.use_chroot:
>>>> + mounts = self.to_mount
>>>> + else:
>>>> + mounts = [(os.path.join(self.dirname, target), type,
>>>> + for target, type, source in self.to_mount]
>>>> cmdline = morphlib.util.containerised_cmdline(
>>>> argv, cwd=kwargs.pop('cwd', '/'),
>>>> - root=chroot_dir, mounts=self.to_mount,
>>>> + root=chroot_dir, mounts=mounts,
>>>> binds=binds, mount_proc=mount_proc,
>>> Seems sane, but can you remind me why we need to mount /dev/shm at
>>> all if we're building in bootstrap mode (where 'use_chroot' is
>>> false) ? Seems that builds would use the host /dev/shm anyway rather
>>> than looking for /tmp/staging/dev/shm ...
>> Strictly we don't need /dev/shm, so we could set mounts to `()`, but
>> I wanted to fix up the logic so mounting stuff inside the staging area
>> works in general, in case we need to expand it later.
> OK. +1 to merge this, but might be nice if you could also add a
> comment somewhere noting that the /dev/shm mount is redundant in the
> 'use_chroot=False' case and we should stop Morph from doing that.
Given how trivial it would be to prevent it using /dev/shm, I don't think
there's much value in commenting that we should stop it doing so.
I don't think just setting the list of things to mount to be empty is a good
idea either though, as we would lose the awareness that extra handling is
required in bootstrap mode, which would cause headaches if we later needed to
add mounts in bootstrap mode.
So to handle that, I think we should split to_mount up into what it needs for
staging mode and what it needs for bootstrap mode like this:
to_mount_in_staging = (('/dev/shm', 'tmpfs', 'none'),)
to_mount_in_bootstrap = ()
mounts = self.to_mount_in_staging
mounts = [(os.path.join(self.dirname, target), type, source)
for target, type, source in self.to_mount_in_bootstrap]
Unfortunately, I won't have time to rework the patch for a while, so I'd be
happy if someone else were to pick this up.
I've made this change in a separate commit and merged the change to
master of morph.git. Thanks for the patch, and sorry for the delay in
getting around to merging it.
Sam Thursfield, Codethink Ltd.
Office telephone: +44 161 236 5575