答复: Review Request 46249: Hook and module process operation before main process initialize.

[pangbingqiang]

OK，I have post review request, thanks.

发件人: Vinod Kone [mailto:nore...@reviews.apache.org] 代表 Vinod Kone
发送时间: 2016年4月22日 8:37
收件人: Kapil Arya; Niklas Nielsen; Adam B; Joseph Wu; Vinod Kone
抄送: pangbingqiang; mesos
主题: Re: Review Request 46249: Hook and module process operation before main 
process initialize.

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




Committed this. But can you send another review for slave/main.cpp as well?


- Vinod Kone


On April 19th, 2016, 2:52 a.m. UTC, Andy Pang wrote:
Review request for mesos, Adam B, Kapil Arya, Joseph Wu, Niklas Nielsen, and 
Vinod Kone.
By Andy Pang.

Updated April 19, 2016, 2:52 a.m.
Repository: mesos
Description

If we in hook/module manager use libprocess library,

it will auto use process initialize fucntion,

as the process::initialize will only call once,

then in the main fucnton process initialize


process::initialize("master") will not be called.

So the HTTP request process "delegate" will be wrong,

http request will not be response.


Testing

make check


Diffs

  *   src/master/main.cpp (7bbc982)

View Diff

Re: Review Request 46609: [WIP] Add the test "SlaveRecoveryTest.RecoverTerminatedHTTPExecutor".

[Vinod Kone]

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



Can you rebase this?

- Vinod Kone


On April 24, 2016, 12:26 a.m., Qian Zhang wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/46609/
> ---
> 
> (Updated April 24, 2016, 12:26 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
> ---
> 
> Add the test "SlaveRecoveryTest.RecoverTerminatedHTTPExecutor".
> 
> 
> Diffs
> -
> 
>   src/tests/slave_recovery_tests.cpp 8ad63224e0fdd203cb8dbfbc2d2484e3ce52a10c 
> 
> Diff: https://reviews.apache.org/r/46609/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Qian Zhang
> 
>

Re: Review Request 46610: Fix 'pivot_root is not available' error on ARM.

[zhiwei]

According to the initial comment "A workaround for systems that have an old
glib but have a new kernel" made by Ian, I am not sure which version of
glib is old and which version of kernel if new.

At the previous commit, we included a syscall.h to fix this issue[1], but I
am not sure this is the right way, I asked Ian but did not get response,
then I added an elseif which is the safe way.

In my previous testing, I can successfully build Mesos by including the
header file on RHEL7.x, Ubuntu 14.04(both x86_64 and ppc64le).

I think it's ok to include a header file to solve this issue on all the
platforms, but should at least make sure this works on the main OS and
version(RHEL 7.x, Ubuntu 14.04/16.04 and others? Windows? Mac?).


[1]: https://reviews.apache.org/r/42551/


Thanks,
Zhiwei

On Mon, Apr 25, 2016 at 9:05 AM, haosdent  wrote:

> Hi @idownes and @chenzhiwei. @Tomasz proposal to add the correct header
> file `` for pivot_root syscall. According my understand, we
> could drop these workaround code:
>
> ```
> #elif __x86_64__
>   // A workaround for systems that have an old glib but have a new
>   // kernel. The magic number '155' is the syscall number for
>   // 'pivot_root' on the x86_64 architecture, see
>   // arch/x86/syscalls/syscall_64.tbl
>   int ret = ::syscall(155, newRoot.c_str(), putOld.c_str());
> ```
>
> ```
> #elif __powerpc__ || __ppc__ || __powerpc64__ || __ppc64__
>   // A workaround for powerpc. The magic number '203' is the syscall
>   // number for 'pivot_root' on the powerpc architecture, see
>   // https://w3challs.com/syscalls/?arch=powerpc_64
>   int ret = ::syscall(203, newRoot.c_str(), putOld.c_str());
> ```
>
> Would you help double check at your convenience? Thank you in advance.
>
> On Mon, Apr 25, 2016 at 9:00 AM, haosdent huang 
> wrote:
>
>> This is an automatically generated e-mail. To reply, visit:
>> https://reviews.apache.org/r/46610/
>>
>> On April 24th, 2016, 12:42 p.m. UTC, *haosdent huang* wrote:
>>
>> In additional, could you add @chenzhiwei and @vinodkone in reviewers? I 
>> think powerpc could drop
>>
>> #elif __powerpc__ || __ppc__ || __powerpc64__ || __ppc64__
>>   // A workaround for powerpc. The magic number '203' is the syscall
>>   // number for 'pivot_root' on the powerpc architecture, see
>>   // https://w3challs.com/syscalls/?arch=powerpc_64
>>   int ret = ::syscall(203, newRoot.c_str(), putOld.c_str());
>>
>> as well. And you need fill a ticket for this in
>> https://issues.apache.org/jira/browse/MESOS and find a shepherd by
>> sending email to dev mailing list.
>>
>> On April 24th, 2016, 7:57 p.m. UTC, *Tomasz Janiszewski* wrote:
>>
>> It looks like powerpc is properly handled with 203 
>> 
>>
>> On April 24th, 2016, 8:19 p.m. UTC, *Tomasz Janiszewski* wrote:
>>
>> Can we also drop
>>
>> #elif __x86_64__
>>   // A workaround for systems that have an old glib but have a new
>>   // kernel. The magic number '155' is the syscall number for
>>   // 'pivot_root' on the x86_64 architecture, see
>>   // arch/x86/syscalls/syscall_64.tbl
>>   int ret = ::syscall(155, newRoot.c_str(), putOld.c_str());
>>
>> ? If no then we probably can't drop powerpc and arm section.
>>
>> On April 25th, 2016, 12:32 a.m. UTC, *haosdent huang* wrote:
>>
>> Yes, I think could remove as well. I double check x86, arm, arm64, powerpc 
>> in 3.8 just now.  already inlcudes them by 
>>  or .
>>
>> On April 25th, 2016, 12:36 a.m. UTC, *haosdent huang* wrote:
>>
>> By the way, we usually fill ticket number in Bugs: field in review board. 
>> You can fill MESOS-5263 it here.
>>
>> I need @idownes and @chenzhiwei to help check whether we could drop it after 
>> add the correct header file.
>>
>>
>> - haosdent
>>
>> On April 24th, 2016, 11:36 a.m. UTC, Tomasz Janiszewski wrote:
>> Review request for mesos and haosdent huang.
>> By Tomasz Janiszewski.
>>
>> *Updated April 24, 2016, 11:36 a.m.*
>> *Repository: * mesos
>> Description
>>
>> Fix 'pivot_root is not available' error on ARM.
>>
>> Diffs
>>
>>- src/linux/fs.cpp (2087b4ac1503e0fd085319b1017389f1f947536f)
>>
>> View Diff 
>>
>
>
>
> --
> Best Regards,
> Haosdent Huang
>

Re: Review Request 46203: Added authorization of the '/flags' endpoint.

[Alexander Rukletsov]

> On April 26, 2016, 3:40 p.m., Alexander Rukletsov wrote:
> > docs/configuration.md, line 899
> > 
> >
> > Let's entertain our reader a bit : ). Instead of "a", we can use 
> > something like "padavan", and if you plan to introduce say 
> > `post_endpoints`, you can reuse "yoda".
> 
> Jan Schlicht wrote:
> All the other examples for principals use "a" and "b", though. If we want 
> to introduce better principal names, we should do it everywhere.

https://github.com/apache/mesos/blob/dc8b9a4abff74d1a2e33039836682bdfa7188f87/src/slave/flags.cpp#L764


- Alexander


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


On April 26, 2016, 6:29 p.m., Jan Schlicht wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/46203/
> ---
> 
> (Updated April 26, 2016, 6:29 p.m.)
> 
> 
> Review request for mesos, Adam B, Alexander Rojas, and Benjamin Bannier.
> 
> 
> Bugs: MESOS-5142
> https://issues.apache.org/jira/browse/MESOS-5142
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> See summary.
> 
> 
> Diffs
> -
> 
>   docs/configuration.md 2796a812b72f208b1ae2d65a4ba843b50d70 
>   include/mesos/authorizer/acls.proto 
> c50deeb5565dfd5b3e5e7210283d9a36a3bfd579 
>   include/mesos/authorizer/authorizer.proto 
> 40d93ea257d1df8d22eee8a21667db90d579a8fe 
>   src/Makefile.am e024c6d65608a55765e527a8668c415723dcfcca 
>   src/authorizer/local/authorizer.cpp 
> 0a3805fe4ce8eb89e096e8cd4326035513ba892b 
>   src/slave/flags.cpp a319d60c006d1104836c1c40f3617ceac9cb7b1e 
>   src/slave/http.cpp 537736d1fe42e8150bad91326299ef9a17041a8e 
>   src/slave/slave.hpp 57b18882e30e44dcc40449b0e3be8ee970c45bc8 
>   src/tests/slave_authorization_tests.cpp PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/46203/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Jan Schlicht
> 
>

Re: Review Request 46203: Added authorization of the '/flags' endpoint.

[Jan Schlicht]

> On April 26, 2016, 8:27 a.m., Adam B wrote:
> > src/slave/http.cpp, line 804
> > 
> >
> > s/access/GET/ and shouldn't you be checking the Verb here, for when we 
> > have to authorize things other than GETs?

I've put the whole LOG(INFO) inside the switch-case for `GET`. That saves us 
from checking the verb, but introduces some slight code dublication.


- Jan


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


On April 26, 2016, 1:10 p.m., Jan Schlicht wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/46203/
> ---
> 
> (Updated April 26, 2016, 1:10 p.m.)
> 
> 
> Review request for mesos, Adam B, Alexander Rojas, and Benjamin Bannier.
> 
> 
> Bugs: MESOS-5142
> https://issues.apache.org/jira/browse/MESOS-5142
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> See summary.
> 
> 
> Diffs
> -
> 
>   docs/configuration.md 318275fc5f935e6992ed4e8048cc4b42cc5d2cab 
>   include/mesos/authorizer/acls.proto 
> c50deeb5565dfd5b3e5e7210283d9a36a3bfd579 
>   include/mesos/authorizer/authorizer.proto 
> 40d93ea257d1df8d22eee8a21667db90d579a8fe 
>   src/Makefile.am e024c6d65608a55765e527a8668c415723dcfcca 
>   src/authorizer/local/authorizer.cpp 
> 0a3805fe4ce8eb89e096e8cd4326035513ba892b 
>   src/slave/flags.cpp a319d60c006d1104836c1c40f3617ceac9cb7b1e 
>   src/slave/http.cpp 537736d1fe42e8150bad91326299ef9a17041a8e 
>   src/slave/slave.hpp 57b18882e30e44dcc40449b0e3be8ee970c45bc8 
>   src/tests/slave_authorization_tests.cpp PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/46203/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Jan Schlicht
> 
>

Re: Review Request 46682: Made volume isolator logic injectable for MockDriverClient.

[Guangya Liu]

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




src/slave/containerizer/mesos/isolators/docker/volume/isolator.cpp (lines 83 - 
84)


Thanks Gilbert for the help! Sorry did not quite catch up how this can help 
inject the mock driver client here, we can sync up this tomorrow.


- Guangya Liu


On April 26, 2016, 8:32 a.m., Gilbert Song wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/46682/
> ---
> 
> (Updated April 26, 2016, 8:32 a.m.)
> 
> 
> Review request for mesos, Guangya Liu, Artem Harutyunyan, Jie Yu, and Timothy 
> Chen.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Made volume isolator logic injectable for MockDriverClient.
> 
> 
> Diffs
> -
> 
>   src/slave/containerizer/mesos/isolators/docker/volume/isolator.hpp 
> 5402973c04ea2045c9ec7b53b58795bf22c9a631 
>   src/slave/containerizer/mesos/isolators/docker/volume/isolator.cpp 
> 4675c767a562a3ca7f99265c9198a37786933c2d 
> 
> Diff: https://reviews.apache.org/r/46682/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Gilbert Song
> 
>

Re: Review Request 46203: Added authorization of the '/flags' endpoint.

[Jan Schlicht]

> On April 26, 2016, 8:27 a.m., Adam B wrote:
> > src/slave/http.cpp, line 360
> > 
> >
> > Should this perhaps be a `Shared<>`?
> 
> Jan Schlicht wrote:
> I don't think so. `Shared<>` is about shared ownership, but the closure 
> shouldn't own `Slave*`, but only use it. If we'd want to make sure that 
> `slave_ != NULL` during the lifetime of the closure, we would have to use 
> something like `std::weak_ptr`, managed by a shared pointer.
> I'd argue that because the `Slave` instance routes its HTTP requests to 
> an `Http` instance, `slave_ != NULL` during the lifetime of the `Http` 
> instance. Hence it's safe to use the pointer to `Slave` in the closure.
> 
> Anyways, I'll change the closure to use `Flags` instead of `Slave*` as a 
> parameter. The will be copied by value, object lifetime isn't a problem in 
> that case.

Can we drop this?


- Jan


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


On April 26, 2016, 1:10 p.m., Jan Schlicht wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/46203/
> ---
> 
> (Updated April 26, 2016, 1:10 p.m.)
> 
> 
> Review request for mesos, Adam B, Alexander Rojas, and Benjamin Bannier.
> 
> 
> Bugs: MESOS-5142
> https://issues.apache.org/jira/browse/MESOS-5142
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> See summary.
> 
> 
> Diffs
> -
> 
>   docs/configuration.md 318275fc5f935e6992ed4e8048cc4b42cc5d2cab 
>   include/mesos/authorizer/acls.proto 
> c50deeb5565dfd5b3e5e7210283d9a36a3bfd579 
>   include/mesos/authorizer/authorizer.proto 
> 40d93ea257d1df8d22eee8a21667db90d579a8fe 
>   src/Makefile.am e024c6d65608a55765e527a8668c415723dcfcca 
>   src/authorizer/local/authorizer.cpp 
> 0a3805fe4ce8eb89e096e8cd4326035513ba892b 
>   src/slave/flags.cpp a319d60c006d1104836c1c40f3617ceac9cb7b1e 
>   src/slave/http.cpp 537736d1fe42e8150bad91326299ef9a17041a8e 
>   src/slave/slave.hpp 57b18882e30e44dcc40449b0e3be8ee970c45bc8 
>   src/tests/slave_authorization_tests.cpp PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/46203/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Jan Schlicht
> 
>

Re: Review Request 46680: Fixed docker volume isolator const ref and vlog quotation.

[Guangya Liu]

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




src/slave/containerizer/mesos/isolators/docker/volume/isolator.cpp (line 201)


remove the quota here?



src/slave/containerizer/mesos/isolators/docker/volume/isolator.cpp (lines 289 - 
290)


According to 
https://github.com/apache/mesos/blob/master/docs/c%2B%2B-style-guide.md#capture-by-reference
 , seems we disallow capturing temporaries by reference?

Can you please add some detail in description for why using reference here?



src/slave/containerizer/mesos/isolators/docker/volume/isolator.cpp (line 473)


remove quota?


- Guangya Liu


On April 26, 2016, 8:32 a.m., Gilbert Song wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/46680/
> ---
> 
> (Updated April 26, 2016, 8:32 a.m.)
> 
> 
> Review request for mesos, Guangya Liu, Artem Harutyunyan, Jie Yu, and Timothy 
> Chen.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Fixed docker volume isolator const ref and vlog quotation.
> 
> 
> Diffs
> -
> 
>   src/slave/containerizer/mesos/isolators/docker/volume/isolator.cpp 
> 4675c767a562a3ca7f99265c9198a37786933c2d 
> 
> Diff: https://reviews.apache.org/r/46680/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Gilbert Song
> 
>

Re: Review Request 46203: Added authorization of the '/flags' endpoint.

[Jan Schlicht]

> On April 26, 2016, 8:27 a.m., Adam B wrote:
> > src/slave/http.cpp, line 362
> > 
> >
> > This function still assumes GET. Please pass a something like a Verb 
> > enum as a parameter, or else you'll need an `authorizeGetEndpoint()`, 
> > `authorizePostEndpoint()`, etc.

Okay, I'll add an enum that holds the verb (or method, as that's what it's 
called in `process::http::Request`. In this patch the enum will only have a 
`GET` value. Only when other patches need authorization of `POST` requests, 
will they have to add the `POST` value and add the respective authorization 
code in `authorizeEndpoint`.


- Jan


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


On April 25, 2016, 2:50 p.m., Jan Schlicht wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/46203/
> ---
> 
> (Updated April 25, 2016, 2:50 p.m.)
> 
> 
> Review request for mesos, Adam B, Alexander Rojas, and Benjamin Bannier.
> 
> 
> Bugs: MESOS-5142
> https://issues.apache.org/jira/browse/MESOS-5142
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> See summary.
> 
> 
> Diffs
> -
> 
>   docs/configuration.md 86ba66ac62295ca148524bcb2e57fee560ac4ac5 
>   include/mesos/authorizer/acls.proto 
> c50deeb5565dfd5b3e5e7210283d9a36a3bfd579 
>   include/mesos/authorizer/authorizer.proto 
> 40d93ea257d1df8d22eee8a21667db90d579a8fe 
>   src/Makefile.am e024c6d65608a55765e527a8668c415723dcfcca 
>   src/authorizer/local/authorizer.cpp 
> 0a3805fe4ce8eb89e096e8cd4326035513ba892b 
>   src/slave/flags.cpp 10d2974bd2b6e79255fc894979607f0d2d00c315 
>   src/slave/http.cpp 537736d1fe42e8150bad91326299ef9a17041a8e 
>   src/slave/slave.hpp 20a4bcd0bb9dad06ea81fc4ad9b2fa462c69d2c5 
>   src/tests/slave_authorization_tests.cpp PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/46203/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Jan Schlicht
> 
>

Re: Review Request 46373: Request /files/read.json with a negative length value causes error.

[Mesos ReviewBot]

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



Patch looks great!

Reviews applied: [46373]

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 26, 2016, 8:15 a.m., zhou xing wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/46373/
> ---
> 
> (Updated April 26, 2016, 8:15 a.m.)
> 
> 
> Review request for mesos, Ben Mahler and Greg Mann.
> 
> 
> Bugs: mesos-5060
> https://issues.apache.org/jira/browse/mesos-5060
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> [MESOS-5060]
> The patch did the following changes:
> 1. Fix the length logic in files.cpp.
> 2. Add some tests to test the /files/read.json endponit with
> negative length.
> 
> 
> Diffs
> -
> 
>   src/files/files.cpp 4e916101b378b0e9032a08a3f6c73e195b2a08a1 
>   src/tests/files_tests.cpp 5d6620d13babaf0bb7f9c888bb1b4fa2228b6ccd 
> 
> Diff: https://reviews.apache.org/r/46373/diff/
> 
> 
> Testing
> ---
> 
> make
> make check
> 
> request 'files/read.json' endpoint with negative offset or length argument
> 
> 
> Thanks,
> 
> zhou xing
> 
>

Re: Review Request 46203: Added authorization of the '/flags' endpoint.

[Jan Schlicht]

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

(Updated April 26, 2016, 1:10 p.m.)


Review request for mesos, Adam B, Alexander Rojas, and Benjamin Bannier.


Changes
---

Addressed issues and added an enum to distinguish between authorization types.


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


Repository: mesos


Description
---

See summary.


Diffs (updated)
-

  docs/configuration.md 318275fc5f935e6992ed4e8048cc4b42cc5d2cab 
  include/mesos/authorizer/acls.proto c50deeb5565dfd5b3e5e7210283d9a36a3bfd579 
  include/mesos/authorizer/authorizer.proto 
40d93ea257d1df8d22eee8a21667db90d579a8fe 
  src/Makefile.am e024c6d65608a55765e527a8668c415723dcfcca 
  src/authorizer/local/authorizer.cpp 0a3805fe4ce8eb89e096e8cd4326035513ba892b 
  src/slave/flags.cpp a319d60c006d1104836c1c40f3617ceac9cb7b1e 
  src/slave/http.cpp 537736d1fe42e8150bad91326299ef9a17041a8e 
  src/slave/slave.hpp 57b18882e30e44dcc40449b0e3be8ee970c45bc8 
  src/tests/slave_authorization_tests.cpp PRE-CREATION 

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


Testing
---

make check


Thanks,

Jan Schlicht

Re: Review Request 46687: Added test cases for passing lambda to `dispatch`.

[Mesos ReviewBot]

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



Patch looks great!

Reviews applied: [46686, 46687]

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 26, 2016, 9:21 a.m., haosdent huang wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/46687/
> ---
> 
> (Updated April 26, 2016, 9:21 a.m.)
> 
> 
> Review request for mesos, Kevin Klues and Michael Park.
> 
> 
> Bugs: MESOS-4611
> https://issues.apache.org/jira/browse/MESOS-4611
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Only used for test compile, please don't submit this.
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/src/tests/future_tests.cpp 
> 383d260a4c0b17e9a0b5af002bb35070e3ec0b01 
> 
> Diff: https://reviews.apache.org/r/46687/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> haosdent huang
> 
>

Re: Review Request 46681: Fixed docker volume isolator checkpointing empty info.

[Guangya Liu]

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




src/slave/containerizer/mesos/isolators/docker/volume/isolator.cpp (line 334)


s/containerId/container



src/slave/containerizer/mesos/isolators/docker/volume/isolator.cpp (line 337)


s/'info'/`info`


- Guangya Liu


On April 26, 2016, 8:32 a.m., Gilbert Song wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/46681/
> ---
> 
> (Updated April 26, 2016, 8:32 a.m.)
> 
> 
> Review request for mesos, Guangya Liu, Artem Harutyunyan, Jie Yu, and Timothy 
> Chen.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Fixed docker volume isolator checkpointing empty info.
> 
> 
> Diffs
> -
> 
>   src/slave/containerizer/mesos/isolators/docker/volume/isolator.cpp 
> 4675c767a562a3ca7f99265c9198a37786933c2d 
> 
> Diff: https://reviews.apache.org/r/46681/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Gilbert Song
> 
>

Re: Review Request 46621: Added alias support for flags.

[Vinod Kone]

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




3rdparty/libprocess/3rdparty/stout/include/stout/flags/flags.hpp (line 386)


use `->`  instead of .get(). here and everywhere else.



3rdparty/libprocess/3rdparty/stout/include/stout/flags/flags.hpp (line 853)


refactor this load (in a separate review) to be more readable.



3rdparty/libprocess/3rdparty/stout/include/stout/flags/flags.hpp (line 999)


instead of `--{a, b}=VALUE` do `--a=VALUE, --b=VALUE`



3rdparty/libprocess/3rdparty/stout/include/stout/flags/flags.hpp (line 1007)


change the format of the usage to do it in the next line and change 
`=VALUE` to `=`


- Vinod Kone


On April 26, 2016, 1:04 a.m., Vinod Kone wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/46621/
> ---
> 
> (Updated April 26, 2016, 1:04 a.m.)
> 
> 
> Review request for mesos and Ben Mahler.
> 
> 
> Bugs: MESOS-5271
> https://issues.apache.org/jira/browse/MESOS-5271
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added alias support for flags.
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/3rdparty/stout/include/stout/flags/flag.hpp 
> 2502d9e8a515b9adc1b3aa2f719e5710a0e7ef29 
>   3rdparty/libprocess/3rdparty/stout/include/stout/flags/flags.hpp 
> c3cbcdb781e1c282d381de1ad2bf4f386ee1db21 
> 
> Diff: https://reviews.apache.org/r/46621/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Vinod Kone
> 
>

Review Request 46725: Set default Content-Type for HTTP responses.

[Vinod Kone]

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

Review request for mesos and Anand Mazumdar.


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


Repository: mesos


Description
---

The default Content-Type for HTTP responses that contain body is set
as "text/plain; charset=utf-8."


Diffs
-

  3rdparty/libprocess/include/process/http.hpp 
8f4eabcbb71ead1f5c28e1d8a2dd40db7af1f297 
  3rdparty/libprocess/src/tests/encoder_tests.cpp 
61ec8d8722245179a929e954e6338606973b819b 

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


Testing
---

make check


Thanks,

Vinod Kone

Re: Review Request 46220: Added documentation for Nvidia GPU support.

[Qian Zhang]

> On April 22, 2016, 5:55 p.m., Qian Zhang wrote:
> > docs/gpu-support.md, line 440
> > 
> >
> > Does this limitation mean that currently we do not support container 
> > with an image (e.g., Docker image, Appc image)? Instead we only support 
> > image-less container since such kind of containers will be able to directly 
> > access agent host filesystem so that they can access the needed GPU library 
> > to perform GPU-aware jobs.
> 
> Kevin Klues wrote:
> Yes. This is what it means currently.  However, I am planning to remove 
> this restriction before the 0.29 release.
> 
> Qian Zhang wrote:
> Can you please let me know a bit more about why removing this restriction 
> before 0.29 release? Do we already have a solution to make the container with 
> an image can access the GPU library?
> 
> Qian Zhang wrote:
> @Kevin, any comments? :-)

