--- 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): else: binds = () + if self.use_chroot: + mounts = self.to_mount + else: + mounts = [(os.path.join(self.dirname, target), type, source) + 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, writable_paths=do_not_mount_dirs) try:

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): >> else: >> binds = () >> >> + if self.use_chroot: >> + mounts = self.to_mount >> + else: >> + mounts = [(os.path.join(self.dirname, target), type, source) >> + 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, >> writable_paths=do_not_mount_dirs) >> try: > > > 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.

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): >>>> else: >>>> binds = () >>>> >>>> + if self.use_chroot: >>>> + mounts = self.to_mount >>>> + else: >>>> + mounts = [(os.path.join(self.dirname, target), type, source) >>>> + 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, >>>> writable_paths=do_not_mount_dirs) >>>> try: >>> >>> >>> 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 = () def runcmd(...): ... if self.use_chroot: mounts = self.to_mount_in_staging else: 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.