Re: Review Request 46146: Fixed libprocess tests to use smart pointers.

[Benjamin Bannier]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/46146/#review128864
---


Ship it!




- Benjamin Bannier


On April 13, 2016, 4:24 p.m., Neil Conway wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/46146/
> ---
> 
> (Updated April 13, 2016, 4:24 p.m.)
> 
> 
> Review request for mesos, Benjamin Bannier and Joris Van Remoortere.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This ensures that dynamically allocated memory is released when
> a test aborts prematurely due to an EXPECT_XXX failure.
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/src/tests/decoder_tests.cpp 
> bd990c5eb77e47d7f617199bcc89e9432af7cc51 
>   3rdparty/libprocess/src/tests/process_tests.cpp 
> 274a76fa61a5cd4b1324be124f73979d4b980660 
>   3rdparty/libprocess/src/tests/sequence_tests.cpp 
> 566393fbf3f19644b86843f07194d1de36a2015e 
> 
> Diff: https://reviews.apache.org/r/46146/diff/
> 
> 
> Testing
> ---
> 
> make check on OSX and Linux.
> 
> 
> Thanks,
> 
> Neil Conway
> 
>

Review Request 46189: Slave rename - Update strings in error messages and other strings.

[zhou xing]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/46189/
---

Review request for mesos, Kevin Klues and Vinod Kone.


Bugs: MESOS-5057
https://issues.apache.org/jira/browse/MESOS-5057


Repository: mesos


Description
---

[#MESOS-5057]
This patch renames 'slave' to 'agent' in the following strings:
1. Error/Warning messages
2. Flag help/description messages
3. Test case messages
4. Other standard output messages


Diffs
-

  CHANGELOG 1e07c8c2de8eff87c171378ef207c91a20d435d9 
  src/common/resources_utils.cpp 70e6f025d89383084ab8b2cda23ab1cd55d959b2 
  src/examples/dynamic_reservation_framework.cpp 
8f00bcf50c25cf46c3dc32e3e77370b39fbd46bc 
  src/hook/manager.cpp 692b9ea862442f3bac58da678425f03e4b00a79d 
  src/internal/devolve.cpp 0f58dc151c3de5a25d0ca73030a8222cac0f43c1 
  src/local/flags.hpp b3cd811c78bb7b57d82d755e36a89269d8173c2d 
  src/logging/flags.cpp 446eb921443481f4025132653e0bfa8ab42aa240 
  src/master/flags.cpp e522499586b731d522180f171731a9dd38b8344c 
  src/master/http.cpp b8a83b58b60416f61610cad16fc6f70028a5ee10 
  src/master/master.hpp 1f480f03900a0dc2996c2ed3a9534dfa8036940f 
  src/master/master.cpp 781402c04fded159183e1ca28894e48355200f0c 
  src/master/validation.cpp 13423436a4e6361fde6fa75133eebf5c02c8381f 
  src/python/executor/src/mesos/executor/mesos_executor_driver_impl.cpp 
843771ae5b4f7e283d85c093e63c235dc753397d 
  src/python/scheduler/src/mesos/scheduler/mesos_scheduler_driver_impl.cpp 
78dc298ec19a61cd491b2b43b463db67528c5526 
  src/scaling/scaling_sched.py d1008c79b1f1302d9ff6ae6dc9e1d9d0b7379773 
  src/sched/sched.cpp 5f6f5518f0858c680dc0dffc933c0bb03bba6991 
  src/slave/containerizer/mesos/isolators/network/port_mapping.cpp 
4f3f210b8d0ab9a453ab56c5e23024e2ab7c4259 
  src/slave/flags.cpp 316feec26e52a8cb1f0fd0739176b3f502e1ed86 
  src/slave/http.cpp 922aaad6e83ca9d5ab503ddb733a332982843300 
  src/slave/main.cpp 70df4f384b09a1fb078cd893efe52a5e3b116f48 
  src/slave/slave.cpp 49fa4a06e26d0d4475ed50db254a320a7030f896 
  src/slave/state.cpp e7b44c78500e07f0f1655f41a6a977adae2ca0c0 
  src/tests/cluster.cpp b4d69106388892b88f7de20b248cac8b950b861c 
  src/tests/containerizer/docker_containerizer_tests.cpp 
7accd32fba5eed196a82b1a171cb16d37b9e0539 
  src/tests/containerizer/filesystem_isolator_tests.cpp 
51e60c2e8c6c8b76b51de0e7761ecbb0ca9c3304 
  src/tests/flags.hpp af15360491b5433dcfbda03a72407667eb3977d1 
  src/tests/hierarchical_allocator_tests.cpp 
8ed0df45fc745d338482d1944a346cd40c18bb37 
  src/tests/master_allocator_tests.cpp 17607df7d488c8dd42c57504a5ca326697f57ffa 
  src/tests/master_tests.cpp 1ae72600e26c5f31476f689cbb600d41d7cc9b74 
  src/tests/paths_tests.cpp 81498e368cbb77e2cb8af71d8570dc690d3d0dfc 
  src/tests/slave_recovery_tests.cpp 79132344be3bcd2bda54357cd5e7e0c59a766fd8 
  src/tests/sorter_tests.cpp 0f3266f1222163c4d03eb4c4ca88f96836de601e 

Diff: https://reviews.apache.org/r/46189/diff/


Testing
---

make
make check


Thanks,

zhou xing

Re: Review Request 46165: Removed dependency on Boost.Foreach.

[Benjamin Bannier]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/46165/#review128851
---



LGTM and is a very nice C++11 update in the spirit of the original 
`BOOST_FOREACH` (e.g., the creative use of 6.4.3 :D).

Would you mind updating `stout/tests/foreach_tests.cpp` as well? It currently 
doesn't seem to test this, and it also should be updated to test e.g., 
iterating rvalues, or move-only key or value types.


3rdparty/libprocess/3rdparty/stout/include/stout/foreach.hpp (line 19)


Not necessarily an issue, but this does include a lot of preprocessor 
definitions just to have `BOOST_PP_CAT` from `boost/preprocessor/cat.hpp` 
available. I think we could potentially make this more light-weight by directly 
using `BOOST_PP_CAT` instead of `CAT`.



3rdparty/libprocess/3rdparty/stout/include/stout/foreach.hpp (line 66)


This one is subtle as in principle we could use a moved-from 
`FOREACH_ELEM`. This happens for cases where the `begin_expr` returns an rvalue 
in violation of general iterator requirements (e.g., that the result of `*it` 
is assignable).

Could you explain why we'd need to `forward` here and above in the first 
place? I appears we should be able to just `get` directly from the 
`FOREACH_ELEM` in scope.


- Benjamin Bannier


On April 13, 2016, 11:58 p.m., Michael Park wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/46165/
> ---
> 
> (Updated April 13, 2016, 11:58 p.m.)
> 
> 
> Review request for mesos, Benjamin Bannier, Benjamin Hindman, Ben Mahler, and 
> Joris Van Remoortere.
> 
> 
> Bugs: MESOS-3214
> https://issues.apache.org/jira/browse/MESOS-3214
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> See summary.
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/3rdparty/stout/include/stout/foreach.hpp 
> 7fb0044790ee249b69e07b81a26851bd5bfb110f 
> 
> Diff: https://reviews.apache.org/r/46165/diff/
> 
> 
> Testing
> ---
> 
> `make check`
> 
> 
> Thanks,
> 
> Michael Park
> 
>

Re: Review Request 46188: Added the missing 'break' when handling ERROR event.

[Mesos ReviewBot]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/46188/#review128853
---



Patch looks great!

Reviews applied: [46187, 46188]

Passed command: export OS='ubuntu:14.04' CONFIGURATION='--verbose' 
COMPILER='gcc' ENVIRONMENT='GLOG_v=1 MESOS_VERBOSE=1'; ./support/docker_build.sh

- Mesos ReviewBot


On April 14, 2016, 7:02 a.m., Qian Zhang wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/46188/
> ---
> 
> (Updated April 14, 2016, 7:02 a.m.)
> 
> 
> Review request for mesos, Anand Mazumdar and Vinod Kone.
> 
> 
> Bugs: MESOS-3558
> https://issues.apache.org/jira/browse/MESOS-3558
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added the missing 'break' when handling ERROR event.
> 
> 
> Diffs
> -
> 
>   src/launcher/http_command_executor.cpp 
> ad484e0e6f5067b6c166111c91b1ff1e8c05d9ac 
> 
> Diff: https://reviews.apache.org/r/46188/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Qian Zhang
> 
>

Re: Review Request 46013: Stout: Implemented `os::processes` on Windows.

[Alex Clemmer]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/46013/
---

(Updated April 14, 2016, 8:57 a.m.)


Review request for mesos, Alex Naparu, Daniel Pravat, Artem Harutyunyan, Joris 
Van Remoortere, Michael Park, M Lawindi, and Yi Sun.


Bugs: MESOS-4471
https://issues.apache.org/jira/browse/MESOS-4471


Repository: mesos


Description
---

Stout: Implemented `os::processes` on Windows.


Diffs (updated)
-

  3rdparty/libprocess/3rdparty/stout/include/stout/os.hpp 
79e30ca04c6d23f92e3a2f80fbe38ae63fde3520 
  3rdparty/libprocess/3rdparty/stout/include/stout/posix/os.hpp 
edaa76a5322d0bf60b7172405aa754b5aca95458 
  3rdparty/libprocess/3rdparty/stout/include/stout/windows/os.hpp 
c48106e5905e3be0faeba7177ef534766089faff 

Diff: https://reviews.apache.org/r/46013/diff/


Testing
---


Thanks,

Alex Clemmer

Re: Review Request 46193: Stout:[2/2] Added `systems_tests.cpp`.

[Mesos ReviewBot]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/46193/#review128865
---



Bad patch!

Reviews applied: [46193, 46192, 46191, 46015, 46014, 46013, 46012, 43985]

Failed command: ./support/apply-review.sh -n -r 43985

Error:
2016-04-14 09:51:51 URL:https://reviews.apache.org/r/43985/diff/raw/ 
[1185/1185] -> "43985.patch" [1]
3rdparty/libprocess/3rdparty/stout/include/stout/os/windows/sendfile.hpp:36:  
Missing space after ,  [whitespace/comma] [3]
Total errors found: 1
Checking 1 files

Full log: https://builds.apache.org/job/mesos-reviewbot/12521/console

- Mesos ReviewBot


On April 14, 2016, 8:28 a.m., Alex Clemmer wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/46193/
> ---
> 
> (Updated April 14, 2016, 8:28 a.m.)
> 
> 
> Review request for mesos, Alex Naparu, Daniel Pravat, Artem Harutyunyan, 
> Joris Van Remoortere, Michael Park, M Lawindi, and Yi Sun.
> 
> 
> Bugs: MESOS-4470
> https://issues.apache.org/jira/browse/MESOS-4470
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Stout:[2/2] Added `systems_tests.cpp`.
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/3rdparty/Makefile.am 
> 217283cd3ce6df699b63dc5b8fb3aab0c6debd04 
> 
> Diff: https://reviews.apache.org/r/46193/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Alex Clemmer
> 
>

Review Request 46212: Added documentation around using AuthN for HTTP frameworks.

[Anand Mazumdar]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/46212/
---

Review request for mesos and Vinod Kone.


Bugs: MESOS-3923
https://issues.apache.org/jira/browse/MESOS-3923


Repository: mesos


Description
---

This change adds config docs around authenticating HTTP
frameworks using the newly introduced flags. Also added a
small note to the `authenticate` flag that this does not
work for HTTP based frameworks.


Diffs
-

  docs/configuration.md ba00ec563c449345effb3114111812601addcfc2 

Diff: https://reviews.apache.org/r/46212/diff/


Testing
---

make check


Thanks,

Anand Mazumdar

Re: Review Request 46118: Fixed tests impacted by enabling AuthN for HTTP frameworks.

[Anand Mazumdar]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/46118/
---

(Updated April 14, 2016, 3:39 p.m.)


Review request for mesos and Vinod Kone.


Changes
---

Added the `authenticated_http_frameworks` flag to the tests.


Bugs: MESOS-4459
https://issues.apache.org/jira/browse/MESOS-4459


Repository: mesos


Description
---

See summary.


Diffs (updated)
-

  src/tests/master_maintenance_tests.cpp 
31d6c70e7ecb584c75404556508dd9cdcf63fb78 
  src/tests/master_quota_tests.cpp e4a65bf09c8fdd2d6b6161042c3702a8cc4cd454 
  src/tests/mesos.hpp 20370a277d55efeea8daae7ea5e2f6575b5a2d62 
  src/tests/mesos.cpp 1b7a8fd703b90d77ffa3df079bdc2744752caac6 
  src/tests/scheduler_http_api_tests.cpp 
d469adf7230ce5bb5e71a241a06e389295905e03 
  src/tests/scheduler_tests.cpp b630944b1d3163345e912cb2bfc3301daa4c6362 

Diff: https://reviews.apache.org/r/46118/diff/


Testing
---

make check


Thanks,

Anand Mazumdar

Re: Review Request 46204: Updated slave recovery tests with HTTP command executor.

[Vinod Kone]

Can you run the tests in a loop (100 iterations) to check for flakiness?

@vinodkone

> On Apr 14, 2016, at 8:14 AM, Qian Zhang  wrote:
> 
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/46204/
> ---
> 
> Review request for mesos, Anand Mazumdar and Vinod Kone.
> 
> 
> Bugs: MESOS-3558
>https://issues.apache.org/jira/browse/MESOS-3558
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Updated slave recovery tests with HTTP command executor.
> 
> 
> Diffs
> -
> 
>  src/tests/slave_recovery_tests.cpp 79132344be3bcd2bda54357cd5e7e0c59a766fd8 
> 
> Diff: https://reviews.apache.org/r/46204/diff/
> 
> 
> Testing
> ---
> 
> $ sudo make check GTEST_FILTER="SlaveRecoveryTest*HTTPExecutor"
> ...
> 
> [==] Running 4 tests from 1 test case.
> [--] Global test environment set-up.
> [--] 4 tests from SlaveRecoveryTest/0, where TypeParam = 
> mesos::internal::slave::MesosContainerizer
> [ RUN  ] SlaveRecoveryTest/0.ReconnectHTTPExecutor
> I0414 23:13:11.715150  1989 executor.cpp:174] Version: 0.29.0
> Received SUBSCRIBED event
> Subscribed executor on mesos
> Received LAUNCH event
> Starting task bf66407c-b780-4bcc-b8de-6d30fe7b2660
> Forked command at 1997
> sh -c 'sleep 1000'
> Received ERROR event
> Error: Received unexpected '500 Internal Server Error' () for UPDATE
> E0414 23:13:11.819635  1994 executor.cpp:663] End-Of-File received from 
> agent. The agent closed the event stream
> Received ERROR event
> Error: Received unexpected '400 Bad Request' () for SUBSCRIBE
> Received SUBSCRIBED event
> Subscribed executor on mesos
> Received ACKNOWLEDGED event
> Received SHUTDOWN event
> Shutting down
> Sending SIGTERM to process tree at pid 1997
> [   OK ] SlaveRecoveryTest/0.ReconnectHTTPExecutor (3418 ms)
> [ RUN  ] SlaveRecoveryTest/0.RecoverUnregisteredHTTPExecutor
> I0414 23:13:15.050401  2021 executor.cpp:174] Version: 0.29.0
> Received ERROR event
> Error: Received unexpected '500 Internal Server Error' () for SUBSCRIBE
> [   OK ] SlaveRecoveryTest/0.RecoverUnregisteredHTTPExecutor (1502 ms)
> [ RUN  ] SlaveRecoveryTest/0.CleanupHTTPExecutor
> I0414 23:13:16.502077  2081 executor.cpp:174] Version: 0.29.0
> Received SUBSCRIBED event
> Subscribed executor on mesos
> Received LAUNCH event
> Starting task 1efe8ac8-d2d5-46d3-99e5-b09cfbab269d
> Forked command at 2088
> sh -c 'sleep 1000'
> Received ERROR event
> Error: Received unexpected '500 Internal Server Error' () for UPDATE
> E0414 23:13:16.604841  2080 executor.cpp:663] End-Of-File received from 
> agent. The agent closed the event stream
> Received ERROR event
> Error: Received unexpected '400 Bad Request' () for SUBSCRIBE
> [   OK ] SlaveRecoveryTest/0.CleanupHTTPExecutor (2348 ms)
> [ RUN  ] SlaveRecoveryTest/0.KillTaskWithHTTPExecutor
> I0414 23:13:18.936816  2130 executor.cpp:174] Version: 0.29.0
> Received SUBSCRIBED event
> Subscribed executor on mesos
> Received LAUNCH event
> Starting task f399801f-0f78-4458-8485-a50a2f082c45
> Forked command at 2133
> sh -c 'sleep 1000'
> Received ACKNOWLEDGED event
> E0414 23:13:19.176312  2127 executor.cpp:663] End-Of-File received from 
> agent. The agent closed the event stream
> Received ERROR event
> Error: Received unexpected '400 Bad Request' () for SUBSCRIBE
> Received SUBSCRIBED event
> Subscribed executor on mesos
> Received KILL event
> Received kill for task f399801f-0f78-4458-8485-a50a2f082c45
> Sending SIGTERM to process tree at pid 2133
> Sent SIGTERM to the following process trees:
> [ 
> -+- 2133 sh -c sleep 1000 
> --- 2134 sleep 1000 
> ]
> Command terminated with signal Terminated (pid: 2133)
> Received ACKNOWLEDGED event
> [   OK ] SlaveRecoveryTest/0.KillTaskWithHTTPExecutor (3725 ms)
> [--] 4 tests from SlaveRecoveryTest/0 (11033 ms total)
> 
> [--] Global test environment tear-down
> [==] 4 tests from 1 test case ran. (11052 ms total)
> [  PASSED  ] 4 tests.
> 
> 
> Thanks,
> 
> Qian Zhang
> 