And what about user has already manually created a Docker image with the needed 
GPU library in it? I think our current implementation should also support 
container created from such image, right?


- Qian


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


On April 15, 2016, 4:33 a.m., Kevin Klues wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/46220/
> ---
> 
> (Updated April 15, 2016, 4:33 a.m.)
> 
> 
> Review request for mesos, Ben Mahler, Rob Todd, and Vikrama Ditya.
> 
> 
> Bugs: MESOS-5221
> https://issues.apache.org/jira/browse/MESOS-5221
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added documentation for Nvidia GPU support.
> 
> 
> Diffs
> -
> 
>   docs/gpu-support.md PRE-CREATION 
>   docs/home.md dc41fc4479e6c23650cd8ac78dcc4b9161d00721 
> 
> Diff: https://reviews.apache.org/r/46220/diff/
> 
> 
> Testing
> ---
> 
> Ran a local copy of the documentation site to make sure everything looks OK.
> 
> 
> Thanks,
> 
> Kevin Klues
> 
>

Re: Review Request 46676: Slave/Agent Rename Phase I: Rename '/include/mesos/slave' folder.

[zhou xing]

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

(Updated 四月 27, 2016, 1:57 a.m.)


Review request for mesos, Kevin Klues and Vinod Kone.


Changes
---

Rebase code, change the commit description and update the makefile target 
comments


Bugs: mesos-5230
https://issues.apache.org/jira/browse/mesos-5230


Repository: mesos


Description (updated)
---

This patch did the following changes:

1. Change the 'make install' headers installation location from
'$(DESTDIR)/include/mesos/slave' to '$(DESTDIR)include/mesos/agent'

2. As we do not change the folder name from 'include/mesos/slave'
to 'include/mesos/agent', create a 'slave -> agent' symlink in dir
'$(DESTDIR)/include/mesos'


Diffs (updated)
-

  CHANGELOG 57f5457640c9b7ca2cb7707f2962dd0421f090f7 
  src/Makefile.am e024c6d65608a55765e527a8668c415723dcfcca 

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


Testing
---

cd mesos
./bootstrap
mkdir build
cd build
../configure --prefix=${HOME}/install/mesos
make
make check
make install


Thanks,

zhou xing

Re: Review Request 46609: [WIP] Add the test "SlaveRecoveryTest.RecoverTerminatedHTTPExecutor".

[Mesos ReviewBot]

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



Bad patch!

Reviews applied: [46609, 46204, 46188, 46187]

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

Error:
2016-04-27 02:08:39 URL:https://reviews.apache.org/r/46609/diff/raw/ 
[5233/5233] -> "46609.patch" [1]
Total errors found: 0
Checking 1 files
Error: Commit message summary (the first line) must start with a capital letter.

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

- Mesos ReviewBot


