You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@mesos.apache.org by Artem Harutyunyan <ar...@mesosphere.io> on 2016/01/13 00:48:35 UTC
Re: Review Request 41818: Handled quota when volumes are bind mounted
into the sandbox.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/41818/
-----------------------------------------------------------
(Updated Jan. 12, 2016, 3:48 p.m.)
Review request for mesos, Artem Harutyunyan and Jie Yu.
Summary (updated)
-----------------
Handled quota when volumes are bind mounted into the sandbox.
Bugs: MESOS-4281
https://issues.apache.org/jira/browse/MESOS-4281
Repository: mesos
Description
-------
Handled quota when volumes are bind mounted into the sandbox.
Diffs (updated)
-----
src/slave/containerizer/mesos/isolators/posix/disk.hpp 31808c1e8199fbf2cea36c273860fdbf0a2388f8
src/slave/containerizer/mesos/isolators/posix/disk.cpp 248c34adb63907911d89bed5b1519682a852bb2d
src/tests/containerizer/filesystem_isolator_tests.cpp 5bb85034c22caef64054c1629f6fd55d227e48b1
src/tests/disk_quota_tests.cpp 1577cf71c4d1fbad11e2ec939c4ceae2d5b25f97
Diff: https://reviews.apache.org/r/41818/diff/
Testing
-------
sudo mesos-tests.sh
Thanks,
Artem Harutyunyan
Re: Review Request 41818: Handled quota when volumes are bind mounted
into the sandbox.
Posted by Mesos ReviewBot <re...@mesos.apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/41818/#review114111
-----------------------------------------------------------
Patch looks great!
Reviews applied: [41704, 41705, 41818]
Passed command: export OS=ubuntu:14.04;export CONFIGURATION="--verbose";export COMPILER=gcc; ./support/docker_build.sh
- Mesos ReviewBot
On Jan. 12, 2016, 11:48 p.m., Artem Harutyunyan wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/41818/
> -----------------------------------------------------------
>
> (Updated Jan. 12, 2016, 11:48 p.m.)
>
>
> Review request for mesos, Artem Harutyunyan and Jie Yu.
>
>
> Bugs: MESOS-4281
> https://issues.apache.org/jira/browse/MESOS-4281
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Handled quota when volumes are bind mounted into the sandbox.
>
>
> Diffs
> -----
>
> src/slave/containerizer/mesos/isolators/posix/disk.hpp 31808c1e8199fbf2cea36c273860fdbf0a2388f8
> src/slave/containerizer/mesos/isolators/posix/disk.cpp 248c34adb63907911d89bed5b1519682a852bb2d
> src/tests/containerizer/filesystem_isolator_tests.cpp 5bb85034c22caef64054c1629f6fd55d227e48b1
> src/tests/disk_quota_tests.cpp 1577cf71c4d1fbad11e2ec939c4ceae2d5b25f97
>
> Diff: https://reviews.apache.org/r/41818/diff/
>
>
> Testing
> -------
>
> sudo mesos-tests.sh
>
>
> Thanks,
>
> Artem Harutyunyan
>
>
Re: Review Request 41818: Handled quota when volumes are bind mounted
into the sandbox.
Posted by Mesos ReviewBot <re...@mesos.apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/41818/#review116441
-----------------------------------------------------------
Patch looks great!
Reviews applied: [41704, 41705, 41818]
Passed command: export OS=ubuntu:14.04;export CONFIGURATION="--verbose";export COMPILER=gcc; ./support/docker_build.sh
- Mesos ReviewBot
On Jan. 26, 2016, 6:48 p.m., Artem Harutyunyan wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/41818/
> -----------------------------------------------------------
>
> (Updated Jan. 26, 2016, 6:48 p.m.)
>
>
> Review request for mesos, Artem Harutyunyan and Jie Yu.
>
>
> Bugs: MESOS-4281
> https://issues.apache.org/jira/browse/MESOS-4281
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Handled quota when volumes are bind mounted into the sandbox.
>
>
> Diffs
> -----
>
> src/slave/containerizer/mesos/isolators/posix/disk.hpp 31808c1e8199fbf2cea36c273860fdbf0a2388f8
> src/slave/containerizer/mesos/isolators/posix/disk.cpp 248c34adb63907911d89bed5b1519682a852bb2d
> src/tests/containerizer/filesystem_isolator_tests.cpp 5bb85034c22caef64054c1629f6fd55d227e48b1
> src/tests/disk_quota_tests.cpp 1577cf71c4d1fbad11e2ec939c4ceae2d5b25f97
>
> Diff: https://reviews.apache.org/r/41818/diff/
>
>
> Testing
> -------
>
> sudo mesos-tests.sh
>
>
> Thanks,
>
> Artem Harutyunyan
>
>
Re: Review Request 41818: Handled quota when volumes are bind mounted
into the sandbox.
Posted by Artem Harutyunyan <ar...@mesosphere.io>.
> On Jan. 26, 2016, 12:26 p.m., Jie Yu wrote:
> > src/tests/disk_quota_tests.cpp, lines 144-147
> > <https://reviews.apache.org/r/41818/diff/6/?file=1221677#file1221677line144>
> >
> > This should be wrapped with ifdef linux. I fixed it for you.
Thanks Jie, there is a ticket to make sure we handle excludes correctly on Mac OS https://issues.apache.org/jira/browse/MESOS-4516.
- Artem
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/41818/#review116424
-----------------------------------------------------------
On Jan. 26, 2016, 10:48 a.m., Artem Harutyunyan wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/41818/
> -----------------------------------------------------------
>
> (Updated Jan. 26, 2016, 10:48 a.m.)
>
>
> Review request for mesos, Artem Harutyunyan and Jie Yu.
>
>
> Bugs: MESOS-4281
> https://issues.apache.org/jira/browse/MESOS-4281
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Handled quota when volumes are bind mounted into the sandbox.
>
>
> Diffs
> -----
>
> src/slave/containerizer/mesos/isolators/posix/disk.hpp 31808c1e8199fbf2cea36c273860fdbf0a2388f8
> src/slave/containerizer/mesos/isolators/posix/disk.cpp 248c34adb63907911d89bed5b1519682a852bb2d
> src/tests/containerizer/filesystem_isolator_tests.cpp 5bb85034c22caef64054c1629f6fd55d227e48b1
> src/tests/disk_quota_tests.cpp 1577cf71c4d1fbad11e2ec939c4ceae2d5b25f97
>
> Diff: https://reviews.apache.org/r/41818/diff/
>
>
> Testing
> -------
>
> sudo mesos-tests.sh
>
>
> Thanks,
>
> Artem Harutyunyan
>
>
Re: Review Request 41818: Handled quota when volumes are bind mounted
into the sandbox.
Posted by Jie Yu <yu...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/41818/#review116424
-----------------------------------------------------------
Fix it, then Ship it!
src/tests/disk_quota_tests.cpp (lines 144 - 147)
<https://reviews.apache.org/r/41818/#comment177448>
This should be wrapped with ifdef linux. I fixed it for you.
- Jie Yu
On Jan. 26, 2016, 6:48 p.m., Artem Harutyunyan wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/41818/
> -----------------------------------------------------------
>
> (Updated Jan. 26, 2016, 6:48 p.m.)
>
>
> Review request for mesos, Artem Harutyunyan and Jie Yu.
>
>
> Bugs: MESOS-4281
> https://issues.apache.org/jira/browse/MESOS-4281
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Handled quota when volumes are bind mounted into the sandbox.
>
>
> Diffs
> -----
>
> src/slave/containerizer/mesos/isolators/posix/disk.hpp 31808c1e8199fbf2cea36c273860fdbf0a2388f8
> src/slave/containerizer/mesos/isolators/posix/disk.cpp 248c34adb63907911d89bed5b1519682a852bb2d
> src/tests/containerizer/filesystem_isolator_tests.cpp 5bb85034c22caef64054c1629f6fd55d227e48b1
> src/tests/disk_quota_tests.cpp 1577cf71c4d1fbad11e2ec939c4ceae2d5b25f97
>
> Diff: https://reviews.apache.org/r/41818/diff/
>
>
> Testing
> -------
>
> sudo mesos-tests.sh
>
>
> Thanks,
>
> Artem Harutyunyan
>
>
Re: Review Request 41818: Handled quota when volumes are bind mounted
into the sandbox.
Posted by Artem Harutyunyan <ar...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/41818/
-----------------------------------------------------------
(Updated Jan. 26, 2016, 10:48 a.m.)
Review request for mesos, Artem Harutyunyan and Jie Yu.
Bugs: MESOS-4281
https://issues.apache.org/jira/browse/MESOS-4281
Repository: mesos
Description
-------
Handled quota when volumes are bind mounted into the sandbox.
Diffs (updated)
-----
src/slave/containerizer/mesos/isolators/posix/disk.hpp 31808c1e8199fbf2cea36c273860fdbf0a2388f8
src/slave/containerizer/mesos/isolators/posix/disk.cpp 248c34adb63907911d89bed5b1519682a852bb2d
src/tests/containerizer/filesystem_isolator_tests.cpp 5bb85034c22caef64054c1629f6fd55d227e48b1
src/tests/disk_quota_tests.cpp 1577cf71c4d1fbad11e2ec939c4ceae2d5b25f97
Diff: https://reviews.apache.org/r/41818/diff/
Testing
-------
sudo mesos-tests.sh
Thanks,
Artem Harutyunyan
Re: Review Request 41818: Handled quota when volumes are bind mounted
into the sandbox.
Posted by Artem Harutyunyan <ar...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/41818/
-----------------------------------------------------------
(Updated Jan. 26, 2016, 12:13 a.m.)
Review request for Artem Harutyunyan and Jie Yu.
Bugs: MESOS-4281
https://issues.apache.org/jira/browse/MESOS-4281
Repository: mesos
Description
-------
Handled quota when volumes are bind mounted into the sandbox.
Diffs (updated)
-----
src/slave/containerizer/mesos/isolators/posix/disk.hpp 31808c1e8199fbf2cea36c273860fdbf0a2388f8
src/slave/containerizer/mesos/isolators/posix/disk.cpp 248c34adb63907911d89bed5b1519682a852bb2d
src/tests/containerizer/filesystem_isolator_tests.cpp 5bb85034c22caef64054c1629f6fd55d227e48b1
src/tests/disk_quota_tests.cpp 1577cf71c4d1fbad11e2ec939c4ceae2d5b25f97
Diff: https://reviews.apache.org/r/41818/diff/
Testing
-------
sudo mesos-tests.sh
Thanks,
Artem Harutyunyan
Re: Review Request 41818: Handled quota when volumes are bind mounted
into the sandbox.
Posted by Artem Harutyunyan <ar...@mesosphere.io>.
> On Jan. 25, 2016, 4:36 p.m., Jie Yu wrote:
> > src/slave/containerizer/mesos/isolators/posix/disk.cpp, line 205
> > <https://reviews.apache.org/r/41818/diff/4/?file=1195103#file1195103line205>
> >
> > can you do the following?
> >
> > ```
> > info->paths[path].usage = collector.usage(
> > path,
> > (path == info->directory) ? excludes : {}));
> > ```
`{}` causes a compilation error, but I did re-introduce the ternary operator back.
> On Jan. 25, 2016, 4:36 p.m., Jie Yu wrote:
> > src/slave/containerizer/mesos/isolators/posix/disk.cpp, line 388
> > <https://reviews.apache.org/r/41818/diff/4/?file=1195103#file1195103line388>
> >
> > Please put `{` in a new line.
The body of the constructor is empty again so `{}` will remain on the same line.
> On Jan. 25, 2016, 4:36 p.m., Jie Yu wrote:
> > src/slave/containerizer/mesos/isolators/posix/disk.cpp, lines 393-400
> > <https://reviews.apache.org/r/41818/diff/4/?file=1195103#file1195103line393>
> >
> > You can just do
> > ```
> > foreach (const string& exclude, excludes) {
> > argv.push_back("--exclude");
> > argv.push_back(exclude);
> > }
> > ```
Thanks! This looks much nicer.
> On Jan. 25, 2016, 4:36 p.m., Jie Yu wrote:
> > src/slave/containerizer/mesos/isolators/posix/disk.cpp, line 461
> > <https://reviews.apache.org/r/41818/diff/4/?file=1195103#file1195103line461>
> >
> > You can create the argv first based on 'path' and 'excludes' in Entry.
I am not sure I understand this one.
- Artem
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/41818/#review116224
-----------------------------------------------------------
On Jan. 26, 2016, 12:13 a.m., Artem Harutyunyan wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/41818/
> -----------------------------------------------------------
>
> (Updated Jan. 26, 2016, 12:13 a.m.)
>
>
> Review request for Artem Harutyunyan and Jie Yu.
>
>
> Bugs: MESOS-4281
> https://issues.apache.org/jira/browse/MESOS-4281
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Handled quota when volumes are bind mounted into the sandbox.
>
>
> Diffs
> -----
>
> src/slave/containerizer/mesos/isolators/posix/disk.hpp 31808c1e8199fbf2cea36c273860fdbf0a2388f8
> src/slave/containerizer/mesos/isolators/posix/disk.cpp 248c34adb63907911d89bed5b1519682a852bb2d
> src/tests/containerizer/filesystem_isolator_tests.cpp 5bb85034c22caef64054c1629f6fd55d227e48b1
> src/tests/disk_quota_tests.cpp 1577cf71c4d1fbad11e2ec939c4ceae2d5b25f97
>
> Diff: https://reviews.apache.org/r/41818/diff/
>
>
> Testing
> -------
>
> sudo mesos-tests.sh
>
>
> Thanks,
>
> Artem Harutyunyan
>
>
Re: Review Request 41818: Handled quota when volumes are bind mounted
into the sandbox.
Posted by Artem Harutyunyan <ar...@mesosphere.io>.
> On Jan. 25, 2016, 4:36 p.m., Jie Yu wrote:
> > src/tests/disk_quota_tests.cpp, lines 132-133
> > <https://reviews.apache.org/r/41818/diff/4/?file=1195105#file1195105line132>
> >
> > Can you add a simple DiskUsageCollectorTest here to test the exclude path?
`du` excludes symlinks anyway, so what exactly do we want to test here?
- Artem
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/41818/#review116224
-----------------------------------------------------------
On Jan. 26, 2016, 12:13 a.m., Artem Harutyunyan wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/41818/
> -----------------------------------------------------------
>
> (Updated Jan. 26, 2016, 12:13 a.m.)
>
>
> Review request for Artem Harutyunyan and Jie Yu.
>
>
> Bugs: MESOS-4281
> https://issues.apache.org/jira/browse/MESOS-4281
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Handled quota when volumes are bind mounted into the sandbox.
>
>
> Diffs
> -----
>
> src/slave/containerizer/mesos/isolators/posix/disk.hpp 31808c1e8199fbf2cea36c273860fdbf0a2388f8
> src/slave/containerizer/mesos/isolators/posix/disk.cpp 248c34adb63907911d89bed5b1519682a852bb2d
> src/tests/containerizer/filesystem_isolator_tests.cpp 5bb85034c22caef64054c1629f6fd55d227e48b1
> src/tests/disk_quota_tests.cpp 1577cf71c4d1fbad11e2ec939c4ceae2d5b25f97
>
> Diff: https://reviews.apache.org/r/41818/diff/
>
>
> Testing
> -------
>
> sudo mesos-tests.sh
>
>
> Thanks,
>
> Artem Harutyunyan
>
>
Re: Review Request 41818: Handled quota when volumes are bind mounted
into the sandbox.
Posted by Jie Yu <yu...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/41818/#review116224
-----------------------------------------------------------
src/slave/containerizer/mesos/isolators/posix/disk.cpp (line 205)
<https://reviews.apache.org/r/41818/#comment177229>
can you do the following?
```
info->paths[path].usage = collector.usage(
path,
(path == info->directory) ? excludes : {}));
```
src/slave/containerizer/mesos/isolators/posix/disk.cpp (lines 206 - 211)
<https://reviews.apache.org/r/41818/#comment177230>
The indentation here should be 2 because it's not method parameter.
src/slave/containerizer/mesos/isolators/posix/disk.cpp
<https://reviews.apache.org/r/41818/#comment177231>
Why removing this line?
src/slave/containerizer/mesos/isolators/posix/disk.cpp (line 386)
<https://reviews.apache.org/r/41818/#comment177233>
We typically put ':' in the next line.
src/slave/containerizer/mesos/isolators/posix/disk.cpp (lines 386 - 404)
<https://reviews.apache.org/r/41818/#comment177235>
I find it more intuitive to construct the command while creating the subprocess. Can you move the code there?
src/slave/containerizer/mesos/isolators/posix/disk.cpp (line 387)
<https://reviews.apache.org/r/41818/#comment177234>
Please put `{` in a new line.
src/slave/containerizer/mesos/isolators/posix/disk.cpp (line 391)
<https://reviews.apache.org/r/41818/#comment177226>
typo.
src/slave/containerizer/mesos/isolators/posix/disk.cpp (lines 392 - 399)
<https://reviews.apache.org/r/41818/#comment177238>
You can just do
```
foreach (const string& exclude, excludes) {
argv.push_back("--exclude");
argv.push_back(exclude);
}
```
src/slave/containerizer/mesos/isolators/posix/disk.cpp (line 460)
<https://reviews.apache.org/r/41818/#comment177236>
You can create the argv first based on 'path' and 'excludes' in Entry.
src/tests/containerizer/filesystem_isolator_tests.cpp (line 1065)
<https://reviews.apache.org/r/41818/#comment177240>
Please add a short description about what this test is doing?
src/tests/containerizer/filesystem_isolator_tests.cpp (line 1120)
<https://reviews.apache.org/r/41818/#comment177242>
Why the sleep here? This will slowdown this test by 1 seconds. If it's needed, definitely need some comments here.
src/tests/disk_quota_tests.cpp (lines 132 - 133)
<https://reviews.apache.org/r/41818/#comment177239>
Can you add a simple DiskUsageCollectorTest here to test the exclude path?
- Jie Yu
On Jan. 12, 2016, 11:48 p.m., Artem Harutyunyan wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/41818/
> -----------------------------------------------------------
>
> (Updated Jan. 12, 2016, 11:48 p.m.)
>
>
> Review request for mesos, Artem Harutyunyan and Jie Yu.
>
>
> Bugs: MESOS-4281
> https://issues.apache.org/jira/browse/MESOS-4281
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Handled quota when volumes are bind mounted into the sandbox.
>
>
> Diffs
> -----
>
> src/slave/containerizer/mesos/isolators/posix/disk.hpp 31808c1e8199fbf2cea36c273860fdbf0a2388f8
> src/slave/containerizer/mesos/isolators/posix/disk.cpp 248c34adb63907911d89bed5b1519682a852bb2d
> src/tests/containerizer/filesystem_isolator_tests.cpp 5bb85034c22caef64054c1629f6fd55d227e48b1
> src/tests/disk_quota_tests.cpp 1577cf71c4d1fbad11e2ec939c4ceae2d5b25f97
>
> Diff: https://reviews.apache.org/r/41818/diff/
>
>
> Testing
> -------
>
> sudo mesos-tests.sh
>
>
> Thanks,
>
> Artem Harutyunyan
>
>