Re: Review Request 45670: Updated tests for HTTP command executor.

[Vinod Kone]

Anand might have ideas. If we can get its os pid we could do kill on it?

@vinodkone

> On Apr 14, 2016, at 8:21 AM, Qian Zhang  wrote:
> 
> 
> 
>>> On April 13, 2016, 5:11 a.m., Vinod Kone wrote:
>>> Are you also planning to update slave recovery tests? Those are the most 
>>> crucial.
>> 
>> Qian Zhang wrote:
>>Sure, I will update slave recovery tests soon. Just want to double 
>> confirm, in `slave_recovery_tests.cpp`, I see there are two TODOs related to 
>> HTTP based command executor in two test cases:
>>
>> https://github.com/apache/mesos/blob/0.28.0/src/tests/slave_recovery_tests.cpp#L462
>>
>> https://github.com/apache/mesos/blob/0.28.0/src/tests/slave_recovery_tests.cpp#L1082
>> 
>>You mean I need to update these two test cases with the HTTP command 
>> executor I implemented in MESOS-3558, right?
>> 
>> Vinod Kone wrote:
>>Definitely those two and the one we talked about in the earlier review.
>> 
>>It would also be good to add the following tests for HTTP command 
>> executor. There are already tests for this for driver based command executor.
>> 
>>--> RecoverUnregisteredHTTPExecutor 
>>--> RecoverTerminatedExecutor
>>--> KillTask
> 
> OK, I have posted a patch (https://reviews.apache.org/r/46204/) to update 
> those two tests and add two more (`RecoverUnregisteredHTTPExecutor` and 
> `KillTaskWithHTTPExecutor`) in slave recovery tests, and I will post a 
> separate patch for adding the test that slave restarts with flag change.
> 
> But for the test `RecoverTerminatedExecutor`, I have a question: we need to 
> shutdown the executor when agent is down, for the driver based executor, I 
> see we use `process::post(executorPid, ShutdownExecutorMessage())`, but for 
> HTTP based executor, I am not sure how to shutdown it in the test, any 
> suggestions? Thanks.
> 
> 
> - Qian
> 
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/45670/#review128535
> ---
> 
> 
>> On April 13, 2016, 5:15 p.m., Qian Zhang wrote:
>> 
>> ---
>> This is an automatically generated e-mail. To reply, visit:
>> https://reviews.apache.org/r/45670/
>> ---
>> 
>> (Updated April 13, 2016, 5:15 p.m.)
>> 
>> 
>> Review request for mesos, Anand Mazumdar and Vinod Kone.
>> 
>> 
>> Bugs: MESOS-3558
>>https://issues.apache.org/jira/browse/MESOS-3558
>> 
>> 
>> Repository: mesos
>> 
>> 
>> Description
>> ---
>> 
>> Updated tests for HTTP command executor.
>> 
>> 
>> Diffs
>> -
>> 
>>  src/tests/command_executor_tests.cpp 
>> 843233adaa68ab1f5cedb7b075439bb8b77469a8 
>> 
>> Diff: https://reviews.apache.org/r/45670/diff/
>> 
>> 
>> Testing
>> ---
>> 
>> 
>> Thanks,
>> 
>> Qian Zhang
> 

Re: Review Request 44454: Checkpointed the external mount info for container.

[Guangya Liu]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/44454/
---

(Updated 四月 14, 2016, 3:45 p.m.)


Review request for mesos, David vonThenen, Gilbert Song, haosdent huang, and 
Jie Yu.


Bugs: MESOS-5104
https://issues.apache.org/jira/browse/MESOS-5104


Repository: mesos


Description
---

Checkpointed the external mount info for container.


Diffs (updated)
-

  src/slave/containerizer/mesos/isolators/docker/volume/isolator.hpp 
PRE-CREATION 
  src/slave/containerizer/mesos/isolators/docker/volume/isolator.cpp 
PRE-CREATION 

Diff: https://reviews.apache.org/r/44454/diff/


Testing
---

make
make check


Thanks,

Guangya Liu

Re: Review Request 45360: Added volume client for mount and unmount.

[Gilbert Song]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/45360/#review128931
---




src/slave/containerizer/mesos/isolators/docker/volume/volume_client.cpp (line 
57)


How about using:

`const stirng&`



src/slave/containerizer/mesos/isolators/docker/volume/volume_client.cpp (line 
58)


ditto.



src/slave/containerizer/mesos/isolators/docker/volume/volume_client.cpp (lines 
64 - 81)


It is still the issue I mentioned above:

Since we have to specify `--volumedriver` and `--volumename`, and possibly 
have a bunch of `--volumeopt` appended. I would suggest collecting all this 
arguments (e.g., {dvdcliPath, "mount", "--volumedriver=", ...}) into a 
`vector argv;`.

and in subprocess, if you grep the examples `Try`, your can see 
for most cases we have cmd and argv separated. Considering the cmd may not be 
short. Let's make it more clear:

```
  Try s = subprocess(
  dvdcliPath,
  argv,
  Subprocess::PATH("/dev/null"),
  Subprocess::PIPE(),
  Subprocess::PIPE());
```



src/slave/containerizer/mesos/isolators/docker/volume/volume_client.cpp (line 
87)


Let's following the pattern in CNI isolator by using await here and passing 
the `tuple` to `launch()` instead of passing a subprocess.



src/slave/containerizer/mesos/isolators/docker/volume/volume_client.cpp (line 
104)


ditto.



src/slave/containerizer/mesos/isolators/docker/volume/volume_client.cpp (lines 
128 - 150)


Understand that you want to simplify the logic for mount and unmount here. 
Let us discuss this comparing the logic in CNI isolator `attach()` and 
`detach()` V.S. passing the reference of subprocess to launch.


- Gilbert Song


On April 13, 2016, 11:05 p.m., Guangya Liu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/45360/
> ---
> 
> (Updated April 13, 2016, 11:05 p.m.)
> 
> 
> Review request for mesos, David vonThenen, Gilbert Song, haosdent huang, and 
> Jie Yu.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added volume client for mount and unmount.
> 
> 
> Diffs
> -
> 
>   src/CMakeLists.txt ca59a1824352423f4db6ef8bb41acc6fe602c041 
>   src/Makefile.am 139935fb40f8986427c85537a400ff945e54433f 
>   src/slave/containerizer/mesos/isolators/docker/volume/volume_client.hpp 
> PRE-CREATION 
>   src/slave/containerizer/mesos/isolators/docker/volume/volume_client.cpp 
> PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/45360/diff/
> 
> 
> Testing
> ---
> 
> make
> make check
> 
> 
> Thanks,
> 
> Guangya Liu
> 
>

Re: Review Request 46009: Stout:[1/2] Added simple tests for `os::` functions.

[Joris Van Remoortere]

> On April 12, 2016, 3:09 p.m., Joris Van Remoortere wrote:
> > 3rdparty/libprocess/3rdparty/stout/tests/os/env_tests.cpp, lines 26-27
> > 
> >
> > Please use plain names that are self descriptive.
> 
> Alex Clemmer wrote:
> I'm not sure whether you're talking about the symbol `key` or the string 
> that is its value so I made the first more descriptive and the second more 
> boring. Christmas comes early to the Van Remoortere household!

How about:
```
string key = "key";
string value = "value";
string new_value = "new_value";
```


- Joris


---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/46009/#review128420
---


On April 13, 2016, 10:05 p.m., Alex Clemmer wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/46009/
> ---
> 
> (Updated April 13, 2016, 10:05 p.m.)
> 
> 
> Review request for mesos, Alex Naparu, Daniel Pravat, Artem Harutyunyan, 
> Joris Van Remoortere, Michael Park, M Lawindi, and Yi Sun.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Stout:[1/2] Added simple tests for `os::` functions.
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/3rdparty/stout/Makefile.am 
> 400c6dc451602926f93b22713af8c66d7ca59ca6 
>   3rdparty/libprocess/3rdparty/stout/tests/CMakeLists.txt 
> c9d331df2f4496183b5734d2434413f68b9c1b4b 
>   3rdparty/libprocess/3rdparty/stout/tests/os/env_tests.cpp PRE-CREATION 
>   3rdparty/libprocess/3rdparty/stout/tests/os/strerror_tests.cpp 
> d5a96ad6b8b1c09b4f016e0c8e3e5c5672b55ef3 
> 
> Diff: https://reviews.apache.org/r/46009/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Alex Clemmer
> 
>

Re: Review Request 45991: Fixed memory leak in Route::Route() in libprocess.

[Joseph Wu]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/45991/#review128954
---


Ship it!




Ship It!

- Joseph Wu


On April 10, 2016, 6:38 p.m., Neil Conway wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/45991/
> ---
> 
> (Updated April 10, 2016, 6:38 p.m.)
> 
> 
> Review request for mesos and Joris Van Remoortere.
> 
> 
> Bugs: MESOS-5144
> https://issues.apache.org/jira/browse/MESOS-5144
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Fixed memory leak in Route::Route() in libprocess.
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/src/process.cpp 
> 5e9dcfdc52f3a8223bc43af149b8e1f5dbdf5b0a 
> 
> Diff: https://reviews.apache.org/r/45991/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Neil Conway
> 
>

Re: Review Request 40266: Libprocess Reinit: Cleanup SocketManager alongside ProcessManager.

[Joseph Wu]

> On Dec. 2, 2015, 8:08 a.m., Benjamin Bannier wrote:
> > 3rdparty/libprocess/src/process.cpp, line 993
> > 
> >
> > Once you use a proper smart pointer for `process_route` this comment 
> > will be right (maybe `s/deleted/cleaned up/`).

Note: This change was moved into a prior commit in the chain.


> On Dec. 2, 2015, 8:08 a.m., Benjamin Bannier wrote:
> > 3rdparty/libprocess/src/process.cpp, line 493
> > 
> >
> > You should be more explicit about lifetimes here and use `unique_ptrs` 
> > of .. instead (you can always `reset` in place of `delete` if you need to 
> > destroy at a certain point).

Note: This change was moved into a prior commit in the chain.


- Joseph


---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/40266/#review108650
---


On April 14, 2016, 1:28 p.m., Joseph Wu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/40266/
> ---
> 
> (Updated April 14, 2016, 1:28 p.m.)
> 
> 
> Review request for mesos, Artem Harutyunyan and Joris Van Remoortere.
> 
> 
> Bugs: MESOS-3910
> https://issues.apache.org/jira/browse/MESOS-3910
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> The `SocketManager` and `ProcessManager` are highly inter-dependent, 
> which requires some untangling in `process::finalize`.
> 
> * Logic originally found in `~ProcessManager` has been split into 
>   `ProcessManager::finalize` due to what happens during cleanup.
> * The future from `__s__->accept()` must be explicitly discarded as 
>   libevent does not detect a locally closed socket.
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/src/process.cpp 
> afeddec20495bb9621c3e26b0d425c9419654739 
> 
> Diff: https://reviews.apache.org/r/40266/diff/
> 
> 
> Testing
> ---
> 
> `make check` (libev)
> `make check` (--enable-libevent --enable-ssl)
> 
> 
> Thanks,
> 
> Joseph Wu
> 
>

Re: Review Request 46246: Log executor commands w/o verbose logs enabled.

[Mesos ReviewBot]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/46246/#review129066
---



Patch looks great!

Reviews applied: [46246]

Passed command: export OS='ubuntu:14.04' CONFIGURATION='--verbose' 
COMPILER='gcc' ENVIRONMENT='GLOG_v=1 MESOS_VERBOSE=1'; ./support/docker_build.sh

- Mesos ReviewBot


On April 15, 2016, 2:30 a.m., Yong Tang wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/46246/
> ---
> 
> (Updated April 15, 2016, 2:30 a.m.)
> 
> 
> Review request for mesos, Artem Harutyunyan and Joseph Wu.
> 
> 
> Bugs: MESOS-5197
> https://issues.apache.org/jira/browse/MESOS-5197
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This patch changes the logging in docker.cpp from VLOG to
> LOG(INFO). The reason is that, the command line that has
> been executed by executor is useful in debug purposes.
> 
> 
> Diffs
> -
> 
>   src/docker/docker.cpp 19cf424dfd5748ae66a7023840aa2b0652e8f2c0 
> 
> Diff: https://reviews.apache.org/r/46246/diff/
> 
> 
> Testing
> ---
> 
> make check (Ubuntu 14.04)
> 
> 
> Thanks,
> 
> Yong Tang
> 
>

Re: Review Request 46008: Stout: Initialize Windows socket stack in Stout tests.

[Alex Clemmer]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/46008/
---

(Updated April 15, 2016, 5:58 a.m.)


Review request for mesos, Alex Naparu, Daniel Pravat, Artem Harutyunyan, Joris 
Van Remoortere, Michael Park, M Lawindi, and Yi Sun.


Repository: mesos


Description
---

Stout: Initialize Windows socket stack in Stout tests.


Diffs (updated)
-

  3rdparty/libprocess/3rdparty/stout/include/stout/os/windows/socket.hpp 
b5aeab75c2365a3431c397743b95ce7fbd6d7a1a 
  3rdparty/libprocess/3rdparty/stout/tests/main.cpp 
c449de87bede4d3bf1df368eaf1d22f857c2298f 

Diff: https://reviews.apache.org/r/46008/diff/


Testing
---


Thanks,

Alex Clemmer

Re: Review Request 46149: Speed up DynamicReservationFramework.

[Klaus Ma]

> On April 14, 2016, 7:56 p.m., Benjamin Bannier wrote:
> > This is much better, but still long. I see that now most of the time is 
> > spent in `resourceOffers` -- can you think of a way to speed these up as 
> > well?

@Bannier, how did you check the time in `resourceOffers`?


- Klaus


---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/46149/#review128877
---


On April 14, 2016, 8:59 p.m., Klaus Ma wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/46149/
> ---
> 
> (Updated April 14, 2016, 8:59 p.m.)
> 
> 
> Review request for mesos, Benjamin Bannier and Michael Park.
> 
> 
> Bugs: MESOS-5166
> https://issues.apache.org/jira/browse/MESOS-5166
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> **Phenomenon**: The runtime of "ExamplesTest.DynamicReservationFramework" is 
> ~13s
> **Root Cause**: In dynamic reservation example, the framework will accept 
> offer when reserving resources, launching tasks and unreserving tasks. The 
> un-used resources will return to allocator with 5s RefusedFilters. It impact 
> the performance of this example.
> **How to Fix**: Set filters.refused_timeout to zero when launching task, 
> reserving/unreserving resources.
> 
> 
> Diffs
> -
> 
>   src/examples/dynamic_reservation_framework.cpp 
> 8f00bcf50c25cf46c3dc32e3e77370b39fbd46bc 
> 
> Diff: https://reviews.apache.org/r/46149/diff/
> 
> 
> Testing
> ---
> 
> make && make check GTEST_FILTER="ExamplesTest.*"
> 
> [==] Running 5 tests from 1 test case.
> [--] Global test environment set-up.
> [--] 5 tests from ExamplesTest
> [ RUN  ] ExamplesTest.TestFramework
> [   OK ] ExamplesTest.TestFramework (5252 ms)
> [ RUN  ] ExamplesTest.NoExecutorFramework
> [   OK ] ExamplesTest.NoExecutorFramework (5407 ms)
> [ RUN  ] ExamplesTest.TestHTTPFramework
> [   OK ] ExamplesTest.TestHTTPFramework (1265 ms)
> [ RUN  ] ExamplesTest.PersistentVolumeFramework
> [   OK ] ExamplesTest.PersistentVolumeFramework (3379 ms)
> [ RUN  ] ExamplesTest.DynamicReservationFramework
> [   OK ] ExamplesTest.DynamicReservationFramework (5127 ms)
> [--] 5 tests from ExamplesTest (20433 ms total)
> 
> [--] Global test environment tear-down
> [==] 5 tests from 1 test case ran. (20443 ms total)
> [  PASSED  ] 5 tests.
> 
> 
> Thanks,
> 
> Klaus Ma
> 
>

Re: Review Request 46149: Speed up DynamicReservationFramework.

[Benjamin Bannier]

> On April 14, 2016, 1:56 p.m., Benjamin Bannier wrote:
> > This is much better, but still long. I see that now most of the time is 
> > spent in `resourceOffers` -- can you think of a way to speed these up as 
> > well?
> 
> Klaus Ma wrote:
> @Bannier, how did you check the time in `resourceOffers`?

I looked at the verbose log -- and obviously cannot convert microseconds to 
seconds (duh). The time seems to be spent elsewhere.


- Benjamin


---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/46149/#review128877
---


On April 14, 2016, 2:59 p.m., Klaus Ma wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/46149/
> ---
> 
> (Updated April 14, 2016, 2:59 p.m.)
> 
> 
> Review request for mesos, Benjamin Bannier and Michael Park.
> 
> 
> Bugs: MESOS-5166
> https://issues.apache.org/jira/browse/MESOS-5166
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> **Phenomenon**: The runtime of "ExamplesTest.DynamicReservationFramework" is 
> ~13s
> **Root Cause**: In dynamic reservation example, the framework will accept 
> offer when reserving resources, launching tasks and unreserving tasks. The 
> un-used resources will return to allocator with 5s RefusedFilters. It impact 
> the performance of this example.
> **How to Fix**: Set filters.refused_timeout to zero when launching task, 
> reserving/unreserving resources.
> 
> 
> Diffs
> -
> 
>   src/examples/dynamic_reservation_framework.cpp 
> 8f00bcf50c25cf46c3dc32e3e77370b39fbd46bc 
> 
> Diff: https://reviews.apache.org/r/46149/diff/
> 
> 
> Testing
> ---
> 
> make && make check GTEST_FILTER="ExamplesTest.*"
> 
> [==] Running 5 tests from 1 test case.
> [--] Global test environment set-up.
> [--] 5 tests from ExamplesTest
> [ RUN  ] ExamplesTest.TestFramework
> [   OK ] ExamplesTest.TestFramework (5252 ms)
> [ RUN  ] ExamplesTest.NoExecutorFramework
> [   OK ] ExamplesTest.NoExecutorFramework (5407 ms)
> [ RUN  ] ExamplesTest.TestHTTPFramework
> [   OK ] ExamplesTest.TestHTTPFramework (1265 ms)
> [ RUN  ] ExamplesTest.PersistentVolumeFramework
> [   OK ] ExamplesTest.PersistentVolumeFramework (3379 ms)
> [ RUN  ] ExamplesTest.DynamicReservationFramework
> [   OK ] ExamplesTest.DynamicReservationFramework (5127 ms)
> [--] 5 tests from ExamplesTest (20433 ms total)
> 
> [--] Global test environment tear-down
> [==] 5 tests from 1 test case ran. (20443 ms total)
> [  PASSED  ] 5 tests.
> 
> 
> Thanks,
> 
> Klaus Ma
> 
>

Re: Review Request 46149: Speed up ExamplesTest.DynamicReservationFramework.

[Mesos ReviewBot]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/46149/#review128883
---



Bad patch!

Reviews applied: [46149]

Failed command: ./support/apply-review.sh -n -r 46149

Error:
2016-04-14 12:46:50 URL:https://reviews.apache.org/r/46149/diff/raw/ 
[1757/1757] -> "46149.patch" [1]
Total errors found: 0
Checking 1 files
Error: No line in the commit message summary may exceed 72 characters.

Full log: https://builds.apache.org/job/mesos-reviewbot/12524/console

- Mesos ReviewBot


On April 13, 2016, 3:20 p.m., Klaus Ma wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/46149/
> ---
> 
> (Updated April 13, 2016, 3:20 p.m.)
> 
> 
> Review request for mesos, Benjamin Bannier and Michael Park.
> 
> 
> Bugs: MESOS-5166
> https://issues.apache.org/jira/browse/MESOS-5166
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> **Phenomenon**: The runtime of "ExamplesTest.DynamicReservationFramework" is 
> ~13s
> **Root Cause**: In dynamic reservation example, the framework will accept 
> offer when reserving resources, launching tasks and unreserving tasks. The 
> un-used resources will return to allocator with 5s RefusedFilters. It impact 
> the performance of this example.
> **How to Fix**: Set filters.refused_timeout to zero when launching task, 
> reserving/unreserving resources.
> 
> 
> Diffs
> -
> 
>   src/examples/dynamic_reservation_framework.cpp 
> 8f00bcf50c25cf46c3dc32e3e77370b39fbd46bc 
> 
> Diff: https://reviews.apache.org/r/46149/diff/
> 
> 
> Testing
> ---
> 
> make && make check GTEST_FILTER="ExamplesTest.*"
> 
> [==] Running 5 tests from 1 test case.
> [--] Global test environment set-up.
> [--] 5 tests from ExamplesTest
> [ RUN  ] ExamplesTest.TestFramework
> [   OK ] ExamplesTest.TestFramework (5252 ms)
> [ RUN  ] ExamplesTest.NoExecutorFramework
> [   OK ] ExamplesTest.NoExecutorFramework (5407 ms)
> [ RUN  ] ExamplesTest.TestHTTPFramework
> [   OK ] ExamplesTest.TestHTTPFramework (1265 ms)
> [ RUN  ] ExamplesTest.PersistentVolumeFramework
> [   OK ] ExamplesTest.PersistentVolumeFramework (3379 ms)
> [ RUN  ] ExamplesTest.DynamicReservationFramework
> [   OK ] ExamplesTest.DynamicReservationFramework (5127 ms)
> [--] 5 tests from ExamplesTest (20433 ms total)
> 
> [--] Global test environment tear-down
> [==] 5 tests from 1 test case ran. (20443 ms total)
> [  PASSED  ] 5 tests.
> 
> 
> Thanks,
> 
> Klaus Ma
> 
>

Re: Review Request 46149: Speed up DynamicReservationFramework.

[Klaus Ma]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/46149/
---

(Updated April 14, 2016, 8:59 p.m.)


Review request for mesos, Benjamin Bannier and Michael Park.


Changes
---

Update summary.


Summary (updated)
-

Speed up DynamicReservationFramework.


Bugs: MESOS-5166
https://issues.apache.org/jira/browse/MESOS-5166


Repository: mesos


Description
---

**Phenomenon**: The runtime of "ExamplesTest.DynamicReservationFramework" is 
~13s
**Root Cause**: In dynamic reservation example, the framework will accept offer 
when reserving resources, launching tasks and unreserving tasks. The un-used 
resources will return to allocator with 5s RefusedFilters. It impact the 
performance of this example.
**How to Fix**: Set filters.refused_timeout to zero when launching task, 
reserving/unreserving resources.


Diffs
-

  src/examples/dynamic_reservation_framework.cpp 
8f00bcf50c25cf46c3dc32e3e77370b39fbd46bc 

Diff: https://reviews.apache.org/r/46149/diff/


Testing
---

make && make check GTEST_FILTER="ExamplesTest.*"

[==] Running 5 tests from 1 test case.
[--] Global test environment set-up.
[--] 5 tests from ExamplesTest
[ RUN  ] ExamplesTest.TestFramework
[   OK ] ExamplesTest.TestFramework (5252 ms)
[ RUN  ] ExamplesTest.NoExecutorFramework
[   OK ] ExamplesTest.NoExecutorFramework (5407 ms)
[ RUN  ] ExamplesTest.TestHTTPFramework
[   OK ] ExamplesTest.TestHTTPFramework (1265 ms)
[ RUN  ] ExamplesTest.PersistentVolumeFramework
[   OK ] ExamplesTest.PersistentVolumeFramework (3379 ms)
[ RUN  ] ExamplesTest.DynamicReservationFramework
[   OK ] ExamplesTest.DynamicReservationFramework (5127 ms)
[--] 5 tests from ExamplesTest (20433 ms total)

[--] Global test environment tear-down
[==] 5 tests from 1 test case ran. (20443 ms total)
[  PASSED  ] 5 tests.


Thanks,

Klaus Ma

Re: Review Request 46180: Implemented create() for docker volume isolator.

[Guangya Liu]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/46180/
---

(Updated 四月 14, 2016, 1:40 p.m.)


Review request for mesos, David vonThenen, Gilbert Song, haosdent huang, and 
Jie Yu.


Bugs: MESOS-5013
https://issues.apache.org/jira/browse/MESOS-5013


Repository: mesos


Description
---

Implemented create() for docker volume isolator.


Diffs (updated)
-

  src/slave/containerizer/mesos/isolators/docker/volume/isolator.hpp 
PRE-CREATION 
  src/slave/containerizer/mesos/isolators/docker/volume/isolator.cpp 
PRE-CREATION 

Diff: https://reviews.apache.org/r/46180/diff/


Testing
---

make
make check


Thanks,

Guangya Liu

Review Request 46214: Valided role when framework subscribe.

[Klaus Ma]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/46214/
---

Review request for mesos and Vinod Kone.


Bugs: MESOS-5184
https://issues.apache.org/jira/browse/MESOS-5184


Repository: mesos


Description
---

Valided role when framework subscribe.


Diffs
-

  src/master/master.cpp 781402c04fded159183e1ca28894e48355200f0c 
  src/tests/master_tests.cpp 1ae72600e26c5f31476f689cbb600d41d7cc9b74 
  src/tests/scheduler_http_api_tests.cpp 
d469adf7230ce5bb5e71a241a06e389295905e03 

Diff: https://reviews.apache.org/r/46214/diff/


Testing
---

make && make check


Thanks,

Klaus Ma

Re: Review Request 45955: Added CNI helper subcommand to `mesos-containerizer`.

[Jie Yu]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/45955/#review128907
---


Ship it!




Ship It!

- Jie Yu


On April 14, 2016, 3:24 p.m., Avinash sridharan wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/45955/
> ---
> 
> (Updated April 14, 2016, 3:24 p.m.)
> 
> 
> Review request for mesos, Jie Yu and Qian Zhang.
> 
> 
> Bugs: MESOS-4922
> https://issues.apache.org/jira/browse/MESOS-4922
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This will be used by the `network/cni` isolator to setup hostname and
> various network files within the container UTS and mnt namespace.
> 
> 
> Diffs
> -
> 
>   src/slave/containerizer/mesos/main.cpp 
> 63b257e5ea079d516c0cbed9593a116f22677255 
> 
> Diff: https://reviews.apache.org/r/45955/diff/
> 
> 
> Testing
> ---
> 
> make
> 
> * Ran mesos-execute with the a single master/slave setup that the container 
> attaches to the CNI network correctly and that its network files and hostname 
> are setup correctly.
> 
> 
> Thanks,
> 
> Avinash sridharan
> 
>

Re: Review Request 45983: Enabled the `network/cni` isolator in `MesosContainerizer`.

[Jie Yu]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/45983/#review128908
---


Ship it!




Ship It!

- Jie Yu


On April 14, 2016, 3:26 p.m., Avinash sridharan wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/45983/
> ---
> 
> (Updated April 14, 2016, 3:26 p.m.)
> 
> 
> Review request for mesos, Jie Yu and Qian Zhang.
> 
> 
> Bugs: MESOS-5130
> https://issues.apache.org/jira/browse/MESOS-5130
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Enabled the `network/cni` isolator in `MesosContainerizer`.
> 
> 
> Diffs
> -
> 
>   src/slave/containerizer/mesos/containerizer.cpp 
> dfaffd79b807b50c61e1d1a6a0c1b0c99f5df693 
> 
> Diff: https://reviews.apache.org/r/45983/diff/
> 
> 
> Testing
> ---
> 
> make (Ubuntu 14.04 and OS X)
> 
> * Ran the Agent without the `network/cni` enabled in the `--isolation` flags. 
> Without the `--network_cni_plugins_dir` and the `--network_cni_config_dir`, 
> container using a docker image was launched on the host-network. With the 
> `--network_cni_plugins_dir` and the `--network_cni_config_dir` specified the 
> CNI network isolator correctly invoked the specified plugin to given the 
> container its own network namespace.
> 
> 
> Thanks,
> 
> Avinash sridharan
> 
>

Re: Review Request 45214: Updated protobuf to support external storage.

[Jie Yu]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/45214/#review128913
---


Ship it!




Ship It!

- Jie Yu


On April 14, 2016, 4:37 a.m., Guangya Liu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/45214/
> ---
> 
> (Updated April 14, 2016, 4:37 a.m.)
> 
> 
> Review request for mesos, David vonThenen, Gilbert Song, haosdent huang, and 
> Jie Yu.
> 
> 
> Bugs: MESOS-5012
> https://issues.apache.org/jira/browse/MESOS-5012
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Updated protobuf to support external storage.
> 
> 
> Diffs
> -
> 
>   include/mesos/mesos.proto 1232bcda053db3451d18413e81e54e51774f7f22 
>   include/mesos/v1/mesos.proto 1048fae62733926b5e863a0c1629bba69e35e43b 
> 
> Diff: https://reviews.apache.org/r/45214/diff/
> 
> 
> Testing
> ---
> 
> make
> make check
> 
> 
> Thanks,
> 
> Guangya Liu
> 
>

Re: Review Request 45922: Added agent authorization flags.

[Jan Schlicht]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/45922/
---

(Updated April 14, 2016, 6:16 p.m.)


Review request for mesos, Adam B and Alexander Rojas.


Bugs: MESOS-5142
https://issues.apache.org/jira/browse/MESOS-5142


Repository: mesos


Description (updated)
---

Added agent authorization flags.


Diffs (updated)
-

  docs/configuration.md ce51f26f0584eeecb5c96e39a5bcc21f5906a29c 
  src/local/local.cpp df72ac52110b75d63df1076496b48e63d06d42ce 
  src/slave/constants.hpp 9978c11fec40055dd42f19c20cd3e9fef4e78cea 
  src/slave/flags.hpp ee520acc459564fe68272950948fc80c5e24513a 
  src/slave/flags.cpp 316feec26e52a8cb1f0fd0739176b3f502e1ed86 
  src/slave/main.cpp 70df4f384b09a1fb078cd893efe52a5e3b116f48 
  src/slave/slave.hpp 76f3aff394e5cecc54dcb3065cb872e238bc228e 
  src/slave/slave.cpp 49fa4a06e26d0d4475ed50db254a320a7030f896 
  src/tests/cluster.hpp 887342acc72b33b4c904d610da47394f9a7d7188 
  src/tests/cluster.cpp b4d69106388892b88f7de20b248cac8b950b861c 
  src/tests/mesos.cpp 1b7a8fd703b90d77ffa3df079bdc2744752caac6 

Diff: https://reviews.apache.org/r/45922/diff/


Testing
---

make check


Thanks,

Jan Schlicht

Re: Review Request 45922: Added agent authorization flags.

[Jan Schlicht]

> On April 12, 2016, 11:31 a.m., Adam B wrote:
> > docs/configuration.md, line 886
> > 
> >
> > None of these examples apply to an agent. Maybe we need to implement an 
> > ACL (e.g. GET_ENDPOINT_WITH_PATH "/flags") before we can provide any 
> > example ACLs for the agent.

Will be implemented in https://reviews.apache.org/r/46203


- Jan


---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/45922/#review128350
---


On April 14, 2016, 6:16 p.m., Jan Schlicht wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/45922/
> ---
> 
> (Updated April 14, 2016, 6:16 p.m.)
> 
> 
> Review request for mesos, Adam B and Alexander Rojas.
> 
> 
> Bugs: MESOS-5142
> https://issues.apache.org/jira/browse/MESOS-5142
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added agent authorization flags.
> 
> 
> Diffs
> -
> 
>   docs/configuration.md ce51f26f0584eeecb5c96e39a5bcc21f5906a29c 
>   src/local/local.cpp df72ac52110b75d63df1076496b48e63d06d42ce 
>   src/slave/constants.hpp 9978c11fec40055dd42f19c20cd3e9fef4e78cea 
>   src/slave/flags.hpp ee520acc459564fe68272950948fc80c5e24513a 
>   src/slave/flags.cpp 316feec26e52a8cb1f0fd0739176b3f502e1ed86 
>   src/slave/main.cpp 70df4f384b09a1fb078cd893efe52a5e3b116f48 
>   src/slave/slave.hpp 76f3aff394e5cecc54dcb3065cb872e238bc228e 
>   src/slave/slave.cpp 49fa4a06e26d0d4475ed50db254a320a7030f896 
>   src/tests/cluster.hpp 887342acc72b33b4c904d610da47394f9a7d7188 
>   src/tests/cluster.cpp b4d69106388892b88f7de20b248cac8b950b861c 
>   src/tests/mesos.cpp 1b7a8fd703b90d77ffa3df079bdc2744752caac6 
> 
> Diff: https://reviews.apache.org/r/45922/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Jan Schlicht
> 
>

Re: Review Request 46204: Updated slave recovery tests with HTTP command executor.

[Mesos ReviewBot]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/46204/#review128911
---



Patch looks great!

Reviews applied: [46187, 46188, 46204]

Passed command: export OS='ubuntu:14.04' CONFIGURATION='--verbose' 
COMPILER='gcc' ENVIRONMENT='GLOG_v=1 MESOS_VERBOSE=1'; ./support/docker_build.sh

- Mesos ReviewBot


On April 14, 2016, 3:14 p.m., Qian Zhang wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/46204/
> ---
> 
> (Updated April 14, 2016, 3:14 p.m.)
> 
> 
> Review request for mesos, Anand Mazumdar and Vinod Kone.
> 
> 
> Bugs: MESOS-3558
> https://issues.apache.org/jira/browse/MESOS-3558
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Updated slave recovery tests with HTTP command executor.
> 
> 
> Diffs
> -
> 
>   src/tests/slave_recovery_tests.cpp 79132344be3bcd2bda54357cd5e7e0c59a766fd8 
> 
> Diff: https://reviews.apache.org/r/46204/diff/
> 
> 
> Testing
> ---
> 
> $ sudo make check GTEST_FILTER="SlaveRecoveryTest*HTTPExecutor"
> ...
> 
> [==] Running 4 tests from 1 test case.
> [--] Global test environment set-up.
> [--] 4 tests from SlaveRecoveryTest/0, where TypeParam = 
> mesos::internal::slave::MesosContainerizer
> [ RUN  ] SlaveRecoveryTest/0.ReconnectHTTPExecutor
> I0414 23:13:11.715150  1989 executor.cpp:174] Version: 0.29.0
> Received SUBSCRIBED event
> Subscribed executor on mesos
> Received LAUNCH event
> Starting task bf66407c-b780-4bcc-b8de-6d30fe7b2660
> Forked command at 1997
> sh -c 'sleep 1000'
> Received ERROR event
> Error: Received unexpected '500 Internal Server Error' () for UPDATE
> E0414 23:13:11.819635  1994 executor.cpp:663] End-Of-File received from 
> agent. The agent closed the event stream
> Received ERROR event
> Error: Received unexpected '400 Bad Request' () for SUBSCRIBE
> Received SUBSCRIBED event
> Subscribed executor on mesos
> Received ACKNOWLEDGED event
> Received SHUTDOWN event
> Shutting down
> Sending SIGTERM to process tree at pid 1997
> [   OK ] SlaveRecoveryTest/0.ReconnectHTTPExecutor (3418 ms)
> [ RUN  ] SlaveRecoveryTest/0.RecoverUnregisteredHTTPExecutor
> I0414 23:13:15.050401  2021 executor.cpp:174] Version: 0.29.0
> Received ERROR event
> Error: Received unexpected '500 Internal Server Error' () for SUBSCRIBE
> [   OK ] SlaveRecoveryTest/0.RecoverUnregisteredHTTPExecutor (1502 ms)
> [ RUN  ] SlaveRecoveryTest/0.CleanupHTTPExecutor
> I0414 23:13:16.502077  2081 executor.cpp:174] Version: 0.29.0
> Received SUBSCRIBED event
> Subscribed executor on mesos
> Received LAUNCH event
> Starting task 1efe8ac8-d2d5-46d3-99e5-b09cfbab269d
> Forked command at 2088
> sh -c 'sleep 1000'
> Received ERROR event
> Error: Received unexpected '500 Internal Server Error' () for UPDATE
> E0414 23:13:16.604841  2080 executor.cpp:663] End-Of-File received from 
> agent. The agent closed the event stream
> Received ERROR event
> Error: Received unexpected '400 Bad Request' () for SUBSCRIBE
> [   OK ] SlaveRecoveryTest/0.CleanupHTTPExecutor (2348 ms)
> [ RUN  ] SlaveRecoveryTest/0.KillTaskWithHTTPExecutor
> I0414 23:13:18.936816  2130 executor.cpp:174] Version: 0.29.0
> Received SUBSCRIBED event
> Subscribed executor on mesos
> Received LAUNCH event
> Starting task f399801f-0f78-4458-8485-a50a2f082c45
> Forked command at 2133
> sh -c 'sleep 1000'
> Received ACKNOWLEDGED event
> E0414 23:13:19.176312  2127 executor.cpp:663] End-Of-File received from 
> agent. The agent closed the event stream
> Received ERROR event
> Error: Received unexpected '400 Bad Request' () for SUBSCRIBE
> Received SUBSCRIBED event
> Subscribed executor on mesos
> Received KILL event
> Received kill for task f399801f-0f78-4458-8485-a50a2f082c45
> Sending SIGTERM to process tree at pid 2133
> Sent SIGTERM to the following process trees:
> [ 
> -+- 2133 sh -c sleep 1000 
>  --- 2134 sleep 1000 
> ]
> Command terminated with signal Terminated (pid: 2133)
> Received ACKNOWLEDGED event
> [   OK ] SlaveRecoveryTest/0.KillTaskWithHTTPExecutor (3725 ms)
> [--] 4 tests from SlaveRecoveryTest/0 (11033 ms total)
> 
> [--] Global test environment tear-down
> [==] 4 tests from 1 test case ran. (11052 ms total)
> [  PASSED  ] 4 tests.
> 
> 
> Thanks,
> 
> Qian Zhang
> 
>