On April 27, 2016, 1:26 a.m., Qian Zhang wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/46609/
> ---
> 
> (Updated April 27, 2016, 1:26 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
> ---
> 
> Add the test "SlaveRecoveryTest.RecoverTerminatedHTTPExecutor".
> 
> 
> Diffs
> -
> 
>   src/tests/slave_recovery_tests.cpp 8ad63224e0fdd203cb8dbfbc2d2484e3ce52a10c 
> 
> Diff: https://reviews.apache.org/r/46609/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Qian Zhang
> 
>

Review Request 46717: Added sketch of fetcher cache metrics.

[Michael Browning]

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

Review request for mesos and Bernd Mathiske.


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


Repository: mesos


Description
---

Added sketch of fetcher cache metrics.


Diffs
-

  src/slave/containerizer/fetcher.hpp 9e3018dc087ed55c61b2824d0105bc5339b83043 
  src/slave/containerizer/fetcher.cpp 176d8863d1becd8864218a0012ab45c614f0ad77 
  src/slave/metrics.cpp 86eb8db644227cb593e52305bfbd05444bb87a6e 
  src/slave/slave.hpp 57b18882e30e44dcc40449b0e3be8ee970c45bc8 

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


Testing
---


Thanks,

Michael Browning

Re: Review Request 46676: Slave/Agent Rename Phase I: Rename '/include/mesos/slave' folder.

[Mesos ReviewBot]

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



Patch looks great!

Reviews applied: [46720, 46676]

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 27, 2016, 1:57 a.m., zhou xing wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/46676/
> ---
> 
> (Updated April 27, 2016, 1:57 a.m.)
> 
> 
> Review request for mesos, Kevin Klues and Vinod Kone.
> 
> 
> Bugs: mesos-5230
> https://issues.apache.org/jira/browse/mesos-5230
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This patch did the following changes:
> 
> 1. Change the 'make install' headers installation location from
> '$(DESTDIR)/include/mesos/slave' to '$(DESTDIR)include/mesos/agent'
> 
> 2. As we do not change the folder name from 'include/mesos/slave'
> to 'include/mesos/agent', create a 'slave -> agent' symlink in dir
> '$(DESTDIR)/include/mesos'
> 
> 
> Diffs
> -
> 
>   CHANGELOG 57f5457640c9b7ca2cb7707f2962dd0421f090f7 
>   src/Makefile.am e024c6d65608a55765e527a8668c415723dcfcca 
> 
> Diff: https://reviews.apache.org/r/46676/diff/
> 
> 
> Testing
> ---
> 
> cd mesos
> ./bootstrap
> mkdir build
> cd build
> ../configure --prefix=${HOME}/install/mesos
> make
> make check
> make install
> 
> 
> Thanks,
> 
> zhou xing
> 
>

Review Request 46723: Send error message to the framework when it is not connected.

[Anand Mazumdar]

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

Review request for mesos and Vinod Kone.


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


Repository: mesos


Description
---

This change adds logic to send the framework an error message
if the master receives a call from the framework after it has
disconnected. Since driver based frameworks don't receive
heartbeats from the master. There is no way for them to detect
this unless a new master is detected that eventually triggers
re-registration.


Diffs
-

  src/master/master.cpp ff41da3d077b65b44277e1bbae88c61b7bb88a3d 

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


Testing
---

Added test in r46724


Thanks,

Anand Mazumdar

Review Request 46722: Made the master detection log messages more verbose.

[Anand Mazumdar]

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

Review request for mesos and Vinod Kone.


Repository: mesos


Description
---

This change increases the verbosity of master detection messages
in the scheduler library to be in sync with the verbosity level
in the driver.


Diffs
-

  src/scheduler/scheduler.cpp c75e02c3c0de2527631adc88c080786f39a7fea6 

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


Testing
---

make check


Thanks,

Anand Mazumdar

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

[Qian Zhang]

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

(Updated April 27, 2016, 9:24 a.m.)


Review request for mesos, Anand Mazumdar and Vinod Kone.


Changes
---

Rebased


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


Repository: mesos


Description
---

Added the missing 'break' when handling ERROR event.


Diffs (updated)
-

  src/launcher/http_command_executor.cpp 
0b4136c040ec9cf585c0d38f8471495a855cd640 

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


Testing
---

make check


Thanks,

Qian Zhang

Re: 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/
---

(Updated April 27, 2016, 9:25 a.m.)


Review request for mesos, Anand Mazumdar and Vinod Kone.


Changes
---

Rebased


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


Repository: mesos


Description
---

Updated slave recovery tests with HTTP command executor.


Diffs (updated)
-

  src/tests/slave_recovery_tests.cpp 8ad63224e0fdd203cb8dbfbc2d2484e3ce52a10c 

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

Review Request 46720: Fix the titles of 'Slave/Agent Rename' series tickets in CHANGELOG.

[zhou xing]

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

Review request for mesos, Kevin Klues and Vinod Kone.


Bugs: mesos-5230
https://issues.apache.org/jira/browse/mesos-5230


Repository: mesos


Description
---

Fix the following tickets' title in CHANGELOG to use 'Slave/Agent':

* [MESOS-5055] - Slave/Agent Rename Phase I - Update strings in the
  log message and standard output.
* [MESOS-3782] - Slave/Agent Rename Phase I - Duplicate/Rename
  binaries.
* [MESOS-5057] - Slave/Agent Rename Phase I - Update strings in
  error messages and other strings.


Diffs
-

  CHANGELOG 57f5457640c9b7ca2cb7707f2962dd0421f090f7 

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


Testing
---

make
make check


Thanks,

zhou xing

Re: Review Request 46720: Fix the titles of 'Slave/Agent Rename' series tickets in CHANGELOG.

[Vinod Kone]

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


Ship it!




Ship It!

- Vinod Kone


On April 27, 2016, 1:56 a.m., zhou xing wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/46720/
> ---
> 
> (Updated April 27, 2016, 1:56 a.m.)
> 
> 
> Review request for mesos, Kevin Klues and Vinod Kone.
> 
> 
> Bugs: mesos-5230
> https://issues.apache.org/jira/browse/mesos-5230
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Fix the following tickets' title in CHANGELOG to use 'Slave/Agent':
> 
> * [MESOS-5055] - Slave/Agent Rename Phase I - Update strings in the
>   log message and standard output.
> * [MESOS-3782] - Slave/Agent Rename Phase I - Duplicate/Rename
>   binaries.
> * [MESOS-5057] - Slave/Agent Rename Phase I - Update strings in
>   error messages and other strings.
> 
> 
> Diffs
> -
> 
>   CHANGELOG 57f5457640c9b7ca2cb7707f2962dd0421f090f7 
> 
> Diff: https://reviews.apache.org/r/46720/diff/
> 
> 
> Testing
> ---
> 
> make
> make check
> 
> 
> Thanks,
> 
> zhou xing
> 
>

Re: Review Request 46676: Slave/Agent Rename Phase I: Rename '/include/mesos/slave' folder.

[Kevin Klues]

> On April 26, 2016, 7:08 p.m., Vinod Kone wrote:
> > src/Makefile.am, line 535
> > 
> >
> > what does setting this variable do?

This is an autotools thing. If you define this and then list which headers to 
installe here with e.g. `agent_HEADERS`, they will be installed here.  This is 
the right thing to do, in my opinion.


> On April 26, 2016, 7:08 p.m., Vinod Kone wrote:
> > src/Makefile.am, lines 2151-2152
> > 
> >
> > IIUC, when someone does a  `make install` there will be 2 directories 
> > in the installation directory.
> > 
> > ./mesos/agent
> > ./mesos/slave  (this is a symlink to ../agent)
> > 
> > In the git repo itself we will still have `include/mesos/slave` 
> > directory? I guess renaming this directory to `incluee/mesos/agent`? is too 
> > much work?

The `include/mesos/slave` directory is decoupled from the actually install 
directory specified by `agentdir`.  We can decide to change 
`include/mesos/slave` to `include/mesos/agent` in the future, but it has 
nothing to do with this installation directory.


- Kevin


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


On April 26, 2016, 6:06 a.m., zhou xing wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/46676/
> ---
> 
> (Updated April 26, 2016, 6:06 a.m.)
> 
> 
> Review request for mesos, Kevin Klues and Vinod Kone.
> 
> 
> Bugs: mesos-5230
> https://issues.apache.org/jira/browse/mesos-5230
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> [#MESOS-5230]
> Change the 'make install' includedir name from slave to agent.
> 
> 
> Diffs
> -
> 
>   CHANGELOG 57f5457640c9b7ca2cb7707f2962dd0421f090f7 
>   src/Makefile.am e024c6d65608a55765e527a8668c415723dcfcca 
> 
> Diff: https://reviews.apache.org/r/46676/diff/
> 
> 
> Testing
> ---
> 
> cd mesos
> ./bootstrap
> mkdir build
> cd build
> ../configure --prefix=${HOME}/install/mesos
> make
> make check
> make install
> 
> 
> Thanks,
> 
> zhou xing
> 
>

Re: Review Request 46676: Slave/Agent Rename Phase I: Rename '/include/mesos/slave' folder.

[zhou xing]

> On 四月 26, 2016, 7:08 p.m., Vinod Kone wrote:
> > src/Makefile.am, lines 2151-2152
> > 
> >
> > IIUC, when someone does a  `make install` there will be 2 directories 
> > in the installation directory.
> > 
> > ./mesos/agent
> > ./mesos/slave  (this is a symlink to ../agent)
> > 
> > In the git repo itself we will still have `include/mesos/slave` 
> > directory? I guess renaming this directory to `incluee/mesos/agent`? is too 
> > much work?
> 
> Kevin Klues wrote:
> The `include/mesos/slave` directory is decoupled from the actually 
> install directory specified by `agentdir`.  We can decide to change 
> `include/mesos/slave` to `include/mesos/agent` in the future, but it has 
> nothing to do with this installation directory.

If we want to change include/mesos/slave to include/mesos/agent in the future, 
for the files that depend on the headers in include/mesos/slave, we need to 
keep the path, is that ok?


- zhou


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


On 四月 26, 2016, 6:06 a.m., zhou xing wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/46676/
> ---
> 
> (Updated 四月 26, 2016, 6:06 a.m.)
> 
> 
> Review request for mesos, Kevin Klues and Vinod Kone.
> 
> 
> Bugs: mesos-5230
> https://issues.apache.org/jira/browse/mesos-5230
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> [#MESOS-5230]
> Change the 'make install' includedir name from slave to agent.
> 
> 
> Diffs
> -
> 
>   CHANGELOG 57f5457640c9b7ca2cb7707f2962dd0421f090f7 
>   src/Makefile.am e024c6d65608a55765e527a8668c415723dcfcca 
> 
> Diff: https://reviews.apache.org/r/46676/diff/
> 
> 
> Testing
> ---
> 
> cd mesos
> ./bootstrap
> mkdir build
> cd build
> ../configure --prefix=${HOME}/install/mesos
> make
> make check
> make install
> 
> 
> Thanks,
> 
> zhou xing
> 
>

Re: Review Request 46609: [WIP] Add the test "SlaveRecoveryTest.RecoverTerminatedHTTPExecutor".

[Qian Zhang]

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

(Updated April 27, 2016, 9:26 a.m.)


Review request for mesos, Anand Mazumdar and Vinod Kone.


Summary (updated)
-

[WIP] Add the test "SlaveRecoveryTest.RecoverTerminatedHTTPExecutor".


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


Repository: mesos


Description
---

Add the test "SlaveRecoveryTest.RecoverTerminatedHTTPExecutor".


Diffs
-

  src/tests/slave_recovery_tests.cpp 8ad63224e0fdd203cb8dbfbc2d2484e3ce52a10c 

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


Testing
---


Thanks,

Qian Zhang

Re: Review Request 46609: Add the test "SlaveRecoveryTest.RecoverTerminatedHTTPExecutor".

[Qian Zhang]

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

(Updated April 27, 2016, 9:25 a.m.)


Review request for mesos, Anand Mazumdar and Vinod Kone.


Changes
---

Rebased


Summary (updated)
-

Add the test "SlaveRecoveryTest.RecoverTerminatedHTTPExecutor".


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


Repository: mesos


Description
---

Add the test "SlaveRecoveryTest.RecoverTerminatedHTTPExecutor".


Diffs (updated)
-

  src/tests/slave_recovery_tests.cpp 8ad63224e0fdd203cb8dbfbc2d2484e3ce52a10c 

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


Testing
---


Thanks,

Qian Zhang

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

[Qian Zhang]

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

(Updated April 27, 2016, 9:23 a.m.)


Review request for mesos, Anand Mazumdar and Vinod Kone.


Changes
---

Rebased


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


Repository: mesos


Description
---

Terminate when receiving the ACK of terminal status update.


Diffs (updated)
-

  src/launcher/http_command_executor.cpp 
0b4136c040ec9cf585c0d38f8471495a855cd640 

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


Testing
---

make check


Thanks,

Qian Zhang

Re: Review Request 46682: Made volume isolator logic injectable for MockDriverClient.

[Mesos ReviewBot]

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



Patch looks great!

Reviews applied: [46680, 46681, 46682]

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 26, 2016, 8:32 a.m., Gilbert Song wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/46682/
> ---
> 
> (Updated April 26, 2016, 8:32 a.m.)
> 
> 
> Review request for mesos, Guangya Liu, Artem Harutyunyan, Jie Yu, and Timothy 
> Chen.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Made volume isolator logic injectable for MockDriverClient.
> 
> 
> Diffs
> -
> 
>   src/slave/containerizer/mesos/isolators/docker/volume/isolator.hpp 
> 5402973c04ea2045c9ec7b53b58795bf22c9a631 
>   src/slave/containerizer/mesos/isolators/docker/volume/isolator.cpp 
> 4675c767a562a3ca7f99265c9198a37786933c2d 
> 
> Diff: https://reviews.apache.org/r/46682/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Gilbert Song
> 
>

Re: Review Request 46319: Added authorization to agents' `/monitor/statistics` endpoints.

[Alexander Rojas]

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


Ship it!




Ship It!

- Alexander Rojas


On April 26, 2016, 3:34 p.m., Benjamin Bannier wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/46319/
> ---
> 
> (Updated April 26, 2016, 3:34 p.m.)
> 
> 
> Review request for mesos, Adam B, Alexander Rojas, and Jan Schlicht.
> 
> 
> Bugs: MESOS-5164
> https://issues.apache.org/jira/browse/MESOS-5164
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> See summary.
> 
> 
> Diffs
> -
> 
>   src/slave/http.cpp 537736d1fe42e8150bad91326299ef9a17041a8e 
>   src/slave/slave.hpp 57b18882e30e44dcc40449b0e3be8ee970c45bc8 
>   src/tests/slave_authorization_tests.cpp PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/46319/diff/
> 
> 
> Testing
> ---
> 
> make check (OS X, clang w/o optimization)
> 
> 
> Thanks,
> 
> Benjamin Bannier
> 
>

Re: Review Request 46319: Added authorization to agents' `/monitor/statistics` endpoints.

[Benjamin Bannier]

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

(Updated April 26, 2016, 1:34 p.m.)


Review request for mesos, Adam B, Alexander Rojas, and Jan Schlicht.


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


Repository: mesos


Description (updated)
---

See summary.


Diffs
-

  src/slave/http.cpp 537736d1fe42e8150bad91326299ef9a17041a8e 
  src/slave/slave.hpp 57b18882e30e44dcc40449b0e3be8ee970c45bc8 
  src/tests/slave_authorization_tests.cpp PRE-CREATION 

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


Testing
---

make check (OS X, clang w/o optimization)


Thanks,

Benjamin Bannier

Re: Review Request 46461: Updated gperftools to version 2.5 (libprocess).

[Kapil Arya]

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


Ship it!




Ship It!

- Kapil Arya


On April 25, 2016, 1:25 p.m., Greg Mann wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/46461/
> ---
> 
> (Updated April 25, 2016, 1:25 p.m.)
> 
> 
> Review request for mesos and Kapil Arya.
> 
> 
> Bugs: MESOS-3319
> https://issues.apache.org/jira/browse/MESOS-3319
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Updated gperftools to version 2.5 (libprocess).
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/3rdparty/gperftools-2.0.tar.gz 
> 13b03cae44e5e6dcfdec4756b975f425b2ea73cf 
>   3rdparty/libprocess/3rdparty/gperftools-2.5.tar.gz PRE-CREATION 
>   3rdparty/libprocess/3rdparty/versions.am 5506eb3 
> 
> Diff: https://reviews.apache.org/r/46461/diff/
> 
> 
> Testing
> ---
> 
> `sudo make check`
> 
> 
> Thanks,
> 
> Greg Mann
> 
>

Re: Review Request 46501: Updated authorization.md to reflect current changes.

[Alexander Rojas]

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

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


Review request for mesos, Adam B, Greg Mann, and Neil Conway.


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


Repository: mesos


Description
---

The API of the authorization has been changing constantly over the
last few versions. This patch attempts to update the documentation to
the those changes into account.


Diffs (updated)
-

  docs/authorization.md 0db5c345b3239814b3b9d2e8a87601ff69d0f869 

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


Testing
---


Thanks,

Alexander Rojas

Review Request 46686: Allowed to pass lambda in `dispatch`.

[haosdent huang]

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

Review request for mesos, Kevin Klues and Michael Park.


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


Repository: mesos


Description
---

Allowed to pass lambda in `dispatch`.


Diffs
-

  3rdparty/libprocess/include/process/dispatch.hpp 
a4c35b2a5668df79415dc5156358df3cd0621d11 

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


Testing
---


Thanks,

haosdent huang

Review Request 46687: Added test cases for passing lambda to `dispatch`.

[haosdent huang]

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

Review request for mesos, Kevin Klues and Michael Park.


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


Repository: mesos


Description
---

Only used for test compile, please don't submit this.


Diffs
-

  3rdparty/libprocess/src/tests/future_tests.cpp 
383d260a4c0b17e9a0b5af002bb35070e3ec0b01 

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


Testing
---


Thanks,

haosdent huang

Re: Review Request 46319: Added authorization to agents' `/monitor/statistics` endpoints.

[Alexander Rukletsov]

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


Fix it, then Ship it!





src/slave/http.cpp (line 627)


Indent the block by 2 spaces, not 4.



src/slave/http.cpp (line 635)


Indent the block by 2 spaces, not 4.



src/slave/http.cpp (line 637)


Indent the block by 2 spaces, not 4.



src/slave/http.cpp 


You remove the `Future` here. I believe this is on purpose as it seems the 
right way to pass the parameter (without future). Is it a bugfix or it doesn't 
influence the thrust?



src/tests/slave_authorization_tests.cpp (lines 172 - 173)


I was about to ask why don't you use parameterized tests, but noticed you 
do it in the next review. Any reason you don't want to fix it up in this patch?



src/tests/slave_authorization_tests.cpp (lines 198 - 200)


Maybe kill this blank lines?


- Alexander Rukletsov


On April 26, 2016, 1:34 p.m., Benjamin Bannier wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/46319/
> ---
> 
> (Updated April 26, 2016, 1:34 p.m.)
> 
> 
> Review request for mesos, Adam B, Alexander Rojas, and Jan Schlicht.
> 
> 
> Bugs: MESOS-5164
> https://issues.apache.org/jira/browse/MESOS-5164
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> See summary.
> 
> 
> Diffs
> -
> 
>   src/slave/http.cpp 537736d1fe42e8150bad91326299ef9a17041a8e 
>   src/slave/slave.hpp 57b18882e30e44dcc40449b0e3be8ee970c45bc8 
>   src/tests/slave_authorization_tests.cpp PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/46319/diff/
> 
> 
> Testing
> ---
> 
> make check (OS X, clang w/o optimization)
> 
> 
> Thanks,
> 
> Benjamin Bannier
> 
>

Re: Review Request 46569: Parameterized agent endpoint authorization tests on tested endpoint.

[Alexander Rukletsov]

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


Fix it, then Ship it!





src/tests/slave_authorization_tests.cpp (line 172)


Ooops, `{` belongs to a separate line.



src/tests/slave_authorization_tests.cpp (line 215)


Let's not wrap it, it reduces readability!



src/tests/slave_authorization_tests.cpp (lines 244 - 249)


Does it make sense to pull it up, around L169?



src/tests/slave_authorization_tests.cpp (line 246)


s/SlaveAuthorizationTest/Endpoint

I'd say it's more descriptive.


- Alexander Rukletsov


On April 26, 2016, 7:59 a.m., Benjamin Bannier wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/46569/
> ---
> 
> (Updated April 26, 2016, 7:59 a.m.)
> 
> 
> Review request for mesos and Adam B.
> 
> 
> Bugs: MESOS-5164
> https://issues.apache.org/jira/browse/MESOS-5164
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> At the moment we only provide a harness for `GET` methods, but will
> generalize this further once we authorize `POST` requests against
> agent endpoints as well.
> 
> 
> Diffs
> -
> 
>   src/tests/slave_authorization_tests.cpp PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/46569/diff/
> 
> 
> Testing
> ---
> 
> make check (OS X, clang trunk w/o optimization)
> 
> 
> Thanks,
> 
> Benjamin Bannier
> 
>

Re: Review Request 46641: Added tests for the Profiler to libprocess.

[Greg Mann]

> On April 26, 2016, 2:40 p.m., Kapil Arya wrote:
> > Should we also add another test(s) that sets/unsets the environment 
> > variable to ensure coverage? We can do that in a separate request.

The difficulty here is that with the current implementation, the environment 
variable has to be set *before* libprocess gets initialized, so we can't set it 
dynamically during the tests.


- Greg


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


On April 25, 2016, 8:47 p.m., Greg Mann wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/46641/
> ---
> 
> (Updated April 25, 2016, 8:47 p.m.)
> 
> 
> Review request for mesos, Alexander Rojas and Kapil Arya.
> 
> 
> Bugs: MESOS-4902
> https://issues.apache.org/jira/browse/MESOS-4902
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added tests for the Profiler to libprocess.
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/Makefile.am c51c31eb91dd7be4cff8a188942ea77b3b361d45 
>   3rdparty/libprocess/src/tests/profiler_tests.cpp PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/46641/diff/
> 
> 
> Testing
> ---
> 
> `make check` in each of the following configurations:
> * after `../configure`
> * after `../configure --enable-perftools && export 
> LIBPROCESS_ENABLE_PROFILER=0`
> * after `../configure --enable-perftools && export 
> LIBPROCESS_ENABLE_PROFILER=1`
> 
> 
> Thanks,
> 
> Greg Mann
> 
>

Re: Review Request 46501: Updated authorization.md to reflect current changes.

[Mesos ReviewBot]

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



Patch looks great!

Reviews applied: [46501]

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 26, 2016, 3:14 p.m., Alexander Rojas wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/46501/
> ---
> 
> (Updated April 26, 2016, 3:14 p.m.)
> 
> 
> Review request for mesos, Adam B, Greg Mann, and Neil Conway.
> 
> 
> Bugs: MESOS-4785
> https://issues.apache.org/jira/browse/MESOS-4785
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> The API of the authorization has been changing constantly over the
> last few versions. This patch attempts to update the documentation to
> the those changes into account.
> 
> 
> Diffs
> -
> 
>   docs/authorization.md 0db5c345b3239814b3b9d2e8a87601ff69d0f869 
> 
> Diff: https://reviews.apache.org/r/46501/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Alexander Rojas
> 
>

Re: Review Request 46203: Added authorization of the '/flags' endpoint.

[Jan Schlicht]

> On April 26, 2016, 5:40 p.m., Alexander Rukletsov wrote:
> > src/slave/slave.hpp, lines 471-473
> > 
> >
> > It feels like this class should be somewhere in a more general place. 
> > Moreover, libprocess' `Request` struct has the `method` field, which is a 
> > string. Any reason why you introduce an enum here?

Take a look at revision 16 please :D


> On April 26, 2016, 5:40 p.m., Alexander Rukletsov wrote:
> > docs/configuration.md, line 899
> > 
> >
> > Let's entertain our reader a bit : ). Instead of "a", we can use 
> > something like "padavan", and if you plan to introduce say 
> > `post_endpoints`, you can reuse "yoda".

All the other examples for principals use "a" and "b", though. If we want to 
introduce better principal names, we should do it everywhere.


- Jan


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


On April 26, 2016, 5:27 p.m., Jan Schlicht wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/46203/
> ---
> 
> (Updated April 26, 2016, 5:27 p.m.)
> 
> 
> Review request for mesos, Adam B, Alexander Rojas, and Benjamin Bannier.
> 
> 
> Bugs: MESOS-5142
> https://issues.apache.org/jira/browse/MESOS-5142
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> See summary.
> 
> 
> Diffs
> -
> 
>   docs/configuration.md 318275fc5f935e6992ed4e8048cc4b42cc5d2cab 
>   include/mesos/authorizer/acls.proto 
> c50deeb5565dfd5b3e5e7210283d9a36a3bfd579 
>   include/mesos/authorizer/authorizer.proto 
> 40d93ea257d1df8d22eee8a21667db90d579a8fe 
>   src/Makefile.am e024c6d65608a55765e527a8668c415723dcfcca 
>   src/authorizer/local/authorizer.cpp 
> 0a3805fe4ce8eb89e096e8cd4326035513ba892b 
>   src/slave/flags.cpp a319d60c006d1104836c1c40f3617ceac9cb7b1e 
>   src/slave/http.cpp 537736d1fe42e8150bad91326299ef9a17041a8e 
>   src/slave/slave.hpp 57b18882e30e44dcc40449b0e3be8ee970c45bc8 
>   src/tests/slave_authorization_tests.cpp PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/46203/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Jan Schlicht
> 
>

Re: Review Request 46569: Parameterized agent endpoint authorization tests on tested endpoint.

[Benjamin Bannier]

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

(Updated April 26, 2016, 8:22 p.m.)


Review request for mesos and Adam B.


Changes
---

Stylish fixes w/ alexr's guidance.


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


Repository: mesos


Description
---

At the moment we only provide a harness for `GET` methods, but will
generalize this further once we authorize `POST` requests against
agent endpoints as well.


Diffs (updated)
-

  src/tests/slave_authorization_tests.cpp PRE-CREATION 

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


Testing
---

make check (OS X, clang trunk w/o optimization)


Thanks,

Benjamin Bannier

Re: Review Request 46569: Parameterized agent endpoint authorization tests on tested endpoint.

[Benjamin Bannier]

> On April 26, 2016, 6:45 p.m., Alexander Rukletsov wrote:
> > src/tests/slave_authorization_tests.cpp, line 251
> > 
> >
> > s/SlaveAuthorizationTest/Endpoint
> > 
> > I'd say it's more descriptive.

Right now the tests will appear as e.g., 
`SlaveAuthorizationTest/EndpointAuthorization.Endpoint/0`. I do not agree that 
calling them `Endpoint/EndpointAuthorization.Endpoint/0` is more descriptive.


> On April 26, 2016, 6:45 p.m., Alexander Rukletsov wrote:
> > src/tests/slave_authorization_tests.cpp, lines 172-173
> > 
> >
> > Ooops, `{` belongs to a separate line.

Ups, fixed now.


> On April 26, 2016, 6:45 p.m., Alexander Rukletsov wrote:
> > src/tests/slave_authorization_tests.cpp, lines 218-219
> > 
> >
> > Let's not wrap it, it reduces readability!

So be it.


- Benjamin


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


On April 26, 2016, 8:22 p.m., Benjamin Bannier wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/46569/
> ---
> 
> (Updated April 26, 2016, 8:22 p.m.)
> 
> 
> Review request for mesos and Adam B.
> 
> 
> Bugs: MESOS-5164
> https://issues.apache.org/jira/browse/MESOS-5164
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> At the moment we only provide a harness for `GET` methods, but will
> generalize this further once we authorize `POST` requests against
> agent endpoints as well.
> 
> 
> Diffs
> -
> 
>   src/tests/slave_authorization_tests.cpp PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/46569/diff/
> 
> 
> Testing
> ---
> 
> make check (OS X, clang trunk w/o optimization)
> 
> 
> Thanks,
> 
> Benjamin Bannier
> 
>

Re: Review Request 46319: Added authorization to agents' `/monitor/statistics` endpoints.

[Benjamin Bannier]

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

(Updated April 26, 2016, 8:22 p.m.)


Review request for mesos, Adam B, Alexander Rojas, and Jan Schlicht.


Changes
---

Rebased.


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


Repository: mesos


Description (updated)
---

Added authorization to agents' `/monitor/statistics` endpoints.


Diffs (updated)
-

  src/slave/http.cpp 537736d1fe42e8150bad91326299ef9a17041a8e 
  src/slave/slave.hpp 57b18882e30e44dcc40449b0e3be8ee970c45bc8 
  src/tests/slave_authorization_tests.cpp PRE-CREATION 

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


Testing
---

make check (OS X, clang w/o optimization)


Thanks,

Benjamin Bannier

Re: Review Request 46203: Added authorization of the '/flags' endpoint.

[Jan Schlicht]

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

(Updated April 26, 2016, 8:29 p.m.)


Review request for mesos, Adam B, Alexander Rojas, and Benjamin Bannier.


Changes
---

Uses an equivalent but simpler validation if-clause.


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


Repository: mesos


Description
---

See summary.


Diffs (updated)
-

  docs/configuration.md 2796a812b72f208b1ae2d65a4ba843b50d70 
  include/mesos/authorizer/acls.proto c50deeb5565dfd5b3e5e7210283d9a36a3bfd579 
  include/mesos/authorizer/authorizer.proto 
40d93ea257d1df8d22eee8a21667db90d579a8fe 
  src/Makefile.am e024c6d65608a55765e527a8668c415723dcfcca 
  src/authorizer/local/authorizer.cpp 0a3805fe4ce8eb89e096e8cd4326035513ba892b 
  src/slave/flags.cpp a319d60c006d1104836c1c40f3617ceac9cb7b1e 
  src/slave/http.cpp 537736d1fe42e8150bad91326299ef9a17041a8e 
  src/slave/slave.hpp 57b18882e30e44dcc40449b0e3be8ee970c45bc8 
  src/tests/slave_authorization_tests.cpp PRE-CREATION 

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


Testing
---

make check


Thanks,

Jan Schlicht

Re: Review Request 46319: Added authorization to agents' `/monitor/statistics` endpoints.

[Benjamin Bannier]

> On April 26, 2016, 6:51 p.m., Alexander Rukletsov wrote:
> > src/slave/http.cpp, line 655
> > 
> >
> > You remove the `Future` here. I believe this is on purpose as it seems 
> > the right way to pass the parameter (without future). Is it a bugfix or it 
> > doesn't influence the thrust?

Did you review the latest revision?


> On April 26, 2016, 6:51 p.m., Alexander Rukletsov wrote:
> > src/tests/slave_authorization_tests.cpp, lines 198-200
> > 
> >
> > Maybe kill this blank lines?

Let's leave some room here.


> On April 26, 2016, 6:51 p.m., Alexander Rukletsov wrote:
> > src/slave/http.cpp, line 627
> > 
> >
> > Indent the block by 2 spaces, not 4.

Knowing we care a lot about this, I did actually put some care into making this 
consistent with the style guide.

Here and elsewhere: Our style guide states in the section *Function 
Definition/Invocation* that
> Newline when calling or defining a function: indent with four spaces.

Are you confusing this with the rule on continuations?,
> Newline for an assignment statement: indent with two spaces.

I believe we are not performing an assignment, but a function invocation here.

Note that further down in the style guide there are examples of `.then` 
inconsistent with the indention rules.


> On April 26, 2016, 6:51 p.m., Alexander Rukletsov wrote:
> > src/slave/http.cpp, line 635
> > 
> >
> > Indent the block by 2 spaces, not 4.

ditto.


> On April 26, 2016, 6:51 p.m., Alexander Rukletsov wrote:
> > src/slave/http.cpp, line 637
> > 
> >
> > Indent the block by 2 spaces, not 4.

ditto.


- Benjamin


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


On April 26, 2016, 8:22 p.m., Benjamin Bannier wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/46319/
> ---
> 
> (Updated April 26, 2016, 8:22 p.m.)
> 
> 
> Review request for mesos, Adam B, Alexander Rojas, and Jan Schlicht.
> 
> 
> Bugs: MESOS-5164
> https://issues.apache.org/jira/browse/MESOS-5164
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added authorization to agents' `/monitor/statistics` endpoints.
> 
> 
> Diffs
> -
> 
>   src/slave/http.cpp 537736d1fe42e8150bad91326299ef9a17041a8e 
>   src/slave/slave.hpp 57b18882e30e44dcc40449b0e3be8ee970c45bc8 
>   src/tests/slave_authorization_tests.cpp PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/46319/diff/
> 
> 
> Testing
> ---
> 
> make check (OS X, clang w/o optimization)
> 
> 
> Thanks,
> 
> Benjamin Bannier
> 
>

Re: Review Request 46203: Added authorization of the '/flags' endpoint.

[Jan Schlicht]

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




src/slave/http.cpp (lines 797 - 799)


Nobody caught that one yet.


- Jan Schlicht


On April 26, 2016, 5:27 p.m., Jan Schlicht wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/46203/
> ---
> 
> (Updated April 26, 2016, 5:27 p.m.)
> 
> 
> Review request for mesos, Adam B, Alexander Rojas, and Benjamin Bannier.
> 
> 
> Bugs: MESOS-5142
> https://issues.apache.org/jira/browse/MESOS-5142
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> See summary.
> 
> 
> Diffs
> -
> 
>   docs/configuration.md 318275fc5f935e6992ed4e8048cc4b42cc5d2cab 
>   include/mesos/authorizer/acls.proto 
> c50deeb5565dfd5b3e5e7210283d9a36a3bfd579 
>   include/mesos/authorizer/authorizer.proto 
> 40d93ea257d1df8d22eee8a21667db90d579a8fe 
>   src/Makefile.am e024c6d65608a55765e527a8668c415723dcfcca 
>   src/authorizer/local/authorizer.cpp 
> 0a3805fe4ce8eb89e096e8cd4326035513ba892b 
>   src/slave/flags.cpp a319d60c006d1104836c1c40f3617ceac9cb7b1e 
>   src/slave/http.cpp 537736d1fe42e8150bad91326299ef9a17041a8e 
>   src/slave/slave.hpp 57b18882e30e44dcc40449b0e3be8ee970c45bc8 
>   src/tests/slave_authorization_tests.cpp PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/46203/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Jan Schlicht
> 
>

Re: Review Request 46203: Added authorization of the '/flags' endpoint.

[Jan Schlicht]

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

(Updated April 26, 2016, 8:04 p.m.)


Review request for mesos, Adam B, Alexander Rojas, and Benjamin Bannier.


Changes
---

Uses `strings::tokenize` instead of `strings::split` and fixes a validation bug.


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


Repository: mesos


Description
---

See summary.


Diffs (updated)
-

  docs/configuration.md 318275fc5f935e6992ed4e8048cc4b42cc5d2cab 
  include/mesos/authorizer/acls.proto c50deeb5565dfd5b3e5e7210283d9a36a3bfd579 
  include/mesos/authorizer/authorizer.proto 
40d93ea257d1df8d22eee8a21667db90d579a8fe 
  src/Makefile.am e024c6d65608a55765e527a8668c415723dcfcca 
  src/authorizer/local/authorizer.cpp 0a3805fe4ce8eb89e096e8cd4326035513ba892b 
  src/slave/flags.cpp a319d60c006d1104836c1c40f3617ceac9cb7b1e 
  src/slave/http.cpp 537736d1fe42e8150bad91326299ef9a17041a8e 
  src/slave/slave.hpp 57b18882e30e44dcc40449b0e3be8ee970c45bc8 
  src/tests/slave_authorization_tests.cpp PRE-CREATION 

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


Testing
---

make check


Thanks,

Jan Schlicht

Re: Review Request 46203: Added authorization of the '/flags' endpoint.

[Alexander Rukletsov]

> On April 20, 2016, 8:35 a.m., Adam B wrote:
> > src/slave/http.cpp, lines 658-660
> > 
> >
> > Where did you come up with the magic number 3? What if we reorganize 
> > the operator endpoints in the (1.0) future? How will we know what the new 
> > value should be here?
> > What if the user setup a reverse proxy (like in dcos) and these 
> > requests are actually coming from a different base url than expected?
> 
> Benjamin Bannier wrote:
> @adam: The three here is needed so that this just strips the agent part 
> of the path, not everything up to the last `/`. An example endpoint would be 
> `/slave(1)/monitor/statistics`.
> 
> Jan Schlicht wrote:
> Seems like a hard problem to fully support both requirements. Maybe 
> reverting back to using `std::string` instead of `http::URL` as the function 
> parameter for `endpoint` could resolve this.
> 
> Benjamin Bannier wrote:
> Please use some typed entity that the usual endpoint handlers are aware 
> of. They currently have a `Request`, but e.g., have no idea how they are 
> being routed.
> 
> Jan Schlicht wrote:
> I'll go back to using the "magic number 3". At this point `URL::path` 
> will look like this: "/slave(n)/name/of/endpoint". By splitting into 3 
> components we get rid of the "/slave(n)/". The path is not the full URL that 
> has been requested, hence reverse proxies shouldn't be an issue here. I'll 
> add a comment, explaining this.
> 
> Adam B wrote:
> I see. And will this value be the same for the master's endpoints?
> Good to hear that reverse proxies won't be affected since it's not a full 
> URL.
> 
> Jan Schlicht wrote:
> This values won't work for the master's endpoint. In that case 
> `URL::path` will be "/name/of/endpoint" and we wouldn't need to split. 
> Because we're in `Slave::Http` we can expect that this code is only called 
> for agents.
> 
> Alexander Rojas wrote:
> So here is my issue wit this, you break it into three, and pass only the 
> second one to the authorizer, but that just sets a bad precedent. There are 
> endpoint that added with more components, e.g. `/api/v1/scheduler`. The right 
> way to solve this is to do something like:
> 
> ```c++
> // … code to handle when `url.path` is empty.
> 
> std::string path = url.path;
> std::size_t position = path.find('/', 1); 
> if (position != std::string::npos) {
>   path = path.substr(position);
> }
> 
> // Call the authorizer.
> ```
> 
> And we can add code to the authorizer module instead on how to handle 
> objects which encode paths (just like we dispatch to the endpoint handlers).
> 
> Alexander Rojas wrote:
> Forget what I said, I now understand what split with parameter does.

Once we `tokenize` instead of `split`, 3 will become 2, which will make it 
easier to understand where the magic number is coming from.


- Alexander


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


On April 26, 2016, 3:27 p.m., Jan Schlicht wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/46203/
> ---
> 
> (Updated April 26, 2016, 3:27 p.m.)
> 
> 
> Review request for mesos, Adam B, Alexander Rojas, and Benjamin Bannier.
> 
> 
> Bugs: MESOS-5142
> https://issues.apache.org/jira/browse/MESOS-5142
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> See summary.
> 
> 
> Diffs
> -
> 
>   docs/configuration.md 318275fc5f935e6992ed4e8048cc4b42cc5d2cab 
>   include/mesos/authorizer/acls.proto 
> c50deeb5565dfd5b3e5e7210283d9a36a3bfd579 
>   include/mesos/authorizer/authorizer.proto 
> 40d93ea257d1df8d22eee8a21667db90d579a8fe 
>   src/Makefile.am e024c6d65608a55765e527a8668c415723dcfcca 
>   src/authorizer/local/authorizer.cpp 
> 0a3805fe4ce8eb89e096e8cd4326035513ba892b 
>   src/slave/flags.cpp a319d60c006d1104836c1c40f3617ceac9cb7b1e 
>   src/slave/http.cpp 537736d1fe42e8150bad91326299ef9a17041a8e 
>   src/slave/slave.hpp 57b18882e30e44dcc40449b0e3be8ee970c45bc8 
>   src/tests/slave_authorization_tests.cpp PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/46203/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Jan Schlicht
> 
>

Re: Review Request 46096: Updated prepare() method of "network/cni" isolator for tests.

[Qian Zhang]

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

(Updated April 26, 2016, 11:53 p.m.)


Review request for mesos, Avinash sridharan and Jie Yu.


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


Repository: mesos


Description
---

Updated prepare() method of "network/cni" isolator for tests.


Diffs (updated)
-

  src/slave/containerizer/mesos/isolators/network/cni/cni.cpp 
ef0bc1092d761804ca6bd73016c0f3dd23719257 

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


Testing
---

make check


Thanks,

Qian Zhang

Re: Review Request 46097: Added the test "CniIsolatorTest.ROOT_LaunchCommandTask".

[Qian Zhang]

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

(Updated April 26, 2016, 11:53 p.m.)


Review request for mesos, Avinash sridharan and Jie Yu.


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


Repository: mesos


Description
---

Added the test "CniIsolatorTest.ROOT_LaunchCommandTask".


Diffs (updated)
-

  src/Makefile.am e024c6d65608a55765e527a8668c415723dcfcca 
  src/tests/containerizer/cni_isolator_tests.cpp PRE-CREATION 

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


Testing
---

[ RUN  ] CniIsolatorTest.ROOT_LaunchCommandTask
+ /home/stack/workspace/mesos/build/src/mesos-containerizer mount --help=false 
--operation=make-rslave --path=/
+ grep+  -E /tmp/CniIsolatorTest_ROOT_LaunchCommandTask_HRK4Dz/.+ 
/proc/self/mountinfo
+ grepcut -v -d  -f5
 d06b117d-518b-41e2-b8e0-62a12083773c
+ xargs --no-run-if-empty umount -l
+ mount -n --rbind 
/tmp/CniIsolatorTest_ROOT_LaunchCommandTask_HRK4Dz/provisioner/containers/d06b117d-518b-41e2-b8e0-62a12083773c/backends/copy/rootfses/7ea27011-cd3a-43b0-8301-b0b94d9f9b47
 
/tmp/CniIsolatorTest_ROOT_LaunchCommandTask_HRK4Dz/slaves/18dea042-5bb5-4336-8bc8-358ed1fbf6dd-S0/frameworks/18dea042-5bb5-4336-8bc8-358ed1fbf6dd-/executors/60e6d35d-6d33-47ae-9c23-d2e5c913c892/runs/d06b117d-518b-41e2-b8e0-62a12083773c/.rootfs
I0420 22:26:00.924844  9305 exec.cpp:150] Version: 0.29.0
I0420 22:26:00.942319  9375 exec.cpp:225] Executor registered on agent 
18dea042-5bb5-4336-8bc8-358ed1fbf6dd-S0
Registered executor on mesos
Starting task 60e6d35d-6d33-47ae-9c23-d2e5c913c892
Forked command at 9382
sh -c 'ls /'
bin  dev  etc  home lib  linuxrc  mediamnt  proc
 root run  sbin sys  tmp  usr  var
Command exited with status 0 (pid: 9382)
I0420 22:26:01.098331  9380 exec.cpp:399] Executor asked to shutdown
[   OK ] CniIsolatorTest.ROOT_LaunchCommandTask (42603 ms)


