Re: Review Request 37714: Updated Multimap and multihashmap so their signatures resemble that of hashmap and hashset.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/37714/#review96849 --- Patch looks great! Reviews applied: [37714] All tests passed. - Mesos ReviewBot On Aug. 28, 2015, 8:11 a.m., Alexander Rojas wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/37714/ --- (Updated Aug. 28, 2015, 8:11 a.m.) Review request for mesos, Joerg Schad, Michael Park, and Jan Schlicht. Bugs: MESOS-2924 https://issues.apache.org/jira/browse/MESOS-2924 Repository: mesos Description --- Adds extra template parameters to `multihashmap` which offer control over the hash function to use as well as the equality operator. Implements initializer_list, copy and move constructors for both, `multihashmap` and `Multimap` in a similar way as it was done for `hashmap` and `hashset`. Diffs - 3rdparty/libprocess/3rdparty/stout/include/stout/multihashmap.hpp d9e4031cee64e48ad50541c04ca11e7861d0a17c 3rdparty/libprocess/3rdparty/stout/include/stout/multimap.hpp fb3e7a1d0377001389980302342813217f49cf5f 3rdparty/libprocess/3rdparty/stout/tests/multimap_tests.cpp 535cd2d10e3074c86c149ce85b205e73ca42ddd3 Diff: https://reviews.apache.org/r/37714/diff/ Testing --- make check Thanks, Alexander Rojas
Re: Review Request 37787: Reworked Jenkins build script. Added test images for ubuntu-14.04-clang-3.6, ubuntu-12.04-gcc-4.8, centos-6.6-gcc-4.8.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/37787/#review96848 --- Patch looks great! Reviews applied: [37787] All tests passed. - Mesos ReviewBot On Aug. 28, 2015, 7:51 a.m., Artem Harutyunyan wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/37787/ --- (Updated Aug. 28, 2015, 7:51 a.m.) Review request for mesos, Benjamin Hindman, Timothy Chen, and Vinod Kone. Bugs: MESOS-3313 https://issues.apache.org/jira/browse/MESOS-3313 Repository: mesos Description --- Reworked Jenkins build script. Added test images for ubuntu-14.04-clang-3.6, ubuntu-12.04-gcc-4.8, centos-6.6-gcc-4.8. Diffs - support/docker/centos-7.1-gcc-4.8/Dockerfile PRE-CREATION support/docker/ubuntu-12.04-gcc-4.8/Dockerfile PRE-CREATION support/docker/ubuntu-14.04-clang-3.6/Dockerfile PRE-CREATION support/docker/ubuntu-14.04-gcc-4.8/Dockerfile PRE-CREATION support/jenkins_build_docker.sh PRE-CREATION Diff: https://reviews.apache.org/r/37787/diff/ Testing --- `jenkins-build-docker.sh` is a reworked version of the original `jenkins-build.sh` that is ran by Jenkins buildbot for building and testing Mesos distributions. Features: * Runs libevent, SSL and ROOT tests. * Easily add OS/compiler Docker images for testing Mesos. * Exclude tests on per-image basis. * Easily reproduce the test image locally. * Three new test images (ubuntu-14.04-clang-3.6, ubuntu-12.04-gcc-4.8, centos-6.6-gcc-4.8). How to run The following environment variables have to be set for the script to run: * OS - OS name/version. Currently images are available for ubuntu-14.04, ubuntu-12.04, centos-7.1. * CONFIGURATION - ./configure flags (e.g. '--enable-libevent'). * COMPILER - Compiler name/version. Currently available images include gcc-4.8 (default value) on all platforms, clang-3.6 on ubuntu-14.04. Examples: `OS=ubuntu-14.04 CONFIGURATION='--enable-ssl --enable-libevent' COMPILER=clang-3.6 ./jenkins_build_docker.sh` `OS=centos-7.1 CONFIGURATION='--enable-ssl --enable-libevent' ./jenkins_build_docker.sh` NOTE: Mesos Python module has a known issue on centos-6.6 ( https://issues.apache.org/jira/browse/MESOS-3314 ), so **for now centos-6.6 should not be enabled in Jenkins**. Docker image is removed from the final review (it will be added later once the issue is resolved). Thanks, Artem Harutyunyan
Re: Review Request 37531: MESOS-3070
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/37531/ --- (Updated Aug. 28, 2015, 9:48 a.m.) Review request for mesos. Changes --- Add UT case Bugs: MESOS-3070 https://issues.apache.org/jira/browse/MESOS-3070 Repository: mesos Description --- MESOS-3070 (Master CHECK failure if a framework uses duplicated task id.) Diffs (updated) - src/master/http.cpp 37d76ee src/master/master.hpp 36c6759 src/master/master.cpp 95207d2 src/tests/master_tests.cpp 8a6b98b Diff: https://reviews.apache.org/r/37531/diff/ Testing (updated) --- make make check Thanks, Klaus Ma
Review Request 37892: Renamed cgroups::kill to cgroups::signal
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/37892/ --- Review request for mesos. Bugs: MESOS-3086 https://issues.apache.org/jira/browse/MESOS-3086 Repository: mesos Description --- As this function is actually just sending the signal we renamed it. Additionally we want to introduce an actual cgroups::kill() with MESOS-3086. Diffs - src/linux/cgroups.cpp 6ef42ed1bc719f334d1ac6e90919a1bc1840d31f src/tests/containerizer/cgroups_tests.cpp 0b171eeb53037f26b7e952830e88e59f1278e7c6 Diff: https://reviews.apache.org/r/37892/diff/ Testing --- Thanks, Joerg Schad
Re: Review Request 37891: Fetcher now does not erroneously report a non-archive when extracting from one.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/37891/ --- (Updated Aug. 28, 2015, 8:51 a.m.) Review request for mesos, Adam B, Benjamin Hindman, Till Toenshoff, and Timothy Chen. Changes --- Added bug number. Bugs: MESOS-3321 https://issues.apache.org/jira/browse/MESOS-3321 Repository: mesos Description --- Prevented fetcher from erroneously reporting a non-archive when successfully extracting from one. Diffs - src/launcher/fetcher.cpp 79b1d98c6dbd802bf3b4d6e60019ee9155f5c7b9 Diff: https://reviews.apache.org/r/37891/diff/ Testing --- make check Ran one affected test manually and inspected that the erroneous output is now missing from the sandbox. Thanks, Bernd Mathiske
Re: Review Request 36321: Maintenance primitives: Add Unavailability and InverseOffer protobufs.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36321/#review96866 --- include/mesos/mesos.proto (line 876) https://reviews.apache.org/r/36321/#comment152548 In your last diff, you changed a bunch of `window` to `interval`. Do you want to change this one as well? If not, are you sure you wanted all the `window` to `interval` changes in diff 13-14? - Joris Van Remoortere On Aug. 27, 2015, 10:46 p.m., Joseph Wu wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36321/ --- (Updated Aug. 27, 2015, 10:46 p.m.) Review request for mesos, Benjamin Hindman, Ben Mahler, Artem Harutyunyan, Joris Van Remoortere, and Vinod Kone. Bugs: MESOS-2061 and MESOS-2066 https://issues.apache.org/jira/browse/MESOS-2061 https://issues.apache.org/jira/browse/MESOS-2066 Repository: mesos Description --- MESOS-2061: Add Unavailability and InverseOffer protobufs declarations. MESOS-2066: Add the Unavailability field to Offers. Also copied to v1 API. No integration with other components (that part is tracked in separate JIRAs, see MESOS-1474). Diffs - include/mesos/mesos.proto 715b8cf38e1e56c18a3f2ddbb82c920bd9414f05 include/mesos/v1/mesos.proto 382b978dca769757171c5558b7f259870592c321 Diff: https://reviews.apache.org/r/36321/diff/ Testing --- `make check` Thanks, Joseph Wu
Re: Review Request 37914: Updated the ReviewBot to flag reviews that do not contain reviewers.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/37914/#review96977 --- Patch looks great! Reviews applied: [37914] All tests passed. - Mesos ReviewBot On Aug. 29, 2015, 12:08 a.m., Vinod Kone wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/37914/ --- (Updated Aug. 29, 2015, 12:08 a.m.) Review request for mesos, Benjamin Hindman and Michael Park. Repository: mesos Description --- I'm seeing more and more reviews being sent without tagging reviewers. This should flag such reviews to guide new contributors. Diffs - support/verify_reviews.py b85a3245b2f248dbdb8e2783f2384092e3d53031 Diff: https://reviews.apache.org/r/37914/diff/ Testing --- Tested locally ./support/verify_reviews.py vinod kone 10 ?from-user=neilc git rev-parse HEAD Checking if review: 37445 needs verification Latest diff timestamp: 2015-08-13 21:01:08 Verifying review 37445 Posting review: Bad patch! Reviews applied: [] Error: No reviewers specified. Please find a reviewer by asking on JIRA or the mailing list. Thanks, Vinod Kone
Re: Review Request 37908: Silence oversubscription logging
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/37908/#review96974 --- Patch looks great! Reviews applied: [37908] All tests passed. - Mesos ReviewBot On Aug. 28, 2015, 10:19 p.m., Niklas Nielsen wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/37908/ --- (Updated Aug. 28, 2015, 10:19 p.m.) Review request for mesos and Jie Yu. Repository: mesos Description --- Resource estimates are continuously written to the logs even though they are empty (as with the majority of Mesos clusters at the moment). To reduce the noise, this patch moves log lines for empty resources to VLOG(2). Diffs - src/slave/slave.cpp 8cfad7b86675b10c98524174975828bbcf47aed2 Diff: https://reviews.apache.org/r/37908/diff/ Testing --- make check - with and without the patch, verifying the log statements. Thanks, Niklas Nielsen
Re: Review Request 37894: Renamed cgroups::kill to cgroups::signal.
On Aug. 28, 2015, 4:45 p.m., Cong Wang wrote: Why? Everyone knows kill(2) sends a signal while signal(2) installs a signal handler... IMO, that naming is confusing, and should be 'signal' and 'install'. - Jie --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/37894/#review96883 --- On Aug. 28, 2015, 4:39 p.m., Joerg Schad wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/37894/ --- (Updated Aug. 28, 2015, 4:39 p.m.) Review request for mesos, Jie Yu, Till Toenshoff, and Timothy Chen. Bugs: MESOS-3086 https://issues.apache.org/jira/browse/MESOS-3086 Repository: mesos Description --- As this function is actually just sending the signal we renamed it. Additionally we want to introduce an actual cgroups::kill() with MESOS-3086. Diffs - src/linux/cgroups.hpp 204c53038a1ccfa693f4f2293488cff8cdd60835 src/linux/cgroups.cpp 6ef42ed1bc719f334d1ac6e90919a1bc1840d31f src/tests/containerizer/cgroups_tests.cpp 0b171eeb53037f26b7e952830e88e59f1278e7c6 Diff: https://reviews.apache.org/r/37894/diff/ Testing --- Thanks, Joerg Schad
Re: Review Request 36321: Maintenance primitives: Add Unavailability and InverseOffer protobufs.
On Aug. 27, 2015, 1:06 p.m., Benjamin Hindman wrote: include/mesos/mesos.proto, lines 926-927 https://reviews.apache.org/r/36321/diff/13/?file=1053309#file1053309line926 Why do we need the URL? Can we comment for folks so they know what they might need/use this for? Added some explanation. We can probably update the URL field in `Offer` with the same comment, separately. - Joseph --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36321/#review96743 --- On Aug. 28, 2015, 10:11 a.m., Joseph Wu wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36321/ --- (Updated Aug. 28, 2015, 10:11 a.m.) Review request for mesos, Benjamin Hindman, Ben Mahler, Artem Harutyunyan, Joris Van Remoortere, and Vinod Kone. Bugs: MESOS-2061 and MESOS-2066 https://issues.apache.org/jira/browse/MESOS-2061 https://issues.apache.org/jira/browse/MESOS-2066 Repository: mesos Description --- MESOS-2061: Add Unavailability and InverseOffer protobufs declarations. MESOS-2066: Add the Unavailability field to Offers. Also copied to v1 API. No integration with other components (that part is tracked in separate JIRAs, see MESOS-1474). Diffs - include/mesos/mesos.proto c40a09b2bb2b9444e9b90e86eefe6fc8f98b613d include/mesos/v1/mesos.proto ee15b9ae70aeb8cf803ea7e4b06f4443bd7bc9e2 Diff: https://reviews.apache.org/r/36321/diff/ Testing --- `make check` Thanks, Joseph Wu
Re: Review Request 37892: Renamed cgroups::kill to cgroups::signal
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/37892/ --- (Updated Aug. 28, 2015, 4:12 p.m.) Review request for mesos. Bugs: MESOS-3086 https://issues.apache.org/jira/browse/MESOS-3086 Repository: mesos Description --- As this function is actually just sending the signal we renamed it. Additionally we want to introduce an actual cgroups::kill() with MESOS-3086. Diffs (updated) - src/linux/cgroups.cpp 6ef42ed1bc719f334d1ac6e90919a1bc1840d31f src/tests/containerizer/cgroups_tests.cpp 0b171eeb53037f26b7e952830e88e59f1278e7c6 Diff: https://reviews.apache.org/r/37892/diff/ Testing --- Thanks, Joerg Schad
Review Request 37894: Renamed cgroups::kill to cgroups::signal.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/37894/ --- Review request for mesos, Jie Yu, Till Toenshoff, and Timothy Chen. Bugs: MESOS-3086 https://issues.apache.org/jira/browse/MESOS-3086 Repository: mesos Description --- As this function is actually just sending the signal we renamed it. Additionally we want to introduce an actual cgroups::kill() with MESOS-3086. Diffs - src/linux/cgroups.hpp 204c53038a1ccfa693f4f2293488cff8cdd60835 src/linux/cgroups.cpp 6ef42ed1bc719f334d1ac6e90919a1bc1840d31f src/tests/containerizer/cgroups_tests.cpp 0b171eeb53037f26b7e952830e88e59f1278e7c6 Diff: https://reviews.apache.org/r/37894/diff/ Testing --- Thanks, Joerg Schad
Re: Review Request 37427: Docker registry: adding TokenManager.
On Aug. 28, 2015, 12:18 a.m., Timothy Chen wrote: src/slave/containerizer/provisioners/docker/token_manager.hpp, line 223 https://reviews.apache.org/r/37427/diff/12/?file=1052806#file1052806line223 Is there a reason you need to include this in the header? Can we just put it in cpp? Jojy Varghese wrote: The only reason being that this is a property of the TokenManager. Timothy Chen wrote: I see, does it need to be? Can't we define a static const in the cpp? We could but then it wont be a class property. Here the constant reflects the property of the class. - Jojy --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/37427/#review96752 --- On Aug. 28, 2015, 4:27 a.m., Jojy Varghese wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/37427/ --- (Updated Aug. 28, 2015, 4:27 a.m.) Review request for mesos, Lily Chen, Joris Van Remoortere, and Timothy Chen. Repository: mesos Description --- Changes: - Added Token implementation (RFC 7519). - Added TokenManager implementation. This component keeps a cache of tokens requested for any future requests. Diffs - src/Makefile.am 7b620ff66856b3f0adac121b3297d55ed71a3d99 src/slave/containerizer/provisioners/docker/token_manager.hpp PRE-CREATION src/slave/containerizer/provisioners/docker/token_manager.cpp PRE-CREATION src/tests/provisioners/docker_provisioner_tests.cpp PRE-CREATION Diff: https://reviews.apache.org/r/37427/diff/ Testing --- make check. Thanks, Jojy Varghese
Re: Review Request 36321: Maintenance primitives: Add Unavailability and InverseOffer protobufs.
On Aug. 28, 2015, 7:49 a.m., Joris Van Remoortere wrote: include/mesos/mesos.proto, line 877 https://reviews.apache.org/r/36321/diff/13-14/?file=1053309#file1053309line877 In your last diff, you changed a bunch of `window` to `interval`. Do you want to change this one as well? If not, are you sure you wanted all the `window` to `interval` changes in diff 13-14? Missed that one. Good catch :) I'm changing all the synonyms I use to describe unavailability (period, window, interval, range, etc) just to interval, for clarity. - Joseph --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36321/#review96866 --- On Aug. 28, 2015, 10:11 a.m., Joseph Wu wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36321/ --- (Updated Aug. 28, 2015, 10:11 a.m.) Review request for mesos, Benjamin Hindman, Ben Mahler, Artem Harutyunyan, Joris Van Remoortere, and Vinod Kone. Bugs: MESOS-2061 and MESOS-2066 https://issues.apache.org/jira/browse/MESOS-2061 https://issues.apache.org/jira/browse/MESOS-2066 Repository: mesos Description --- MESOS-2061: Add Unavailability and InverseOffer protobufs declarations. MESOS-2066: Add the Unavailability field to Offers. Also copied to v1 API. No integration with other components (that part is tracked in separate JIRAs, see MESOS-1474). Diffs - include/mesos/mesos.proto c40a09b2bb2b9444e9b90e86eefe6fc8f98b613d include/mesos/v1/mesos.proto ee15b9ae70aeb8cf803ea7e4b06f4443bd7bc9e2 Diff: https://reviews.apache.org/r/36321/diff/ Testing --- `make check` Thanks, Joseph Wu
Re: Review Request 36571: Maintenance Primitives: Add maintenance-related, registry protobufs.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36571/ --- (Updated Aug. 28, 2015, 10:25 a.m.) Review request for mesos, Benjamin Hindman, Ben Mahler, Artem Harutyunyan, Joris Van Remoortere, and Vinod Kone. Changes --- Fix a small, but important typo. Bugs: MESOS-3066 https://issues.apache.org/jira/browse/MESOS-3066 Repository: mesos Description --- New protobufs: * MachineID - Describes a single box that holds one or more agents. * MachineIDs - A list of boxes. * MachineInfo - Holds a box and some extra information about its status. * MachineInfo::Mode - An enum for the three states of a machine: UP, DRAINING, DOWN. * maintenance::Window - A set of machines and a planned downtime period. * maintenance::Schedule - A set of maintenance windows. Diffs (updated) - include/mesos/maintenance/maintenance.hpp PRE-CREATION include/mesos/maintenance/maintenance.proto PRE-CREATION include/mesos/mesos.proto c40a09b2bb2b9444e9b90e86eefe6fc8f98b613d include/mesos/v1/mesos.proto ee15b9ae70aeb8cf803ea7e4b06f4443bd7bc9e2 src/Makefile.am 7b620ff66856b3f0adac121b3297d55ed71a3d99 src/master/registry.proto a1995e56886f5296bc71d1bdfebec0d8316396c6 Diff: https://reviews.apache.org/r/36571/diff/ Testing --- `make check` Thanks, Joseph Wu
Re: Review Request 37894: Renamed cgroups::kill to cgroups::signal.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/37894/#review96883 --- Why? Everyone knows kill(2) sends a signal while signal(2) installs a signal handler... - Cong Wang On Aug. 28, 2015, 4:39 p.m., Joerg Schad wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/37894/ --- (Updated Aug. 28, 2015, 4:39 p.m.) Review request for mesos, Jie Yu, Till Toenshoff, and Timothy Chen. Bugs: MESOS-3086 https://issues.apache.org/jira/browse/MESOS-3086 Repository: mesos Description --- As this function is actually just sending the signal we renamed it. Additionally we want to introduce an actual cgroups::kill() with MESOS-3086. Diffs - src/linux/cgroups.hpp 204c53038a1ccfa693f4f2293488cff8cdd60835 src/linux/cgroups.cpp 6ef42ed1bc719f334d1ac6e90919a1bc1840d31f src/tests/containerizer/cgroups_tests.cpp 0b171eeb53037f26b7e952830e88e59f1278e7c6 Diff: https://reviews.apache.org/r/37894/diff/ Testing --- Thanks, Joerg Schad
Re: Review Request 37894: Renamed cgroups::kill to cgroups::signal.
On Aug. 28, 2015, 4:45 p.m., Cong Wang wrote: Why? Everyone knows kill(2) sends a signal while signal(2) installs a signal handler... Jie Yu wrote: IMO, that naming is confusing, and should be 'signal' and 'install'. This is actually answering a discussion here https://reviews.apache.org/r/36620/#comment152376. And I agree that the original names are weird, but you are right in that we should have an open discussion about this. I will send a mail to dev later on... - Joerg --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/37894/#review96883 --- On Aug. 28, 2015, 4:39 p.m., Joerg Schad wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/37894/ --- (Updated Aug. 28, 2015, 4:39 p.m.) Review request for mesos, Jie Yu, Till Toenshoff, and Timothy Chen. Bugs: MESOS-3086 https://issues.apache.org/jira/browse/MESOS-3086 Repository: mesos Description --- As this function is actually just sending the signal we renamed it. Additionally we want to introduce an actual cgroups::kill() with MESOS-3086. Diffs - src/linux/cgroups.hpp 204c53038a1ccfa693f4f2293488cff8cdd60835 src/linux/cgroups.cpp 6ef42ed1bc719f334d1ac6e90919a1bc1840d31f src/tests/containerizer/cgroups_tests.cpp 0b171eeb53037f26b7e952830e88e59f1278e7c6 Diff: https://reviews.apache.org/r/37894/diff/ Testing --- Thanks, Joerg Schad
Re: Review Request 37894: Renamed cgroups::kill to cgroups::signal.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/37894/#review96897 --- Patch looks great! Reviews applied: [37894] All tests passed. - Mesos ReviewBot On Aug. 28, 2015, 4:39 p.m., Joerg Schad wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/37894/ --- (Updated Aug. 28, 2015, 4:39 p.m.) Review request for mesos, Jie Yu, Till Toenshoff, and Timothy Chen. Bugs: MESOS-3086 https://issues.apache.org/jira/browse/MESOS-3086 Repository: mesos Description --- As this function is actually just sending the signal we renamed it. Additionally we want to introduce an actual cgroups::kill() with MESOS-3086. Diffs - src/linux/cgroups.hpp 204c53038a1ccfa693f4f2293488cff8cdd60835 src/linux/cgroups.cpp 6ef42ed1bc719f334d1ac6e90919a1bc1840d31f src/tests/containerizer/cgroups_tests.cpp 0b171eeb53037f26b7e952830e88e59f1278e7c6 Diff: https://reviews.apache.org/r/37894/diff/ Testing --- Thanks, Joerg Schad
Re: Review Request 37427: Docker registry: adding TokenManager.
On Aug. 28, 2015, 7:56 a.m., Timothy Chen wrote: src/slave/containerizer/provisioners/docker/token_manager.cpp, line 223 https://reviews.apache.org/r/37427/diff/14/?file=1057180#file1057180line223 If you put the cache here in TokenManager instead of TokenManagerProcess then you'll going to be concurrent access of all the fields since it's not serialized by libprocess. I think tokenCache_ will have undefined behavior when you run at/contains while it's being inserted or deleted then. I suggest you move all this into the process instead. Here the getToken is called in the context of a dispatch from RegistryClient. Do we still need another dispatch? - Jojy --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/37427/#review96841 --- On Aug. 28, 2015, 4:27 a.m., Jojy Varghese wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/37427/ --- (Updated Aug. 28, 2015, 4:27 a.m.) Review request for mesos, Lily Chen, Joris Van Remoortere, and Timothy Chen. Repository: mesos Description --- Changes: - Added Token implementation (RFC 7519). - Added TokenManager implementation. This component keeps a cache of tokens requested for any future requests. Diffs - src/Makefile.am 7b620ff66856b3f0adac121b3297d55ed71a3d99 src/slave/containerizer/provisioners/docker/token_manager.hpp PRE-CREATION src/slave/containerizer/provisioners/docker/token_manager.cpp PRE-CREATION src/tests/provisioners/docker_provisioner_tests.cpp PRE-CREATION Diff: https://reviews.apache.org/r/37427/diff/ Testing --- make check. Thanks, Jojy Varghese
Re: Review Request 36620: Added Non-Freezeer Task Killer.
On Aug. 27, 2015, 7:02 p.m., Jie Yu wrote:
src/linux/cgroups.cpp, lines 1776-1791
https://reviews.apache.org/r/36620/diff/13/?file=1039336#file1039336line1776
Could you please introduce a new function under cgroups namespace and
put this logic there:
```
// Kill all processes in the given cgroup.
FutureNothing cgroups::kill(
const string hierarchy,
const string cgroup)
{
...
if (freezerCheckError.isNone()) {
} else {
}
return ...;
}
```
You may want to rename the exsiting `cgroups::kill(hierarchy, cgroup,
signal)` to `cgroups::signal`.
Timothy Chen wrote:
Hi jie, thanks for chiming in. This should be ready after the comments
are addressed. I'll try to merge this as well when it's done. I'll ping Joerg
about this.
Joerg Schad wrote:
Hi Jie,
I am not sure whether I should move the entire logic further into
cgroups::kill. Both path (freezer vs Non-freezer) have distinct logic on how
to destroy the cgroups, which in my understanding makes sense to have in
seperate TasksKiller entities. Secondly cgroups::kill currently is little
more than trying to kill the cgroup, when moving the logic in here
cgroups::kill would have to call cgroups::freeze or call the iterative logic.
Secondly, in my understanding the interface here is the destroy() call. I
actually would like to refactor this piece of code in general, but this is
outside the scope of this patch. If we want to discuss this further -or
misunderstand something here- I would be happy to discuss it further!
Jie Yu wrote:
Hi Joerg,
I think you misunderstood what I propose here. I am not asking you to
merge the logics of TasksKiller and FreezerTasksKiller. I think it's great
that you separate these two Processes. What I am sugguesting here is that you
pull the code from 1777-1790 to a separate function (like the following). We
may want to reuse that function in Mesos containerizer launcher in the future
(i.e., not rely on freezer). It also makes the logics in Destroyer cleaner.
Let me know your thoughts! Thank you for taking on this!
```
virtual void initialize()
{
// Stop when no one cares.
promise.future().onDiscard(lambda::bind(
static_castvoid (*)(const UPID, bool)(terminate), self(), true));
// Kill tasks in the given cgroups in parallel. Use collect mechanism to
// wait until all kill processes finish.
foreach (const string cgroup, cgroups) {
killers.push_back(cgroups:kill(hierarchy, cgroup));
}
collect(killers)
.onAny(defer(self(), Destroyer::killed, lambda::_1));
}
// Kill all processes in the given cgroup.
FutureNothing cgroups::kill(
const string hierarchy,
const string cgroup)
{
FutureNothing future;
// Use the freezer subsystem if available.
OptionError freezerCheckError =
verify(hierarchy, cgroup, freezer.state);
if (freezerCheckError.isNone()) {
internal::FreezerTasksKiller* killer =
new internal::FreezerTasksKiller(hierarchy, cgroup);
future = killer-future();
spawn(killer, true);
} else {
internal::TasksKiller* killer =
new internal::TasksKiller(hierarchy, cgroup);
future = killer-future();
spawn(killer, true);
}
return future;
}
```
As the renaming seems to be controversial
(https://reviews.apache.org/r/37894/), I would right now introduce
cgroups::killProcesses. Does that work for you?
- Joerg
---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/36620/#review96734
---
On Aug. 28, 2015, 3:48 p.m., Joerg Schad wrote:
---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/36620/
---
(Updated Aug. 28, 2015, 3:48 p.m.)
Review request for mesos, Benjamin Hindman, Till Toenshoff, and Timothy Chen.
Bugs: MESOS-3086
https://issues.apache.org/jira/browse/MESOS-3086
Repository: mesos
Description
---
Added Non-Freezeer Task Killer.
Diffs
-
src/linux/cgroups.cpp 6ef42ed1bc719f334d1ac6e90919a1bc1840d31f
Diff: https://reviews.apache.org/r/36620/diff/
Testing
---
sudo make check
+ manual tests
Thanks,
Joerg Schad
Re: Review Request 37894: Renamed cgroups::kill to cgroups::signal.
On Aug. 28, 2015, 4:45 p.m., Cong Wang wrote: Why? Everyone knows kill(2) sends a signal while signal(2) installs a signal handler... Jie Yu wrote: IMO, that naming is confusing, and should be 'signal' and 'install'. Joerg Schad wrote: This is actually answering a discussion here https://reviews.apache.org/r/36620/#comment152376. And I agree that the original names are weird, but you are right in that we should have an open discussion about this. I will send a mail to dev later on... Like it or not, it is already a part of POSIX (if not just UNIX), pthread_kill() has the same problem. It's just too late to change, as it simply breaks every programmer's expectation. - Cong --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/37894/#review96883 --- On Aug. 28, 2015, 4:39 p.m., Joerg Schad wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/37894/ --- (Updated Aug. 28, 2015, 4:39 p.m.) Review request for mesos, Jie Yu, Till Toenshoff, and Timothy Chen. Bugs: MESOS-3086 https://issues.apache.org/jira/browse/MESOS-3086 Repository: mesos Description --- As this function is actually just sending the signal we renamed it. Additionally we want to introduce an actual cgroups::kill() with MESOS-3086. Diffs - src/linux/cgroups.hpp 204c53038a1ccfa693f4f2293488cff8cdd60835 src/linux/cgroups.cpp 6ef42ed1bc719f334d1ac6e90919a1bc1840d31f src/tests/containerizer/cgroups_tests.cpp 0b171eeb53037f26b7e952830e88e59f1278e7c6 Diff: https://reviews.apache.org/r/37894/diff/ Testing --- Thanks, Joerg Schad
Re: Review Request 36571: Maintenance Primitives: Add maintenance-related, registry protobufs.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36571/ --- (Updated Aug. 28, 2015, 10:20 a.m.) Review request for mesos, Benjamin Hindman, Ben Mahler, Artem Harutyunyan, Joris Van Remoortere, and Vinod Kone. Changes --- Comments, some tweaks for naming, and a rebase. Bugs: MESOS-3066 https://issues.apache.org/jira/browse/MESOS-3066 Repository: mesos Description --- New protobufs: * MachineID - Describes a single box that holds one or more agents. * MachineIDs - A list of boxes. * MachineInfo - Holds a box and some extra information about its status. * MachineInfo::Mode - An enum for the three states of a machine: UP, DRAINING, DOWN. * maintenance::Window - A set of machines and a planned downtime period. * maintenance::Schedule - A set of maintenance windows. Diffs (updated) - include/mesos/maintenance/maintenance.hpp PRE-CREATION include/mesos/maintenance/maintenance.proto PRE-CREATION include/mesos/mesos.proto c40a09b2bb2b9444e9b90e86eefe6fc8f98b613d include/mesos/v1/mesos.proto ee15b9ae70aeb8cf803ea7e4b06f4443bd7bc9e2 src/Makefile.am 7b620ff66856b3f0adac121b3297d55ed71a3d99 src/master/registry.proto a1995e56886f5296bc71d1bdfebec0d8316396c6 Diff: https://reviews.apache.org/r/36571/diff/ Testing --- `make check` Thanks, Joseph Wu
Re: Review Request 37314: Maintenance Primitives: Populate master's maintenance-related local state upon recovery from registry.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/37314/ --- (Updated Aug. 28, 2015, 10:38 a.m.) Review request for mesos, Benjamin Hindman, Ben Mahler, Artem Harutyunyan, Joris Van Remoortere, and Vinod Kone. Changes --- Rebased. Bugs: MESOS-3045 https://issues.apache.org/jira/browse/MESOS-3045 Repository: mesos Description --- Note: Tests will be added with the related HTTP endpoints and registrar operations, later in this review chain. Diffs (updated) - include/mesos/type_utils.hpp 1c8f95b894285140a228ab4202851ee391ffdcc6 src/master/master.hpp 36c67599ef2c470da8d95f2caf926a154342d2cc src/master/master.cpp 95207d24db0aa052eb70c4cc7eb75d0611c365cf Diff: https://reviews.apache.org/r/37314/diff/ Testing --- `make check` Thanks, Joseph Wu
Re: Review Request 36908: Added QuotaInfo Protobuf.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36908/#review96906 --- include/mesos/master/quota.proto (line 38) https://reviews.apache.org/r/36908/#comment152591 s/guarantees/guarantee - Alexander Rukletsov On Aug. 5, 2015, 2:03 p.m., Joerg Schad wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36908/ --- (Updated Aug. 5, 2015, 2:03 p.m.) Review request for mesos, Alexander Rukletsov, Bernd Mathiske, and Till Toenshoff. Bugs: MESOS-3164 https://issues.apache.org/jira/browse/MESOS-3164 Repository: mesos Description --- Added QuotaInfo Protobuf. Diffs - include/mesos/master/quota.hpp PRE-CREATION include/mesos/master/quota.proto PRE-CREATION src/Makefile.am 54eaf205eecb6bf1a9a5c4b5ddad55f46ad635ec Diff: https://reviews.apache.org/r/36908/diff/ Testing --- make distcheck Thanks, Joerg Schad
Re: Review Request 36620: Added Non-Freezeer Task Killer.
On Aug. 27, 2015, 7:02 p.m., Jie Yu wrote:
src/linux/cgroups.cpp, lines 1776-1791
https://reviews.apache.org/r/36620/diff/13/?file=1039336#file1039336line1776
Could you please introduce a new function under cgroups namespace and
put this logic there:
```
// Kill all processes in the given cgroup.
FutureNothing cgroups::kill(
const string hierarchy,
const string cgroup)
{
...
if (freezerCheckError.isNone()) {
} else {
}
return ...;
}
```
You may want to rename the exsiting `cgroups::kill(hierarchy, cgroup,
signal)` to `cgroups::signal`.
Timothy Chen wrote:
Hi jie, thanks for chiming in. This should be ready after the comments
are addressed. I'll try to merge this as well when it's done. I'll ping Joerg
about this.
Joerg Schad wrote:
Hi Jie,
I am not sure whether I should move the entire logic further into
cgroups::kill. Both path (freezer vs Non-freezer) have distinct logic on how
to destroy the cgroups, which in my understanding makes sense to have in
seperate TasksKiller entities. Secondly cgroups::kill currently is little
more than trying to kill the cgroup, when moving the logic in here
cgroups::kill would have to call cgroups::freeze or call the iterative logic.
Secondly, in my understanding the interface here is the destroy() call. I
actually would like to refactor this piece of code in general, but this is
outside the scope of this patch. If we want to discuss this further -or
misunderstand something here- I would be happy to discuss it further!
Jie Yu wrote:
Hi Joerg,
I think you misunderstood what I propose here. I am not asking you to
merge the logics of TasksKiller and FreezerTasksKiller. I think it's great
that you separate these two Processes. What I am sugguesting here is that you
pull the code from 1777-1790 to a separate function (like the following). We
may want to reuse that function in Mesos containerizer launcher in the future
(i.e., not rely on freezer). It also makes the logics in Destroyer cleaner.
Let me know your thoughts! Thank you for taking on this!
```
virtual void initialize()
{
// Stop when no one cares.
promise.future().onDiscard(lambda::bind(
static_castvoid (*)(const UPID, bool)(terminate), self(), true));
// Kill tasks in the given cgroups in parallel. Use collect mechanism to
// wait until all kill processes finish.
foreach (const string cgroup, cgroups) {
killers.push_back(cgroups:kill(hierarchy, cgroup));
}
collect(killers)
.onAny(defer(self(), Destroyer::killed, lambda::_1));
}
// Kill all processes in the given cgroup.
FutureNothing cgroups::kill(
const string hierarchy,
const string cgroup)
{
FutureNothing future;
// Use the freezer subsystem if available.
OptionError freezerCheckError =
verify(hierarchy, cgroup, freezer.state);
if (freezerCheckError.isNone()) {
internal::FreezerTasksKiller* killer =
new internal::FreezerTasksKiller(hierarchy, cgroup);
future = killer-future();
spawn(killer, true);
} else {
internal::TasksKiller* killer =
new internal::TasksKiller(hierarchy, cgroup);
future = killer-future();
spawn(killer, true);
}
return future;
}
```
Joerg Schad wrote:
As the renaming seems to be controversial
(https://reviews.apache.org/r/37894/), I would right now introduce
cgroups::killProcesses. Does that work for you?
yeah, that sounds good to me.
- Jie
---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/36620/#review96734
---
On Aug. 28, 2015, 3:48 p.m., Joerg Schad wrote:
---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/36620/
---
(Updated Aug. 28, 2015, 3:48 p.m.)
Review request for mesos, Benjamin Hindman, Till Toenshoff, and Timothy Chen.
Bugs: MESOS-3086
https://issues.apache.org/jira/browse/MESOS-3086
Repository: mesos
Description
---
Added Non-Freezeer Task Killer.
Diffs
-
src/linux/cgroups.cpp 6ef42ed1bc719f334d1ac6e90919a1bc1840d31f
Diff: https://reviews.apache.org/r/36620/diff/
Testing
---
sudo make check
+ manual tests
Thanks,
Joerg Schad
Re: Review Request 37427: Docker registry: adding TokenManager.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/37427/ --- (Updated Aug. 28, 2015, 6:36 p.m.) Review request for mesos, Lily Chen, Joris Van Remoortere, and Timothy Chen. Changes --- review comments. Repository: mesos Description --- Changes: - Added Token implementation (RFC 7519). - Added TokenManager implementation. This component keeps a cache of tokens requested for any future requests. Diffs (updated) - src/Makefile.am 7b620ff66856b3f0adac121b3297d55ed71a3d99 src/slave/containerizer/provisioners/docker/token_manager.hpp PRE-CREATION src/slave/containerizer/provisioners/docker/token_manager.cpp PRE-CREATION src/tests/provisioners/docker_provisioner_tests.cpp PRE-CREATION Diff: https://reviews.apache.org/r/37427/diff/ Testing --- make check. Thanks, Jojy Varghese
Re: Review Request 37773: Docker: Adding registry client.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/37773/#review96905 --- src/slave/containerizer/provisioners/docker/registry_client.hpp (line 46) https://reviews.apache.org/r/37773/#comment152585 Why not just use option.getOrElse()? src/slave/containerizer/provisioners/docker/registry_client.hpp (line 77) https://reviews.apache.org/r/37773/#comment152586 userId src/slave/containerizer/provisioners/docker/registry_client.hpp (line 116) https://reviews.apache.org/r/37773/#comment152587 We usually spell timeout as one word everywhere else, so all lower case. src/slave/containerizer/provisioners/docker/registry_client.hpp (line 150) https://reviews.apache.org/r/37773/#comment152588 Do we need to define static here if it's not being used anywhere else but the cpp files? src/slave/containerizer/provisioners/docker/registry_client.cpp (line 41) https://reviews.apache.org/r/37773/#comment152589 Kill extra line src/slave/containerizer/provisioners/docker/registry_client.cpp (line 87) https://reviews.apache.org/r/37773/#comment152593 What's the reasoning behind prefixing underscores for these? You've already added trailing underscores for your class variables right? src/slave/containerizer/provisioners/docker/registry_client.cpp (line 91) https://reviews.apache.org/r/37773/#comment152590 getOrElse src/slave/containerizer/provisioners/docker/registry_client.cpp (line 114) https://reviews.apache.org/r/37773/#comment152592 I'm wondering why you need to use promise here. If you put all this logic in getManifest then essentially you just need to return the dispatch right? And in getManifest you just chian your return with onFailed and onAny too. Also even if you don't move it in getManifest why can't you just return result.onFailed(...).onAny(...)? src/slave/containerizer/provisioners/docker/registry_client.cpp (line 153) https://reviews.apache.org/r/37773/#comment152594 What residue? - Timothy Chen On Aug. 28, 2015, 6:38 p.m., Jojy Varghese wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/37773/ --- (Updated Aug. 28, 2015, 6:38 p.m.) Review request for mesos, Lily Chen and Timothy Chen. Repository: mesos Description --- Added implementation for docker registry's Get Manifest and Get Blob APIs. Diffs - src/Makefile.am 7b620ff66856b3f0adac121b3297d55ed71a3d99 src/slave/containerizer/provisioners/docker/registry_client.hpp PRE-CREATION src/slave/containerizer/provisioners/docker/registry_client.cpp PRE-CREATION src/tests/provisioners/docker_provisioner_tests.cpp PRE-CREATION Diff: https://reviews.apache.org/r/37773/diff/ Testing --- make check Thanks, Jojy Varghese
Re: Review Request 37894: Renamed cgroups::kill to cgroups::signal.
On Aug. 28, 2015, 4:45 p.m., Cong Wang wrote: Why? Everyone knows kill(2) sends a signal while signal(2) installs a signal handler... Jie Yu wrote: IMO, that naming is confusing, and should be 'signal' and 'install'. Joerg Schad wrote: This is actually answering a discussion here https://reviews.apache.org/r/36620/#comment152376. And I agree that the original names are weird, but you are right in that we should have an open discussion about this. I will send a mail to dev later on... Cong Wang wrote: Like it or not, it is already a part of POSIX (if not just UNIX), pthread_kill() has the same problem. It's just too late to change, as it simply breaks every programmer's expectation. This is cgroups::signal and cgroups::kill (has nothing to do with Posix kill and signal). It reads more natural: signal a cgroup, kill a cgroup. I don't think it'll confuse the reader. Are you suggesting that any thing suffixed with 'kill' should be linked to kill(2) in posix? I have to disagree with that. - Jie --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/37894/#review96883 --- On Aug. 28, 2015, 4:39 p.m., Joerg Schad wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/37894/ --- (Updated Aug. 28, 2015, 4:39 p.m.) Review request for mesos, Jie Yu, Till Toenshoff, and Timothy Chen. Bugs: MESOS-3086 https://issues.apache.org/jira/browse/MESOS-3086 Repository: mesos Description --- As this function is actually just sending the signal we renamed it. Additionally we want to introduce an actual cgroups::kill() with MESOS-3086. Diffs - src/linux/cgroups.hpp 204c53038a1ccfa693f4f2293488cff8cdd60835 src/linux/cgroups.cpp 6ef42ed1bc719f334d1ac6e90919a1bc1840d31f src/tests/containerizer/cgroups_tests.cpp 0b171eeb53037f26b7e952830e88e59f1278e7c6 Diff: https://reviews.apache.org/r/37894/diff/ Testing --- Thanks, Joerg Schad
Re: Review Request 37773: Docker: Adding registry client.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/37773/ --- (Updated Aug. 28, 2015, 6:38 p.m.) Review request for mesos, Lily Chen and Timothy Chen. Changes --- review comments addressed Repository: mesos Description --- Added implementation for docker registry's Get Manifest and Get Blob APIs. Diffs (updated) - src/Makefile.am 7b620ff66856b3f0adac121b3297d55ed71a3d99 src/slave/containerizer/provisioners/docker/registry_client.hpp PRE-CREATION src/slave/containerizer/provisioners/docker/registry_client.cpp PRE-CREATION src/tests/provisioners/docker_provisioner_tests.cpp PRE-CREATION Diff: https://reviews.apache.org/r/37773/diff/ Testing --- make check Thanks, Jojy Varghese
Re: Review Request 37773: Docker: Adding registry client.
On Aug. 28, 2015, 5:21 p.m., Lily Chen wrote: src/slave/containerizer/provisioners/docker/registry_client.hpp, line 34 https://reviews.apache.org/r/37773/diff/5/?file=1057183#file1057183line34 No need for provisioners namespace, see appc example of mesos::internal::slave::appc Why is that? - Jojy --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/37773/#review96896 --- On Aug. 28, 2015, 6:38 p.m., Jojy Varghese wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/37773/ --- (Updated Aug. 28, 2015, 6:38 p.m.) Review request for mesos, Lily Chen and Timothy Chen. Repository: mesos Description --- Added implementation for docker registry's Get Manifest and Get Blob APIs. Diffs - src/Makefile.am 7b620ff66856b3f0adac121b3297d55ed71a3d99 src/slave/containerizer/provisioners/docker/registry_client.hpp PRE-CREATION src/slave/containerizer/provisioners/docker/registry_client.cpp PRE-CREATION src/tests/provisioners/docker_provisioner_tests.cpp PRE-CREATION Diff: https://reviews.apache.org/r/37773/diff/ Testing --- make check Thanks, Jojy Varghese
Re: Review Request 37773: Docker: Adding registry client.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/37773/#review96932 --- src/slave/containerizer/provisioners/docker/registry_client.cpp (line 409) https://reviews.apache.org/r/37773/#comment152627 I think path may be misleading, can we name this repo? src/slave/containerizer/provisioners/docker/registry_client.cpp (line 415) https://reviews.apache.org/r/37773/#comment152624 can we do a path::join here? It would be hard to get the /s correct just passing path through src/slave/containerizer/provisioners/docker/registry_client.cpp (line 465) https://reviews.apache.org/r/37773/#comment152628 Same issue with path naming as with getManifest src/slave/containerizer/provisioners/docker/registry_client.cpp (lines 468 - 469) https://reviews.apache.org/r/37773/#comment152632 Add defaults here as with as with image manifest. - Lily Chen On Aug. 28, 2015, 6:38 p.m., Jojy Varghese wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/37773/ --- (Updated Aug. 28, 2015, 6:38 p.m.) Review request for mesos, Lily Chen and Timothy Chen. Repository: mesos Description --- Added implementation for docker registry's Get Manifest and Get Blob APIs. Diffs - src/Makefile.am 7b620ff66856b3f0adac121b3297d55ed71a3d99 src/slave/containerizer/provisioners/docker/registry_client.hpp PRE-CREATION src/slave/containerizer/provisioners/docker/registry_client.cpp PRE-CREATION src/tests/provisioners/docker_provisioner_tests.cpp PRE-CREATION Diff: https://reviews.apache.org/r/37773/diff/ Testing --- make check Thanks, Jojy Varghese
Re: Review Request 37913: Improve allocator filtering by keeping per-slave filter sets.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/37913/#review96952 --- Diff seems weird -- maybe needs a rebase. - Neil Conway On Aug. 28, 2015, 11:38 p.m., James Peach wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/37913/ --- (Updated Aug. 28, 2015, 11:38 p.m.) Review request for mesos and Ben Mahler. Bugs: MESOS-3052 https://issues.apache.org/jira/browse/MESOS-3052 Repository: mesos Description --- When frameworks refuse a lot of resources, the list of filters gets long. Since the filters are per-slave, HierarchicalAllocatorProcess::isFiltered spends a lot of time just comparing SlaveID (which tend to be long strings). Eliminate this whole problem by organizing the filters by SlaveID in the first place. Diffs - 3rdparty/libprocess/3rdparty/CMakeLists.txt 997cc0d0e316e316136d4746e50e9e292a82b36b 3rdparty/libprocess/3rdparty/Makefile.am eb34251d24b1e5d1540151b59cf1062ca85aeb03 3rdparty/libprocess/3rdparty/gmock-1.6.0.tar.gz PRE-CREATION 3rdparty/libprocess/3rdparty/gmock-1.7.0.tar.gz 09f5ea3ce95fab34c505367ae04965e01c8bb30d 3rdparty/libprocess/3rdparty/stout/Makefile.am f95ed03004d4e5382874d75969bc9285a0f44918 3rdparty/libprocess/3rdparty/stout/README.md d7596e58e67699c1bc9c28da841b89ccd26f3a34 3rdparty/libprocess/3rdparty/stout/cmake/StoutTestsConfigure.cmake 08325297ceb79b80c305ba4f2164ffd37591a0e8 3rdparty/libprocess/3rdparty/stout/include/Makefile.am 8853f92fcfcff81d0a3197bade02110685fa0325 3rdparty/libprocess/3rdparty/stout/include/stout/abort.hpp 2b003d26d6b6b65b1d7b1dd6396f808c35b53177 3rdparty/libprocess/3rdparty/stout/include/stout/attributes.hpp 27087041d8255b96159bb10c184f00cf5bc9c34e 3rdparty/libprocess/3rdparty/stout/include/stout/base64.hpp 4893e7ba0b7d83fd3ba36bf18aa541c60293cc23 3rdparty/libprocess/3rdparty/stout/include/stout/bytes.hpp e9cf85637157f98a0eac166096bb18fa5652c669 3rdparty/libprocess/3rdparty/stout/include/stout/cache.hpp 0d51c8d418acb49b52cebfb644ee0476d6ec4502 3rdparty/libprocess/3rdparty/stout/include/stout/duration.hpp 90551541f59889e96b21dbe1b65d3904850464c2 3rdparty/libprocess/3rdparty/stout/include/stout/exit.hpp 38dabd45aa1d7f02e9991ce4ae28b44cd39db87c 3rdparty/libprocess/3rdparty/stout/include/stout/fatal.hpp PRE-CREATION 3rdparty/libprocess/3rdparty/stout/include/stout/flags/flags.hpp 9da213f802aec6a7768ce6f5aea7b437d980356a 3rdparty/libprocess/3rdparty/stout/include/stout/format.hpp 1cf6dd18aa163688d6c8f3a6d33eacad3918015d 3rdparty/libprocess/3rdparty/stout/include/stout/gzip.hpp 85c773ac675c88b313dffda7a9c32bac42ebe62d 3rdparty/libprocess/3rdparty/stout/include/stout/hashmap.hpp f09bea125035aa3621402b83608b233e42877559 3rdparty/libprocess/3rdparty/stout/include/stout/hashset.hpp 1839d28638cd82dae10ba9b0f99c1a97cf34f9c9 3rdparty/libprocess/3rdparty/stout/include/stout/interval.hpp d207dc5ad7558818da7fd0d04c6ef8df217b78c1 3rdparty/libprocess/3rdparty/stout/include/stout/ip.hpp 1ad119d54820e97497b1773518875be25ddbf98a 3rdparty/libprocess/3rdparty/stout/include/stout/json.hpp f28138c4682c41e94ab6c7641a78d66b2f9daa5f 3rdparty/libprocess/3rdparty/stout/include/stout/mac.hpp 9428717fac4655898d7768957f02937592e1a398 3rdparty/libprocess/3rdparty/stout/include/stout/multihashmap.hpp d9e4031cee64e48ad50541c04ca11e7861d0a17c 3rdparty/libprocess/3rdparty/stout/include/stout/net.hpp 3f829bafe96526bc2263c9f228f85de38c435f60 3rdparty/libprocess/3rdparty/stout/include/stout/option.hpp db5e33220844d20ef08a7324f641eeb1ff6d2052 3rdparty/libprocess/3rdparty/stout/include/stout/os.hpp 5141c1369af60afd6cd5c70c6295d575ea960a83 3rdparty/libprocess/3rdparty/stout/include/stout/os/close.hpp 972f833835633ec343f97b3cde504772537c5272 3rdparty/libprocess/3rdparty/stout/include/stout/os/exists.hpp 6eb7f8f2be208e591d43088b2541d030a272f328 3rdparty/libprocess/3rdparty/stout/include/stout/os/fcntl.hpp 3728bc49477dff6203f255f87bc852beea86 3rdparty/libprocess/3rdparty/stout/include/stout/os/fork.hpp 8ea971fe72237423164adb0a4a10ddf1603d49cf 3rdparty/libprocess/3rdparty/stout/include/stout/os/killtree.hpp de11bdadf3222fc955fd4d1870d1c406535dc1b9 3rdparty/libprocess/3rdparty/stout/include/stout/os/linux.hpp b994b13941628947c9d12b8baae155d5da1ec7bd 3rdparty/libprocess/3rdparty/stout/include/stout/os/ls.hpp e80d885725b3f51c6640e24abba1f37d556fb476 3rdparty/libprocess/3rdparty/stout/include/stout/os/open.hpp 43f261fd7a60b534f642f826ebf6ab18d72180c2 3rdparty/libprocess/3rdparty/stout/include/stout/os/os.hpp
Re: Review Request 37814: Added documentation for libprocess environment variables
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/37814/#review96958 --- Patch looks great! Reviews applied: [37814] All tests passed. - Mesos ReviewBot On Aug. 28, 2015, 10:25 p.m., Greg Mann wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/37814/ --- (Updated Aug. 28, 2015, 10:25 p.m.) Review request for mesos, Alexander Rojas and haosdent huang. Bugs: MESOS-2466 https://issues.apache.org/jira/browse/MESOS-2466 Repository: mesos Description --- Added documentation for libprocess environment variables Diffs - docs/configuration.md 2b23d48c8841e43e6e2776dfe6bfa7c022a941a7 Diff: https://reviews.apache.org/r/37814/diff/ Testing --- Built the updated website using the docker site builder (https://github.com/mesosphere/mesos-website-container). Thanks, Greg Mann
Re: Review Request 37773: Docker: Adding registry client.
On Aug. 28, 2015, 11:03 p.m., Lily Chen wrote: src/slave/containerizer/provisioners/docker/registry_client.cpp, line 415 https://reviews.apache.org/r/37773/diff/6/?file=1058283#file1058283line415 can we do a path::join here? It would be hard to get the /s correct just passing path through This is not a file path. So os::path wont work. - Jojy --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/37773/#review96932 --- On Aug. 28, 2015, 6:38 p.m., Jojy Varghese wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/37773/ --- (Updated Aug. 28, 2015, 6:38 p.m.) Review request for mesos, Lily Chen and Timothy Chen. Repository: mesos Description --- Added implementation for docker registry's Get Manifest and Get Blob APIs. Diffs - src/Makefile.am 7b620ff66856b3f0adac121b3297d55ed71a3d99 src/slave/containerizer/provisioners/docker/registry_client.hpp PRE-CREATION src/slave/containerizer/provisioners/docker/registry_client.cpp PRE-CREATION src/tests/provisioners/docker_provisioner_tests.cpp PRE-CREATION Diff: https://reviews.apache.org/r/37773/diff/ Testing --- make check Thanks, Jojy Varghese
Re: Review Request 37773: Docker: Adding registry client.
On Aug. 28, 2015, 11:03 p.m., Lily Chen wrote: src/slave/containerizer/provisioners/docker/registry_client.cpp, line 409 https://reviews.apache.org/r/37773/diff/6/?file=1058283#file1058283line409 I think path may be misleading, can we name this repo? https://docs.docker.com/registry/spec/api/. If you look at the description of the field, its a path. - Jojy --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/37773/#review96932 --- On Aug. 28, 2015, 6:38 p.m., Jojy Varghese wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/37773/ --- (Updated Aug. 28, 2015, 6:38 p.m.) Review request for mesos, Lily Chen and Timothy Chen. Repository: mesos Description --- Added implementation for docker registry's Get Manifest and Get Blob APIs. Diffs - src/Makefile.am 7b620ff66856b3f0adac121b3297d55ed71a3d99 src/slave/containerizer/provisioners/docker/registry_client.hpp PRE-CREATION src/slave/containerizer/provisioners/docker/registry_client.cpp PRE-CREATION src/tests/provisioners/docker_provisioner_tests.cpp PRE-CREATION Diff: https://reviews.apache.org/r/37773/diff/ Testing --- make check Thanks, Jojy Varghese
Re: Review Request 37773: Docker: Adding registry client.
On Aug. 28, 2015, 6:46 p.m., Timothy Chen wrote: src/slave/containerizer/provisioners/docker/registry_client.hpp, line 150 https://reviews.apache.org/r/37773/diff/6/?file=1058282#file1058282line150 Do we need to define static here if it's not being used anywhere else but the cpp files? Its a class constant and hence declared here. On Aug. 28, 2015, 6:46 p.m., Timothy Chen wrote: src/slave/containerizer/provisioners/docker/registry_client.cpp, line 87 https://reviews.apache.org/r/37773/diff/6/?file=1058283#file1058283line87 What's the reasoning behind prefixing underscores for these? You've already added trailing underscores for your class variables right? The function body uses similar variables. Hence to disambiguate. On Aug. 28, 2015, 6:46 p.m., Timothy Chen wrote: src/slave/containerizer/provisioners/docker/registry_client.cpp, line 153 https://reviews.apache.org/r/37773/diff/6/?file=1058283#file1058283line153 What residue? The new directory created. - Jojy --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/37773/#review96905 --- On Aug. 28, 2015, 6:38 p.m., Jojy Varghese wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/37773/ --- (Updated Aug. 28, 2015, 6:38 p.m.) Review request for mesos, Lily Chen and Timothy Chen. Repository: mesos Description --- Added implementation for docker registry's Get Manifest and Get Blob APIs. Diffs - src/Makefile.am 7b620ff66856b3f0adac121b3297d55ed71a3d99 src/slave/containerizer/provisioners/docker/registry_client.hpp PRE-CREATION src/slave/containerizer/provisioners/docker/registry_client.cpp PRE-CREATION src/tests/provisioners/docker_provisioner_tests.cpp PRE-CREATION Diff: https://reviews.apache.org/r/37773/diff/ Testing --- make check Thanks, Jojy Varghese
Re: Review Request 37881: Implemented AppcProvisioner.
On Aug. 28, 2015, 7:59 p.m., Jie Yu wrote: src/slave/containerizer/provisioners/appc.cpp, lines 79-97 https://reviews.apache.org/r/37881/diff/1/?file=1057722#file1057722line79 Some high level comments. I think it will be beneficial if we can move the fetch and discovery logic to the Store. For the MVP, we don't have to impl. that in the Store. The Store only has one interface, which is the 'get' method. 'get' takes an Image::Appc and returns a vector of paths to layers. THis semantics is more natural and easy to understand: get layers for my image, if some layers are already cached, return them directly, otherwise, discover it and fetch it. The job of the provisioner is to get layers from the Store and stack them into a rootfs using the backend. It also manages the rootfs directories. To me, this is a more natural split of functionalities among components. In the future, we can also potentially unify the impl. of the provisioner (i.e., one single provisioner for both Docker and Appc images). All the image specific details are hiden in the Store interface. Thoughts? Jiang Yan Xu wrote: I am OK the this direction but a few points: 1) I think a dumb store that only returns what it has and stores what is given to it is natual too. (So how about stripping the fetching logic from the store and put it in the provisioner? This way the provisioner is the manager that controls all the dumb single-purpose helpers.) 2) Unifying the provisioner would be nice but one of the remaining obstacles is that we need to make sure we have a common provisioner direcotry layout, otherwise they need to be further extracted out from the provisioner and the provisioner becomes so thin that there is no point in having a unified provisioner anymore. -- So I am not totally sure if this will happen. 3) In the future there may be a lot of more logic required for the new Store if it is responsible for doing all the heavy-lifting: cache eviction, P2P fetching, registering new provision requests with ongoing fetching processes, etc. But I guess this is not an argument for keeping these in the provisioner, we should create other single purpose class to handle them, we just need to design a Store API that's takes these into consideration. For now since none of these aformentioned functionality is necessary for our read-only, no dependency provisioner let's find a way to check this as long as it doesn't make it hard to refactor in the future. Those are valid points! Let me express my rationale in some depth: Store manages store_dir and privisioner manages rootfs_dir. A natural split is that: any thing involved in updating 'store_dir' should be handled by Store and anything involved in updating 'rootfs_dir' should be handled by rootfs_dir. In that sense, fetching and caching should be handled by the Store. As you said, you can introduce sub-component in 'Store' to dealing with Discovery, fetching, caching, etc. Another reason I want fetching and caching to be done in Store, independent of provisioner, is that: maybe, in the future, we can support 'pre-fetching' which is not associated with any provisioning at all. Putting all such logics in the provisioner will complicate it a lot. A component with less dependency is definitely more managable. - Jie --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/37881/#review96903 --- On Aug. 28, 2015, 9:04 a.m., Jiang Yan Xu wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/37881/ --- (Updated Aug. 28, 2015, 9:04 a.m.) Review request for mesos, Jie Yu and Timothy Chen. Bugs: MESOS-2796 https://issues.apache.org/jira/browse/MESOS-2796 Repository: mesos Description --- Implemented AppcProvisioner. Diffs - src/Makefile.am 7b620ff66856b3f0adac121b3297d55ed71a3d99 src/slave/containerizer/provisioner.cpp efc7e6996ff6663bebaf61989a7e040bd2ad7a5e src/slave/containerizer/provisioners/appc.hpp PRE-CREATION src/slave/containerizer/provisioners/appc.cpp PRE-CREATION src/slave/containerizer/provisioners/appc/paths.hpp 41e3bf79da0854406c488855f953111e67353829 src/slave/containerizer/provisioners/appc/paths.cpp 3113c84b9526dd9e9e89fb9aa4ec75ed66a996c7 src/slave/flags.hpp e56738e2dfd6593ef8f093687919da287af78f77 src/slave/flags.cpp b36710d6d7a7250bc071a57310a2d54bfb3bc624 src/tests/containerizer/appc_provisioner_tests.cpp 47b66b9c30cefe8f9a8e2c1c1341776c2d235020 Diff:
Review Request 37908: Silence oversubscription logging
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/37908/ --- Review request for mesos and Jie Yu. Repository: mesos Description --- Resource estimates are continuously written to the logs even though they are empty (as with the majority of Mesos clusters at the moment). To reduce the noise, this patch moves log lines for empty resources to VLOG(2). Diffs - src/slave/slave.cpp 8cfad7b86675b10c98524174975828bbcf47aed2 Diff: https://reviews.apache.org/r/37908/diff/ Testing --- make check - with and without the patch, verifying the log statements. Thanks, Niklas Nielsen
Re: Review Request 37881: Implemented AppcProvisioner.
On Aug. 28, 2015, 7:59 p.m., Jie Yu wrote: src/slave/flags.cpp, lines 72-76 https://reviews.apache.org/r/37881/diff/1/?file=1057726#file1057726line72 Why do you still need this flag? Jiang Yan Xu wrote: We instantiate all supported backends but this flag specifies the user's choice for new images. I don't think we can make a perfect guess based soley on system compatibility: what if the user only has small images and don't have the mount point created? User does not care which backend we use, right? We just pick a backend that works. If there are multiple supported backend, just pick the one that's the best. The flag is a little wiered to me because what if bind backend is not supported but copy backend is supported, the user specifes the backend to be bind, should we fail, or fall back to the copy backend? I think we can punt on this flag for now since we only have one backend right now. - Jie --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/37881/#review96903 --- On Aug. 28, 2015, 9:04 a.m., Jiang Yan Xu wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/37881/ --- (Updated Aug. 28, 2015, 9:04 a.m.) Review request for mesos, Jie Yu and Timothy Chen. Bugs: MESOS-2796 https://issues.apache.org/jira/browse/MESOS-2796 Repository: mesos Description --- Implemented AppcProvisioner. Diffs - src/Makefile.am 7b620ff66856b3f0adac121b3297d55ed71a3d99 src/slave/containerizer/provisioner.cpp efc7e6996ff6663bebaf61989a7e040bd2ad7a5e src/slave/containerizer/provisioners/appc.hpp PRE-CREATION src/slave/containerizer/provisioners/appc.cpp PRE-CREATION src/slave/containerizer/provisioners/appc/paths.hpp 41e3bf79da0854406c488855f953111e67353829 src/slave/containerizer/provisioners/appc/paths.cpp 3113c84b9526dd9e9e89fb9aa4ec75ed66a996c7 src/slave/flags.hpp e56738e2dfd6593ef8f093687919da287af78f77 src/slave/flags.cpp b36710d6d7a7250bc071a57310a2d54bfb3bc624 src/tests/containerizer/appc_provisioner_tests.cpp 47b66b9c30cefe8f9a8e2c1c1341776c2d235020 Diff: https://reviews.apache.org/r/37881/diff/ Testing --- sudo make check. More test cases coming. Thanks, Jiang Yan Xu
Re: Review Request 37881: Implemented AppcProvisioner.
On Aug. 28, 2015, 12:59 p.m., Jie Yu wrote: src/slave/flags.cpp, lines 72-76 https://reviews.apache.org/r/37881/diff/1/?file=1057726#file1057726line72 Why do you still need this flag? Jiang Yan Xu wrote: We instantiate all supported backends but this flag specifies the user's choice for new images. I don't think we can make a perfect guess based soley on system compatibility: what if the user only has small images and don't have the mount point created? Jie Yu wrote: User does not care which backend we use, right? We just pick a backend that works. If there are multiple supported backend, just pick the one that's the best. The flag is a little wiered to me because what if bind backend is not supported but copy backend is supported, the user specifes the backend to be bind, should we fail, or fall back to the copy backend? I think we can punt on this flag for now since we only have one backend right now. Sorry I meant the operator and the image in the default container info: If the operator just downloads an Appc image from the interenet which doesn't have the mountpoint for BindBackend but he does't have the choice to choose CopyBackend instead (which we should add soon).. - Jiang Yan --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/37881/#review96903 --- On Aug. 28, 2015, 2:04 a.m., Jiang Yan Xu wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/37881/ --- (Updated Aug. 28, 2015, 2:04 a.m.) Review request for mesos, Jie Yu and Timothy Chen. Bugs: MESOS-2796 https://issues.apache.org/jira/browse/MESOS-2796 Repository: mesos Description --- Implemented AppcProvisioner. Diffs - src/Makefile.am 7b620ff66856b3f0adac121b3297d55ed71a3d99 src/slave/containerizer/provisioner.cpp efc7e6996ff6663bebaf61989a7e040bd2ad7a5e src/slave/containerizer/provisioners/appc.hpp PRE-CREATION src/slave/containerizer/provisioners/appc.cpp PRE-CREATION src/slave/containerizer/provisioners/appc/paths.hpp 41e3bf79da0854406c488855f953111e67353829 src/slave/containerizer/provisioners/appc/paths.cpp 3113c84b9526dd9e9e89fb9aa4ec75ed66a996c7 src/slave/flags.hpp e56738e2dfd6593ef8f093687919da287af78f77 src/slave/flags.cpp b36710d6d7a7250bc071a57310a2d54bfb3bc624 src/tests/containerizer/appc_provisioner_tests.cpp 47b66b9c30cefe8f9a8e2c1c1341776c2d235020 Diff: https://reviews.apache.org/r/37881/diff/ Testing --- sudo make check. More test cases coming. Thanks, Jiang Yan Xu
Re: Review Request 37903: stout: Fix bug in IPNetwork::create() with zero prefix.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/37903/#review96934 --- Patch looks great! Reviews applied: [37903] All tests passed. - Mesos ReviewBot On Aug. 28, 2015, 8:02 p.m., Neil Conway wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/37903/ --- (Updated Aug. 28, 2015, 8:02 p.m.) Review request for mesos. Bugs: MESOS-3328 https://issues.apache.org/jira/browse/MESOS-3328 Repository: mesos Description --- The previous coding would try to shift a uint32_t value 32 bits to the left; per C++ spec, this yields undefined behavior. On my machine, this resulted in treating a prefix of 0 as equivalent to a prefix of 32, which is obviously wrong. Spotted via ubsan: see MESOS-3328. Diffs - 3rdparty/libprocess/3rdparty/stout/include/stout/ip.hpp 1ad119d54820e97497b1773518875be25ddbf98a 3rdparty/libprocess/3rdparty/stout/tests/ip_tests.cpp b0cbcb38cfcb923ec7c185bacf139ceb0a28924f Diff: https://reviews.apache.org/r/37903/diff/ Testing --- make check Thanks, Neil Conway
Re: Review Request 37325: Maintenance Primitives: Adds an endpoint for scheduling agents for maintenance.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/37325/ --- (Updated Aug. 28, 2015, 2:37 p.m.) Review request for mesos, Benjamin Hindman, Ben Mahler, Artem Harutyunyan, Joris Van Remoortere, and Vinod Kone. Changes --- Significant reworking to reflect earlier commits: * Some code was split into an earlier review. * Plenty of renaming and tweaking to match protobuf changes. * Commenting changes. * Testing helpers moved into main test file. Bugs: MESOS-2067 and MESOS-3069 https://issues.apache.org/jira/browse/MESOS-2067 https://issues.apache.org/jira/browse/MESOS-3069 Repository: mesos Description (updated) --- Endpoint: /maintenance.schedule Registry operation = maintenance::UpdateSchedule Replaces the schedule with the given one. Also sets all scheduled machines into Draining mode. Diffs (updated) - src/Makefile.am 7b620ff66856b3f0adac121b3297d55ed71a3d99 src/master/http.cpp 37d76ee72f6a037f551bf2609e9393e16b496e44 src/master/maintenance.hpp PRE-CREATION src/master/maintenance.cpp PRE-CREATION src/master/master.hpp 36c67599ef2c470da8d95f2caf926a154342d2cc src/master/master.cpp 95207d24db0aa052eb70c4cc7eb75d0611c365cf src/tests/master_maintenance_tests.cpp PRE-CREATION src/tests/registrar_tests.cpp 032e644ee19751b4ce5767d46f474d34ec4b9166 Diff: https://reviews.apache.org/r/37325/diff/ Testing --- `make check` New Tests: RegistrarTest.UpdateMaintenanceSchedule Schedules 3 machines, 1 at a time. Rearranges schedules. Checks that machines are put into Draining mode. Removes machines. MasterMaintenanceTest.UpdateSchedule Hits the new endpoint with some valid and invalid schedules. Only tests a subset of invalid schedules (requires other endpoints to fully test). Thanks, Joseph Wu
Re: Review Request 37900: Maintenance Primitives: Add helper functions for the maintenance protobufs.
---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/37900/#review96912
---
Ship it!
src/master/maintenance.hpp (line 48)
https://reviews.apache.org/r/37900/#comment152599
s/agenda/schedule/
Here AND below.
src/master/maintenance.cpp (line 78)
https://reviews.apache.org/r/37900/#comment152601
s/DEACTIVATED/DOWN/
src/master/maintenance.cpp (line 90)
https://reviews.apache.org/r/37900/#comment152600
s/interval/unavailability/
src/master/maintenance.cpp (line 94)
https://reviews.apache.org/r/37900/#comment152602
Duration::zero() exists, so you can kill this.
src/master/maintenance.cpp (line 95)
https://reviews.apache.org/r/37900/#comment152605
It seems weird to not let start be something before the epoch ... but we
should still validate that the duration is greater than zero (because IIUC
there isn't code that assumes backwards in time maintenance).
src/master/maintenance.cpp (line 99)
https://reviews.apache.org/r/37900/#comment152603
if (start Duration::zero() || duration Duration::zero()) {
src/master/maintenance.cpp (line 100)
https://reviews.apache.org/r/37900/#comment152604
'start' and 'duration'
src/master/maintenance.cpp (line 117)
https://reviews.apache.org/r/37900/#comment152606
Let's do a sweep and clean up all of these comments that no longer apply
please!
src/master/maintenance.cpp (line 140)
https://reviews.apache.org/r/37900/#comment152607
Single quotes not double here.
- Benjamin Hindman
On Aug. 28, 2015, 7:15 p.m., Joseph Wu wrote:
---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/37900/
---
(Updated Aug. 28, 2015, 7:15 p.m.)
Review request for mesos, Benjamin Hindman, Ben Mahler, Artem Harutyunyan,
Joris Van Remoortere, and Vinod Kone.
Repository: mesos
Description
---
This includes helpers for constructing (used in tests) and for validation
(used in HTTP endpoints).
Diffs
-
src/Makefile.am 7b620ff66856b3f0adac121b3297d55ed71a3d99
src/common/protobuf_utils.hpp 63eeb77da5e6020085b9927ce15338aa6ed00488
src/common/protobuf_utils.cpp 6b283558b5ef5c16f1d9f3dc2639df4f75c92f7d
src/master/maintenance.hpp PRE-CREATION
src/master/maintenance.cpp PRE-CREATION
src/master/registrar.hpp c6a0655c212646618d93c9c85918af482a9ffd50
Diff: https://reviews.apache.org/r/37900/diff/
Testing
---
`make check`
Thanks,
Joseph Wu
Re: Review Request 37582: Maintenance Primitives: Add test for the hierarchical DRF allocator sending inverse offers.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/37582/#review96914 --- Ship it! src/tests/hierarchical_allocator_tests.cpp (line 474) https://reviews.apache.org/r/37582/#comment152608 s/uRes/unavailableResources/ - Benjamin Hindman On Aug. 25, 2015, 5:39 p.m., Joseph Wu wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/37582/ --- (Updated Aug. 25, 2015, 5:39 p.m.) Review request for mesos, Benjamin Hindman, Ben Mahler, Artem Harutyunyan, Joris Van Remoortere, and Vinod Kone. Bugs: MESOS-3042 https://issues.apache.org/jira/browse/MESOS-3042 Repository: mesos Description --- Adds a test for the previous review in this chain. Diffs - src/tests/hierarchical_allocator_tests.cpp 9748ca0b3fee25dcec51c64d8ba84dbd4aaf Diff: https://reviews.apache.org/r/37582/diff/ Testing --- `make check` Thanks, Joseph Wu
Re: Review Request 37178: Maintenance Primitives: Added InverseOffers to Scheduler Event Offers.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/37178/#review96915 --- Ship it! Ship It! - Benjamin Hindman On Aug. 26, 2015, 2:12 a.m., Joris Van Remoortere wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/37178/ --- (Updated Aug. 26, 2015, 2:12 a.m.) Review request for mesos, Benjamin Hindman, Artem Harutyunyan, and Joseph Wu. Bugs: MESOS-1474 https://issues.apache.org/jira/browse/MESOS-1474 Repository: mesos Description --- See summary. Diffs - include/mesos/scheduler/scheduler.proto 89daf8a6b74057ee156b3ad691397e76fcb835b8 include/mesos/v1/scheduler/scheduler.proto bd5e82a614b1163b29f9b20e562208efa1ba4b55 Diff: https://reviews.apache.org/r/37178/diff/ Testing --- Thanks, Joris Van Remoortere
Re: Review Request 37282: Maintenance Primitives: Added InverseOffer to V1 API.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/37282/#review96923 --- Ship it! Looks like this stuff is actually needed for https://reviews.apache.org/r/37180. But can we add a comment in ResourcesOffersMessage that explains that we've added the InverseOffer field because that's what we use in master.cpp but we HAVE NOT fully implemented this yet so don't expect it blah blah blah so that people don't go down a rat hole and then get mad at us for stuff not working. Thanks! - Benjamin Hindman On Aug. 26, 2015, 2:12 a.m., Joris Van Remoortere wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/37282/ --- (Updated Aug. 26, 2015, 2:12 a.m.) Review request for mesos, Benjamin Hindman, Artem Harutyunyan, and Joseph Wu. Bugs: MESOS-1474 https://issues.apache.org/jira/browse/MESOS-1474 Repository: mesos Description --- See summary. Diffs - src/internal/devolve.hpp 6e4306df78b9b8d2054e6550209341fd7b0972d6 src/internal/devolve.cpp 0a069e51053b572a8d5dc95380732119504dd0c9 src/internal/evolve.hpp 13e9f52da98567038ec717f394f79e526a1521e9 src/internal/evolve.cpp 11ce9e77490e93f781ceebc33063d13953a11765 src/messages/messages.proto 8977d8e0f3b16003128b6b9cab556a7b224f083c Diff: https://reviews.apache.org/r/37282/diff/ Testing --- Thanks, Joris Van Remoortere
Re: Review Request 37669: Ignore overflow components in version parsing.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/37669/#review96878 --- 3rdparty/libprocess/3rdparty/stout/include/stout/version.hpp (line 59) https://reviews.apache.org/r/37669/#comment152566 allowed and would are not quite right. Suggested phrasing: only maxComponents components will be recognized, the rest will be ignored. - Bernd Mathiske On Aug. 22, 2015, 10:04 a.m., haosdent huang wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/37669/ --- (Updated Aug. 22, 2015, 10:04 a.m.) Review request for mesos, Isabel Jimenez and Timothy Chen. Bugs: MESOS-2986 https://issues.apache.org/jira/browse/MESOS-2986 Repository: mesos Description --- Ignore overflow components in version parsing. Diffs - 3rdparty/libprocess/3rdparty/stout/include/stout/version.hpp 51010845e8885ac52ce88ec0deb6b65a81122cba 3rdparty/libprocess/3rdparty/stout/tests/version_tests.cpp e8f8358f3c113b4e21e30cb5e9d6a3d229191484 Diff: https://reviews.apache.org/r/37669/diff/ Testing --- Thanks, haosdent huang
Re: Review Request 37881: Implemented AppcProvisioner.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/37881/#review96903 --- src/slave/containerizer/provisioner.cpp (line 27) https://reviews.apache.org/r/37881/#comment152581 See my comments below. This is no longer needed. src/slave/containerizer/provisioner.cpp (lines 43 - 46) https://reviews.apache.org/r/37881/#comment152580 I would love to get this TODO solved in this patch. It should be pretty straightfoward, right? Just hard code them. And right now, only Appc is supported. src/slave/containerizer/provisioners/appc.cpp (lines 79 - 97) https://reviews.apache.org/r/37881/#comment152598 Some high level comments. I think it will be beneficial if we can move the fetch and discovery logic to the Store. For the MVP, we don't have to impl. that in the Store. The Store only has one interface, which is the 'get' method. 'get' takes an Image::Appc and returns a vector of paths to layers. THis semantics is more natural and easy to understand: get layers for my image, if some layers are already cached, return them directly, otherwise, discover it and fetch it. The job of the provisioner is to get layers from the Store and stack them into a rootfs using the backend. It also manages the rootfs directories. To me, this is a more natural split of functionalities among components. In the future, we can also potentially unify the impl. of the provisioner (i.e., one single provisioner for both Docker and Appc images). All the image specific details are hiden in the Store interface. Thoughts? src/slave/flags.hpp (line 53) https://reviews.apache.org/r/37881/#comment152583 See my comments below. Why you still need this flag? src/slave/flags.cpp (lines 72 - 76) https://reviews.apache.org/r/37881/#comment152582 Why do you still need this flag? - Jie Yu On Aug. 28, 2015, 9:04 a.m., Jiang Yan Xu wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/37881/ --- (Updated Aug. 28, 2015, 9:04 a.m.) Review request for mesos, Jie Yu and Timothy Chen. Bugs: MESOS-2796 https://issues.apache.org/jira/browse/MESOS-2796 Repository: mesos Description --- Implemented AppcProvisioner. Diffs - src/Makefile.am 7b620ff66856b3f0adac121b3297d55ed71a3d99 src/slave/containerizer/provisioner.cpp efc7e6996ff6663bebaf61989a7e040bd2ad7a5e src/slave/containerizer/provisioners/appc.hpp PRE-CREATION src/slave/containerizer/provisioners/appc.cpp PRE-CREATION src/slave/containerizer/provisioners/appc/paths.hpp 41e3bf79da0854406c488855f953111e67353829 src/slave/containerizer/provisioners/appc/paths.cpp 3113c84b9526dd9e9e89fb9aa4ec75ed66a996c7 src/slave/flags.hpp e56738e2dfd6593ef8f093687919da287af78f77 src/slave/flags.cpp b36710d6d7a7250bc071a57310a2d54bfb3bc624 src/tests/containerizer/appc_provisioner_tests.cpp 47b66b9c30cefe8f9a8e2c1c1341776c2d235020 Diff: https://reviews.apache.org/r/37881/diff/ Testing --- sudo make check. More test cases coming. Thanks, Jiang Yan Xu
Review Request 37914: Updated the ReviewBot to flag reviews that do not contain reviewers.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/37914/ --- Review request for mesos, Benjamin Hindman and Michael Park. Repository: mesos Description --- I'm seeing more and more reviews being sent without tagging reviewers. This should flag such reviews to guide new contributors. Diffs - support/verify_reviews.py b85a3245b2f248dbdb8e2783f2384092e3d53031 Diff: https://reviews.apache.org/r/37914/diff/ Testing --- Tested locally ./support/verify_reviews.py vinod kone 10 ?from-user=neilc git rev-parse HEAD Checking if review: 37445 needs verification Latest diff timestamp: 2015-08-13 21:01:08 Verifying review 37445 Posting review: Bad patch! Reviews applied: [] Error: No reviewers specified. Please find a reviewer by asking on JIRA or the mailing list. Thanks, Vinod Kone