Re: Review Request 46187: Terminate when receiving the ACK of terminal status update.

[Vinod Kone]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/46187/#review128916
---




src/launcher/http_command_executor.cpp 


Looking at slave::statusUpdate() code there are several scenarios where the 
slave ignores a status update sent by the executor; this means this executor 
could end up not terminating forever.

Can you do the following:

--> Enque a message in the queue to self terminate after some timeout (you 
can use the delay() function) to be safe.

--> Add a TODO that we do this to be safe and also because slave sometimes 
doesn't ACK a status update. Link to a ticket that fixes the slave status 
update semantics to always ACK a status update sent by an executor.

sounds good?


- Vinod Kone


On April 14, 2016, 5:17 a.m., Qian Zhang wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/46187/
> ---
> 
> (Updated April 14, 2016, 5:17 a.m.)
> 
> 
> Review request for mesos, Anand Mazumdar and Vinod Kone.
> 
> 
> Bugs: MESOS-3558
> https://issues.apache.org/jira/browse/MESOS-3558
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Terminate when receiving the ACK of terminal status update.
> 
> 
> Diffs
> -
> 
>   src/launcher/http_command_executor.cpp 
> ad484e0e6f5067b6c166111c91b1ff1e8c05d9ac 
> 
> Diff: https://reviews.apache.org/r/46187/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Qian Zhang
> 
>