Thanks,

Qian Zhang

Re: Review Request 46203: Added authorization of the '/flags' endpoint.

[Jan Schlicht]

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

(Updated April 26, 2016, 5:27 p.m.)


Review request for mesos, Adam B, Alexander Rojas, and Benjamin Bannier.


Changes
---

Addressed Benjamin's issues (made `authorizeEndpoint` use a full `Request` as 
parameter and extracting `url` and `method` from that)


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


Repository: mesos


Description
---

See summary.


Diffs (updated)
-

  docs/configuration.md 318275fc5f935e6992ed4e8048cc4b42cc5d2cab 
  include/mesos/authorizer/acls.proto c50deeb5565dfd5b3e5e7210283d9a36a3bfd579 
  include/mesos/authorizer/authorizer.proto 
40d93ea257d1df8d22eee8a21667db90d579a8fe 
  src/Makefile.am e024c6d65608a55765e527a8668c415723dcfcca 
  src/authorizer/local/authorizer.cpp 0a3805fe4ce8eb89e096e8cd4326035513ba892b 
  src/slave/flags.cpp a319d60c006d1104836c1c40f3617ceac9cb7b1e 
  src/slave/http.cpp 537736d1fe42e8150bad91326299ef9a17041a8e 
  src/slave/slave.hpp 57b18882e30e44dcc40449b0e3be8ee970c45bc8 
  src/tests/slave_authorization_tests.cpp PRE-CREATION 

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


