You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@mesos.apache.org by James Peach <jp...@apache.org> on 2018/05/11 18:32:57 UTC
Review Request 67098: Updated the container launcher mount sequence.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/67098/
-----------------------------------------------------------
Review request for mesos, Gilbert Song, Jason Lai, Jie Yu, and Zhitao Li.
Bugs: MESOS-8792
https://issues.apache.org/jira/browse/MESOS-8792
Repository: mesos
Description
-------
The `linux/devices` isolator needs to make bind mounts into
the `/dev` directory of the container. However, the container
mounts are made before the container `/dev` is mounted as part
of the chroot preparation. We need to prepare the chroot,
then make any necessary container mounts, and finally enter
the chroot. This sequence of operations also requires us to
touch the target mount point, since we can't do it from the
isolator because the '/dev' directory doesn't exist yet.
Diffs
-----
src/slave/containerizer/mesos/launch.cpp f25d90651ef32495c9161c3eaed8a327d1b2b926
Diff: https://reviews.apache.org/r/67098/diff/1/
Testing
-------
manual
Thanks,
James Peach
Re: Review Request 67098: Updated the container launcher mount
sequence.
Posted by Mesos Reviewbot Windows <re...@mesos.apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/67098/#review202963
-----------------------------------------------------------
PASS: Mesos patch 67098 was successfully built and tested.
Reviews applied: `['67094', '67095', '67096', '67097', '67098']`
All the build artifacts available at: http://dcos-win.westus.cloudapp.azure.com/mesos-build/review/67098
- Mesos Reviewbot Windows
On May 11, 2018, 6:32 p.m., James Peach wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/67098/
> -----------------------------------------------------------
>
> (Updated May 11, 2018, 6:32 p.m.)
>
>
> Review request for mesos, Gilbert Song, Jason Lai, Jie Yu, and Zhitao Li.
>
>
> Bugs: MESOS-8792
> https://issues.apache.org/jira/browse/MESOS-8792
>
>
> Repository: mesos
>
>
> Description
> -------
>
> The `linux/devices` isolator needs to make bind mounts into
> the `/dev` directory of the container. However, the container
> mounts are made before the container `/dev` is mounted as part
> of the chroot preparation. We need to prepare the chroot,
> then make any necessary container mounts, and finally enter
> the chroot. This sequence of operations also requires us to
> touch the target mount point, since we can't do it from the
> isolator because the '/dev' directory doesn't exist yet.
>
>
> Diffs
> -----
>
> src/slave/containerizer/mesos/launch.cpp f25d90651ef32495c9161c3eaed8a327d1b2b926
>
>
> Diff: https://reviews.apache.org/r/67098/diff/1/
>
>
> Testing
> -------
>
> manual
>
>
> Thanks,
>
> James Peach
>
>
Re: Review Request 67098: Updated the container launcher mount
sequence.
Posted by James Peach <jp...@apache.org>.
> On May 23, 2018, 6:56 p.m., Jie Yu wrote:
> > src/slave/containerizer/mesos/launch.cpp
> > Lines 401 (patched)
> > <https://reviews.apache.org/r/67098/diff/3/?file=2024123#file2024123line401>
> >
> > I'd still prefer printing the error so that the error can be more specific.
Fixed.
> On May 23, 2018, 6:56 p.m., Jie Yu wrote:
> > src/slave/containerizer/mesos/launch.cpp
> > Lines 406-407 (patched)
> > <https://reviews.apache.org/r/67098/diff/3/?file=2024123#file2024123line406>
> >
> > Can you address this TODO? sounds like just a `dirname` and `mkdir`?
>
> James Peach wrote:
> Jason's patch series from [/r/65811](https://reviews.apache.org/r/65811/) addresses this, shall we leave it for that?
Fixed.
- James
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/67098/#review203675
-----------------------------------------------------------
On May 11, 2018, 6:32 p.m., James Peach wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/67098/
> -----------------------------------------------------------
>
> (Updated May 11, 2018, 6:32 p.m.)
>
>
> Review request for mesos, Gilbert Song, Jason Lai, and Jie Yu.
>
>
> Bugs: MESOS-8792
> https://issues.apache.org/jira/browse/MESOS-8792
>
>
> Repository: mesos
>
>
> Description
> -------
>
> The `linux/devices` isolator needs to make bind mounts into
> the `/dev` directory of the container. However, the container
> mounts are made before the container `/dev` is mounted as part
> of the chroot preparation. We need to prepare the chroot,
> then make any necessary container mounts, and finally enter
> the chroot. This sequence of operations also requires us to
> touch the target mount point, since we can't do it from the
> isolator because the '/dev' directory doesn't exist yet.
>
>
> Diffs
> -----
>
> src/slave/containerizer/mesos/launch.cpp f25d90651ef32495c9161c3eaed8a327d1b2b926
>
>
> Diff: https://reviews.apache.org/r/67098/diff/3/
>
>
> Testing
> -------
>
> make check (Fedora 27)
>
>
> Thanks,
>
> James Peach
>
>
Re: Review Request 67098: Updated the container launcher mount
sequence.
Posted by James Peach <jp...@apache.org>.
> On May 23, 2018, 6:56 p.m., Jie Yu wrote:
> > src/slave/containerizer/mesos/launch.cpp
> > Lines 406-407 (patched)
> > <https://reviews.apache.org/r/67098/diff/3/?file=2024123#file2024123line406>
> >
> > Can you address this TODO? sounds like just a `dirname` and `mkdir`?
Jason's patch series from [/r/65811](https://reviews.apache.org/r/65811/) addresses this, shall we leave it for that?
- James
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/67098/#review203675
-----------------------------------------------------------
On May 11, 2018, 6:32 p.m., James Peach wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/67098/
> -----------------------------------------------------------
>
> (Updated May 11, 2018, 6:32 p.m.)
>
>
> Review request for mesos, Gilbert Song, Jason Lai, and Jie Yu.
>
>
> Bugs: MESOS-8792
> https://issues.apache.org/jira/browse/MESOS-8792
>
>
> Repository: mesos
>
>
> Description
> -------
>
> The `linux/devices` isolator needs to make bind mounts into
> the `/dev` directory of the container. However, the container
> mounts are made before the container `/dev` is mounted as part
> of the chroot preparation. We need to prepare the chroot,
> then make any necessary container mounts, and finally enter
> the chroot. This sequence of operations also requires us to
> touch the target mount point, since we can't do it from the
> isolator because the '/dev' directory doesn't exist yet.
>
>
> Diffs
> -----
>
> src/slave/containerizer/mesos/launch.cpp f25d90651ef32495c9161c3eaed8a327d1b2b926
>
>
> Diff: https://reviews.apache.org/r/67098/diff/3/
>
>
> Testing
> -------
>
> make check (Fedora 27)
>
>
> Thanks,
>
> James Peach
>
>
Re: Review Request 67098: Updated the container launcher mount
sequence.
Posted by Jie Yu <yu...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/67098/#review203675
-----------------------------------------------------------
src/slave/containerizer/mesos/launch.cpp
Lines 401 (patched)
<https://reviews.apache.org/r/67098/#comment285956>
I'd still prefer printing the error so that the error can be more specific.
src/slave/containerizer/mesos/launch.cpp
Lines 406-407 (patched)
<https://reviews.apache.org/r/67098/#comment285957>
Can you address this TODO? sounds like just a `dirname` and `mkdir`?
- Jie Yu
On May 11, 2018, 6:32 p.m., James Peach wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/67098/
> -----------------------------------------------------------
>
> (Updated May 11, 2018, 6:32 p.m.)
>
>
> Review request for mesos, Gilbert Song, Jason Lai, and Jie Yu.
>
>
> Bugs: MESOS-8792
> https://issues.apache.org/jira/browse/MESOS-8792
>
>
> Repository: mesos
>
>
> Description
> -------
>
> The `linux/devices` isolator needs to make bind mounts into
> the `/dev` directory of the container. However, the container
> mounts are made before the container `/dev` is mounted as part
> of the chroot preparation. We need to prepare the chroot,
> then make any necessary container mounts, and finally enter
> the chroot. This sequence of operations also requires us to
> touch the target mount point, since we can't do it from the
> isolator because the '/dev' directory doesn't exist yet.
>
>
> Diffs
> -----
>
> src/slave/containerizer/mesos/launch.cpp f25d90651ef32495c9161c3eaed8a327d1b2b926
>
>
> Diff: https://reviews.apache.org/r/67098/diff/3/
>
>
> Testing
> -------
>
> make check (Fedora 27)
>
>
> Thanks,
>
> James Peach
>
>
Re: Review Request 67098: Updated the container launcher mount
sequence.
Posted by Jie Yu <yu...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/67098/#review203897
-----------------------------------------------------------
Ship it!
Ship It!
- Jie Yu
On May 24, 2018, 12:27 a.m., James Peach wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/67098/
> -----------------------------------------------------------
>
> (Updated May 24, 2018, 12:27 a.m.)
>
>
> Review request for mesos, Gilbert Song, Jason Lai, and Jie Yu.
>
>
> Bugs: MESOS-8792
> https://issues.apache.org/jira/browse/MESOS-8792
>
>
> Repository: mesos
>
>
> Description
> -------
>
> The `linux/devices` isolator needs to make bind mounts into
> the `/dev` directory of the container. However, the container
> mounts are made before the container `/dev` is mounted as part
> of the chroot preparation. We need to prepare the chroot,
> then make any necessary container mounts, and finally enter
> the chroot. This sequence of operations also requires us to
> touch the target mount point, since we can't do it from the
> isolator because the '/dev' directory doesn't exist yet.
>
>
> Diffs
> -----
>
> src/slave/containerizer/mesos/launch.cpp f25d90651ef32495c9161c3eaed8a327d1b2b926
>
>
> Diff: https://reviews.apache.org/r/67098/diff/4/
>
>
> Testing
> -------
>
> make check (Fedora 27)
>
>
> Thanks,
>
> James Peach
>
>
Re: Review Request 67098: Updated the container launcher mount
sequence.
Posted by James Peach <jp...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/67098/
-----------------------------------------------------------
(Updated May 24, 2018, 12:27 a.m.)
Review request for mesos, Gilbert Song, Jason Lai, and Jie Yu.
Bugs: MESOS-8792
https://issues.apache.org/jira/browse/MESOS-8792
Repository: mesos
Description
-------
The `linux/devices` isolator needs to make bind mounts into
the `/dev` directory of the container. However, the container
mounts are made before the container `/dev` is mounted as part
of the chroot preparation. We need to prepare the chroot,
then make any necessary container mounts, and finally enter
the chroot. This sequence of operations also requires us to
touch the target mount point, since we can't do it from the
isolator because the '/dev' directory doesn't exist yet.
Diffs (updated)
-----
src/slave/containerizer/mesos/launch.cpp f25d90651ef32495c9161c3eaed8a327d1b2b926
Diff: https://reviews.apache.org/r/67098/diff/4/
Changes: https://reviews.apache.org/r/67098/diff/3-4/
Testing
-------
make check (Fedora 27)
Thanks,
James Peach
Re: Review Request 67098: Updated the container launcher mount
sequence.
Posted by Mesos Reviewbot <re...@mesos.apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/67098/#review202964
-----------------------------------------------------------
Patch looks great!
Reviews applied: [67094, 67095, 67096, 67097, 67098]
Passed command: export OS='ubuntu:14.04' BUILDTOOL='autotools' COMPILER='gcc' CONFIGURATION='--verbose --disable-libtool-wrappers' ENVIRONMENT='GLOG_v=1 MESOS_VERBOSE=1'; ./support/docker-build.sh
- Mesos Reviewbot
On May 11, 2018, 6:32 p.m., James Peach wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/67098/
> -----------------------------------------------------------
>
> (Updated May 11, 2018, 6:32 p.m.)
>
>
> Review request for mesos, Gilbert Song, Jason Lai, Jie Yu, and Zhitao Li.
>
>
> Bugs: MESOS-8792
> https://issues.apache.org/jira/browse/MESOS-8792
>
>
> Repository: mesos
>
>
> Description
> -------
>
> The `linux/devices` isolator needs to make bind mounts into
> the `/dev` directory of the container. However, the container
> mounts are made before the container `/dev` is mounted as part
> of the chroot preparation. We need to prepare the chroot,
> then make any necessary container mounts, and finally enter
> the chroot. This sequence of operations also requires us to
> touch the target mount point, since we can't do it from the
> isolator because the '/dev' directory doesn't exist yet.
>
>
> Diffs
> -----
>
> src/slave/containerizer/mesos/launch.cpp f25d90651ef32495c9161c3eaed8a327d1b2b926
>
>
> Diff: https://reviews.apache.org/r/67098/diff/1/
>
>
> Testing
> -------
>
> manual
>
>
> Thanks,
>
> James Peach
>
>
Re: Review Request 67098: Updated the container launcher mount
sequence.
Posted by James Peach <jp...@apache.org>.
> On May 11, 2018, 11:46 p.m., Jie Yu wrote:
> > src/slave/containerizer/mesos/launch.cpp
> > Lines 399-410 (patched)
> > <https://reviews.apache.org/r/67098/diff/1/?file=2020542#file2020542line399>
> >
> > + @jasonlai
> >
> > This might be related to your chain. Please let us know what's the best way forward to best aligh with your goal.
Dropping, since we discussed offline.
- James
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/67098/#review202977
-----------------------------------------------------------
On May 11, 2018, 6:32 p.m., James Peach wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/67098/
> -----------------------------------------------------------
>
> (Updated May 11, 2018, 6:32 p.m.)
>
>
> Review request for mesos, Gilbert Song, Jason Lai, and Jie Yu.
>
>
> Bugs: MESOS-8792
> https://issues.apache.org/jira/browse/MESOS-8792
>
>
> Repository: mesos
>
>
> Description
> -------
>
> The `linux/devices` isolator needs to make bind mounts into
> the `/dev` directory of the container. However, the container
> mounts are made before the container `/dev` is mounted as part
> of the chroot preparation. We need to prepare the chroot,
> then make any necessary container mounts, and finally enter
> the chroot. This sequence of operations also requires us to
> touch the target mount point, since we can't do it from the
> isolator because the '/dev' directory doesn't exist yet.
>
>
> Diffs
> -----
>
> src/slave/containerizer/mesos/launch.cpp f25d90651ef32495c9161c3eaed8a327d1b2b926
>
>
> Diff: https://reviews.apache.org/r/67098/diff/3/
>
>
> Testing
> -------
>
> make check (Fedora 27)
>
>
> Thanks,
>
> James Peach
>
>
Re: Review Request 67098: Updated the container launcher mount
sequence.
Posted by James Peach <jp...@apache.org>.
> On May 11, 2018, 11:46 p.m., Jie Yu wrote:
> > high level comments: should we just create `<rootfs>/dev` in the linux devices isolator "prepare" phase? The linux devices isolator should also prepare those standard devices. Logically, this makes sense because `/dev` should just be owned by linux devices isolator?
Yes, eventually I think we can standardize on creating all the devices in this isolator. If we do that, then we can also mount `dev` as a single mount rather than bind mounting all the devices separately. However, I'd prefer to to leave that for future work in a separate patch series.
- James
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/67098/#review202977
-----------------------------------------------------------
On May 11, 2018, 6:32 p.m., James Peach wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/67098/
> -----------------------------------------------------------
>
> (Updated May 11, 2018, 6:32 p.m.)
>
>
> Review request for mesos, Gilbert Song, Jason Lai, Jie Yu, and Zhitao Li.
>
>
> Bugs: MESOS-8792
> https://issues.apache.org/jira/browse/MESOS-8792
>
>
> Repository: mesos
>
>
> Description
> -------
>
> The `linux/devices` isolator needs to make bind mounts into
> the `/dev` directory of the container. However, the container
> mounts are made before the container `/dev` is mounted as part
> of the chroot preparation. We need to prepare the chroot,
> then make any necessary container mounts, and finally enter
> the chroot. This sequence of operations also requires us to
> touch the target mount point, since we can't do it from the
> isolator because the '/dev' directory doesn't exist yet.
>
>
> Diffs
> -----
>
> src/slave/containerizer/mesos/launch.cpp f25d90651ef32495c9161c3eaed8a327d1b2b926
>
>
> Diff: https://reviews.apache.org/r/67098/diff/1/
>
>
> Testing
> -------
>
> manual
>
>
> Thanks,
>
> James Peach
>
>
Re: Review Request 67098: Updated the container launcher mount
sequence.
Posted by Jie Yu <yu...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/67098/#review202977
-----------------------------------------------------------
high level comments: should we just create `<rootfs>/dev` in the linux devices isolator "prepare" phase? The linux devices isolator should also prepare those standard devices. Logically, this makes sense because `/dev` should just be owned by linux devices isolator?
src/slave/containerizer/mesos/launch.cpp
Lines 399-410 (patched)
<https://reviews.apache.org/r/67098/#comment285039>
+ @jasonlai
This might be related to your chain. Please let us know what's the best way forward to best aligh with your goal.
- Jie Yu
On May 11, 2018, 6:32 p.m., James Peach wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/67098/
> -----------------------------------------------------------
>
> (Updated May 11, 2018, 6:32 p.m.)
>
>
> Review request for mesos, Gilbert Song, Jason Lai, Jie Yu, and Zhitao Li.
>
>
> Bugs: MESOS-8792
> https://issues.apache.org/jira/browse/MESOS-8792
>
>
> Repository: mesos
>
>
> Description
> -------
>
> The `linux/devices` isolator needs to make bind mounts into
> the `/dev` directory of the container. However, the container
> mounts are made before the container `/dev` is mounted as part
> of the chroot preparation. We need to prepare the chroot,
> then make any necessary container mounts, and finally enter
> the chroot. This sequence of operations also requires us to
> touch the target mount point, since we can't do it from the
> isolator because the '/dev' directory doesn't exist yet.
>
>
> Diffs
> -----
>
> src/slave/containerizer/mesos/launch.cpp f25d90651ef32495c9161c3eaed8a327d1b2b926
>
>
> Diff: https://reviews.apache.org/r/67098/diff/1/
>
>
> Testing
> -------
>
> manual
>
>
> Thanks,
>
> James Peach
>
>