Re: Review Request 45377: Updated prepare() logic for unified container.

[Guangya Liu]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/45377/
---

(Updated 四月 14, 2016, 4:30 p.m.)


Review request for mesos, David vonThenen, Gilbert Song, haosdent huang, and 
Jie Yu.


Bugs: MESOS-5013
https://issues.apache.org/jira/browse/MESOS-5013


Repository: mesos


Description
---

Updated prepare() logic for unified container.


Diffs (updated)
-

  src/slave/containerizer/mesos/isolators/docker/volume/isolator.cpp 
PRE-CREATION 
  src/slave/slave.cpp de99e9eb5cc812b2e07deb749b98b4f4db363728 

Diff: https://reviews.apache.org/r/45377/diff/


Testing (updated)
---

make
make check


Thanks,

Guangya Liu

Re: Review Request 45922: Added agent authorization flags.

[Jan Schlicht]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/45922/
---

(Updated April 14, 2016, 6:51 p.m.)


Review request for mesos, Adam B and Alexander Rojas.


Changes
---

Rebased.


Bugs: MESOS-5142
https://issues.apache.org/jira/browse/MESOS-5142


Repository: mesos


Description (updated)
---

See summary.


Diffs (updated)
-

  docs/configuration.md ce51f26f0584eeecb5c96e39a5bcc21f5906a29c 
  src/local/local.cpp df72ac52110b75d63df1076496b48e63d06d42ce 
  src/slave/constants.hpp 9978c11fec40055dd42f19c20cd3e9fef4e78cea 
  src/slave/flags.hpp ee520acc459564fe68272950948fc80c5e24513a 
  src/slave/flags.cpp 316feec26e52a8cb1f0fd0739176b3f502e1ed86 
  src/slave/main.cpp 70df4f384b09a1fb078cd893efe52a5e3b116f48 
  src/slave/slave.hpp f78c1b4e4d5378ef7223c6eb3ea45491c30fb4c1 
  src/slave/slave.cpp de99e9eb5cc812b2e07deb749b98b4f4db363728 
  src/tests/cluster.hpp 887342acc72b33b4c904d610da47394f9a7d7188 
  src/tests/cluster.cpp b4d69106388892b88f7de20b248cac8b950b861c 
  src/tests/mesos.cpp 1b7a8fd703b90d77ffa3df079bdc2744752caac6 