Testing
---

make check


Thanks,

Jan Schlicht

Re: Review Request 46203: Added authorization of the '/flags' endpoint.

[Alexander Rukletsov]

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




docs/configuration.md (line 899)


Let's entertain our reader a bit : ). Instead of "a", we can use something 
like "padavan", and if you plan to introduce say `post_endpoints`, you can 
reuse "yoda".



src/slave/http.cpp (lines 796 - 799)


Looks like `strings::tokenize()` suits better here.



src/slave/slave.hpp (lines 471 - 473)


It feels like this class should be somewhere in a more general place. 
Moreover, libprocess' `Request` struct has the `method` field, which is a 
string. Any reason why you introduce an enum here?


- Alexander Rukletsov


On April 26, 2016, 3:27 p.m., Jan Schlicht wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/46203/
> ---
> 
> (Updated April 26, 2016, 3:27 p.m.)
> 
> 
> Review request for mesos, Adam B, Alexander Rojas, and Benjamin Bannier.
> 
> 
> Bugs: MESOS-5142
> https://issues.apache.org/jira/browse/MESOS-5142
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> See summary.
> 
> 
> Diffs
> -
> 
>   docs/configuration.md 318275fc5f935e6992ed4e8048cc4b42cc5d2cab 
>   include/mesos/authorizer/acls.proto 
> c50deeb5565dfd5b3e5e7210283d9a36a3bfd579 
>   include/mesos/authorizer/authorizer.proto 
> 40d93ea257d1df8d22eee8a21667db90d579a8fe 
>   src/Makefile.am e024c6d65608a55765e527a8668c415723dcfcca 
>   src/authorizer/local/authorizer.cpp 
> 0a3805fe4ce8eb89e096e8cd4326035513ba892b 
>   src/slave/flags.cpp a319d60c006d1104836c1c40f3617ceac9cb7b1e 
>   src/slave/http.cpp 537736d1fe42e8150bad91326299ef9a17041a8e 
>   src/slave/slave.hpp 57b18882e30e44dcc40449b0e3be8ee970c45bc8 
>   src/tests/slave_authorization_tests.cpp PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/46203/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Jan Schlicht
> 
>

