Re: Review Request 36389: [WIP] Enable remote execution of arbitrary command.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36389/#review91278 --- Patch looks great! Reviews applied: [36389] All tests passed. - Mesos ReviewBot On July 10, 2015, 7:59 a.m., Marco Massenzio wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36389/ --- (Updated July 10, 2015, 7:59 a.m.) Review request for mesos, Benjamin Hindman and Cody Maloney. Bugs: MESOS-2830 https://issues.apache.org/jira/browse/MESOS-2830 Repository: mesos Description --- Jira: MESOS-2830 Under certain (maintenance) circumnstance, it may be necessary (or desirable) to execute arbitrary operator's commands on the slave (the entire fleet or a subset thereof) bypassing the Mesos Task execution mechanism; this may typically be necessary for maintenance and/or emergency actions. This patch adds an HTTP endpoint (/execute) which accepts a JSON-encoded CommandInfo structure and executes the given command (with optional arguments). The output of the command (along with potentially any stderr messages) is returned JSON-encoded in the Response. For more details, see the design doc at: https://goo.gl/4npTMU Diffs - src/slave/slave.hpp dec4ca8323e151a6d0f9139214ff0ef6e3e3375a src/slave/slave.cpp 2119b5176aa7cfb7b0b551d4d4f65ee12818b9e4 Diff: https://reviews.apache.org/r/36389/diff/ Testing --- make check lots of manual testing (using Postman, REST client) Thanks, Marco Massenzio
Re: Review Request 36197: Documented how to become a committer.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36197/ --- (Updated July 10, 2015, 2:14 a.m.) Review request for mesos, Benjamin Hindman, Ben Mahler, and Vinod Kone. Changes --- Removed unanimous from the sentence about PMC election. Bugs: MESOS-1825 https://issues.apache.org/jira/browse/MESOS-1825 Repository: mesos Description --- Added new document committer-candidate-checklist.md and wrote a paragraph about the path to committership in committers.md. Diffs (updated) - docs/committer-candidate-checklist.md PRE-CREATION docs/committers.md ca8a6995c5272f3534ab63f95332565dfcaaf5b9 Diff: https://reviews.apache.org/r/36197/diff/ Testing --- The rendered files can be viewed here: https://gist.github.com/bernd-mesos/00de63ae13efec4331be Thanks, Bernd Mathiske
Re: Review Request 36360: Adding common constants for HTTP API
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36360/#review91280 --- src/common/http.hpp (lines 52 - 53) https://reviews.apache.org/r/36360/#comment144572 Let's convert them to static functions as per https://issues.apache.org/jira/browse/MESOS-1023. E.g. see https://reviews.apache.org/r/30841/. - Alexander Rukletsov On July 9, 2015, 10:34 p.m., Isabel Jimenez wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36360/ --- (Updated July 9, 2015, 10:34 p.m.) Review request for mesos, Anand Mazumdar, Benjamin Hindman, Ben Mahler, Marco Massenzio, and Vinod Kone. Bugs: MESOS-2860 https://issues.apache.org/jira/browse/MESOS-2860 Repository: mesos-incubating Description --- Adding constants used commonly through the different HTTP endpoints Diffs - src/common/http.hpp bbd063d src/common/http.cpp 73a4de1 Diff: https://reviews.apache.org/r/36360/diff/ Testing --- Thanks, Isabel Jimenez
Re: Review Request 36197: Documented how to become a committer.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36197/#review91282 --- Patch looks great! Reviews applied: [36197] All tests passed. - Mesos ReviewBot On July 10, 2015, 9:14 a.m., Bernd Mathiske wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36197/ --- (Updated July 10, 2015, 9:14 a.m.) Review request for mesos, Benjamin Hindman, Ben Mahler, and Vinod Kone. Bugs: MESOS-1825 https://issues.apache.org/jira/browse/MESOS-1825 Repository: mesos Description --- Added new document committer-candidate-checklist.md and wrote a paragraph about the path to committership in committers.md. Diffs - docs/committer-candidate-checklist.md PRE-CREATION docs/committers.md ca8a6995c5272f3534ab63f95332565dfcaaf5b9 Diff: https://reviews.apache.org/r/36197/diff/ Testing --- The rendered files can be viewed here: https://gist.github.com/bernd-mesos/00de63ae13efec4331be Thanks, Bernd Mathiske
Review Request 36411: Used low cpu.shares for revocable containers.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36411/ --- Review request for mesos, Ian Downes and Vinod Kone. Bugs: MESOS-2652 https://issues.apache.org/jira/browse/MESOS-2652 Repository: mesos Description --- Used low cpu.shares for revocable containers. Diffs - src/slave/containerizer/isolators/cgroups/constants.hpp e6df4a29e9af8076d6454740afa61fce04c3d06b src/slave/containerizer/isolators/cgroups/cpushare.cpp f56e97dcf91a6f5c9a8abe4afe9dc1a1d47df330 Diff: https://reviews.apache.org/r/36411/diff/ Testing --- make check Thanks, Jie Yu
Re: Review Request 34135: Add filesystem/ isolators for persistent volumes.
On June 25, 2015, 12:47 p.m., Jie Yu wrote: src/slave/containerizer/isolators/filesystem/linux.cpp, lines 406-407 https://reviews.apache.org/r/34135/diff/2/?file=989747#file989747line406 Do you need to umount persistent volumes as well? As the comment states, it notably this includes persistent volume mounts :-) On June 25, 2015, 12:47 p.m., Jie Yu wrote: src/slave/containerizer/mesos/containerizer.cpp, lines 158-166 https://reviews.apache.org/r/34135/diff/2/?file=989751#file989751line158 Hum... why this logic is under 'if (ModuleManager::containsIsolator(type)' ? Good catch! No idea why it was there in the first place and why I perpetuated the erroneous code. - Ian --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/34135/#review84775 --- On June 22, 2015, 9:41 a.m., Ian Downes wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/34135/ --- (Updated June 22, 2015, 9:41 a.m.) Review request for mesos, Chi Zhang, Paul Brett, Timothy Chen, and Vinod Kone. Repository: mesos Description --- Moved code from Mesos Containerizer to filesystem isolators - filesystem/posix (symlinks, doesn't support container rootfs) - filesystem/linux (bind mounts, does support container rootfs) The filesystem/posix isolator will be automatically included if no filesystem/ isolator is specified. Diffs - src/Makefile.am e7de0f3d1a5efeaef47d5074defe3b40db94f573 src/slave/containerizer/isolators/filesystem/linux.hpp PRE-CREATION src/slave/containerizer/isolators/filesystem/linux.cpp PRE-CREATION src/slave/containerizer/isolators/filesystem/posix.hpp PRE-CREATION src/slave/containerizer/isolators/filesystem/posix.cpp PRE-CREATION src/slave/containerizer/linux_launcher.cpp 8eae258d81229e19f8c587f5e023b1df7deed025 src/slave/containerizer/mesos/containerizer.cpp 8c102fb7d1f79ee768cb06de3a976ea12f958712 Diff: https://reviews.apache.org/r/34135/diff/ Testing --- existing persistent volumes tests. Thanks, Ian Downes
Re: Review Request 34137: Add support for container image provisioners.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/34137/ --- (Updated July 10, 2015, 5:05 p.m.) Review request for mesos, Chi Zhang, Paul Brett, Timothy Chen, and Vinod Kone. Changes --- Addressed comments. Repository: mesos Description --- The MesosContainerizer can optionally provision a root filesystem for the containers. Diffs - include/mesos/slave/isolator.hpp 18edc030367e42240090f2f3dbc92aec7a4c6234 src/Makefile.am e7de0f3d1a5efeaef47d5074defe3b40db94f573 src/slave/containerizer/mesos/containerizer.hpp 3ac2387eabded61c897a5016655aae10cd1bca91 src/slave/containerizer/mesos/containerizer.cpp 8c102fb7d1f79ee768cb06de3a976ea12f958712 src/slave/containerizer/provisioner.hpp PRE-CREATION src/slave/containerizer/provisioner.cpp PRE-CREATION src/slave/flags.hpp 7634e368c72e83932dcd992d78eaca146326606b src/slave/flags.cpp cbf431eb0627bdaf07241cc0fc4630df06fb20e2 src/slave/paths.hpp 00476f107ed8233123a6eab0925c9f28aac0a86f src/slave/paths.cpp 0741616b656e947cb460dd6ee6a9a4852be001c2 src/slave/state.hpp fed4b7ecf9572a8dbb1a99dbb1769d3e55ef7bc5 src/slave/state.cpp 8eda22a550e5add0f84c46cc2ed762b006c0dcec src/tests/containerizer_tests.cpp 0cdb2d2a3f19a4835e85c6b040759019b03f051e Diff: https://reviews.apache.org/r/34137/diff/ Testing --- Thanks, Ian Downes
Re: Review Request 34135: Add filesystem/ isolators for persistent volumes.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/34135/ --- (Updated July 10, 2015, 5:05 p.m.) Review request for mesos, Chi Zhang, Paul Brett, Timothy Chen, and Vinod Kone. Repository: mesos Description --- Moved code from Mesos Containerizer to filesystem isolators - filesystem/posix (symlinks, doesn't support container rootfs) - filesystem/linux (bind mounts, does support container rootfs) The filesystem/posix isolator will be automatically included if no filesystem/ isolator is specified. Diffs - src/Makefile.am e7de0f3d1a5efeaef47d5074defe3b40db94f573 src/slave/containerizer/isolators/filesystem/linux.hpp PRE-CREATION src/slave/containerizer/isolators/filesystem/linux.cpp PRE-CREATION src/slave/containerizer/isolators/filesystem/posix.hpp PRE-CREATION src/slave/containerizer/isolators/filesystem/posix.cpp PRE-CREATION src/slave/containerizer/linux_launcher.cpp 8eae258d81229e19f8c587f5e023b1df7deed025 src/slave/containerizer/mesos/containerizer.cpp 8c102fb7d1f79ee768cb06de3a976ea12f958712 Diff: https://reviews.apache.org/r/34135/diff/ Testing --- existing persistent volumes tests. Thanks, Ian Downes
Re: Review Request 36326: containerizer: added cgroups based statistics.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36326/#review91372 --- Patch looks great! Reviews applied: [36106, 36326] All tests passed. - Mesos ReviewBot On July 10, 2015, 11:03 p.m., Jojy Varghese wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36326/ --- (Updated July 10, 2015, 11:03 p.m.) Review request for mesos and Timothy Chen. Bugs: MESOS-2951 https://issues.apache.org/jira/browse/MESOS-2951 Repository: mesos Description --- Adding cgroups cpustat and memory statistics as preferred way to get usage statistics for containerizers. Jira: MESOS-2951 Diffs - src/slave/containerizer/docker.hpp 646a277be21deaded47324bea5474bd68699f25f src/slave/containerizer/docker.cpp 6eb1c84b1a6ef17c3e2ea2028e2f2d75110176ba Diff: https://reviews.apache.org/r/36326/diff/ Testing --- make check Thanks, Jojy Varghese
Re: Review Request 34135: Add filesystem/ isolators for persistent volumes.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/34135/#review91378 --- Bad patch! Reviews applied: [34908, 34136, 34137] Failed command: ./support/apply-review.sh -n -r 34137 Error: 2015-07-11 00:17:12 URL:https://reviews.apache.org/r/34137/diff/raw/ [27315/27315] - 34137.patch [1] error: patch failed: include/mesos/slave/isolator.hpp:30 error: include/mesos/slave/isolator.hpp: patch does not apply error: patch failed: src/Makefile.am:525 error: src/Makefile.am: patch does not apply error: patch failed: src/slave/containerizer/mesos/containerizer.cpp:548 error: src/slave/containerizer/mesos/containerizer.cpp: patch does not apply Failed to apply patch - Mesos ReviewBot On July 11, 2015, 12:05 a.m., Ian Downes wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/34135/ --- (Updated July 11, 2015, 12:05 a.m.) Review request for mesos, Chi Zhang, Paul Brett, Timothy Chen, and Vinod Kone. Repository: mesos Description --- Moved code from Mesos Containerizer to filesystem isolators - filesystem/posix (symlinks, doesn't support container rootfs) - filesystem/linux (bind mounts, does support container rootfs) The filesystem/posix isolator will be automatically included if no filesystem/ isolator is specified. Diffs - src/Makefile.am e7de0f3d1a5efeaef47d5074defe3b40db94f573 src/slave/containerizer/isolators/filesystem/linux.hpp PRE-CREATION src/slave/containerizer/isolators/filesystem/linux.cpp PRE-CREATION src/slave/containerizer/isolators/filesystem/posix.hpp PRE-CREATION src/slave/containerizer/isolators/filesystem/posix.cpp PRE-CREATION src/slave/containerizer/linux_launcher.cpp 8eae258d81229e19f8c587f5e023b1df7deed025 src/slave/containerizer/mesos/containerizer.cpp 8c102fb7d1f79ee768cb06de3a976ea12f958712 Diff: https://reviews.apache.org/r/34135/diff/ Testing --- existing persistent volumes tests. Thanks, Ian Downes
Re: Review Request 36413: Removed the code of setting SCHED_IDLE policy for revocable containers.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36413/#review91380 --- Patch looks great! Reviews applied: [36410, 36411, 36412, 36413] All tests passed. - Mesos ReviewBot On July 11, 2015, 12:09 a.m., Jie Yu wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36413/ --- (Updated July 11, 2015, 12:09 a.m.) Review request for mesos, Ian Downes and Vinod Kone. Bugs: MESOS-2652 https://issues.apache.org/jira/browse/MESOS-2652 Repository: mesos Description --- Removed the code of setting SCHED_IDLE policy for revocable containers. Diffs - src/slave/containerizer/isolators/cgroups/cpushare.cpp f56e97dcf91a6f5c9a8abe4afe9dc1a1d47df330 Diff: https://reviews.apache.org/r/36413/diff/ Testing --- sudo make check Thanks, Jie Yu
Review Request 36410: Changed the MIN_CPU_SHARES to match the kernel constant.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36410/ --- Review request for mesos, Ian Downes and Vinod Kone. Bugs: MESOS-2652 https://issues.apache.org/jira/browse/MESOS-2652 Repository: mesos Description --- Changed the MIN_CPU_SHARES to match the kernel constant. Diffs - src/slave/containerizer/isolators/cgroups/constants.hpp e6df4a29e9af8076d6454740afa61fce04c3d06b Diff: https://reviews.apache.org/r/36410/diff/ Testing --- make check Thanks, Jie Yu
Re: Review Request 36326: containerizer: added cgroups based statistics.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36326/#review91376 --- src/slave/containerizer/docker.cpp (line 1325) https://reviews.apache.org/r/36326/#comment144747 We also have a unit test in docker_containerizer_tests testing the usage call, can you see if it makes sense to add something to test the rss data as well? - Timothy Chen On July 10, 2015, 11:03 p.m., Jojy Varghese wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36326/ --- (Updated July 10, 2015, 11:03 p.m.) Review request for mesos and Timothy Chen. Bugs: MESOS-2951 https://issues.apache.org/jira/browse/MESOS-2951 Repository: mesos Description --- Adding cgroups cpustat and memory statistics as preferred way to get usage statistics for containerizers. Jira: MESOS-2951 Diffs - src/slave/containerizer/docker.hpp 646a277be21deaded47324bea5474bd68699f25f src/slave/containerizer/docker.cpp 6eb1c84b1a6ef17c3e2ea2028e2f2d75110176ba Diff: https://reviews.apache.org/r/36326/diff/ Testing --- make check Thanks, Jojy Varghese
Review Request 36412: Adjusted the revocable cpu isolator test.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36412/ --- Review request for mesos, Ian Downes and Vinod Kone. Bugs: MESOS-2652 https://issues.apache.org/jira/browse/MESOS-2652 Repository: mesos Description --- Adjusted the revocable cpu isolator test. Diffs - src/tests/isolator_tests.cpp 8eb732fe90dbab11967e663a5c06890f79463488 Diff: https://reviews.apache.org/r/36412/diff/ Testing --- sudo make check Thanks, Jie Yu
Re: Review Request 36378: Refactor Linux Performance monitor to handle changing 'perf stat' output versions depending on kernel version.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36378/#review91253 --- src/linux/perf.cpp (line 449) https://reviews.apache.org/r/36378/#comment144639 Why a Try argument and then a CHECK()? The caller of os::release() should check the return so either check it in supported() or query and check it in _supported(). Also, should be const Version. src/linux/perf.cpp (lines 468 - 473) https://reviews.apache.org/r/36378/#comment144642 We should document this behavior at os::release() and move this there as an alternative. If it's kept in here then make static? Argument should be named 'version', not 'v'. Does it need to have such a long function name, particularly if it's local? Version canonicalize(const Version version). src/linux/perf.cpp (line 481) https://reviews.apache.org/r/36378/#comment144544 Suggest moving the TODO to here. src/linux/perf.cpp (lines 484 - 485) https://reviews.apache.org/r/36378/#comment144545 There's no map, it returns a single Sample? src/linux/perf.cpp (lines 486 - 488) https://reviews.apache.org/r/36378/#comment144645 This fits on a single line. src/linux/perf.cpp (lines 490 - 492) https://reviews.apache.org/r/36378/#comment144546 newlines, please ;-) src/linux/perf.cpp (line 492) https://reviews.apache.org/r/36378/#comment144648 Can you provide links to the documentation/commits that introduced these changes? src/linux/perf.cpp (line 508) https://reviews.apache.org/r/36378/#comment144549 Check how patch versions are handled is correct, e.g., this would match 3.12.1 (if it existed) when I think the change was in 3.13.0 so it should be = 3.13.0 src/linux/perf.cpp (line 524) https://reviews.apache.org/r/36378/#comment144649 newline src/linux/perf.cpp (line 532) https://reviews.apache.org/r/36378/#comment144651 const T src/linux/perf.cpp (line 536) https://reviews.apache.org/r/36378/#comment144550 newline - Ian Downes On July 9, 2015, 4:08 p.m., Paul Brett wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36378/ --- (Updated July 9, 2015, 4:08 p.m.) Review request for mesos, Ben Mahler, Chi Zhang, Ian Downes, and Cong Wang. Bugs: MESOS-2834 https://issues.apache.org/jira/browse/MESOS-2834 Repository: mesos Description --- Refactor Linux Performance monitor to handle changing 'perf stat' output versions depending on kernel version. Diffs - src/linux/perf.cpp 697b75e846a43d4f106ad8f39a18882836d7dc02 Diff: https://reviews.apache.org/r/36378/diff/ Testing --- sudo make check Thanks, Paul Brett
Re: Review Request 36378: Refactor Linux Performance monitor to handle changing 'perf stat' output versions depending on kernel version.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36378/ --- (Updated July 10, 2015, 5:33 p.m.) Review request for mesos, Ben Mahler, Chi Zhang, Ian Downes, and Cong Wang. Bugs: MESOS-2834 https://issues.apache.org/jira/browse/MESOS-2834 Repository: mesos Description --- Refactor Linux Performance monitor to handle changing 'perf stat' output versions depending on kernel version. Diffs (updated) - src/linux/perf.cpp 697b75e846a43d4f106ad8f39a18882836d7dc02 Diff: https://reviews.apache.org/r/36378/diff/ Testing --- sudo make check Thanks, Paul Brett
Re: Review Request 36378: Refactor Linux Performance monitor to handle changing 'perf stat' output versions depending on kernel version.
On July 10, 2015, 5:25 p.m., Ian Downes wrote: src/linux/perf.cpp, lines 469-474 https://reviews.apache.org/r/36378/diff/1/?file=1004593#file1004593line469 We should document this behavior at os::release() and move this there as an alternative. If it's kept in here then make static? Argument should be named 'version', not 'v'. Does it need to have such a long function name, particularly if it's local? Version canonicalize(const Version version). MESOS-3029 raised to move function to stout, but the solution is not that obvious since we could (1) modify os::release to return the canonical version but then it would not match uname which would be an issue if you used it to try and find something like a kernel module or (2) provide a separate function which returns canonical release but then there is more potential for using the wrong one or (3) provide version with a custom sort order so that the returned string is as reported by uname but Version(3.0.0) sorts as less that Version(2.6.41). - Paul --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36378/#review91253 --- On July 10, 2015, 5:33 p.m., Paul Brett wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36378/ --- (Updated July 10, 2015, 5:33 p.m.) Review request for mesos, Ben Mahler, Chi Zhang, Ian Downes, and Cong Wang. Bugs: MESOS-2834 https://issues.apache.org/jira/browse/MESOS-2834 Repository: mesos Description --- Refactor Linux Performance monitor to handle changing 'perf stat' output versions depending on kernel version. Diffs - src/linux/perf.cpp 697b75e846a43d4f106ad8f39a18882836d7dc02 Diff: https://reviews.apache.org/r/36378/diff/ Testing --- sudo make check Thanks, Paul Brett
Re: Review Request 36378: Refactor Linux Performance monitor to handle changing 'perf stat' output versions depending on kernel version.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36378/ --- (Updated July 10, 2015, 6:16 p.m.) Review request for mesos, Ben Mahler, Chi Zhang, Ian Downes, and Cong Wang. Changes --- Addressed review comments. Bugs: MESOS-2834 https://issues.apache.org/jira/browse/MESOS-2834 Repository: mesos Description --- Refactor Linux Performance monitor to handle changing 'perf stat' output versions depending on kernel version. Diffs (updated) - src/linux/perf.cpp 697b75e846a43d4f106ad8f39a18882836d7dc02 Diff: https://reviews.apache.org/r/36378/diff/ Testing --- sudo make check Thanks, Paul Brett
Re: Review Request 36378: Refactor Linux Performance monitor to handle changing 'perf stat' output versions depending on kernel version.
On July 10, 2015, 5:25 p.m., Ian Downes wrote: src/linux/perf.cpp, line 482 https://reviews.apache.org/r/36378/diff/1/?file=1004593#file1004593line482 Suggest moving the TODO to here. I'd like to keep it with the definition since usage is in multiple locations in the file. - Paul --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36378/#review91253 --- On July 10, 2015, 8:48 p.m., Paul Brett wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36378/ --- (Updated July 10, 2015, 8:48 p.m.) Review request for mesos, Ben Mahler, Chi Zhang, Ian Downes, and Cong Wang. Bugs: MESOS-2834 https://issues.apache.org/jira/browse/MESOS-2834 Repository: mesos Description --- Refactor Linux Performance monitor to handle changing 'perf stat' output versions depending on kernel version. Diffs - src/linux/perf.cpp 697b75e846a43d4f106ad8f39a18882836d7dc02 Diff: https://reviews.apache.org/r/36378/diff/ Testing --- sudo make check Thanks, Paul Brett
Re: Review Request 36380: Test cases for performance monitor support of multiple output versions depending on kernel version.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36380/#review91351 --- Patch looks great! Reviews applied: [36378, 36380] All tests passed. - Mesos ReviewBot On July 10, 2015, 8:52 p.m., Paul Brett wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36380/ --- (Updated July 10, 2015, 8:52 p.m.) Review request for mesos, Ben Mahler, Chi Zhang, Ian Downes, and Cong Wang. Bugs: MESOS-2834 https://issues.apache.org/jira/browse/MESOS-2834 Repository: mesos Description --- Test cases for performance monitor support of multiple output versions depending on kernel version. Diffs - src/tests/perf_tests.cpp 281eed0094faead67dc7f84df6407686aae88b01 Diff: https://reviews.apache.org/r/36380/diff/ Testing --- sudo make check Thanks, Paul Brett
Re: Review Request 36380: Test cases for performance monitor support of multiple output versions depending on kernel version.
---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/36380/#review91346
---
src/tests/perf_tests.cpp (line 52)
https://reviews.apache.org/r/36380/#comment144705
split args onto separate lines
src/tests/perf_tests.cpp (line 62)
https://reviews.apache.org/r/36380/#comment144710
lines? it's 1 or more, how about just calling input?
is debug() a better name for this function?
src/tests/perf_tests.cpp (lines 65 - 69)
https://reviews.apache.org/r/36380/#comment144709
use a ternary if here?
```cpp
s (version.isError() ? Error: + version.error()
: version.get());
```
src/tests/perf_tests.cpp (line 104)
https://reviews.apache.org/r/36380/#comment144706
I don't think we use this, favoring
```cpp
foreach (const tupleVersion, string, input)
```
Is Version hashable? If so, iterating over a Version - input would be much
cleaner
```cpp
foreachpair(const Version version, const string input, inputs)
```
Or, just use the string version and parse the string version inside the
loop?
```cpp
hashmapstring, string input {
2.6.39, 123,cycles\n0.123,task-clock
}
```
src/tests/perf_tests.cpp (line 157)
https://reviews.apache.org/r/36380/#comment144714
Do you need to keep all the logging of the context on failed expectations
after the test has been correctly written? It clutters the code...
src/tests/perf_tests.cpp (line 191)
https://reviews.apache.org/r/36380/#comment144711
ditto
src/tests/perf_tests.cpp (line 226)
https://reviews.apache.org/r/36380/#comment144712
ditto
- Ian Downes
On July 10, 2015, 1:52 p.m., Paul Brett wrote:
---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/36380/
---
(Updated July 10, 2015, 1:52 p.m.)
Review request for mesos, Ben Mahler, Chi Zhang, Ian Downes, and Cong Wang.
Bugs: MESOS-2834
https://issues.apache.org/jira/browse/MESOS-2834
Repository: mesos
Description
---
Test cases for performance monitor support of multiple output versions
depending on kernel version.
Diffs
-
src/tests/perf_tests.cpp 281eed0094faead67dc7f84df6407686aae88b01
Diff: https://reviews.apache.org/r/36380/diff/
Testing
---
sudo make check
Thanks,
Paul Brett
Re: Review Request 35927: Added TaskStatus::Reason to Termination Message.
On June 29, 2015, 7:40 p.m., Jie Yu wrote: Thanks Joerg! See my detailed comments. I suggest you take a look at the discussion in https://reviews.apache.org/r/34720/ first. Hi Joerg, I am wondering if you are still working on this ticket? Some of our internal framework developers noticed that Mesos sends a TASK_FAILED with REASON_MEMORY_LIMIT even if he just specified the wrong executor command (thus causing executor registration timeout). This is very misleading and we want to fix that asap. - Jie --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/35927/#review89779 --- On June 29, 2015, 7:22 a.m., Joerg Schad wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/35927/ --- (Updated June 29, 2015, 7:22 a.m.) Review request for mesos and Alexander Rukletsov. Bugs: MESOS-2035 https://issues.apache.org/jira/browse/MESOS-2035 Repository: mesos Description --- [WIP] -need to add documentation -need to add test -not all potential paths are covered. It is not clear whether isolator sees event before the kill, see first comment in CgroupsMemIsolatorProcess::oom in mem.cpp. Diffs - docs/external-containerizer.md 96932189b74dce1ae28e0bd73d5543d1afaffb0b include/mesos/containerizer/containerizer.proto f16ccc89f83da28c413ccfa0687a06b7515a605c include/mesos/mesos.proto 81008ed85daea798ed19d1031de8421a7dc3fb19 include/mesos/slave/isolator.hpp ef2205dd7f4619e53e6cca7cac301f874d08c036 src/slave/containerizer/docker.cpp 6eb1c84b1a6ef17c3e2ea2028e2f2d75110176ba src/slave/containerizer/isolators/cgroups/mem.cpp b0e343fdc7088b2895d5dc8bb416dbcbf241cae5 src/slave/containerizer/isolators/posix/disk.cpp b2f995cba36b1399db48af1de49d76c607f80abd src/slave/containerizer/mesos/containerizer.cpp 313e9b74d3a0157609226041246575d2c4a503f8 src/slave/slave.cpp b8591116eadcd68b8db2a629fbcf793e6b394f14 src/tests/docker_containerizer_tests.cpp 3a983c6813dab6fa03ccb2c87e1ea71866766d6e Diff: https://reviews.apache.org/r/35927/diff/ Testing --- make check (including docker tests) Thanks, Joerg Schad
Re: Review Request 36326: containerizer: added cgroups based statistics.
On July 10, 2015, 8:50 p.m., Timothy Chen wrote: src/slave/containerizer/docker.cpp, line 1266 https://reviews.apache.org/r/36326/diff/4/?file=1004334#file1004334line1266 I think you reordered things here that used to be in _usage. We used to first check the containers map, then check is it destroying, that look at the pid and finally passing that on. Here the order is now pid check, and some reason skip the map check to get it but then checking it later on. I think we should go back to the exact same order as before unless you have reasons to do this change. The thought was as follows: - We called inspect to get the pid. So first we check for a pid. Then we set the pid for the container (if the container exists). Then we call collectUsage to collect stats. - The check for DESTROYING is already being centralized in the collectUsage lambda. On July 10, 2015, 8:50 p.m., Timothy Chen wrote: src/slave/containerizer/docker.cpp, line 1272 https://reviews.apache.org/r/36326/diff/4/?file=1004334#file1004334line1272 If you like to a leave a comment why we called this function we should probably comment at the place we call it not in the function (before docker-inspect). also s/didnt/didn't/g Wanted to emphasize the logic of why we need to update the pid there (and not say in collectUSage). On July 10, 2015, 8:50 p.m., Timothy Chen wrote: src/slave/containerizer/docker.cpp, line 1342 https://reviews.apache.org/r/36326/diff/4/?file=1004334#file1004334line1342 This seems too verbose being LOG(INFO). And what value do you think we have here logging this? I can change it to LOG(DEBUG). Wanted to log there to help us debug. - Jojy --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36326/#review91342 --- On July 9, 2015, 8:38 p.m., Jojy Varghese wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36326/ --- (Updated July 9, 2015, 8:38 p.m.) Review request for mesos and Timothy Chen. Bugs: MESOS-2951 https://issues.apache.org/jira/browse/MESOS-2951 Repository: mesos Description --- WIP Adding cgroups cpustat and memory statistics as preferred way to get usage statistics for containerizers. Jira: MESOS-2951 Diffs - src/slave/containerizer/docker.hpp 646a277be21deaded47324bea5474bd68699f25f src/slave/containerizer/docker.cpp 6eb1c84b1a6ef17c3e2ea2028e2f2d75110176ba Diff: https://reviews.apache.org/r/36326/diff/ Testing --- make check Thanks, Jojy Varghese
Re: Review Request 36378: Refactor Linux Performance monitor to handle changing 'perf stat' output versions depending on kernel version.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36378/#review91355 --- src/linux/perf.cpp (line 468) https://reviews.apache.org/r/36378/#comment144721 Get version number from 'perf --version' not os::release. src/linux/perf.cpp (line 623) https://reviews.apache.org/r/36378/#comment144720 We should get the version number from 'perf --version' not os::release in case a mismatched perf is installed. - Paul Brett On July 10, 2015, 8:48 p.m., Paul Brett wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36378/ --- (Updated July 10, 2015, 8:48 p.m.) Review request for mesos, Ben Mahler, Chi Zhang, Ian Downes, and Cong Wang. Bugs: MESOS-2834 https://issues.apache.org/jira/browse/MESOS-2834 Repository: mesos Description --- Refactor Linux Performance monitor to handle changing 'perf stat' output versions depending on kernel version. Diffs - src/linux/perf.cpp 697b75e846a43d4f106ad8f39a18882836d7dc02 Diff: https://reviews.apache.org/r/36378/diff/ Testing --- sudo make check Thanks, Paul Brett
Re: Review Request 36326: containerizer: added cgroups based statistics.
On July 10, 2015, 8:50 p.m., Timothy Chen wrote: src/slave/containerizer/docker.cpp, line 1266 https://reviews.apache.org/r/36326/diff/4/?file=1004334#file1004334line1266 I think you reordered things here that used to be in _usage. We used to first check the containers map, then check is it destroying, that look at the pid and finally passing that on. Here the order is now pid check, and some reason skip the map check to get it but then checking it later on. I think we should go back to the exact same order as before unless you have reasons to do this change. Jojy Varghese wrote: The thought was as follows: - We called inspect to get the pid. So first we check for a pid. Then we set the pid for the container (if the container exists). Then we call collectUsage to collect stats. - The check for DESTROYING is already being centralized in the collectUsage lambda. wanted to point out that we are not skipping the check for contanier at all. We pick the pid from the Docker::Container object being passed in by the then continuation. We still check the existence of container in the collection (member variable collection_). - Jojy --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36326/#review91342 --- On July 9, 2015, 8:38 p.m., Jojy Varghese wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36326/ --- (Updated July 9, 2015, 8:38 p.m.) Review request for mesos and Timothy Chen. Bugs: MESOS-2951 https://issues.apache.org/jira/browse/MESOS-2951 Repository: mesos Description --- WIP Adding cgroups cpustat and memory statistics as preferred way to get usage statistics for containerizers. Jira: MESOS-2951 Diffs - src/slave/containerizer/docker.hpp 646a277be21deaded47324bea5474bd68699f25f src/slave/containerizer/docker.cpp 6eb1c84b1a6ef17c3e2ea2028e2f2d75110176ba Diff: https://reviews.apache.org/r/36326/diff/ Testing --- make check Thanks, Jojy Varghese
Re: Review Request 36106: cgroups: added cpuacct subsystem
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36106/ --- (Updated July 10, 2015, 8:47 p.m.) Review request for mesos, Ian Downes, Jie Yu, Joris Van Remoortere, and Timothy Chen. Changes --- review comments @benm Bugs: MESOS-2961 https://issues.apache.org/jira/browse/MESOS-2961 Repository: mesos Description --- cgroups implementation does not have a cpuacct subsystem implementation as of today. Adding the implementation for stat function. Changes: - added Stats class to encapsulate cpuacct.stat data - added implementation for cpuacct::stats - added unit tests Jira: MESOS-2961 Diffs (updated) - src/linux/cgroups.hpp 73b98317880eea3d6a2ba37ac56d1f7e3600ba94 src/linux/cgroups.cpp 4c006d0c7382b940a83359d636c0d48952cdbb00 src/tests/cgroups_tests.cpp 475f48a474eea708f98d8c0300862351a2d4379a Diff: https://reviews.apache.org/r/36106/diff/ Testing --- make check Thanks, Jojy Varghese
Re: Review Request 36378: Refactor Linux Performance monitor to handle changing 'perf stat' output versions depending on kernel version.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36378/ --- (Updated July 10, 2015, 8:48 p.m.) Review request for mesos, Ben Mahler, Chi Zhang, Ian Downes, and Cong Wang. Changes --- Fixed bug handling multiple counters in cgroups. Bugs: MESOS-2834 https://issues.apache.org/jira/browse/MESOS-2834 Repository: mesos Description --- Refactor Linux Performance monitor to handle changing 'perf stat' output versions depending on kernel version. Diffs (updated) - src/linux/perf.cpp 697b75e846a43d4f106ad8f39a18882836d7dc02 Diff: https://reviews.apache.org/r/36378/diff/ Testing --- sudo make check Thanks, Paul Brett
Re: Review Request 36326: containerizer: added cgroups based statistics.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36326/#review91342 --- src/slave/containerizer/docker.cpp (line 1216) https://reviews.apache.org/r/36326/#comment144687 let's spell out cgStats to cgroupStats src/slave/containerizer/docker.cpp (line 1219) https://reviews.apache.org/r/36326/#comment144688 Error getting cgroups statistics for container ' + stringify(containerId) + ': + cgroupStats.error() And why the extra LOG(WARNING) here? src/slave/containerizer/docker.cpp (line 1254) https://reviews.apache.org/r/36326/#comment144689 I think you reordered things here that used to be in _usage. We used to first check the containers map, then check is it destroying, that look at the pid and finally passing that on. Here the order is now pid check, and some reason skip the map check to get it but then checking it later on. I think we should go back to the exact same order as before unless you have reasons to do this change. src/slave/containerizer/docker.cpp (line 1260) https://reviews.apache.org/r/36326/#comment144690 If you like to a leave a comment why we called this function we should probably comment at the place we call it not in the function (before docker-inspect). also s/didnt/didn't/g src/slave/containerizer/docker.cpp (line 1302) https://reviews.apache.org/r/36326/#comment144692 Although it seems obvious from the right hand side you're getting the stat of cgroup, it wasn't obvious what type are you getting back, and more so about it's actually a TryStat If you look at our style guide we only try to use auto in places the right hand side completely capture the type information. I think we should spell out the type here. src/slave/containerizer/docker.cpp (line 1327) https://reviews.apache.org/r/36326/#comment144694 This seems too verbose being LOG(INFO). And what value do you think we have here logging this? - Timothy Chen On July 9, 2015, 8:38 p.m., Jojy Varghese wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36326/ --- (Updated July 9, 2015, 8:38 p.m.) Review request for mesos and Timothy Chen. Bugs: MESOS-2951 https://issues.apache.org/jira/browse/MESOS-2951 Repository: mesos Description --- WIP Adding cgroups cpustat and memory statistics as preferred way to get usage statistics for containerizers. Jira: MESOS-2951 Diffs - src/slave/containerizer/docker.hpp 646a277be21deaded47324bea5474bd68699f25f src/slave/containerizer/docker.cpp 6eb1c84b1a6ef17c3e2ea2028e2f2d75110176ba Diff: https://reviews.apache.org/r/36326/diff/ Testing --- make check Thanks, Jojy Varghese
Review Request 36402: Adding 'Accept' header in request
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36402/ --- Review request for mesos, Anand Mazumdar, Ben Mahler, Marco Massenzio, and Vinod Kone. Repository: mesos-incubating Description --- Adding a method for Accept header in request + refactor of Accept-Encoding Diffs - 3rdparty/libprocess/include/process/http.hpp 72b6d27 3rdparty/libprocess/src/encoder.hpp c5ff761 3rdparty/libprocess/src/http.cpp d168579 3rdparty/libprocess/src/tests/encoder_tests.cpp 0032137 3rdparty/libprocess/src/tests/http_tests.cpp 01f243c Diff: https://reviews.apache.org/r/36402/diff/ Testing --- make check Thanks, Isabel Jimenez
Re: Review Request 36402: Adding 'Accept' header in request
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36402/#review91356 --- 3rdparty/libprocess/src/http.cpp (line 206) https://reviews.apache.org/r/36402/#comment144723 I did not review the entire patch but I stopped at this. Seems like I am missing something here. In my understanding, you can't use the same method for parsing both the Accept, Accept-Encoding as the rules for both of them are entirely different ! :) Let's take an example from the RFC : http://www.w3.org/Protocols/rfc2616/rfc2616-sec14.html So the accept header also understands media-range i.e. you can specify */* or text/* et al meaning all media types can be accepted or in the second case all media types of text/something can only be accepted and so on. There are a couple of other things like accept-param and accept-extension that also need to be handled. I assume that your motive for this change was to add a validation operation for Accept similar to Accept-Encoding header that validates if the header values are well-formed and should be accepted ? If that is the case, you would need to write a separate method/logic and not just use the existing acceptEncoding method. Long story short, we need two methods: 1. Validate if the Accept header is well formed. ( the one you already built minus the mentioned caveats above ) Also , it would be good to add a second one: 2. Given all the accept types mentioned in the Accept header , and the accept types we support ,is it possible for us to send a response back ? If not send a 415 back. What do you think ? - Anand Mazumdar On July 10, 2015, 8:55 p.m., Isabel Jimenez wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36402/ --- (Updated July 10, 2015, 8:55 p.m.) Review request for mesos, Anand Mazumdar, Ben Mahler, Marco Massenzio, and Vinod Kone. Repository: mesos-incubating Description --- Adding a method for Accept header in request + refactor of Accept-Encoding Diffs - 3rdparty/libprocess/include/process/http.hpp 72b6d27 3rdparty/libprocess/src/encoder.hpp c5ff761 3rdparty/libprocess/src/http.cpp d168579 3rdparty/libprocess/src/tests/encoder_tests.cpp 0032137 3rdparty/libprocess/src/tests/http_tests.cpp 01f243c Diff: https://reviews.apache.org/r/36402/diff/ Testing --- make check Thanks, Isabel Jimenez
Re: Review Request 36404: Added support for peek() to process::io
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36404/ --- (Updated July 10, 2015, 3:20 p.m.) Review request for mesos, Joris Van Remoortere and Joseph Wu. Repository: mesos Description --- JIRA: https://issues.apache.org/jira/browse/MESOS-2964 Diffs (updated) - 3rdparty/libprocess/include/process/io.hpp 245716353ad5ffa8d705fc5e826addfa6a3594dc 3rdparty/libprocess/src/io.cpp 4a6e18a17012994d358099ad32d4c282fea3b0b1 3rdparty/libprocess/src/tests/io_tests.cpp c642bab9e2845668767ad237985cb9ce1109 Diff: https://reviews.apache.org/r/36404/diff/ Testing --- - Added a test case for process::io::peek - make check Thanks, Artem Harutyunyan
Re: Review Request 36402: Adding 'Accept' header in request
On July 10, 2015, 10:13 p.m., Anand Mazumdar wrote: 3rdparty/libprocess/src/http.cpp, line 216 https://reviews.apache.org/r/36402/diff/1/?file=1008565#file1008565line216 I did not review the entire patch but I stopped at this. Seems like I am missing something here. In my understanding, you can't use the same method for parsing both the Accept, Accept-Encoding as the rules for both of them are entirely different ! :) Let's take an example from the RFC : http://www.w3.org/Protocols/rfc2616/rfc2616-sec14.html So the accept header also understands media-range i.e. you can specify */* or text/* et al meaning all media types can be accepted or in the second case all media types of text/something can only be accepted and so on. There are a couple of other things like accept-param and accept-extension that also need to be handled. I assume that your motive for this change was to add a validation operation for Accept similar to Accept-Encoding header that validates if the header values are well-formed and should be accepted ? If that is the case, you would need to write a separate method/logic and not just use the existing acceptEncoding method. Long story short, we need two methods: 1. Validate if the Accept header is well formed. ( the one you already built minus the mentioned caveats above ) Also , it would be good to add a second one: 2. Given all the accept types mentioned in the Accept header , and the accept types we support ,is it possible for us to send a response back ? If not send a 415 back. What do you think ? The AcceptHeader method groups validation that's common to both 'Accept' and 'Accept-Encoding'. The logic was already there I just moved it so we can use it for both, and if needed and more things to each one separately. I plan to add 'accept-param' and 'accept-extension' support in a different patch. I also added a TODO to consider handling all this by returning Response, what do you think, should we make that change now? - Isabel --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36402/#review91356 --- On July 10, 2015, 8:55 p.m., Isabel Jimenez wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36402/ --- (Updated July 10, 2015, 8:55 p.m.) Review request for mesos, Anand Mazumdar, Ben Mahler, Marco Massenzio, and Vinod Kone. Repository: mesos-incubating Description --- Adding a method for Accept header in request + refactor of Accept-Encoding Diffs - 3rdparty/libprocess/include/process/http.hpp 72b6d27 3rdparty/libprocess/src/encoder.hpp c5ff761 3rdparty/libprocess/src/http.cpp d168579 3rdparty/libprocess/src/tests/encoder_tests.cpp 0032137 3rdparty/libprocess/src/tests/http_tests.cpp 01f243c Diff: https://reviews.apache.org/r/36402/diff/ Testing --- make check Thanks, Isabel Jimenez
Re: Review Request 36402: Adding 'Accept' header in request
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36402/#review91361 --- Patch looks great! Reviews applied: [36402] All tests passed. - Mesos ReviewBot On July 10, 2015, 8:55 p.m., Isabel Jimenez wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36402/ --- (Updated July 10, 2015, 8:55 p.m.) Review request for mesos, Anand Mazumdar, Ben Mahler, Marco Massenzio, and Vinod Kone. Repository: mesos-incubating Description --- Adding a method for Accept header in request + refactor of Accept-Encoding Diffs - 3rdparty/libprocess/include/process/http.hpp 72b6d27 3rdparty/libprocess/src/encoder.hpp c5ff761 3rdparty/libprocess/src/http.cpp d168579 3rdparty/libprocess/src/tests/encoder_tests.cpp 0032137 3rdparty/libprocess/src/tests/http_tests.cpp 01f243c Diff: https://reviews.apache.org/r/36402/diff/ Testing --- make check Thanks, Isabel Jimenez
Re: Review Request 36106: cgroups: added cpuacct subsystem
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36106/ --- (Updated July 10, 2015, 10:46 p.m.) Review request for mesos, Ian Downes, Jie Yu, Joris Van Remoortere, and Timothy Chen. Changes --- code review @benm Bugs: MESOS-2961 https://issues.apache.org/jira/browse/MESOS-2961 Repository: mesos Description --- cgroups implementation does not have a cpuacct subsystem implementation as of today. Adding the implementation for stat function. Changes: - added Stats class to encapsulate cpuacct.stat data - added implementation for cpuacct::stats - added unit tests Jira: MESOS-2961 Diffs (updated) - src/linux/cgroups.hpp 73b98317880eea3d6a2ba37ac56d1f7e3600ba94 src/linux/cgroups.cpp 4c006d0c7382b940a83359d636c0d48952cdbb00 src/tests/cgroups_tests.cpp 475f48a474eea708f98d8c0300862351a2d4379a Diff: https://reviews.apache.org/r/36106/diff/ Testing --- make check Thanks, Jojy Varghese
Re: Review Request 36326: containerizer: added cgroups based statistics.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36326/ --- (Updated July 10, 2015, 10:47 p.m.) Review request for mesos and Timothy Chen. Changes --- review comments @tim Bugs: MESOS-2951 https://issues.apache.org/jira/browse/MESOS-2951 Repository: mesos Description --- WIP Adding cgroups cpustat and memory statistics as preferred way to get usage statistics for containerizers. Jira: MESOS-2951 Diffs (updated) - src/slave/containerizer/docker.hpp 646a277be21deaded47324bea5474bd68699f25f src/slave/containerizer/docker.cpp 6eb1c84b1a6ef17c3e2ea2028e2f2d75110176ba Diff: https://reviews.apache.org/r/36326/diff/ Testing --- make check Thanks, Jojy Varghese
Re: Review Request 36326: containerizer: added cgroups based statistics.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36326/ --- (Updated July 10, 2015, 11:03 p.m.) Review request for mesos and Timothy Chen. Changes --- review comments @tim Bugs: MESOS-2951 https://issues.apache.org/jira/browse/MESOS-2951 Repository: mesos Description (updated) --- Adding cgroups cpustat and memory statistics as preferred way to get usage statistics for containerizers. Jira: MESOS-2951 Diffs (updated) - src/slave/containerizer/docker.hpp 646a277be21deaded47324bea5474bd68699f25f src/slave/containerizer/docker.cpp 6eb1c84b1a6ef17c3e2ea2028e2f2d75110176ba Diff: https://reviews.apache.org/r/36326/diff/ Testing --- make check Thanks, Jojy Varghese
Re: Review Request 36402: Adding 'Accept' header in request
On July 10, 2015, 10:13 p.m., Anand Mazumdar wrote: 3rdparty/libprocess/src/http.cpp, line 216 https://reviews.apache.org/r/36402/diff/1/?file=1008565#file1008565line216 I did not review the entire patch but I stopped at this. Seems like I am missing something here. In my understanding, you can't use the same method for parsing both the Accept, Accept-Encoding as the rules for both of them are entirely different ! :) Let's take an example from the RFC : http://www.w3.org/Protocols/rfc2616/rfc2616-sec14.html So the accept header also understands media-range i.e. you can specify */* or text/* et al meaning all media types can be accepted or in the second case all media types of text/something can only be accepted and so on. There are a couple of other things like accept-param and accept-extension that also need to be handled. I assume that your motive for this change was to add a validation operation for Accept similar to Accept-Encoding header that validates if the header values are well-formed and should be accepted ? If that is the case, you would need to write a separate method/logic and not just use the existing acceptEncoding method. Long story short, we need two methods: 1. Validate if the Accept header is well formed. ( the one you already built minus the mentioned caveats above ) Also , it would be good to add a second one: 2. Given all the accept types mentioned in the Accept header , and the accept types we support ,is it possible for us to send a response back ? If not send a 415 back. What do you think ? Isabel Jimenez wrote: The AcceptHeader method groups validation that's common to both 'Accept' and 'Accept-Encoding'. The logic was already there I just moved it so we can use it for both, and if needed and more things to each one separately. I plan to add 'accept-param' and 'accept-extension' support in a different patch. I also added a TODO to consider handling all this by returning Response, what do you think, should we make that change now? Unfortunately, This does not make much sense to me. Let's take an example from your test-case. requests[2].headers[Accept] = foo, test;q=0.0; This is not a VALID accept header at all if you see its grammer carefully. The only thing that Accept and Accept-Encoding have in common is the q-value syntax and how you specify them on one line i.e. delimited by , and ;. :) An Accept header must have a type/subtype or a type/*. Makes sense ? ( I am re-opening the issue ) - Anand --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36402/#review91356 --- On July 10, 2015, 8:55 p.m., Isabel Jimenez wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36402/ --- (Updated July 10, 2015, 8:55 p.m.) Review request for mesos, Anand Mazumdar, Ben Mahler, Marco Massenzio, and Vinod Kone. Repository: mesos-incubating Description --- Adding a method for Accept header in request + refactor of Accept-Encoding Diffs - 3rdparty/libprocess/include/process/http.hpp 72b6d27 3rdparty/libprocess/src/encoder.hpp c5ff761 3rdparty/libprocess/src/http.cpp d168579 3rdparty/libprocess/src/tests/encoder_tests.cpp 0032137 3rdparty/libprocess/src/tests/http_tests.cpp 01f243c Diff: https://reviews.apache.org/r/36402/diff/ Testing --- make check Thanks, Isabel Jimenez
Re: Review Request 36185: Create pre-launch hook before a docker container launches in slave.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36185/#review91367 --- src/tests/docker_containerizer_tests.cpp (line 3114) https://reviews.apache.org/r/36185/#comment144736 What you suggested sounds fine. I think we need to comment on the top of the test what this tests and what is it expecting to see. It wasn't too obvious when I read this. - Timothy Chen On July 8, 2015, 5:06 p.m., haosdent huang wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36185/ --- (Updated July 8, 2015, 5:06 p.m.) Review request for mesos and Timothy Chen. Bugs: MESOS-2588 https://issues.apache.org/jira/browse/MESOS-2588 Repository: mesos Description --- Create pre-launch hook before a docker container launches in slave. Diffs - include/mesos/hook.hpp 0995c249e9f07c6c4a26d1c5c369d48bb8056f1f src/examples/test_hook_module.cpp d61cd557d8e44e5089f324edf97b0335a4ededab src/hook/manager.hpp 47e8eb7d54d55049d054cf9b1225e67333f22adc src/hook/manager.cpp 0108534c1fc527a0c66d201d7a5232e80b9928bf src/slave/containerizer/docker.cpp cfb60177fe48ec0eeab12ff392c6c9f89634b92f src/tests/docker_containerizer_tests.cpp a3da786c1b733e9dd4abf1d02d5dae051393d921 Diff: https://reviews.apache.org/r/36185/diff/ Testing --- # Add a new unit test DockerContainerizerTest.ROOT_DOCKER_VerifySlavePreLaunchDockerHook make check -j8 GTEST_FILTER=-* sudo ./bin/mesos-tests.sh --verbose --gtest_filter=DockerContainerizerTest.ROOT_DOCKER_VerifySlavePreLaunchDockerHook Thanks, haosdent huang
Re: Review Request 34908: Rename --docker_sandbox_directory flag for general use.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/34908/#review91369 --- Ship it! Ship It! docs/configuration.md (line 933) https://reviews.apache.org/r/34908/#comment144737 Can we go through a depcreation cycle for this flag? I'm not 100% sure who's using this but we should give users a warning and remove it at the following version. - Timothy Chen On June 22, 2015, 4:55 p.m., Ian Downes wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/34908/ --- (Updated June 22, 2015, 4:55 p.m.) Review request for mesos and Timothy Chen. Repository: mesos Description --- Rename --docker_sandbox_directory flag for general use. This will require users to update configuration scripts etc if they specify the old flag. Diffs - docs/configuration.md aaf65bfa2848318c8d07c772ba2c521aa72c2677 src/slave/containerizer/docker.cpp 00db9811824995fe19a6143aa2bcba3733a93147 src/slave/flags.hpp 7634e368c72e83932dcd992d78eaca146326606b src/slave/flags.cpp cbf431eb0627bdaf07241cc0fc4630df06fb20e2 src/tests/docker_containerizer_tests.cpp 3a983c6813dab6fa03ccb2c87e1ea71866766d6e Diff: https://reviews.apache.org/r/34908/diff/ Testing --- Thanks, Ian Downes