Diff: https://reviews.apache.org/r/45922/diff/


Testing
---

make check


Thanks,

Jan Schlicht

Re: Review Request 45217: Implemented docker volume driver isolator interface.

[Gilbert Song]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/45217/#review128919
---


Ship it!




Ship It!

- Gilbert Song


On April 13, 2016, 5:32 p.m., Guangya Liu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/45217/
> ---
> 
> (Updated April 13, 2016, 5:32 p.m.)
> 
> 
> Review request for mesos, David vonThenen, Gilbert Song, haosdent huang, and 
> Jie Yu.
> 
> 
> Bugs: MESOS-5013
> https://issues.apache.org/jira/browse/MESOS-5013
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Implemented docker volume driver isolator interface.
> 
> 
> Diffs
> -
> 
>   src/CMakeLists.txt ca59a1824352423f4db6ef8bb41acc6fe602c041 
>   src/Makefile.am a8f68316c5d4d4c82d99363535a97fa5b1caafc5 
>   src/slave/containerizer/mesos/isolators/docker/volume/isolator.hpp 
> PRE-CREATION 
>   src/slave/containerizer/mesos/isolators/docker/volume/isolator.cpp 
> PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/45217/diff/
> 
> 
> Testing
> ---
> 
> make
> make check
> 
> 
> Thanks,
> 
> Guangya Liu
> 
>

Review Request 46204: Updated slave recovery tests with HTTP command executor.

[Qian Zhang]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/46204/
---

Review request for mesos, Anand Mazumdar and Vinod Kone.


Bugs: MESOS-3558
https://issues.apache.org/jira/browse/MESOS-3558


Repository: mesos


Description
---

Updated slave recovery tests with HTTP command executor.


Diffs
-

  src/tests/slave_recovery_tests.cpp 79132344be3bcd2bda54357cd5e7e0c59a766fd8 

Diff: https://reviews.apache.org/r/46204/diff/


Testing
---

$ sudo make check GTEST_FILTER="SlaveRecoveryTest*HTTPExecutor"
...