Re: Review Request 46203: Added authorization of the '/flags' endpoint.

[Alexander Rukletsov]

> On April 26, 2016, 2:01 p.m., Benjamin Bannier wrote:
> > src/slave/http.cpp, line 787
> > 
> >
> > I really like that we use a dedicated `enum` in the switch below, but 
> > dislike how users of this function need to manually translate from a 
> > `std::string` to a `Method`.
> > 
> > What about passing some `std::string` like stored inside the `Request` 
> > here (feel free to add a `TODO` to `Request` to strongly type `method`), 
> > and then doing the conversion in here? An unknown method would result in 
> > e.g., a `Failure`.
> 
> Jan Schlicht wrote:
> +1 for making `Request::method` strongly typed. To implement it right now 
> I would remove the `Method` enum and compare the `Request::method` string 
> directly with string of supported methods. I'd rather `switch` over the 
> values of an enum, but the code will stay simple and readable by the 
> string-string comparison.

Yep, let's do it in generally.


- Alexander


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


On April 26, 2016, 3:27 p.m., Jan Schlicht wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/46203/
> ---
> 
> (Updated April 26, 2016, 3:27 p.m.)
> 
> 
> Review request for mesos, Adam B, Alexander Rojas, and Benjamin Bannier.
> 
> 
> Bugs: MESOS-5142
> https://issues.apache.org/jira/browse/MESOS-5142
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> See summary.
> 
> 
> Diffs
> -
> 
>   docs/configuration.md 318275fc5f935e6992ed4e8048cc4b42cc5d2cab 
>   include/mesos/authorizer/acls.proto 
> c50deeb5565dfd5b3e5e7210283d9a36a3bfd579 
>   include/mesos/authorizer/authorizer.proto 
> 40d93ea257d1df8d22eee8a21667db90d579a8fe 
>   src/Makefile.am e024c6d65608a55765e527a8668c415723dcfcca 
>   src/authorizer/local/authorizer.cpp 
> 0a3805fe4ce8eb89e096e8cd4326035513ba892b 
>   src/slave/flags.cpp a319d60c006d1104836c1c40f3617ceac9cb7b1e 
>   src/slave/http.cpp 537736d1fe42e8150bad91326299ef9a17041a8e 
>   src/slave/slave.hpp 57b18882e30e44dcc40449b0e3be8ee970c45bc8 
>   src/tests/slave_authorization_tests.cpp PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/46203/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Jan Schlicht
> 
>

Re: Review Request 46097: Added the test "CniIsolatorTest.ROOT_LaunchCommandTask".

[Qian Zhang]

> On April 22, 2016, 5:19 a.m., Avinash sridharan wrote:
> > src/tests/containerizer/cni_isolator_tests.cpp, line 24
> > 
> >
> > I think `mesos` definitions should come after `process`.

No, I think we should do it in alphabet order, 'm' should come before 'p'. You 
can check command_executor_tests.cpp for an example.


> On April 22, 2016, 5:19 a.m., Avinash sridharan wrote:
> > src/tests/containerizer/cni_isolator_tests.cpp, line 31
> > 
> >
> > Move the `std` definitions above process.

We should do it in alphabet order.


> On April 22, 2016, 5:19 a.m., Avinash sridharan wrote:
> > src/tests/containerizer/cni_isolator_tests.cpp, line 38
> > 
> >
> > We should have an #ifdef for the whole file? This looks a bit odd. I 
> > would suggest put this conditional in the Makefile, so that this doesn't 
> > get compiled for non-linux platforms.

You can check runtime_isolator_tests.cpp which does the same.


> On April 22, 2016, 5:19 a.m., Avinash sridharan wrote:
> > src/tests/containerizer/cni_isolator_tests.cpp, line 53
> > 
> >
> > Do we need to explicitly specify this registry? I thought this would be 
> > the default?

Yeah, I agree!


> On April 22, 2016, 5:19 a.m., Avinash sridharan wrote:
> > src/tests/containerizer/cni_isolator_tests.cpp, line 65
> > 
> >
> > Do you want to make this into a helper function? I am guessing this 
> > will be used in other test cases too ?

Agree!


> On April 22, 2016, 5:19 a.m., Avinash sridharan wrote:
> > src/tests/containerizer/cni_isolator_tests.cpp, line 82
> > 
> >
> > Might Want to convert this into a helper function as well.

Agree!


> On April 22, 2016, 5:19 a.m., Avinash sridharan wrote:
> > src/tests/containerizer/cni_isolator_tests.cpp, line 74
> > 
> >
> > Should you be reading the /etc/resolv.conf and returning the DNS info 
> > as well. Else, the other test cases would never ever create a DNS file in 
> > the ?

Agree!


- Qian


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


On April 20, 2016, 10:26 p.m., Qian Zhang wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/46097/
> ---
> 
> (Updated April 20, 2016, 10:26 p.m.)
> 
> 
> Review request for mesos, Avinash sridharan and Jie Yu.
> 
> 
> Bugs: MESOS-5167
> https://issues.apache.org/jira/browse/MESOS-5167
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added the test "CniIsolatorTest.ROOT_LaunchCommandTask".
> 
> 
> Diffs
> -
> 
>   src/Makefile.am 5a2b4efa781752863d6751f98614fb78bece73ac 
>   src/tests/containerizer/cni_isolator_tests.cpp PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/46097/diff/
> 
> 
> Testing
> ---
> 
> [ RUN  ] CniIsolatorTest.ROOT_LaunchCommandTask
> + /home/stack/workspace/mesos/build/src/mesos-containerizer mount 
> --help=false --operation=make-rslave --path=/
> + grep+  -E /tmp/CniIsolatorTest_ROOT_LaunchCommandTask_HRK4Dz/.+ 
> /proc/self/mountinfo
> + grepcut -v -d  -f5
>  d06b117d-518b-41e2-b8e0-62a12083773c
> + xargs --no-run-if-empty umount -l
> + mount -n --rbind 
> /tmp/CniIsolatorTest_ROOT_LaunchCommandTask_HRK4Dz/provisioner/containers/d06b117d-518b-41e2-b8e0-62a12083773c/backends/copy/rootfses/7ea27011-cd3a-43b0-8301-b0b94d9f9b47
>  
> /tmp/CniIsolatorTest_ROOT_LaunchCommandTask_HRK4Dz/slaves/18dea042-5bb5-4336-8bc8-358ed1fbf6dd-S0/frameworks/18dea042-5bb5-4336-8bc8-358ed1fbf6dd-/executors/60e6d35d-6d33-47ae-9c23-d2e5c913c892/runs/d06b117d-518b-41e2-b8e0-62a12083773c/.rootfs
> I0420 22:26:00.924844  9305 exec.cpp:150] Version: 0.29.0
> I0420 22:26:00.942319  9375 exec.cpp:225] Executor registered on agent 
> 18dea042-5bb5-4336-8bc8-358ed1fbf6dd-S0
> Registered executor on mesos
> Starting task 60e6d35d-6d33-47ae-9c23-d2e5c913c892
> Forked command at 9382
> sh -c 'ls /'
> bin  dev  etc  home lib  linuxrc  mediamnt  proc  
>root run  sbin sys  tmp  usr  var
> Command exited with status 0 (pid: 9382)
> I0420 22:26:01.098331  9380 exec.cpp:399] Executor asked to shutdown
> [   OK ] CniIsolatorTest.ROOT_LaunchCommandTask (42603 ms)
> 
> 
> Thanks,
> 
> Qian Zhang
> 
>

Review Request 46676: Slave/Agent Rename Phase I: Rename '/include/mesos/slave' folder.

[zhou xing]

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

Review request for mesos, Kevin Klues and Vinod Kone.


Bugs: mesos-5230
https://issues.apache.org/jira/browse/mesos-5230


Repository: mesos


Description
---

