Re: Review Request 36041: The configure phase breaks with the IBM JVM.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36041/ --- (Updated July 16, 2015, 1:54 a.m.) Review request for mesos. Bugs: MESOS-2216 https://issues.apache.org/jira/browse/MESOS-2216 Repository: mesos Description --- The configure phase breaks with the IBM JVM. Diffs - configure.ac 92909580ea735a1ccffbe03a2eb44e3d07566488 Diff: https://reviews.apache.org/r/36041/diff/ Testing --- Thanks, Jihun Kang
Re: Review Request 36041: The configure phase breaks with the IBM JVM.
On July 9, 2015, 7:11 a.m., Till Toenshoff wrote: Do we have a JIRA issue that would cover this issue? If so, please add it to this review-request within the Bugs section. If there is no JIRA, please create one that describes the problem briefly. Next question; once the configuration phase is patched like this, do the tests then fully work using the IBM JVM? In other words, right now I don't know if this patch is sufficient to fully support the IBM JVM. Sorry for late response. I will update a JIRA issue. On the testing, I already tested IBM JVM 1.6 and 1.7, and you can also download IBM JDK for linux from the following link. http://www.ibm.com/developerworks/java/jdk/linux/download.html - Jihun --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36041/#review91071 --- On June 30, 2015, 9:09 a.m., Jihun Kang wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36041/ --- (Updated June 30, 2015, 9:09 a.m.) Review request for mesos. Repository: mesos Description --- The configure phase breaks with the IBM JVM. Diffs - configure.ac 92909580ea735a1ccffbe03a2eb44e3d07566488 Diff: https://reviews.apache.org/r/36041/diff/ Testing --- Thanks, Jihun Kang
Re: Review Request 36501: MESOS-3023
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36501/ --- (Updated July 16, 2015, 5:47 a.m.) Review request for mesos. Bugs: MESOS-3023 https://issues.apache.org/jira/browse/MESOS-3023 Repository: mesos Description --- Fix for MESOS-3023 (Factoring out the pattern for URL generation) Diffs (updated) - src/tests/fetcher_tests.cpp ae10c42 src/tests/utils.hpp f2eed2e Diff: https://reviews.apache.org/r/36501/diff/ Testing --- 1. Build successfully in Linux 2. Test passed by src/mesos-tests --gtest_filter=FetcherTest.OSNetUriTest Thanks, Klaus Ma
Re: Review Request 36501: MESOS-3023
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36501/ --- (Updated July 16, 2015, 4:41 a.m.) Review request for mesos. Bugs: MESOS-3023 https://issues.apache.org/jira/browse/MESOS-3023 Repository: mesos Description --- Fix for MESOS-3023 (Factoring out the pattern for URL generation) Diffs (updated) - src/tests/fetcher_tests.cpp ae10c42 src/tests/utils.hpp f2eed2e Diff: https://reviews.apache.org/r/36501/diff/ Testing --- 1. Build successfully in Linux 2. Test passed by src/mesos-tests --gtest_filter=FetcherTest.OSNetUriTest Thanks, Klaus Ma
Re: Review Request 36501: MESOS-3023
On July 15, 2015, 9:19 p.m., Joseph Wu wrote:
src/tests/utils.hpp, line 55
https://reviews.apache.org/r/36501/diff/1/?file=1012134#file1012134line55
* Tab size = 2 spaces.
* Parameters are indented by 4 spaces.
* Comments start with a capital letter and end with a period.
* Logical blocks have the opening { on the same line.
Thanks very much for your input :). The code diff has been updated.
- Klaus
---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/36501/#review91823
---
On July 16, 2015, 4:41 a.m., Klaus Ma wrote:
---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/36501/
---
(Updated July 16, 2015, 4:41 a.m.)
Review request for mesos.
Bugs: MESOS-3023
https://issues.apache.org/jira/browse/MESOS-3023
Repository: mesos
Description
---
Fix for MESOS-3023 (Factoring out the pattern for URL generation)
Diffs
-
src/tests/fetcher_tests.cpp ae10c42
src/tests/utils.hpp f2eed2e
Diff: https://reviews.apache.org/r/36501/diff/
Testing
---
1. Build successfully in Linux
2. Test passed by src/mesos-tests --gtest_filter=FetcherTest.OSNetUriTest
Thanks,
Klaus Ma
Re: Review Request 36501: MESOS-3023
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36501/#review91863 --- Patch looks great! Reviews applied: [36501] All tests passed. - Mesos ReviewBot On July 16, 2015, 4:41 a.m., Klaus Ma wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36501/ --- (Updated July 16, 2015, 4:41 a.m.) Review request for mesos. Bugs: MESOS-3023 https://issues.apache.org/jira/browse/MESOS-3023 Repository: mesos Description --- Fix for MESOS-3023 (Factoring out the pattern for URL generation) Diffs - src/tests/fetcher_tests.cpp ae10c42 src/tests/utils.hpp f2eed2e Diff: https://reviews.apache.org/r/36501/diff/ Testing --- 1. Build successfully in Linux 2. Test passed by src/mesos-tests --gtest_filter=FetcherTest.OSNetUriTest Thanks, Klaus Ma
Re: Review Request 34137: Add support for container image provisioners.
On July 14, 2015, 7:41 p.m., Vinod Kone wrote: src/slave/containerizer/provisioner.cpp, lines 43-56 https://reviews.apache.org/r/34137/diff/2-3/?file=989758#file989758line43 Why do you need foreach loop here if you were going to return error anyway? We need the foreach to go over all the provisioners though, as there could be more than one although there is one listed for now. - Timothy --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/34137/#review91662 --- On July 12, 2015, 4:47 a.m., Ian Downes wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/34137/ --- (Updated July 12, 2015, 4:47 a.m.) Review request for mesos, Chi Zhang, Paul Brett, Timothy Chen, and Vinod Kone. Repository: mesos Description --- The MesosContainerizer can optionally provision a root filesystem for the containers. Diffs - include/mesos/slave/isolator.hpp ef2205dd7f4619e53e6cca7cac301f874d08c036 src/Makefile.am e5b5d36f0ac160e5a3a9fdc50b31c060a413ce2c src/slave/containerizer/mesos/containerizer.hpp 3ac2387eabded61c897a5016655aae10cd1bca91 src/slave/containerizer/mesos/containerizer.cpp 47d146125dfd4ea909e7ec9d94f41cfa11d035e5 src/slave/containerizer/provisioner.hpp PRE-CREATION src/slave/containerizer/provisioner.cpp PRE-CREATION src/slave/flags.hpp 26c778db2303167369af8675fe0441a00a1e9151 src/slave/flags.cpp 8632677ebbdbfef8ffa45204b6f63a700baff7f3 src/slave/paths.hpp 00476f107ed8233123a6eab0925c9f28aac0a86f src/slave/paths.cpp 0741616b656e947cb460dd6ee6a9a4852be001c2 src/slave/state.hpp bb0eee78e118210ccd7fcbd909029412ff106128 src/slave/state.cpp f8a9514f52bf9f886171c2a0e674e5a89f8dbea7 src/tests/containerizer_tests.cpp 0cdb2d2a3f19a4835e85c6b040759019b03f051e Diff: https://reviews.apache.org/r/34137/diff/ Testing --- Thanks, Ian Downes
Re: Review Request 34137: Add support for container image provisioners.
On July 14, 2015, 7:41 p.m., Vinod Kone wrote: can you fix the depends on please? I think the two listed are the correct dependencies for this rb? - Timothy --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/34137/#review91662 --- On July 12, 2015, 4:47 a.m., Ian Downes wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/34137/ --- (Updated July 12, 2015, 4:47 a.m.) Review request for mesos, Chi Zhang, Paul Brett, Timothy Chen, and Vinod Kone. Repository: mesos Description --- The MesosContainerizer can optionally provision a root filesystem for the containers. Diffs - include/mesos/slave/isolator.hpp ef2205dd7f4619e53e6cca7cac301f874d08c036 src/Makefile.am e5b5d36f0ac160e5a3a9fdc50b31c060a413ce2c src/slave/containerizer/mesos/containerizer.hpp 3ac2387eabded61c897a5016655aae10cd1bca91 src/slave/containerizer/mesos/containerizer.cpp 47d146125dfd4ea909e7ec9d94f41cfa11d035e5 src/slave/containerizer/provisioner.hpp PRE-CREATION src/slave/containerizer/provisioner.cpp PRE-CREATION src/slave/flags.hpp 26c778db2303167369af8675fe0441a00a1e9151 src/slave/flags.cpp 8632677ebbdbfef8ffa45204b6f63a700baff7f3 src/slave/paths.hpp 00476f107ed8233123a6eab0925c9f28aac0a86f src/slave/paths.cpp 0741616b656e947cb460dd6ee6a9a4852be001c2 src/slave/state.hpp bb0eee78e118210ccd7fcbd909029412ff106128 src/slave/state.cpp f8a9514f52bf9f886171c2a0e674e5a89f8dbea7 src/tests/containerizer_tests.cpp 0cdb2d2a3f19a4835e85c6b040759019b03f051e Diff: https://reviews.apache.org/r/34137/diff/ Testing --- Thanks, Ian Downes
Re: Review Request 36402: Adding 'Accept' header in request
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36402/#review91859 --- Patch looks great! Reviews applied: [36402] All tests passed. - Mesos ReviewBot On July 15, 2015, 11:54 p.m., Isabel Jimenez wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36402/ --- (Updated July 15, 2015, 11:54 p.m.) Review request for mesos, Anand Mazumdar, Ben Mahler, Marco Massenzio, and Vinod Kone. Repository: mesos-incubating Description --- Adding a method for Accept header in request + refactor of Accept-Encoding Diffs - 3rdparty/libprocess/include/process/http.hpp 72b6d27 3rdparty/libprocess/src/http.cpp d168579 3rdparty/libprocess/src/tests/http_tests.cpp 01f243c Diff: https://reviews.apache.org/r/36402/diff/ Testing --- make check Thanks, Isabel Jimenez
Re: Review Request 36389: Enable remote execution of arbitrary command.
---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/36389/#review91723
---
src/slave/flags.hpp (line 117)
https://reviews.apache.org/r/36389/#comment145304
Just for my own education purpose :-), Is there a reason for a newline here?
src/slave/slave.cpp (line 151)
https://reviews.apache.org/r/36389/#comment145297
Do you think it makes sense to capture the default value in a single
location? Otherwise it's hardcoded in two places (here and in flags.cpp)
src/slave/slave.cpp (lines 4691 - 4694)
https://reviews.apache.org/r/36389/#comment145310
wouldn't const std::vectorstring args(command.arguments()); work here?
src/slave/slave.cpp (line 4698)
https://reviews.apache.org/r/36389/#comment145298
If you make this review dependent on /r/36424 then RB should be smart
enough to apply the patch from there before apllying and building this one.
That should eliminate the need of having a temporary code in the review.
src/slave/slave.cpp (line 4702)
https://reviews.apache.org/r/36389/#comment145311
if we are using command.value() here we might as well use command.args()
and ditch the args variable altogether.
src/slave/slave.cpp (line 4743)
https://reviews.apache.org/r/36389/#comment145302
Is it OK to return HTTP 200 if the command returned a non-zero error code?
In the statements above, for exaple, the error code is returned if the
command execution failed (albeit, the reasons for a failure are different).
This approach makes the HTTP 200 insufficient for verifying successful
execution of the command.
src/slave/slave.cpp (line 4755)
https://reviews.apache.org/r/36389/#comment145299
Shouldn't there be an equivalent of an assert here if we never expect this
to happen? Something like this:
if (response.isReady()) {
ASSERT
}
return http::BadRequest ...
Unless, there is possibility of a race where the result becomes ready right
at the 15th second (in a separate thread) and by the time this lambda is
executed the response becomes actually (and legitimately) ready.
- Artem Harutyunyan
On July 14, 2015, 4:20 p.m., Marco Massenzio wrote:
---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/36389/
---
(Updated July 14, 2015, 4:20 p.m.)
Review request for mesos, Benjamin Hindman and Cody Maloney.
Bugs: MESOS-2830
https://issues.apache.org/jira/browse/MESOS-2830
Repository: mesos
Description
---
Jira: MESOS-2830
Under certain (maintenance) circumnstance, it may be necessary
(or desirable) to execute arbitrary operator's commands on the
slave (the entire fleet or a subset thereof) bypassing the Mesos
Task execution mechanism; this may typically be necessary for
maintenance and/or emergency actions.
This patch adds an HTTP endpoint (/execute) which accepts a
JSON-encoded CommandInfo structure and executes the given
command (with optional arguments).
The output of the command (along with potentially any stderr
messages) is returned JSON-encoded in the Response.
For more details, see the design doc at:
https://goo.gl/4npTMU
Diffs
-
src/slave/flags.hpp 26c778db2303167369af8675fe0441a00a1e9151
src/slave/flags.cpp 8632677ebbdbfef8ffa45204b6f63a700baff7f3
src/slave/main.cpp 8008430e98773d8be9ba5ac6385cffb2e351932a
src/slave/slave.hpp dec4ca8323e151a6d0f9139214ff0ef6e3e3375a
src/slave/slave.cpp 2119b5176aa7cfb7b0b551d4d4f65ee12818b9e4
Diff: https://reviews.apache.org/r/36389/diff/
Testing
---
make check
lots of manual testing (using Postman, REST client)
Thanks,
Marco Massenzio
Re: Review Request 36510: Update .gitignore with intermediate build artifacts.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36510/#review91751 --- Ship it! im assuming you are not building inside build directory? - Vinod Kone On July 15, 2015, 4:11 p.m., James Peach wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36510/ --- (Updated July 15, 2015, 4:11 p.m.) Review request for mesos and Benjamin Hindman. Bugs: MESOS-3054 https://issues.apache.org/jira/browse/MESOS-3054 Repository: mesos Description --- Update .gitignore with intermediate build artifacts. Diffs - .gitignore-template 934cad71a370d5c64d9a0994f775f39db74f5851 Diff: https://reviews.apache.org/r/36510/diff/ Testing --- In a dirty build tree, run ```git status``` and note the reduced clutter. Thanks, James Peach
Re: Review Request 36424: Created a command executor helper method.
On July 13, 2015, 5:20 p.m., Paul Brett wrote: a drive by comment suggested by Joris is inline really necessary here? On July 13, 2015, 5:20 p.m., Paul Brett wrote: 3rdparty/libprocess/include/process/subprocess.hpp, line 307 https://reviews.apache.org/r/36424/diff/1/?file=1008891#file1008891line307 How about returning a tuple/struct of stdout, stderr and return code and let the caller decide what they want? Marco Massenzio wrote: sure, that would be a possibility too, but it seems to me that the approved way in Mesos is to return a `Try` for when something *may* go wrong. This is consistent across the entire code base. Maybe I am missing something, I did a grep for `FutureTry..` and could not find any occurence of it in the code base. Perhaps the reason is that it's customary to use Future's `Failure()` to indicate an error (as opposed to returning a `Try`). If anything `Result` would probably be more appropriate here than Try, but I'd like to hear what a shepherd has to say. The function could just return `Futurestd::string` and you could use `Failure()` to indicate the error. In that case you'll need to change the return type of `.then` lamda to `Futurestd::string` and also to replace a `return Error(...` on line 346 with `return Failure(...` (which you might want to do anyway for the sake of consistency). - Artem --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36424/#review91566 --- On July 13, 2015, 9:21 p.m., Marco Massenzio wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36424/ --- (Updated July 13, 2015, 9:21 p.m.) Review request for mesos, Benjamin Hindman and Cody Maloney. Bugs: MESOS-3035 https://issues.apache.org/jira/browse/MESOS-3035 Repository: mesos Description --- Jira: MESOS-2902 While researching how to execute an arbitrary script to detect the Master IP address, it emerged clearly that a helper method to execute an arbitrary command/script on a node and obtain either stdout or stderr would have been useful and avoided a lot of code repetition. This could not be ultimately used for the purpose at hand, but I believe it to be useful enough (particularly, to avoid people doing coding by copypaste and/or waste time researching the same functionality). This would also be beneficial in MESOS-2830, factoring out the remote command execution logic. Diffs - 3rdparty/libprocess/include/process/subprocess.hpp 310cb4f8e4e2faa5545dffd196d7490c868bc5d6 3rdparty/libprocess/src/tests/subprocess_tests.cpp f6acb204582a9e696c3b09d4e4c543bb052e97d4 Diff: https://reviews.apache.org/r/36424/diff/ Testing --- make check Thanks, Marco Massenzio
Re: Review Request 36510: Update .gitignore with intermediate build artifacts.
On July 15, 2015, 5:04 p.m., Vinod Kone wrote: im assuming you are not building inside build directory? Yup. I just found out today that the ```build``` directory was conventional :) - James --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36510/#review91751 --- On July 15, 2015, 4:11 p.m., James Peach wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36510/ --- (Updated July 15, 2015, 4:11 p.m.) Review request for mesos and Benjamin Hindman. Bugs: MESOS-3054 https://issues.apache.org/jira/browse/MESOS-3054 Repository: mesos Description --- Update .gitignore with intermediate build artifacts. Diffs - .gitignore-template 934cad71a370d5c64d9a0994f775f39db74f5851 Diff: https://reviews.apache.org/r/36510/diff/ Testing --- In a dirty build tree, run ```git status``` and note the reduced clutter. Thanks, James Peach
Re: Review Request 36464: Updated scheduler driver to send ACCEPT call.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36464/#review91764 --- Ship it! Looks good but mind removing the deprecated code in a different review? src/sched/sched.cpp (line 922) https://reviews.apache.org/r/36464/#comment145395 add a space between foreach and the open paren src/sched/sched.cpp https://reviews.apache.org/r/36464/#comment145394 Mind pulling this patch out into a separate review? Seems independent :) src/tests/master_tests.cpp (lines 2566 - 2567) https://reviews.apache.org/r/36464/#comment145393 Is the mesos:: prefix needed? - Ben Mahler On July 13, 2015, 11:59 p.m., Vinod Kone wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36464/ --- (Updated July 13, 2015, 11:59 p.m.) Review request for mesos and Ben Mahler. Bugs: MESOS-2913 https://issues.apache.org/jira/browse/MESOS-2913 Repository: mesos Description --- See summary. Diffs - src/sched/sched.cpp a748686dfc6bff39d81fd7adbd5cce88ddaaa73d src/tests/master_tests.cpp 57721b788d0c70f4c6f5cc44d87465f52a70b6c2 Diff: https://reviews.apache.org/r/36464/diff/ Testing --- make check Thanks, Vinod Kone
Re: Review Request 36469: Updated scheduler driver to send MESSAGE call.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36469/#review91770 --- Ship it! src/sched/sched.cpp (line 1132) https://reviews.apache.org/r/36469/#comment145414 Ditto newline comment from other reviews. src/tests/fault_tolerance_tests.cpp (line 1265) https://reviews.apache.org/r/36469/#comment145418 Did you want to expect that the message is sent to through the master using call, since it looks like no offers go to the second scheduler? src/tests/fault_tolerance_tests.cpp (lines 1313 - 1315) https://reviews.apache.org/r/36469/#comment145415 We don't need to worry about gcc 4.1.* anymore, you can assign now on the same line :) - Ben Mahler On July 14, 2015, 12:30 a.m., Vinod Kone wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36469/ --- (Updated July 14, 2015, 12:30 a.m.) Review request for mesos and Ben Mahler. Bugs: MESOS-2913 https://issues.apache.org/jira/browse/MESOS-2913 Repository: mesos Description --- See summary. Diffs - src/sched/sched.cpp a748686dfc6bff39d81fd7adbd5cce88ddaaa73d src/tests/fault_tolerance_tests.cpp 1070ccf17f98f6b3800684a5edd6517d90631c3e Diff: https://reviews.apache.org/r/36469/diff/ Testing --- make check Thanks, Vinod Kone
Re: Review Request 36466: Updated scheduler driver to send RECONCILE call.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36466/#review91766 --- Ship it! src/sched/sched.cpp (line 1135) https://reviews.apache.org/r/36466/#comment145399 newline before like your other reviews? Seems like setting type should be in a different block than setting the framework id, more related to the line below where you grab a 'Reconcile' from the call src/tests/reconciliation_tests.cpp (lines 451 - 452) https://reviews.apache.org/r/36466/#comment145402 Is mesos:: needed? src/tests/reconciliation_tests.cpp (lines 592 - 593) https://reviews.apache.org/r/36466/#comment145403 Is mesos:: needed? - Ben Mahler On July 14, 2015, 12:01 a.m., Vinod Kone wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36466/ --- (Updated July 14, 2015, 12:01 a.m.) Review request for mesos and Ben Mahler. Bugs: MESOS-2913 https://issues.apache.org/jira/browse/MESOS-2913 Repository: mesos Description --- See summary. Diffs - src/sched/sched.cpp a748686dfc6bff39d81fd7adbd5cce88ddaaa73d src/tests/reconciliation_tests.cpp 6042d8c02d86f486e0c4d82d5a70666d7ac9019b Diff: https://reviews.apache.org/r/36466/diff/ Testing --- make check Thanks, Vinod Kone
Re: Review Request 36465: Updated scheduler driver to send REVIVE call.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36465/#review91765 --- src/sched/sched.cpp (line 1029) https://reviews.apache.org/r/36465/#comment145398 newline before setting the type, like your other reviews? - Ben Mahler On July 14, 2015, midnight, Vinod Kone wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36465/ --- (Updated July 14, 2015, midnight) Review request for mesos and Ben Mahler. Bugs: MESOS-2913 https://issues.apache.org/jira/browse/MESOS-2913 Repository: mesos Description --- See summary. Diffs - src/sched/sched.cpp a748686dfc6bff39d81fd7adbd5cce88ddaaa73d Diff: https://reviews.apache.org/r/36465/diff/ Testing --- make check (NOTE: There is already an existing test that tests the revive workflow, but it didn't need any update) Thanks, Vinod Kone
Re: Review Request 36463: Updated scheduler driver to send Kill Call.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36463/#review91762 --- src/sched/sched.cpp (lines 890 - 892) https://reviews.apache.org/r/36463/#comment145397 Maybe a little comment that we aren't provided the slave id but that's ok? src/sched/sched.cpp (line 891) https://reviews.apache.org/r/36463/#comment145386 Newline here or don't bother storing kill? src/tests/fault_tolerance_tests.cpp (lines 1359 - 1360) https://reviews.apache.org/r/36463/#comment145388 Ditto here, do we need the mesos:: prefix? src/tests/master_tests.cpp (lines 545 - 547) https://reviews.apache.org/r/36463/#comment145387 Hm.. is the mesos:: prefix needed? Looks like we can remove this comment, given it's not in the other test, and it just describes the line of code, yeah? - Ben Mahler On July 13, 2015, 11:58 p.m., Vinod Kone wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36463/ --- (Updated July 13, 2015, 11:58 p.m.) Review request for mesos and Ben Mahler. Bugs: MESOS-2913 https://issues.apache.org/jira/browse/MESOS-2913 Repository: mesos Description --- See summary. Diffs - src/sched/sched.cpp a748686dfc6bff39d81fd7adbd5cce88ddaaa73d src/tests/fault_tolerance_tests.cpp 1070ccf17f98f6b3800684a5edd6517d90631c3e src/tests/master_tests.cpp 57721b788d0c70f4c6f5cc44d87465f52a70b6c2 Diff: https://reviews.apache.org/r/36463/diff/ Testing --- make check Thanks, Vinod Kone
Re: Review Request 36470: Updated scheduler driver to send TEARDOWN call.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36470/#review91773 --- src/sched/sched.cpp (line 852) https://reviews.apache.org/r/36470/#comment145419 Ditto newline comment. src/tests/exception_tests.cpp (lines 195 - 196) https://reviews.apache.org/r/36470/#comment145421 Is mesos:: needed? src/tests/master_allocator_tests.cpp (line 445) https://reviews.apache.org/r/36470/#comment145422 Is mesos:: needed? src/tests/master_tests.cpp (lines 246 - 247) https://reviews.apache.org/r/36470/#comment145423 Is mesos:: needed? src/tests/master_tests.cpp (line 249) https://reviews.apache.org/r/36470/#comment145424 We used to write silly comments.. ;) src/tests/slave_recovery_tests.cpp (lines 2154 - 2155) https://reviews.apache.org/r/36470/#comment145426 Is mesos:: needed? - Ben Mahler On July 14, 2015, 12:30 a.m., Vinod Kone wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36470/ --- (Updated July 14, 2015, 12:30 a.m.) Review request for mesos and Ben Mahler. Bugs: MESOS-2913 https://issues.apache.org/jira/browse/MESOS-2913 Repository: mesos Description --- See summary. Diffs - src/sched/sched.cpp a748686dfc6bff39d81fd7adbd5cce88ddaaa73d src/tests/exception_tests.cpp 9af16748ae67671bd327a1ea7c946a7a38c5f7ff src/tests/master_allocator_tests.cpp 534b2486a6a02ef32b5a7235d3ad30e82a78d6a5 src/tests/master_tests.cpp 57721b788d0c70f4c6f5cc44d87465f52a70b6c2 src/tests/rate_limiting_tests.cpp 49d907b0be45ccfed8af5c8fda89ad560e012c1e src/tests/slave_recovery_tests.cpp 2f882cf7b4235583b0ec8397cfcbbc02930fbc88 Diff: https://reviews.apache.org/r/36470/diff/ Testing --- make check Thanks, Vinod Kone
Re: Review Request 36467: Updated scheduler driver to send ACKNOWLEDGE call.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36467/#review91767 --- Ship it! src/sched/sched.cpp (line 745) https://reviews.apache.org/r/36467/#comment145404 Ditto other reviews, mind adding a newline here? Seems unrelated to setting framework id. src/sched/sched.cpp https://reviews.apache.org/r/36467/#comment145405 Whoops, there is a call to master.get() in the VLOG line, that's why it was up here. Mind moving it back up? src/sched/sched.cpp (line 1076) https://reviews.apache.org/r/36467/#comment145406 Ditto adding a newline. src/tests/fault_tolerance_tests.cpp (lines 1299 - 1300) https://reviews.apache.org/r/36467/#comment145407 Is mesos:: needed? src/tests/reconciliation_tests.cpp (lines 748 - 751) https://reviews.apache.org/r/36467/#comment145408 Is mesos:: needed? src/tests/scheduler_tests.cpp (lines 1033 - 1037) https://reviews.apache.org/r/36467/#comment145409 Is mesos:: needed? Ditto for the rest of this file. src/tests/slave_recovery_tests.cpp (lines 216 - 217) https://reviews.apache.org/r/36467/#comment145411 Is mesos:: needed? src/tests/slave_recovery_tests.cpp (lines 318 - 319) https://reviews.apache.org/r/36467/#comment145412 Shall we store the 'uuid' in a variable to make this a bit easier to read? src/tests/status_update_manager_tests.cpp (lines 423 - 426) https://reviews.apache.org/r/36467/#comment145413 Is mesos:: needed? - Ben Mahler On July 14, 2015, 12:18 a.m., Vinod Kone wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36467/ --- (Updated July 14, 2015, 12:18 a.m.) Review request for mesos and Ben Mahler. Bugs: MESOS-2913 https://issues.apache.org/jira/browse/MESOS-2913 Repository: mesos Description --- See summary. Diffs - src/sched/sched.cpp a748686dfc6bff39d81fd7adbd5cce88ddaaa73d src/tests/fault_tolerance_tests.cpp 1070ccf17f98f6b3800684a5edd6517d90631c3e src/tests/reconciliation_tests.cpp 6042d8c02d86f486e0c4d82d5a70666d7ac9019b src/tests/scheduler_tests.cpp 946fa8245d8ab35e04bad642d69114caf0ccf6a9 src/tests/slave_recovery_tests.cpp 2f882cf7b4235583b0ec8397cfcbbc02930fbc88 src/tests/slave_tests.cpp 4ddc608ab9636fcc0166e8c80a252dcf67b45ad3 src/tests/status_update_manager_tests.cpp 440b07475e28dc491ab640acaca8ee20db8411f8 Diff: https://reviews.apache.org/r/36467/diff/ Testing --- make check Thanks, Vinod Kone
Re: Review Request 36463: Updated scheduler driver to send Kill Call.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36463/#review91779 --- Ship it! Ship It! - Ben Mahler On July 13, 2015, 11:58 p.m., Vinod Kone wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36463/ --- (Updated July 13, 2015, 11:58 p.m.) Review request for mesos and Ben Mahler. Bugs: MESOS-2913 https://issues.apache.org/jira/browse/MESOS-2913 Repository: mesos Description --- See summary. Diffs - src/sched/sched.cpp a748686dfc6bff39d81fd7adbd5cce88ddaaa73d src/tests/fault_tolerance_tests.cpp 1070ccf17f98f6b3800684a5edd6517d90631c3e src/tests/master_tests.cpp 57721b788d0c70f4c6f5cc44d87465f52a70b6c2 Diff: https://reviews.apache.org/r/36463/diff/ Testing --- make check Thanks, Vinod Kone
Re: Review Request 36470: Updated scheduler driver to send TEARDOWN call.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36470/#review91777 --- Ship it! Ship It! - Ben Mahler On July 14, 2015, 12:30 a.m., Vinod Kone wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36470/ --- (Updated July 14, 2015, 12:30 a.m.) Review request for mesos and Ben Mahler. Bugs: MESOS-2913 https://issues.apache.org/jira/browse/MESOS-2913 Repository: mesos Description --- See summary. Diffs - src/sched/sched.cpp a748686dfc6bff39d81fd7adbd5cce88ddaaa73d src/tests/exception_tests.cpp 9af16748ae67671bd327a1ea7c946a7a38c5f7ff src/tests/master_allocator_tests.cpp 534b2486a6a02ef32b5a7235d3ad30e82a78d6a5 src/tests/master_tests.cpp 57721b788d0c70f4c6f5cc44d87465f52a70b6c2 src/tests/rate_limiting_tests.cpp 49d907b0be45ccfed8af5c8fda89ad560e012c1e src/tests/slave_recovery_tests.cpp 2f882cf7b4235583b0ec8397cfcbbc02930fbc88 Diff: https://reviews.apache.org/r/36470/diff/ Testing --- make check Thanks, Vinod Kone
Re: Review Request 36461: Added FutureMessageType, DropMessagesType and ExpectNoFutureMessagesType.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36461/#review91757 --- It looks like this could benefit from a bit of documentation that mentions the protobuf [union technique](https://developers.google.com/protocol-buffers/docs/techniques#union). 3rdparty/libprocess/include/process/gmock.hpp (line 319) https://reviews.apache.org/r/36461/#comment145380 Anything preventing s/t/type/ ? Type in this context is a bit confusing since it sounds like the message type. Can we call this something like 'UnionMessageMatcher' and avoid using the overloaded word type? 3rdparty/libprocess/include/process/gmock.hpp (line 322) https://reviews.apache.org/r/36461/#comment145381 Anything preventing s/n/message/? - Ben Mahler On July 13, 2015, 11:55 p.m., Vinod Kone wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36461/ --- (Updated July 13, 2015, 11:55 p.m.) Review request for mesos and Ben Mahler. Bugs: MESOS-2913 https://issues.apache.org/jira/browse/MESOS-2913 Repository: mesos Description --- Needed these abstractions for capturing specific CALLs explicitly in subsequent reviews. Diffs - 3rdparty/libprocess/include/process/gmock.hpp 0fd3f8cf06c9efd357fa7c933cc28d527855ac9a Diff: https://reviews.apache.org/r/36461/diff/ Testing --- Tested in subsequent reviews. Thanks, Vinod Kone
Re: Review Request 36465: Updated scheduler driver to send REVIVE call.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36465/#review91778 --- Ship it! Ship It! - Ben Mahler On July 14, 2015, midnight, Vinod Kone wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36465/ --- (Updated July 14, 2015, midnight) Review request for mesos and Ben Mahler. Bugs: MESOS-2913 https://issues.apache.org/jira/browse/MESOS-2913 Repository: mesos Description --- See summary. Diffs - src/sched/sched.cpp a748686dfc6bff39d81fd7adbd5cce88ddaaa73d Diff: https://reviews.apache.org/r/36465/diff/ Testing --- make check (NOTE: There is already an existing test that tests the revive workflow, but it didn't need any update) Thanks, Vinod Kone
Re: Review Request 36389: Enable remote execution of arbitrary command.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36389/#review91784 --- To make sure that this is fool proof(ish), I would suggest that this should only ship only when the Authorizer framework (mentioned in the TODO comment) becomes available. Also, I would add a screaming comment to --usage, something along the lines of 'this is insecure, and this enables arbitrary command executrion with root privileges'. In general, I am of the firm opinion that this feature should come with a whitelisting mechanism that will allow operators to whitelist (benign) commands that they want to execute, and forbid anything else. - Artem Harutyunyan On July 14, 2015, 4:20 p.m., Marco Massenzio wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36389/ --- (Updated July 14, 2015, 4:20 p.m.) Review request for mesos, Benjamin Hindman and Cody Maloney. Bugs: MESOS-2830 https://issues.apache.org/jira/browse/MESOS-2830 Repository: mesos Description --- Jira: MESOS-2830 Under certain (maintenance) circumnstance, it may be necessary (or desirable) to execute arbitrary operator's commands on the slave (the entire fleet or a subset thereof) bypassing the Mesos Task execution mechanism; this may typically be necessary for maintenance and/or emergency actions. This patch adds an HTTP endpoint (/execute) which accepts a JSON-encoded CommandInfo structure and executes the given command (with optional arguments). The output of the command (along with potentially any stderr messages) is returned JSON-encoded in the Response. For more details, see the design doc at: https://goo.gl/4npTMU Diffs - src/slave/flags.hpp 26c778db2303167369af8675fe0441a00a1e9151 src/slave/flags.cpp 8632677ebbdbfef8ffa45204b6f63a700baff7f3 src/slave/main.cpp 8008430e98773d8be9ba5ac6385cffb2e351932a src/slave/slave.hpp dec4ca8323e151a6d0f9139214ff0ef6e3e3375a src/slave/slave.cpp 2119b5176aa7cfb7b0b551d4d4f65ee12818b9e4 Diff: https://reviews.apache.org/r/36389/diff/ Testing --- make check lots of manual testing (using Postman, REST client) Thanks, Marco Massenzio
Review Request 36517: Disable SharedFilesystemIsolator tests.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36517/ --- Review request for mesos, Adam B and Jie Yu. Bugs: MESOS-3050 https://issues.apache.org/jira/browse/MESOS-3050 Repository: mesos Description --- Disable SharedFilesystemIsolator tests. Diffs - src/tests/isolator_tests.cpp 8eb732fe90dbab11967e663a5c06890f79463488 Diff: https://reviews.apache.org/r/36517/diff/ Testing --- make check Thanks, Timothy Chen
Re: Review Request 36462: Added FUTURE_CALL, DROP_CALL, DROP_CALLS and EXPECT_NO_FUTURE_CALLS.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36462/#review91760 --- src/tests/mesos.hpp (lines 1328 - 1332) https://reviews.apache.org/r/36462/#comment145385 Mind referencing the protobuf union technique here when mentioning re-use? src/tests/mesos.hpp (line 1418) https://reviews.apache.org/r/36462/#comment145384 Comparing this with ExpectNoFutureProtobufs, it's a bit confusing that 'T' above represents the message type and 'T' here represents the .type() value to match. Ditto for DropProtobufsType and FutureProtobufType. Perhaps 'Message' and 'UnionType' would be clearer? Also, can we call this ExpectNoFutureUnionProtobuf rather than ExpectNoFutureProtobufsType? The latter is a bit confusing because type is overloaded with the type of the protobuf itself. - Ben Mahler On July 13, 2015, 11:57 p.m., Vinod Kone wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36462/ --- (Updated July 13, 2015, 11:57 p.m.) Review request for mesos and Ben Mahler. Bugs: MESOS-2913 https://issues.apache.org/jira/browse/MESOS-2913 Repository: mesos Description --- Needed these abstractions for capturing specific CALLs explicitly in subsequent reviews. Diffs - src/tests/mesos.hpp 9157ac079808d2686592e54ea26a26e6a0825ed3 Diff: https://reviews.apache.org/r/36462/diff/ Testing --- Tested in subsequent reviews. Thanks, Vinod Kone
Re: Review Request 36360: Adding common constants for HTTP API
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36360/#review91782 --- Ship it! src/common/http.hpp https://reviews.apache.org/r/36360/#comment145431 Whoops, this is still used? src/common/http.cpp (line 44) https://reviews.apache.org/r/36360/#comment145432 Whoops, space after =? - Ben Mahler On July 15, 2015, 2:50 a.m., Isabel Jimenez wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36360/ --- (Updated July 15, 2015, 2:50 a.m.) Review request for mesos, Anand Mazumdar, Benjamin Hindman, Ben Mahler, Marco Massenzio, and Vinod Kone. Bugs: MESOS-2860 https://issues.apache.org/jira/browse/MESOS-2860 Repository: mesos-incubating Description --- Adding constants used commonly through the different HTTP endpoints Diffs - src/common/http.hpp bbd063d src/common/http.cpp 73a4de1 Diff: https://reviews.apache.org/r/36360/diff/ Testing --- Thanks, Isabel Jimenez
Re: Review Request 35752: Implemented the ERROR Event handler in the scheduler driver.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/35752/#review91798 --- Ship it! Ship It! - Vinod Kone On July 15, 2015, 1:47 a.m., Ben Mahler wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/35752/ --- (Updated July 15, 2015, 1:47 a.m.) Review request for mesos and Vinod Kone. Bugs: MESOS-2910 https://issues.apache.org/jira/browse/MESOS-2910 Repository: mesos Description --- See above. Diffs - src/Makefile.am e5b5d36f0ac160e5a3a9fdc50b31c060a413ce2c src/sched/sched.cpp e372a15db035f74d525561839b873ed659e2c33f src/tests/scheduler_event_call_tests.cpp PRE-CREATION Diff: https://reviews.apache.org/r/35752/diff/ Testing --- For now, testing needs to be done via manual injection of events. I've added a new test file for this, which will include the driver and library tests for event / call handling. We should be able to remove these tests as all of the existing tests will later test the event/call path. Thanks, Ben Mahler
Re: Review Request 36450: Introduced Address and URL protobufs.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36450/#review91795 --- include/mesos/mesos.proto (line 102) https://reviews.apache.org/r/36450/#comment145445 s/ip/IP/ src/common/type_utils.cpp (line 131) https://reviews.apache.org/r/36450/#comment145447 Is the order of query parameters important? Aren't these URLs equivalent? http://a.b.c/?k1=ak2=b http://a.b.c/?k2=bk1=a - Vinod Kone On July 15, 2015, 1:01 a.m., Ben Mahler wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36450/ --- (Updated July 15, 2015, 1:01 a.m.) Review request for mesos, Benjamin Hindman, Jie Yu, and Vinod Kone. Bugs: MESOS-3012 https://issues.apache.org/jira/browse/MESOS-3012 Repository: mesos Description --- To make the API more consistent, we'd like to have a single way to express a network address. Also would like a way to express an HTTP address (a URL), which may include a path prefix. This also enables the message passing optimization in the scheduler driver when receiving events, per MESOS-3012. Diffs - include/mesos/mesos.proto 1763129da535561503e89cbd8c4a371f8553d8d6 include/mesos/type_utils.hpp e7bfe8ca60af945e76b5d85ab37cc97b17ff1b4a src/common/type_utils.cpp 19f79b47539ab51a5dff97f381a44c679cf5ecaf src/master/master.cpp b877676afa6f3833eb7d2fb06beeaa288bd8bd5d src/tests/master_tests.cpp 57721b788d0c70f4c6f5cc44d87465f52a70b6c2 Diff: https://reviews.apache.org/r/36450/diff/ Testing --- Modified the simplest test I could find for offers. Thanks, Ben Mahler
Re: Review Request 36517: Disable SharedFilesystemIsolator tests.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36517/#review91796 --- Ship it! Ship It! - Jie Yu On July 15, 2015, 6:32 p.m., Timothy Chen wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36517/ --- (Updated July 15, 2015, 6:32 p.m.) Review request for mesos, Adam B and Jie Yu. Bugs: MESOS-3050 https://issues.apache.org/jira/browse/MESOS-3050 Repository: mesos Description --- Disable SharedFilesystemIsolator tests. Diffs - src/tests/isolator_tests.cpp 8eb732fe90dbab11967e663a5c06890f79463488 Diff: https://reviews.apache.org/r/36517/diff/ Testing --- make check Thanks, Timothy Chen
Re: Review Request 36424: Created a command executor helper method.
On July 14, 2015, 12:20 a.m., Paul Brett wrote: 3rdparty/libprocess/include/process/subprocess.hpp, line 307 https://reviews.apache.org/r/36424/diff/1/?file=1008891#file1008891line307 How about returning a tuple/struct of stdout, stderr and return code and let the caller decide what they want? Marco Massenzio wrote: sure, that would be a possibility too, but it seems to me that the approved way in Mesos is to return a `Try` for when something *may* go wrong. This is consistent across the entire code base. Artem Harutyunyan wrote: Maybe I am missing something, I did a grep for `FutureTry..` and could not find any occurence of it in the code base. Perhaps the reason is that it's customary to use Future's `Failure()` to indicate an error (as opposed to returning a `Try`). If anything `Result` would probably be more appropriate here than Try, but I'd like to hear what a shepherd has to say. The function could just return `Futurestd::string` and you could use `Failure()` to indicate the error. In that case you'll need to change the return type of `.then` lamda to `Futurestd::string` and also to replace a `return Error(...` on line 346 with `return Failure(...` (which you might want to do anyway for the sake of consistency). I'd like to hear what a shepherd has to say. eh eh, no kidding - while writing this code, I swear my brain melted :) The one thing to bear in mind (and that's probably the reason this is a bit of a 'only child') is that the 'error mode' here is different than elsewhere: if the command 'fails' the request to run a command actually 'succeeded' - if I try to execute: 'ls -la /tmp/foo' well, the command executes successfully, it's just that `foo` ain't there. So, the semantics of a Future of a Try is that, yep, your request succeeded and, yay!, your command succeeded too *or* dang! that failed and here's the error message. (side note: that's also the reason why I return a 200 OK from the `/execute` endpoint, even if the command fails - your Request, nonetheless succeeded). But I can be convinced both ways, and return instead a `Failure(stderr)` However, I like the tuple idea better because (a) the exit code carries information that we'd be losing and (b) sometimes, to understand what really went wrong, one needs both stdout **and** stderr, so I'm considering returning a `(code, stdout, stderr)` tuple (yay! C++11 FTW) - Marco --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36424/#review91566 --- On July 14, 2015, 4:21 a.m., Marco Massenzio wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36424/ --- (Updated July 14, 2015, 4:21 a.m.) Review request for mesos, Benjamin Hindman and Cody Maloney. Bugs: MESOS-3035 https://issues.apache.org/jira/browse/MESOS-3035 Repository: mesos Description --- Jira: MESOS-2902 While researching how to execute an arbitrary script to detect the Master IP address, it emerged clearly that a helper method to execute an arbitrary command/script on a node and obtain either stdout or stderr would have been useful and avoided a lot of code repetition. This could not be ultimately used for the purpose at hand, but I believe it to be useful enough (particularly, to avoid people doing coding by copypaste and/or waste time researching the same functionality). This would also be beneficial in MESOS-2830, factoring out the remote command execution logic. Diffs - 3rdparty/libprocess/include/process/subprocess.hpp 310cb4f8e4e2faa5545dffd196d7490c868bc5d6 3rdparty/libprocess/src/tests/subprocess_tests.cpp f6acb204582a9e696c3b09d4e4c543bb052e97d4 Diff: https://reviews.apache.org/r/36424/diff/ Testing --- make check Thanks, Marco Massenzio
Re: Review Request 36517: Disable SharedFilesystemIsolator tests.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36517/#review91805 --- Patch looks great! Reviews applied: [36517] All tests passed. - Mesos ReviewBot On July 15, 2015, 6:32 p.m., Timothy Chen wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36517/ --- (Updated July 15, 2015, 6:32 p.m.) Review request for mesos, Adam B and Jie Yu. Bugs: MESOS-3050 https://issues.apache.org/jira/browse/MESOS-3050 Repository: mesos Description --- Disable SharedFilesystemIsolator tests. Diffs - src/tests/isolator_tests.cpp 8eb732fe90dbab11967e663a5c06890f79463488 Diff: https://reviews.apache.org/r/36517/diff/ Testing --- make check Thanks, Timothy Chen
Re: Review Request 36389: Enable remote execution of arbitrary command.
On July 15, 2015, 6:13 p.m., Artem Harutyunyan wrote: To make sure that this is fool proof(ish), I would suggest that this should only ship only when the Authorizer framework (mentioned in the TODO comment) becomes available. Also, I would add a screaming comment to --usage, something along the lines of 'this is insecure, and this enables arbitrary command executrion with root privileges'. In general, I am of the firm opinion that this feature should come with a whitelisting mechanism that will allow operators to whitelist (benign) commands that they want to execute, and forbid anything else. To make sure that this is fool proof(ish), I would suggest that this should only ship only when the Authorizer framework (mentioned in the TODO comment) becomes available. Also, I would add a screaming comment to --usage, something along the lines of 'this is insecure, and this enables arbitrary command executrion with root privileges'. This has to be deliberately enabled: ``` --allow_remote_execution Allows the execution of arbitrary shell commands on the Slave, by POSTing to the /execute endpoint with a JSON-serialized CommandInfo protobuf, containing the command to execute and, optionally, the arguments. WARNING: this may cause a security vulnerability and is disabled by default; enable only if you know exactly what you are doing. ``` The WARNING seems explicit enough to me, but if people agree, I'm happy TO MAKE IT SCREAM ;-) In general, I am of the firm opinion that this feature should come with a whitelisting mechanism that will allow operators to whitelist (benign) commands that they want to execute, and forbid anything else. Perhaps; but please note that this is meant mostly for emergency/off-cycle maintenance reasons, so it's very difficult to know in advance *what* you will need (if you did, you probably wouldn't need the feauture in the first place). This is also meant to be a highly scriptable endpoint, where you have either a generating function or a discovery methodology, that finds out (potentially 1,000s of) IPs for your Slaves and then does in very rapid succession a POST with the command to execute (I'm envisioning some catastrophic failure mode that needs to be prevented; or a security breach; or something like that). In any event, in modern systems, the way to protect these endpoints is at the network security level, by isolating services within a protected, private subnet, with access granted only from bastion servers (hardened and secured) - re-implementing authentication/authorization on every single service is not only prone to leaving some vulnerable to security breaches, but it also becomes quickly unmanageable (as you know have disparate, and inconsistent, security policies and authentication mechanism and no centralized way to manage and revoke access). (well, when I say modern... that was 5-6 years ago at Google... but I figure we were 4-5 years ahead of everyone else :) ) - Marco --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36389/#review91784 --- On July 14, 2015, 11:20 p.m., Marco Massenzio wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36389/ --- (Updated July 14, 2015, 11:20 p.m.) Review request for mesos, Benjamin Hindman and Cody Maloney. Bugs: MESOS-2830 https://issues.apache.org/jira/browse/MESOS-2830 Repository: mesos Description --- Jira: MESOS-2830 Under certain (maintenance) circumnstance, it may be necessary (or desirable) to execute arbitrary operator's commands on the slave (the entire fleet or a subset thereof) bypassing the Mesos Task execution mechanism; this may typically be necessary for maintenance and/or emergency actions. This patch adds an HTTP endpoint (/execute) which accepts a JSON-encoded CommandInfo structure and executes the given command (with optional arguments). The output of the command (along with potentially any stderr messages) is returned JSON-encoded in the Response. For more details, see the design doc at: https://goo.gl/4npTMU Diffs - src/slave/flags.hpp 26c778db2303167369af8675fe0441a00a1e9151 src/slave/flags.cpp 8632677ebbdbfef8ffa45204b6f63a700baff7f3 src/slave/main.cpp 8008430e98773d8be9ba5ac6385cffb2e351932a src/slave/slave.hpp dec4ca8323e151a6d0f9139214ff0ef6e3e3375a src/slave/slave.cpp 2119b5176aa7cfb7b0b551d4d4f65ee12818b9e4 Diff: https://reviews.apache.org/r/36389/diff/ Testing --- make check lots of manual testing (using Postman, REST client) Thanks, Marco Massenzio
Re: Review Request 36529: Fixing cgroups cpuacct stats test
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36529/#review91831 --- Ship it! Ship It! - Timothy Chen On July 15, 2015, 10:25 p.m., Jojy Varghese wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36529/ --- (Updated July 15, 2015, 10:25 p.m.) Review request for mesos and Timothy Chen. Repository: mesos Description --- cgroups::destroy does not succeed when there is a running task in the cgroup.The test was failing as current task was in the cgroup being destroyed. Diffs - src/tests/cgroups_tests.cpp a371ee205f89de10e15902057848c35f7c278d77 Diff: https://reviews.apache.org/r/36529/diff/ Testing --- Thanks, Jojy Varghese
Re: Review Request 36529: Fixing cgroups cpuacct stats test
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36529/#review91830 --- src/tests/cgroups_tests.cpp (line 1199) https://reviews.apache.org/r/36529/#comment145471 I would add a comment that we're moving to the root cgroup. I'll add it for you this time. - Timothy Chen On July 15, 2015, 10:25 p.m., Jojy Varghese wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36529/ --- (Updated July 15, 2015, 10:25 p.m.) Review request for mesos and Timothy Chen. Repository: mesos Description --- cgroups::destroy does not succeed when there is a running task in the cgroup.The test was failing as current task was in the cgroup being destroyed. Diffs - src/tests/cgroups_tests.cpp a371ee205f89de10e15902057848c35f7c278d77 Diff: https://reviews.apache.org/r/36529/diff/ Testing --- Thanks, Jojy Varghese
Re: Review Request 36217: Adding http validations to master call request validations
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36217/ --- (Updated July 15, 2015, 10:46 p.m.) Review request for mesos, Anand Mazumdar, Ben Mahler, Marco Massenzio, and Vinod Kone. Changes --- review comments Bugs: MESOS-2860 https://issues.apache.org/jira/browse/MESOS-2860 Repository: mesos-incubating Description --- Adding helper validations for http requests Diffs (updated) - 3rdparty/libprocess/include/process/http.hpp 72b6d27 3rdparty/libprocess/src/encoder.hpp c5ff761 3rdparty/libprocess/src/http.cpp d168579 3rdparty/libprocess/src/tests/encoder_tests.cpp 0032137 3rdparty/libprocess/src/tests/http_tests.cpp 01f243c Diff: https://reviews.apache.org/r/36217/diff/ Testing --- make check Thanks, Isabel Jimenez
Re: Review Request 36402: Adding 'Accept' header in request
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36402/ --- (Updated July 15, 2015, 10:51 p.m.) Review request for mesos, Anand Mazumdar, Ben Mahler, Marco Massenzio, and Vinod Kone. Repository: mesos-incubating Description --- Adding a method for Accept header in request + refactor of Accept-Encoding Diffs (updated) - 3rdparty/libprocess/include/process/http.hpp 72b6d27 3rdparty/libprocess/src/encoder.hpp c5ff761 3rdparty/libprocess/src/http.cpp d168579 3rdparty/libprocess/src/tests/encoder_tests.cpp 0032137 3rdparty/libprocess/src/tests/http_tests.cpp 01f243c Diff: https://reviews.apache.org/r/36402/diff/ Testing --- make check Thanks, Isabel Jimenez
Re: Review Request 36402: Adding 'Accept' header in request
On July 10, 2015, 10:13 p.m., Anand Mazumdar wrote: 3rdparty/libprocess/src/http.cpp, line 216 https://reviews.apache.org/r/36402/diff/1/?file=1008565#file1008565line216 I did not review the entire patch but I stopped at this. Seems like I am missing something here. In my understanding, you can't use the same method for parsing both the Accept, Accept-Encoding as the rules for both of them are entirely different ! :) Let's take an example from the RFC : http://www.w3.org/Protocols/rfc2616/rfc2616-sec14.html So the accept header also understands media-range i.e. you can specify */* or text/* et al meaning all media types can be accepted or in the second case all media types of text/something can only be accepted and so on. There are a couple of other things like accept-param and accept-extension that also need to be handled. I assume that your motive for this change was to add a validation operation for Accept similar to Accept-Encoding header that validates if the header values are well-formed and should be accepted ? If that is the case, you would need to write a separate method/logic and not just use the existing acceptEncoding method. Long story short, we need two methods: 1. Validate if the Accept header is well formed. ( the one you already built minus the mentioned caveats above ) Also , it would be good to add a second one: 2. Given all the accept types mentioned in the Accept header , and the accept types we support ,is it possible for us to send a response back ? If not send a 415 back. What do you think ? Isabel Jimenez wrote: The AcceptHeader method groups validation that's common to both 'Accept' and 'Accept-Encoding'. The logic was already there I just moved it so we can use it for both, and if needed and more things to each one separately. I plan to add 'accept-param' and 'accept-extension' support in a different patch. I also added a TODO to consider handling all this by returning Response, what do you think, should we make that change now? Anand Mazumdar wrote: Unfortunately, This does not make much sense to me. Let's take an example from your test-case. requests[2].headers[Accept] = foo, test;q=0.0; This is not a VALID accept header at all if you see its grammer carefully. The only thing that Accept and Accept-Encoding have in common is the q-value syntax and how you specify them on one line i.e. delimited by , and ;. :) An Accept header must have a type/subtype or a type/*. Makes sense ? ( I am re-opening the issue ) I was assuming that for 'accept' headers users will always provide 'type/subtype', but you're right. Fix this. - Isabel --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36402/#review91356 --- On July 15, 2015, 10:51 p.m., Isabel Jimenez wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36402/ --- (Updated July 15, 2015, 10:51 p.m.) Review request for mesos, Anand Mazumdar, Ben Mahler, Marco Massenzio, and Vinod Kone. Repository: mesos-incubating Description --- Adding a method for Accept header in request + refactor of Accept-Encoding Diffs - 3rdparty/libprocess/include/process/http.hpp 72b6d27 3rdparty/libprocess/src/encoder.hpp c5ff761 3rdparty/libprocess/src/http.cpp d168579 3rdparty/libprocess/src/tests/encoder_tests.cpp 0032137 3rdparty/libprocess/src/tests/http_tests.cpp 01f243c Diff: https://reviews.apache.org/r/36402/diff/ Testing --- make check Thanks, Isabel Jimenez
Re: Review Request 36529: Fixing cgroups cpuacct stats test
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36529/#review91838 --- Patch looks great! Reviews applied: [36529] All tests passed. - Mesos ReviewBot On July 15, 2015, 10:25 p.m., Jojy Varghese wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36529/ --- (Updated July 15, 2015, 10:25 p.m.) Review request for mesos and Timothy Chen. Repository: mesos Description --- cgroups::destroy does not succeed when there is a running task in the cgroup.The test was failing as current task was in the cgroup being destroyed. Diffs - src/tests/cgroups_tests.cpp a371ee205f89de10e15902057848c35f7c278d77 Diff: https://reviews.apache.org/r/36529/diff/ Testing --- Thanks, Jojy Varghese
Re: Review Request 36510: Update .gitignore with intermediate build artifacts.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36510/#review91753 --- Patch looks great! Reviews applied: [36510] All tests passed. - Mesos ReviewBot On July 15, 2015, 4:11 p.m., James Peach wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36510/ --- (Updated July 15, 2015, 4:11 p.m.) Review request for mesos and Benjamin Hindman. Bugs: MESOS-3054 https://issues.apache.org/jira/browse/MESOS-3054 Repository: mesos Description --- Update .gitignore with intermediate build artifacts. Diffs - .gitignore-template 934cad71a370d5c64d9a0994f775f39db74f5851 Diff: https://reviews.apache.org/r/36510/diff/ Testing --- In a dirty build tree, run ```git status``` and note the reduced clutter. Thanks, James Peach
Re: Review Request 35687: Added capabilities to state.json
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/35687/ --- (Updated July 15, 2015, 5:20 p.m.) Review request for mesos, Marco Massenzio and Vinod Kone. Bugs: MESOS-2900 https://issues.apache.org/jira/browse/MESOS-2900 Repository: mesos Description --- Added capabilities to state.json and test for the same Diffs (updated) - src/master/http.cpp 350383362311cfbc830965e1155a8515f0dfb332 src/tests/master_tests.cpp 962455cc368c6e5405599d6565660d4c3fd0fc22 Diff: https://reviews.apache.org/r/35687/diff/ Testing --- make check Thanks, Aditi Dixit
Re: Review Request 35687: Added capabilities to state.json
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/35687/#review91774 --- Patch looks great! Reviews applied: [35687] All tests passed. - Mesos ReviewBot On July 15, 2015, 5:20 p.m., Aditi Dixit wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/35687/ --- (Updated July 15, 2015, 5:20 p.m.) Review request for mesos, Marco Massenzio and Vinod Kone. Bugs: MESOS-2900 https://issues.apache.org/jira/browse/MESOS-2900 Repository: mesos Description --- Added capabilities to state.json and test for the same Diffs - src/master/http.cpp 350383362311cfbc830965e1155a8515f0dfb332 src/tests/master_tests.cpp 962455cc368c6e5405599d6565660d4c3fd0fc22 Diff: https://reviews.apache.org/r/35687/diff/ Testing --- make check Thanks, Aditi Dixit
Re: Review Request 36524: Ignore warnings around removing unknown user in isolator tests.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36524/#review91816 --- Ship it! Ship It! - Jie Yu On July 15, 2015, 8:21 p.m., Timothy Chen wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36524/ --- (Updated July 15, 2015, 8:21 p.m.) Review request for mesos, Adam B and Jie Yu. Repository: mesos Description --- Ignore warnings around removing unknown user in isolator tests. Diffs - src/tests/isolator_tests.cpp b63a9cce6d9652890c011df8781565a11c52e2d1 Diff: https://reviews.apache.org/r/36524/diff/ Testing --- make check Thanks, Timothy Chen
Review Request 36522: Improve base hierarchy detection in cgroup tests.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36522/ --- Review request for mesos, Adam B and Jie Yu. Repository: mesos Description --- Improve base hierarchy detection in cgroup tests. Diffs - src/tests/cgroups_tests.cpp af4b37b01337e2c881c99635097facf5b15fa05d Diff: https://reviews.apache.org/r/36522/diff/ Testing --- make check Thanks, Timothy Chen
Re: Review Request 36518: Fixed a bug in master to properly handle resubscription.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36518/#review91813 --- Patch looks great! Reviews applied: [36518] All tests passed. - Mesos ReviewBot On July 15, 2015, 6:40 p.m., Vinod Kone wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36518/ --- (Updated July 15, 2015, 6:40 p.m.) Review request for mesos and Ben Mahler. Bugs: MESOS-3055 https://issues.apache.org/jira/browse/MESOS-3055 Repository: mesos Description --- In the process of fixing this, added an additional check in Master::registerFramework() that should've been there in the first place. Similar check exists in Master::reregisterFramework(). Diffs - src/master/master.cpp b877676afa6f3833eb7d2fb06beeaa288bd8bd5d src/tests/scheduler_tests.cpp 946fa8245d8ab35e04bad642d69114caf0ccf6a9 Diff: https://reviews.apache.org/r/36518/diff/ Testing --- make check Thanks, Vinod Kone
Review Request 36524: Ignore warnings around removing unknown user in isolator tests.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36524/ --- Review request for mesos, Adam B and Jie Yu. Repository: mesos Description --- Ignore warnings around removing unknown user in isolator tests. Diffs - src/tests/isolator_tests.cpp b63a9cce6d9652890c011df8781565a11c52e2d1 Diff: https://reviews.apache.org/r/36524/diff/ Testing --- make check Thanks, Timothy Chen
Review Request 36523: Fix container cgroup detection in isolator tests.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36523/ --- Review request for mesos, Adam B and Jie Yu. Repository: mesos Description --- Fix container cgroup detection in isolator tests. Diffs - src/tests/isolator_tests.cpp b63a9cce6d9652890c011df8781565a11c52e2d1 Diff: https://reviews.apache.org/r/36523/diff/ Testing --- make check Thanks, Timothy Chen
Re: Review Request 36522: Improve base hierarchy detection in cgroup tests.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36522/#review91818 --- Ship it! Please put some context in the review please next time (e.g., the error message, ticket, etc.). - Jie Yu On July 15, 2015, 8:20 p.m., Timothy Chen wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36522/ --- (Updated July 15, 2015, 8:20 p.m.) Review request for mesos, Adam B and Jie Yu. Repository: mesos Description --- Improve base hierarchy detection in cgroup tests. Diffs - src/tests/cgroups_tests.cpp af4b37b01337e2c881c99635097facf5b15fa05d Diff: https://reviews.apache.org/r/36522/diff/ Testing --- make check Thanks, Timothy Chen
Re: Review Request 36494: Implemented the MESSAGE Event handler in the scheduler driver.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36494/#review91819 --- src/sched/sched.cpp (lines 489 - 492) https://reviews.apache.org/r/36494/#comment145458 instead of calling into frameworkMessage() here can you have frameworkMessage() call into a new message(const Event event) method? this is how we did it in the master and i think it will make deprecating old message handlers easy. - Vinod Kone On July 15, 2015, 1:47 a.m., Ben Mahler wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36494/ --- (Updated July 15, 2015, 1:47 a.m.) Review request for mesos and Vinod Kone. Bugs: MESOS-2910 https://issues.apache.org/jira/browse/MESOS-2910 Repository: mesos Description --- See above. Diffs - src/sched/sched.cpp e372a15db035f74d525561839b873ed659e2c33f src/tests/scheduler_event_call_tests.cpp PRE-CREATION Diff: https://reviews.apache.org/r/36494/diff/ Testing --- Added a test. Thanks, Ben Mahler
Re: Review Request 36402: Adding 'Accept' header in request
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36402/#review91843 --- Have not gone through the whole review yet. It would be worth discussing this with @bmahler more on what he thinks about my comment on refactoring acceptMediaType and then go over this again based on his feedback. 3rdparty/libprocess/src/http.cpp (line 125) https://reviews.apache.org/r/36402/#comment145496 Whoops ! Let's return false as earlier. Also add a test-case for empty check. ( Accept-Encoding ) 3rdparty/libprocess/src/http.cpp https://reviews.apache.org/r/36402/#comment145495 Can we get these comments back ? 3rdparty/libprocess/src/http.cpp (lines 135 - 136) https://reviews.apache.org/r/36402/#comment145510 Can we put these comments back ? 3rdparty/libprocess/src/http.cpp https://reviews.apache.org/r/36402/#comment145511 Same here. 3rdparty/libprocess/src/http.cpp (line 163) https://reviews.apache.org/r/36402/#comment145509 How do we intend to use this method ? Let's say we have 2 media types that we support application/json and application/x-protobuf. I would need to call acceptMediaType twice ( in a loop ): - acceptMediaType(application/json) , if it returns false , call again with - acceptMediaType(application/x-protobuf). Return back a 415 if both of these return false. Why not we change the method signature to: Trystd::string Request::findFirstAcceptedType(const std::vectorstd::string acceptedMediaTypes) that just returns a string with the first match and an Error if none of the options in acceptedMediaTypes match. We did not face the same issue with Accept-Encoding may be because gzip is the only encoding we support ( and not deflate ? ) 3rdparty/libprocess/src/http.cpp (line 196) https://reviews.apache.org/r/36402/#comment145512 Nit-pick : Modify the comment string here, gzip;q=0.0 does not hold good here. - Anand Mazumdar On July 15, 2015, 11:54 p.m., Isabel Jimenez wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36402/ --- (Updated July 15, 2015, 11:54 p.m.) Review request for mesos, Anand Mazumdar, Ben Mahler, Marco Massenzio, and Vinod Kone. Repository: mesos-incubating Description --- Adding a method for Accept header in request + refactor of Accept-Encoding Diffs - 3rdparty/libprocess/include/process/http.hpp 72b6d27 3rdparty/libprocess/src/http.cpp d168579 3rdparty/libprocess/src/tests/http_tests.cpp 01f243c Diff: https://reviews.apache.org/r/36402/diff/ Testing --- make check Thanks, Isabel Jimenez
Re: Review Request 36488: Added oversubscription user doc.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36488/#review91840 --- Looks great! Helped me understand the feature much better. Comments below. docs/oversubscription.md (line 5) https://reviews.apache.org/r/36488/#comment145484 I find Online confusing. How about High-priority user-facing services...? (then it also ties into the low-priority jobs you mention at the end of the paragraph. docs/oversubscription.md (lines 7 - 8) https://reviews.apache.org/r/36488/#comment145485 Oversubscription takes advantage... docs/oversubscription.md (line 15) https://reviews.apache.org/r/36488/#comment145487 s/QoS/Quality of Service (QoS)/ just to introduce the term. docs/oversubscription.md (line 19) https://reviews.apache.org/r/36488/#comment145488 In the image text, 6) s/doesn't/don't/ docs/oversubscription.md (line 28) https://reviews.apache.org/r/36488/#comment145489 Define 'slack'? docs/oversubscription.md (lines 28 - 29) https://reviews.apache.org/r/36488/#comment145490 Should (2) go in its own bullet item? docs/oversubscription.md (line 36) https://reviews.apache.org/r/36488/#comment145503 What types of resources can be oversubscribed? Just cpu and memory? What about disk? Ports? Custom resource types (bananas)? docs/oversubscription.md (line 37) https://reviews.apache.org/r/36488/#comment145492 s/the offer/those offers/? Also, can you provide an example Resource message with 'revocable' set? docs/oversubscription.md (lines 41 - 42) https://reviews.apache.org/r/36488/#comment145502 Can you dynamically reserve revocable resources? Can you create persistent volumes out of revocable disk resources? docs/oversubscription.md (line 73) https://reviews.apache.org/r/36488/#comment145504 issue revocable offers to this framework. docs/oversubscription.md (lines 75 - 76) https://reviews.apache.org/r/36488/#comment145505 As an operator, how do I enable oversubscription then? Maybe add See below for instructions for Configuring Mesos for Oversubscription, or reorder these? docs/oversubscription.md (line 80) https://reviews.apache.org/r/36488/#comment145506 s/That be, frameworks/Thus, a framework/ docs/oversubscription.md (line 82) https://reviews.apache.org/r/36488/#comment145507 s/for regular and revocable resources separately./separately for regular and revocable resources./ docs/oversubscription.md (line 83) https://reviews.apache.org/r/36488/#comment145508 s/have/has/ docs/oversubscription.md (line 91) https://reviews.apache.org/r/36488/#comment145515 Noop too? docs/oversubscription.md (lines 121 - 124) https://reviews.apache.org/r/36488/#comment145516 You said exactly the same thing above in (6). Do you need to repeat yourself? What specific info do you want in each section? docs/oversubscription.md (line 153) https://reviews.apache.org/r/36488/#comment145517 Is this currently the only type? What about THROTTLE? docs/oversubscription.md (line 166) https://reviews.apache.org/r/36488/#comment145521 s/the Mesos/Mesos/ Is there a switch to globally disable the feature from the master? Can some slaves enable it while others don't? docs/oversubscription.md (line 168) https://reviews.apache.org/r/36488/#comment145524 I count 5 flags, not 3. docs/oversubscription.md (line 233) https://reviews.apache.org/r/36488/#comment145527 Can you have multiple resource estimators running on the same node simultaneously? What about QoS controllers? - Adam B On July 14, 2015, 5:08 p.m., Niklas Nielsen wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36488/ --- (Updated July 14, 2015, 5:08 p.m.) Review request for mesos, Adam B and Jie Yu. Repository: mesos Description --- Added first draft of oversubscription user doc Diffs - docs/images/oversubscription-overview.jpg PRE-CREATION docs/oversubscription.md PRE-CREATION Diff: https://reviews.apache.org/r/36488/diff/ Testing --- Rendered at: https://github.com/nqn/mesos/blob/niklas/oversubscription-user-doc/docs/oversubscription.md Thanks, Niklas Nielsen
Re: Review Request 36532: Added warning in the SSL Doc about potentially failing tests in unclean build directory
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36532/#review91854 --- Patch looks great! Reviews applied: [36532] All tests passed. - Mesos ReviewBot On July 15, 2015, 11:52 p.m., Marco Massenzio wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36532/ --- (Updated July 15, 2015, 11:52 p.m.) Review request for mesos, Adam B and Joris Van Remoortere. Repository: mesos Description --- Added warning in the SSL Doc about potentially failing tests in unclean build directory Diffs - docs/mesos-ssl.md bb218492df90c922bc4955daeca5513ba9d18633 Diff: https://reviews.apache.org/r/36532/diff/ Testing --- Thanks, Marco Massenzio
Review Request 36532: Added warning in the SSL Doc about potentially failing tests in unclean build directory
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36532/ --- Review request for mesos, Adam B and Joris Van Remoortere. Repository: mesos Description --- Added warning in the SSL Doc about potentially failing tests in unclean build directory Diffs - docs/mesos-ssl.md bb218492df90c922bc4955daeca5513ba9d18633 Diff: https://reviews.apache.org/r/36532/diff/ Testing --- Thanks, Marco Massenzio
Re: Review Request 36402: Adding 'Accept' header in request
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36402/ --- (Updated July 15, 2015, 11:54 p.m.) Review request for mesos, Anand Mazumdar, Ben Mahler, Marco Massenzio, and Vinod Kone. Repository: mesos-incubating Description --- Adding a method for Accept header in request + refactor of Accept-Encoding Diffs (updated) - 3rdparty/libprocess/include/process/http.hpp 72b6d27 3rdparty/libprocess/src/http.cpp d168579 3rdparty/libprocess/src/tests/http_tests.cpp 01f243c Diff: https://reviews.apache.org/r/36402/diff/ Testing --- make check Thanks, Isabel Jimenez
Re: Review Request 36402: Adding 'Accept' header in request
On July 16, 2015, 12:10 a.m., Anand Mazumdar wrote: 3rdparty/libprocess/src/http.cpp, lines 135-143 https://reviews.apache.org/r/36402/diff/3/?file=1012922#file1012922line135 Can we get these comments back ? They're on the header file now, do we want them duplciate here? On July 16, 2015, 12:10 a.m., Anand Mazumdar wrote: 3rdparty/libprocess/src/http.cpp, lines 145-146 https://reviews.apache.org/r/36402/diff/3/?file=1012922#file1012922line145 Can we put these comments back ? Because rules aren't here anymore but in the header file. I thought it was confusing to let this here. On July 16, 2015, 12:10 a.m., Anand Mazumdar wrote: 3rdparty/libprocess/src/http.cpp, lines 169-177 https://reviews.apache.org/r/36402/diff/3/?file=1012922#file1012922line169 Same here. same here, moved to header. On July 16, 2015, 12:10 a.m., Anand Mazumdar wrote: 3rdparty/libprocess/src/http.cpp, line 126 https://reviews.apache.org/r/36402/diff/3/?file=1012922#file1012922line126 Whoops ! Let's return false as earlier. Also add a test-case for empty check. ( Accept-Encoding ) Added a TODO since it's not standard to return false here. - Isabel --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36402/#review91843 --- On July 15, 2015, 11:54 p.m., Isabel Jimenez wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36402/ --- (Updated July 15, 2015, 11:54 p.m.) Review request for mesos, Anand Mazumdar, Ben Mahler, Marco Massenzio, and Vinod Kone. Repository: mesos-incubating Description --- Adding a method for Accept header in request + refactor of Accept-Encoding Diffs - 3rdparty/libprocess/include/process/http.hpp 72b6d27 3rdparty/libprocess/src/http.cpp d168579 3rdparty/libprocess/src/tests/http_tests.cpp 01f243c Diff: https://reviews.apache.org/r/36402/diff/ Testing --- make check Thanks, Isabel Jimenez
Re: Review Request 36402: Adding 'Accept' header in request
On July 16, 2015, 12:10 a.m., Anand Mazumdar wrote: 3rdparty/libprocess/src/http.cpp, line 126 https://reviews.apache.org/r/36402/diff/3/?file=1012922#file1012922line126 Whoops ! Let's return false as earlier. Also add a test-case for empty check. ( Accept-Encoding ) Isabel Jimenez wrote: Added a TODO since it's not standard to return false here. Looks like I am missing something here. This is a method for Accept-Encoding. If the header is not present and you pass gzip as argument to the method , it should return false as was the case earlier since the client can't accept gzip ( gzip is a bad example here owing to the exception in the rfc around it/compress). Did you get confused with it being the Accept header ? Re-opening the issue for now. I think we can get rid of the TODO here. - Anand --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36402/#review91843 --- On July 15, 2015, 11:54 p.m., Isabel Jimenez wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36402/ --- (Updated July 15, 2015, 11:54 p.m.) Review request for mesos, Anand Mazumdar, Ben Mahler, Marco Massenzio, and Vinod Kone. Repository: mesos-incubating Description --- Adding a method for Accept header in request + refactor of Accept-Encoding Diffs - 3rdparty/libprocess/include/process/http.hpp 72b6d27 3rdparty/libprocess/src/http.cpp d168579 3rdparty/libprocess/src/tests/http_tests.cpp 01f243c Diff: https://reviews.apache.org/r/36402/diff/ Testing --- make check Thanks, Isabel Jimenez
Review Request 36527: Fix cgroup tests to not verify order.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36527/ --- Review request for mesos and Jie Yu. Bugs: MESOS-3058 https://issues.apache.org/jira/browse/MESOS-3058 Repository: mesos Description --- Fix cgroup tests to not verify order. Diffs - src/tests/cgroups_tests.cpp a371ee205f89de10e15902057848c35f7c278d77 Diff: https://reviews.apache.org/r/36527/diff/ Testing --- make check Thanks, Timothy Chen
Re: Review Request 36518: Fixed a bug in master to properly handle resubscription.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36518/#review91821 --- Ship it! Looks good given the current Call API. However, per our chat, 'force' doesn't make sense for initial SUBSCRIBE calls, I left a comment for how we might want to address that. =/ src/master/master.cpp (line 1747) https://reviews.apache.org/r/36518/#comment145459 Seems like we might want to keep the condition consistent across all of these checks (i.e. has_id id != ), up to you. At least, would be nice to add an != operator for FrameworkID vs string. src/tests/scheduler_tests.cpp (lines 110 - 112) https://reviews.apache.org/r/36518/#comment145461 Since there's no RESUBSCRIBE, shall we simply call this test 'Subscribe' (I noticed there is no Subscribe test currently) and test the full semantics within it? Looks like a test of the 'Subscribe' call to me. src/tests/scheduler_tests.cpp (line 143) https://reviews.apache.org/r/36518/#comment145465 Hm.. 'force' doesn't make sense for SUBSCRIBE without a framework id. Seems like either we: (1) Make 'force' optional, and document that it is only relevant when the framework id is set (re-subscription). (2) Remove 'force' from SUBSCRIBE, add a RESUBSCRIBE with 'force'? =/ src/tests/scheduler_tests.cpp (line 163) https://reviews.apache.org/r/36518/#comment145462 why the newline here but not above? - Ben Mahler On July 15, 2015, 6:40 p.m., Vinod Kone wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36518/ --- (Updated July 15, 2015, 6:40 p.m.) Review request for mesos and Ben Mahler. Bugs: MESOS-3055 https://issues.apache.org/jira/browse/MESOS-3055 Repository: mesos Description --- In the process of fixing this, added an additional check in Master::registerFramework() that should've been there in the first place. Similar check exists in Master::reregisterFramework(). Diffs - src/master/master.cpp b877676afa6f3833eb7d2fb06beeaa288bd8bd5d src/tests/scheduler_tests.cpp 946fa8245d8ab35e04bad642d69114caf0ccf6a9 Diff: https://reviews.apache.org/r/36518/diff/ Testing --- make check Thanks, Vinod Kone
Re: Review Request 36524: Ignore warnings around removing unknown user in isolator tests.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36524/#review91824 --- Patch looks great! Reviews applied: [36522, 36523, 36524] All tests passed. - Mesos ReviewBot On July 15, 2015, 8:21 p.m., Timothy Chen wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36524/ --- (Updated July 15, 2015, 8:21 p.m.) Review request for mesos, Adam B and Jie Yu. Repository: mesos Description --- Ignore warnings around removing unknown user in isolator tests. Diffs - src/tests/isolator_tests.cpp b63a9cce6d9652890c011df8781565a11c52e2d1 Diff: https://reviews.apache.org/r/36524/diff/ Testing --- make check Thanks, Timothy Chen
Re: Review Request 36527: Fix cgroup tests to not verify order.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36527/#review91828 --- Patch looks great! Reviews applied: [36527] All tests passed. - Mesos ReviewBot On July 15, 2015, 9:31 p.m., Timothy Chen wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36527/ --- (Updated July 15, 2015, 9:31 p.m.) Review request for mesos and Jie Yu. Bugs: MESOS-3058 https://issues.apache.org/jira/browse/MESOS-3058 Repository: mesos Description --- Fix cgroup tests to not verify order. Diffs - src/tests/cgroups_tests.cpp a371ee205f89de10e15902057848c35f7c278d77 Diff: https://reviews.apache.org/r/36527/diff/ Testing --- make check Thanks, Timothy Chen
Review Request 36529: Fixing cgroups cpuacct stats test
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36529/ --- Review request for mesos and Timothy Chen. Repository: mesos Description --- cgroups::destroy does not succeed when there is a running task in the cgroup.The test was failing as current task was in the cgroup being destroyed. Diffs - src/tests/cgroups_tests.cpp a371ee205f89de10e15902057848c35f7c278d77 Diff: https://reviews.apache.org/r/36529/diff/ Testing --- Thanks, Jojy Varghese
Re: Review Request 36501: MESOS-3023
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36501/#review91732 --- src/tests/utils.hpp (line 26) https://reviews.apache.org/r/36501/#comment145331 Using ::strlen to get the length of ://, did not want to hardcord to 3. src/tests/utils.hpp (line 54) https://reviews.apache.org/r/36501/#comment145334 As far as I known, C++ can not declare template in header file and implete it in cpp. I also have a try with the example. In C++11, it only introduced extern template to avoid duplicated template instance. src/tests/utils.hpp (line 55) https://reviews.apache.org/r/36501/#comment145335 Do you mean ident of parameters? - Klaus Ma On July 15, 2015, 2:59 a.m., Klaus Ma wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36501/ --- (Updated July 15, 2015, 2:59 a.m.) Review request for mesos. Bugs: MESOS-3023 https://issues.apache.org/jira/browse/MESOS-3023 Repository: mesos Description --- Fix for MESOS-3023 (Factoring out the pattern for URL generation) Diffs - src/tests/fetcher_tests.cpp ae10c420f7dddb8650377c91b5343591e8560392 src/tests/utils.hpp f2eed2e6fbc2cc8772c642bba976b25b426784e8 Diff: https://reviews.apache.org/r/36501/diff/ Testing --- 1. Build successfully in Linux 2. Test passed by src/mesos-tests --gtest_filter=FetcherTest.OSNetUriTest Thanks, Klaus Ma
Review Request 36510: Update .gitignore with intermediate build artifacts.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36510/ --- Review request for mesos and Benjamin Hindman. Bugs: MESOS-3054 https://issues.apache.org/jira/browse/MESOS-3054 Repository: mesos Description --- Update .gitignore with intermediate build artifacts. Diffs - .gitignore-template 934cad71a370d5c64d9a0994f775f39db74f5851 Diff: https://reviews.apache.org/r/36510/diff/ Testing --- In a dirty build tree, run ```git status``` and note the reduced clutter. Thanks, James Peach