[==] Running 4 tests from 1 test case.
[--] Global test environment set-up.
[--] 4 tests from SlaveRecoveryTest/0, where TypeParam = 
mesos::internal::slave::MesosContainerizer
[ RUN  ] SlaveRecoveryTest/0.ReconnectHTTPExecutor
I0414 23:13:11.715150  1989 executor.cpp:174] Version: 0.29.0
Received SUBSCRIBED event
Subscribed executor on mesos
Received LAUNCH event
Starting task bf66407c-b780-4bcc-b8de-6d30fe7b2660
Forked command at 1997
sh -c 'sleep 1000'
Received ERROR event
Error: Received unexpected '500 Internal Server Error' () for UPDATE
E0414 23:13:11.819635  1994 executor.cpp:663] End-Of-File received from agent. 
The agent closed the event stream
Received ERROR event
Error: Received unexpected '400 Bad Request' () for SUBSCRIBE
Received SUBSCRIBED event
Subscribed executor on mesos
Received ACKNOWLEDGED event
Received SHUTDOWN event
Shutting down
Sending SIGTERM to process tree at pid 1997
[   OK ] SlaveRecoveryTest/0.ReconnectHTTPExecutor (3418 ms)
[ RUN  ] SlaveRecoveryTest/0.RecoverUnregisteredHTTPExecutor
I0414 23:13:15.050401  2021 executor.cpp:174] Version: 0.29.0
Received ERROR event
Error: Received unexpected '500 Internal Server Error' () for SUBSCRIBE
[   OK ] SlaveRecoveryTest/0.RecoverUnregisteredHTTPExecutor (1502 ms)
[ RUN  ] SlaveRecoveryTest/0.CleanupHTTPExecutor
I0414 23:13:16.502077  2081 executor.cpp:174] Version: 0.29.0
Received SUBSCRIBED event
Subscribed executor on mesos
Received LAUNCH event
Starting task 1efe8ac8-d2d5-46d3-99e5-b09cfbab269d
Forked command at 2088
sh -c 'sleep 1000'
Received ERROR event
Error: Received unexpected '500 Internal Server Error' () for UPDATE
E0414 23:13:16.604841  2080 executor.cpp:663] End-Of-File received from agent. 
The agent closed the event stream
Received ERROR event
Error: Received unexpected '400 Bad Request' () for SUBSCRIBE
[   OK ] SlaveRecoveryTest/0.CleanupHTTPExecutor (2348 ms)
[ RUN  ] SlaveRecoveryTest/0.KillTaskWithHTTPExecutor
I0414 23:13:18.936816  2130 executor.cpp:174] Version: 0.29.0
Received SUBSCRIBED event
Subscribed executor on mesos
Received LAUNCH event
Starting task f399801f-0f78-4458-8485-a50a2f082c45
Forked command at 2133
sh -c 'sleep 1000'
Received ACKNOWLEDGED event
E0414 23:13:19.176312  2127 executor.cpp:663] End-Of-File received from agent. 
The agent closed the event stream
Received ERROR event
Error: Received unexpected '400 Bad Request' () for SUBSCRIBE
Received SUBSCRIBED event
Subscribed executor on mesos
Received KILL event
Received kill for task f399801f-0f78-4458-8485-a50a2f082c45
Sending SIGTERM to process tree at pid 2133
Sent SIGTERM to the following process trees:
[ 
-+- 2133 sh -c sleep 1000 
 --- 2134 sleep 1000 
]
Command terminated with signal Terminated (pid: 2133)
Received ACKNOWLEDGED event
[   OK ] SlaveRecoveryTest/0.KillTaskWithHTTPExecutor (3725 ms)
[--] 4 tests from SlaveRecoveryTest/0 (11033 ms total)

[--] Global test environment tear-down
[==] 4 tests from 1 test case ran. (11052 ms total)
[  PASSED  ] 4 tests.


Thanks,

Qian Zhang

Re: Review Request 45670: Updated tests for HTTP command executor.

[Qian Zhang]

> On April 13, 2016, 5:11 a.m., Vinod Kone wrote:
> > Are you also planning to update slave recovery tests? Those are the most 
> > crucial.
> 
> Qian Zhang wrote:
> Sure, I will update slave recovery tests soon. Just want to double 
> confirm, in `slave_recovery_tests.cpp`, I see there are two TODOs related to 
> HTTP based command executor in two test cases:
> 
> https://github.com/apache/mesos/blob/0.28.0/src/tests/slave_recovery_tests.cpp#L462
> 
> https://github.com/apache/mesos/blob/0.28.0/src/tests/slave_recovery_tests.cpp#L1082
> 
> You mean I need to update these two test cases with the HTTP command 
> executor I implemented in MESOS-3558, right?
> 
> Vinod Kone wrote:
> Definitely those two and the one we talked about in the earlier review.
> 
> It would also be good to add the following tests for HTTP command 
> executor. There are already tests for this for driver based command executor.
> 
> --> RecoverUnregisteredHTTPExecutor 
> --> RecoverTerminatedExecutor
> --> KillTask

