Re: Review Request 67098: Updated the container launcher mount sequence.

[Jie Yu]

---
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.

[James Peach]

---
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.

[James Peach]

> On May 11, 2018, 11:46 p.m., Jie Yu wrote:
> > src/slave/containerizer/mesos/launch.cpp
> > Lines 399-410 (patched)
> > 
> >
> > + @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.

[James Peach]

> On May 23, 2018, 6:56 p.m., Jie Yu wrote:
> > src/slave/containerizer/mesos/launch.cpp
> > Lines 401 (patched)
> > 
> >
> > 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)
> > 
> >
> > 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.

[James Peach]

> On May 23, 2018, 6:56 p.m., Jie Yu wrote:
> > src/slave/containerizer/mesos/launch.cpp
> > Lines 406-407 (patched)
> > 
> >
> > 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.

[Jie Yu]

---
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)


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)


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.

[James Peach]

> On May 11, 2018, 11:46 p.m., Jie Yu wrote:
> > high level comments: should we just create `/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.

[Jie Yu]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/67098/#review202977
---



high level comments: should we just create `/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)


+ @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
> 
>

Re: Review Request 67098: Updated the container launcher mount sequence.

[Mesos Reviewbot]

---
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.

[Mesos Reviewbot Windows]

---
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
> 
>

Review Request 67098: Updated the container launcher mount sequence.

[James Peach]

---
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