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/10/19 17:38:09 UTC
Review Request 69086: Move the container `/dev` construction to the
isolators.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/69086/
-----------------------------------------------------------
Review request for mesos, Gilbert Song, Jason Lai, Jie Yu, and Jiang Yan Xu.
Bugs: MESOS-9319
https://issues.apache.org/jira/browse/MESOS-9319
Repository: mesos
Description
-------
Previously, if the container was configured with a root filesystem,
the container `/dev` was populated by the chroot API and this API
had a special case for adding GPU devices. This change extends
the approach that was introduced in the `linux/devices` isolator
to construct the whole of the Linux container `/dev` hierarchy
before launching the container. The `linux/filesystem` isolator is
now responsible for mounting the container `/dev`, and any other
isolators that enable access to devices can simply populate device
nodes in the container devices directory. After this change, the
container '/dev' is mounted read-only so that this cannot be used
to escape any disk quota.
Diffs
-----
src/linux/fs.hpp 502f85c4a32d8658bdd701975dd5ac3d802d308e
src/linux/fs.cpp 9055ef42edd1fb90e1026d1d603a9ba902cfc1fd
src/slave/containerizer/mesos/isolators/filesystem/linux.cpp a47899cb528eef103f299def3bd3466905ac5b51
src/slave/containerizer/mesos/isolators/gpu/isolator.hpp 4645c625877d9451516133b24bd3959e0f49c0a9
src/slave/containerizer/mesos/isolators/gpu/isolator.cpp dbbf92ffbe4a46cedca5b53f6ba172bfb308100e
src/slave/containerizer/mesos/isolators/linux/devices.cpp 8f8ff95ec3856ba06647637a80315365d0e66e23
src/slave/containerizer/mesos/launch.cpp 7193da0a094df3e441e185c62b3a0379a0bdc4a2
Diff: https://reviews.apache.org/r/69086/diff/1/
Testing
-------
sudo make check (Fedora 28)
Thanks,
James Peach
Re: Review Request 69086: Move the container `/dev` construction to
the isolators.
Posted by James Peach <jp...@apache.org>.
> On Oct. 29, 2018, 4:42 a.m., Jie Yu wrote:
> > src/linux/fs.hpp
> > Lines 397-401 (patched)
> > <https://reviews.apache.org/r/69086/diff/3/?file=2100940#file2100940line397>
> >
> > Any reason need this option? I was thinking just doing dev mounts always from linux fileystem isolator.
Since `fs::chroot` was originally designed as a stand-alone API, I wanted to preserve the ability to use it without the isolator layer. I'm not strongly attached to this approach, though, so we could just make all the mounts from the linux filesystem isolator.
- James
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/69086/#review210131
-----------------------------------------------------------
On Oct. 19, 2018, 5:38 p.m., James Peach wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/69086/
> -----------------------------------------------------------
>
> (Updated Oct. 19, 2018, 5:38 p.m.)
>
>
> Review request for mesos, Gilbert Song, Jason Lai, Jie Yu, and Jiang Yan Xu.
>
>
> Bugs: MESOS-9319
> https://issues.apache.org/jira/browse/MESOS-9319
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Previously, if the container was configured with a root filesystem,
> the container `/dev` was populated by the chroot API and this API
> had a special case for adding GPU devices. This change extends
> the approach that was introduced in the `linux/devices` isolator
> to construct the whole of the Linux container `/dev` hierarchy
> before launching the container. The `linux/filesystem` isolator is
> now responsible for mounting the container `/dev`, and any other
> isolators that enable access to devices can simply populate device
> nodes in the container devices directory. After this change, the
> container `/dev` is mounted read-only so that this cannot be used
> to escape any disk quota.
>
>
> Diffs
> -----
>
> src/linux/fs.hpp 502f85c4a32d8658bdd701975dd5ac3d802d308e
> src/linux/fs.cpp 9055ef42edd1fb90e1026d1d603a9ba902cfc1fd
> src/slave/containerizer/mesos/isolators/filesystem/linux.cpp a47899cb528eef103f299def3bd3466905ac5b51
> src/slave/containerizer/mesos/isolators/gpu/isolator.hpp 4645c625877d9451516133b24bd3959e0f49c0a9
> src/slave/containerizer/mesos/isolators/gpu/isolator.cpp dbbf92ffbe4a46cedca5b53f6ba172bfb308100e
> src/slave/containerizer/mesos/isolators/linux/devices.cpp 8f8ff95ec3856ba06647637a80315365d0e66e23
> src/slave/containerizer/mesos/launch.cpp 7193da0a094df3e441e185c62b3a0379a0bdc4a2
>
>
> Diff: https://reviews.apache.org/r/69086/diff/3/
>
>
> Testing
> -------
>
> sudo make check (Fedora 28)
>
>
> Thanks,
>
> James Peach
>
>
Re: Review Request 69086: Move the container `/dev` construction to
the isolators.
Posted by James Peach <jp...@apache.org>.
> On Oct. 29, 2018, 4:42 a.m., Jie Yu wrote:
> > src/slave/containerizer/mesos/isolators/filesystem/linux.cpp
> > Lines 414 (patched)
> > <https://reviews.apache.org/r/69086/diff/3/?file=2100942#file2100942line415>
> >
> > Not related to this patch. When I review this patch, I was looking at paths.hpp and couldn't find any comments related to `devices` folder. Can you update the comments there (in the begining of `src/slave/containerizer/mesos/paths.hpp`)
Fixed in https://reviews.apache.org/r/69211/
- James
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/69086/#review210131
-----------------------------------------------------------
On Oct. 19, 2018, 5:38 p.m., James Peach wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/69086/
> -----------------------------------------------------------
>
> (Updated Oct. 19, 2018, 5:38 p.m.)
>
>
> Review request for mesos, Gilbert Song, Jason Lai, Jie Yu, and Jiang Yan Xu.
>
>
> Bugs: MESOS-9319
> https://issues.apache.org/jira/browse/MESOS-9319
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Previously, if the container was configured with a root filesystem,
> the container `/dev` was populated by the chroot API and this API
> had a special case for adding GPU devices. This change extends
> the approach that was introduced in the `linux/devices` isolator
> to construct the whole of the Linux container `/dev` hierarchy
> before launching the container. The `linux/filesystem` isolator is
> now responsible for mounting the container `/dev`, and any other
> isolators that enable access to devices can simply populate device
> nodes in the container devices directory. After this change, the
> container `/dev` is mounted read-only so that this cannot be used
> to escape any disk quota.
>
>
> Diffs
> -----
>
> src/linux/fs.hpp 502f85c4a32d8658bdd701975dd5ac3d802d308e
> src/linux/fs.cpp 9055ef42edd1fb90e1026d1d603a9ba902cfc1fd
> src/slave/containerizer/mesos/isolators/filesystem/linux.cpp a47899cb528eef103f299def3bd3466905ac5b51
> src/slave/containerizer/mesos/isolators/gpu/isolator.hpp 4645c625877d9451516133b24bd3959e0f49c0a9
> src/slave/containerizer/mesos/isolators/gpu/isolator.cpp dbbf92ffbe4a46cedca5b53f6ba172bfb308100e
> src/slave/containerizer/mesos/isolators/linux/devices.cpp 8f8ff95ec3856ba06647637a80315365d0e66e23
> src/slave/containerizer/mesos/launch.cpp 7193da0a094df3e441e185c62b3a0379a0bdc4a2
>
>
> Diff: https://reviews.apache.org/r/69086/diff/3/
>
>
> Testing
> -------
>
> sudo make check (Fedora 28)
>
>
> Thanks,
>
> James Peach
>
>
Re: Review Request 69086: Move the container `/dev` construction to
the isolators.
Posted by Jie Yu <yu...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/69086/#review210131
-----------------------------------------------------------
src/linux/fs.hpp
Lines 397-401 (patched)
<https://reviews.apache.org/r/69086/#comment294766>
Any reason need this option? I was thinking just doing dev mounts always from linux fileystem isolator.
src/linux/fs.cpp
Line 697 (original), 675 (patched)
<https://reviews.apache.org/r/69086/#comment294767>
Can we do those in linux filesystem isolator too?
src/slave/containerizer/mesos/isolators/filesystem/linux.cpp
Lines 414 (patched)
<https://reviews.apache.org/r/69086/#comment294759>
Not related to this patch. When I review this patch, I was looking at paths.hpp and couldn't find any comments related to `devices` folder. Can you update the comments there (in the begining of `src/slave/containerizer/mesos/paths.hpp`)
src/slave/containerizer/mesos/isolators/filesystem/linux.cpp
Lines 417-420 (patched)
<https://reviews.apache.org/r/69086/#comment294760>
This sounds unnecessary given we just created an empty `launchInfo` above.
We don't yet pass `launchInfo` to other isolators yet.
src/slave/containerizer/mesos/isolators/filesystem/linux.cpp
Lines 433 (patched)
<https://reviews.apache.org/r/69086/#comment294763>
Instead of CHECK_SOME, i'd still prefer we return a Failure here.
src/slave/containerizer/mesos/isolators/filesystem/linux.cpp
Lines 436 (patched)
<https://reviews.apache.org/r/69086/#comment294764>
Ditto.
src/slave/containerizer/mesos/launch.cpp
Lines 510 (patched)
<https://reviews.apache.org/r/69086/#comment294765>
Is this intentional?
- Jie Yu
On Oct. 19, 2018, 5:38 p.m., James Peach wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/69086/
> -----------------------------------------------------------
>
> (Updated Oct. 19, 2018, 5:38 p.m.)
>
>
> Review request for mesos, Gilbert Song, Jason Lai, Jie Yu, and Jiang Yan Xu.
>
>
> Bugs: MESOS-9319
> https://issues.apache.org/jira/browse/MESOS-9319
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Previously, if the container was configured with a root filesystem,
> the container `/dev` was populated by the chroot API and this API
> had a special case for adding GPU devices. This change extends
> the approach that was introduced in the `linux/devices` isolator
> to construct the whole of the Linux container `/dev` hierarchy
> before launching the container. The `linux/filesystem` isolator is
> now responsible for mounting the container `/dev`, and any other
> isolators that enable access to devices can simply populate device
> nodes in the container devices directory. After this change, the
> container `/dev` is mounted read-only so that this cannot be used
> to escape any disk quota.
>
>
> Diffs
> -----
>
> src/linux/fs.hpp 502f85c4a32d8658bdd701975dd5ac3d802d308e
> src/linux/fs.cpp 9055ef42edd1fb90e1026d1d603a9ba902cfc1fd
> src/slave/containerizer/mesos/isolators/filesystem/linux.cpp a47899cb528eef103f299def3bd3466905ac5b51
> src/slave/containerizer/mesos/isolators/gpu/isolator.hpp 4645c625877d9451516133b24bd3959e0f49c0a9
> src/slave/containerizer/mesos/isolators/gpu/isolator.cpp dbbf92ffbe4a46cedca5b53f6ba172bfb308100e
> src/slave/containerizer/mesos/isolators/linux/devices.cpp 8f8ff95ec3856ba06647637a80315365d0e66e23
> src/slave/containerizer/mesos/launch.cpp 7193da0a094df3e441e185c62b3a0379a0bdc4a2
>
>
> Diff: https://reviews.apache.org/r/69086/diff/3/
>
>
> Testing
> -------
>
> sudo make check (Fedora 28)
>
>
> Thanks,
>
> James Peach
>
>
Re: Review Request 69086: Move the container `/dev` construction to
the isolators.
Posted by James Peach <jp...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/69086/#review209792
-----------------------------------------------------------
src/slave/containerizer/mesos/isolators/filesystem/linux.cpp
Lines 104 (patched)
<https://reviews.apache.org/r/69086/#comment294387>
Remove this. It is superseded by `fs::chroot::copyDeviceNode`
- James Peach
On Oct. 19, 2018, 5:38 p.m., James Peach wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/69086/
> -----------------------------------------------------------
>
> (Updated Oct. 19, 2018, 5:38 p.m.)
>
>
> Review request for mesos, Gilbert Song, Jason Lai, Jie Yu, and Jiang Yan Xu.
>
>
> Bugs: MESOS-9319
> https://issues.apache.org/jira/browse/MESOS-9319
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Previously, if the container was configured with a root filesystem,
> the container `/dev` was populated by the chroot API and this API
> had a special case for adding GPU devices. This change extends
> the approach that was introduced in the `linux/devices` isolator
> to construct the whole of the Linux container `/dev` hierarchy
> before launching the container. The `linux/filesystem` isolator is
> now responsible for mounting the container `/dev`, and any other
> isolators that enable access to devices can simply populate device
> nodes in the container devices directory. After this change, the
> container `/dev` is mounted read-only so that this cannot be used
> to escape any disk quota.
>
>
> Diffs
> -----
>
> src/linux/fs.hpp 502f85c4a32d8658bdd701975dd5ac3d802d308e
> src/linux/fs.cpp 9055ef42edd1fb90e1026d1d603a9ba902cfc1fd
> src/slave/containerizer/mesos/isolators/filesystem/linux.cpp a47899cb528eef103f299def3bd3466905ac5b51
> src/slave/containerizer/mesos/isolators/gpu/isolator.hpp 4645c625877d9451516133b24bd3959e0f49c0a9
> src/slave/containerizer/mesos/isolators/gpu/isolator.cpp dbbf92ffbe4a46cedca5b53f6ba172bfb308100e
> src/slave/containerizer/mesos/isolators/linux/devices.cpp 8f8ff95ec3856ba06647637a80315365d0e66e23
> src/slave/containerizer/mesos/launch.cpp 7193da0a094df3e441e185c62b3a0379a0bdc4a2
>
>
> Diff: https://reviews.apache.org/r/69086/diff/2/
>
>
> Testing
> -------
>
> sudo make check (Fedora 28)
>
>
> Thanks,
>
> James Peach
>
>
Re: Review Request 69086: Move the container `/dev` construction to
the isolators.
Posted by Mesos Reviewbot Windows <re...@mesos.apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/69086/#review209794
-----------------------------------------------------------
FAIL: Some of the unit tests failed. Please check the relevant logs.
Reviews applied: `['69086']`
Failed command: `Start-MesosCITesting`
All the build artifacts available at: http://dcos-win.westus2.cloudapp.azure.com/artifacts/mesos-reviewbot-testing/2491/mesos-review-69086
Relevant logs:
- [mesos-tests-cmake.log](http://dcos-win.westus2.cloudapp.azure.com/artifacts/mesos-reviewbot-testing/2491/mesos-review-69086/logs/mesos-tests-cmake.log):
```
d:\dcos\mesos\3rdparty\zookeeper-3.4.8\src\zookeeper-3.4.8\src\c\src\recordio.c(170): warning C4267: '=': conversion from 'size_t' to 'int32_t', possible loss of data [D:\DCOS\mesos\3rdparty\zookeeper-3.4.8\src\zookeeper-3.4.8-build\zookeeper.vcxproj] [D:\DCOS\mesos\3rdparty\zookeeper-3.4.8.vcxproj]
d:\dcos\mesos\3rdparty\zookeeper-3.4.8\src\zookeeper-3.4.8\src\c\src\mt_adaptor.c(496): warning C4244: '=': conversion from 'time_t' to 'int32_t', possible loss of data [D:\DCOS\mesos\3rdparty\zookeeper-3.4.8\src\zookeeper-3.4.8-build\zookeeper.vcxproj] [D:\DCOS\mesos\3rdparty\zookeeper-3.4.8.vcxproj]
d:\dcos\mesos\3rdparty\zookeeper-3.4.8\src\zookeeper-3.4.8\src\c\src\winport.c(256): warning C4090: 'function': different 'const' qualifiers [D:\DCOS\mesos\3rdparty\zookeeper-3.4.8\src\zookeeper-3.4.8-build\zookeeper.vcxproj] [D:\DCOS\mesos\3rdparty\zookeeper-3.4.8.vcxproj]
d:\dcos\mesos\3rdparty\zookeeper-3.4.8\src\zookeeper-3.4.8\src\c\src\winport.c(166): warning C4716: 'pthread_cond_broadcast': must return a value [D:\DCOS\mesos\3rdparty\zookeeper-3.4.8\src\zookeeper-3.4.8-build\zookeeper.vcxproj] [D:\DCOS\mesos\3rdparty\zookeeper-3.4.8.vcxproj]
d:\dcos\mesos\3rdparty\zookeeper-3.4.8\src\zookeeper-3.4.8\src\c\src\winport.c(205): warning C4716: 'pthread_cond_wait': must return a value [D:\DCOS\mesos\3rdparty\zookeeper-3.4.8\src\zookeeper-3.4.8-build\zookeeper.vcxproj] [D:\DCOS\mesos\3rdparty\zookeeper-3.4.8.vcxproj]
d:\dcos\mesos\3rdparty\zookeeper-3.4.8\src\zookeeper-3.4.8\src\c\src\cli.c(124): warning C4996: 'fopen': This function or variable may be unsafe. Consider using fopen_s instead. To disable deprecation, use _CRT_SECURE_NO_WARNINGS. See online help for details. [D:\DCOS\mesos\3rdparty\zookeeper-3.4.8\src\zookeeper-3.4.8-build\cli.vcxproj] [D:\DCOS\mesos\3rdparty\zookeeper-3.4.8.vcxproj]
d:\dcos\mesos\3rdparty\zookeeper-3.4.8\src\zookeeper-3.4.8\src\c\src\cli.c(128): warning C4267: 'initializing': conversion from 'size_t' to 'int', possible loss of data [D:\DCOS\mesos\3rdparty\zookeeper-3.4.8\src\zookeeper-3.4.8-build\cli.vcxproj] [D:\DCOS\mesos\3rdparty\zookeeper-3.4.8.vcxproj]
d:\dcos\mesos\3rdparty\zookeeper-3.4.8\src\zookeeper-3.4.8\src\c\src\cli.c(279): warning C4267: 'function': conversion from 'size_t' to 'int', possible loss of data [D:\DCOS\mesos\3rdparty\zookeeper-3.4.8\src\zookeeper-3.4.8-build\cli.vcxproj] [D:\DCOS\mesos\3rdparty\zookeeper-3.4.8.vcxproj]
d:\dcos\mesos\3rdparty\zookeeper-3.4.8\src\zookeeper-3.4.8\src\c\src\cli.c(301): warning C4267: 'initializing': conversion from 'size_t' to 'int', possible loss of data [D:\DCOS\mesos\3rdparty\zookeeper-3.4.8\src\zookeeper-3.4.8-build\cli.vcxproj] [D:\DCOS\mesos\3rdparty\zookeeper-3.4.8.vcxproj]
d:\dcos\mesos\3rdparty\zookeeper-3.4.8\src\zookeeper-3.4.8\src\c\src\cli.c(368): warning C4267: 'function': conversion from 'size_t' to 'int', possible loss of data [D:\DCOS\mesos\3rdparty\zookeeper-3.4.8\src\zookeeper-3.4.8-build\cli.vcxproj] [D:\DCOS\mesos\3rdparty\zookeeper-3.4.8.vcxproj]
d:\dcos\mesos\3rdparty\zookeeper-3.4.8\src\zookeeper-3.4.8\src\c\src\cli.c(372): warning C4267: 'function': conversion from 'size_t' to 'int', possible loss of data [D:\DCOS\mesos\3rdparty\zookeeper-3.4.8\src\zookeeper-3.4.8-build\cli.vcxproj] [D:\DCOS\mesos\3rdparty\zookeeper-3.4.8.vcxproj]
d:\dcos\mesos\3rdparty\zookeeper-3.4.8\src\zookeeper-3.4.8\src\c\src\cli.c(512): warning C4267: 'function': conversion from 'size_t' to 'int', possible loss of data [D:\DCOS\mesos\3rdparty\zookeeper-3.4.8\src\zookeeper-3.4.8-build\cli.vcxproj] [D:\DCOS\mesos\3rdparty\zookeeper-3.4.8.vcxproj]
d:\dcos\mesos\3rdparty\zookeeper-3.4.8\src\zookeeper-3.4.8\src\c\src\cli.c(543): warning C4996: 'strcpy': This function or variable may be unsafe. Consider using strcpy_s instead. To disable deprecation, use _CRT_SECURE_NO_WARNINGS. See online help for details. [D:\DCOS\mesos\3rdparty\zookeeper-3.4.8\src\zookeeper-3.4.8-build\cli.vcxproj] [D:\DCOS\mesos\3rdparty\zookeeper-3.4.8.vcxproj]
d:\dcos\mesos\3rdparty\zookeeper-3.4.8\src\zookeeper-3.4.8\src\c\src\cli.c(548): warning C4996: 'fopen': This function or variable may be unsafe. Consider using fopen_s instead. To disable deprecation, use _CRT_SECURE_NO_WARNINGS. See online help for details. [D:\DCOS\mesos\3rdparty\zookeeper-3.4.8\src\zookeeper-3.4.8-build\cli.vcxproj] [D:\DCOS\mesos\3rdparty\zookeeper-3.4.8.vcxproj]
d:\dcos\mesos\3rdparty\zookeeper-3.4.8\src\zookeeper-3.4.8\src\c\src\cli.c(569): warning C4996: 'strcpy': This function or variable may be unsafe. Consider using strcpy_s instead. To disable deprecation, use _CRT_SECURE_NO_WARNINGS. See online help for details. [D:\DCOS\mesos\3rdparty\zookeeper-3.4.8\src\zookeeper-3.4.8-build\cli.vcxproj] [D:\DCOS\mesos\3rdparty\zookeeper-3.4.8.vcxproj]
"D:\DCOS\mesos\src\tests\mesos-tests.vcxproj" (default target) (1) ->
"D:\DCOS\mesos\src\mesos.vcxproj" (default target) (19) ->
(ClCompile target) ->
d:\dcos\mesos\mesos\src\slave\containerizer\mesos\launch.cpp(82): error C3083: 'fs': the symbol to the left of a '::' must be a type [D:\DCOS\mesos\src\mesos.vcxproj]
d:\dcos\mesos\mesos\src\slave\containerizer\mesos\launch.cpp(82): error C3083: 'chroot': the symbol to the left of a '::' must be a type [D:\DCOS\mesos\src\mesos.vcxproj]
d:\dcos\mesos\mesos\src\slave\containerizer\mesos\launch.cpp(82): error C2039: 'PrepareOptions': is not a member of 'mesos::internal' [D:\DCOS\mesos\src\mesos.vcxproj]
d:\dcos\mesos\mesos\src\slave\containerizer\mesos\launch.cpp(82): error C2873: 'PrepareOptions': symbol cannot be used in a using-declaration [D:\DCOS\mesos\src\mesos.vcxproj]
172 Warning(s)
4 Error(s)
Time Elapsed 00:08:16.72
```
- Mesos Reviewbot Windows
On Oct. 19, 2018, 5:38 p.m., James Peach wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/69086/
> -----------------------------------------------------------
>
> (Updated Oct. 19, 2018, 5:38 p.m.)
>
>
> Review request for mesos, Gilbert Song, Jason Lai, Jie Yu, and Jiang Yan Xu.
>
>
> Bugs: MESOS-9319
> https://issues.apache.org/jira/browse/MESOS-9319
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Previously, if the container was configured with a root filesystem,
> the container `/dev` was populated by the chroot API and this API
> had a special case for adding GPU devices. This change extends
> the approach that was introduced in the `linux/devices` isolator
> to construct the whole of the Linux container `/dev` hierarchy
> before launching the container. The `linux/filesystem` isolator is
> now responsible for mounting the container `/dev`, and any other
> isolators that enable access to devices can simply populate device
> nodes in the container devices directory. After this change, the
> container `/dev` is mounted read-only so that this cannot be used
> to escape any disk quota.
>
>
> Diffs
> -----
>
> src/linux/fs.hpp 502f85c4a32d8658bdd701975dd5ac3d802d308e
> src/linux/fs.cpp 9055ef42edd1fb90e1026d1d603a9ba902cfc1fd
> src/slave/containerizer/mesos/isolators/filesystem/linux.cpp a47899cb528eef103f299def3bd3466905ac5b51
> src/slave/containerizer/mesos/isolators/gpu/isolator.hpp 4645c625877d9451516133b24bd3959e0f49c0a9
> src/slave/containerizer/mesos/isolators/gpu/isolator.cpp dbbf92ffbe4a46cedca5b53f6ba172bfb308100e
> src/slave/containerizer/mesos/isolators/linux/devices.cpp 8f8ff95ec3856ba06647637a80315365d0e66e23
> src/slave/containerizer/mesos/launch.cpp 7193da0a094df3e441e185c62b3a0379a0bdc4a2
>
>
> Diff: https://reviews.apache.org/r/69086/diff/2/
>
>
> Testing
> -------
>
> sudo make check (Fedora 28)
>
>
> Thanks,
>
> James Peach
>
>
Re: Review Request 69086: Move the container `/dev` construction to
the isolators.
Posted by Mesos Reviewbot <re...@mesos.apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/69086/#review210141
-----------------------------------------------------------
Bad patch!
Reviews applied: [69086]
Failed command: /usr/bin/python3 support/apply-reviews.py -n -r 69086
Error:
2018-10-29 17:31:20 URL:https://reviews.apache.org/r/69086/diff/raw/ [28095/28095] -> "69086.patch" [1]
error: patch failed: src/linux/fs.cpp:677
error: src/linux/fs.cpp: patch does not apply
error: patch failed: src/slave/containerizer/mesos/isolators/gpu/isolator.cpp:413
error: src/slave/containerizer/mesos/isolators/gpu/isolator.cpp: patch does not apply
Full log: https://builds.apache.org/job/Mesos-Reviewbot/23517/console
- Mesos Reviewbot
On Oct. 19, 2018, 5:38 p.m., James Peach wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/69086/
> -----------------------------------------------------------
>
> (Updated Oct. 19, 2018, 5:38 p.m.)
>
>
> Review request for mesos, Gilbert Song, Jason Lai, Jie Yu, and Jiang Yan Xu.
>
>
> Bugs: MESOS-9319
> https://issues.apache.org/jira/browse/MESOS-9319
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Previously, if the container was configured with a root filesystem,
> the container `/dev` was populated by the chroot API and this API
> had a special case for adding GPU devices. This change extends
> the approach that was introduced in the `linux/devices` isolator
> to construct the whole of the Linux container `/dev` hierarchy
> before launching the container. The `linux/filesystem` isolator is
> now responsible for mounting the container `/dev`, and any other
> isolators that enable access to devices can simply populate device
> nodes in the container devices directory. After this change, the
> container `/dev` is mounted read-only so that this cannot be used
> to escape any disk quota.
>
>
> Diffs
> -----
>
> src/linux/fs.hpp 502f85c4a32d8658bdd701975dd5ac3d802d308e
> src/linux/fs.cpp 9055ef42edd1fb90e1026d1d603a9ba902cfc1fd
> src/slave/containerizer/mesos/isolators/filesystem/linux.cpp a47899cb528eef103f299def3bd3466905ac5b51
> src/slave/containerizer/mesos/isolators/gpu/isolator.hpp 4645c625877d9451516133b24bd3959e0f49c0a9
> src/slave/containerizer/mesos/isolators/gpu/isolator.cpp dbbf92ffbe4a46cedca5b53f6ba172bfb308100e
> src/slave/containerizer/mesos/isolators/linux/devices.cpp 8f8ff95ec3856ba06647637a80315365d0e66e23
> src/slave/containerizer/mesos/launch.cpp 7193da0a094df3e441e185c62b3a0379a0bdc4a2
>
>
> Diff: https://reviews.apache.org/r/69086/diff/3/
>
>
> Testing
> -------
>
> sudo make check (Fedora 28)
>
>
> Thanks,
>
> James Peach
>
>
Re: Review Request 69086: Moved the container root construction to the
isolators.
Posted by Jie Yu <yu...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/69086/#review211012
-----------------------------------------------------------
Ship it!
Ship It!
- Jie Yu
On Nov. 27, 2018, 12:49 a.m., James Peach wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/69086/
> -----------------------------------------------------------
>
> (Updated Nov. 27, 2018, 12:49 a.m.)
>
>
> Review request for mesos, Gilbert Song, Jason Lai, Jie Yu, and Jiang Yan Xu.
>
>
> Bugs: MESOS-9319
> https://issues.apache.org/jira/browse/MESOS-9319
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Previously, if the container was configured with a root filesytem,
> the root was populated by a combination of the `fs::chroot:prepare`
> API and the various isolators. The implementation details of some
> isolators had leaked into the chroot code, which had a special case
> for adding GPU devices.
>
> This change moves all the responsibility for defining the
> root filesystem from the `fs::chroot::prepare()` API to the
> `filesystem/linux` isolator. The `filesystem/linux` isolator is
> now the single place that captures how to mount the container
> pseudo-filesystems as well as how to construct a proper `/dev`
> directory.
>
> Since the `linux/filesystem` isolator is now entirely responsible
> for creating and mounting the container `/dev`, any other isolators
> that enable access to devices should populate device nodes in the
> container devices directory and add a corresponding bind mount.
>
>
> Diffs
> -----
>
> src/common/protobuf_utils.hpp 1662125ed3e47b179ee32d08c1d3af75553066ba
> src/common/protobuf_utils.cpp a45607eed4c4bae5010bcc3f3ffeabd6d911062a
> src/linux/fs.hpp 31969f6ba82bf5ee549bfdf9698a21adaa486a90
> src/linux/fs.cpp 5cdffe1f4c7f00aee5b8f640e7cfa4a0018cfa0a
> src/slave/containerizer/mesos/isolators/filesystem/linux.cpp c7d753ac2e5575a8d687600bfb9e0617fa72c990
> src/slave/containerizer/mesos/isolators/gpu/isolator.hpp 4645c625877d9451516133b24bd3959e0f49c0a9
> src/slave/containerizer/mesos/isolators/gpu/isolator.cpp 56d835779618fd965d928c6926664583e9141f79
> src/slave/containerizer/mesos/isolators/linux/devices.cpp 8f8ff95ec3856ba06647637a80315365d0e66e23
> src/slave/containerizer/mesos/launch.cpp 882bcdf89e2b0cca3d3f62e6d017849a51ceaead
>
>
> Diff: https://reviews.apache.org/r/69086/diff/13/
>
>
> Testing
> -------
>
> sudo make check (Fedora 28)
>
>
> Thanks,
>
> James Peach
>
>
Re: Review Request 69086: Moved the container root construction to the
isolators.
Posted by James Peach <jp...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/69086/
-----------------------------------------------------------
(Updated Nov. 27, 2018, 12:49 a.m.)
Review request for mesos, Gilbert Song, Jason Lai, Jie Yu, and Jiang Yan Xu.
Summary (updated)
-----------------
Moved the container root construction to the isolators.
Bugs: MESOS-9319
https://issues.apache.org/jira/browse/MESOS-9319
Repository: mesos
Description
-------
Previously, if the container was configured with a root filesytem,
the root was populated by a combination of the `fs::chroot:prepare`
API and the various isolators. The implementation details of some
isolators had leaked into the chroot code, which had a special case
for adding GPU devices.
This change moves all the responsibility for defining the
root filesystem from the `fs::chroot::prepare()` API to the
`filesystem/linux` isolator. The `filesystem/linux` isolator is
now the single place that captures how to mount the container
pseudo-filesystems as well as how to construct a proper `/dev`
directory.
Since the `linux/filesystem` isolator is now entirely responsible
for creating and mounting the container `/dev`, any other isolators
that enable access to devices should populate device nodes in the
container devices directory and add a corresponding bind mount.
Diffs (updated)
-----
src/common/protobuf_utils.hpp 1662125ed3e47b179ee32d08c1d3af75553066ba
src/common/protobuf_utils.cpp a45607eed4c4bae5010bcc3f3ffeabd6d911062a
src/linux/fs.hpp 31969f6ba82bf5ee549bfdf9698a21adaa486a90
src/linux/fs.cpp 5cdffe1f4c7f00aee5b8f640e7cfa4a0018cfa0a
src/slave/containerizer/mesos/isolators/filesystem/linux.cpp c7d753ac2e5575a8d687600bfb9e0617fa72c990
src/slave/containerizer/mesos/isolators/gpu/isolator.hpp 4645c625877d9451516133b24bd3959e0f49c0a9
src/slave/containerizer/mesos/isolators/gpu/isolator.cpp 56d835779618fd965d928c6926664583e9141f79
src/slave/containerizer/mesos/isolators/linux/devices.cpp 8f8ff95ec3856ba06647637a80315365d0e66e23
src/slave/containerizer/mesos/launch.cpp 882bcdf89e2b0cca3d3f62e6d017849a51ceaead
Diff: https://reviews.apache.org/r/69086/diff/12/
Changes: https://reviews.apache.org/r/69086/diff/11-12/
Testing
-------
sudo make check (Fedora 28)
Thanks,
James Peach
Re: Review Request 69086: Moved container root construction to the
isolators.
Posted by James Peach <jp...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/69086/
-----------------------------------------------------------
(Updated Nov. 17, 2018, 12:49 a.m.)
Review request for mesos, Gilbert Song, Jason Lai, Jie Yu, and Jiang Yan Xu.
Bugs: MESOS-9319
https://issues.apache.org/jira/browse/MESOS-9319
Repository: mesos
Description
-------
Previously, if the container was configured with a root filesytem,
the root was populated by a combination of the `fs::chroot:prepare`
API and the various isolators. The implementation details of some
isolators had leaked into the chroot code, which had a special case
for adding GPU devices.
This change moves all the responsibility for defining the
root filesystem from the `fs::chroot::prepare()` API to the
`filesystem/linux` isolator. The `filesystem/linux` isolator is
now the single place that captures how to mount the container
pseudo-filesystems as well as how to construct a proper `/dev`
directory.
Since the `linux/filesystem` isolator is now entirely responsible
for creating and mounting the container `/dev`, any other isolators
that enable access to devices should populate device nodes in the
container devices directory and add a corresponding bind mount.
Diffs (updated)
-----
src/linux/fs.hpp 31969f6ba82bf5ee549bfdf9698a21adaa486a90
src/linux/fs.cpp 5cdffe1f4c7f00aee5b8f640e7cfa4a0018cfa0a
src/slave/containerizer/mesos/isolators/filesystem/linux.cpp c7d753ac2e5575a8d687600bfb9e0617fa72c990
src/slave/containerizer/mesos/isolators/gpu/isolator.hpp 4645c625877d9451516133b24bd3959e0f49c0a9
src/slave/containerizer/mesos/isolators/gpu/isolator.cpp 56d835779618fd965d928c6926664583e9141f79
src/slave/containerizer/mesos/isolators/linux/devices.cpp 8f8ff95ec3856ba06647637a80315365d0e66e23
src/slave/containerizer/mesos/launch.cpp 882bcdf89e2b0cca3d3f62e6d017849a51ceaead
Diff: https://reviews.apache.org/r/69086/diff/11/
Changes: https://reviews.apache.org/r/69086/diff/10-11/
Testing
-------
sudo make check (Fedora 28)
Thanks,
James Peach
Re: Review Request 69086: Moved container root construction to the
isolators.
Posted by James Peach <jp...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/69086/
-----------------------------------------------------------
(Updated Nov. 9, 2018, 12:53 a.m.)
Review request for mesos, Gilbert Song, Jason Lai, Jie Yu, and Jiang Yan Xu.
Bugs: MESOS-9319
https://issues.apache.org/jira/browse/MESOS-9319
Repository: mesos
Description
-------
Previously, if the container was configured with a root filesytem,
the root was populated by a combination of the `fs::chroot:prepare`
API and the various isolators. The implementation details of some
isolators had leaked into the chroot code, which had a special case
for adding GPU devices.
This change moves all the responsibility for defining the
root filesystem from the `fs::chroot::prepare()` API to the
`filesystem/linux` isolator. The `filesystem/linux` isolator is
now the single place that captures how to mount the container
pseudo-filesystems as well as how to construct a proper `/dev`
directory.
Since the `linux/filesystem` isolator is now entirely responsible
for creating and mounting the container `/dev`, any other isolators
that enable access to devices should populate device nodes in the
container devices directory and add a corresponding bind mount.
Diffs (updated)
-----
src/linux/fs.hpp 31969f6ba82bf5ee549bfdf9698a21adaa486a90
src/linux/fs.cpp 5cdffe1f4c7f00aee5b8f640e7cfa4a0018cfa0a
src/slave/containerizer/mesos/isolators/filesystem/linux.cpp c7d753ac2e5575a8d687600bfb9e0617fa72c990
src/slave/containerizer/mesos/isolators/gpu/isolator.hpp 4645c625877d9451516133b24bd3959e0f49c0a9
src/slave/containerizer/mesos/isolators/gpu/isolator.cpp 56d835779618fd965d928c6926664583e9141f79
src/slave/containerizer/mesos/isolators/linux/devices.cpp 8f8ff95ec3856ba06647637a80315365d0e66e23
src/slave/containerizer/mesos/launch.cpp 882bcdf89e2b0cca3d3f62e6d017849a51ceaead
Diff: https://reviews.apache.org/r/69086/diff/9/
Changes: https://reviews.apache.org/r/69086/diff/8-9/
Testing
-------
sudo make check (Fedora 28)
Thanks,
James Peach
Re: Review Request 69086: Moved container root construction to the
isolators.
Posted by James Peach <jp...@apache.org>.
> On Nov. 3, 2018, 6:49 p.m., James Peach wrote:
> > src/slave/containerizer/mesos/isolators/filesystem/linux.cpp
> > Lines 146 (patched)
> > <https://reviews.apache.org/r/69086/diff/6/?file=2103846#file2103846line146>
> >
> > This mount fails with user namespaces:
> > ```
> > Failed to prepare mounts: Failed to mount '{"flags":15,"source":"sysfs","target":"/srv/mesos/work/provisioner/containers/ac807664-4109-409e-8cfb-a4285009598b/backends/overlay/rootfses/2b851cdf-08e2-456f-99bd-f5ff5011c678/sys","type":"sysfs"}': Operation not permitted
> > ```
> >
> > I was carrying an out of tree hack for that.
I'll continue to carry an out of tree hack for this.
- James
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/69086/#review210313
-----------------------------------------------------------
On Oct. 30, 2018, 9:03 p.m., James Peach wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/69086/
> -----------------------------------------------------------
>
> (Updated Oct. 30, 2018, 9:03 p.m.)
>
>
> Review request for mesos, Gilbert Song, Jason Lai, Jie Yu, and Jiang Yan Xu.
>
>
> Bugs: MESOS-9319
> https://issues.apache.org/jira/browse/MESOS-9319
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Previously, if the container was configured with a root filesytem,
> the root was populated by a combination of the `fs::chroot:prepare`
> API and the various isolators. The implementation details of some
> isolators had leaked into the chroot code, which had a special case
> for adding GPU devices.
>
> This change moves all the responsibility for defining the
> root filesystem from the `fs::chroot::prepare()` API to the
> `filesystem/linux` isolator. The `filesystem/linux` isolator is
> now the single place that captures how to mount the container
> pseudo-filesystems as well as how to construct a proper `/dev`
> directory.
>
> Since the `linux/filesystem` isolator is now entirely responsible
> for creating and mounting the container `/dev`, any other isolators
> that enable access to devices should populate device nodes in the
> container devices directory and add a corresponding bind mount.
>
>
> Diffs
> -----
>
> src/linux/fs.hpp 31969f6ba82bf5ee549bfdf9698a21adaa486a90
> src/linux/fs.cpp 5cdffe1f4c7f00aee5b8f640e7cfa4a0018cfa0a
> src/slave/containerizer/mesos/isolators/filesystem/linux.cpp c7d753ac2e5575a8d687600bfb9e0617fa72c990
> src/slave/containerizer/mesos/isolators/gpu/isolator.hpp 4645c625877d9451516133b24bd3959e0f49c0a9
> src/slave/containerizer/mesos/isolators/gpu/isolator.cpp 56d835779618fd965d928c6926664583e9141f79
> src/slave/containerizer/mesos/isolators/linux/devices.cpp 8f8ff95ec3856ba06647637a80315365d0e66e23
> src/slave/containerizer/mesos/launch.cpp 7193da0a094df3e441e185c62b3a0379a0bdc4a2
>
>
> Diff: https://reviews.apache.org/r/69086/diff/8/
>
>
> Testing
> -------
>
> sudo make check (Fedora 28)
>
>
> Thanks,
>
> James Peach
>
>
Re: Review Request 69086: Moved container root construction to the
isolators.
Posted by James Peach <jp...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/69086/#review210313
-----------------------------------------------------------
src/slave/containerizer/mesos/isolators/filesystem/linux.cpp
Lines 146 (patched)
<https://reviews.apache.org/r/69086/#comment294959>
This mount fails with user namespaces:
```
Failed to prepare mounts: Failed to mount '{"flags":15,"source":"sysfs","target":"/srv/mesos/work/provisioner/containers/ac807664-4109-409e-8cfb-a4285009598b/backends/overlay/rootfses/2b851cdf-08e2-456f-99bd-f5ff5011c678/sys","type":"sysfs"}': Operation not permitted
```
I was carrying an out of tree hack for that.
- James Peach
On Oct. 30, 2018, 9:03 p.m., James Peach wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/69086/
> -----------------------------------------------------------
>
> (Updated Oct. 30, 2018, 9:03 p.m.)
>
>
> Review request for mesos, Gilbert Song, Jason Lai, Jie Yu, and Jiang Yan Xu.
>
>
> Bugs: MESOS-9319
> https://issues.apache.org/jira/browse/MESOS-9319
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Previously, if the container was configured with a root filesytem,
> the root was populated by a combination of the `fs::chroot:prepare`
> API and the various isolators. The implementation details of some
> isolators had leaked into the chroot code, which had a special case
> for adding GPU devices.
>
> This change moves all the responsibility for defining the
> root filesystem from the `fs::chroot::prepare()` API to the
> `filesystem/linux` isolator. The `filesystem/linux` isolator is
> now the single place that captures how to mount the container
> pseudo-filesystems as well as how to construct a proper `/dev`
> directory.
>
> Since the `linux/filesystem` isolator is now entirely responsible
> for creating and mounting the container `/dev`, any other isolators
> that enable access to devices can simply populate device nodes in
> the container devices directory. After this change, the container
> `/dev` is mounted read-only so that this cannot be used to escape
> any disk quota.
>
>
> Diffs
> -----
>
> src/linux/fs.hpp 31969f6ba82bf5ee549bfdf9698a21adaa486a90
> src/linux/fs.cpp 5cdffe1f4c7f00aee5b8f640e7cfa4a0018cfa0a
> src/slave/containerizer/mesos/isolators/filesystem/linux.cpp c7d753ac2e5575a8d687600bfb9e0617fa72c990
> src/slave/containerizer/mesos/isolators/gpu/isolator.hpp 4645c625877d9451516133b24bd3959e0f49c0a9
> src/slave/containerizer/mesos/isolators/gpu/isolator.cpp 56d835779618fd965d928c6926664583e9141f79
> src/slave/containerizer/mesos/isolators/linux/devices.cpp 8f8ff95ec3856ba06647637a80315365d0e66e23
> src/slave/containerizer/mesos/launch.cpp 7193da0a094df3e441e185c62b3a0379a0bdc4a2
>
>
> Diff: https://reviews.apache.org/r/69086/diff/6/
>
>
> Testing
> -------
>
> sudo make check (Fedora 28)
>
>
> Thanks,
>
> James Peach
>
>
Re: Review Request 69086: Moved container root construction to the
isolators.
Posted by James Peach <jp...@apache.org>.
> On Nov. 5, 2018, 5:14 a.m., Jie Yu wrote:
> > src/slave/containerizer/mesos/isolators/filesystem/linux.cpp
> > Lines 623-631 (patched)
> > <https://reviews.apache.org/r/69086/diff/6/?file=2103846#file2103846line624>
> >
> > I don't think this is needed. `prepareMount` in launch.cpp will actual do this implicitly. Bindly doing rslave will cause shared mount propagation feature to not work (needed by CSI integration)
`chroot::prepare()` currently does this, which is why I kept it here. Maybe it is now OK to drop this since `prepareMounts()` does it better?
- James
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/69086/#review210318
-----------------------------------------------------------
On Nov. 9, 2018, 12:53 a.m., James Peach wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/69086/
> -----------------------------------------------------------
>
> (Updated Nov. 9, 2018, 12:53 a.m.)
>
>
> Review request for mesos, Gilbert Song, Jason Lai, Jie Yu, and Jiang Yan Xu.
>
>
> Bugs: MESOS-9319
> https://issues.apache.org/jira/browse/MESOS-9319
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Previously, if the container was configured with a root filesytem,
> the root was populated by a combination of the `fs::chroot:prepare`
> API and the various isolators. The implementation details of some
> isolators had leaked into the chroot code, which had a special case
> for adding GPU devices.
>
> This change moves all the responsibility for defining the
> root filesystem from the `fs::chroot::prepare()` API to the
> `filesystem/linux` isolator. The `filesystem/linux` isolator is
> now the single place that captures how to mount the container
> pseudo-filesystems as well as how to construct a proper `/dev`
> directory.
>
> Since the `linux/filesystem` isolator is now entirely responsible
> for creating and mounting the container `/dev`, any other isolators
> that enable access to devices should populate device nodes in the
> container devices directory and add a corresponding bind mount.
>
>
> Diffs
> -----
>
> src/linux/fs.hpp 31969f6ba82bf5ee549bfdf9698a21adaa486a90
> src/linux/fs.cpp 5cdffe1f4c7f00aee5b8f640e7cfa4a0018cfa0a
> src/slave/containerizer/mesos/isolators/filesystem/linux.cpp c7d753ac2e5575a8d687600bfb9e0617fa72c990
> src/slave/containerizer/mesos/isolators/gpu/isolator.hpp 4645c625877d9451516133b24bd3959e0f49c0a9
> src/slave/containerizer/mesos/isolators/gpu/isolator.cpp 56d835779618fd965d928c6926664583e9141f79
> src/slave/containerizer/mesos/isolators/linux/devices.cpp 8f8ff95ec3856ba06647637a80315365d0e66e23
> src/slave/containerizer/mesos/launch.cpp 882bcdf89e2b0cca3d3f62e6d017849a51ceaead
>
>
> Diff: https://reviews.apache.org/r/69086/diff/10/
>
>
> Testing
> -------
>
> sudo make check (Fedora 28)
>
>
> Thanks,
>
> James Peach
>
>
Re: Review Request 69086: Moved container root construction to the
isolators.
Posted by James Peach <jp...@apache.org>.
> On Nov. 5, 2018, 5:14 a.m., Jie Yu wrote:
> > src/slave/containerizer/mesos/isolators/filesystem/linux.cpp
> > Lines 73-80 (patched)
> > <https://reviews.apache.org/r/69086/diff/6/?file=2103846#file2103846line73>
> >
> > I would prefer simply use `ContainerMountInfo`, instead of introducing another struct.
Yeh, but you can't do aggregate initialization for protobugs, which means that we can't enumerate a table of mounts for the chroot. I'm open to suggestions here :)
- James
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/69086/#review210318
-----------------------------------------------------------
On Oct. 30, 2018, 9:03 p.m., James Peach wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/69086/
> -----------------------------------------------------------
>
> (Updated Oct. 30, 2018, 9:03 p.m.)
>
>
> Review request for mesos, Gilbert Song, Jason Lai, Jie Yu, and Jiang Yan Xu.
>
>
> Bugs: MESOS-9319
> https://issues.apache.org/jira/browse/MESOS-9319
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Previously, if the container was configured with a root filesytem,
> the root was populated by a combination of the `fs::chroot:prepare`
> API and the various isolators. The implementation details of some
> isolators had leaked into the chroot code, which had a special case
> for adding GPU devices.
>
> This change moves all the responsibility for defining the
> root filesystem from the `fs::chroot::prepare()` API to the
> `filesystem/linux` isolator. The `filesystem/linux` isolator is
> now the single place that captures how to mount the container
> pseudo-filesystems as well as how to construct a proper `/dev`
> directory.
>
> Since the `linux/filesystem` isolator is now entirely responsible
> for creating and mounting the container `/dev`, any other isolators
> that enable access to devices can simply populate device nodes in
> the container devices directory. After this change, the container
> `/dev` is mounted read-only so that this cannot be used to escape
> any disk quota.
>
>
> Diffs
> -----
>
> src/linux/fs.hpp 31969f6ba82bf5ee549bfdf9698a21adaa486a90
> src/linux/fs.cpp 5cdffe1f4c7f00aee5b8f640e7cfa4a0018cfa0a
> src/slave/containerizer/mesos/isolators/filesystem/linux.cpp c7d753ac2e5575a8d687600bfb9e0617fa72c990
> src/slave/containerizer/mesos/isolators/gpu/isolator.hpp 4645c625877d9451516133b24bd3959e0f49c0a9
> src/slave/containerizer/mesos/isolators/gpu/isolator.cpp 56d835779618fd965d928c6926664583e9141f79
> src/slave/containerizer/mesos/isolators/linux/devices.cpp 8f8ff95ec3856ba06647637a80315365d0e66e23
> src/slave/containerizer/mesos/launch.cpp 7193da0a094df3e441e185c62b3a0379a0bdc4a2
>
>
> Diff: https://reviews.apache.org/r/69086/diff/6/
>
>
> Testing
> -------
>
> sudo make check (Fedora 28)
>
>
> Thanks,
>
> James Peach
>
>
Re: Review Request 69086: Moved container root construction to the
isolators.
Posted by Jie Yu <yu...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/69086/#review210318
-----------------------------------------------------------
src/slave/containerizer/mesos/isolators/filesystem/linux.cpp
Lines 73-80 (patched)
<https://reviews.apache.org/r/69086/#comment294976>
I would prefer simply use `ContainerMountInfo`, instead of introducing another struct.
src/slave/containerizer/mesos/isolators/filesystem/linux.cpp
Lines 179 (patched)
<https://reviews.apache.org/r/69086/#comment294977>
You don't need this function if you use `ContainerMountInfo` directly
src/slave/containerizer/mesos/isolators/filesystem/linux.cpp
Lines 181 (patched)
<https://reviews.apache.org/r/69086/#comment294975>
Please fix indentation (2 spaces)
src/slave/containerizer/mesos/isolators/filesystem/linux.cpp
Lines 623-631 (patched)
<https://reviews.apache.org/r/69086/#comment294979>
I don't think this is needed. `prepareMount` in launch.cpp will actual do this implicitly. Bindly doing rslave will cause shared mount propagation feature to not work (needed by CSI integration)
src/slave/containerizer/mesos/launch.cpp
Line 466 (original), 471 (patched)
<https://reviews.apache.org/r/69086/#comment294978>
Do we still need this function? Looks like this is just a verification now. Can you simply move the logic to the main `execute()` method?
- Jie Yu
On Oct. 30, 2018, 9:03 p.m., James Peach wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/69086/
> -----------------------------------------------------------
>
> (Updated Oct. 30, 2018, 9:03 p.m.)
>
>
> Review request for mesos, Gilbert Song, Jason Lai, Jie Yu, and Jiang Yan Xu.
>
>
> Bugs: MESOS-9319
> https://issues.apache.org/jira/browse/MESOS-9319
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Previously, if the container was configured with a root filesytem,
> the root was populated by a combination of the `fs::chroot:prepare`
> API and the various isolators. The implementation details of some
> isolators had leaked into the chroot code, which had a special case
> for adding GPU devices.
>
> This change moves all the responsibility for defining the
> root filesystem from the `fs::chroot::prepare()` API to the
> `filesystem/linux` isolator. The `filesystem/linux` isolator is
> now the single place that captures how to mount the container
> pseudo-filesystems as well as how to construct a proper `/dev`
> directory.
>
> Since the `linux/filesystem` isolator is now entirely responsible
> for creating and mounting the container `/dev`, any other isolators
> that enable access to devices can simply populate device nodes in
> the container devices directory. After this change, the container
> `/dev` is mounted read-only so that this cannot be used to escape
> any disk quota.
>
>
> Diffs
> -----
>
> src/linux/fs.hpp 31969f6ba82bf5ee549bfdf9698a21adaa486a90
> src/linux/fs.cpp 5cdffe1f4c7f00aee5b8f640e7cfa4a0018cfa0a
> src/slave/containerizer/mesos/isolators/filesystem/linux.cpp c7d753ac2e5575a8d687600bfb9e0617fa72c990
> src/slave/containerizer/mesos/isolators/gpu/isolator.hpp 4645c625877d9451516133b24bd3959e0f49c0a9
> src/slave/containerizer/mesos/isolators/gpu/isolator.cpp 56d835779618fd965d928c6926664583e9141f79
> src/slave/containerizer/mesos/isolators/linux/devices.cpp 8f8ff95ec3856ba06647637a80315365d0e66e23
> src/slave/containerizer/mesos/launch.cpp 7193da0a094df3e441e185c62b3a0379a0bdc4a2
>
>
> Diff: https://reviews.apache.org/r/69086/diff/6/
>
>
> Testing
> -------
>
> sudo make check (Fedora 28)
>
>
> Thanks,
>
> James Peach
>
>
Re: Review Request 69086: Moved container root construction to the
isolators.
Posted by James Peach <jp...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/69086/
-----------------------------------------------------------
(Updated Oct. 30, 2018, 9:03 p.m.)
Review request for mesos, Gilbert Song, Jason Lai, Jie Yu, and Jiang Yan Xu.
Bugs: MESOS-9319
https://issues.apache.org/jira/browse/MESOS-9319
Repository: mesos
Description
-------
Previously, if the container was configured with a root filesytem,
the root was populated by a combination of the `fs::chroot:prepare`
API and the various isolators. The implementation details of some
isolators had leaked into the chroot code, which had a special case
for adding GPU devices.
This change moves all the responsibility for defining the
root filesystem from the `fs::chroot::prepare()` API to the
`filesystem/linux` isolator. The `filesystem/linux` isolator is
now the single place that captures how to mount the container
pseudo-filesystems as well as how to construct a proper `/dev`
directory.
Since the `linux/filesystem` isolator is now entirely responsible
for creating and mounting the container `/dev`, any other isolators
that enable access to devices can simply populate device nodes in
the container devices directory. After this change, the container
`/dev` is mounted read-only so that this cannot be used to escape
any disk quota.
Diffs (updated)
-----
src/linux/fs.hpp 31969f6ba82bf5ee549bfdf9698a21adaa486a90
src/linux/fs.cpp 5cdffe1f4c7f00aee5b8f640e7cfa4a0018cfa0a
src/slave/containerizer/mesos/isolators/filesystem/linux.cpp c7d753ac2e5575a8d687600bfb9e0617fa72c990
src/slave/containerizer/mesos/isolators/gpu/isolator.hpp 4645c625877d9451516133b24bd3959e0f49c0a9
src/slave/containerizer/mesos/isolators/gpu/isolator.cpp 56d835779618fd965d928c6926664583e9141f79
src/slave/containerizer/mesos/isolators/linux/devices.cpp 8f8ff95ec3856ba06647637a80315365d0e66e23
src/slave/containerizer/mesos/launch.cpp 7193da0a094df3e441e185c62b3a0379a0bdc4a2
Diff: https://reviews.apache.org/r/69086/diff/5/
Changes: https://reviews.apache.org/r/69086/diff/4-5/
Testing
-------
sudo make check (Fedora 28)
Thanks,
James Peach
Re: Review Request 69086: Moved container root construction to the
isolators.
Posted by James Peach <jp...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/69086/
-----------------------------------------------------------
(Updated Oct. 29, 2018, 11:59 p.m.)
Review request for mesos, Gilbert Song, Jason Lai, Jie Yu, and Jiang Yan Xu.
Summary (updated)
-----------------
Moved container root construction to the isolators.
Bugs: MESOS-9319
https://issues.apache.org/jira/browse/MESOS-9319
Repository: mesos
Description (updated)
-------
Previously, if the container was configured with a root filesytem,
the root was populated by a combination of the `fs::chroot:prepare`
API and the various isolators. The implementation details of some
isolators had leaked into the chroot code, which had a special case
for adding GPU devices.
This change moves all the responsibility for defining the
root filesystem from the `fs::chroot::prepare()` API to the
`filesystem/linux` isolator. The `filesystem/linux` isolator is
now the single place that captures how to mount the container
pseudo-filesystems as well as how to construct a proper `/dev`
directory.
Since the `linux/filesystem` isolator is now entirely responsible
for creating and mounting the container `/dev`, any other isolators
that enable access to devices can simply populate device nodes in
the container devices directory. After this change, the container
`/dev` is mounted read-only so that this cannot be used to escape
any disk quota.
Diffs (updated)
-----
src/linux/fs.hpp 31969f6ba82bf5ee549bfdf9698a21adaa486a90
src/linux/fs.cpp 3a58bf9a44c4e1d454f3d754952705b1fd0a0b1d
src/slave/containerizer/mesos/isolators/filesystem/linux.cpp c7d753ac2e5575a8d687600bfb9e0617fa72c990
src/slave/containerizer/mesos/isolators/gpu/isolator.hpp 4645c625877d9451516133b24bd3959e0f49c0a9
src/slave/containerizer/mesos/isolators/gpu/isolator.cpp 56d835779618fd965d928c6926664583e9141f79
src/slave/containerizer/mesos/isolators/linux/devices.cpp 8f8ff95ec3856ba06647637a80315365d0e66e23
src/slave/containerizer/mesos/launch.cpp 7193da0a094df3e441e185c62b3a0379a0bdc4a2
Diff: https://reviews.apache.org/r/69086/diff/4/
Changes: https://reviews.apache.org/r/69086/diff/3-4/
Testing
-------
sudo make check (Fedora 28)
Thanks,
James Peach
Re: Review Request 69086: Move the container `/dev` construction to
the isolators.
Posted by Mesos Reviewbot Windows <re...@mesos.apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/69086/#review209807
-----------------------------------------------------------
PASS: Mesos patch 69086 was successfully built and tested.
Reviews applied: `['69086']`
All the build artifacts available at: http://dcos-win.westus2.cloudapp.azure.com/artifacts/mesos-reviewbot-testing/2493/mesos-review-69086
- Mesos Reviewbot Windows
On Oct. 19, 2018, 5:38 p.m., James Peach wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/69086/
> -----------------------------------------------------------
>
> (Updated Oct. 19, 2018, 5:38 p.m.)
>
>
> Review request for mesos, Gilbert Song, Jason Lai, Jie Yu, and Jiang Yan Xu.
>
>
> Bugs: MESOS-9319
> https://issues.apache.org/jira/browse/MESOS-9319
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Previously, if the container was configured with a root filesystem,
> the container `/dev` was populated by the chroot API and this API
> had a special case for adding GPU devices. This change extends
> the approach that was introduced in the `linux/devices` isolator
> to construct the whole of the Linux container `/dev` hierarchy
> before launching the container. The `linux/filesystem` isolator is
> now responsible for mounting the container `/dev`, and any other
> isolators that enable access to devices can simply populate device
> nodes in the container devices directory. After this change, the
> container `/dev` is mounted read-only so that this cannot be used
> to escape any disk quota.
>
>
> Diffs
> -----
>
> src/linux/fs.hpp 502f85c4a32d8658bdd701975dd5ac3d802d308e
> src/linux/fs.cpp 9055ef42edd1fb90e1026d1d603a9ba902cfc1fd
> src/slave/containerizer/mesos/isolators/filesystem/linux.cpp a47899cb528eef103f299def3bd3466905ac5b51
> src/slave/containerizer/mesos/isolators/gpu/isolator.hpp 4645c625877d9451516133b24bd3959e0f49c0a9
> src/slave/containerizer/mesos/isolators/gpu/isolator.cpp dbbf92ffbe4a46cedca5b53f6ba172bfb308100e
> src/slave/containerizer/mesos/isolators/linux/devices.cpp 8f8ff95ec3856ba06647637a80315365d0e66e23
> src/slave/containerizer/mesos/launch.cpp 7193da0a094df3e441e185c62b3a0379a0bdc4a2
>
>
> Diff: https://reviews.apache.org/r/69086/diff/3/
>
>
> Testing
> -------
>
> sudo make check (Fedora 28)
>
>
> Thanks,
>
> James Peach
>
>