OK, I have posted a patch (https://reviews.apache.org/r/46204/) to update those 
two tests and add two more (`RecoverUnregisteredHTTPExecutor` and 
`KillTaskWithHTTPExecutor`) in slave recovery tests, and I will post a separate 
patch for adding the test that slave restarts with flag change.

But for the test `RecoverTerminatedExecutor`, I have a question: we need to 
shutdown the executor when agent is down, for the driver based executor, I see 
we use `process::post(executorPid, ShutdownExecutorMessage())`, but for HTTP 
based executor, I am not sure how to shutdown it in the test, any suggestions? 
Thanks.


- Qian


---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/45670/#review128535
---


On April 13, 2016, 5:15 p.m., Qian Zhang wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/45670/
> ---
> 
> (Updated April 13, 2016, 5:15 p.m.)
> 
> 
> Review request for mesos, Anand Mazumdar and Vinod Kone.
> 
> 
> Bugs: MESOS-3558
> https://issues.apache.org/jira/browse/MESOS-3558
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Updated tests for HTTP command executor.
> 
> 
> Diffs
> -
> 
>   src/tests/command_executor_tests.cpp 
> 843233adaa68ab1f5cedb7b075439bb8b77469a8 
> 
> Diff: https://reviews.apache.org/r/45670/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Qian Zhang
> 
>

Re: Review Request 45983: Enabled the `network/cni` isolator in `MesosContainerizer`.

[Avinash sridharan]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/45983/
---

(Updated April 14, 2016, 3:26 p.m.)


Review request for mesos, Jie Yu and Qian Zhang.


Changes
---

Fixed OS X build failure.


Bugs: MESOS-5130
https://issues.apache.org/jira/browse/MESOS-5130


Repository: mesos


Description
---

Enabled the `network/cni` isolator in `MesosContainerizer`.


Diffs (updated)
-

  src/slave/containerizer/mesos/containerizer.cpp 
dfaffd79b807b50c61e1d1a6a0c1b0c99f5df693 

Diff: https://reviews.apache.org/r/45983/diff/


Testing (updated)
---

make (Ubuntu 14.04 and OS X)

* Ran the Agent without the `network/cni` enabled in the `--isolation` flags. 
Without the `--network_cni_plugins_dir` and the `--network_cni_config_dir`, 
container using a docker image was launched on the host-network. With the 
`--network_cni_plugins_dir` and the `--network_cni_config_dir` specified the 
CNI network isolator correctly invoked the specified plugin to given the 
container its own network namespace.


Thanks,

Avinash sridharan

Review Request 46209: Explicitly set `FrameworkInfo.principal` if AuthN is enabled.

[Anand Mazumdar]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/46209/
---

Review request for mesos and Vinod Kone.


Bugs: MESOS-5214
https://issues.apache.org/jira/browse/MESOS-5214


Repository: mesos


Description
---

This change explicitly sets `FrameworkInfo.principal` if
AuthN is enabled for old driver based frameworks. The
value gets defaulted to the authenticated principal (if missing).


Diffs
-

  src/master/master.hpp 1f480f03900a0dc2996c2ed3a9534dfa8036940f 
  src/master/master.cpp 781402c04fded159183e1ca28894e48355200f0c 

Diff: https://reviews.apache.org/r/46209/diff/


Testing
---

make check


Thanks,

Anand Mazumdar

Re: Review Request 46114: Fixed per framework principal metrics for HTTP frameworks.

[Anand Mazumdar]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/46114/
---

(Updated April 14, 2016, 3:34 p.m.)


Review request for mesos and Vinod Kone.


Changes
---

Review comments from Vinod


Bugs: MESOS-3923
https://issues.apache.org/jira/browse/MESOS-3923


Repository: mesos


Description (updated)
---

Use the `principal` set in `FrameworkInfo.principal` to populate
framework metrics instead of looking them up in `authenticated`
map. It also ensures that HTTP->PID framework downgrades preserve 
the metrics and no additional logic is needed to deal with this. 
(Previously HTTP frameworks never had AuthN and no principal was 
passed in `FrameworkInfo`, so this was never a concern).


Diffs (updated)
-

  src/master/master.cpp 781402c04fded159183e1ca28894e48355200f0c 

Diff: https://reviews.apache.org/r/46114/diff/


Testing
---

make check


Thanks,

Anand Mazumdar

Re: Review Request 45955: Added CNI helper subcommand to `mesos-containerizer`.

[Avinash sridharan]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/45955/
---

(Updated April 14, 2016, 3:24 p.m.)


Review request for mesos, Jie Yu and Qian Zhang.


Changes
---

Fixed a OSX build failure.


Bugs: MESOS-4922
https://issues.apache.org/jira/browse/MESOS-4922


Repository: mesos


Description
---

This will be used by the `network/cni` isolator to setup hostname and
various network files within the container UTS and mnt namespace.


Diffs (updated)
-

  src/slave/containerizer/mesos/main.cpp 
63b257e5ea079d516c0cbed9593a116f22677255 

Diff: https://reviews.apache.org/r/45955/diff/


Testing
---

make

* Ran mesos-execute with the a single master/slave setup that the container 
attaches to the CNI network correctly and that its network files and hostname 
are setup correctly.


Thanks,

Avinash sridharan

Re: Review Request 46114: Fixed per framework principal metrics for HTTP frameworks.

[Anand Mazumdar]

> On April 13, 2016, 7:04 p.m., Vinod Kone wrote:
> > src/master/master.cpp, lines 5944-5947
> > 
> >
> > We were discussing about this in another context this morning. I think 
> > we should just set FrameworkInfo.principal to Credential.principal (driver 
> > based) or AuthorizationHeader.principal (HTTP API) when framework has 
> > authenticated. This should be done by the master at the point of 
> > registration/subscription. Can you do that in a different review and make 
> > this review dependent on that?

https://reviews.apache.org/r/46209 now caters to this in the chain.


> On April 13, 2016, 7:04 p.m., Vinod Kone wrote:
> > src/master/master.cpp, lines 5939-5941
> > 
> >
> > The principal in HTTP based framework will be passed in the HTTP 
> > Authorization header. The FrameworkInfo.prinicipal is orthongoal to whether 
> > the framework is using a driver based API or HTTP API; it should be used in 
> > cases where framework doesn't want to authenticate but still want mesos to 
> > authorize its actions.
> > 
> > Also, I was expecting the `authenticated` map to contain information 
> > about if a framework is authenticated or not irrespective of whether it's 
> > driver or HTTP based. The map will need to be updated to take either a PID 
> > or HTTP connection.
> > 
> > I haven't seen the other reviews, but this is what I had in mind.

As discussed offline, we won't be needing this.


- Anand


---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/46114/#review128730
---


On April 14, 2016, 3:34 p.m., Anand Mazumdar wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/46114/
> ---
> 
> (Updated April 14, 2016, 3:34 p.m.)
> 
> 
> Review request for mesos and Vinod Kone.
> 
> 
> Bugs: MESOS-3923
> https://issues.apache.org/jira/browse/MESOS-3923
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Use the `principal` set in `FrameworkInfo.principal` to populate
> framework metrics instead of looking them up in `authenticated`
> map. It also ensures that HTTP->PID framework downgrades preserve 
> the metrics and no additional logic is needed to deal with this. 
> (Previously HTTP frameworks never had AuthN and no principal was 
> passed in `FrameworkInfo`, so this was never a concern).
> 
> 
> Diffs
> -
> 
>   src/master/master.cpp 781402c04fded159183e1ca28894e48355200f0c 
> 
> Diff: https://reviews.apache.org/r/46114/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Anand Mazumdar
> 
>

Re: Review Request 46115: Added AuthN for HTTP based frameworks.

[Anand Mazumdar]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/46115/
---

(Updated April 14, 2016, 3:35 p.m.)


Review request for mesos, Greg Mann and Vinod Kone.


Changes
---

Review comments from Vinod


Bugs: MESOS-3923
https://issues.apache.org/jira/browse/MESOS-3923


Repository: mesos


Description
---

This change adds AuthN support for HTTP based frameworks.

We allow AuthZ without AuthN for `/scheduler` endpoint. Also,
we ensure that `FrameworkInfo.principal` (if present) equals
the authenticated principal. We also allow a framework to
omit specifying the `FrameworkInfo.principal` in case
it is not interested in authorization or if it is disabled.
We do log a warning for this cases similar to what driver
based frameworks already do.


Diffs (updated)
-

  src/master/http.cpp b8a83b58b60416f61610cad16fc6f70028a5ee10 
  src/master/master.hpp 1f480f03900a0dc2996c2ed3a9534dfa8036940f 
  src/master/master.cpp 781402c04fded159183e1ca28894e48355200f0c 
  src/master/validation.hpp d1f2323172cbf3bb052942a119b8531f9ccad48d 
  src/master/validation.cpp 13423436a4e6361fde6fa75133eebf5c02c8381f 

Diff: https://reviews.apache.org/r/46115/diff/


Testing
---

make check. Enabled AuthN for all the tests later in the chain.


Thanks,

Anand Mazumdar

Re: Review Request 45983: Enabled the `network/cni` isolator in `MesosContainerizer`.

[Avinash sridharan]

> On April 14, 2016, 12:47 a.m., Jie Yu wrote:
> > src/slave/containerizer/mesos/containerizer.cpp, lines 165-168
> > 
> >
> > Please wrap this with ifdef linux

Thanks for catching this. Wrapped in `#ifdef` and ran the build on OS X to 
confirm.


- Avinash


---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/45983/#review128811
---


On April 14, 2016, 3:26 p.m., Avinash sridharan wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/45983/
> ---
> 
> (Updated April 14, 2016, 3:26 p.m.)
> 
> 
> Review request for mesos, Jie Yu and Qian Zhang.
> 
> 
> Bugs: MESOS-5130
> https://issues.apache.org/jira/browse/MESOS-5130
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Enabled the `network/cni` isolator in `MesosContainerizer`.
> 
> 
> Diffs
> -
> 
>   src/slave/containerizer/mesos/containerizer.cpp 
> dfaffd79b807b50c61e1d1a6a0c1b0c99f5df693 
> 
> Diff: https://reviews.apache.org/r/45983/diff/
> 
> 
> Testing
> ---
> 
> make (Ubuntu 14.04 and OS X)
> 
> * Ran the Agent without the `network/cni` enabled in the `--isolation` flags. 
> Without the `--network_cni_plugins_dir` and the `--network_cni_config_dir`, 
> container using a docker image was launched on the host-network. With the 
> `--network_cni_plugins_dir` and the `--network_cni_config_dir` specified the 
> CNI network isolator correctly invoked the specified plugin to given the 
> container its own network namespace.
> 
> 
> Thanks,
> 
> Avinash sridharan
> 
>

Review Request 46211: Added flags for authenticating HTTP frameworks to master.

[Anand Mazumdar]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/46211/
---

Review request for mesos and Vinod Kone.


Bugs: MESOS-3923
https://issues.apache.org/jira/browse/MESOS-3923


Repository: mesos


Description
---

This change introduces two new flags `authenticate_http_frameworks`
and `http_framework_authenticators` to the master. This allows us
to selectively turn on/off framework authentication and decouple
them from authentication for operator endpoints.


Diffs
-

  src/master/constants.hpp 7c7cc25fcc897dedb28001fbb944d2e50eca4713 
  src/master/flags.hpp 83bb9088e595b393d610cc65479cb6a35fb31842 
  src/master/flags.cpp e522499586b731d522180f171731a9dd38b8344c 
  src/master/master.cpp 781402c04fded159183e1ca28894e48355200f0c 

Diff: https://reviews.apache.org/r/46211/diff/


Testing
---

make check


Thanks,

Anand Mazumdar

Re: Review Request 46115: Added AuthN for HTTP based frameworks.

[Greg Mann]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/46115/#review128914
---



Looks good! A couple small things below.


src/master/http.cpp (lines 431 - 436)


What about instead:

`if (principal.isSome() && 
!call.subscribe().framework_info().has_principal())`?



src/master/validation.hpp (line 47)


Do we really want to have a default value for an argument to a validation 
function? It makes me a bit uneasy; if the caller wants a `Call` message to be 
validated, shouldn't we require them to provide the exact data to validated, 
without making assumptions for them?



src/master/validation.cpp (lines 79 - 81)


What about: `if (principal.isSome() && frameworkInfo.has_principal() && 
principal != frameworkInfo.principal())`?


- Greg Mann


On April 14, 2016, 3:35 p.m., Anand Mazumdar wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/46115/
> ---
> 
> (Updated April 14, 2016, 3:35 p.m.)
> 
> 
> Review request for mesos, Greg Mann and Vinod Kone.
> 
> 
> Bugs: MESOS-3923
> https://issues.apache.org/jira/browse/MESOS-3923
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This change adds AuthN support for HTTP based frameworks.
> 
> We allow AuthZ without AuthN for `/scheduler` endpoint. Also,
> we ensure that `FrameworkInfo.principal` (if present) equals
> the authenticated principal. We also allow a framework to
> omit specifying the `FrameworkInfo.principal` in case
> it is not interested in authorization or if it is disabled.
> We do log a warning for this cases similar to what driver
> based frameworks already do.
> 
> 
> Diffs
> -
> 
>   src/master/http.cpp b8a83b58b60416f61610cad16fc6f70028a5ee10 
>   src/master/master.hpp 1f480f03900a0dc2996c2ed3a9534dfa8036940f 
>   src/master/master.cpp 781402c04fded159183e1ca28894e48355200f0c 
>   src/master/validation.hpp d1f2323172cbf3bb052942a119b8531f9ccad48d 
>   src/master/validation.cpp 13423436a4e6361fde6fa75133eebf5c02c8381f 
> 
> Diff: https://reviews.apache.org/r/46115/diff/
> 
> 
> Testing
> ---
> 
> make check. Enabled AuthN for all the tests later in the chain.
> 
> 
> Thanks,
> 
> Anand Mazumdar
> 
>

Re: Review Request 46162: Moved mesos::internal::log::Log to mesos::log namespace.

[Jie Yu]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/46162/#review128923
---




include/mesos/log/log.hpp (line 50)


2 lines apart between top level entities.



include/mesos/log/log.hpp (line 109)


s/mesos::internal/internal/



src/java/jni/org_apache_mesos_state_LogState.cpp (line 73)


Hum, don't understand why this compiles. Do you need the log:: namespace 
prefix?



src/log/log.cpp (line 41)


We're moving away from using 'using namespace'. Can you try to do explicit 
'using' clauses since you're on it?


- Jie Yu


On April 13, 2016, 11:50 p.m., Kapil Arya wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/46162/
> ---
> 
> (Updated April 13, 2016, 11:50 p.m.)
> 
> 
> Review request for mesos and Jie Yu.
> 
> 
> Bugs: MESOS-5171
> https://issues.apache.org/jira/browse/MESOS-5171
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Moved mesos::internal::log::Log to mesos::log namespace.
> 
> 
> Diffs
> -
> 
>   include/mesos/log/log.hpp 0f61777dd04e6d7212ac218c4cde3cd7f95d2896 
>   src/java/jni/org_apache_mesos_Log.cpp 
> 140b950136417eed7cba363a89537ed2f33832ff 
>   src/java/jni/org_apache_mesos_state_LogState.cpp 
> 528fe5367f55c63916cd4990abcc3c137e77cfec 
>   src/local/local.cpp df72ac52110b75d63df1076496b48e63d06d42ce 
>   src/log/log.cpp a37676068dae14b1adc61ef75e2742c16e7a6e42 
>   src/log/tool/benchmark.cpp 770c6d85fec76826ca8369b2afc721c07899e32f 
>   src/log/tool/replica.cpp 49415821a32960c78192b89f9a0f2067b9157a63 
>   src/master/main.cpp ea7f0fc87c8912309a4679105dde5d8d5bb9ead6 
>   src/state/log.hpp 5dd30ca180380121639f23a931486e97b660c0d2 
>   src/state/log.cpp fd9f5ef90cacb56d934dd8603d70c5a14a36a477 
>   src/tests/cluster.hpp 887342acc72b33b4c904d610da47394f9a7d7188 
>   src/tests/cluster.cpp b4d69106388892b88f7de20b248cac8b950b861c 
>   src/tests/log_tests.cpp 85fc9d4dfaee232f5ea7c29b2133310f7c6441a8 
>   src/tests/registrar_tests.cpp f18e8030f69d8ebf8de81ff03632106e08823df1 
>   src/tests/state_tests.cpp 0b5a6abadc24d16aa0e5e5f2f4b0c9f524c399ec 
> 
> Diff: https://reviews.apache.org/r/46162/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Kapil Arya
> 
>

Re: Review Request 45270: Added spec protobut for external mount.

[Gilbert Song]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/45270/#review128922
---




src/slave/containerizer/mesos/isolators/docker/volume/spec.proto (line 19)


above `import "mesos/mesos.proto";`



src/slave/containerizer/mesos/isolators/docker/volume/spec.proto (lines 26 - 27)


This field make me feel confused, since we also have a `conainer_path` 
field in volume as a required string. Should they be identical? If yes, we 
should comment it.



src/slave/containerizer/mesos/isolators/docker/volume/spec.proto (line 34)


+1 on parameters.


- Gilbert Song


On April 13, 2016, 9:41 p.m., Guangya Liu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/45270/
> ---
> 
> (Updated April 13, 2016, 9:41 p.m.)
> 
> 
> Review request for mesos, David vonThenen, Gilbert Song, haosdent huang, and 
> Jie Yu.
> 
> 
> Bugs: MESOS-5013
> https://issues.apache.org/jira/browse/MESOS-5013
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added spec protobut for external mount.
> 
> 
> Diffs
> -
> 
>   src/CMakeLists.txt ca59a1824352423f4db6ef8bb41acc6fe602c041 
>   src/Makefile.am 139935fb40f8986427c85537a400ff945e54433f 
>   src/slave/containerizer/mesos/isolators/docker/volume/spec.hpp PRE-CREATION 
>   src/slave/containerizer/mesos/isolators/docker/volume/spec.proto 
> PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/45270/diff/
> 
> 
> Testing
> ---
> 
> make
> make check
> 
> 
> Thanks,
> 
> Guangya Liu
> 
>

Re: Review Request 46212: Added documentation around using AuthN for HTTP frameworks.

[Greg Mann]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/46212/#review128926
---



Could we add a section to 'docs/authentication.md' about authenticating HTTP 
frameworks as well?

- Greg Mann


On April 14, 2016, 3:34 p.m., Anand Mazumdar wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/46212/
> ---
> 
> (Updated April 14, 2016, 3:34 p.m.)
> 
> 
> Review request for mesos and Vinod Kone.
> 
> 
> Bugs: MESOS-3923
> https://issues.apache.org/jira/browse/MESOS-3923
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This change adds config docs around authenticating HTTP
> frameworks using the newly introduced flags. Also added a
> small note to the `authenticate` flag that this does not
> work for HTTP based frameworks.
> 
> 
> Diffs
> -
> 
>   docs/configuration.md ba00ec563c449345effb3114111812601addcfc2 
> 
> Diff: https://reviews.apache.org/r/46212/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Anand Mazumdar
> 
>

Re: Review Request 46211: Added flags for authenticating HTTP frameworks to master.

[Greg Mann]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/46211/#review128925
---




src/master/flags.cpp (lines 225 - 226)


This seems a tiny bit misleading: if the value is `false`, no 
authentication will be performed at all. Perhaps something like "If `false`, 
HTTP frameworks are not authenticated."



src/master/master.cpp (lines 665 - 673)


This can be moved into the `if (flags.authenticate_http_frameworks)` scope, 
as well as the above declaration of `httpFrameworkAuthenticator`.


- Greg Mann


On April 14, 2016, 3:34 p.m., Anand Mazumdar wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/46211/
> ---
> 
> (Updated April 14, 2016, 3:34 p.m.)
> 
> 
> Review request for mesos and Vinod Kone.
> 
> 
> Bugs: MESOS-3923
> https://issues.apache.org/jira/browse/MESOS-3923
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This change introduces two new flags `authenticate_http_frameworks`
> and `http_framework_authenticators` to the master. This allows us
> to selectively turn on/off framework authentication and decouple
> them from authentication for operator endpoints.
> 
> 
> Diffs
> -
> 
>   src/master/constants.hpp 7c7cc25fcc897dedb28001fbb944d2e50eca4713 
>   src/master/flags.hpp 83bb9088e595b393d610cc65479cb6a35fb31842 
>   src/master/flags.cpp e522499586b731d522180f171731a9dd38b8344c 
>   src/master/master.cpp 781402c04fded159183e1ca28894e48355200f0c 
> 
> Diff: https://reviews.apache.org/r/46211/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Anand Mazumdar
> 
>

Re: Review Request 46115: Added AuthN for HTTP based frameworks.

[Anand Mazumdar]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/46115/
---

(Updated April 14, 2016, 5:46 p.m.)


Review request for mesos, Greg Mann and Vinod Kone.


Changes
---

Review comments from Greg


Bugs: MESOS-3923
https://issues.apache.org/jira/browse/MESOS-3923


Repository: mesos


Description
---

This change adds AuthN support for HTTP based frameworks.

We allow AuthZ without AuthN for `/scheduler` endpoint. Also,
we ensure that `FrameworkInfo.principal` (if present) equals
the authenticated principal. We also allow a framework to
omit specifying the `FrameworkInfo.principal` in case
it is not interested in authorization or if it is disabled.
We do log a warning for this cases similar to what driver
based frameworks already do.


Diffs (updated)
-

  src/master/http.cpp b8a83b58b60416f61610cad16fc6f70028a5ee10 
  src/master/master.hpp 1f480f03900a0dc2996c2ed3a9534dfa8036940f 
  src/master/master.cpp 781402c04fded159183e1ca28894e48355200f0c 
  src/master/validation.hpp d1f2323172cbf3bb052942a119b8531f9ccad48d 
  src/master/validation.cpp 13423436a4e6361fde6fa75133eebf5c02c8381f 

Diff: https://reviews.apache.org/r/46115/diff/


Testing
---

make check. Enabled AuthN for all the tests later in the chain.


Thanks,

Anand Mazumdar

Re: Review Request 46115: Added AuthN for HTTP based frameworks.

[Anand Mazumdar]

> On April 14, 2016, 5:10 p.m., Greg Mann wrote:
> > src/master/validation.hpp, line 47
> > 
> >
> > Do we really want to have a default value for an argument to a 
> > validation function? It makes me a bit uneasy; if the caller wants a `Call` 
> > message to be validated, shouldn't we require them to provide the exact 
> > data to validated, without making assumptions for them?

Unfortunately, this method is also used by the validation function in `receive` 
of master: 
https://github.com/apache/mesos/blob/master/src/master/master.cpp#L1952 by 
driver based frameworks.


> On April 14, 2016, 5:10 p.m., Greg Mann wrote:
> > src/master/validation.cpp, lines 79-81
> > 
> >
> > What about: `if (principal.isSome() && frameworkInfo.has_principal() && 
> > principal != frameworkInfo.principal())`?

My bad, the previous version had two `if` statements. But, I ended up moving 
one but forgot to clean this up.


- Anand


---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/46115/#review128914
---


On April 14, 2016, 5:46 p.m., Anand Mazumdar wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/46115/
> ---
> 
> (Updated April 14, 2016, 5:46 p.m.)
> 
> 
> Review request for mesos, Greg Mann and Vinod Kone.
> 
> 
> Bugs: MESOS-3923
> https://issues.apache.org/jira/browse/MESOS-3923
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This change adds AuthN support for HTTP based frameworks.
> 
> We allow AuthZ without AuthN for `/scheduler` endpoint. Also,
> we ensure that `FrameworkInfo.principal` (if present) equals
> the authenticated principal. We also allow a framework to
> omit specifying the `FrameworkInfo.principal` in case
> it is not interested in authorization or if it is disabled.
> We do log a warning for this cases similar to what driver
> based frameworks already do.
> 
> 
> Diffs
> -
> 
>   src/master/http.cpp b8a83b58b60416f61610cad16fc6f70028a5ee10 
>   src/master/master.hpp 1f480f03900a0dc2996c2ed3a9534dfa8036940f 
>   src/master/master.cpp 781402c04fded159183e1ca28894e48355200f0c 
>   src/master/validation.hpp d1f2323172cbf3bb052942a119b8531f9ccad48d 
>   src/master/validation.cpp 13423436a4e6361fde6fa75133eebf5c02c8381f 
> 
> Diff: https://reviews.apache.org/r/46115/diff/
> 
> 
> Testing
> ---
> 
> make check. Enabled AuthN for all the tests later in the chain.
> 
> 
> Thanks,
> 
> Anand Mazumdar
> 
>

Re: Review Request 46118: Fixed tests impacted by enabling AuthN for HTTP frameworks.

[Mesos ReviewBot]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/46118/#review128932
---



Patch looks great!

Reviews applied: [46113, 46209, 46114, 46211, 46212, 46115, 46116, 46117, 46118]

Passed command: export OS='ubuntu:14.04' CONFIGURATION='--verbose' 
COMPILER='gcc' ENVIRONMENT='GLOG_v=1 MESOS_VERBOSE=1'; ./support/docker_build.sh

- Mesos ReviewBot


On April 14, 2016, 3:39 p.m., Anand Mazumdar wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/46118/
> ---
> 
> (Updated April 14, 2016, 3:39 p.m.)
> 
> 
> Review request for mesos and Vinod Kone.
> 
> 
> Bugs: MESOS-4459
> https://issues.apache.org/jira/browse/MESOS-4459
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> See summary.
> 
> 
> Diffs
> -
> 
>   src/tests/master_maintenance_tests.cpp 
> 31d6c70e7ecb584c75404556508dd9cdcf63fb78 
>   src/tests/master_quota_tests.cpp e4a65bf09c8fdd2d6b6161042c3702a8cc4cd454 
>   src/tests/mesos.hpp 20370a277d55efeea8daae7ea5e2f6575b5a2d62 
>   src/tests/mesos.cpp 1b7a8fd703b90d77ffa3df079bdc2744752caac6 
>   src/tests/scheduler_http_api_tests.cpp 
> d469adf7230ce5bb5e71a241a06e389295905e03 
>   src/tests/scheduler_tests.cpp b630944b1d3163345e912cb2bfc3301daa4c6362 
> 
> Diff: https://reviews.apache.org/r/46118/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Anand Mazumdar
> 
>

Re: Review Request 46164: Moved mesos::internal::state to mesos::state namespace.

[Jie Yu]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/46164/#review128935
---




include/mesos/state/state.hpp (line 82)


Let's void mesos:: if possible. Ditto for others.


- Jie Yu


On April 13, 2016, 11:49 p.m., Kapil Arya wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/46164/
> ---
> 
> (Updated April 13, 2016, 11:49 p.m.)
> 
> 
> Review request for mesos and Jie Yu.
> 
> 
> Bugs: MESOS-5171
> https://issues.apache.org/jira/browse/MESOS-5171
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Moved mesos::internal::state to mesos::state namespace.
> 
> 
> Diffs
> -
> 
>   include/mesos/state/state.hpp ecae61fa4fdfb30928c1ccb4e85aa6ebe5addbcd 
>   include/mesos/state/storage.hpp 5de3b174cb80924c6b0dc4cab4569674f70c689a 
>   src/java/jni/org_apache_mesos_state_AbstractState.cpp 
> 4fd43ca31c6917e81ea1b331b8507ca42a2249cf 
>   src/java/jni/org_apache_mesos_state_LevelDBState.cpp 
> fb3c5418220f66c6320581033ee7963de47eafd3 
>   src/java/jni/org_apache_mesos_state_LogState.cpp 
> 528fe5367f55c63916cd4990abcc3c137e77cfec 
>   src/java/jni/org_apache_mesos_state_ZooKeeperState.cpp 
> 23a89c3e5c5309a8a9b45c168fb61b12d89db1ec 
>   src/local/local.cpp df72ac52110b75d63df1076496b48e63d06d42ce 
>   src/master/main.cpp ea7f0fc87c8912309a4679105dde5d8d5bb9ead6 
>   src/state/in_memory.hpp 28ad76134f1cfe64d916679b4f6409d6a5fdd532 
>   src/state/leveldb.hpp 626ed82d84e15f65bc6187f9183cc376b705e1a3 
>   src/state/log.hpp 5dd30ca180380121639f23a931486e97b660c0d2 
>   src/state/protobuf.hpp 7ac5bca1c1a0b92549fc6d7e692e8bdb920b8a0e 
>   src/state/zookeeper.hpp d8f9df8d1f5575e7dcbfaf27e92656baf3b4f0ff 
>   src/tests/cluster.hpp 887342acc72b33b4c904d610da47394f9a7d7188 
>   src/tests/registrar_tests.cpp f18e8030f69d8ebf8de81ff03632106e08823df1 
>   src/tests/state_tests.cpp 0b5a6abadc24d16aa0e5e5f2f4b0c9f524c399ec 
> 
> Diff: https://reviews.apache.org/r/46164/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Kapil Arya
> 
>

Re: Review Request 46163: Exposed state/{state,storage}.hpp files.

[Jie Yu]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/46163/#review128929
---




src/Makefile.am (line 174)


I think the mesos.internal.state.Entry in state.proto needs to be expose. 
However, mesos.internal.state.Operation is impl. details for log storage, and 
should not be exposed.



src/tests/state_tests.cpp (lines 49 - 52)


I think we should expose them as well. We definitely need 
state/protobuf.hpp and state/log.hpp. But it doesn't make sense to just expose 
log storage and leave the rest in private.


- Jie Yu


On April 13, 2016, 11:49 p.m., Kapil Arya wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/46163/
> ---
> 
> (Updated April 13, 2016, 11:49 p.m.)
> 
> 
> Review request for mesos and Jie Yu.
> 
> 
> Bugs: MESOS-5171
> https://issues.apache.org/jira/browse/MESOS-5171
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Exposed state/{state,storage}.hpp files.
> 
> 
> Diffs
> -
> 
>   src/Makefile.am a8f68316c5d4d4c82d99363535a97fa5b1caafc5 
>   src/java/jni/org_apache_mesos_state_AbstractState.cpp 
> 4fd43ca31c6917e81ea1b331b8507ca42a2249cf 
>   src/java/jni/org_apache_mesos_state_LevelDBState.cpp 
> fb3c5418220f66c6320581033ee7963de47eafd3 
>   src/java/jni/org_apache_mesos_state_LogState.cpp 
> 528fe5367f55c63916cd4990abcc3c137e77cfec 
>   src/java/jni/org_apache_mesos_state_Variable.cpp 
> 7a69b3d878e6eaa87bfaf56c0500e11efe4ac44d 
>   src/java/jni/org_apache_mesos_state_ZooKeeperState.cpp 
> 23a89c3e5c5309a8a9b45c168fb61b12d89db1ec 
>   src/local/local.cpp df72ac52110b75d63df1076496b48e63d06d42ce 
>   src/master/main.cpp ea7f0fc87c8912309a4679105dde5d8d5bb9ead6 
>   src/messages/state.proto  
>   src/state/in_memory.hpp 28ad76134f1cfe64d916679b4f6409d6a5fdd532 
>   src/state/in_memory.cpp ac4f34bd2332c8b42a26454809d56c225aa44587 
>   src/state/leveldb.hpp 626ed82d84e15f65bc6187f9183cc376b705e1a3 
>   src/state/leveldb.cpp f925a73856075f08c4af2af0fcaae8e81358667e 
>   src/state/log.hpp 5dd30ca180380121639f23a931486e97b660c0d2 
>   src/state/protobuf.hpp 7ac5bca1c1a0b92549fc6d7e692e8bdb920b8a0e 
>   src/state/state.hpp ecae61fa4fdfb30928c1ccb4e85aa6ebe5addbcd 
>   src/state/storage.hpp 5de3b174cb80924c6b0dc4cab4569674f70c689a 
>   src/state/zookeeper.hpp d8f9df8d1f5575e7dcbfaf27e92656baf3b4f0ff 
>   src/state/zookeeper.cpp 5578fa5f0b86f8dfeabf1be2984bfa0443cc049c 
>   src/tests/cluster.hpp 887342acc72b33b4c904d610da47394f9a7d7188 
>   src/tests/cluster.cpp b4d69106388892b88f7de20b248cac8b950b861c 
>   src/tests/registrar_tests.cpp f18e8030f69d8ebf8de81ff03632106e08823df1 
>   src/tests/state_tests.cpp 0b5a6abadc24d16aa0e5e5f2f4b0c9f524c399ec 
> 
> Diff: https://reviews.apache.org/r/46163/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Kapil Arya
> 
>