Re: Review Request 35981: Added persistent volume user guide.
On June 30, 2015, 12:14 a.m., Adam B wrote:
docs/persistent-volume.md, line 56
https://reviews.apache.org/r/35981/diff/1/?file=994064#file994064line56
Can a volume/reservation created by one framework principal only be
destroyed/unreserved by a framework scheduler using the same principal? If
the principal is different, then the operation would fail, even if the role
or even frameworkId is identical?
We only look at the principal of an framework or an operator. The role does not
matter because an operator does not have a role. The frameworkId is also
irrelavent because framework B can unreserve/destroy a reservation/volume
created by framework B as long as ACL allows it.
On June 30, 2015, 12:14 a.m., Adam B wrote:
docs/persistent-volume.md, lines 67-69
https://reviews.apache.org/r/35981/diff/1/?file=994064#file994064line67
You might need a blank line preceding this, and/or some indentation for
it to show up as a numbered list. Doesn't render properly as is.
Unique per role per slave? So I could use the same volumeId for related
volume instances (e.g. my 'logging' or 'data' volumes), up to a max of one
per slave? But if I happen to launch another executor/task on a slave where
one of those volumes is already created/mounted, then I have to rename it
('data2')?
Yes, this is in the design doc. Persistent volumes can be shared within a role,
so it has to be unique per role.
On June 30, 2015, 12:14 a.m., Adam B wrote:
docs/persistent-volume.md, lines 120-122
https://reviews.apache.org/r/35981/diff/1/?file=994064#file994064line120
Any chance it could return the host_path too? Or do I have to go
digging through slave logs to figure out where my volume was created?
Why the framework wants to know that? This is similar to sandbox. Do we have a
way to let the framework know about the location of the sandbox?
- Jie
---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/35981/#review89827
---
On July 7, 2015, 12:34 p.m., Michael Park wrote:
---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/35981/
---
(Updated July 7, 2015, 12:34 p.m.)
Review request for mesos, Adam B, Jie Yu, and Timothy Chen.
Bugs: MESOS-2405
https://issues.apache.org/jira/browse/MESOS-2405
Repository: mesos
Description
---
The Github rendered version is available [here](
https://github.com/mesosphere/mesos/blob/user-docs/docs/persistent-volume.md)
Diffs
-
docs/persistent-volume.md PRE-CREATION
Diff: https://reviews.apache.org/r/35981/diff/
Testing
---
Documentation.
Thanks,
Michael Park
Re: Review Request 35986: Allow slave attributes flag take a value with ':'.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/35986/ --- (Updated July 7, 2015, 5:55 p.m.) Review request for mesos, Adam B and Isabel Jimenez. Bugs: MESOS-2868 https://issues.apache.org/jira/browse/MESOS-2868 Repository: mesos Description --- Allow slave attributes flag take a value with ':'. Diffs (updated) - src/common/attributes.cpp aab114e1a5932e3f218b850e1afc7f2ef0f10e21 src/tests/attributes_tests.cpp 2e10eaf3c33e418603ea19090bb0bf9179af71f7 Diff: https://reviews.apache.org/r/35986/diff/ Testing --- make check Thanks, haosdent huang
Re: Review Request 35777: Made post-reviews.py handle bad (or not) ReviewBoard URLs.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/35777/#review90714 --- Patch looks great! Reviews applied: [35777] All tests passed. - Mesos ReviewBot On July 7, 2015, 3:19 p.m., Michael Park wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/35777/ --- (Updated July 7, 2015, 3:19 p.m.) Review request for mesos, Benjamin Hindman, Marco Massenzio, and Till Toenshoff. Repository: mesos Description --- (1) Handles the case where the `Review: ...` line isn't the last of the commit message. ``` 01596ce802669e9dac7dd24193f041cef3354830 - Fixed post-reviews.py hanging bug. (6 seconds ago) ReviewBoard URL must be the last line of the commit message! Fixed post-reviews.py hanging bug. Review: https://reviews.apache.org/r/35771 abcd ``` (2) Handles the case where the ReviewBoard URL is invalid. ``` af081a07234794f60a2575e0383ce537d7111c18 - Fixed post-reviews.py hanging bug. (29 seconds ago) Invalid ReviewBoard URL : 'https://reviews.apache.org/r/35771 abcd' ``` Diffs - support/post-reviews.py b04e26b85e6b056098c15078f9b31dc352682351 Diff: https://reviews.apache.org/r/35777/diff/ Testing --- Modified the commit message with `git rebase` and ran `./support/post-reviews.py` to observe the above error messages. Used the valid commit message for this patch and created it by running `./support/post-reviews.py`. Thanks, Michael Park
Re: Review Request 35986: Allow slave attributes flag take a value with ':'.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/35986/ --- (Updated July 7, 2015, 5:46 p.m.) Review request for mesos, Adam B and Isabel Jimenez. Bugs: MESOS-2868 https://issues.apache.org/jira/browse/MESOS-2868 Repository: mesos Description --- Allow slave attributes flag take a value with ':'. Diffs (updated) - src/common/attributes.cpp aab114e1a5932e3f218b850e1afc7f2ef0f10e21 src/tests/attributes_tests.cpp 2e10eaf3c33e418603ea19090bb0bf9179af71f7 Diff: https://reviews.apache.org/r/35986/diff/ Testing --- make check Thanks, haosdent huang
Re: Review Request 36204: Pass slave's total resources in ResourceUsage.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36204/#review90727 --- Ship it! LGTM! Could you please add a follow up patch to test this code path? src/slave/slave.cpp (lines 4382 - 4383) https://reviews.apache.org/r/36204/#comment143863 You should just add a CHECK here: ``` CHECK_SOME(totalResources) Failed to apply checkpointed resource checkpointedResources to slave's resources info-resources(); usage-mutable_total()-CopyFrom(totalResources.get()); ``` - Jie Yu On July 7, 2015, 4:20 p.m., Bartek Plotka wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36204/ --- (Updated July 7, 2015, 4:20 p.m.) Review request for mesos, Ben Mahler, Jie Yu, Niklas Nielsen, Szymon Konefal, and Vinod Kone. Bugs: MESOS-2933 https://issues.apache.org/jira/browse/MESOS-2933 Repository: mesos Description --- Pass slave's total resources to the ResourceEstimator and QoSController via Slave::usage(). Diffs - include/mesos/mesos.proto 3dd4a5b7a4b3bc56bdc690d6adf05f88c0d28273 src/slave/slave.cpp 008170f4ce7b97eda43b50b48083b32ae65925c0 Diff: https://reviews.apache.org/r/36204/diff/ Testing --- make check. Thanks, Bartek Plotka
Re: Review Request 36204: Pass slave's total resources in ResourceUsage.
On July 6, 2015, 5:50 p.m., Jie Yu wrote: src/slave/slave.cpp, line 4379 https://reviews.apache.org/r/36204/diff/1/?file=1000223#file1000223line4379 Per my comments above, the logic here needs to be adjusted. We need to apply checkpointed resource to info.resources() here. Please refer to the following code: https://github.com/apache/mesos/blob/master/src/slave/slave.cpp#L3883 Thanks Jie for your feedback! I addressed your issue, however I'm just curious if there is any possibility to get an error (here in *::usage()*) from *applyCheckpointedResources* function? (https://github.com/apache/mesos/blob/master/src/slave/slave.cpp#L3883). IMO here we are quite sure that *checkpointedResources* are proper. If yes, would it make sense to just put *info.resources()* into *usage-total* in case we cannot apply checkpointed resources? - Bartek --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36204/#review90510 --- On July 7, 2015, 4:20 p.m., Bartek Plotka wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36204/ --- (Updated July 7, 2015, 4:20 p.m.) Review request for mesos, Ben Mahler, Jie Yu, Niklas Nielsen, Szymon Konefal, and Vinod Kone. Bugs: MESOS-2933 https://issues.apache.org/jira/browse/MESOS-2933 Repository: mesos Description --- Pass slave's total resources to the ResourceEstimator and QoSController via Slave::usage(). Diffs - include/mesos/mesos.proto 3dd4a5b7a4b3bc56bdc690d6adf05f88c0d28273 src/slave/slave.cpp 008170f4ce7b97eda43b50b48083b32ae65925c0 Diff: https://reviews.apache.org/r/36204/diff/ Testing --- make check. Thanks, Bartek Plotka
Review Request 36267: MESOS-2943: Add comment for explicit return type.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36267/ --- Review request for mesos, Benjamin Hindman, Artem Harutyunyan, and Michael Park. Bugs: MESOS-2943 https://issues.apache.org/jira/browse/MESOS-2943 Repository: mesos Description --- See summary. Diffs - 3rdparty/libprocess/src/libevent_ssl_socket.cpp 9424dd421b290bec25b4a7c4dc0071ffef6fdc5e Diff: https://reviews.apache.org/r/36267/diff/ Testing --- Only adding a comment. Thanks, Joris Van Remoortere
Re: Review Request 36214: Fix running docker executor tests.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36214/#review90665 --- Ship it! - Joerg Schad On July 6, 2015, 8:40 p.m., Timothy Chen wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36214/ --- (Updated July 6, 2015, 8:40 p.m.) Review request for mesos, Bernd Mathiske, Joerg Schad, and Till Toenshoff. Bugs: MESOS-2996 https://issues.apache.org/jira/browse/MESOS-2996 Repository: mesos Description --- When running on CentOS 7.1 it didn't have /bin in it's PATH and therefore it failed to find test-executor that is in /bin/ in the container. This works on other systems since the tests inherits the environment variables from the system and /bin usually is in the PATH. Diffs - src/tests/docker_containerizer_tests.cpp d4ccb0b30fe27980846d913e292d2e18fd3d1055 Diff: https://reviews.apache.org/r/36214/diff/ Testing --- make check Thanks, Timothy Chen
Re: Review Request 36048: Updated authorizer to allow for modularized implementations.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36048/ --- (Updated July 7, 2015, 9:33 a.m.) Review request for mesos, Adam B and Till Toenshoff. Changes --- No longer pointing to the JIRA epic. Bugs: MESOS-2946 https://issues.apache.org/jira/browse/MESOS-2946 Repository: mesos Description --- Splits and updates the original declaration of the `Authorizer` into its interface and a default implementation, the `LocalAuthorizer`. Following the pattern of the modularized `Authenticator`, it generates a default constructor which is required when writing a `TYPED_TEST` in a follow up patch. Additionally, an initialize method has been added, needed for passing in the current ACL definitions as provided via flags. Other changes are just updates to allow for compilation. Diffs - include/mesos/authorizer/authorizer.hpp PRE-CREATION include/mesos/authorizer/authorizer.proto PRE-CREATION include/mesos/mesos.proto 3dd4a5b7a4b3bc56bdc690d6adf05f88c0d28273 include/mesos/type_utils.hpp e7bfe8ca60af945e76b5d85ab37cc97b17ff1b4a src/Makefile.am addb63f615f16ae6b25f745b2e79fd9fc0e27851 src/authorizer/authorizer.hpp c039d9412780aa199db169b31991bf9f45b07d0f src/authorizer/authorizer.cpp 21e97e315478a4ca9442af83732665f85eb2f8fc src/common/parse.hpp 8d7ddd6819dad98cd96d5aaae8fe57caf1ee7098 src/examples/persistent_volume_framework.cpp c6d6ed337bfca91dc146cb31298cabebdbb13509 src/local/local.cpp 1953d84c75a83f4ace944d6243456235d8a193ff src/master/flags.hpp f2cd19a6edfaa4e5bb31f024ef8d5beda32fbc2f src/master/http.cpp 2be613b2e4c913b74c12d0d8f2d0e25da3cd3656 src/master/main.cpp 2624b7ea4920a534c98f5dfbf9286c54c50f11a9 src/master/master.hpp fb4d6fac85e284987ec8fbf6949b5023875573fb src/master/master.cpp c5a4875f0d43c5091ae9a52c6b1d04105dfa3914 src/tests/authorization_tests.cpp 99bb06c1ee73a90abaeeabb742e45aa188c21a87 src/tests/cluster.hpp cfe7ef0c7a6dc62cddc3e5f5b5b28c8bcb2bed26 src/tests/mesos.hpp 9157ac079808d2686592e54ea26a26e6a0825ed3 src/tests/mesos.cpp 5eab6dea6058865847425ab8d31708c92c6f098a Diff: https://reviews.apache.org/r/36048/diff/ Testing --- make check Thanks, Alexander Rojas
Re: Review Request 36049: Added support for modularized Authorizer
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36049/ --- (Updated July 7, 2015, 9:34 a.m.) Review request for mesos, Adam B and Till Toenshoff. Changes --- Update JIRA entry. Bugs: MESOS-2947 https://issues.apache.org/jira/browse/MESOS-2947 Repository: mesos Description --- Adds and integrates helper classes needed to support an `Authorizer` module. Also adds a flag to the master, allowing the selection of an `Authorizer` module. Diffs - include/mesos/authorizer/authorizer.hpp PRE-CREATION include/mesos/module/authorizer.hpp PRE-CREATION src/Makefile.am addb63f615f16ae6b25f745b2e79fd9fc0e27851 src/authorizer/authorizer.cpp PRE-CREATION src/local/local.cpp 1953d84c75a83f4ace944d6243456235d8a193ff src/master/constants.hpp 7cec18b7fdfd3b96cde42a30d217c026b2695dce src/master/constants.cpp fbcae60c43e835f96ec061bd0e9f7961e31fc341 src/master/flags.hpp f2cd19a6edfaa4e5bb31f024ef8d5beda32fbc2f src/master/flags.cpp 60ac64d98d53f74f904846b27a3833a7c44a9756 src/master/main.cpp 2624b7ea4920a534c98f5dfbf9286c54c50f11a9 src/module/manager.cpp 909ca56eea85d365cb9ebe1b3cce43051cabb670 src/tests/cluster.hpp cfe7ef0c7a6dc62cddc3e5f5b5b28c8bcb2bed26 Diff: https://reviews.apache.org/r/36049/diff/ Testing --- make check Thanks, Alexander Rojas
Re: Review Request 36214: Fix running docker executor tests.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36214/#review90663 --- Ship it! Ship It! - Bernd Mathiske On July 6, 2015, 1:40 p.m., Timothy Chen wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36214/ --- (Updated July 6, 2015, 1:40 p.m.) Review request for mesos, Bernd Mathiske, Joerg Schad, and Till Toenshoff. Bugs: MESOS-2996 https://issues.apache.org/jira/browse/MESOS-2996 Repository: mesos Description --- When running on CentOS 7.1 it didn't have /bin in it's PATH and therefore it failed to find test-executor that is in /bin/ in the container. This works on other systems since the tests inherits the environment variables from the system and /bin usually is in the PATH. Diffs - src/tests/docker_containerizer_tests.cpp d4ccb0b30fe27980846d913e292d2e18fd3d1055 Diff: https://reviews.apache.org/r/36214/diff/ Testing --- make check Thanks, Timothy Chen
Re: Review Request 36216: Only run netcat tests when nc is available.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36216/#review90662 --- Ship it! Ship It! - Bernd Mathiske On July 6, 2015, 3:58 p.m., Timothy Chen wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36216/ --- (Updated July 6, 2015, 3:58 p.m.) Review request for mesos, Bernd Mathiske, Chi Zhang, Joerg Schad, and Till Toenshoff. Bugs: MESOS-2996 https://issues.apache.org/jira/browse/MESOS-2996 Repository: mesos Description --- nc is not available on CentOS 7.1 and all the tests using nc will fail. Added a new NetcatFilter so we detect nc before running these tests. Diffs - src/tests/docker_containerizer_tests.cpp d4ccb0b30fe27980846d913e292d2e18fd3d1055 src/tests/environment.cpp 3726e5d95b9388f0881184e1b4874e1eb2e7f531 src/tests/port_mapping_tests.cpp ac49cdfdcf6baf00ac2907e193c683c8b6c83ffb Diff: https://reviews.apache.org/r/36216/diff/ Testing --- make check Thanks, Timothy Chen
Re: Review Request 36205: Added AWAIT_EXPECT_TRUE and AWAIT_EXPECT_FALSE macros.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36205/#review90661 --- Ship it! After fixing my previous issue... - Joerg Schad On July 6, 2015, 3:44 p.m., Alexander Rojas wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36205/ --- (Updated July 6, 2015, 3:44 p.m.) Review request for mesos, Bernd Mathiske, Joerg Schad, and Till Toenshoff. Repository: mesos Description --- See summary. Diffs - 3rdparty/libprocess/include/process/gtest.hpp 8518c38c9cbf62a098b43b4d0b6d80e561b17ad4 Diff: https://reviews.apache.org/r/36205/diff/ Testing --- Used in tests in follow up patch. Thanks, Alexander Rojas
Re: Review Request 36205: Added AWAIT_EXPECT_TRUE and AWAIT_EXPECT_FALSE macros.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36205/ --- (Updated July 7, 2015, 10:20 a.m.) Review request for mesos, Bernd Mathiske, Joerg Schad, and Till Toenshoff. Changes --- Added mixed empty line. Repository: mesos Description --- See summary. Diffs (updated) - 3rdparty/libprocess/include/process/gtest.hpp 8518c38c9cbf62a098b43b4d0b6d80e561b17ad4 Diff: https://reviews.apache.org/r/36205/diff/ Testing --- Used in tests in follow up patch. Thanks, Alexander Rojas
Re: Review Request 36205: Added AWAIT_EXPECT_TRUE and AWAIT_EXPECT_FALSE macros.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36205/#review90660 --- 3rdparty/libprocess/include/process/gtest.hpp (line 301) https://reviews.apache.org/r/36205/#comment143797 Shouldn't this be also two empty lines here? - Joerg Schad On July 6, 2015, 3:44 p.m., Alexander Rojas wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36205/ --- (Updated July 6, 2015, 3:44 p.m.) Review request for mesos, Bernd Mathiske, Joerg Schad, and Till Toenshoff. Repository: mesos Description --- See summary. Diffs - 3rdparty/libprocess/include/process/gtest.hpp 8518c38c9cbf62a098b43b4d0b6d80e561b17ad4 Diff: https://reviews.apache.org/r/36205/diff/ Testing --- Used in tests in follow up patch. Thanks, Alexander Rojas
Re: Review Request 36216: Only run netcat tests when nc is available.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36216/#review90664 --- src/tests/docker_containerizer_tests.cpp (line 2123) https://reviews.apache.org/r/36216/#comment143798 Should we comment the NC in the comment above? src/tests/environment.cpp (line 251) https://reviews.apache.org/r/36216/#comment143799 s/so no 'nc' tests/so no tests depending on 'nc' src/tests/port_mapping_tests.cpp (line 432) https://reviews.apache.org/r/36216/#comment143800 Should we comment about NC? (see above) - Joerg Schad On July 6, 2015, 10:58 p.m., Timothy Chen wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36216/ --- (Updated July 6, 2015, 10:58 p.m.) Review request for mesos, Bernd Mathiske, Chi Zhang, Joerg Schad, and Till Toenshoff. Bugs: MESOS-2996 https://issues.apache.org/jira/browse/MESOS-2996 Repository: mesos Description --- nc is not available on CentOS 7.1 and all the tests using nc will fail. Added a new NetcatFilter so we detect nc before running these tests. Diffs - src/tests/docker_containerizer_tests.cpp d4ccb0b30fe27980846d913e292d2e18fd3d1055 src/tests/environment.cpp 3726e5d95b9388f0881184e1b4874e1eb2e7f531 src/tests/port_mapping_tests.cpp ac49cdfdcf6baf00ac2907e193c683c8b6c83ffb Diff: https://reviews.apache.org/r/36216/diff/ Testing --- make check Thanks, Timothy Chen
Re: Review Request 36267: MESOS-2943: Add comment for explicit return type.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36267/#review90744 --- Patch looks great! Reviews applied: [36267] All tests passed. - Mesos ReviewBot On July 7, 2015, 5:21 p.m., Joris Van Remoortere wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36267/ --- (Updated July 7, 2015, 5:21 p.m.) Review request for mesos, Benjamin Hindman, Artem Harutyunyan, and Michael Park. Bugs: MESOS-2943 https://issues.apache.org/jira/browse/MESOS-2943 Repository: mesos Description --- See summary. Diffs - 3rdparty/libprocess/src/libevent_ssl_socket.cpp 9424dd421b290bec25b4a7c4dc0071ffef6fdc5e Diff: https://reviews.apache.org/r/36267/diff/ Testing --- Only adding a comment. Thanks, Joris Van Remoortere
Re: Review Request 31444: Support chrooting in MesosContainerizer launch helper.
---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/31444/#review90757
---
Bad patch!
Reviews applied: [32891, 31444]
Failed command: ./support/apply-review.sh -n -r 31444
Error:
2015-07-07 18:42:10 URL:https://reviews.apache.org/r/31444/diff/raw/
[12941/12941] - 31444.patch [1]
error: patch failed: src/slave/containerizer/mesos/launch.cpp:20
error: src/slave/containerizer/mesos/launch.cpp: patch does not apply
Failed to apply patch
- Mesos ReviewBot
On July 7, 2015, 6:33 p.m., Ian Downes wrote:
---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/31444/
---
(Updated July 7, 2015, 6:33 p.m.)
Review request for mesos, Chi Zhang, Dominic Hamon, Jay Buffington, Jie Yu,
and James Peach.
Bugs: MESOS-2350
https://issues.apache.org/jira/browse/MESOS-2350
Repository: mesos
Description
---
Optionally take a path that the launch helper should chroot to before
exec'ing the executor. It is assumed that the work directory is mounted to
the appropriate location under the chroot. In particular, the path to the
executor must be relative to the chroot.
Configuration that should be private to the chroot is done during the launch,
e.g. mounting proc and statically configuring basic devices. It is assumed
that other configuration, e.g., preparing the image, mounting in volumes or
persistent resources, is done by the caller.
Mounts can be made to the chroot (e.g., updating the volumes or persistent
resources) and they will propagate in to the container but mounts made inside
the container will not propagate out to the host.
It currently assumes that at least {{chroot}}/tmp is writeable and that mount
points {{chroot}}/{tmp,dev,proc,sys} exist in the chroot.
This is specific to Linux.
Diffs
-
src/Makefile.am e7de0f3d1a5efeaef47d5074defe3b40db94f573
src/linux/fs.cpp 568565f878b34708170a886dc4d62849aa01f263
src/slave/containerizer/mesos/launch.hpp
7c8b535746b5ce9add00afef86fdb6faefb5620e
src/slave/containerizer/mesos/launch.cpp
2f2d60e2011f60ec711d3b29fd2c157e30c83c34
src/tests/launch_tests.cpp PRE-CREATION
Diff: https://reviews.apache.org/r/31444/diff/
Testing
---
Manual testing only so far. This is harder to automate because we need a
self-contained chroot to execute something in... Suggestions welcome.
Thanks,
Ian Downes
Re: Review Request 36269: Update CHANGELOG to reflect obsolete code cleanup
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36269/#review90756 --- CHANGELOG (line 337) https://reviews.apache.org/r/36269/#comment143884 I would just put this under deprecations section. Also, mind updating MESOS-2058 in deprecation section to do s/deprecate/remove/ because its been removed. - Vinod Kone On July 7, 2015, 5:59 p.m., Jiang Yan Xu wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36269/ --- (Updated July 7, 2015, 5:59 p.m.) Review request for mesos and Adam B. Repository: mesos Description --- Per Vinod's comment on /r/36005 The `**Cleanup` section makes sense? Diffs - CHANGELOG 433924a0e614f061c448a8e85d0a2825567150dc Diff: https://reviews.apache.org/r/36269/diff/ Testing --- Thanks, Jiang Yan Xu
Re: Review Request 35986: Allow slave attributes flag take a value with ':'.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/35986/ --- (Updated July 7, 2015, 7:07 p.m.) Review request for mesos, Adam B and Isabel Jimenez. Bugs: MESOS-2868 https://issues.apache.org/jira/browse/MESOS-2868 Repository: mesos Description --- Allow slave attributes flag take a value with ':'. Diffs (updated) - src/common/attributes.cpp aab114e1a5932e3f218b850e1afc7f2ef0f10e21 src/tests/attributes_tests.cpp 2e10eaf3c33e418603ea19090bb0bf9179af71f7 Diff: https://reviews.apache.org/r/35986/diff/ Testing --- make check Thanks, haosdent huang
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 7, 2015, 12:42 p.m.) Review request for mesos, Chi Zhang, Paul Brett, Timothy Chen, and Vinod Kone. Changes --- Updated dependencies. 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 36275: MESOS-3005: Fix SSL test hostname dependency.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36275/ --- (Updated July 7, 2015, 8:04 p.m.) Review request for mesos, Benjamin Hindman, Artem Harutyunyan, and Michael Park. Changes --- rebased. Repository: mesos Description --- We generate the certificate using the hostname associated with INADDR_LOOPBACK and explicitly bind the test server on INADDR_LOOPBACK. This way there is no inconsistency with the hostname of the certificate versus the test. Diffs (updated) - 3rdparty/libprocess/src/tests/ssl_tests.cpp 869ed6572392e3cdcf0c0152bcca4b91130e3c04 Diff: https://reviews.apache.org/r/36275/diff/ Testing --- make check. verifying on other dev's systems. Thanks, Joris Van Remoortere
Re: Review Request 35986: Allow slave attributes flag take a value with ':'.
On July 7, 2015, 1:30 a.m., Adam B wrote: src/common/attributes.cpp, lines 150-152 https://reviews.apache.org/r/35986/diff/1/?file=994137#file994137line150 There's a subtle difference in behavior between strings::tokenize and strings::split. For tokenize, Empty tokens will not be included in the result. whereas for split, Empty tokens are allowed in the result. so you need to test not only that `pairs.size() == 2`, but also that `!pairs[0].empty()` and `!pairs[1].empty()`. Let's create unit tests for handling `:foo` and `foo:`. Hi, @adam-mesos, could not add test unit tests for handling :foo and foo:. Because use LOG(FATAL) in parse. - haosdent --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/35986/#review90623 --- On July 7, 2015, 5:55 p.m., haosdent huang wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/35986/ --- (Updated July 7, 2015, 5:55 p.m.) Review request for mesos, Adam B and Isabel Jimenez. Bugs: MESOS-2868 https://issues.apache.org/jira/browse/MESOS-2868 Repository: mesos Description --- Allow slave attributes flag take a value with ':'. Diffs - src/common/attributes.cpp aab114e1a5932e3f218b850e1afc7f2ef0f10e21 src/tests/attributes_tests.cpp 2e10eaf3c33e418603ea19090bb0bf9179af71f7 Diff: https://reviews.apache.org/r/35986/diff/ Testing --- make check Thanks, haosdent huang
Re: Review Request 36269: Update CHANGELOG to reflect obsolete code cleanup
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36269/#review90754 --- Do you actually want/need MESOS-2640 to go into 0.23.0? CHANGELOG (line 337) https://reviews.apache.org/r/36269/#comment143882 Do you actually want/need MESOS-2640 to go into 0.23.0? If so, MESOS-2640 should have its Target Version and Fix Version set to 0.23.0 and the release manager (me) should be notified. Since we've already cut rc1, we are only cherry-picking select patches into 0.23.0-rc2. Does this need to be one? If not, create a new `(WIP) Release Notes - Mesos - Version 0.24.0` section at the top to place your Cleanup ticket under. - Adam B On July 7, 2015, 10:59 a.m., Jiang Yan Xu wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36269/ --- (Updated July 7, 2015, 10:59 a.m.) Review request for mesos and Adam B. Repository: mesos Description --- Per Vinod's comment on /r/36005 The `**Cleanup` section makes sense? Diffs - CHANGELOG 433924a0e614f061c448a8e85d0a2825567150dc Diff: https://reviews.apache.org/r/36269/diff/ Testing --- Thanks, Jiang Yan Xu
Re: Review Request 36269: Update CHANGELOG to reflect obsolete code cleanup
On July 7, 2015, 11:40 a.m., Vinod Kone wrote: CHANGELOG, line 337 https://reviews.apache.org/r/36269/diff/1/?file=1001355#file1001355line337 I would just put this under deprecations section. Also, mind updating MESOS-2058 in deprecation section to do s/deprecate/remove/ because its been removed. Adam is against putting MESOS-2640 in 0.23.0 because it's committed after rc1 is cut. This review doesn't even need to land now. s/deprecate/remove/ on MESOS-2058 can be done separately. - Jiang Yan --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36269/#review90756 --- On July 7, 2015, 10:59 a.m., Jiang Yan Xu wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36269/ --- (Updated July 7, 2015, 10:59 a.m.) Review request for mesos and Adam B. Repository: mesos Description --- Per Vinod's comment on /r/36005 The `**Cleanup` section makes sense? Diffs - CHANGELOG 433924a0e614f061c448a8e85d0a2825567150dc Diff: https://reviews.apache.org/r/36269/diff/ Testing --- Thanks, Jiang Yan Xu
Re: Review Request 34141: AppC provsioning backend.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/34141/ --- (Updated July 7, 2015, 12:43 p.m.) Review request for mesos, Chi Zhang, Paul Brett, Timothy Chen, and Vinod Kone. Changes --- Updated dependencies. Repository: mesos Description --- Simple copy backend that forms the rootfs by copying each layer. Diffs - src/Makefile.am e7de0f3d1a5efeaef47d5074defe3b40db94f573 src/slave/containerizer/provisioners/appc/backend.hpp PRE-CREATION src/slave/containerizer/provisioners/appc/backend.cpp PRE-CREATION src/slave/flags.hpp 7634e368c72e83932dcd992d78eaca146326606b src/slave/flags.cpp cbf431eb0627bdaf07241cc0fc4630df06fb20e2 Diff: https://reviews.apache.org/r/34141/diff/ Testing --- Thanks, Ian Downes
Re: Review Request 34142: AppC provisioner.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/34142/ --- (Updated July 7, 2015, 12:43 p.m.) Review request for mesos, Chi Zhang, Paul Brett, Timothy Chen, and Vinod Kone. Repository: mesos Description --- Discovers image(s), fetches to the image store, then provisions using a backend. Diffs - include/mesos/mesos.proto 8df1211165169c9595e0e6e85b5ddc404345ff70 src/Makefile.am e7de0f3d1a5efeaef47d5074defe3b40db94f573 src/slave/containerizer/mesos/containerizer.cpp 8c102fb7d1f79ee768cb06de3a976ea12f958712 src/slave/containerizer/provisioners/appc.hpp PRE-CREATION src/slave/containerizer/provisioners/appc.cpp PRE-CREATION src/slave/flags.hpp 7634e368c72e83932dcd992d78eaca146326606b src/slave/flags.cpp cbf431eb0627bdaf07241cc0fc4630df06fb20e2 Diff: https://reviews.apache.org/r/34142/diff/ Testing --- Thanks, Ian Downes
Re: Review Request 36246: SSL: Fix connection issue on OSX.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36246/ --- (Updated July 7, 2015, 7:51 p.m.) Review request for mesos, Benjamin Hindman and Michael Park. Repository: mesos Description --- using the protocol based size for the `connect()` argument. Diffs - 3rdparty/libprocess/src/libevent_ssl_socket.cpp 9424dd421b290bec25b4a7c4dc0071ffef6fdc5e Diff: https://reviews.apache.org/r/36246/diff/ Testing --- Thanks, Joris Van Remoortere
Re: Review Request 35986: Allow slave attributes flag take a value with ':'.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/35986/#review90769 --- Patch looks great! Reviews applied: [35986] All tests passed. - Mesos ReviewBot On July 7, 2015, 7:07 p.m., haosdent huang wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/35986/ --- (Updated July 7, 2015, 7:07 p.m.) Review request for mesos, Adam B and Isabel Jimenez. Bugs: MESOS-2868 https://issues.apache.org/jira/browse/MESOS-2868 Repository: mesos Description --- Allow slave attributes flag take a value with ':'. Diffs - src/common/attributes.cpp aab114e1a5932e3f218b850e1afc7f2ef0f10e21 src/tests/attributes_tests.cpp 2e10eaf3c33e418603ea19090bb0bf9179af71f7 Diff: https://reviews.apache.org/r/35986/diff/ Testing --- make check Thanks, haosdent huang
Re: Review Request 35986: Allow slave attributes flag take a value with ':'.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/35986/ --- (Updated July 7, 2015, 6:33 p.m.) Review request for mesos, Adam B and Isabel Jimenez. Changes --- Sorry for so much noisy... Bugs: MESOS-2868 https://issues.apache.org/jira/browse/MESOS-2868 Repository: mesos Description --- Allow slave attributes flag take a value with ':'. Diffs (updated) - src/common/attributes.cpp aab114e1a5932e3f218b850e1afc7f2ef0f10e21 src/tests/attributes_tests.cpp 2e10eaf3c33e418603ea19090bb0bf9179af71f7 Diff: https://reviews.apache.org/r/35986/diff/ Testing --- make check Thanks, haosdent huang
Re: Review Request 31444: Support chrooting in MesosContainerizer launch helper.
---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/31444/
---
(Updated July 7, 2015, 11:33 a.m.)
Review request for mesos, Chi Zhang, Dominic Hamon, Jay Buffington, Jie Yu, and
James Peach.
Bugs: MESOS-2350
https://issues.apache.org/jira/browse/MESOS-2350
Repository: mesos
Description
---
Optionally take a path that the launch helper should chroot to before exec'ing
the executor. It is assumed that the work directory is mounted to the
appropriate location under the chroot. In particular, the path to the executor
must be relative to the chroot.
Configuration that should be private to the chroot is done during the launch,
e.g. mounting proc and statically configuring basic devices. It is assumed that
other configuration, e.g., preparing the image, mounting in volumes or
persistent resources, is done by the caller.
Mounts can be made to the chroot (e.g., updating the volumes or persistent
resources) and they will propagate in to the container but mounts made inside
the container will not propagate out to the host.
It currently assumes that at least {{chroot}}/tmp is writeable and that mount
points {{chroot}}/{tmp,dev,proc,sys} exist in the chroot.
This is specific to Linux.
Diffs
-
src/Makefile.am e7de0f3d1a5efeaef47d5074defe3b40db94f573
src/linux/fs.cpp 568565f878b34708170a886dc4d62849aa01f263
src/slave/containerizer/mesos/launch.hpp
7c8b535746b5ce9add00afef86fdb6faefb5620e
src/slave/containerizer/mesos/launch.cpp
2f2d60e2011f60ec711d3b29fd2c157e30c83c34
src/tests/launch_tests.cpp PRE-CREATION
Diff: https://reviews.apache.org/r/31444/diff/
Testing
---
Manual testing only so far. This is harder to automate because we need a
self-contained chroot to execute something in... Suggestions welcome.
Thanks,
Ian Downes
Re: Review Request 36269: Update CHANGELOG to reflect obsolete code cleanup
On July 7, 2015, 6:40 p.m., Vinod Kone wrote: CHANGELOG, line 337 https://reviews.apache.org/r/36269/diff/1/?file=1001355#file1001355line337 I would just put this under deprecations section. Also, mind updating MESOS-2058 in deprecation section to do s/deprecate/remove/ because its been removed. Jiang Yan Xu wrote: Adam is against putting MESOS-2640 in 0.23.0 because it's committed after rc1 is cut. This review doesn't even need to land now. s/deprecate/remove/ on MESOS-2058 can be done separately. SG. Yea I wanted a WIP 24.0 CHANGELOG too when I originially made that comment on the older review. Forgot about that. - Vinod --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36269/#review90756 --- On July 7, 2015, 5:59 p.m., Jiang Yan Xu wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36269/ --- (Updated July 7, 2015, 5:59 p.m.) Review request for mesos and Adam B. Repository: mesos Description --- Per Vinod's comment on /r/36005 The `**Cleanup` section makes sense? Diffs - CHANGELOG 433924a0e614f061c448a8e85d0a2825567150dc Diff: https://reviews.apache.org/r/36269/diff/ Testing --- Thanks, Jiang Yan Xu
Re: Review Request 34140: AppC image store
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/34140/ --- (Updated July 7, 2015, 12:43 p.m.) Review request for mesos, Chi Zhang, Paul Brett, Timothy Chen, and Vinod Kone. Changes --- Updated dependencies. Repository: mesos Description --- Images are fetched into the store (after discovery). Stored images are currently kept indefinitely. Diffs - src/Makefile.am e7de0f3d1a5efeaef47d5074defe3b40db94f573 src/slave/containerizer/provisioners/appc/store.hpp PRE-CREATION src/slave/containerizer/provisioners/appc/store.cpp PRE-CREATION src/slave/flags.hpp 7634e368c72e83932dcd992d78eaca146326606b src/slave/flags.cpp cbf431eb0627bdaf07241cc0fc4630df06fb20e2 Diff: https://reviews.apache.org/r/34140/diff/ Testing --- Thanks, Ian Downes
Re: Review Request 34139: AppC image discovery.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/34139/ --- (Updated July 7, 2015, 12:42 p.m.) Review request for mesos, Chi Zhang, Paul Brett, Timothy Chen, and Vinod Kone. Changes --- Updated dependencies. Repository: mesos Description --- Local and simple discovery only. Diffs - src/Makefile.am e7de0f3d1a5efeaef47d5074defe3b40db94f573 src/slave/containerizer/provisioners/appc/discovery.hpp PRE-CREATION src/slave/containerizer/provisioners/appc/discovery.cpp PRE-CREATION src/slave/flags.hpp 7634e368c72e83932dcd992d78eaca146326606b src/slave/flags.cpp cbf431eb0627bdaf07241cc0fc4630df06fb20e2 Diff: https://reviews.apache.org/r/34139/diff/ Testing --- Thanks, Ian Downes
Re: Review Request 34138: AppC hash computation.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/34138/ --- (Updated July 7, 2015, 12:42 p.m.) Review request for mesos, Chi Zhang, Paul Brett, Timothy Chen, and Vinod Kone. Changes --- Updated dependencies. Repository: mesos Description --- AppC hash computation. Diffs - src/Makefile.am e7de0f3d1a5efeaef47d5074defe3b40db94f573 src/slave/containerizer/provisioners/appc/hash.hpp PRE-CREATION Diff: https://reviews.apache.org/r/34138/diff/ Testing --- Thanks, Ian Downes
Re: Review Request 35986: Allow slave attributes flag take a value with ':'.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/35986/ --- (Updated July 7, 2015, 6:26 p.m.) Review request for mesos, Adam B and Isabel Jimenez. Bugs: MESOS-2868 https://issues.apache.org/jira/browse/MESOS-2868 Repository: mesos Description --- Allow slave attributes flag take a value with ':'. Diffs (updated) - Makefile.am f8e958d376efa55ace7c5727a31b8e747403641e configure.ac 546c9bbf775a4ef481fafb3a58c85c6d80e19500 docs/ec2-scripts.md PRE-CREATION docs/persistent-volume.md e3dfe6d785bbfda2489ba5d71cad043f290fb23a docs/tools.md 09619f2c162b1765e7718fc314e77b0f77148b97 docs/using-the-mesos-submit-tool.md PRE-CREATION ec2/Makefile.am PRE-CREATION ec2/deploy.amazon64-old/root/ephemeral-hdfs/conf/core-site.xml PRE-CREATION ec2/deploy.amazon64-old/root/ephemeral-hdfs/conf/hadoop-env.sh PRE-CREATION ec2/deploy.amazon64-old/root/ephemeral-hdfs/conf/hdfs-site.xml PRE-CREATION ec2/deploy.amazon64-old/root/ephemeral-hdfs/conf/mapred-site.xml PRE-CREATION ec2/deploy.amazon64-old/root/ephemeral-hdfs/conf/masters PRE-CREATION ec2/deploy.amazon64-old/root/ephemeral-hdfs/conf/slaves PRE-CREATION ec2/deploy.amazon64-old/root/mesos-ec2/cluster-url PRE-CREATION ec2/deploy.amazon64-old/root/mesos-ec2/copy-dir PRE-CREATION ec2/deploy.amazon64-old/root/mesos-ec2/create-swap PRE-CREATION ec2/deploy.amazon64-old/root/mesos-ec2/hadoop-framework-conf/core-site.xml PRE-CREATION ec2/deploy.amazon64-old/root/mesos-ec2/hadoop-framework-conf/hadoop-env.sh PRE-CREATION ec2/deploy.amazon64-old/root/mesos-ec2/hadoop-framework-conf/mapred-site.xml PRE-CREATION ec2/deploy.amazon64-old/root/mesos-ec2/haproxy+apache/haproxy.config.template PRE-CREATION ec2/deploy.amazon64-old/root/mesos-ec2/hypertable/Capfile PRE-CREATION ec2/deploy.amazon64-old/root/mesos-ec2/hypertable/hypertable.cfg PRE-CREATION ec2/deploy.amazon64-old/root/mesos-ec2/masters PRE-CREATION ec2/deploy.amazon64-old/root/mesos-ec2/mesos-daemon PRE-CREATION ec2/deploy.amazon64-old/root/mesos-ec2/redeploy-mesos PRE-CREATION ec2/deploy.amazon64-old/root/mesos-ec2/setup PRE-CREATION ec2/deploy.amazon64-old/root/mesos-ec2/setup-slave PRE-CREATION ec2/deploy.amazon64-old/root/mesos-ec2/setup-torque PRE-CREATION ec2/deploy.amazon64-old/root/mesos-ec2/slaves PRE-CREATION ec2/deploy.amazon64-old/root/mesos-ec2/ssh-no-keychecking PRE-CREATION ec2/deploy.amazon64-old/root/mesos-ec2/start-hypertable PRE-CREATION ec2/deploy.amazon64-old/root/mesos-ec2/start-mesos PRE-CREATION ec2/deploy.amazon64-old/root/mesos-ec2/stop-hypertable PRE-CREATION ec2/deploy.amazon64-old/root/mesos-ec2/stop-mesos PRE-CREATION ec2/deploy.amazon64-old/root/mesos-ec2/zoo PRE-CREATION ec2/deploy.amazon64-old/root/persistent-hdfs/conf/core-site.xml PRE-CREATION ec2/deploy.amazon64-old/root/persistent-hdfs/conf/hadoop-env.sh PRE-CREATION ec2/deploy.amazon64-old/root/persistent-hdfs/conf/hdfs-site.xml PRE-CREATION ec2/deploy.amazon64-old/root/persistent-hdfs/conf/mapred-site.xml PRE-CREATION ec2/deploy.amazon64-old/root/persistent-hdfs/conf/masters PRE-CREATION ec2/deploy.amazon64-old/root/persistent-hdfs/conf/slaves PRE-CREATION ec2/deploy.amazon64-old/root/spark/conf/spark-env.sh PRE-CREATION ec2/deploy.amazon64/root/ephemeral-hdfs/conf/core-site.xml PRE-CREATION ec2/deploy.amazon64/root/ephemeral-hdfs/conf/hadoop-env.sh PRE-CREATION ec2/deploy.amazon64/root/ephemeral-hdfs/conf/hdfs-site.xml PRE-CREATION ec2/deploy.amazon64/root/ephemeral-hdfs/conf/mapred-site.xml PRE-CREATION ec2/deploy.amazon64/root/ephemeral-hdfs/conf/masters PRE-CREATION ec2/deploy.amazon64/root/ephemeral-hdfs/conf/slaves PRE-CREATION ec2/deploy.amazon64/root/mesos-ec2/cluster-url PRE-CREATION ec2/deploy.amazon64/root/mesos-ec2/copy-dir PRE-CREATION ec2/deploy.amazon64/root/mesos-ec2/create-swap PRE-CREATION ec2/deploy.amazon64/root/mesos-ec2/hadoop-framework-conf/core-site.xml PRE-CREATION ec2/deploy.amazon64/root/mesos-ec2/hadoop-framework-conf/hadoop-env.sh PRE-CREATION ec2/deploy.amazon64/root/mesos-ec2/hadoop-framework-conf/mapred-site.xml PRE-CREATION ec2/deploy.amazon64/root/mesos-ec2/haproxy+apache/haproxy.config.template PRE-CREATION ec2/deploy.amazon64/root/mesos-ec2/hypertable/Capfile PRE-CREATION ec2/deploy.amazon64/root/mesos-ec2/hypertable/hypertable.cfg PRE-CREATION ec2/deploy.amazon64/root/mesos-ec2/masters PRE-CREATION ec2/deploy.amazon64/root/mesos-ec2/mesos-daemon PRE-CREATION ec2/deploy.amazon64/root/mesos-ec2/redeploy-mesos PRE-CREATION ec2/deploy.amazon64/root/mesos-ec2/setup PRE-CREATION ec2/deploy.amazon64/root/mesos-ec2/setup-slave PRE-CREATION ec2/deploy.amazon64/root/mesos-ec2/setup-torque PRE-CREATION
Re: Review Request 36267: MESOS-2943: Add comment for explicit return type.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36267/#review90761 --- Ship it! Ship It! - Artem Harutyunyan On July 7, 2015, 10:21 a.m., Joris Van Remoortere wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36267/ --- (Updated July 7, 2015, 10:21 a.m.) Review request for mesos, Benjamin Hindman, Artem Harutyunyan, and Michael Park. Bugs: MESOS-2943 https://issues.apache.org/jira/browse/MESOS-2943 Repository: mesos Description --- See summary. Diffs - 3rdparty/libprocess/src/libevent_ssl_socket.cpp 9424dd421b290bec25b4a7c4dc0071ffef6fdc5e Diff: https://reviews.apache.org/r/36267/diff/ Testing --- Only adding a comment. Thanks, Joris Van Remoortere
Review Request 36275: MESOS-3005: Fix SSL test hostname dependency.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36275/ --- Review request for mesos, Benjamin Hindman, Artem Harutyunyan, and Michael Park. Repository: mesos Description --- We generate the certificate using the hostname associated with INADDR_LOOPBACK and explicitly bind the test server on INADDR_LOOPBACK. This way there is no inconsistency with the hostname of the certificate versus the test. Diffs - 3rdparty/libprocess/src/tests/ssl_tests.cpp 869ed6572392e3cdcf0c0152bcca4b91130e3c04 Diff: https://reviews.apache.org/r/36275/diff/ Testing --- make check. verifying on other dev's systems. Thanks, Joris Van Remoortere
Re: Review Request 36246: SSL: Fix connection issue on OSX.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36246/ --- (Updated July 7, 2015, 8:04 p.m.) Review request for mesos, Benjamin Hindman and Michael Park. Changes --- rebased. Repository: mesos Description --- using the protocol based size for the `connect()` argument. Diffs (updated) - 3rdparty/libprocess/src/libevent_ssl_socket.cpp 1ef925cd8b6c6b07a64df9d409b9a25e9be539c9 Diff: https://reviews.apache.org/r/36246/diff/ Testing --- Thanks, Joris Van Remoortere
Re: Review Request 36204: Pass slave's total resources in ResourceUsage.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36204/ --- (Updated July 7, 2015, 4:20 p.m.) Review request for mesos, Ben Mahler, Jie Yu, Niklas Nielsen, Szymon Konefal, and Vinod Kone. Changes --- Slave's total resources now includes checkpointed dynamic reservations and persistent volumes. Bugs: MESOS-2933 https://issues.apache.org/jira/browse/MESOS-2933 Repository: mesos Description --- Pass slave's total resources to the ResourceEstimator and QoSController via Slave::usage(). Diffs (updated) - include/mesos/mesos.proto 3dd4a5b7a4b3bc56bdc690d6adf05f88c0d28273 src/slave/slave.cpp 008170f4ce7b97eda43b50b48083b32ae65925c0 Diff: https://reviews.apache.org/r/36204/diff/ Testing --- make check. Thanks, Bartek Plotka
Re: Review Request 36267: MESOS-2943: Add comment for explicit return type.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36267/#review90787 --- Ship it! Ship It! - Benjamin Hindman On July 7, 2015, 5:21 p.m., Joris Van Remoortere wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36267/ --- (Updated July 7, 2015, 5:21 p.m.) Review request for mesos, Benjamin Hindman, Artem Harutyunyan, and Michael Park. Bugs: MESOS-2943 https://issues.apache.org/jira/browse/MESOS-2943 Repository: mesos Description --- See summary. Diffs - 3rdparty/libprocess/src/libevent_ssl_socket.cpp 9424dd421b290bec25b4a7c4dc0071ffef6fdc5e Diff: https://reviews.apache.org/r/36267/diff/ Testing --- Only adding a comment. Thanks, Joris Van Remoortere
Review Request 36281: Document per-container unique egress flows and network queueing statistics.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36281/ --- Review request for mesos, Ian Downes, Jie Yu, and Cong Wang. Repository: mesos Description --- Document per-container unique egress flows and network queueing statistics. Diffs - docs/home.md bc277910907c381c08835b6e9d485b27d6da5002 docs/network-isolation.md PRE-CREATION docs/network-monitoring.md 8889fb165cc70bc382be0c99de8d7748328abf57 Diff: https://reviews.apache.org/r/36281/diff/ Testing --- Rendered at https://www.notehub.org/2015/7/7/network-isolation for review. Thanks, Paul Brett
Re: Review Request 36275: MESOS-3005: Fix SSL test hostname dependency.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36275/ --- (Updated July 7, 2015, 9:02 p.m.) Review request for mesos, Benjamin Hindman, Artem Harutyunyan, and Michael Park. Bugs: MESOS-3005 https://issues.apache.org/jira/browse/MESOS-3005 Repository: mesos Description --- We generate the certificate using the hostname associated with INADDR_LOOPBACK and explicitly bind the test server on INADDR_LOOPBACK. This way there is no inconsistency with the hostname of the certificate versus the test. Diffs - 3rdparty/libprocess/src/tests/ssl_tests.cpp 869ed6572392e3cdcf0c0152bcca4b91130e3c04 Diff: https://reviews.apache.org/r/36275/diff/ Testing --- make check. verifying on other dev's systems. Thanks, Joris Van Remoortere
Re: Review Request 34141: AppC provsioning backend.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/34141/#review90778 --- src/slave/containerizer/provisioners/appc/backend.cpp (line 139) https://reviews.apache.org/r/34141/#comment143901 Should we try to keep the logging of each image provisioning to a seperate log file in the image folder, instead of just writing out to stdout/stderr? - Timothy Chen On July 7, 2015, 7:43 p.m., Ian Downes wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/34141/ --- (Updated July 7, 2015, 7:43 p.m.) Review request for mesos, Chi Zhang, Paul Brett, Timothy Chen, and Vinod Kone. Repository: mesos Description --- Simple copy backend that forms the rootfs by copying each layer. Diffs - src/Makefile.am e7de0f3d1a5efeaef47d5074defe3b40db94f573 src/slave/containerizer/provisioners/appc/backend.hpp PRE-CREATION src/slave/containerizer/provisioners/appc/backend.cpp PRE-CREATION src/slave/flags.hpp 7634e368c72e83932dcd992d78eaca146326606b src/slave/flags.cpp cbf431eb0627bdaf07241cc0fc4630df06fb20e2 Diff: https://reviews.apache.org/r/34141/diff/ Testing --- Thanks, Ian Downes
Re: Review Request 36277: MESOS-3002: Fix getOrElse compilation error for network isolator.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36277/#review90782 --- Ship it! Ship It! - Adam B On July 7, 2015, 1:59 p.m., Joris Van Remoortere wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36277/ --- (Updated July 7, 2015, 1:59 p.m.) Review request for mesos and Benjamin Hindman. Bugs: MESOS-3002 https://issues.apache.org/jira/browse/MESOS-3002 Repository: mesos Description --- See summary. Diffs - src/slave/containerizer/isolators/network/port_mapping.cpp 2ef79eb9be76803d9a26223ee5763a582ca89736 Diff: https://reviews.apache.org/r/36277/diff/ Testing --- build with network isolator configured. Thanks, Joris Van Remoortere
Re: Review Request 36269: Update CHANGELOG to reflect obsolete code cleanup
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36269/#review90776 --- Patch looks great! Reviews applied: [36269] All tests passed. - Mesos ReviewBot On July 7, 2015, 5:59 p.m., Jiang Yan Xu wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36269/ --- (Updated July 7, 2015, 5:59 p.m.) Review request for mesos and Adam B. Repository: mesos Description --- Per Vinod's comment on /r/36005 The `**Cleanup` section makes sense? Diffs - CHANGELOG 433924a0e614f061c448a8e85d0a2825567150dc Diff: https://reviews.apache.org/r/36269/diff/ Testing --- Thanks, Jiang Yan Xu
Re: Review Request 35777: Made post-reviews.py handle bad (or not) ReviewBoard URLs.
On July 7, 2015, 9:42 p.m., Vinod Kone wrote:
support/post-reviews.py, line 173
https://reviews.apache.org/r/35777/diff/2/?file=1001308#file1001308line173
Why 'format' instead of directly printing the message with a %s? Just
curious.
It's a more portable way of printing - also, using `format()` allows for better
ways of formatting messages.
Personally, now I'm always using (if I still have to support Python 2.7):
```
from future import __print_function__
...
print(foo bar {baz}.format(baz=something))
```
but that's pedantic me :)
- Marco
---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/35777/#review90785
---
On July 7, 2015, 3:19 p.m., Michael Park wrote:
---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/35777/
---
(Updated July 7, 2015, 3:19 p.m.)
Review request for mesos, Benjamin Hindman, Marco Massenzio, and Till
Toenshoff.
Repository: mesos
Description
---
(1) Handles the case where the `Review: ...` line isn't the last of the
commit message.
```
01596ce802669e9dac7dd24193f041cef3354830 - Fixed post-reviews.py hanging bug.
(6 seconds ago)
ReviewBoard URL must be the last line of the commit message!
Fixed post-reviews.py hanging bug.
Review: https://reviews.apache.org/r/35771
abcd
```
(2) Handles the case where the ReviewBoard URL is invalid.
```
af081a07234794f60a2575e0383ce537d7111c18 - Fixed post-reviews.py hanging bug.
(29 seconds ago)
Invalid ReviewBoard URL : 'https://reviews.apache.org/r/35771 abcd'
```
Diffs
-
support/post-reviews.py b04e26b85e6b056098c15078f9b31dc352682351
Diff: https://reviews.apache.org/r/35777/diff/
Testing
---
Modified the commit message with `git rebase` and ran
`./support/post-reviews.py` to observe the above error messages.
Used the valid commit message for this patch and created it by running
`./support/post-reviews.py`.
Thanks,
Michael Park
Re: Review Request 36277: MESOS-3002: Fix getOrElse compilation error for network isolator.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36277/#review90784 --- Patch looks great! Reviews applied: [36277] All tests passed. - Mesos ReviewBot On July 7, 2015, 8:59 p.m., Joris Van Remoortere wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36277/ --- (Updated July 7, 2015, 8:59 p.m.) Review request for mesos and Benjamin Hindman. Bugs: MESOS-3002 https://issues.apache.org/jira/browse/MESOS-3002 Repository: mesos Description --- See summary. Diffs - src/slave/containerizer/isolators/network/port_mapping.cpp 2ef79eb9be76803d9a26223ee5763a582ca89736 Diff: https://reviews.apache.org/r/36277/diff/ Testing --- build with network isolator configured. Thanks, Joris Van Remoortere
Re: Review Request 35777: Made post-reviews.py handle bad (or not) ReviewBoard URLs.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/35777/#review90785 --- support/post-reviews.py (line 170) https://reviews.apache.org/r/35777/#comment143907 Jie recently refactored this file to avoid using hard coded strings/urls in this file so that this script can be used with internal (org specific) repos and review servers. Is there a way you can get the review board url from reviewboardrc isntead? support/post-reviews.py (line 173) https://reviews.apache.org/r/35777/#comment143908 Why 'format' instead of directly printing the message with a %s? Just curious. - Vinod Kone On July 7, 2015, 3:19 p.m., Michael Park wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/35777/ --- (Updated July 7, 2015, 3:19 p.m.) Review request for mesos, Benjamin Hindman, Marco Massenzio, and Till Toenshoff. Repository: mesos Description --- (1) Handles the case where the `Review: ...` line isn't the last of the commit message. ``` 01596ce802669e9dac7dd24193f041cef3354830 - Fixed post-reviews.py hanging bug. (6 seconds ago) ReviewBoard URL must be the last line of the commit message! Fixed post-reviews.py hanging bug. Review: https://reviews.apache.org/r/35771 abcd ``` (2) Handles the case where the ReviewBoard URL is invalid. ``` af081a07234794f60a2575e0383ce537d7111c18 - Fixed post-reviews.py hanging bug. (29 seconds ago) Invalid ReviewBoard URL : 'https://reviews.apache.org/r/35771 abcd' ``` Diffs - support/post-reviews.py b04e26b85e6b056098c15078f9b31dc352682351 Diff: https://reviews.apache.org/r/35777/diff/ Testing --- Modified the commit message with `git rebase` and ran `./support/post-reviews.py` to observe the above error messages. Used the valid commit message for this patch and created it by running `./support/post-reviews.py`. Thanks, Michael Park
Re: Review Request 36277: MESOS-3002: Fix getOrElse compilation error for network isolator.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36277/#review90786 --- Ship it! Ship It! - Benjamin Hindman On July 7, 2015, 8:59 p.m., Joris Van Remoortere wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36277/ --- (Updated July 7, 2015, 8:59 p.m.) Review request for mesos and Benjamin Hindman. Bugs: MESOS-3002 https://issues.apache.org/jira/browse/MESOS-3002 Repository: mesos Description --- See summary. Diffs - src/slave/containerizer/isolators/network/port_mapping.cpp 2ef79eb9be76803d9a26223ee5763a582ca89736 Diff: https://reviews.apache.org/r/36277/diff/ Testing --- build with network isolator configured. Thanks, Joris Van Remoortere
Re: Review Request 36246: SSL: Fix connection issue on OSX.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36246/#review90788 --- Ship it! Ship It! - Benjamin Hindman On July 7, 2015, 8:04 p.m., Joris Van Remoortere wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36246/ --- (Updated July 7, 2015, 8:04 p.m.) Review request for mesos, Benjamin Hindman and Michael Park. Repository: mesos Description --- using the protocol based size for the `connect()` argument. Diffs - 3rdparty/libprocess/src/libevent_ssl_socket.cpp 1ef925cd8b6c6b07a64df9d409b9a25e9be539c9 Diff: https://reviews.apache.org/r/36246/diff/ Testing --- Thanks, Joris Van Remoortere
Re: Review Request 36049: Added support for modularized Authorizer
---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/36049/#review90774
---
There are some nits and slight inconsistencies but overall I think we are in
good shape here.
src/local/local.cpp (line 217)
https://reviews.apache.org/r/36049/#comment143899
Capital The please.
src/local/local.cpp (line 220)
https://reviews.apache.org/r/36049/#comment143898
Please start with a capital Add after that colon.
src/local/local.cpp (lines 227 - 228)
https://reviews.apache.org/r/36049/#comment143900
I think we should rephrase the message here;
```
Could not create ' flags.authorizers ' authorizer:
create.error()
```
src/local/local.cpp (line 232)
https://reviews.apache.org/r/36049/#comment143903
For validating the configuration, I always found it very helpful that we
were showing the activated authenticator name/s in the master log -- hence I
would like to suggest to do the same here as well;
```
LOG(INFO) Using ' flags.authorizers ' authorizer;
```
src/local/local.cpp (line 234)
https://reviews.apache.org/r/36049/#comment143909
I am assuming that the `LocalAuthorizer` should be considered unusable
should its initialize function ever fail.
My most favored solution here would be to log the failure and make sure
that `authorizer` remains unset so that we can operate without any
authorization. That would be following the approach of the authenticator
`initialize` failure handling.
```
TryNothing initialize = authorizer.get()-initialize(flags.acls.get());
if (initialize.isError()) {
// A failure to initialize the authorizer does lead to unusable
authorization
// but allows actions to skip authorization.
LOG(WARNING) Authorization is disabled: Failed to initialize '
flags.authorizers ' authorizer:
initialize.error();
delete authorizer.get();
authorizer = None();
}
```
Inherited from
https://github.com/apache/mesos/blob/master/src/master/master.cpp#L484
src/master/flags.cpp (line 230)
https://reviews.apache.org/r/36049/#comment143910
s/authorizer/authorizers/
src/master/flags.cpp (line 231)
https://reviews.apache.org/r/36049/#comment143911
Lets make sure we match the flag name and also replace that default by
the actual implementation name.
```
Note that if the flag --authorizers is provided with a value different\n
than ' + DEFAULT_AUTHORIZER + ', the ACLs contents will be ignored.\n
\n
```
src/master/flags.cpp (line 421)
https://reviews.apache.org/r/36049/#comment143912
s/authorizer/authorizers/
Please sure you check if you properly renamed that flag in all references.
Thanks Alexander :)
src/master/flags.cpp (lines 423 - 424)
https://reviews.apache.org/r/36049/#comment143913
That looks like weird wrapping to me.
src/master/main.cpp (lines 301 - 317)
https://reviews.apache.org/r/36049/#comment143916
See my comments on local.cpp starting at line 217 ff. regarding this entire
block.
- Till Toenshoff
On July 7, 2015, 7:34 a.m., Alexander Rojas wrote:
---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/36049/
---
(Updated July 7, 2015, 7:34 a.m.)
Review request for mesos, Adam B and Till Toenshoff.
Bugs: MESOS-2947
https://issues.apache.org/jira/browse/MESOS-2947
Repository: mesos
Description
---
Adds and integrates helper classes needed to support an `Authorizer` module.
Also adds a flag to the master, allowing the selection of an `Authorizer`
module.
Diffs
-
include/mesos/authorizer/authorizer.hpp PRE-CREATION
include/mesos/module/authorizer.hpp PRE-CREATION
src/Makefile.am addb63f615f16ae6b25f745b2e79fd9fc0e27851
src/authorizer/authorizer.cpp PRE-CREATION
src/local/local.cpp 1953d84c75a83f4ace944d6243456235d8a193ff
src/master/constants.hpp 7cec18b7fdfd3b96cde42a30d217c026b2695dce
src/master/constants.cpp fbcae60c43e835f96ec061bd0e9f7961e31fc341
src/master/flags.hpp f2cd19a6edfaa4e5bb31f024ef8d5beda32fbc2f
src/master/flags.cpp 60ac64d98d53f74f904846b27a3833a7c44a9756
src/master/main.cpp 2624b7ea4920a534c98f5dfbf9286c54c50f11a9
src/module/manager.cpp 909ca56eea85d365cb9ebe1b3cce43051cabb670
src/tests/cluster.hpp cfe7ef0c7a6dc62cddc3e5f5b5b28c8bcb2bed26
Diff: https://reviews.apache.org/r/36049/diff/
Testing
---
make check
Thanks,
Alexander Rojas
Re: Review Request 36282: Remove os environment for docker executor enviornment setup.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36282/#review90802 --- Question about MESOS-2832, but otherwise looks good. src/slave/containerizer/docker.hpp (line 239) https://reviews.apache.org/r/36282/#comment143934 Should executorEnvironment be static as well? src/slave/containerizer/docker.cpp (line 1552) https://reviews.apache.org/r/36282/#comment143931 Filter? How does this cooperate with our fix for MESOS-2832 where `--executor_environment_variables` which lets an admin specify the environment variables that should be passed to an executor? Should we filter even if that flag is set? - Adam B On July 7, 2015, 3:10 p.m., Timothy Chen wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36282/ --- (Updated July 7, 2015, 3:10 p.m.) Review request for mesos, Adam B, Benjamin Hindman, and Joerg Schad. Bugs: MESOS-2996 https://issues.apache.org/jira/browse/MESOS-2996 Repository: mesos Description --- Remove os environment when calling executorEnvironment when running docker executors, since all the environment variables will be passed into the container and causes bad behavior such as overriding hostname. Diffs - src/slave/containerizer/docker.hpp 646a277be21deaded47324bea5474bd68699f25f src/slave/containerizer/docker.cpp 6eb1c84b1a6ef17c3e2ea2028e2f2d75110176ba Diff: https://reviews.apache.org/r/36282/diff/ Testing --- make check Thanks, Timothy Chen
Re: Review Request 36106: cgroups: added cpuacct subsystem
On July 2, 2015, 11:32 p.m., Ben Mahler wrote: src/linux/cgroups.hpp, lines 443-472 https://reviews.apache.org/r/36106/diff/1/?file=997646#file997646line443 Thanks! (1) Do you mind updating my TODO on cgroups::stat() to reflect that cpuacct::stat is implemented? (2) Can we just make this a simple struct with two non-const fields and remove the parse method and getters? Keep it simple and small, if we want immutability we can just have a 'const Stat' rather than forcing it on the caller :) (3) Any reason not to use 'Duration' for these fields? Jojy Varghese wrote: 1) Absolutely I can. 2) I wanted to reflect the semantics of the stats call. When you make a stats call, the data you get is immutable. By forcing external const, it would imply that the value is immutable. 3) Duration is a period ( i think) and the values here are absolute values derived from ticks. Joris Van Remoortere wrote: Regarding 3): Duration indeed represents an amount of time, rather than a specific point in time. I believe this is what we intend to represent in this code as well, right? The comments for your functions suggest an amount of time rather than a point in time. Regarding 1): I am working on another patch that addresses the containerizer (docker) using this API. I will also change the cpushare code along with that patch. - Jojy --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36106/#review90288 --- On July 7, 2015, 12:26 a.m., Jojy Varghese wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36106/ --- (Updated July 7, 2015, 12:26 a.m.) Review request for mesos, Ian Downes, Jie Yu, Joris Van Remoortere, and Timothy Chen. 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 - 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 36282: Remove os environment for docker executor enviornment setup.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36282/ --- (Updated July 7, 2015, 11:05 p.m.) Review request for mesos, Adam B, Benjamin Hindman, and Joerg Schad. Bugs: MESOS-2996 https://issues.apache.org/jira/browse/MESOS-2996 Repository: mesos Description --- Remove os environment when calling executorEnvironment when running docker executors, since all the environment variables will be passed into the container and causes bad behavior such as overriding hostname. Diffs (updated) - src/slave/containerizer/containerizer.hpp 0ee17e6bc52d1e3acefad6bda3a1b7ba64a8a54b src/slave/containerizer/containerizer.cpp 69dfac04cfd9c388fc908b68ac7abbc14304e621 src/slave/containerizer/docker.hpp 646a277be21deaded47324bea5474bd68699f25f src/slave/containerizer/docker.cpp 6eb1c84b1a6ef17c3e2ea2028e2f2d75110176ba Diff: https://reviews.apache.org/r/36282/diff/ Testing --- make check Thanks, Timothy Chen
Re: Review Request 36282: Remove os environment for docker executor enviornment setup.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36282/#review90821 --- Updated the review to use a new optional boolean flag. The docker bridge test passes with this as well. Once I have a new ship it then I can merge. - Timothy Chen On July 7, 2015, 11:05 p.m., Timothy Chen wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36282/ --- (Updated July 7, 2015, 11:05 p.m.) Review request for mesos, Adam B, Benjamin Hindman, and Joerg Schad. Bugs: MESOS-2996 https://issues.apache.org/jira/browse/MESOS-2996 Repository: mesos Description --- Remove os environment when calling executorEnvironment when running docker executors, since all the environment variables will be passed into the container and causes bad behavior such as overriding hostname. Diffs - src/slave/containerizer/containerizer.hpp 0ee17e6bc52d1e3acefad6bda3a1b7ba64a8a54b src/slave/containerizer/containerizer.cpp 69dfac04cfd9c388fc908b68ac7abbc14304e621 src/slave/containerizer/docker.hpp 646a277be21deaded47324bea5474bd68699f25f src/slave/containerizer/docker.cpp 6eb1c84b1a6ef17c3e2ea2028e2f2d75110176ba Diff: https://reviews.apache.org/r/36282/diff/ Testing --- make check Thanks, Timothy Chen
Re: Review Request 36226: Missing Apache headers for libprocess 3rdparty
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36226/#review90832 --- 3rdparty/libprocess/3rdparty/Makefile.am (lines 1 - 7) https://reviews.apache.org/r/36226/#comment143980 This should be the “Apache License Version 2.0” header instead which applies to libprocess and stout, no? (see docs/mesos-c++-styleguide.md at File Headers). - Till Toenshoff On July 6, 2015, 10:17 p.m., Isabel Jimenez wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36226/ --- (Updated July 6, 2015, 10:17 p.m.) Review request for mesos and Benjamin Hindman. Repository: mesos-incubating Description --- Adding missing Apache licence header Diffs - 3rdparty/libprocess/3rdparty/Makefile.am 519e38c Diff: https://reviews.apache.org/r/36226/diff/ Testing --- Thanks, Isabel Jimenez
Re: Review Request 36281: Document per-container unique egress flows and network queueing statistics.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36281/ --- (Updated July 8, 2015, 12:03 a.m.) Review request for mesos, Ian Downes, Jie Yu, and Cong Wang. Changes --- Incorporate comments from Ian Downes Repository: mesos Description --- Document per-container unique egress flows and network queueing statistics. Diffs (updated) - docs/home.md bc277910907c381c08835b6e9d485b27d6da5002 docs/network-isolation.md PRE-CREATION docs/network-monitoring.md 8889fb165cc70bc382be0c99de8d7748328abf57 Diff: https://reviews.apache.org/r/36281/diff/ Testing --- Rendered at https://www.notehub.org/2015/7/7/network-isolation for review. Thanks, Paul Brett
Review Request 36282: Remove os environment for docker executor enviornment setup.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36282/ --- Review request for mesos, Benjamin Hindman and Joerg Schad. Bugs: MESOS-2996 https://issues.apache.org/jira/browse/MESOS-2996 Repository: mesos Description --- Remove os environment when calling executorEnvironment when running docker executors, since all the environment variables will be passed into the container and causes bad behavior such as overriding hostname. Diffs - src/slave/containerizer/docker.hpp 646a277be21deaded47324bea5474bd68699f25f src/slave/containerizer/docker.cpp 6eb1c84b1a6ef17c3e2ea2028e2f2d75110176ba Diff: https://reviews.apache.org/r/36282/diff/ Testing --- make check Thanks, Timothy Chen
Re: Review Request 36282: Remove os environment for docker executor enviornment setup.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36282/#review90796 --- Ship it! Looks good but see the issue below about moving this inside the translation unit. src/slave/containerizer/docker.hpp (line 239) https://reviews.apache.org/r/36282/#comment143920 Let's not expose this in a public header since it's only used internally and instead let's just keep it as a static function inside of src/slave/containerizer/docker.cpp. src/slave/containerizer/docker.cpp (line 1551) https://reviews.apache.org/r/36282/#comment143921 Newline above this please! src/slave/containerizer/docker.cpp (line 1552) https://reviews.apache.org/r/36282/#comment143923 s/the os/the current process/ - Benjamin Hindman On July 7, 2015, 10:10 p.m., Timothy Chen wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36282/ --- (Updated July 7, 2015, 10:10 p.m.) Review request for mesos, Adam B, Benjamin Hindman, and Joerg Schad. Bugs: MESOS-2996 https://issues.apache.org/jira/browse/MESOS-2996 Repository: mesos Description --- Remove os environment when calling executorEnvironment when running docker executors, since all the environment variables will be passed into the container and causes bad behavior such as overriding hostname. Diffs - src/slave/containerizer/docker.hpp 646a277be21deaded47324bea5474bd68699f25f src/slave/containerizer/docker.cpp 6eb1c84b1a6ef17c3e2ea2028e2f2d75110176ba Diff: https://reviews.apache.org/r/36282/diff/ Testing --- make check Thanks, Timothy Chen
Re: Review Request 36282: Remove os environment for docker executor enviornment setup.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36282/#review90812 --- src/slave/containerizer/docker.hpp (line 239) https://reviews.apache.org/r/36282/#comment143952 It's actually being used in the header if you see below. - Timothy Chen On July 7, 2015, 10:10 p.m., Timothy Chen wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36282/ --- (Updated July 7, 2015, 10:10 p.m.) Review request for mesos, Adam B, Benjamin Hindman, and Joerg Schad. Bugs: MESOS-2996 https://issues.apache.org/jira/browse/MESOS-2996 Repository: mesos Description --- Remove os environment when calling executorEnvironment when running docker executors, since all the environment variables will be passed into the container and causes bad behavior such as overriding hostname. Diffs - src/slave/containerizer/docker.hpp 646a277be21deaded47324bea5474bd68699f25f src/slave/containerizer/docker.cpp 6eb1c84b1a6ef17c3e2ea2028e2f2d75110176ba Diff: https://reviews.apache.org/r/36282/diff/ Testing --- make check Thanks, Timothy Chen
Re: Review Request 36282: Remove os environment for docker executor enviornment setup.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36282/#review90814 --- src/slave/containerizer/docker.cpp (line 1544) https://reviews.apache.org/r/36282/#comment143955 Wouldn't it be easier --especially considering Adam's comment about MESOS-2832 and the --executor_environment_variable --- to add an additional (optional) flag to executorEnvironment intialize=true. if explicitly set to false (such as in this case, we would not initialize environment with os::environment() - Joerg Schad On July 7, 2015, 10:10 p.m., Timothy Chen wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36282/ --- (Updated July 7, 2015, 10:10 p.m.) Review request for mesos, Adam B, Benjamin Hindman, and Joerg Schad. Bugs: MESOS-2996 https://issues.apache.org/jira/browse/MESOS-2996 Repository: mesos Description --- Remove os environment when calling executorEnvironment when running docker executors, since all the environment variables will be passed into the container and causes bad behavior such as overriding hostname. Diffs - src/slave/containerizer/docker.hpp 646a277be21deaded47324bea5474bd68699f25f src/slave/containerizer/docker.cpp 6eb1c84b1a6ef17c3e2ea2028e2f2d75110176ba Diff: https://reviews.apache.org/r/36282/diff/ Testing --- make check Thanks, Timothy Chen
Re: Review Request 36281: Document per-container unique egress flows and network queueing statistics.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36281/#review90819 --- docs/network-isolation.md (line 29) https://reviews.apache.org/r/36281/#comment143960 libnl3-devel is the package on Red Hat distribution, not for other disto. docs/network-isolation.md (line 48) https://reviews.apache.org/r/36281/#comment143963 Please explicitly mention that currently binding to an unassigned port is allowed by kernel too (we need a patch to disallow this), just that packets will be dropped silently. docs/network-isolation.md (line 81) https://reviews.apache.org/r/36281/#comment143964 s/Separating container traffic/Egress traffic isolation/ docs/network-isolation.md (line 83) https://reviews.apache.org/r/36281/#comment143969 That is called flow classification and isolation. Please also mention that flow is classified based on port ranges. - Cong Wang On July 7, 2015, 9:54 p.m., Paul Brett wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36281/ --- (Updated July 7, 2015, 9:54 p.m.) Review request for mesos, Ian Downes, Jie Yu, and Cong Wang. Repository: mesos Description --- Document per-container unique egress flows and network queueing statistics. Diffs - docs/home.md bc277910907c381c08835b6e9d485b27d6da5002 docs/network-isolation.md PRE-CREATION docs/network-monitoring.md 8889fb165cc70bc382be0c99de8d7748328abf57 Diff: https://reviews.apache.org/r/36281/diff/ Testing --- Rendered at https://www.notehub.org/2015/7/7/network-isolation for review. Thanks, Paul Brett
Re: Review Request 36282: Remove os environment for docker executor enviornment setup.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36282/#review90826 --- Ship it! Barring Adams comment. src/slave/containerizer/containerizer.hpp (line 152) https://reviews.apache.org/r/36282/#comment143967 Shouldnt it be called `includeOSEnvironment` instead? Bit unsure right now but please consider that :) - Till Toenshoff On July 7, 2015, 11:05 p.m., Timothy Chen wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36282/ --- (Updated July 7, 2015, 11:05 p.m.) Review request for mesos, Adam B, Benjamin Hindman, and Joerg Schad. Bugs: MESOS-2996 https://issues.apache.org/jira/browse/MESOS-2996 Repository: mesos Description --- Remove os environment when calling executorEnvironment when running docker executors, since all the environment variables will be passed into the container and causes bad behavior such as overriding hostname. Diffs - src/slave/containerizer/containerizer.hpp 0ee17e6bc52d1e3acefad6bda3a1b7ba64a8a54b src/slave/containerizer/containerizer.cpp 69dfac04cfd9c388fc908b68ac7abbc14304e621 src/slave/containerizer/docker.hpp 646a277be21deaded47324bea5474bd68699f25f src/slave/containerizer/docker.cpp 6eb1c84b1a6ef17c3e2ea2028e2f2d75110176ba Diff: https://reviews.apache.org/r/36282/diff/ Testing --- make check Thanks, Timothy Chen
Re: Review Request 36226: Missing Apache headers for libprocess 3rdparty
On July 7, 2015, 11:43 p.m., Till Toenshoff wrote: 3rdparty/libprocess/3rdparty/Makefile.am, lines 1-7 https://reviews.apache.org/r/36226/diff/1/?file=1000612#file1000612line1 This should be the “Apache License Version 2.0” header instead which applies to libprocess and stout, no? (see docs/mesos-c++-styleguide.md at File Headers). Yes, Till, used the wrong script for this one. My bad. - Isabel --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36226/#review90832 --- On July 8, 2015, 12:12 a.m., Isabel Jimenez wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36226/ --- (Updated July 8, 2015, 12:12 a.m.) Review request for mesos and Benjamin Hindman. Repository: mesos-incubating Description --- Adding missing Apache licence header Diffs - 3rdparty/libprocess/3rdparty/Makefile.am 519e38c Diff: https://reviews.apache.org/r/36226/diff/ Testing --- Thanks, Isabel Jimenez
Re: Review Request 36226: Missing Apache headers for libprocess 3rdparty
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36226/ --- (Updated July 8, 2015, 12:12 a.m.) Review request for mesos and Benjamin Hindman. Repository: mesos-incubating Description --- Adding missing Apache licence header Diffs (updated) - 3rdparty/libprocess/3rdparty/Makefile.am 519e38c Diff: https://reviews.apache.org/r/36226/diff/ Testing --- Thanks, Isabel Jimenez
Re: Review Request 36273: Doxygen-ification of comments in libprocess process headers.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36273/ --- (Updated July 7, 2015, 5:24 p.m.) Review request for mesos, Benjamin Hindman, Artem Harutyunyan, and Joerg Schad. Repository: mesos Description --- Doxygen-ification of comments in libprocess process headers. Diffs - 3rdparty/libprocess/include/process/process.hpp 59b50af86e059463a01f3c83701bc5fd143d51a4 Diff: https://reviews.apache.org/r/36273/diff/ Testing --- `doxygen ../Doxyfile` and visually verified the HTML output. Thanks, Joseph Wu
Re: Review Request 36275: MESOS-3005: Fix SSL test hostname dependency.
On July 7, 2015, 10:08 p.m., Benjamin Hindman wrote: Added comment and committed, thanks. - Benjamin --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36275/#review90792 --- On July 7, 2015, 9:02 p.m., Joris Van Remoortere wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36275/ --- (Updated July 7, 2015, 9:02 p.m.) Review request for mesos, Benjamin Hindman, Artem Harutyunyan, and Michael Park. Bugs: MESOS-3005 https://issues.apache.org/jira/browse/MESOS-3005 Repository: mesos Description --- We generate the certificate using the hostname associated with INADDR_LOOPBACK and explicitly bind the test server on INADDR_LOOPBACK. This way there is no inconsistency with the hostname of the certificate versus the test. Diffs - 3rdparty/libprocess/src/tests/ssl_tests.cpp 869ed6572392e3cdcf0c0152bcca4b91130e3c04 Diff: https://reviews.apache.org/r/36275/diff/ Testing --- make check. verifying on other dev's systems. Thanks, Joris Van Remoortere
Re: Review Request 36275: MESOS-3005: Fix SSL test hostname dependency.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36275/#review90792 --- Ship it! 3rdparty/libprocess/src/tests/ssl_tests.cpp (line 312) https://reviews.apache.org/r/36275/#comment143919 I think a brief comment here and below on why we need to do a `bind` is helpful. - Benjamin Hindman On July 7, 2015, 9:02 p.m., Joris Van Remoortere wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36275/ --- (Updated July 7, 2015, 9:02 p.m.) Review request for mesos, Benjamin Hindman, Artem Harutyunyan, and Michael Park. Bugs: MESOS-3005 https://issues.apache.org/jira/browse/MESOS-3005 Repository: mesos Description --- We generate the certificate using the hostname associated with INADDR_LOOPBACK and explicitly bind the test server on INADDR_LOOPBACK. This way there is no inconsistency with the hostname of the certificate versus the test. Diffs - 3rdparty/libprocess/src/tests/ssl_tests.cpp 869ed6572392e3cdcf0c0152bcca4b91130e3c04 Diff: https://reviews.apache.org/r/36275/diff/ Testing --- make check. verifying on other dev's systems. Thanks, Joris Van Remoortere
Re: Review Request 34138: AppC hash computation.
---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/34138/#review90791
---
1. Agree that this is useful as a utility in libprocess. Not much overhead to
move it over right?
2. It feels like something that could be exposed as a function rather than
class, maybe a TODO.
src/slave/containerizer/provisioners/appc/hash.hpp (lines 22 - 31)
https://reviews.apache.org/r/34138/#comment143922
```
#include string
#include vector
#include stout/...
#include process/...
...
```
According to the style guide.
src/slave/containerizer/provisioners/appc/hash.hpp (lines 45 - 55)
https://reviews.apache.org/r/34138/#comment143917
I would do
```
if (!system(sha512sum -h /dev/null)) {...}
```
to avoid fixing the binary location.
src/slave/containerizer/provisioners/appc/hash.hpp (lines 89 - 91)
https://reviews.apache.org/r/34138/#comment143948
I think we use 4 spaces to continue a left angle bracket the same way we
continue an left paren.
src/slave/containerizer/provisioners/appc/hash.hpp (line 97)
https://reviews.apache.org/r/34138/#comment143939
The `command` is not necessarily `sha512sum`. Maybe use it a static member
so we detect once and use it with every hash() invocation?
src/slave/containerizer/provisioners/appc/hash.hpp (line 98)
https://reviews.apache.org/r/34138/#comment143940
Misaligned indentation.
src/slave/containerizer/provisioners/appc/hash.hpp (line 102)
https://reviews.apache.org/r/34138/#comment143949
The `command` is not necessarily `sha512sum`.
src/slave/containerizer/provisioners/appc/hash.hpp (line 109)
https://reviews.apache.org/r/34138/#comment143941
Misaligned indentation.
src/slave/containerizer/provisioners/appc/hash.hpp (lines 112 - 122)
https://reviews.apache.org/r/34138/#comment143946
This check doesn't work with openssl.
```
/usr/bin/openssl dgst -sha512 somefile.txt
SHA512(somefile.txt)=
5a73e55fd845981be5d5b87039c678b87404405d5d054c579cf684a18893d181085b9afde535c034221f858d2bcc2b14978b4d5f4d6facfaa1f81e727a010f3c
```
I think we only need shasum and sha512sum to cover both Linux and OSX.
- Jiang Yan Xu
On July 7, 2015, 12:42 p.m., Ian Downes wrote:
---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/34138/
---
(Updated July 7, 2015, 12:42 p.m.)
Review request for mesos, Chi Zhang, Paul Brett, Timothy Chen, and Vinod Kone.
Repository: mesos
Description
---
AppC hash computation.
Diffs
-
src/Makefile.am e7de0f3d1a5efeaef47d5074defe3b40db94f573
src/slave/containerizer/provisioners/appc/hash.hpp PRE-CREATION
Diff: https://reviews.apache.org/r/34138/diff/
Testing
---
Thanks,
Ian Downes
Re: Review Request 36282: Remove os environment for docker executor enviornment setup.
On July 7, 2015, 10:58 p.m., Joerg Schad wrote: src/slave/containerizer/docker.cpp, line 1544 https://reviews.apache.org/r/36282/diff/1/?file=1001811#file1001811line1544 Wouldn't it be easier --especially considering Adam's comment about MESOS-2832 and the --executor_environment_variable --- to add an additional (optional) flag to executorEnvironment intialize=true. if explicitly set to false (such as in this case, we would not initialize environment with os::environment() I like this approach better as well - seems much cleaner to me. - Till --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36282/#review90814 --- On July 7, 2015, 10:10 p.m., Timothy Chen wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36282/ --- (Updated July 7, 2015, 10:10 p.m.) Review request for mesos, Adam B, Benjamin Hindman, and Joerg Schad. Bugs: MESOS-2996 https://issues.apache.org/jira/browse/MESOS-2996 Repository: mesos Description --- Remove os environment when calling executorEnvironment when running docker executors, since all the environment variables will be passed into the container and causes bad behavior such as overriding hostname. Diffs - src/slave/containerizer/docker.hpp 646a277be21deaded47324bea5474bd68699f25f src/slave/containerizer/docker.cpp 6eb1c84b1a6ef17c3e2ea2028e2f2d75110176ba Diff: https://reviews.apache.org/r/36282/diff/ Testing --- make check Thanks, Timothy Chen
Re: Review Request 36281: Document per-container unique egress flows and network queueing statistics.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36281/#review90828 --- Bad patch! Reviews applied: [36281] Failed command: ./support/apply-review.sh -n -r 36281 Error: 2015-07-07 23:20:54 URL:https://reviews.apache.org/r/36281/diff/raw/ [21625/21625] - 36281.patch [1] 36281.patch:59: trailing whitespace. Network isolation is enabled on the slave by appending `network/port_mapping` to the slave command line. If the slave has not been compiled with network isolation support, it will refuse to start and print an appropriate error. 36281.patch:297: new blank line at EOF. + warning: 2 lines add whitespace errors. Successfully applied: Document per-container unique egress flows and network queueing statistics. Document per-container unique egress flows and network queueing statistics. Review: https://reviews.apache.org/r/36281 docs/network-isolation.md:40: trailing whitespace. +Network isolation is enabled on the slave by appending `network/port_mapping` to the slave command line. If the slave has not been compiled with network isolation support, it will refuse to start and print an appropriate error. docs/network-isolation.md:278: new blank line at EOF. Failed to commit patch - Mesos ReviewBot On July 7, 2015, 9:54 p.m., Paul Brett wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36281/ --- (Updated July 7, 2015, 9:54 p.m.) Review request for mesos, Ian Downes, Jie Yu, and Cong Wang. Repository: mesos Description --- Document per-container unique egress flows and network queueing statistics. Diffs - docs/home.md bc277910907c381c08835b6e9d485b27d6da5002 docs/network-isolation.md PRE-CREATION docs/network-monitoring.md 8889fb165cc70bc382be0c99de8d7748328abf57 Diff: https://reviews.apache.org/r/36281/diff/ Testing --- Rendered at https://www.notehub.org/2015/7/7/network-isolation for review. Thanks, Paul Brett
Re: Review Request 36281: Document per-container unique egress flows and network queueing statistics.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36281/#review90827 --- Thanks for writing this up! I had a few minor suggestions and some questions. docs/network-isolation.md (line 11) https://reviews.apache.org/r/36281/#comment143970 follow the following is a little redundant. docs/network-isolation.md (line 15) https://reviews.apache.org/r/36281/#comment143971 s/kernels versions/kernel versions/ docs/network-isolation.md (lines 24 - 29) https://reviews.apache.org/r/36281/#comment143974 Do these dependencies need to be added to getting-started.md? docs/network-isolation.md (line 31) https://reviews.apache.org/r/36281/#comment143975 `### Build Configuration`? docs/network-isolation.md (lines 40 - 42) https://reviews.apache.org/r/36281/#comment143976 I would put the parameter example between these two sentences. Where it is now, I expect to see the appropriate error just described. docs/network-isolation.md (line 48) https://reviews.apache.org/r/36281/#comment143978 s/so that service discovery/so that the service discovery/ docs/network-isolation.md (line 68) https://reviews.apache.org/r/36281/#comment143981 s/ephemerlal/ephemeral/ docs/network-isolation.md (lines 70 - 73) https://reviews.apache.org/r/36281/#comment143983 Is 1024 a reasonable example ephemeral_ports_per_container? Your current example only allows 24 containers on that slave, which is extremely low. Maybe 16 ports per container would make more sense, yielding 1536 possible containers. docs/network-isolation.md (line 72) https://reviews.apache.org/r/36281/#comment143987 `s/\/\/`? docs/network-isolation.md (line 79) https://reviews.apache.org/r/36281/#comment143986 Why is this a fixed constant limit? Seems like we might want to adjust this depending on how many containers are running, or give some containers more bandwidth than others. Why isn't this just another resource type (outbound network bandwidth) with a fixed total, where subsets can be reserved/claimed by different frameworks/tasks. docs/network-isolation.md (line 85) https://reviews.apache.org/r/36281/#comment143989 Is the `=true` necessary, or would `--egress_unique_flow_per_container` work on its own? docs/network-isolation.md (line 93) https://reviews.apache.org/r/36281/#comment143988 Maybe leave out cpu/mem/disk to let the slave auto-detect them (and shorten this line)? docs/network-isolation.md (line 178) https://reviews.apache.org/r/36281/#comment143991 lowercase? docs/network-isolation.md (line 198) https://reviews.apache.org/r/36281/#comment143992 lowercase? docs/network-isolation.md (line 204) https://reviews.apache.org/r/36281/#comment143990 This footnote `[1]` is never referenced above - Adam B On July 7, 2015, 2:54 p.m., Paul Brett wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36281/ --- (Updated July 7, 2015, 2:54 p.m.) Review request for mesos, Ian Downes, Jie Yu, and Cong Wang. Repository: mesos Description --- Document per-container unique egress flows and network queueing statistics. Diffs - docs/home.md bc277910907c381c08835b6e9d485b27d6da5002 docs/network-isolation.md PRE-CREATION docs/network-monitoring.md 8889fb165cc70bc382be0c99de8d7748328abf57 Diff: https://reviews.apache.org/r/36281/diff/ Testing --- Rendered at https://www.notehub.org/2015/7/7/network-isolation for review. Thanks, Paul Brett
Re: Review Request 36279: Doxygen-ification of comments in libprocess IO headers.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36279/ --- (Updated July 7, 2015, 3:14 p.m.) Review request for mesos, Benjamin Hindman, Artem Harutyunyan, and Joerg Schad. Repository: mesos Description --- Doxygen-ification of comments in libprocess IO headers. Diffs - 3rdparty/libprocess/include/process/io.hpp 63887704743f53c6e49ab504f549419e1d5910ce Diff: https://reviews.apache.org/r/36279/diff/ Testing --- `doxygen ../Doxyfile` and visually verified the HTML output. Thanks, Joseph Wu
Re: Review Request 36282: Remove os environment for docker executor enviornment setup.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36282/#review90811 --- src/slave/containerizer/docker.cpp (line 1552) https://reviews.apache.org/r/36282/#comment143951 We have to filter even if the flag is set, since we're actually including more than what the flags has specified. What's interesting though is that we might unintentionally filter env variables if it's set from the flags and it's also included already in the os environment with the same key and value. I can further check with the flags.executor_environment_variables so I don't filter a env variable that's also set in there, but I think it might be a lot easier to introduce a optional boolean flag (includeOsEnvironment) on executorEnvironment method to be able to not include os::environment(), and it's set to true by default? Then all the DockerContainerizer has to do is to set that extra flag to false. What you think Ben? - Timothy Chen On July 7, 2015, 10:10 p.m., Timothy Chen wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36282/ --- (Updated July 7, 2015, 10:10 p.m.) Review request for mesos, Adam B, Benjamin Hindman, and Joerg Schad. Bugs: MESOS-2996 https://issues.apache.org/jira/browse/MESOS-2996 Repository: mesos Description --- Remove os environment when calling executorEnvironment when running docker executors, since all the environment variables will be passed into the container and causes bad behavior such as overriding hostname. Diffs - src/slave/containerizer/docker.hpp 646a277be21deaded47324bea5474bd68699f25f src/slave/containerizer/docker.cpp 6eb1c84b1a6ef17c3e2ea2028e2f2d75110176ba Diff: https://reviews.apache.org/r/36282/diff/ Testing --- make check Thanks, Timothy Chen
Re: Review Request 36037: Adding /call endpoint to Master
On July 3, 2015, 12:29 a.m., Ben Mahler wrote: I chatted with Isabel on IRC and asked her to break apart this change into more bite-sized chunks, so that we can do smaller reviews and get things committed incrementally: (1) Dummy /call handler on the master. (2) Validation. (3) Partial implementation of Call (i.e. parsing logic). Each part can have its own tests. She will be discarding this review in favor of smaller chunks, which we can commit incrementally. :) I also asked her to: (a) Punt on the constants and remove master/http_constants.hpp, since these constants aren't adding value (CLOSE - close) for the added indirection, and our existing code doesn't follow this pattern. (b) Pull out the change to src/tests/mesos.hpp, since it is independent. Marco Massenzio wrote: All good. However, I beg to disagree on this point: (a) Punt on the constants and remove master/http_constants.hpp, since these constants aren't adding value (CLOSE - close) for the added indirection, and our existing code doesn't follow this pattern. We *do* have a `constants.hpp` (and relative .cpp) file and I do believe it does add value (I, for one, certainly appreciated having it when I did the JSON/ZK change ;) ): for example, I've already seen the string `application/x-protobuf` typed up 10 times in just two reviews: there is value in having an APPLICATION_PROTOBUF constant to: - avoid difficult-to-spot bugs to typos (`application/x-prolobuf`) that may only surface at runtime in production; - avoid typing the same stuff again and again (especially those of us using modern IDEs can take advantage of code-completion ;) ) - this is anyway common standard good practice and would allow us to not having to agonize too much in case we need to refactor something (say, at some point we want to use `application/x-protobuf-binary` for whatever reason - there's only one place to do so; sure, this is an unlikely example, but there may be cases where it may not be so far-fetched). Also, *not* doing it does not save (I think?) any effort in reviewing and/or committing, so seems very low cost for a potential sizeable payoff. Ah, yes, and there's also the fact that hard-coded strings sprinkled all over the code base are hard to maintain - I know, I've had to pick up the pieces at least twice in the last 4 years ;) PS - I do agree that defining `const string CLOSE = close` may be pushing this one step too far... but I'd like to retain it for those more commonly used strings. Ben Mahler wrote: I don't think we're in disagreement, I just want this to be punted so that we can think carefully about how to improve 'Request' and 'Response' usage, rather than bundling it in this code review. Doing more than one thing in a review tends to drag out the review, and I didn't want Isabel to get distracted with this. So let's follow up! In particular, having http constants in master/http_constants.hpp is strange because it isn't master specific (we have common/http.hpp for ones relevant to many components in mesos, http.hpp for libprocess). Also, where possible, I'm hoping to avoid the difficulty in header map manipulation entirely by providing typed members (there's a [TODO](https://github.com/apache/mesos/blob/0.23.0-rc1/3rdparty/libprocess/include/process/http.hpp#L107) which briefly alludes to this). For example, `request.connection` could be an enum to capture the possible connection types. I don't think we're in disagreement Awesome! :) Doing more than one thing in a review tends to drag out the review, Completely agree, I just thought that factoring out the constants at the outset was rather uncontroversial and best done now rather than have to refactor later. having http constants in master/http_constants.hpp yep - I had actually missed that: do you have a better suggestion? (maybe `common/api_constants.hpp` could be a better place/name? something else?) For example, request.connection could be an enum to capture the possible connection types. Oh, yes - totally: typed enum string constant hard-coded string :) - Marco --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36037/#review90302 --- On July 2, 2015, 8:16 a.m., Isabel Jimenez wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36037/ --- (Updated July 2, 2015, 8:16 a.m.) Review request for mesos, Anand Mazumdar, Benjamin Hindman, Ben Mahler, Marco Massenzio, and Vinod Kone. Bugs: MESOS-2860
Re: Review Request 34139: AppC image discovery.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/34139/#review90830 --- src/slave/containerizer/provisioners/appc/discovery.hpp (lines 43 - 46) https://reviews.apache.org/r/34139/#comment143985 Some comments explaining that the return value is a URI string would be nice. src/slave/containerizer/provisioners/appc/discovery.hpp (line 46) https://reviews.apache.org/r/34139/#comment143994 The `id` is not use in this review and not specified in https://github.com/appc/spec/blob/master/spec/discovery.md I assume its the sha512 but is it used during discovery? src/slave/containerizer/provisioners/appc/discovery.hpp (line 79) https://reviews.apache.org/r/34139/#comment144003 No need for the trailing `;` src/slave/containerizer/provisioners/appc/discovery.cpp (line 21) https://reviews.apache.org/r/34139/#comment143999 Kill empty line. src/slave/containerizer/provisioners/appc/discovery.cpp (line 60) https://reviews.apache.org/r/34139/#comment143977 canonicalize() not introduced until /r/34142/. Is there anything else outside discovery that uses the method? Can it be moved to class `Discovery` (so we don't have to deal with cyclic review dependency)? src/slave/containerizer/provisioners/appc/discovery.cpp (line 90) https://reviews.apache.org/r/34139/#comment143998 FWIW https://github.com/appc/spec/blob/master/spec/discovery.md uses https exclusively. src/slave/flags.cpp (line 63) https://reviews.apache.org/r/34139/#comment144000 List all supported values? src/slave/flags.cpp (line 68) https://reviews.apache.org/r/34139/#comment144001 State that this is only for local discovery? The sentences already mentions 'local images' but I think --appc_discovery=local is more explict in telling what the operator should do. - Jiang Yan Xu On July 7, 2015, 12:42 p.m., Ian Downes wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/34139/ --- (Updated July 7, 2015, 12:42 p.m.) Review request for mesos, Chi Zhang, Paul Brett, Timothy Chen, and Vinod Kone. Repository: mesos Description --- Local and simple discovery only. Diffs - src/Makefile.am e7de0f3d1a5efeaef47d5074defe3b40db94f573 src/slave/containerizer/provisioners/appc/discovery.hpp PRE-CREATION src/slave/containerizer/provisioners/appc/discovery.cpp PRE-CREATION src/slave/flags.hpp 7634e368c72e83932dcd992d78eaca146326606b src/slave/flags.cpp cbf431eb0627bdaf07241cc0fc4630df06fb20e2 Diff: https://reviews.apache.org/r/34139/diff/ Testing --- Thanks, Ian Downes
Re: Review Request 36281: Document per-container unique egress flows and network queueing statistics.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36281/#review90798 --- docs/network-isolation.md (line 7) https://reviews.apache.org/r/36281/#comment143942 which require to listen is incorrect a minimum choose a port in the Linux host ephemeral port range h, what? I presume you really mean they should bind(0) and use what the kernel tells them to... saying specific ports here will be interpreted as 'I want my container to listen to port 80', not that I want to bind some specific port that I can then discover. docs/network-isolation.md (line 25) https://reviews.apache.org/r/36281/#comment143925 This sentence is not clear, suggest rewording as 2.6.39 is advised for debugging purposes but is not required docs/network-isolation.md (line 27) https://reviews.apache.org/r/36281/#comment143926 s/development package for libnl3/libnl3 development package/ docs/network-isolation.md (line 48) https://reviews.apache.org/r/36281/#comment143927 what does but only assigned ports will be allocated by the kernel mean? this is not clear. Please also state here that the ephemeral range is split and assigned. docs/network-isolation.md (line 52) https://reviews.apache.org/r/36281/#comment143930 I think it's potentially confusing to call them short-lived (yes, I know that's historically how they've been used and how wikipedia categorizes them), since applications are free to bind to them as use them for the entirety of the job lifetime. docs/network-isolation.md (line 60) https://reviews.apache.org/r/36281/#comment143933 You can write directly to `/proc/sys/net/ipv4/ip_local_port_range`. Please state why the reboot is (strongly) advised. docs/network-isolation.md (line 70) https://reviews.apache.org/r/36281/#comment143935 Is it also recommended that ephemeral_ports per container be power-2 sized and aligned? Can you be precise in the limit on the number of containers? Can you document here the master flag to set a global limit to the number of containers to each slave used as a workaround because ephemeral ports are not exposed to the master. s/packets/packet/ docs/network-isolation.md (line 77) https://reviews.apache.org/r/36281/#comment143938 Can you state and explain why there's no shaping/limit on ingress? State explicitly that shaping delays traffic and will not drop packets. - Ian Downes On July 7, 2015, 2:54 p.m., Paul Brett wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36281/ --- (Updated July 7, 2015, 2:54 p.m.) Review request for mesos, Ian Downes, Jie Yu, and Cong Wang. Repository: mesos Description --- Document per-container unique egress flows and network queueing statistics. Diffs - docs/home.md bc277910907c381c08835b6e9d485b27d6da5002 docs/network-isolation.md PRE-CREATION docs/network-monitoring.md 8889fb165cc70bc382be0c99de8d7748328abf57 Diff: https://reviews.apache.org/r/36281/diff/ Testing --- Rendered at https://www.notehub.org/2015/7/7/network-isolation for review. Thanks, Paul Brett
Re: Review Request 36282: Remove os environment for docker executor enviornment setup.
On July 7, 2015, 10:58 p.m., Joerg Schad wrote: src/slave/containerizer/docker.cpp, line 1544 https://reviews.apache.org/r/36282/diff/1/?file=1001811#file1001811line1544 Wouldn't it be easier --especially considering Adam's comment about MESOS-2832 and the --executor_environment_variable --- to add an additional (optional) flag to executorEnvironment intialize=true. if explicitly set to false (such as in this case, we would not initialize environment with os::environment() Till Toenshoff wrote: I like this approach better as well - seems much cleaner to me. flag - parameter... - Joerg --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36282/#review90814 --- On July 7, 2015, 10:10 p.m., Timothy Chen wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36282/ --- (Updated July 7, 2015, 10:10 p.m.) Review request for mesos, Adam B, Benjamin Hindman, and Joerg Schad. Bugs: MESOS-2996 https://issues.apache.org/jira/browse/MESOS-2996 Repository: mesos Description --- Remove os environment when calling executorEnvironment when running docker executors, since all the environment variables will be passed into the container and causes bad behavior such as overriding hostname. Diffs - src/slave/containerizer/docker.hpp 646a277be21deaded47324bea5474bd68699f25f src/slave/containerizer/docker.cpp 6eb1c84b1a6ef17c3e2ea2028e2f2d75110176ba Diff: https://reviews.apache.org/r/36282/diff/ Testing --- make check Thanks, Timothy Chen
Re: Review Request 36282: Remove os environment for docker executor enviornment setup.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36282/#review90822 --- Ship it! Looks like a simpler, more generic fix. Maybe external_containerizer will want to use includeOsEnvironment flag too. src/slave/containerizer/containerizer.cpp (lines 248 - 253) https://reviews.apache.org/r/36282/#comment143962 Seems like one of these could be an else if, since they both wipe `environment` and start from scratch. I would probably start with `if(flags.executor_environment_variables)` then `else if(includeOsEnvironment)`. - Adam B On July 7, 2015, 4:05 p.m., Timothy Chen wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36282/ --- (Updated July 7, 2015, 4:05 p.m.) Review request for mesos, Adam B, Benjamin Hindman, and Joerg Schad. Bugs: MESOS-2996 https://issues.apache.org/jira/browse/MESOS-2996 Repository: mesos Description --- Remove os environment when calling executorEnvironment when running docker executors, since all the environment variables will be passed into the container and causes bad behavior such as overriding hostname. Diffs - src/slave/containerizer/containerizer.hpp 0ee17e6bc52d1e3acefad6bda3a1b7ba64a8a54b src/slave/containerizer/containerizer.cpp 69dfac04cfd9c388fc908b68ac7abbc14304e621 src/slave/containerizer/docker.hpp 646a277be21deaded47324bea5474bd68699f25f src/slave/containerizer/docker.cpp 6eb1c84b1a6ef17c3e2ea2028e2f2d75110176ba Diff: https://reviews.apache.org/r/36282/diff/ Testing --- make check Thanks, Timothy Chen
Re: Review Request 36279: Doxygen-ification of comments in libprocess IO headers.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36279/#review90823 --- Patch looks great! Reviews applied: [36279] All tests passed. - Mesos ReviewBot On July 7, 2015, 10:14 p.m., Joseph Wu wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36279/ --- (Updated July 7, 2015, 10:14 p.m.) Review request for mesos, Benjamin Hindman, Artem Harutyunyan, and Joerg Schad. Repository: mesos Description --- Doxygen-ification of comments in libprocess IO headers. Diffs - 3rdparty/libprocess/include/process/io.hpp 63887704743f53c6e49ab504f549419e1d5910ce Diff: https://reviews.apache.org/r/36279/diff/ Testing --- `doxygen ../Doxyfile` and visually verified the HTML output. Thanks, Joseph Wu
Re: Review Request 36281: Document per-container unique egress flows and network queueing statistics.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36281/#review90813 --- Ship it! Thanks Paul! This is great! It's a little hard to review this change because you change the file name (I won't be able to see the diff). It would be better if you change the file name in a separate patch. Next time! docs/home.md (line 24) https://reviews.apache.org/r/36281/#comment143953 I would suggest calling it 'Network Monitoring and Isolation'. Do not forget to change the file name as well. docs/network-isolation.md (line 5) https://reviews.apache.org/r/36281/#comment143954 Ditto. Calling Network Monitoring and Isolation is more appropriate. Please to a sweep to fix all occurances in this doc. docs/network-isolation.md (line 7) https://reviews.apache.org/r/36281/#comment143956 Please do not delete the version information. Network monitoring is added in mesos 0.20 and network isolation is added in mesos 0.23. I suggest the following layout for the summary paragraph: ``` # Network Monitoring and Isolation. Mesos 0.20.0 adds the support for per container network monitoring. ... Mesos 0.23.0 adds the support for per container network isolation. ... Our solution is transparent to the tasks running on the slave ... ``` docs/network-isolation.md (line 98) https://reviews.apache.org/r/36281/#comment143973 Container Network Statistics? - Jie Yu On July 7, 2015, 9:54 p.m., Paul Brett wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36281/ --- (Updated July 7, 2015, 9:54 p.m.) Review request for mesos, Ian Downes, Jie Yu, and Cong Wang. Repository: mesos Description --- Document per-container unique egress flows and network queueing statistics. Diffs - docs/home.md bc277910907c381c08835b6e9d485b27d6da5002 docs/network-isolation.md PRE-CREATION docs/network-monitoring.md 8889fb165cc70bc382be0c99de8d7748328abf57 Diff: https://reviews.apache.org/r/36281/diff/ Testing --- Rendered at https://www.notehub.org/2015/7/7/network-isolation for review. Thanks, Paul Brett
Re: Review Request 36071: Add flow diagram for docker containerizer.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36071/#review90685 --- 1. Inconsistent capitalization in box labels. 2. You explain different paths to obtain an executor pid and then you checkpoint a container pid. You lost me there. 3. What happens to the tasks? Is this diagram for the executor only? If so, it is incomplete. - Bernd Mathiske On July 6, 2015, 2:37 p.m., Timothy Chen wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36071/ --- (Updated July 6, 2015, 2:37 p.m.) Review request for mesos, Benjamin Hindman, Bernd Mathiske, and Till Toenshoff. Repository: mesos Description --- Add flow diagram for docker containerizer. Diffs - docs/docker-containerizer.md 73f897780a0bb72ab092cb08186a228e3084e798 docs/images/docker_containerizer_flow.jpg PRE-CREATION Diff: https://reviews.apache.org/r/36071/diff/ Testing --- make File Attachments docker_containerizer_flow.png https://reviews.apache.org/media/uploaded/files/2015/07/06/d888a674-17d8-4faf-ab03-f1892537a6e5__docker_containerizer_flow.png Thanks, Timothy Chen
Review Request 36255: Rearranged Option constructors to suppress compiler bug.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36255/ --- Review request for mesos, Adam B and Michael Park. Bugs: MESOS-2991 https://issues.apache.org/jira/browse/MESOS-2991 Repository: mesos Description --- We believe there is a compiler bug in the `g++` compiler based on LLVM 3.5 on Mac OS 10.10.4. The bug seems to be related to the template instantiation mechanism. Rearranging c-tors of the affected class suppresses the bug for the given compiler version. Diffs - 3rdparty/libprocess/3rdparty/stout/include/stout/option.hpp 3d9b7a7568de6734097733f4e6d59acba629b849 Diff: https://reviews.apache.org/r/36255/diff/ Testing --- Make check on Mac OS 10.10.4 with the following compiler: ``` g++ --version Configured with: --prefix=/Applications/Xcode.app/Contents/Developer/usr --with-gxx-include-dir=/usr/include/c++/4.2.1 Apple LLVM version 6.0 (clang-600.0.54) (based on LLVM 3.5svn) Target: x86_64-apple-darwin14.4.0 Thread model: posix ``` Thanks, Alexander Rukletsov
Re: Review Request 35998: Added doxygen styled comments to Path::basename and Path::dirname.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/35998/#review90677 --- 3rdparty/libprocess/3rdparty/stout/include/stout/path.hpp (line 24) https://reviews.apache.org/r/35998/#comment143810 I know that in many other places we have written Basic abstraction, but this really adds no meaning. So let's stop that! And we don't deal with just about any file system here (e.g. FAT32), do we? Suggestion: Represents UNIX file systems paths and offers common path manipulations. Or something like that :-) 3rdparty/libprocess/3rdparty/stout/include/stout/path.hpp (line 39) https://reviews.apache.org/r/35998/#comment143811 Do you mean std::basename()? 3rdparty/libprocess/3rdparty/stout/include/stout/path.hpp (line 48) https://reviews.apache.org/r/35998/#comment143816 It would be good to explain why we are opting this way. How is this useful? OK, maybe nobody knows and we don't want to touch the code? There are various other inexplicable outcomes here. 3rdparty/libprocess/3rdparty/stout/include/stout/path.hpp (line 52) https://reviews.apache.org/r/35998/#comment143812 Component - The component 3rdparty/libprocess/3rdparty/stout/include/stout/path.hpp (line 53) https://reviews.apache.org/r/35998/#comment143815 Why suddenly `slash` and not '/' as before? - Bernd Mathiske On June 29, 2015, 4:43 a.m., Till Toenshoff wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/35998/ --- (Updated June 29, 2015, 4:43 a.m.) Review request for mesos, Bernd Mathiske and Joerg Schad. Repository: mesos Description --- see summary. Diffs - 3rdparty/libprocess/3rdparty/stout/include/stout/path.hpp a4afdad0b5f053186ace4d6a37b41cd02e7d415b Diff: https://reviews.apache.org/r/35998/diff/ Testing --- Thanks, Till Toenshoff
Re: Review Request 35981: Added persistent volume user guide.
On June 29, 2015, 5:14 p.m., Adam B wrote:
docs/persistent-volume.md, line 169
https://reviews.apache.org/r/35981/diff/1/?file=994064#file994064line169
There is no `volumes` field. Just a `resources` field, where each
resource in the list must contain a `disk.volume` to be destroyed.
Michael Park wrote:
I was actually referring to:
```
message Create {
repeated Resource volumes = 1;
}
```
I see, so the inconsistency was in the example json, not in the comment.
Dropping this issue in favor of new ones.
- Adam
---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/35981/#review89827
---
On June 29, 2015, 10:59 p.m., Michael Park wrote:
---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/35981/
---
(Updated June 29, 2015, 10:59 p.m.)
Review request for mesos, Adam B, Jie Yu, and Timothy Chen.
Bugs: MESOS-2405
https://issues.apache.org/jira/browse/MESOS-2405
Repository: mesos
Description
---
The Github rendered version is available [here](
https://github.com/mesosphere/mesos/blob/user-docs/docs/persistent-volume.md)
Diffs
-
docs/persistent-volume.md PRE-CREATION
Diff: https://reviews.apache.org/r/35981/diff/
Testing
---
Documentation.
Thanks,
Michael Park
Re: Review Request 36281: Document per-container unique egress flows and network queueing statistics.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36281/#review90858 --- Patch looks great! Reviews applied: [36281] All tests passed. - Mesos ReviewBot On July 8, 2015, 12:59 a.m., Paul Brett wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36281/ --- (Updated July 8, 2015, 12:59 a.m.) Review request for mesos, Ian Downes, Jie Yu, and Cong Wang. Repository: mesos Description --- Document per-container unique egress flows and network queueing statistics. Diffs - docs/home.md bc277910907c381c08835b6e9d485b27d6da5002 docs/network-monitoring.md 8889fb165cc70bc382be0c99de8d7748328abf57 Diff: https://reviews.apache.org/r/36281/diff/ Testing --- Rendered at https://www.notehub.org/2015/7/7/network-isolation for review. Thanks, Paul Brett
Re: Review Request 36281: Document per-container unique egress flows and network queueing statistics.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36281/#review90857 --- Ship it! A few other minor nits, but I think we can commit this for 0.23.0-rc2. I'd even be willing to make some of the changes myself before committing. docs/network-monitoring.md (line 7) https://reviews.apache.org/r/36281/#comment144013 s/running under/on/? s/which bind/those that bind/ docs/network-monitoring.md (line 15) https://reviews.apache.org/r/36281/#comment144014 Mesos will automatically check for those kernel functionalities and will abort if they are not supported Is this still true? docs/network-monitoring.md (line 67) https://reviews.apache.org/r/36281/#comment144016 If it is necessary to reduce this range to free ports to be allocated by the slave, it can be configured by defining the new range in `etc/sysctl.conf` and rebooting to eliminate In free ports, I couldn't tell if free was supposed to be a verb or adjective. How about If ports need to be set aside for slave containers, the host ephemeral port range can be updated in `etc/sysctl.conf`. Rebooting after the update will eliminate docs/network-monitoring.md (lines 69 - 71) https://reviews.apache.org/r/36281/#comment144015 This comment confused me. What you really mean is that the previous range was 32768-61000, but then you took away the first chunk for Mesos to use for its containers. ``` # Set aside 32768-57344 for Mesos containers. # net.ipv4.ip_local_port_range = 32768 61000 net.ipv4.ip_local_port_range = 57345 61000 ``` docs/network-monitoring.md (line 77) https://reviews.apache.org/r/36281/#comment144017 Maybe insert a br before `number of ephemeral_ports / ephemeral_ports_per_container` so that it goes on a single line. docs/network-monitoring.md (line 79) https://reviews.apache.org/r/36281/#comment144020 What is the default value of ephemeral_ports_per_container? (1024) docs/network-monitoring.md (line 92) https://reviews.apache.org/r/36281/#comment144018 Remove indentation if you want this to render as a header. docs/network-monitoring.md (line 106) https://reviews.apache.org/r/36281/#comment144021 We should only bother to include this flag if we set a non-default value. docs/network-monitoring.md (line 107) https://reviews.apache.org/r/36281/#comment144019 Can't you just say `300MB` like in your description? Or does this flag only handle KB? - Adam B On July 7, 2015, 5:59 p.m., Paul Brett wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36281/ --- (Updated July 7, 2015, 5:59 p.m.) Review request for mesos, Ian Downes, Jie Yu, and Cong Wang. Repository: mesos Description --- Document per-container unique egress flows and network queueing statistics. Diffs - docs/home.md bc277910907c381c08835b6e9d485b27d6da5002 docs/network-monitoring.md 8889fb165cc70bc382be0c99de8d7748328abf57 Diff: https://reviews.apache.org/r/36281/diff/ Testing --- Rendered at https://www.notehub.org/2015/7/7/network-isolation for review. Thanks, Paul Brett
Re: Review Request 35947: Added a new API call 'updateAvailable' to the allocator.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/35947/#review90839 --- Ship it! src/master/allocator/mesos/hierarchical.hpp (lines 734 - 737) https://reviews.apache.org/r/35947/#comment144004 Could you please add a NOTE here explaining why this is possible? (e.g., it's possible because an allocation can happen after updateAvailable is enqueued but before it's actually being processed). - Jie Yu On June 26, 2015, 10:55 p.m., Michael Park wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/35947/ --- (Updated June 26, 2015, 10:55 p.m.) Review request for mesos, Alexander Rukletsov, Benjamin Hindman, Ben Mahler, and Jie Yu. Repository: mesos Description --- Needed to implement the master HTTP endpoints: `/reserve`, `/unreserve`, `/create` and `/destroy`. This API is similar to `updateSlave` which is currently very specific to oversubscription. I considered consolidating `updateAvailable` and `updateSlave` but it will require making offers be generated within the allocator to enable this. In specific, `updateAvailable` could fail if there aren't sufficient available resources to update, whereas `updateSlave` avoids failing by keeping the allocator in an over-allocated state. For `updateSlave`, leaving the allocator in an over-allocated state is ok. This is because it does not modify resources therefore `total - allocated` will work out to __empty__. `updateAvailable` cannot leave the allocator in an over-allocated state, because it modifies resources, and therefore `total - allocated` is not guaranteed to yield __empty__. Diffs - include/mesos/master/allocator.hpp 22992c0c77058af4fcd28aa8e4a1191693a16f44 src/master/allocator/mesos/allocator.hpp 72470ec7f56f84a9a9815c09adb88def90ef672f src/master/allocator/mesos/hierarchical.hpp 3264d145d52b48852878abf7ab9be29ab98208cc src/tests/hierarchical_allocator_tests.cpp 3258840135290cd008ca09235d18b7f093dafd2e src/tests/mesos.hpp 9157ac079808d2686592e54ea26a26e6a0825ed3 Diff: https://reviews.apache.org/r/35947/diff/ Testing --- (1) Added `HierarchicalAllocatorTest.updateAvailableSuccess` and `HierarchicalAllocatorTest.updateAvailableFail` (2) `make check` Thanks, Michael Park
Re: Review Request 34138: AppC hash computation.
On July 7, 2015, 3:56 p.m., Jiang Yan Xu wrote: 1. Agree that this is useful as a utility in libprocess. Not much overhead to move it over right? 2. It feels like something that could be exposed as a function rather than class, maybe a TODO. OK I realized that doing the aforementioned refactorings is not as simple as moving the file so probably punting it is the right thing to do for now. 1. As a generic utility it's probably giong to be SHA instead SHA512. 2. OK SHA512::hash() is already static but I meant if made more generic like SHA(alorightm).hash() then shorthand functions like `sha1()`, `sha512()` is easier to use. - Jiang Yan --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/34138/#review90791 --- On July 7, 2015, 12:42 p.m., Ian Downes wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/34138/ --- (Updated July 7, 2015, 12:42 p.m.) Review request for mesos, Chi Zhang, Paul Brett, Timothy Chen, and Vinod Kone. Repository: mesos Description --- AppC hash computation. Diffs - src/Makefile.am e7de0f3d1a5efeaef47d5074defe3b40db94f573 src/slave/containerizer/provisioners/appc/hash.hpp PRE-CREATION Diff: https://reviews.apache.org/r/34138/diff/ Testing --- Thanks, Ian Downes
Re: Review Request 36281: Document per-container unique egress flows and network queueing statistics.
On July 7, 2015, 11:31 p.m., Jie Yu wrote: docs/network-isolation.md, line 7 https://reviews.apache.org/r/36281/diff/1/?file=1001779#file1001779line7 Please do not delete the version information. Network monitoring is added in mesos 0.20 and network isolation is added in mesos 0.23. I suggest the following layout for the summary paragraph: ``` # Network Monitoring and Isolation. Mesos 0.20.0 adds the support for per container network monitoring. ... Mesos 0.23.0 adds the support for per container network isolation. ... Our solution is transparent to the tasks running on the slave ... ``` We should address the changes in the changelog and publish historical documentation on the website for those running older releaes (raised MESOS-3011 to addresss). - Paul --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36281/#review90813 --- On July 8, 2015, 12:03 a.m., Paul Brett wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36281/ --- (Updated July 8, 2015, 12:03 a.m.) Review request for mesos, Ian Downes, Jie Yu, and Cong Wang. Repository: mesos Description --- Document per-container unique egress flows and network queueing statistics. Diffs - docs/home.md bc277910907c381c08835b6e9d485b27d6da5002 docs/network-isolation.md PRE-CREATION docs/network-monitoring.md 8889fb165cc70bc382be0c99de8d7748328abf57 Diff: https://reviews.apache.org/r/36281/diff/ Testing --- Rendered at https://www.notehub.org/2015/7/7/network-isolation for review. Thanks, Paul Brett
Re: Review Request 36281: Document per-container unique egress flows and network queueing statistics.
On July 8, 2015, 12:01 a.m., Adam B wrote: docs/network-isolation.md, line 79 https://reviews.apache.org/r/36281/diff/1/?file=1001779#file1001779line79 Why is this a fixed constant limit? Seems like we might want to adjust this depending on how many containers are running, or give some containers more bandwidth than others. Why isn't this just another resource type (outbound network bandwidth) with a fixed total, where subsets can be reserved/claimed by different frameworks/tasks. Outbound network bandwidth sounds like a reasonable future enhancement - do you want to raise a ticket? On July 8, 2015, 12:01 a.m., Adam B wrote: docs/network-isolation.md, lines 70-73 https://reviews.apache.org/r/36281/diff/1/?file=1001779#file1001779line70 Is 1024 a reasonable example ephemeral_ports_per_container? Your current example only allows 24 containers on that slave, which is extremely low. Maybe 16 ports per container would make more sense, yielding 1536 possible containers. Its a reasonable number for the kinds of services I see running on mesos, after all we don't churn through containers very quickly. On July 8, 2015, 12:01 a.m., Adam B wrote: docs/network-isolation.md, lines 24-29 https://reviews.apache.org/r/36281/diff/1/?file=1001779#file1001779line24 Do these dependencies need to be added to getting-started.md? I don't believe so since this is not a default configuration. - Paul --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36281/#review90827 --- On July 8, 2015, 12:03 a.m., Paul Brett wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36281/ --- (Updated July 8, 2015, 12:03 a.m.) Review request for mesos, Ian Downes, Jie Yu, and Cong Wang. Repository: mesos Description --- Document per-container unique egress flows and network queueing statistics. Diffs - docs/home.md bc277910907c381c08835b6e9d485b27d6da5002 docs/network-isolation.md PRE-CREATION docs/network-monitoring.md 8889fb165cc70bc382be0c99de8d7748328abf57 Diff: https://reviews.apache.org/r/36281/diff/ Testing --- Rendered at https://www.notehub.org/2015/7/7/network-isolation for review. Thanks, Paul Brett
Re: Review Request 36273: Doxygen-ification of comments in libprocess process headers.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36273/#review90852 --- Patch looks great! Reviews applied: [36273] All tests passed. - Mesos ReviewBot On July 8, 2015, 12:24 a.m., Joseph Wu wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36273/ --- (Updated July 8, 2015, 12:24 a.m.) Review request for mesos, Benjamin Hindman, Artem Harutyunyan, and Joerg Schad. Repository: mesos Description --- Doxygen-ification of comments in libprocess process headers. Diffs - 3rdparty/libprocess/include/process/process.hpp 59b50af86e059463a01f3c83701bc5fd143d51a4 Diff: https://reviews.apache.org/r/36273/diff/ Testing --- `doxygen ../Doxyfile` and visually verified the HTML output. Thanks, Joseph Wu
Re: Review Request 36281: Document per-container unique egress flows and network queueing statistics.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36281/ --- (Updated July 8, 2015, 12:59 a.m.) Review request for mesos, Ian Downes, Jie Yu, and Cong Wang. Changes --- Incorporate remaining reviewer comments. Repository: mesos Description --- Document per-container unique egress flows and network queueing statistics. Diffs (updated) - docs/home.md bc277910907c381c08835b6e9d485b27d6da5002 docs/network-monitoring.md 8889fb165cc70bc382be0c99de8d7748328abf57 Diff: https://reviews.apache.org/r/36281/diff/ Testing --- Rendered at https://www.notehub.org/2015/7/7/network-isolation for review. Thanks, Paul Brett
Re: Review Request 36281: Document per-container unique egress flows and network queueing statistics.
On July 7, 2015, 5:01 p.m., Adam B wrote: docs/network-isolation.md, line 79 https://reviews.apache.org/r/36281/diff/1/?file=1001779#file1001779line79 Why is this a fixed constant limit? Seems like we might want to adjust this depending on how many containers are running, or give some containers more bandwidth than others. Why isn't this just another resource type (outbound network bandwidth) with a fixed total, where subsets can be reserved/claimed by different frameworks/tasks. Paul Brett wrote: Outbound network bandwidth sounds like a reasonable future enhancement - do you want to raise a ticket? Sure. Created https://issues.apache.org/jira/browse/MESOS-3014 - Adam --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36281/#review90827 --- On July 7, 2015, 5:59 p.m., Paul Brett wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36281/ --- (Updated July 7, 2015, 5:59 p.m.) Review request for mesos, Ian Downes, Jie Yu, and Cong Wang. Repository: mesos Description --- Document per-container unique egress flows and network queueing statistics. Diffs - docs/home.md bc277910907c381c08835b6e9d485b27d6da5002 docs/network-monitoring.md 8889fb165cc70bc382be0c99de8d7748328abf57 Diff: https://reviews.apache.org/r/36281/diff/ Testing --- Rendered at https://www.notehub.org/2015/7/7/network-isolation for review. Thanks, Paul Brett
Review Request 36277: MESOS-3002: Fix getOrElse compilation error for network isolator.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36277/ --- Review request for mesos and Benjamin Hindman. Bugs: MESOS-3002 https://issues.apache.org/jira/browse/MESOS-3002 Repository: mesos Description --- See summary. Diffs - src/slave/containerizer/isolators/network/port_mapping.cpp 2ef79eb9be76803d9a26223ee5763a582ca89736 Diff: https://reviews.apache.org/r/36277/diff/ Testing --- build with network isolator configured. Thanks, Joris Van Remoortere
Re: Review Request 35777: Made post-reviews.py handle bad (or not) ReviewBoard URLs.
---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/35777/#review90783
---
Ship it!
Minor nit on error message - fix it and commit.
Thanks for fixing this!
support/post-reviews.py (line 173)
https://reviews.apache.org/r/35777/#comment143906
note that here you will be printing the whole commit message, which in this
case is not appropriate.
I would do instead:
```
pos = message.find('Review:')
if pos != -1:
...
print ...format(message[pos:])
```
should make the error message clearer.
- Marco Massenzio
On July 7, 2015, 3:19 p.m., Michael Park wrote:
---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/35777/
---
(Updated July 7, 2015, 3:19 p.m.)
Review request for mesos, Benjamin Hindman, Marco Massenzio, and Till
Toenshoff.
Repository: mesos
Description
---
(1) Handles the case where the `Review: ...` line isn't the last of the
commit message.
```
01596ce802669e9dac7dd24193f041cef3354830 - Fixed post-reviews.py hanging bug.
(6 seconds ago)
ReviewBoard URL must be the last line of the commit message!
Fixed post-reviews.py hanging bug.
Review: https://reviews.apache.org/r/35771
abcd
```
(2) Handles the case where the ReviewBoard URL is invalid.
```
af081a07234794f60a2575e0383ce537d7111c18 - Fixed post-reviews.py hanging bug.
(29 seconds ago)
Invalid ReviewBoard URL : 'https://reviews.apache.org/r/35771 abcd'
```
Diffs
-
support/post-reviews.py b04e26b85e6b056098c15078f9b31dc352682351
Diff: https://reviews.apache.org/r/35777/diff/
Testing
---
Modified the commit message with `git rebase` and ran
`./support/post-reviews.py` to observe the above error messages.
Used the valid commit message for this patch and created it by running
`./support/post-reviews.py`.
Thanks,
Michael Park
