-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/31444/#review77074
-----------------------------------------------------------
Take this all with plenty of salt since I really haven't figured out a lot of
Mesos yet ;)
In changeRoot(), do you really need to do a full os::user(), or
would it be sufficient to test os::getuid() == 0?
I think that it would be more consistent to put the Linux-specific
chrooting code in src/linux/chroot.cpp. This would let you remove
all the __linux__ code paths from containerizer/mesos/launch.cpp.
If the --chroot option was specified on an unsupported platform,
this would fail at runtime. I'm not sure what the Mesos policy is
on this kind of thing, but my preference is to keep a consistent
set of options across platforms.
I like what you have done with the mount table in mountAll(). I
think it would be useful to do the same thing with symlinks, breaking
the chroot construction process into:
1) mount filesystems
2) create device nodes
3) apply symlink fixups
I noticed that you are mounting /run, which seems like a pretty
recent distribution change. It might make sense to symlink /var/run
to that. In general, I expect that there could be many reasonable
ways to construct the chroot, so it's probably worth starting to
plan for operators to be able to extend how this is done. Basically,
we ought to plan for separating the construction of the chroot from
the act of entering the chroot.
Should you set the size of the tmpfs mounts rather than relying on
the default?
Am I right in believing that there's no need to unmount anything
when mountAll() fails because we are guaranteed to be running in a
separate mount namespace at this point? I don't see that the Linux
launcher actually *guarantees* to pass CLONE_NEWNS. I guess
that's why you are attempting to check this.
It would be helpful to have a more detailed explanation of why the
pivot_root, chroot sequence is necessary. Is this related to the
manual page warning that "depending on the implementation of
pivot_root, root and cwd of the caller may or may not change"?
Though you added documentation for the --chroot option, there doesn't
seem to be a way to get a usage message from mesos-containerizer.
Are you planning to make a corresponding documentation change?
It's not at all clear to me how to actually use the --chroot option,
though that's probably because I'm a Mesos noob.
- James Peach
On March 17, 2015, 10:44 p.m., Ian Downes wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/31444/
> -----------------------------------------------------------
>
> (Updated March 17, 2015, 10:44 p.m.)
>
>
> Review request for mesos, Chi Zhang, Dominic Hamon, Jay Buffington, Jie Yu,
> and James Peach.
>
>
> Bugs: MESOS-2350
> https://issues.apache.org/jira/browse/MESOS-2350
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Optionally take a path that the launch helper should chroot to before
> exec'ing the executor. It is assumed that the work directory is mounted to
> the appropriate location under the chroot. In particular, the path to the
> executor must be relative to the chroot.
>
> Configuration that should be private to the chroot is done during the launch,
> e.g. mounting proc and statically configuring basic devices. It is assumed
> that other configuration, e.g., preparing the image, mounting in volumes or
> persistent resources, is done by the caller.
>
> Mounts can be made to the chroot (e.g., updating the volumes or persistent
> resources) and they will propagate in to the container but mounts made inside
> the container will not propagate out to the host.
>
> It currently assumes that at least {{chroot}}/tmp is writeable and that mount
> points {{chroot}}/{tmp,dev,proc,sys} exist in the chroot.
>
> This is specific to Linux.
>
>
> Diffs
> -----
>
> src/slave/containerizer/mesos/launch.hpp
> 7c8b535746b5ce9add00afef86fdb6faefb5620e
> src/slave/containerizer/mesos/launch.cpp
> 2f2d60e2011f60ec711d3b29fd2c157e30c83c34
>
> Diff: https://reviews.apache.org/r/31444/diff/
>
>
> Testing
> -------
>
> Manual testing only so far. This is harder to automate because we need a
> self-contained chroot to execute something in... Suggestions welcome.
>
>
> Thanks,
>
> Ian Downes
>
>