[#MESOS-5230]
Change the 'make install' includedir name from slave to agent.


Diffs
-

  CHANGELOG 57f5457640c9b7ca2cb7707f2962dd0421f090f7 
  src/Makefile.am e024c6d65608a55765e527a8668c415723dcfcca 

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


Testing
---

cd mesos
./bootstrap
mkdir build
cd build
../configure --prefix=${HOME}/install/mesos
make
make check
make install


Thanks,

zhou xing

Re: Review Request 46319: Added authorization to agents' `/monitor/statistics` endpoints.

[Adam B]

> On April 22, 2016, 1:22 a.m., Adam B wrote:
> > src/tests/slave_tests.cpp, line 1895
> > 
> >
> > Should we create a TYPED_TEST that tests this ACL in the local 
> > authorizer (direct and as a module), or do we only need these tests for one 
> > (of the many) ACCESS_ENDPOINT_WITH_PATH endpoints?
> 
> Benjamin Bannier wrote:
> I would prefer if we'd separate whether some authorizer works and whether 
> an endpoint correctly queries the authorizer. You already suggested testing 
> the former to Jan [here](https://reviews.apache.org/r/46203/#comment193252); 
> how about I migrate the current test to a *parameterized test* (on the 
> endpoint) so we have a single infrastructure to check authorization for all 
> `GET` requests against agent endpoints? Once we start tackling e.g., `POST` 
> requests we could extend this harness.
> 
> I made this test parameterized on the agent endpoint in 
> https://reviews.apache.org/r/46569/; if we'd want to go that direction we 
> could squash it into this patch.

Sounds great. Feel free to squash r/46569 into this patch or leave it separate.


- Adam


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


On April 25, 2016, 7:16 a.m., Benjamin Bannier wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/46319/
> ---
> 
> (Updated April 25, 2016, 7:16 a.m.)
> 
> 
> Review request for mesos, Adam B, Alexander Rojas, and Jan Schlicht.
> 
> 
> Bugs: MESOS-5164
> https://issues.apache.org/jira/browse/MESOS-5164
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added authorization to agents' `/monitor/statistics` endpoints.
> 
> 
> Diffs
> -
> 
>   src/slave/http.cpp 537736d1fe42e8150bad91326299ef9a17041a8e 
>   src/slave/slave.hpp 20a4bcd0bb9dad06ea81fc4ad9b2fa462c69d2c5 
>   src/tests/slave_authorization_tests.cpp PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/46319/diff/
> 
> 
> Testing
> ---
> 
> make check (OS X, clang w/o optimization)
> 
> 
> Thanks,
> 
> Benjamin Bannier
> 
>

Re: Review Request 46319: Added authorization to agents' `/monitor/statistics` endpoints.

[Adam B]

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


Fix it, then Ship it!




Only minor nits left beyond my two remaining issues. Fix these and rebase once 
the previous patch in the chain lands, then we can commit this too.


src/slave/http.cpp (lines 630 - 638)


Seems like these lines need to be indented more.



src/slave/http.cpp (line 641)


Is this always an authoriaztion error/warning? Couldn't this also be a 
failure from limiter->acquire() or Slave::usage?


- Adam B


On April 25, 2016, 7:16 a.m., Benjamin Bannier wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/46319/
> ---
> 
> (Updated April 25, 2016, 7:16 a.m.)
> 
> 
> Review request for mesos, Adam B, Alexander Rojas, and Jan Schlicht.
> 
> 
> Bugs: MESOS-5164
> https://issues.apache.org/jira/browse/MESOS-5164
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added authorization to agents' `/monitor/statistics` endpoints.
> 
> 
> Diffs
> -
> 
>   src/slave/http.cpp 537736d1fe42e8150bad91326299ef9a17041a8e 
>   src/slave/slave.hpp 20a4bcd0bb9dad06ea81fc4ad9b2fa462c69d2c5 
>   src/tests/slave_authorization_tests.cpp PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/46319/diff/
> 
> 
> Testing
> ---
> 
> make check (OS X, clang w/o optimization)
> 
> 
> Thanks,
> 
> Benjamin Bannier
> 
>

Re: Review Request 46203: Added authorization of the '/flags' endpoint.

[Adam B]

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



Looks great! I think we just need to pass the GET/POST verb into 
`authorizeEndpoint()` and fix the other minor nits, then we'll be ready to 
ship. Or maybe you can convince me that we don't need to add the verb until we 
actually have to authorize a non-GET verb (e.g. for maintenance primitives).


docs/configuration.md (line 900)


How about you give some real example endpoints, like "/flags" and 
"/monitor/statistics" (the latter shows that longer URLs are allowed)?
Same in flags.cpp



include/mesos/authorizer/acls.proto (line 150)


s/access/GET HTTP/



include/mesos/authorizer/acls.proto (line 152)


Not necessarily an operator.
s/Operator/HTTP/?



src/slave/http.cpp (line 360)


Should this perhaps be a `Shared<>`?



src/slave/http.cpp (line 362)


This function still assumes GET. Please pass a something like a Verb enum 
as a parameter, or else you'll need an `authorizeGetEndpoint()`, 
`authorizePostEndpoint()`, etc.



src/slave/http.cpp (line 365)


Why pass the entire Slave down when you only use the flags?



src/slave/http.cpp (lines 797 - 799)


For my comfort, can you also validate that `pathComponents[0] == ""` and 
`pathComponents[1].startsWith("slave(")` so that it's clearer how this string 
is being split?
Then we'll fail fast if the format changes, rather than passing incorrect 
substrings to the authorizer.
Then we can drop the other issue about the magic number '3', since it's 
more clearly documented/explained.



src/slave/http.cpp (line 804)


s/access/GET/ and shouldn't you be checking the Verb here, for when we have 
to authorize things other than GETs?



src/tests/slave_authorization_tests.cpp (line 61)


`s/Parameter *parameter/Parameter* parameter/`



src/tests/slave_authorization_tests.cpp (lines 73 - 75)


I'd rather you wrap the first line at `<` so LocalAuthorizer and 
tests::Module start at the same indentation as AuthorizerTypes.
I know the other AuthorizerTypes and AllocatorTypes follow the same pattern 
you have here, but they look ugly/jagged too.
I prefer the look of HttpAuthenticatorTypes in http_authentication_tests.cpp



src/tests/slave_authorization_tests.cpp (lines 90 - 92)


s/acl1/acl/g



src/tests/slave_authorization_tests.cpp (lines 100 - 102)


If you wrap after the `=`, you can fit the entire rhs on one line.



src/tests/slave_authorization_tests.cpp (line 144)


After reading the description of the test, I expected to see ACLs that set 
permissive=false, but adds a rule for GetEndpoint(ANY, "/flags")
What you're testing is fully permissive ACLs, which is a bit different, and 
probably tested throughout the rest of the existing tests.


- Adam B


On April 25, 2016, 5:50 a.m., Jan Schlicht wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/46203/
> ---
> 
> (Updated April 25, 2016, 5:50 a.m.)
> 
> 
> Review request for mesos, Adam B, Alexander Rojas, and Benjamin Bannier.
> 
> 
> Bugs: MESOS-5142
> https://issues.apache.org/jira/browse/MESOS-5142
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> See summary.
> 
> 
> Diffs
> -
> 
>   docs/configuration.md 86ba66ac62295ca148524bcb2e57fee560ac4ac5 
>   include/mesos/authorizer/acls.proto 
> c50deeb5565dfd5b3e5e7210283d9a36a3bfd579 
>   include/mesos/authorizer/authorizer.proto 
> 40d93ea257d1df8d22eee8a21667db90d579a8fe 
>   src/Makefile.am e024c6d65608a55765e527a8668c415723dcfcca 
>   src/authorizer/local/authorizer.cpp 
> 0a3805fe4ce8eb89e096e8cd4326035513ba892b 
>   src/slave/flags.cpp 10d2974bd2b6e79255fc894979607f0d2d00c315 
>   src/slave/http.cpp 537736d1fe42e8150bad91326299ef9a17041a8e 
>   src/slave/slave.hpp 20a4bcd0bb9dad06ea81fc4ad9b2fa462c69d2c5 
>   src/tests/slave_authorization_tests.cpp PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/46203/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Jan Schlicht
> 
>

Re: Review Request 46319: Added authorization to agents' `/monitor/statistics` endpoints.

[Adam B]

> On April 22, 2016, 1:22 a.m., Adam B wrote:
> > src/slave/slave.hpp, lines 471-472
> > 
> >
> > How did this come up? The original `_statistics()` is not static, and 
> > it had no issues.
> 
> Benjamin Bannier wrote:
> The original `statistics` did not make any use of member data in any of 
> the `defer`ed actions (all the used data came from some 
> `Future` directly passed into the callback; the call of 
> `slave->self()` was not executed in the `defer`ed action). Now here we need 
> to access a rate limiter which is a member of `Http` which in turn might go 
> away before `_statistics` is executed.

And now that `_statistics()` is synchronous and no longer uses the limiter, do 
we still need to worry about this?


- Adam


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


On April 25, 2016, 7:16 a.m., Benjamin Bannier wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/46319/
> ---
> 
> (Updated April 25, 2016, 7:16 a.m.)
> 
> 
> Review request for mesos, Adam B, Alexander Rojas, and Jan Schlicht.
> 
> 
> Bugs: MESOS-5164
> https://issues.apache.org/jira/browse/MESOS-5164
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added authorization to agents' `/monitor/statistics` endpoints.
> 
> 
> Diffs
> -
> 
>   src/slave/http.cpp 537736d1fe42e8150bad91326299ef9a17041a8e 
>   src/slave/slave.hpp 20a4bcd0bb9dad06ea81fc4ad9b2fa462c69d2c5 
>   src/tests/slave_authorization_tests.cpp PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/46319/diff/
> 
> 
> Testing
> ---
> 
> make check (OS X, clang w/o optimization)
> 
> 
> Thanks,
> 
> Benjamin Bannier
> 
>

Re: Review Request 46569: Parameterized agent endpoint authorization tests on tested endpoint.

[Mesos ReviewBot]

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



Patch looks great!

Reviews applied: [45922, 46318, 46203, 46319, 46569]

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 26, 2016, 6:22 p.m., Benjamin Bannier wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/46569/
> ---
> 
> (Updated April 26, 2016, 6:22 p.m.)
> 
> 
> Review request for mesos and Adam B.
> 
> 
> Bugs: MESOS-5164
> https://issues.apache.org/jira/browse/MESOS-5164
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> At the moment we only provide a harness for `GET` methods, but will
> generalize this further once we authorize `POST` requests against
> agent endpoints as well.
> 
> 
> Diffs
> -
> 
>   src/tests/slave_authorization_tests.cpp PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/46569/diff/
> 
> 
> Testing
> ---
> 
> make check (OS X, clang trunk w/o optimization)
> 
> 
> Thanks,
> 
> Benjamin Bannier
> 
>

Re: Review Request 46676: Slave/Agent Rename Phase I: Rename '/include/mesos/slave' folder.

[Vinod Kone]

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



Can you make the description more descriptive please? Make it clear what the 
change is here. Also, no need for including bug-id in the description. It is 
already attached to the review.


CHANGELOG (line 76)


"Slave/Agent" is correct. The earlier ones called it "Replace Master/Slave 
Terminology". Can you fix the earlier ones? You need to fix the name of the 
actual tickets in the JIRA and also this changelog. Please send a separate 
review for the CHANGELOG.



src/Makefile.am (line 535)


what does setting this variable do?



src/Makefile.am (line 2144)


Update the comment.



src/Makefile.am (lines 2151 - 2152)


IIUC, when someone does a  `make install` there will be 2 directories in 
the installation directory.

./mesos/agent
./mesos/slave  (this is a symlink to ../agent)

In the git repo itself we will still have `include/mesos/slave` directory? 
I guess renaming this directory to `incluee/mesos/agent`? is too much work?


- Vinod Kone


On April 26, 2016, 6:06 a.m., zhou xing wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/46676/
> ---
> 
> (Updated April 26, 2016, 6:06 a.m.)
> 
> 
> Review request for mesos, Kevin Klues and Vinod Kone.
> 
> 
> Bugs: mesos-5230
> https://issues.apache.org/jira/browse/mesos-5230
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> [#MESOS-5230]
> Change the 'make install' includedir name from slave to agent.
> 
> 
> Diffs
> -
> 
>   CHANGELOG 57f5457640c9b7ca2cb7707f2962dd0421f090f7 
>   src/Makefile.am e024c6d65608a55765e527a8668c415723dcfcca 
> 
> Diff: https://reviews.apache.org/r/46676/diff/
> 
> 
> Testing
> ---
> 
> cd mesos
> ./bootstrap
> mkdir build
> cd build
> ../configure --prefix=${HOME}/install/mesos
> make
> make check
> make install
> 
> 
> Thanks,
> 
> zhou xing
> 
>

Re: Review Request 46610: Fix 'pivot_root is not available' error on ARM.

[Ben Mahler]

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


Ship it!




Since this followed the existing approach (used by PowerPC) I've gone ahead and 
committed it.

The cleanup mentioned by haosdent sounds great if it removes unnecessary #ifdef 
logic. Let me know when the cleanup is available!

- Ben Mahler


On April 24, 2016, 11:36 a.m., Tomasz Janiszewski wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/46610/
> ---
> 
> (Updated April 24, 2016, 11:36 a.m.)
> 
> 
> Review request for mesos and haosdent huang.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Fix 'pivot_root is not available' error on ARM.
> 
> 
> Diffs
> -
> 
>   src/linux/fs.cpp 2087b4ac1503e0fd085319b1017389f1f947536f 
> 
> Diff: https://reviews.apache.org/r/46610/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Tomasz Janiszewski
> 
>

Re: Review Request 46373: Request /files/read.json with a negative length value causes error.

[zhou xing]

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

(Updated 四月 26, 2016, 8:15 a.m.)


Review request for mesos, Ben Mahler and Greg Mann.


Changes
---

rebase and update code according to Greg's comments


Bugs: mesos-5060
https://issues.apache.org/jira/browse/mesos-5060


Repository: mesos


Description
---

[MESOS-5060]
The patch did the following changes:
1. Fix the length logic in files.cpp.
2. Add some tests to test the /files/read.json endponit with
negative length.


Diffs (updated)
-

  src/files/files.cpp 4e916101b378b0e9032a08a3f6c73e195b2a08a1 
  src/tests/files_tests.cpp 5d6620d13babaf0bb7f9c888bb1b4fa2228b6ccd 

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


Testing
---

make
make check

request 'files/read.json' endpoint with negative offset or length argument


Thanks,

zhou xing

Review Request 46682: Made volume isolator logic injectable for MockDriverClient.

[Gilbert Song]

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

Review request for mesos, Guangya Liu, Artem Harutyunyan, Jie Yu, and Timothy 
Chen.


Repository: mesos


Description
---

Made volume isolator logic injectable for MockDriverClient.


Diffs
-

  src/slave/containerizer/mesos/isolators/docker/volume/isolator.hpp 
5402973c04ea2045c9ec7b53b58795bf22c9a631 
  src/slave/containerizer/mesos/isolators/docker/volume/isolator.cpp 
4675c767a562a3ca7f99265c9198a37786933c2d 

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


Testing
---

make check


Thanks,

Gilbert Song

Review Request 46680: Fixed docker volume isolator const ref and vlog quotation.

[Gilbert Song]

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

Review request for mesos, Guangya Liu, Artem Harutyunyan, Jie Yu, and Timothy 
Chen.


Repository: mesos


Description
---

Fixed docker volume isolator const ref and vlog quotation.


Diffs
-

  src/slave/containerizer/mesos/isolators/docker/volume/isolator.cpp 
4675c767a562a3ca7f99265c9198a37786933c2d 

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


Testing
---

make check


Thanks,

Gilbert Song

Review Request 46681: Fixed docker volume isolator checkpointing empty info.

[Gilbert Song]

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

Review request for mesos, Guangya Liu, Artem Harutyunyan, Jie Yu, and Timothy 
Chen.


Repository: mesos


Description
---

Fixed docker volume isolator checkpointing empty info.


Diffs
-

  src/slave/containerizer/mesos/isolators/docker/volume/isolator.cpp 
4675c767a562a3ca7f99265c9198a37786933c2d 

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


Testing
---

make check


Thanks,

Gilbert Song

Re: Review Request 46569: Parameterized agent endpoint authorization tests on tested endpoint.

[Mesos ReviewBot]

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



Patch looks great!

Reviews applied: [45922, 46318, 46203, 46319, 46569]

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 26, 2016, 7:59 a.m., Benjamin Bannier wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/46569/
> ---
> 
> (Updated April 26, 2016, 7:59 a.m.)
> 
> 
> Review request for mesos and Adam B.
> 
> 
> Bugs: MESOS-5164
> https://issues.apache.org/jira/browse/MESOS-5164
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> At the moment we only provide a harness for `GET` methods, but will
> generalize this further once we authorize `POST` requests against
> agent endpoints as well.
> 
> 
> Diffs
> -
> 
>   src/tests/slave_authorization_tests.cpp PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/46569/diff/
> 
> 
> Testing
> ---
> 
> make check (OS X, clang trunk w/o optimization)
> 
> 
> Thanks,
> 
> Benjamin Bannier
> 
>

Re: Review Request 46569: Parameterized agent endpoint authorization tests on tested endpoint.

[Adam B]

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




src/tests/slave_authorization_tests.cpp (line 171)


s/some agent's endpoint/the specified agent endpoint/



src/tests/slave_authorization_tests.cpp (line 246)


What does this parameter say/do?


- Adam B


On April 25, 2016, 7:16 a.m., Benjamin Bannier wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/46569/
> ---
> 
> (Updated April 25, 2016, 7:16 a.m.)
> 
> 
> Review request for mesos and Adam B.
> 
> 
> Bugs: MESOS-5164
> https://issues.apache.org/jira/browse/MESOS-5164
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> At the moment we only provide a harness for `GET` methods, but will
> generalize this further once we authorize `POST` requests against
> agent endpoints as well.
> 
> 
> Diffs
> -
> 
>   src/tests/slave_authorization_tests.cpp PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/46569/diff/
> 
> 
> Testing
> ---
> 
> make check (OS X, clang trunk w/o optimization)
> 
> 
> Thanks,
> 
> Benjamin Bannier
> 
>

Re: Review Request 45668: Enable CMake build for Linux as a BUILDTOOL option.

[Juan Larriba]

> On Abr. 18, 2016, 6:18 p.m., Vinod Kone wrote:
> > support/docker_build.sh, lines 127-140
> > 
> >
> > Hmm. I was hoping for something more generic than hard coding these 3 
> > configurations. But if it's not easy to auto translate and the 
> > configuration options for CMAKE are truly different from Autotools, I'm ok 
> > with not doing this change here and instead have 2 different Jenkins jobs 
> > (one for Mesos w/ Auto-tools and one for Mesos w/ CMake); was just hoping 
> > we could get away with having just one Jenkins job (parameterized).
> 
> Juan Larriba wrote:
> I will try to submit a more generic proposal to convert from autotools to 
> cmake parameters so the same Jenkins job can be used for building, I have had 
> a lot of work this past week and couldn't do anything. However, I still do 
> not understand why the BUILDTOOL or the COMPILER parameters can change 
> between job executions and the CONFIGURATION parameter cannot change at the 
> same time.
> 
> Vinod Kone wrote:
> The ASF CI job is setup as a paremeterized job 
> (https://builds.apache.org/view/M-R/view/Mesos/job/Mesos/). The parameters 
> are the environment varialbes (BUILDTOOL, COMPILER, CONFIGURATION etc). So 
> all the combinations of the environmental varialbles should work.
> 
> For example if we have the following envs,
> 
> BUILDTOOL = "'autotools' 'cmake'"
> COMPILER = "'gcc' 'clang'"
> CONFIGURATION = "'--verbose' '--enable_libevent'"
> 
> Then all the combinations of these should work.
> 
> autotools x gcc x --verbose
> autotools x gcc x --enable_libevent
> autotools x clang x --verbose
> autotools x clang x --enable_libevent
> cmake x gcc x --verbose
> cmake x gcc x --enable_libevent
> cmake x clang x --verbose
> cmake x clang x --enable_libevent
> 
> Hope this makes sense?

Thank you very much Vinod, now it makes sense

The fact is that I do not have permissions to see the internals of the job (the 
link you pasted), so I couldn't figure out how the build was being executed. I 
will submit the modifications needed to convert from autotools CONFIGURATION to 
cmake CONFIGURATION.


- Juan


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


On Abr. 18, 2016, 1:58 p.m., Juan Larriba wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/45668/
> ---
> 
> (Updated Abr. 18, 2016, 1:58 p.m.)
> 
> 
> Review request for mesos, Alex Clemmer, Joerg Schad, and Vinod Kone.
> 
> 
> Bugs: MESOS-5101
> https://issues.apache.org/jira/browse/MESOS-5101
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Enable CMake build for Linux as a BUILDTOOL option.
> 
> 
> Diffs
> -
> 
>   support/docker_build.sh c5917bcce4cf1f98a1808ceabe340648edd7d2a9 
> 
> Diff: https://reviews.apache.org/r/45668/diff/
> 
> 
> Testing
> ---
> 
> Built using docker_build.sh on both centos:7 and ubuntu:14.04 using both 
> cmake and autotools. In ubuntu:14.04 was built using gcc and clang, in 
> centos:7 only gcc.
> 
> 
> Thanks,
> 
> Juan Larriba
> 
>

Re: Review Request 46319: Added authorization to agents' `/monitor/statistics` endpoints.

[Benjamin Bannier]

> On April 26, 2016, 8:53 a.m., Adam B wrote:
> > src/slave/http.cpp, lines 630-638
> > 
> >
> > Seems like these lines need to be indented more.

Indeed they need to be and now are.


> On April 26, 2016, 8:53 a.m., Adam B wrote:
> > src/slave/http.cpp, line 641
> > 
> >
> > Is this always an authoriaztion error/warning? Couldn't this also be a 
> > failure from limiter->acquire() or Slave::usage?

Good catch. Now we emit `"Could not collect statistics: "` 


- Benjamin


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


On April 26, 2016, 9:59 a.m., Benjamin Bannier wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/46319/
> ---
> 
> (Updated April 26, 2016, 9:59 a.m.)
> 
> 
> Review request for mesos, Adam B, Alexander Rojas, and Jan Schlicht.
> 
> 
> Bugs: MESOS-5164
> https://issues.apache.org/jira/browse/MESOS-5164
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added authorization to agents' `/monitor/statistics` endpoints.
> 
> 
> Diffs
> -
> 
>   src/slave/http.cpp 537736d1fe42e8150bad91326299ef9a17041a8e 
>   src/slave/slave.hpp 57b18882e30e44dcc40449b0e3be8ee970c45bc8 
>   src/tests/slave_authorization_tests.cpp PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/46319/diff/
> 
> 
> Testing
> ---
> 
> make check (OS X, clang w/o optimization)
> 
> 
> Thanks,
> 
> Benjamin Bannier
> 
>

Re: Review Request 46569: Parameterized agent endpoint authorization tests on tested endpoint.

[Benjamin Bannier]

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

(Updated April 26, 2016, 9:59 a.m.)


Review request for mesos and Adam B.


Changes
---

Translate comment into English.


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


Repository: mesos


Description
---

At the moment we only provide a harness for `GET` methods, but will
generalize this further once we authorize `POST` requests against
agent endpoints as well.


Diffs (updated)
-

  src/tests/slave_authorization_tests.cpp PRE-CREATION 

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


Testing
---

make check (OS X, clang trunk w/o optimization)


Thanks,

Benjamin Bannier

Re: Review Request 46319: Added authorization to agents' `/monitor/statistics` endpoints.

[Benjamin Bannier]

> On April 22, 2016, 10:22 a.m., Adam B wrote:
> > src/slave/slave.hpp, lines 471-472
> > 
> >
> > How did this come up? The original `_statistics()` is not static, and 
> > it had no issues.
> 
> Benjamin Bannier wrote:
> The original `statistics` did not make any use of member data in any of 
> the `defer`ed actions (all the used data came from some 
> `Future` directly passed into the callback; the call of 
> `slave->self()` was not executed in the `defer`ed action). Now here we need 
> to access a rate limiter which is a member of `Http` which in turn might go 
> away before `_statistics` is executed.
> 
> Adam B wrote:
> And now that `_statistics()` is synchronous and no longer uses the 
> limiter, do we still need to worry about this?

Even if we do not use any member data, it is still undefined behavior to invoke 
a non-static member function on a destroyed object.


- Benjamin


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


On April 26, 2016, 9:59 a.m., Benjamin Bannier wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/46319/
> ---
> 
> (Updated April 26, 2016, 9:59 a.m.)
> 
> 
> Review request for mesos, Adam B, Alexander Rojas, and Jan Schlicht.
> 
> 
> Bugs: MESOS-5164
> https://issues.apache.org/jira/browse/MESOS-5164
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added authorization to agents' `/monitor/statistics` endpoints.
> 
> 
> Diffs
> -
> 
>   src/slave/http.cpp 537736d1fe42e8150bad91326299ef9a17041a8e 
>   src/slave/slave.hpp 57b18882e30e44dcc40449b0e3be8ee970c45bc8 
>   src/tests/slave_authorization_tests.cpp PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/46319/diff/
> 
> 
> Testing
> ---
> 
> make check (OS X, clang w/o optimization)
> 
> 
> Thanks,
> 
> Benjamin Bannier
> 
>

Re: Review Request 46676: Slave/Agent Rename Phase I: Rename '/include/mesos/slave' folder.

[Mesos ReviewBot]

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



Patch looks great!

Reviews applied: [46676]

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 26, 2016, 6:06 a.m., zhou xing wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/46676/
> ---
> 
> (Updated April 26, 2016, 6:06 a.m.)
> 
> 
> Review request for mesos, Kevin Klues and Vinod Kone.
> 
> 
> Bugs: mesos-5230
> https://issues.apache.org/jira/browse/mesos-5230
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> [#MESOS-5230]
> Change the 'make install' includedir name from slave to agent.
> 
> 
> Diffs
> -
> 
>   CHANGELOG 57f5457640c9b7ca2cb7707f2962dd0421f090f7 
>   src/Makefile.am e024c6d65608a55765e527a8668c415723dcfcca 
> 
> Diff: https://reviews.apache.org/r/46676/diff/
> 
> 
> Testing
> ---
> 
> cd mesos
> ./bootstrap
> mkdir build
> cd build
> ../configure --prefix=${HOME}/install/mesos
> make
> make check
> make install
> 
> 
> Thanks,
> 
> zhou xing
> 
>

Re: Review Request 46613: Introduced VIEW_(FRAMEWORK, TASK}_WITH_INFO actions to authorizer.

[Adam B]

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



This is still a work in progress, I assume. Do we need to modify the authorizer 
interface to be able to take a Shared before we can add these new 
actions?


docs/authorization.md (lines 37 - 38)


Numbers should keep incrementing



include/mesos/authorizer/acls.proto (line 152)


Not necessarily an operator



include/mesos/authorizer/authorizer.proto (line 62)


s/TASK/EXECUTOR/?



src/master/http.cpp (line 1604)


These are not actually called anywhere yet, correct?



src/master/http.cpp (line 1615)


This line isn't correct. We'd rather be passing the entire TaskInfo for 
WITH_INFO, not some undefined field as a string value.


- Adam B


On April 24, 2016, 11:19 a.m., Joerg Schad wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/46613/
> ---
> 
> (Updated April 24, 2016, 11:19 a.m.)
> 
> 
> Review request for mesos, Adam B and Alexander Rojas.
> 
> 
> Bugs: MESOS-5169
> https://issues.apache.org/jira/browse/MESOS-5169
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Introduced VIEW_(FRAMEWORK,TASK}_WITH_INFO actions to authorizer.
> 
> 
> Diffs
> -
> 
>   docs/authorization.md 0db5c345b3239814b3b9d2e8a87601ff69d0f869 
>   include/mesos/authorizer/acls.proto 
> c50deeb5565dfd5b3e5e7210283d9a36a3bfd579 
>   include/mesos/authorizer/authorizer.proto 
> 40d93ea257d1df8d22eee8a21667db90d579a8fe 
>   src/authorizer/local/authorizer.cpp 
> c744a16041c2466d3839a37fbee2bf86887bf4e1 
>   src/master/http.cpp de06985cafc93022fffc0d13812a119ae43b7e57 
> 
> Diff: https://reviews.apache.org/r/46613/diff/
> 
> 
> Testing
> ---
> 
> make check (OSX)
> 
> 
> Thanks,
> 
> Joerg Schad
> 
>

Re: Review Request 46319: Added authorization to agents' `/monitor/statistics` endpoints.

[Benjamin Bannier]

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

(Updated April 26, 2016, 9:59 a.m.)


Review request for mesos, Adam B, Alexander Rojas, and Jan Schlicht.


Changes
---

Addressed comments from Adam (indention, error string).


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


Repository: mesos


Description
---

Added authorization to agents' `/monitor/statistics` endpoints.


Diffs (updated)
-

  src/slave/http.cpp 537736d1fe42e8150bad91326299ef9a17041a8e 
  src/slave/slave.hpp 57b18882e30e44dcc40449b0e3be8ee970c45bc8 
  src/tests/slave_authorization_tests.cpp PRE-CREATION 

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


Testing
---

make check (OS X, clang w/o optimization)


Thanks,

Benjamin Bannier

Re: Review Request 46569: Parameterized agent endpoint authorization tests on tested endpoint.

[Benjamin Bannier]

> On April 26, 2016, 9 a.m., Adam B wrote:
> > src/tests/slave_authorization_tests.cpp, line 171
> > 
> >
> > s/some agent's endpoint/the specified agent endpoint/

Done.


> On April 26, 2016, 9 a.m., Adam B wrote:
> > src/tests/slave_authorization_tests.cpp, line 251
> > 
> >
> > What does this parameter say/do?

This is used to construct case names and will be the prefix. I picked the same 
string as used for the fixture in this file for easier discoverablity; note 
however that since parameterized tests construct new fixture names from both 
prefix and case name on the preprocessor level, this will not result in a name 
conflict. In this case the added test cases will be called like this

SlaveAuthorizationTest/EndpointAuthorization.
  Endpoint/0  # GetParam() = "monitor/statistics"
  Endpoint/1  # GetParam() = "monitor/statistics.json"


- Benjamin


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


On April 26, 2016, 9:59 a.m., Benjamin Bannier wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/46569/
> ---
> 
> (Updated April 26, 2016, 9:59 a.m.)
> 
> 
> Review request for mesos and Adam B.
> 
> 
> Bugs: MESOS-5164
> https://issues.apache.org/jira/browse/MESOS-5164
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> At the moment we only provide a harness for `GET` methods, but will
> generalize this further once we authorize `POST` requests against
> agent endpoints as well.
> 
> 
> Diffs
> -
> 
>   src/tests/slave_authorization_tests.cpp PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/46569/diff/
> 
> 
> Testing
> ---
> 
> make check (OS X, clang trunk w/o optimization)
> 
> 
> Thanks,
> 
> Benjamin Bannier
> 
>