Re: Review Request 41075: Added support for implicit roles.

[Neil Conway]

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

(Updated Dec. 8, 2015, 8:29 a.m.)


Review request for mesos, Adam B, Alexander Rukletsov, Benjamin Hindman, Greg 
Mann, and Yong Qiao Wang.


Changes
---

Rebase, a few minor tweaks/cleanups.


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


Repository: mesos


Description
---

Changed the behavior of the master when the `--roles` flag is NOT
specified. Previously, this would allow only the `*` role to be used. Now,
omitting `--roles` means that any role can be used. This is called "implicit
roles". Configuring which principals can perform operations as which roles
should be done using ACLs in the authorization system.

Note that this changes the behavior of the system when `--roles` is not
specified. This is likely acceptable: if the operator didn't specify `--roles`
in prior versions of Mesos, they were likely not using roles or authorization at
that time.

Another minor behavioral change is that the "/roles" endpoint will now only
return results for currently "active" roles (those with one or more registered
frameworks).

The `--roles` flag is now considered deprecated and will be removed in a future
version of Mesos.


Diffs (updated)
-

  include/mesos/master/allocator.hpp 619ef01c3a7d640560653cfc1838dd09046d1da0 
  include/mesos/master/allocator.proto 702f56f56c3b1331613cecf26522986f6b572f8c 
  src/master/allocator/mesos/allocator.hpp 
97ee80726ad155917811265a983258b0165d3451 
  src/master/allocator/mesos/hierarchical.hpp 
99c742906874c30c39c159e58a65277ade3c07fd 
  src/master/allocator/mesos/hierarchical.cpp 
5da825a1d578a9ee40b4985378fddb3c5fb3b416 
  src/master/http.cpp 9097eda47558fb5306af5a61b464937d7ab34e83 
  src/master/master.hpp 4683fa542a740f9a0b80fff7fbe0e63ec66266f2 
  src/master/master.cpp 953fa4f14929581b226a7e27d30aea7a5aa1fd7c 
  src/master/quota_handler.cpp b209da42ace752953686eeda9577007a33556d5d 
  src/tests/allocator.hpp c7670525765491fe931a4ee38446fa7e9d79af42 
  src/tests/hierarchical_allocator_tests.cpp 
fb214a829a57529d3f5c49730ae9733f53e622ca 

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


Testing
---

"make check" on OSX 10.10 and Ubuntu 15.10; `--gtest_repeat=1000` for the more 
likely role-related tests.

TODOs:

* Update documentation
* Add tests for allocation behavior for weights + implicit roles
* Add tests for quota + implicit roles?


Thanks,

Neil Conway

Re: Review Request 41076: Added tests for implicit roles.

[Neil Conway]

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

(Updated Dec. 8, 2015, 8:28 a.m.)


Review request for mesos, Adam B, Alexander Rukletsov, Benjamin Hindman, Greg 
Mann, and Yong Qiao Wang.


Changes
---

Rebase.


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


Repository: mesos


Description
---

Also added tests for the "/role" HTTP endpoint.


Diffs (updated)
-

  src/tests/role_tests.cpp PRE-CREATION 

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


Testing
---


Thanks,

Neil Conway

Re: Review Request 40995: Added test cases for role behavior.

[Neil Conway]

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

(Updated Dec. 8, 2015, 8:28 a.m.)


Review request for mesos, Adam B, Alexander Rukletsov, Greg Mann, and Yong Qiao 
Wang.


Changes
---

Rebase.


Repository: mesos


Description
---

Added test cases for role behavior.


Diffs (updated)
-

  src/Makefile.am e96e0ec41e11acff00fbfb3e86427b48a0625bd2 
  src/tests/reservation_tests.cpp 3fdf5e121840fe99057e917cca48f1425eff6624 
  src/tests/role_tests.cpp PRE-CREATION 

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


Testing
---

make check.


Thanks,

Neil Conway

Re: Review Request 41075: Added support for implicit roles.

[Neil Conway]

> On Dec. 8, 2015, 7:02 a.m., Yong Qiao Wang wrote:
> > include/mesos/master/allocator.hpp, line 101
> > 
> >
> > When allocator initialize, their should be no active frameworks in 
> > mesos, can we consider to remove this parmeter from initialize()? and can 
> > initialize role-related information in addFramework().
> 
> Neil Conway wrote:
> Hmmm -- I think it is better as written. Right now, weights are static 
> and set at initialization-time, so I think it makes more sense for them to be 
> parameters of `initialize`, not `addFramework`. (When we do dynamic weights 
> we'll need to reconsider this, but even then, I don't think passing weights 
> to `addFramework` will be the right API).
> 
> Yong Qiao Wang wrote:
> For long term solution in Implicit Roles, --roles and --weights should be 
> removed, so weights parameter will only have one default value(*,1). so it is 
> make sence to remove this parameter from initialize (),and when framework 
> register, we can add its role and the corresponding weight into allocator, 
> then they can become active.
> 
> Adam B wrote:
> Even if we eliminate --weights, we'll be persisting the weights map to 
> the registry, and on failover to a new master or on a new master's startup, 
> it will recover the weights from the registry. It seems reasonable to me that 
> after recovering the weights, the master could start the allocator with the 
> newly recovered weights, rather than have to add them after allocator 
> initialization.

Note also that a role's weight is required even _before_ `addFramework`, 
because the weight of a role with quota set is relevant even if the role has no 
registered frameworks. So I think specifying the weight in `addFramework` isn't 
right. We could consider replacing the `initialize` argument with a separate 
`updateWeight` allocator API call, but I'm inclined to defer that until the 
dynamic weight work is actually done.


- Neil


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


On Dec. 8, 2015, 8:33 a.m., Neil Conway wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/41075/
> ---
> 
> (Updated Dec. 8, 2015, 8:33 a.m.)
> 
> 
> Review request for mesos, Adam B, Alexander Rukletsov, Benjamin Hindman, Greg 
> Mann, and Yong Qiao Wang.
> 
> 
> Bugs: MESOS-4085
> https://issues.apache.org/jira/browse/MESOS-4085
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Changed the behavior of the master when the `--roles` flag is NOT
> specified. Previously, this would allow only the `*` role to be used. Now,
> omitting `--roles` means that any role can be used. This is called "implicit
> roles". Configuring which principals can perform operations as which roles
> should be done using ACLs in the authorization system.
> 
> Note that this changes the behavior of the system when `--roles` is not
> specified. This is likely acceptable: if the operator didn't specify `--roles`
> in prior versions of Mesos, they were likely not using roles or authorization 
> at
> that time.
> 
> Another minor behavioral change is that the "/roles" endpoint will now only
> return results for currently "active" roles (those with one or more registered
> frameworks).
> 
> The `--roles` flag is now considered deprecated and will be removed in a 
> future
> version of Mesos.
> 
> 
> Diffs
> -
> 
>   include/mesos/master/allocator.hpp 619ef01c3a7d640560653cfc1838dd09046d1da0 
>   include/mesos/master/allocator.proto 
> 702f56f56c3b1331613cecf26522986f6b572f8c 
>   src/master/allocator/mesos/allocator.hpp 
> 97ee80726ad155917811265a983258b0165d3451 
>   src/master/allocator/mesos/hierarchical.hpp 
> 99c742906874c30c39c159e58a65277ade3c07fd 
>   src/master/allocator/mesos/hierarchical.cpp 
> 5da825a1d578a9ee40b4985378fddb3c5fb3b416 
>   src/master/http.cpp 9097eda47558fb5306af5a61b464937d7ab34e83 
>   src/master/master.hpp 4683fa542a740f9a0b80fff7fbe0e63ec66266f2 
>   src/master/master.cpp 953fa4f14929581b226a7e27d30aea7a5aa1fd7c 
>   src/master/quota_handler.cpp b209da42ace752953686eeda9577007a33556d5d 
>   src/tests/allocator.hpp c7670525765491fe931a4ee38446fa7e9d79af42 
>   src/tests/hierarchical_allocator_tests.cpp 
> fb214a829a57529d3f5c49730ae9733f53e622ca 
> 
> Diff: https://reviews.apache.org/r/41075/diff/
> 
> 
> Testing
> ---
> 
> "make check" on OSX 10.10 and Ubuntu 15.10; `--gtest_repeat=1000` for the 
> more likely role-related tests.
> 
> TODOs:
> 
> * Update documentation
> * Add tests for allocation behavior for weights + implicit roles
> * Add tests for quota + implicit roles?
> 
> Notes:
> 
> * 

Re: Review Request 41075: Added support for implicit roles.

[Adam B]

> On Dec. 7, 2015, 11:02 p.m., Yong Qiao Wang wrote:
> >
> 
> Yong Qiao Wang wrote:
> I have talked with our shepherd Adam B yesterday, and we all agree to 
> improve /roles endpoint to update/remove/list active roles, and Implicit 
> Roles will focus on removing the static role list(specified by --roles flag) 
> and let framework can register with any role. In addition, RoleInfo is still 
> needed, and will be persisted when the default values are changed by /roles 
> endpoint. Currently I also have posted some patches for Dynamic Roles, and 
> there are some serious comflicts between our patches. Can you have a talk 
> with our shepherd? I think we should reach a consensus before coding.

I'm still not sure about removeRole, but update/list make sense for dynamic 
weights. We'll have to discuss the pros/cons of RoleInfo vs. a separate weights 
hashmap, but let's try to communicate & collaborate on our approaches to avoid 
too many unnecessary merge conflicts.


> On Dec. 7, 2015, 11:02 p.m., Yong Qiao Wang wrote:
> > include/mesos/master/allocator.hpp, line 101
> > 
> >
> > When allocator initialize, their should be no active frameworks in 
> > mesos, can we consider to remove this parmeter from initialize()? and can 
> > initialize role-related information in addFramework().
> 
> Neil Conway wrote:
> Hmmm -- I think it is better as written. Right now, weights are static 
> and set at initialization-time, so I think it makes more sense for them to be 
> parameters of `initialize`, not `addFramework`. (When we do dynamic weights 
> we'll need to reconsider this, but even then, I don't think passing weights 
> to `addFramework` will be the right API).
> 
> Yong Qiao Wang wrote:
> For long term solution in Implicit Roles, --roles and --weights should be 
> removed, so weights parameter will only have one default value(*,1). so it is 
> make sence to remove this parameter from initialize (),and when framework 
> register, we can add its role and the corresponding weight into allocator, 
> then they can become active.

Even if we eliminate --weights, we'll be persisting the weights map to the 
registry, and on failover to a new master or on a new master's startup, it will 
recover the weights from the registry. It seems reasonable to me that after 
recovering the weights, the master could start the allocator with the newly 
recovered weights, rather than have to add them after allocator initialization.


- Adam


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


On Dec. 7, 2015, 9:41 p.m., Neil Conway wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/41075/
> ---
> 
> (Updated Dec. 7, 2015, 9:41 p.m.)
> 
> 
> Review request for mesos, Adam B, Alexander Rukletsov, Benjamin Hindman, Greg 
> Mann, and Yong Qiao Wang.
> 
> 
> Bugs: MESOS-4085
> https://issues.apache.org/jira/browse/MESOS-4085
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Changed the behavior of the master when the `--roles` flag is NOT
> specified. Previously, this would allow only the `*` role to be used. Now,
> omitting `--roles` means that any role can be used. This is called "implicit
> roles". Configuring which principals can perform operations as which roles
> should be done using ACLs in the authorization system.
> 
> Note that this changes the behavior of the system when `--roles` is not
> specified. This is likely acceptable: if the operator didn't specify `--roles`
> in prior versions of Mesos, they were likely not using roles or authorization 
> at
> that time.
> 
> Another minor behavioral change is that the "/roles" endpoint will now only
> return results for currently "active" roles (those with one or more registered
> frameworks).
> 
> The `--roles` flag is now considered deprecated and will be removed in a 
> future
> version of Mesos.
> 
> 
> Diffs
> -
> 
>   include/mesos/master/allocator.hpp 619ef01c3a7d640560653cfc1838dd09046d1da0 
>   include/mesos/master/allocator.proto 
> 702f56f56c3b1331613cecf26522986f6b572f8c 
>   src/master/allocator/mesos/allocator.hpp 
> 97ee80726ad155917811265a983258b0165d3451 
>   src/master/allocator/mesos/hierarchical.hpp 
> 99c742906874c30c39c159e58a65277ade3c07fd 
>   src/master/allocator/mesos/hierarchical.cpp 
> 5da825a1d578a9ee40b4985378fddb3c5fb3b416 
>   src/master/http.cpp 9097eda47558fb5306af5a61b464937d7ab34e83 
>   src/master/master.hpp 4683fa542a740f9a0b80fff7fbe0e63ec66266f2 
>   src/master/master.cpp 953fa4f14929581b226a7e27d30aea7a5aa1fd7c 
>   src/master/quota_handler.cpp b209da42ace752953686eeda9577007a33556d5d 
>   

Re: Review Request 41075: Added support for implicit roles.

[Neil Conway]

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

(Updated Dec. 8, 2015, 8:33 a.m.)


Review request for mesos, Adam B, Alexander Rukletsov, Benjamin Hindman, Greg 
Mann, and Yong Qiao Wang.


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


Repository: mesos


Description
---

Changed the behavior of the master when the `--roles` flag is NOT
specified. Previously, this would allow only the `*` role to be used. Now,
omitting `--roles` means that any role can be used. This is called "implicit
roles". Configuring which principals can perform operations as which roles
should be done using ACLs in the authorization system.

Note that this changes the behavior of the system when `--roles` is not
specified. This is likely acceptable: if the operator didn't specify `--roles`
in prior versions of Mesos, they were likely not using roles or authorization at
that time.

Another minor behavioral change is that the "/roles" endpoint will now only
return results for currently "active" roles (those with one or more registered
frameworks).

The `--roles` flag is now considered deprecated and will be removed in a future
version of Mesos.


Diffs
-

  include/mesos/master/allocator.hpp 619ef01c3a7d640560653cfc1838dd09046d1da0 
  include/mesos/master/allocator.proto 702f56f56c3b1331613cecf26522986f6b572f8c 
  src/master/allocator/mesos/allocator.hpp 
97ee80726ad155917811265a983258b0165d3451 
  src/master/allocator/mesos/hierarchical.hpp 
99c742906874c30c39c159e58a65277ade3c07fd 
  src/master/allocator/mesos/hierarchical.cpp 
5da825a1d578a9ee40b4985378fddb3c5fb3b416 
  src/master/http.cpp 9097eda47558fb5306af5a61b464937d7ab34e83 
  src/master/master.hpp 4683fa542a740f9a0b80fff7fbe0e63ec66266f2 
  src/master/master.cpp 953fa4f14929581b226a7e27d30aea7a5aa1fd7c 
  src/master/quota_handler.cpp b209da42ace752953686eeda9577007a33556d5d 
  src/tests/allocator.hpp c7670525765491fe931a4ee38446fa7e9d79af42 
  src/tests/hierarchical_allocator_tests.cpp 
fb214a829a57529d3f5c49730ae9733f53e622ca 

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


Testing (updated)
---

"make check" on OSX 10.10 and Ubuntu 15.10; `--gtest_repeat=1000` for the more 
likely role-related tests.

TODOs:

* Update documentation
* Add tests for allocation behavior for weights + implicit roles
* Add tests for quota + implicit roles?

Notes:

* There's two places where we use manual `new`/`delete` where a `unique_ptr` 
would probably be nicer. I'm inclined to leave this as-is for now though 
(making use of unique_ptr is a broader issue).


Thanks,

Neil Conway

Re: Review Request 40469: Update Allocator interface to support dynamic roles

[Mesos ReviewBot]

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


Patch looks great!

Reviews applied: [40431, 40469]

Passed command: export OS=ubuntu:14.04;export CONFIGURATION="--verbose";export 
COMPILER=gcc; ./support/docker_build.sh

- Mesos ReviewBot


On Dec. 8, 2015, 5:20 a.m., Yong Qiao Wang wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/40469/
> ---
> 
> (Updated Dec. 8, 2015, 5:20 a.m.)
> 
> 
> Review request for mesos, Adam B, Guangya Liu, Qian Zhang, and Jian Qiu.
> 
> 
> Bugs: MESOS-3956
> https://issues.apache.org/jira/browse/MESOS-3956
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Update Allocator interface to support dynamic roles
> 
> 
> Diffs
> -
> 
>   include/mesos/master/allocator.hpp 619ef01c3a7d640560653cfc1838dd09046d1da0 
>   src/master/allocator/mesos/allocator.hpp 
> 97ee80726ad155917811265a983258b0165d3451 
>   src/master/allocator/mesos/hierarchical.hpp 
> 99c742906874c30c39c159e58a65277ade3c07fd 
>   src/master/allocator/mesos/hierarchical.cpp 
> 5da825a1d578a9ee40b4985378fddb3c5fb3b416 
>   src/tests/allocator.hpp c7670525765491fe931a4ee38446fa7e9d79af42 
> 
> Diff: https://reviews.apache.org/r/40469/diff/
> 
> 
> Testing
> ---
> 
> Make check successfully.
> 
> 
> Thanks,
> 
> Yong Qiao Wang
> 
>

Re: Review Request 39043: Added support for HTTP Authentication in Mesos.

[Alexander Rojas]

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

(Updated Dec. 8, 2015, 11:24 a.m.)


Review request for mesos, Adam B, Bernd Mathiske, Isabel Jimenez, and Till 
Toenshoff.


Changes
---

Forcing buildbot rebuild.


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


Repository: mesos


Description
---

1. Adds a flag to load an HTTP Authenticator module from the flags.
2. If provided, uses the credentials file to initialize the default HTTP 
Authenticator.
3. Updates the existing endpoints which implement their own basic HTTP 
authenticator with the libprocess one.
4. Updates one test which expected the wrong results since now credentials are 
checked before the body of the request.


Diffs (updated)
-

  src/master/constants.hpp cc38dfcf57ecbc8555379084acbf2bfa4b3fc759 
  src/master/constants.cpp 98ea7c8f1e7c3057cfe941f82f1b71f7731a8f32 
  src/master/flags.hpp 9af6c68eef6bcf39d5776809fab6c66dc95da6b2 
  src/master/flags.cpp f9223edb33483cb5619e7dd75221f735306462c6 
  src/master/http.cpp 9097eda47558fb5306af5a61b464937d7ab34e83 
  src/master/master.hpp 4683fa542a740f9a0b80fff7fbe0e63ec66266f2 
  src/master/master.cpp 953fa4f14929581b226a7e27d30aea7a5aa1fd7c 

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


Testing
---

make check


Thanks,

Alexander Rojas

Re: Review Request 38950: Http Authenticators can be loaded as modules from mesos.

[Alexander Rojas]

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

(Updated Dec. 8, 2015, 11:24 a.m.)


Review request for mesos, Adam B, Bernd Mathiske, and Till Toenshoff.


Changes
---

Fixed copyright notice in new files.


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


Repository: mesos


Description
---

Adds support for modularization of HTTP Authenticators. 

It includes an example of how to do it with the Basic HTTP Authenticator.


Diffs (updated)
-

  include/mesos/authentication/http/basic_authenticator_factory.hpp 
PRE-CREATION 
  include/mesos/module/http_authenticator.hpp PRE-CREATION 
  src/CMakeLists.txt c0d77c745eb5b12dd6d9d7afaba7e820f8d848ef 
  src/Makefile.am e96e0ec41e11acff00fbfb3e86427b48a0625bd2 
  src/authentication/http/basic_authenticator_factory.cpp PRE-CREATION 
  src/examples/test_http_authenticator_module.cpp PRE-CREATION 
  src/master/constants.hpp cc38dfcf57ecbc8555379084acbf2bfa4b3fc759 
  src/master/constants.cpp 98ea7c8f1e7c3057cfe941f82f1b71f7731a8f32 
  src/module/manager.cpp 1f04790510a2ab9ccd6907fd01be192f52ee90c6 
  src/tests/http_authentication_tests.cpp PRE-CREATION 
  src/tests/module.hpp e46ed12c80707bf44ceef3ed1a4eb2f321ce10f6 
  src/tests/module.cpp e272bf0eccb61ae54440ec79adac8efad804c828 

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


Testing
---

make check


Thanks,

Alexander Rojas

Re: Review Request 38094: Added implementation of Http Basic authentication scheme.

[Alexander Rojas]

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



3rdparty/libprocess/src/tests/http_tests.cpp (line 1408)


Because the previous RR was testing a specific but that raises under 
specifi circumstances, and this just test that authentication works.


- Alexander Rojas


On Dec. 7, 2015, 4:11 p.m., Alexander Rojas wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/38094/
> ---
> 
> (Updated Dec. 7, 2015, 4:11 p.m.)
> 
> 
> Review request for mesos, Adam B, Benjamin Hindman, Bernd Mathiske, and Till 
> Toenshoff.
> 
> 
> Bugs: MESOS-3232
> https://issues.apache.org/jira/browse/MESOS-3232
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> See summary.
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/Makefile.am 6ec6d7989df647f466ac6079738835ffcb2ea8ee 
>   3rdparty/libprocess/include/process/authenticator.hpp 
> 5a32e9a38a0bec7aa3faef23b792f3bf3d659d4f 
>   3rdparty/libprocess/src/CMakeLists.txt 
> 681f0cfec57e152568da41698c8bdd52c05f65a6 
>   3rdparty/libprocess/src/authenticator.cpp PRE-CREATION 
>   3rdparty/libprocess/src/tests/http_tests.cpp 
> 2de75ca1c7e224c36b534c368e7379dc158aa5bb 
> 
> Diff: https://reviews.apache.org/r/38094/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Alexander Rojas
> 
>

Re: Review Request 41075: Added support for implicit roles.

[Yong Qiao Wang]

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



src/master/allocator/mesos/hierarchical.cpp (line 301)


In the current implementation, quota(quotaSorter) does not been removed 
after the corresponding role is removed, then some resources will still be 
reserved by this non-active role, this will result in the waste of resources. 
If you rely on cluster operator to remove this quota, I think it is does not 
make sence and have a bad user experience, because role is implicitly removed 
when the last framework gone without notify the clsuter admin.


- Yong Qiao Wang


On Dec. 8, 2015, 8:33 a.m., Neil Conway wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/41075/
> ---
> 
> (Updated Dec. 8, 2015, 8:33 a.m.)
> 
> 
> Review request for mesos, Adam B, Alexander Rukletsov, Benjamin Hindman, Greg 
> Mann, and Yong Qiao Wang.
> 
> 
> Bugs: MESOS-4085
> https://issues.apache.org/jira/browse/MESOS-4085
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Changed the behavior of the master when the `--roles` flag is NOT
> specified. Previously, this would allow only the `*` role to be used. Now,
> omitting `--roles` means that any role can be used. This is called "implicit
> roles". Configuring which principals can perform operations as which roles
> should be done using ACLs in the authorization system.
> 
> Note that this changes the behavior of the system when `--roles` is not
> specified. This is likely acceptable: if the operator didn't specify `--roles`
> in prior versions of Mesos, they were likely not using roles or authorization 
> at
> that time.
> 
> Another minor behavioral change is that the "/roles" endpoint will now only
> return results for currently "active" roles (those with one or more registered
> frameworks).
> 
> The `--roles` flag is now considered deprecated and will be removed in a 
> future
> version of Mesos.
> 
> 
> Diffs
> -
> 
>   include/mesos/master/allocator.hpp 619ef01c3a7d640560653cfc1838dd09046d1da0 
>   include/mesos/master/allocator.proto 
> 702f56f56c3b1331613cecf26522986f6b572f8c 
>   src/master/allocator/mesos/allocator.hpp 
> 97ee80726ad155917811265a983258b0165d3451 
>   src/master/allocator/mesos/hierarchical.hpp 
> 99c742906874c30c39c159e58a65277ade3c07fd 
>   src/master/allocator/mesos/hierarchical.cpp 
> 5da825a1d578a9ee40b4985378fddb3c5fb3b416 
>   src/master/http.cpp 9097eda47558fb5306af5a61b464937d7ab34e83 
>   src/master/master.hpp 4683fa542a740f9a0b80fff7fbe0e63ec66266f2 
>   src/master/master.cpp 953fa4f14929581b226a7e27d30aea7a5aa1fd7c 
>   src/master/quota_handler.cpp b209da42ace752953686eeda9577007a33556d5d 
>   src/tests/allocator.hpp c7670525765491fe931a4ee38446fa7e9d79af42 
>   src/tests/hierarchical_allocator_tests.cpp 
> fb214a829a57529d3f5c49730ae9733f53e622ca 
> 
> Diff: https://reviews.apache.org/r/41075/diff/
> 
> 
> Testing
> ---
> 
> "make check" on OSX 10.10 and Ubuntu 15.10; `--gtest_repeat=1000` for the 
> more likely role-related tests.
> 
> TODOs:
> 
> * Update documentation
> * Add tests for allocation behavior for weights + implicit roles
> * Add tests for quota + implicit roles?
> 
> Notes:
> 
> * There's two places where we use manual `new`/`delete` where a `unique_ptr` 
> would probably be nicer. I'm inclined to leave this as-is for now though 
> (making use of unique_ptr is a broader issue).
> 
> 
> Thanks,
> 
> Neil Conway
> 
>

Re: Review Request 41075: Added support for implicit roles.

[Yong Qiao Wang]

> On Dec. 8, 2015, 7:02 a.m., Yong Qiao Wang wrote:
> >
> 
> Yong Qiao Wang wrote:
> I have talked with our shepherd Adam B yesterday, and we all agree to 
> improve /roles endpoint to update/remove/list active roles, and Implicit 
> Roles will focus on removing the static role list(specified by --roles flag) 
> and let framework can register with any role. In addition, RoleInfo is still 
> needed, and will be persisted when the default values are changed by /roles 
> endpoint. Currently I also have posted some patches for Dynamic Roles, and 
> there are some serious comflicts between our patches. Can you have a talk 
> with our shepherd? I think we should reach a consensus before coding.
> 
> Adam B wrote:
> I'm still not sure about removeRole, but update/list make sense for 
> dynamic weights. We'll have to discuss the pros/cons of RoleInfo vs. a 
> separate weights hashmap, but let's try to communicate & collaborate on our 
> approaches to avoid too many unnecessary merge conflicts.

In the current implementation, role will be implicitly removed after the last 
framework gone, but it related configurations, such as quota, weight will not 
be removed, this will cause data inconsistencies, so I think we should let 
cluster operator to remove the role and it's related configuration after the 
corresponding framework no longer recovery.

In addition, if we all agree to use /roles to update the weight in RoleInfo and 
persist RoleInfo, then I think it is better to still use RoleInfo rather than 
weights hashmap, because it can pass more information to allocator, then it can 
avoid the frequent changes of the interface of allocator. For example, if we 
add Grace Period for a role in Opetimisitic Offer ticket, and maybe allocator 
also need to know it, then we on longer need to change this interface again 
later.


- Yong Qiao


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


On Dec. 8, 2015, 8:33 a.m., Neil Conway wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/41075/
> ---
> 
> (Updated Dec. 8, 2015, 8:33 a.m.)
> 
> 
> Review request for mesos, Adam B, Alexander Rukletsov, Benjamin Hindman, Greg 
> Mann, and Yong Qiao Wang.
> 
> 
> Bugs: MESOS-4085
> https://issues.apache.org/jira/browse/MESOS-4085
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Changed the behavior of the master when the `--roles` flag is NOT
> specified. Previously, this would allow only the `*` role to be used. Now,
> omitting `--roles` means that any role can be used. This is called "implicit
> roles". Configuring which principals can perform operations as which roles
> should be done using ACLs in the authorization system.
> 
> Note that this changes the behavior of the system when `--roles` is not
> specified. This is likely acceptable: if the operator didn't specify `--roles`
> in prior versions of Mesos, they were likely not using roles or authorization 
> at
> that time.
> 
> Another minor behavioral change is that the "/roles" endpoint will now only
> return results for currently "active" roles (those with one or more registered
> frameworks).
> 
> The `--roles` flag is now considered deprecated and will be removed in a 
> future
> version of Mesos.
> 
> 
> Diffs
> -
> 
>   include/mesos/master/allocator.hpp 619ef01c3a7d640560653cfc1838dd09046d1da0 
>   include/mesos/master/allocator.proto 
> 702f56f56c3b1331613cecf26522986f6b572f8c 
>   src/master/allocator/mesos/allocator.hpp 
> 97ee80726ad155917811265a983258b0165d3451 
>   src/master/allocator/mesos/hierarchical.hpp 
> 99c742906874c30c39c159e58a65277ade3c07fd 
>   src/master/allocator/mesos/hierarchical.cpp 
> 5da825a1d578a9ee40b4985378fddb3c5fb3b416 
>   src/master/http.cpp 9097eda47558fb5306af5a61b464937d7ab34e83 
>   src/master/master.hpp 4683fa542a740f9a0b80fff7fbe0e63ec66266f2 
>   src/master/master.cpp 953fa4f14929581b226a7e27d30aea7a5aa1fd7c 
>   src/master/quota_handler.cpp b209da42ace752953686eeda9577007a33556d5d 
>   src/tests/allocator.hpp c7670525765491fe931a4ee38446fa7e9d79af42 
>   src/tests/hierarchical_allocator_tests.cpp 
> fb214a829a57529d3f5c49730ae9733f53e622ca 
> 
> Diff: https://reviews.apache.org/r/41075/diff/
> 
> 
> Testing
> ---
> 
> "make check" on OSX 10.10 and Ubuntu 15.10; `--gtest_repeat=1000` for the 
> more likely role-related tests.
> 
> TODOs:
> 
> * Update documentation
> * Add tests for allocation behavior for weights + implicit roles
> * Add tests for quota + implicit roles?
> 
> Notes:
> 
> * There's two places where we use manual `new`/`delete` where a `unique_ptr` 
> would probably be nicer. I'm inclined to leave this as-is for now though 
> (making use 

Re: Review Request 41075: Added support for implicit roles.

[Qian Zhang]

> On Dec. 8, 2015, 3:02 p.m., Yong Qiao Wang wrote:
> > include/mesos/master/allocator.hpp, line 101
> > 
> >
> > When allocator initialize, their should be no active frameworks in 
> > mesos, can we consider to remove this parmeter from initialize()? and can 
> > initialize role-related information in addFramework().
> 
> Neil Conway wrote:
> Hmmm -- I think it is better as written. Right now, weights are static 
> and set at initialization-time, so I think it makes more sense for them to be 
> parameters of `initialize`, not `addFramework`. (When we do dynamic weights 
> we'll need to reconsider this, but even then, I don't think passing weights 
> to `addFramework` will be the right API).
> 
> Yong Qiao Wang wrote:
> For long term solution in Implicit Roles, --roles and --weights should be 
> removed, so weights parameter will only have one default value(*,1). so it is 
> make sence to remove this parameter from initialize (),and when framework 
> register, we can add its role and the corresponding weight into allocator, 
> then they can become active.
> 
> Adam B wrote:
> Even if we eliminate --weights, we'll be persisting the weights map to 
> the registry, and on failover to a new master or on a new master's startup, 
> it will recover the weights from the registry. It seems reasonable to me that 
> after recovering the weights, the master could start the allocator with the 
> newly recovered weights, rather than have to add them after allocator 
> initialization.
> 
> Neil Conway wrote:
> Note also that a role's weight is required even _before_ `addFramework`, 
> because the weight of a role with quota set is relevant even if the role has 
> no registered frameworks. So I think specifying the weight in `addFramework` 
> isn't right. We could consider replacing the `initialize` argument with a 
> separate `updateWeight` allocator API call, but I'm inclined to defer that 
> until the dynamic weight work is actually done.

Neil, can you please clarify a little more about why the weight of a role with 
quota set is relevant even if the role has no registered frameworks?


- Qian


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


On Dec. 8, 2015, 4:33 p.m., Neil Conway wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/41075/
> ---
> 
> (Updated Dec. 8, 2015, 4:33 p.m.)
> 
> 
> Review request for mesos, Adam B, Alexander Rukletsov, Benjamin Hindman, Greg 
> Mann, and Yong Qiao Wang.
> 
> 
> Bugs: MESOS-4085
> https://issues.apache.org/jira/browse/MESOS-4085
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Changed the behavior of the master when the `--roles` flag is NOT
> specified. Previously, this would allow only the `*` role to be used. Now,
> omitting `--roles` means that any role can be used. This is called "implicit
> roles". Configuring which principals can perform operations as which roles
> should be done using ACLs in the authorization system.
> 
> Note that this changes the behavior of the system when `--roles` is not
> specified. This is likely acceptable: if the operator didn't specify `--roles`
> in prior versions of Mesos, they were likely not using roles or authorization 
> at
> that time.
> 
> Another minor behavioral change is that the "/roles" endpoint will now only
> return results for currently "active" roles (those with one or more registered
> frameworks).
> 
> The `--roles` flag is now considered deprecated and will be removed in a 
> future
> version of Mesos.
> 
> 
> Diffs
> -
> 
>   include/mesos/master/allocator.hpp 619ef01c3a7d640560653cfc1838dd09046d1da0 
>   include/mesos/master/allocator.proto 
> 702f56f56c3b1331613cecf26522986f6b572f8c 
>   src/master/allocator/mesos/allocator.hpp 
> 97ee80726ad155917811265a983258b0165d3451 
>   src/master/allocator/mesos/hierarchical.hpp 
> 99c742906874c30c39c159e58a65277ade3c07fd 
>   src/master/allocator/mesos/hierarchical.cpp 
> 5da825a1d578a9ee40b4985378fddb3c5fb3b416 
>   src/master/http.cpp 9097eda47558fb5306af5a61b464937d7ab34e83 
>   src/master/master.hpp 4683fa542a740f9a0b80fff7fbe0e63ec66266f2 
>   src/master/master.cpp 953fa4f14929581b226a7e27d30aea7a5aa1fd7c 
>   src/master/quota_handler.cpp b209da42ace752953686eeda9577007a33556d5d 
>   src/tests/allocator.hpp c7670525765491fe931a4ee38446fa7e9d79af42 
>   src/tests/hierarchical_allocator_tests.cpp 
> fb214a829a57529d3f5c49730ae9733f53e622ca 
> 
> Diff: https://reviews.apache.org/r/41075/diff/
> 
> 
> Testing
> ---
> 
> "make check" on OSX 10.10 and Ubuntu 15.10; `--gtest_repeat=1000` for the 
> more likely 

Re: Review Request 38000: Introduced support for user interaction with HTTP AuthenticationRouter.

[Alexander Rojas]

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

(Updated Dec. 8, 2015, 11:23 a.m.)


Review request for mesos, Adam B, Benjamin Hindman, Bernd Mathiske, and Till 
Toenshoff.


Changes
---

Alex's review.


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


Repository: mesos


Description
---

Adds functions which allow libprocess users to register HTTP authenticators.
Overloads `ProcesBase::route()` to allow for registering of authenticating 
endpoints.
Includes tests.


Diffs (updated)
-

  3rdparty/libprocess/include/process/event.hpp 
a03824c061c4a0eb865b163999a763635e56744c 
  3rdparty/libprocess/include/process/process.hpp 
81c094414d4d5ac5eb593df2a6d14aaacb19a826 
  3rdparty/libprocess/src/process.cpp e93709d3bb4ac588457bb9331fc05ec5ab539f6d 
  3rdparty/libprocess/src/tests/http_tests.cpp 
2de75ca1c7e224c36b534c368e7379dc158aa5bb 

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


Testing
---

make check


Thanks,

Alexander Rojas

Re: Review Request 38000: Introduced support for user interaction with HTTP AuthenticationRouter.

[Alexander Rojas]

> On Dec. 7, 2015, 11:51 p.m., Alexander Rukletsov wrote:
> > 3rdparty/libprocess/src/process.cpp, lines 3218-3220
> > 
> >
> > If this is an internal invariant, how about adding a `CHECK` here?

How would the check be liked? if you read the comment it either has a principal 
because otherwise authentication failed or there is no principal because 
authentication wasn't needed at all. I changed the comment though.


- Alexander


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


On Dec. 7, 2015, 3:14 p.m., Alexander Rojas wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/38000/
> ---
> 
> (Updated Dec. 7, 2015, 3:14 p.m.)
> 
> 
> Review request for mesos, Adam B, Benjamin Hindman, Bernd Mathiske, and Till 
> Toenshoff.
> 
> 
> Bugs: MESOS-3233
> https://issues.apache.org/jira/browse/MESOS-3233
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Adds functions which allow libprocess users to register HTTP authenticators.
> Overloads `ProcesBase::route()` to allow for registering of authenticating 
> endpoints.
> Includes tests.
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/include/process/event.hpp 
> a03824c061c4a0eb865b163999a763635e56744c 
>   3rdparty/libprocess/include/process/process.hpp 
> 81c094414d4d5ac5eb593df2a6d14aaacb19a826 
>   3rdparty/libprocess/src/process.cpp 
> e93709d3bb4ac588457bb9331fc05ec5ab539f6d 
>   3rdparty/libprocess/src/tests/http_tests.cpp 
> 2de75ca1c7e224c36b534c368e7379dc158aa5bb 
> 
> Diff: https://reviews.apache.org/r/38000/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Alexander Rojas
> 
>

Re: Review Request 40617: Added Load QoS Controller for simple eviction when system load is above configured threshold.

[Bartek Plotka]

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

(Updated Dec. 8, 2015, 11:16 a.m.)


Review request for mesos, Niklas Nielsen and Vinod Kone.


Changes
---

Moved to standard license header (with `//` comment syntax)


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


Repository: mesos


Description
---

Added Load QoS Controller for the simple eviction when system load is above 
configured system load threshold for 5min and 15min:
- Made os::loadavg called from the lambda and passed via the contructor.
- Added unit test.


Diffs (updated)
-

  src/Makefile.am a57e46d06c8c26a32a9444be4f0e1269d775f8b0 
  src/slave/qos_controllers/load.hpp PRE-CREATION 
  src/slave/qos_controllers/load.cpp PRE-CREATION 
  src/tests/oversubscription_tests.cpp 0333281c247dd182860a49f39be791c00679bf6b 

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


Testing
---

make check
run mesos with org_apache_mesos_LoadQoSController module included.


Thanks,

Bartek Plotka

Re: Review Request 41077: Fixed the license header in src/linux/ns.hpp.

[Benjamin Bannier]

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

Ship it!


Ship It!

- Benjamin Bannier


On Dec. 8, 2015, 6:26 a.m., Jie Yu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/41077/
> ---
> 
> (Updated Dec. 8, 2015, 6:26 a.m.)
> 
> 
> Review request for mesos and Benjamin Bannier.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Fixed the license header in src/linux/ns.hpp.
> 
> 
> Diffs
> -
> 
>   src/linux/ns.hpp 1ebf2dbf9bbb350e91fc52925bd424b0a44c34d5 
> 
> Diff: https://reviews.apache.org/r/41077/diff/
> 
> 
> Testing
> ---
> 
> trivial
> 
> 
> Thanks,
> 
> Jie Yu
> 
>

Re: Review Request 41042: Added description of the LoadQoSController in the oversubscription.md

[Bartek Plotka]

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

(Updated Dec. 8, 2015, 1:24 p.m.)


Review request for mesos, Niklas Nielsen and Vinod Kone.


Changes
---

Issues addressed.


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


Repository: mesos


Description
---

Added description to _Writing a custom QoS controller_ and _Configuring  
oversubscription_ sections.


Diffs (updated)
-

  docs/oversubscription.md 7d1415a712f818f7664ed8322ddcdc57d3a1fb1f 

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


Testing
---


Thanks,

Bartek Plotka

Re: Review Request 40995: Added test cases for role behavior.

[Alexander Rukletsov]

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


One thing we also agreed is to disallow empty string roles. Do you think it 
makes sense to extend this patch with a test for that or do it separately?


src/tests/role_tests.cpp (line 36)


I believe we use third person singular in such cases.



src/tests/role_tests.cpp (lines 76 - 77)


I think a comment here expaining why you reduce the allocation interval 
will be good for posterity. Alternatively, you can use 
`Clock::advance(flags.allocation_interval)` before `AWAIT_READY(offers)` to 
achieve the same. Perhaps it's a cleaner solution.



src/tests/role_tests.cpp (lines 83 - 84)


`MesosTest` exposes the `defaultAgentResourcesString` constant. I think you 
can get rid of these lines in order not to distract the user and make sure 
`unreserved` is contained in `defaultAgentResourcesString` later in the code:
```

EXPECT_TRUE(Resources::parse(defaultAgentResourcesString).get().contains(unreserved));
```



src/tests/role_tests.cpp (line 172)


Backticks?


- Alexander Rukletsov


On Dec. 8, 2015, 8:28 a.m., Neil Conway wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/40995/
> ---
> 
> (Updated Dec. 8, 2015, 8:28 a.m.)
> 
> 
> Review request for mesos, Adam B, Alexander Rukletsov, Greg Mann, and Yong 
> Qiao Wang.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added test cases for role behavior.
> 
> 
> Diffs
> -
> 
>   src/Makefile.am e96e0ec41e11acff00fbfb3e86427b48a0625bd2 
>   src/tests/reservation_tests.cpp 3fdf5e121840fe99057e917cca48f1425eff6624 
>   src/tests/role_tests.cpp PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/40995/diff/
> 
> 
> Testing
> ---
> 
> make check.
> 
> 
> Thanks,
> 
> Neil Conway
> 
>

Re: Review Request 41075: Added support for implicit roles.

[Qian Zhang]

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



src/master/allocator/mesos/hierarchical.hpp (line 367)


Why "Frameworks are removed from this map"? I think it should be "Role" 
removed from this map.



src/master/allocator/mesos/hierarchical.hpp (line 407)


s/iff/if/



src/master/master.hpp (line 1331)


Do we need to put a TODO here to mention that `validRoles` needs to be 
removed once "explicit roles" feature is deprecated? I believe at that time, we 
will just rely on ACL.



src/master/master.cpp (lines 5822 - 5823)


Suggest to erase the role first, and then delete it, like how we remove 
offer in `Master::removeOffer()`


- Qian Zhang


On Dec. 8, 2015, 4:33 p.m., Neil Conway wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/41075/
> ---
> 
> (Updated Dec. 8, 2015, 4:33 p.m.)
> 
> 
> Review request for mesos, Adam B, Alexander Rukletsov, Benjamin Hindman, Greg 
> Mann, and Yong Qiao Wang.
> 
> 
> Bugs: MESOS-4085
> https://issues.apache.org/jira/browse/MESOS-4085
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Changed the behavior of the master when the `--roles` flag is NOT
> specified. Previously, this would allow only the `*` role to be used. Now,
> omitting `--roles` means that any role can be used. This is called "implicit
> roles". Configuring which principals can perform operations as which roles
> should be done using ACLs in the authorization system.
> 
> Note that this changes the behavior of the system when `--roles` is not
> specified. This is likely acceptable: if the operator didn't specify `--roles`
> in prior versions of Mesos, they were likely not using roles or authorization 
> at
> that time.
> 
> Another minor behavioral change is that the "/roles" endpoint will now only
> return results for currently "active" roles (those with one or more registered
> frameworks).
> 
> The `--roles` flag is now considered deprecated and will be removed in a 
> future
> version of Mesos.
> 
> 
> Diffs
> -
> 
>   include/mesos/master/allocator.hpp 619ef01c3a7d640560653cfc1838dd09046d1da0 
>   include/mesos/master/allocator.proto 
> 702f56f56c3b1331613cecf26522986f6b572f8c 
>   src/master/allocator/mesos/allocator.hpp 
> 97ee80726ad155917811265a983258b0165d3451 
>   src/master/allocator/mesos/hierarchical.hpp 
> 99c742906874c30c39c159e58a65277ade3c07fd 
>   src/master/allocator/mesos/hierarchical.cpp 
> 5da825a1d578a9ee40b4985378fddb3c5fb3b416 
>   src/master/http.cpp 9097eda47558fb5306af5a61b464937d7ab34e83 
>   src/master/master.hpp 4683fa542a740f9a0b80fff7fbe0e63ec66266f2 
>   src/master/master.cpp 953fa4f14929581b226a7e27d30aea7a5aa1fd7c 
>   src/master/quota_handler.cpp b209da42ace752953686eeda9577007a33556d5d 
>   src/tests/allocator.hpp c7670525765491fe931a4ee38446fa7e9d79af42 
>   src/tests/hierarchical_allocator_tests.cpp 
> fb214a829a57529d3f5c49730ae9733f53e622ca 
> 
> Diff: https://reviews.apache.org/r/41075/diff/
> 
> 
> Testing
> ---
> 
> "make check" on OSX 10.10 and Ubuntu 15.10; `--gtest_repeat=1000` for the 
> more likely role-related tests.
> 
> TODOs:
> 
> * Update documentation
> * Add tests for allocation behavior for weights + implicit roles
> * Add tests for quota + implicit roles?
> 
> Notes:
> 
> * There's two places where we use manual `new`/`delete` where a `unique_ptr` 
> would probably be nicer. I'm inclined to leave this as-is for now though 
> (making use of unique_ptr is a broader issue).
> 
> 
> Thanks,
> 
> Neil Conway
> 
>

Re: Review Request 40995: Added test cases for role behavior.

[Alexander Rukletsov]

> On Dec. 8, 2015, 12:51 a.m., Guangya Liu wrote:
> > src/tests/role_tests.cpp, lines 24-27
> > 
> >
> > I think that we should always use std first?
> > 
> > using std::vector;
> > 
> > using process::Future;
> > using process::PID;
> 
> Neil Conway wrote:
> All the test cases I looked at place `process` before `std`: e.g., 
> `fault_tolerance_tests.cpp`, `master_tests.cpp`, `reservation_tests.cpp`.
> 
> Guangya Liu wrote:
> I think that there are bugs here, you can also refer to 
> master_quota_test.cpp , I recalled that I discussed with alex-mesos for this 
> issue and his comments is we should put std first.
> 
> Neil Conway wrote:
> Can you open a JIRA for this? ISTM we aren't consistent right now, and as 
> far as I can tell, the style guide doesn't say anything about the order of 
> `using` directives.
> 
> Guangya Liu wrote:
> Filed a JIRA here https://issues.apache.org/jira/browse/MESOS-4093 Thanks!

We are not consistent (I think we do 50 / 50 now), but we try to become one day 
: ). An argument for putting `std` first is because we do the same for includes.


- Alexander


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


On Dec. 8, 2015, 8:28 a.m., Neil Conway wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/40995/
> ---
> 
> (Updated Dec. 8, 2015, 8:28 a.m.)
> 
> 
> Review request for mesos, Adam B, Alexander Rukletsov, Greg Mann, and Yong 
> Qiao Wang.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added test cases for role behavior.
> 
> 
> Diffs
> -
> 
>   src/Makefile.am e96e0ec41e11acff00fbfb3e86427b48a0625bd2 
>   src/tests/reservation_tests.cpp 3fdf5e121840fe99057e917cca48f1425eff6624 
>   src/tests/role_tests.cpp PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/40995/diff/
> 
> 
> Testing
> ---
> 
> make check.
> 
> 
> Thanks,
> 
> Neil Conway
> 
>

Re: Review Request 38000: Introduced support for user interaction with HTTP AuthenticationRouter.

[Alexander Rojas]

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

(Updated Dec. 8, 2015, 3:38 p.m.)


Review request for mesos, Adam B, Benjamin Hindman, Bernd Mathiske, and Till 
Toenshoff.


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


Repository: mesos


Description
---

Adds functions which allow libprocess users to register HTTP authenticators.
Overloads `ProcesBase::route()` to allow for registering of authenticating 
endpoints.
Includes tests.


Diffs (updated)
-

  3rdparty/libprocess/include/process/event.hpp 
a03824c061c4a0eb865b163999a763635e56744c 
  3rdparty/libprocess/include/process/process.hpp 
81c094414d4d5ac5eb593df2a6d14aaacb19a826 
  3rdparty/libprocess/src/authentication_router.hpp 
5777deafd7420324627802a7ab9da9aaa2b46825 
  3rdparty/libprocess/src/process.cpp e93709d3bb4ac588457bb9331fc05ec5ab539f6d 
  3rdparty/libprocess/src/tests/http_tests.cpp 
2de75ca1c7e224c36b534c368e7379dc158aa5bb 

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


Testing
---

make check


Thanks,

Alexander Rojas

Re: Review Request 39043: Added support for HTTP Authentication in Mesos.

[Alexander Rukletsov]

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



src/master/constants.cpp (line 49)


I though it should be capitlized, according to 
[RFC2617](https://tools.ietf.org/html/rfc2617).



src/master/http.cpp 


Looks like the scope of the `Credential` protobuf is reduced. It looks like 
now it's only used to load credentials on startup and for framework 
authentication. Do we need to store `credentials` in the master then?



src/master/master.cpp (lines 29 - 30)


Blank line, please



src/master/master.cpp (lines 121 - 122)


Blank line, please



src/master/master.cpp (lines 475 - 478)


I'm slightly confused. Now we have two different authenticators: one for 
frameworks and one for endpoints? And they are both called `Authenticator`? Am 
I missing something?



src/master/master.cpp (line 501)


We agreed to use backticks instead of single quotes.



src/master/master.cpp (line 503)


No periods at the end of the log statements. Here and below.



src/master/master.cpp (lines 510 - 511)


We usually insert a blank line if a previous statement spans multiple 
lines. Here and below.



src/master/master.cpp (lines 519 - 551)


For allocator modules, we created a factory method in `allocator.cpp` in 
order to be more concise and keep master code small and clean.


- Alexander Rukletsov


On Dec. 8, 2015, 10:24 a.m., Alexander Rojas wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/39043/
> ---
> 
> (Updated Dec. 8, 2015, 10:24 a.m.)
> 
> 
> Review request for mesos, Adam B, Bernd Mathiske, Isabel Jimenez, and Till 
> Toenshoff.
> 
> 
> Bugs: MESOS-3756
> https://issues.apache.org/jira/browse/MESOS-3756
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> 1. Adds a flag to load an HTTP Authenticator module from the flags.
> 2. If provided, uses the credentials file to initialize the default HTTP 
> Authenticator.
> 3. Updates the existing endpoints which implement their own basic HTTP 
> authenticator with the libprocess one.
> 4. Updates one test which expected the wrong results since now credentials 
> are checked before the body of the request.
> 
> 
> Diffs
> -
> 
>   src/master/constants.hpp cc38dfcf57ecbc8555379084acbf2bfa4b3fc759 
>   src/master/constants.cpp 98ea7c8f1e7c3057cfe941f82f1b71f7731a8f32 
>   src/master/flags.hpp 9af6c68eef6bcf39d5776809fab6c66dc95da6b2 
>   src/master/flags.cpp f9223edb33483cb5619e7dd75221f735306462c6 
>   src/master/http.cpp 9097eda47558fb5306af5a61b464937d7ab34e83 
>   src/master/master.hpp 4683fa542a740f9a0b80fff7fbe0e63ec66266f2 
>   src/master/master.cpp 953fa4f14929581b226a7e27d30aea7a5aa1fd7c 
> 
> Diff: https://reviews.apache.org/r/39043/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Alexander Rojas
> 
>

Re: Review Request 41078: Fixed tests to call socket accept before sending response.

[Mesos ReviewBot]

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


Patch looks great!

Reviews applied: [40872, 40873, 41078]

Passed command: export OS=ubuntu:14.04;export CONFIGURATION="--verbose";export 
COMPILER=gcc; ./support/docker_build.sh

- Mesos ReviewBot


On Dec. 8, 2015, 7:03 a.m., Jojy Varghese wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/41078/
> ---
> 
> (Updated Dec. 8, 2015, 7:03 a.m.)
> 
> 
> Review request for mesos and Timothy Chen.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> By calling accept before sending a response, the server would be ready for
> client's next request.
> 
> 
> Diffs
> -
> 
>   src/tests/containerizer/provisioner_docker_tests.cpp 
> c63bf53fee40ef12536a16e11f4d5224c4e4278e 
> 
> Diff: https://reviews.apache.org/r/41078/diff/
> 
> 
> Testing
> ---
> 
> make check.
> 
> 
> Thanks,
> 
> Jojy Varghese
> 
>

Re: Review Request 39043: Added support for HTTP Authentication in Mesos.

[Alexander Rojas]

> On Dec. 8, 2015, 3:07 p.m., Alexander Rukletsov wrote:
> > src/master/master.cpp, lines 475-478
> > 
> >
> > I'm slightly confused. Now we have two different authenticators: one 
> > for frameworks and one for endpoints? And they are both called 
> > `Authenticator`? Am I missing something?

Well, the old authenticator which is not even secure, will be deprecated, but 
we cannot just delete it, can we? moreover, the new authenticator exists at 
libprocess layer of abstraction.

In fact, the `Credentials` protobuf predates the use of 
`Master::Http::authenticate()`


> On Dec. 8, 2015, 3:07 p.m., Alexander Rukletsov wrote:
> > src/master/constants.cpp, line 49
> > 
> >
> > I though it should be capitlized, according to 
> > [RFC2617](https://tools.ietf.org/html/rfc2617).

There was a good reason to do so, but right now I don't remember why.


> On Dec. 8, 2015, 3:07 p.m., Alexander Rukletsov wrote:
> > src/master/http.cpp, lines 2276-2282
> > 
> >
> > Looks like the scope of the `Credential` protobuf is reduced. It looks 
> > like now it's only used to load credentials on startup and for framework 
> > authentication. Do we need to store `credentials` in the master then?

I isn't reduced, It hasn't change at all. And as far as I'm concerned, as long 
as framework/slave authentication uses the procedure we have, it needs to stay 
there. In fact the method `authenticate` isn't very old at all. I think the 
first implementation of these patches actually predates the inclusion of that 
method.


> On Dec. 8, 2015, 3:07 p.m., Alexander Rukletsov wrote:
> > src/master/master.cpp, lines 519-551
> > 
> >
> > For allocator modules, we created a factory method in `allocator.cpp` 
> > in order to be more concise and keep master code small and clean.

You are right, but the rationale behind it is that the other authenticator's 
initialization code is already here, so one can notice the difference between 
them both while having them here. Once this lands I will open a ticket to move 
them both to their own factories.


> On Dec. 8, 2015, 3:07 p.m., Alexander Rukletsov wrote:
> > src/master/master.cpp, line 501
> > 
> >
> > We agreed to use backticks instead of single quotes.

Here is when you realized how long has this code waited, that it actually 
predates that rule.


- Alexander


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


On Dec. 8, 2015, 11:24 a.m., Alexander Rojas wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/39043/
> ---
> 
> (Updated Dec. 8, 2015, 11:24 a.m.)
> 
> 
> Review request for mesos, Adam B, Bernd Mathiske, Isabel Jimenez, and Till 
> Toenshoff.
> 
> 
> Bugs: MESOS-3756
> https://issues.apache.org/jira/browse/MESOS-3756
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> 1. Adds a flag to load an HTTP Authenticator module from the flags.
> 2. If provided, uses the credentials file to initialize the default HTTP 
> Authenticator.
> 3. Updates the existing endpoints which implement their own basic HTTP 
> authenticator with the libprocess one.
> 4. Updates one test which expected the wrong results since now credentials 
> are checked before the body of the request.
> 
> 
> Diffs
> -
> 
>   src/master/constants.hpp cc38dfcf57ecbc8555379084acbf2bfa4b3fc759 
>   src/master/constants.cpp 98ea7c8f1e7c3057cfe941f82f1b71f7731a8f32 
>   src/master/flags.hpp 9af6c68eef6bcf39d5776809fab6c66dc95da6b2 
>   src/master/flags.cpp f9223edb33483cb5619e7dd75221f735306462c6 
>   src/master/http.cpp 9097eda47558fb5306af5a61b464937d7ab34e83 
>   src/master/master.hpp 4683fa542a740f9a0b80fff7fbe0e63ec66266f2 
>   src/master/master.cpp 953fa4f14929581b226a7e27d30aea7a5aa1fd7c 
> 
> Diff: https://reviews.apache.org/r/39043/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Alexander Rojas
> 
>

Re: Review Request 41075: Added support for implicit roles.

[Alexander Rukletsov]

> On Dec. 8, 2015, 7:02 a.m., Yong Qiao Wang wrote:
> > include/mesos/master/allocator.hpp, line 101
> > 
> >
> > When allocator initialize, their should be no active frameworks in 
> > mesos, can we consider to remove this parmeter from initialize()? and can 
> > initialize role-related information in addFramework().
> 
> Neil Conway wrote:
> Hmmm -- I think it is better as written. Right now, weights are static 
> and set at initialization-time, so I think it makes more sense for them to be 
> parameters of `initialize`, not `addFramework`. (When we do dynamic weights 
> we'll need to reconsider this, but even then, I don't think passing weights 
> to `addFramework` will be the right API).
> 
> Yong Qiao Wang wrote:
> For long term solution in Implicit Roles, --roles and --weights should be 
> removed, so weights parameter will only have one default value(*,1). so it is 
> make sence to remove this parameter from initialize (),and when framework 
> register, we can add its role and the corresponding weight into allocator, 
> then they can become active.
> 
> Adam B wrote:
> Even if we eliminate --weights, we'll be persisting the weights map to 
> the registry, and on failover to a new master or on a new master's startup, 
> it will recover the weights from the registry. It seems reasonable to me that 
> after recovering the weights, the master could start the allocator with the 
> newly recovered weights, rather than have to add them after allocator 
> initialization.
> 
> Neil Conway wrote:
> Note also that a role's weight is required even _before_ `addFramework`, 
> because the weight of a role with quota set is relevant even if the role has 
> no registered frameworks. So I think specifying the weight in `addFramework` 
> isn't right. We could consider replacing the `initialize` argument with a 
> separate `updateWeight` allocator API call, but I'm inclined to defer that 
> until the dynamic weight work is actually done.
> 
> Qian Zhang wrote:
> Neil, can you please clarify a little more about why the weight of a role 
> with quota set is relevant even if the role has no registered frameworks?

My 2¢: weight is the property of a role, not a frameworks, hence I don't think 
we should put it into `addFramework()`. `updateWeight(const string& role, 
double weight)` will be the right thing to do *once* we introduce dynamic 
weights.


- Alexander


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


On Dec. 8, 2015, 8:33 a.m., Neil Conway wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/41075/
> ---
> 
> (Updated Dec. 8, 2015, 8:33 a.m.)
> 
> 
> Review request for mesos, Adam B, Alexander Rukletsov, Benjamin Hindman, Greg 
> Mann, and Yong Qiao Wang.
> 
> 
> Bugs: MESOS-4085
> https://issues.apache.org/jira/browse/MESOS-4085
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Changed the behavior of the master when the `--roles` flag is NOT
> specified. Previously, this would allow only the `*` role to be used. Now,
> omitting `--roles` means that any role can be used. This is called "implicit
> roles". Configuring which principals can perform operations as which roles
> should be done using ACLs in the authorization system.
> 
> Note that this changes the behavior of the system when `--roles` is not
> specified. This is likely acceptable: if the operator didn't specify `--roles`
> in prior versions of Mesos, they were likely not using roles or authorization 
> at
> that time.
> 
> Another minor behavioral change is that the "/roles" endpoint will now only
> return results for currently "active" roles (those with one or more registered
> frameworks).
> 
> The `--roles` flag is now considered deprecated and will be removed in a 
> future
> version of Mesos.
> 
> 
> Diffs
> -
> 
>   include/mesos/master/allocator.hpp 619ef01c3a7d640560653cfc1838dd09046d1da0 
>   include/mesos/master/allocator.proto 
> 702f56f56c3b1331613cecf26522986f6b572f8c 
>   src/master/allocator/mesos/allocator.hpp 
> 97ee80726ad155917811265a983258b0165d3451 
>   src/master/allocator/mesos/hierarchical.hpp 
> 99c742906874c30c39c159e58a65277ade3c07fd 
>   src/master/allocator/mesos/hierarchical.cpp 
> 5da825a1d578a9ee40b4985378fddb3c5fb3b416 
>   src/master/http.cpp 9097eda47558fb5306af5a61b464937d7ab34e83 
>   src/master/master.hpp 4683fa542a740f9a0b80fff7fbe0e63ec66266f2 
>   src/master/master.cpp 953fa4f14929581b226a7e27d30aea7a5aa1fd7c 
>   src/master/quota_handler.cpp b209da42ace752953686eeda9577007a33556d5d 
>   src/tests/allocator.hpp 

Re: Review Request 40999: Fixed flakiness in ReservationTest.ACLMultipleOperations.

[Greg Mann]

> On Dec. 5, 2015, 12:54 a.m., Joseph Wu wrote:
> > src/tests/reservation_tests.cpp, lines 1663-1664
> > 
> >
> > You can be more explicit in your expectations (about batch allocations) 
> > by using `Clock::advance(flags.allocation_interval)`.
> > 
> > Using the above will ensure that only one allocation cycle can be 
> > triggered.

Great suggestion, thanks Joseph! :-)


- Greg


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


On Dec. 8, 2015, 4:57 p.m., Greg Mann wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/40999/
> ---
> 
> (Updated Dec. 8, 2015, 4:57 p.m.)
> 
> 
> Review request for mesos, Jie Yu and Michael Park.
> 
> 
> Bugs: MESOS-4067
> https://issues.apache.org/jira/browse/MESOS-4067
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Fixed flakiness in ReservationTest.ACLMultipleOperations.
> 
> 
> Diffs
> -
> 
>   src/tests/reservation_tests.cpp eccbb8f8a02b65b26f34e020e736afe0445a6d0d 
> 
> Diff: https://reviews.apache.org/r/40999/diff/
> 
> 
> Testing
> ---
> 
> `GTEST_FILTER="ReservationTest.ACLMultipleOperations" bin/mesos-tests.sh 
> --gtest_repeat=2000 --gtest_break_on_failure`
> 
> To reproduce the original test failure, you may have to set 
> `allocation_interval` to a shorter period (5ms, for example).
> 
> 
> Thanks,
> 
> Greg Mann
> 
>

Re: Review Request 41042: Added description of the LoadQoSController in the oversubscription.md

[Mesos ReviewBot]

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


Patch looks great!

Reviews applied: [40617, 41042]

Passed command: export OS=ubuntu:14.04;export CONFIGURATION="--verbose";export 
COMPILER=gcc; ./support/docker_build.sh

- Mesos ReviewBot


On Dec. 8, 2015, 1:24 p.m., Bartek Plotka wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/41042/
> ---
> 
> (Updated Dec. 8, 2015, 1:24 p.m.)
> 
> 
> Review request for mesos, Niklas Nielsen and Vinod Kone.
> 
> 
> Bugs: MESOS-4076
> https://issues.apache.org/jira/browse/MESOS-4076
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added description to _Writing a custom QoS controller_ and _Configuring  
> oversubscription_ sections.
> 
> 
> Diffs
> -
> 
>   docs/oversubscription.md 7d1415a712f818f7664ed8322ddcdc57d3a1fb1f 
> 
> Diff: https://reviews.apache.org/r/41042/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Bartek Plotka
> 
>

Review Request 41090: Second iteration of changes for cmake build on linux.

[Diana Arroyo]

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

Review request for mesos, Artem Harutyunyan, Alex Clemmer, Joris Van 
Remoortere, and Joseph Wu.


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


Repository: mesos


Description
---

Second iteration of changes for cmake build on linux.


Diffs
-

  src/slave/cmake/FindCurl.cmake PRE-CREATION 

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


Testing
---

Tested on Ubuntu and OSX.


Thanks,

Diana Arroyo

Re: Review Request 41076: Added tests for implicit roles.

[Mesos ReviewBot]

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


Patch looks great!

Reviews applied: [40995, 41075, 41076]

Passed command: export OS=ubuntu:14.04;export CONFIGURATION="--verbose";export 
COMPILER=gcc; ./support/docker_build.sh

- Mesos ReviewBot


On Dec. 8, 2015, 8:28 a.m., Neil Conway wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/41076/
> ---
> 
> (Updated Dec. 8, 2015, 8:28 a.m.)
> 
> 
> Review request for mesos, Adam B, Alexander Rukletsov, Benjamin Hindman, Greg 
> Mann, and Yong Qiao Wang.
> 
> 
> Bugs: MESOS-4085
> https://issues.apache.org/jira/browse/MESOS-4085
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Also added tests for the "/role" HTTP endpoint.
> 
> 
> Diffs
> -
> 
>   src/tests/role_tests.cpp PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/41076/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Neil Conway
> 
>

Re: Review Request 40999: Fixed flakiness in ReservationTest.ACLMultipleOperations.

[Greg Mann]

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

(Updated Dec. 8, 2015, 4:57 p.m.)


Review request for mesos, Jie Yu and Michael Park.


Changes
---

Control clock manually.


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


Repository: mesos


Description
---

Fixed flakiness in ReservationTest.ACLMultipleOperations.


Diffs (updated)
-

  src/tests/reservation_tests.cpp eccbb8f8a02b65b26f34e020e736afe0445a6d0d 

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


Testing (updated)
---

`GTEST_FILTER="ReservationTest.ACLMultipleOperations" bin/mesos-tests.sh 
--gtest_repeat=2000 --gtest_break_on_failure`

To reproduce the original test failure, you may have to set 
`allocation_interval` to a shorter period (5ms, for example).


Thanks,

Greg Mann

Re: Review Request 39043: Added support for HTTP Authentication in Mesos.

[Alexander Rojas]

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

(Updated Dec. 8, 2015, 4:53 p.m.)


Review request for mesos, Adam B, Bernd Mathiske, Isabel Jimenez, and Till 
Toenshoff.


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


Repository: mesos


Description
---

1. Adds a flag to load an HTTP Authenticator module from the flags.
2. If provided, uses the credentials file to initialize the default HTTP 
Authenticator.
3. Updates the existing endpoints which implement their own basic HTTP 
authenticator with the libprocess one.
4. Updates one test which expected the wrong results since now credentials are 
checked before the body of the request.


Diffs (updated)
-

  src/master/constants.hpp cc38dfcf57ecbc8555379084acbf2bfa4b3fc759 
  src/master/constants.cpp 98ea7c8f1e7c3057cfe941f82f1b71f7731a8f32 
  src/master/flags.hpp 9af6c68eef6bcf39d5776809fab6c66dc95da6b2 
  src/master/flags.cpp f9223edb33483cb5619e7dd75221f735306462c6 
  src/master/http.cpp 9097eda47558fb5306af5a61b464937d7ab34e83 
  src/master/master.hpp 4683fa542a740f9a0b80fff7fbe0e63ec66266f2 
  src/master/master.cpp 953fa4f14929581b226a7e27d30aea7a5aa1fd7c 

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


Testing
---

make check


Thanks,

Alexander Rojas

Re: Review Request 39043: Added support for HTTP Authentication in Mesos.

[Alexander Rukletsov]

> On Dec. 8, 2015, 2:07 p.m., Alexander Rukletsov wrote:
> > src/master/http.cpp, lines 2276-2282
> > 
> >
> > Looks like the scope of the `Credential` protobuf is reduced. It looks 
> > like now it's only used to load credentials on startup and for framework 
> > authentication. Do we need to store `credentials` in the master then?
> 
> Alexander Rojas wrote:
> I isn't reduced, It hasn't change at all. And as far as I'm concerned, as 
> long as framework/slave authentication uses the procedure we have, it needs 
> to stay there. In fact the method `authenticate` isn't very old at all. I 
> think the first implementation of these patches actually predates the 
> inclusion of that method.

It looks like `Optional credentials` has been introduced in 
https://github.com/apache/mesos/commit/bb8375975e92ee722befb478ddc3b2541d1ccaa9 
and factored out in 
https://github.com/apache/mesos/commit/a5cc9b435aad080a79230f0366a6ce77116c95a4.
 I think we do not need `master->credentials` any longer.


- Alexander


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


On Dec. 8, 2015, 2:38 p.m., Alexander Rojas wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/39043/
> ---
> 
> (Updated Dec. 8, 2015, 2:38 p.m.)
> 
> 
> Review request for mesos, Adam B, Bernd Mathiske, Isabel Jimenez, and Till 
> Toenshoff.
> 
> 
> Bugs: MESOS-3756
> https://issues.apache.org/jira/browse/MESOS-3756
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> 1. Adds a flag to load an HTTP Authenticator module from the flags.
> 2. If provided, uses the credentials file to initialize the default HTTP 
> Authenticator.
> 3. Updates the existing endpoints which implement their own basic HTTP 
> authenticator with the libprocess one.
> 4. Updates one test which expected the wrong results since now credentials 
> are checked before the body of the request.
> 
> 
> Diffs
> -
> 
>   src/master/constants.hpp cc38dfcf57ecbc8555379084acbf2bfa4b3fc759 
>   src/master/constants.cpp 98ea7c8f1e7c3057cfe941f82f1b71f7731a8f32 
>   src/master/flags.hpp 9af6c68eef6bcf39d5776809fab6c66dc95da6b2 
>   src/master/flags.cpp f9223edb33483cb5619e7dd75221f735306462c6 
>   src/master/http.cpp 9097eda47558fb5306af5a61b464937d7ab34e83 
>   src/master/master.hpp 4683fa542a740f9a0b80fff7fbe0e63ec66266f2 
>   src/master/master.cpp 953fa4f14929581b226a7e27d30aea7a5aa1fd7c 
> 
> Diff: https://reviews.apache.org/r/39043/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Alexander Rojas
> 
>

Re: Review Request 40951: Initial set of source files missing for cmake agent binary.

[Diana Arroyo]

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

(Updated Dec. 8, 2015, 3:59 p.m.)


Review request for mesos, Artem Harutyunyan, Alex Clemmer, Joris Van 
Remoortere, and Joseph Wu.


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


Repository: mesos


Description
---

Initial set of source files missing for cmake agent binary.


Diffs (updated)
-

  src/CMakeLists.txt c0d77c745eb5b12dd6d9d7afaba7e820f8d848ef 

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


Testing
---

Tested to make sure library builds successfully.


Thanks,

Diana Arroyo

Re: Review Request 41075: Added support for implicit roles.

[Alexander Rukletsov]

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



src/master/allocator/mesos/hierarchical.hpp (line 367)


s/Frameworks/Roles?



src/master/allocator/mesos/hierarchical.cpp (lines 213 - 215)


We would like to keep all `CHECK*` blocks in the beginning of the function 
for clarity. Now after you removed several checks, let's swap `const string& 
role = frameworkInfo.role();` and `CHECK(initialized);`



src/master/http.cpp (lines 1543 - 1545)


Why don't we consider roles without frameworks but with a non-default 
weight active? Or roles with quota but without frameworks?

Or if you would like to reserve the term "active" for roles with 
frameworks, how about expanding the terminology to something like:
```
Visible = {active} U {weighted} U {quota'ed} U {with-dyn-res}
```

Anyway, my point is that we should show all "visible" roles here.



src/master/master.hpp (lines 1332 - 1333)


Do you want to add a `TODO` here to remove this hashtable once the 
deprecation cycle is over?



src/master/master.cpp (lines 5529 - 5531)


Why don't we check `framework->info.role()` is valid?



src/master/master.cpp (lines 5817 - 5818)


Fits one line.


- Alexander Rukletsov


On Dec. 8, 2015, 8:33 a.m., Neil Conway wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/41075/
> ---
> 
> (Updated Dec. 8, 2015, 8:33 a.m.)
> 
> 
> Review request for mesos, Adam B, Alexander Rukletsov, Benjamin Hindman, Greg 
> Mann, and Yong Qiao Wang.
> 
> 
> Bugs: MESOS-4085
> https://issues.apache.org/jira/browse/MESOS-4085
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Changed the behavior of the master when the `--roles` flag is NOT
> specified. Previously, this would allow only the `*` role to be used. Now,
> omitting `--roles` means that any role can be used. This is called "implicit
> roles". Configuring which principals can perform operations as which roles
> should be done using ACLs in the authorization system.
> 
> Note that this changes the behavior of the system when `--roles` is not
> specified. This is likely acceptable: if the operator didn't specify `--roles`
> in prior versions of Mesos, they were likely not using roles or authorization 
> at
> that time.
> 
> Another minor behavioral change is that the "/roles" endpoint will now only
> return results for currently "active" roles (those with one or more registered
> frameworks).
> 
> The `--roles` flag is now considered deprecated and will be removed in a 
> future
> version of Mesos.
> 
> 
> Diffs
> -
> 
>   include/mesos/master/allocator.hpp 619ef01c3a7d640560653cfc1838dd09046d1da0 
>   include/mesos/master/allocator.proto 
> 702f56f56c3b1331613cecf26522986f6b572f8c 
>   src/master/allocator/mesos/allocator.hpp 
> 97ee80726ad155917811265a983258b0165d3451 
>   src/master/allocator/mesos/hierarchical.hpp 
> 99c742906874c30c39c159e58a65277ade3c07fd 
>   src/master/allocator/mesos/hierarchical.cpp 
> 5da825a1d578a9ee40b4985378fddb3c5fb3b416 
>   src/master/http.cpp 9097eda47558fb5306af5a61b464937d7ab34e83 
>   src/master/master.hpp 4683fa542a740f9a0b80fff7fbe0e63ec66266f2 
>   src/master/master.cpp 953fa4f14929581b226a7e27d30aea7a5aa1fd7c 
>   src/master/quota_handler.cpp b209da42ace752953686eeda9577007a33556d5d 
>   src/tests/allocator.hpp c7670525765491fe931a4ee38446fa7e9d79af42 
>   src/tests/hierarchical_allocator_tests.cpp 
> fb214a829a57529d3f5c49730ae9733f53e622ca 
> 
> Diff: https://reviews.apache.org/r/41075/diff/
> 
> 
> Testing
> ---
> 
> "make check" on OSX 10.10 and Ubuntu 15.10; `--gtest_repeat=1000` for the 
> more likely role-related tests.
> 
> TODOs:
> 
> * Update documentation
> * Add tests for allocation behavior for weights + implicit roles
> * Add tests for quota + implicit roles?
> 
> Notes:
> 
> * There's two places where we use manual `new`/`delete` where a `unique_ptr` 
> would probably be nicer. I'm inclined to leave this as-is for now though 
> (making use of unique_ptr is a broader issue).
> 
> 
> Thanks,
> 
> Neil Conway
> 
>

Re: Review Request 41001: Improved 'ReservationTest.ACLMultipleOperations'.

[Greg Mann]

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

(Updated Dec. 8, 2015, 5:14 p.m.)


Review request for mesos, Jie Yu and Michael Park.


Changes
---

Rebase.


Repository: mesos


Description
---

Improved 'ReservationTest.ACLMultipleOperations'.


Diffs (updated)
-

  src/tests/reservation_tests.cpp eccbb8f8a02b65b26f34e020e736afe0445a6d0d 

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


Testing (updated)
---

`GTEST_FILTER="ReservationTest.ACLMultipleOperations" bin/mesos-tests.sh 
--gtest_repeat=1000 --gtest_break_on_failure`

This test was originally written to test the interaction of multiple 
RESERVE/UNRESERVE/LAUNCH operations in a single `acceptOffers()` call when 
authorization is enabled. In order to probe the effect of authorization on this 
interaction, some operations should fail due to failed authorization. However, 
this test included operations that failed simply because they were malformed. I 
altered the test so that now operations will fail due to failed authorization, 
which tests the desired functionality more precisely.


Thanks,

Greg Mann

Re: Review Request 41075: Added support for implicit roles.

[Neil Conway]

> On Dec. 8, 2015, noon, Yong Qiao Wang wrote:
> > src/master/allocator/mesos/hierarchical.cpp, line 320
> > 
> >
> > In the current implementation, quota(quotaSorter) does not been removed 
> > after the corresponding role is removed, then some resources will still be 
> > reserved by this non-active role, this will result in the waste of 
> > resources. If you rely on cluster operator to remove this quota, I think it 
> > is does not make sence and have a bad user experience, because role is 
> > implicitly removed when the last framework gone without notify the clsuter 
> > admin.

Roles are never "removed" -- just because we no longer track the role in 
`activeRoles` does not mean that the cluster admin needs to be notified.

Re: wasting resources, my understanding of the quota design is that roles with 
a quota defined should still be reserved resources even if they don't have any 
frameworks currently registered. So this is the intended behavior.


- Neil


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


On Dec. 8, 2015, 8:33 a.m., Neil Conway wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/41075/
> ---
> 
> (Updated Dec. 8, 2015, 8:33 a.m.)
> 
> 
> Review request for mesos, Adam B, Alexander Rukletsov, Benjamin Hindman, Greg 
> Mann, and Yong Qiao Wang.
> 
> 
> Bugs: MESOS-4085
> https://issues.apache.org/jira/browse/MESOS-4085
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Changed the behavior of the master when the `--roles` flag is NOT
> specified. Previously, this would allow only the `*` role to be used. Now,
> omitting `--roles` means that any role can be used. This is called "implicit
> roles". Configuring which principals can perform operations as which roles
> should be done using ACLs in the authorization system.
> 
> Note that this changes the behavior of the system when `--roles` is not
> specified. This is likely acceptable: if the operator didn't specify `--roles`
> in prior versions of Mesos, they were likely not using roles or authorization 
> at
> that time.
> 
> Another minor behavioral change is that the "/roles" endpoint will now only
> return results for currently "active" roles (those with one or more registered
> frameworks).
> 
> The `--roles` flag is now considered deprecated and will be removed in a 
> future
> version of Mesos.
> 
> 
> Diffs
> -
> 
>   include/mesos/master/allocator.hpp 619ef01c3a7d640560653cfc1838dd09046d1da0 
>   include/mesos/master/allocator.proto 
> 702f56f56c3b1331613cecf26522986f6b572f8c 
>   src/master/allocator/mesos/allocator.hpp 
> 97ee80726ad155917811265a983258b0165d3451 
>   src/master/allocator/mesos/hierarchical.hpp 
> 99c742906874c30c39c159e58a65277ade3c07fd 
>   src/master/allocator/mesos/hierarchical.cpp 
> 5da825a1d578a9ee40b4985378fddb3c5fb3b416 
>   src/master/http.cpp 9097eda47558fb5306af5a61b464937d7ab34e83 
>   src/master/master.hpp 4683fa542a740f9a0b80fff7fbe0e63ec66266f2 
>   src/master/master.cpp 953fa4f14929581b226a7e27d30aea7a5aa1fd7c 
>   src/master/quota_handler.cpp b209da42ace752953686eeda9577007a33556d5d 
>   src/tests/allocator.hpp c7670525765491fe931a4ee38446fa7e9d79af42 
>   src/tests/hierarchical_allocator_tests.cpp 
> fb214a829a57529d3f5c49730ae9733f53e622ca 
> 
> Diff: https://reviews.apache.org/r/41075/diff/
> 
> 
> Testing
> ---
> 
> "make check" on OSX 10.10 and Ubuntu 15.10; `--gtest_repeat=1000` for the 
> more likely role-related tests.
> 
> TODOs:
> 
> * Update documentation
> * Add tests for allocation behavior for weights + implicit roles
> * Add tests for quota + implicit roles?
> 
> Notes:
> 
> * There's two places where we use manual `new`/`delete` where a `unique_ptr` 
> would probably be nicer. I'm inclined to leave this as-is for now though 
> (making use of unique_ptr is a broader issue).
> 
> 
> Thanks,
> 
> Neil Conway
> 
>

Re: Review Request 41075: Added support for implicit roles.

[Anand Mazumdar]

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


Would it be a good idea to split this review into smaller bits for ease of 
reviewing ? It would make it easier to commit the individual sub system 
component changes.

One possible split can be:

1. Removal of `RoleInfo` protobuf.
2. Changes to Allocator + Tests. (`src/master/allocator/*`)
3. Changes to Master (`src/master/master.cpp`)
4. Changes to quota related code to handle Implicit Roles. 
(`src/master/quota_handler.cpp`)
5. Changes to HTTP handler code `/roles` on master (`src/master/http.cpp`)

What do you think ?

- Anand Mazumdar


On Dec. 8, 2015, 8:33 a.m., Neil Conway wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/41075/
> ---
> 
> (Updated Dec. 8, 2015, 8:33 a.m.)
> 
> 
> Review request for mesos, Adam B, Alexander Rukletsov, Benjamin Hindman, Greg 
> Mann, and Yong Qiao Wang.
> 
> 
> Bugs: MESOS-4085
> https://issues.apache.org/jira/browse/MESOS-4085
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Changed the behavior of the master when the `--roles` flag is NOT
> specified. Previously, this would allow only the `*` role to be used. Now,
> omitting `--roles` means that any role can be used. This is called "implicit
> roles". Configuring which principals can perform operations as which roles
> should be done using ACLs in the authorization system.
> 
> Note that this changes the behavior of the system when `--roles` is not
> specified. This is likely acceptable: if the operator didn't specify `--roles`
> in prior versions of Mesos, they were likely not using roles or authorization 
> at
> that time.
> 
> Another minor behavioral change is that the "/roles" endpoint will now only
> return results for currently "active" roles (those with one or more registered
> frameworks).
> 
> The `--roles` flag is now considered deprecated and will be removed in a 
> future
> version of Mesos.
> 
> 
> Diffs
> -
> 
>   include/mesos/master/allocator.hpp 619ef01c3a7d640560653cfc1838dd09046d1da0 
>   include/mesos/master/allocator.proto 
> 702f56f56c3b1331613cecf26522986f6b572f8c 
>   src/master/allocator/mesos/allocator.hpp 
> 97ee80726ad155917811265a983258b0165d3451 
>   src/master/allocator/mesos/hierarchical.hpp 
> 99c742906874c30c39c159e58a65277ade3c07fd 
>   src/master/allocator/mesos/hierarchical.cpp 
> 5da825a1d578a9ee40b4985378fddb3c5fb3b416 
>   src/master/http.cpp 9097eda47558fb5306af5a61b464937d7ab34e83 
>   src/master/master.hpp 4683fa542a740f9a0b80fff7fbe0e63ec66266f2 
>   src/master/master.cpp 953fa4f14929581b226a7e27d30aea7a5aa1fd7c 
>   src/master/quota_handler.cpp b209da42ace752953686eeda9577007a33556d5d 
>   src/tests/allocator.hpp c7670525765491fe931a4ee38446fa7e9d79af42 
>   src/tests/hierarchical_allocator_tests.cpp 
> fb214a829a57529d3f5c49730ae9733f53e622ca 
> 
> Diff: https://reviews.apache.org/r/41075/diff/
> 
> 
> Testing
> ---
> 
> "make check" on OSX 10.10 and Ubuntu 15.10; `--gtest_repeat=1000` for the 
> more likely role-related tests.
> 
> TODOs:
> 
> * Update documentation
> * Add tests for allocation behavior for weights + implicit roles
> * Add tests for quota + implicit roles?
> 
> Notes:
> 
> * There's two places where we use manual `new`/`delete` where a `unique_ptr` 
> would probably be nicer. I'm inclined to leave this as-is for now though 
> (making use of unique_ptr is a broader issue).
> 
> 
> Thanks,
> 
> Neil Conway
> 
>

Re: Review Request 41090: Second iteration of changes for cmake build on linux.

[Diana Arroyo]

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

(Updated Dec. 8, 2015, 5:52 p.m.)


Review request for mesos, Artem Harutyunyan, Alex Clemmer, Joris Van 
Remoortere, and Joseph Wu.


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


Repository: mesos


Description
---

Second iteration of changes for cmake build on linux.


Diffs (updated)
-

  src/slave/cmake/FindCurl.cmake PRE-CREATION 

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


Testing
---

Tested on Ubuntu and OSX.


Thanks,

Diana Arroyo

Review Request 41092: Added CMake file for agent executable build.

[Diana Arroyo]

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

Review request for mesos, Artem Harutyunyan, Alex Clemmer, Joris Van 
Remoortere, and Joseph Wu.


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


Repository: mesos


Description
---

Added CMake file for agent executable build.


Diffs
-

  src/slave/CMakeLists.txt PRE-CREATION 

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


Testing
---

Tested on Ubuntu and OSX.


Thanks,

Diana Arroyo

Re: Review Request 39043: Added support for HTTP Authentication in Mesos.

[Mesos ReviewBot]

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


Patch looks great!

Reviews applied: [40539, 37999, 38000, 38094, 38950, 39043]

Passed command: export OS=ubuntu:14.04;export CONFIGURATION="--verbose";export 
COMPILER=gcc; ./support/docker_build.sh

- Mesos ReviewBot


On Dec. 8, 2015, 3:53 p.m., Alexander Rojas wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/39043/
> ---
> 
> (Updated Dec. 8, 2015, 3:53 p.m.)
> 
> 
> Review request for mesos, Adam B, Bernd Mathiske, Isabel Jimenez, and Till 
> Toenshoff.
> 
> 
> Bugs: MESOS-3756
> https://issues.apache.org/jira/browse/MESOS-3756
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> 1. Adds a flag to load an HTTP Authenticator module from the flags.
> 2. If provided, uses the credentials file to initialize the default HTTP 
> Authenticator.
> 3. Updates the existing endpoints which implement their own basic HTTP 
> authenticator with the libprocess one.
> 4. Updates one test which expected the wrong results since now credentials 
> are checked before the body of the request.
> 
> 
> Diffs
> -
> 
>   src/master/constants.hpp cc38dfcf57ecbc8555379084acbf2bfa4b3fc759 
>   src/master/constants.cpp 98ea7c8f1e7c3057cfe941f82f1b71f7731a8f32 
>   src/master/flags.hpp 9af6c68eef6bcf39d5776809fab6c66dc95da6b2 
>   src/master/flags.cpp f9223edb33483cb5619e7dd75221f735306462c6 
>   src/master/http.cpp 9097eda47558fb5306af5a61b464937d7ab34e83 
>   src/master/master.hpp 4683fa542a740f9a0b80fff7fbe0e63ec66266f2 
>   src/master/master.cpp 953fa4f14929581b226a7e27d30aea7a5aa1fd7c 
> 
> Diff: https://reviews.apache.org/r/39043/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Alexander Rojas
> 
>

Re: Review Request 40167: [2/7] Added ACL protobuf messages 'CreateVolume' and 'DestroyVolume'.

[Greg Mann]

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

(Updated Dec. 8, 2015, 5:51 p.m.)


Review request for mesos, Jie Yu, Michael Park, and Neil Conway.


Changes
---

Updated comment.


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


Repository: mesos


Description
---

Added ACL protobuf messages 'CreateVolume' and 'DestroyVolume'.


Diffs (updated)
-

  include/mesos/authorizer/authorizer.proto 
74fcc86d3c92cb3aa27e45b647b1653705b3201c 

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


Testing
---

This is the second in a chain of 7 patches. `make check` was used to test after 
all patches were applied.

Note that this chain of patches touches many of the same files as another chain 
beginning with Review #39985 and ending with Review #39989, which is currently 
in review as well. To avoid conflicts, the beginning of this chain begins on 
top of Review #39989.


Thanks,

Greg Mann

Re: Review Request 41090: Second iteration of changes for cmake build on linux.

[Joseph Wu]

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


Initial review.  I haven't applied/tested this patche yet.
---
I'd recommend re-wording your summary (the title of this review) to reflect the 
contents.  When we look back at the git history, I'm pretty sure the order of 
commits won't matter as much as a concise summary.
Something like: `CMake: Add FindCurl macro`


src/slave/cmake/FindCurl.cmake (line 1)


Nit: Missing a space here.



src/slave/cmake/FindCurl.cmake (lines 18 - 22)


s/IMPORTANT NOTE:/**NOTE:**/

Also, you don't need to block-indent the subsequent lines.



src/slave/cmake/FindCurl.cmake (line 28)


(I'm a bit of a CMake noob.)  Where is this defined?



src/slave/cmake/FindCurl.cmake (line 92)


Where was this variable defined/declared/documented?



src/slave/cmake/FindCurl.cmake (line 103)


Nit: extra space at end.


- Joseph Wu


On Dec. 8, 2015, 9:52 a.m., Diana Arroyo wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/41090/
> ---
> 
> (Updated Dec. 8, 2015, 9:52 a.m.)
> 
> 
> Review request for mesos, Artem Harutyunyan, Alex Clemmer, Joris Van 
> Remoortere, and Joseph Wu.
> 
> 
> Bugs: MESOS-3843
> https://issues.apache.org/jira/browse/MESOS-3843
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Second iteration of changes for cmake build on linux.
> 
> 
> Diffs
> -
> 
>   src/slave/cmake/FindCurl.cmake PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/41090/diff/
> 
> 
> Testing
> ---
> 
> Tested on Ubuntu and OSX.
> 
> 
> Thanks,
> 
> Diana Arroyo
> 
>

Re: Review Request 41090: Second iteration of changes for cmake build on linux.

[Joseph Wu]

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



src/slave/cmake/FindCurl.cmake (line 111)


This patch doesn't apply:
```
2015-12-08 10:25:51 URL:https://reviews.apache.org/r/41090/diff/raw/ 
[4071/4071] -> "41090.patch" [1]
41090.patch:117: new blank line at EOF.
+
warning: 1 line adds whitespace errors.
src/slave/cmake/FindCurl.cmake:111: new blank line at EOF.
```

(You should only leave one newline at the EOF, not two.)


- Joseph Wu


On Dec. 8, 2015, 9:52 a.m., Diana Arroyo wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/41090/
> ---
> 
> (Updated Dec. 8, 2015, 9:52 a.m.)
> 
> 
> Review request for mesos, Artem Harutyunyan, Alex Clemmer, Joris Van 
> Remoortere, and Joseph Wu.
> 
> 
> Bugs: MESOS-3843
> https://issues.apache.org/jira/browse/MESOS-3843
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Second iteration of changes for cmake build on linux.
> 
> 
> Diffs
> -
> 
>   src/slave/cmake/FindCurl.cmake PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/41090/diff/
> 
> 
> Testing
> ---
> 
> Tested on Ubuntu and OSX.
> 
> 
> Thanks,
> 
> Diana Arroyo
> 
>

Re: Review Request 41075: Added support for implicit roles.

[Neil Conway]

> On Dec. 8, 2015, 3:20 p.m., Qian Zhang wrote:
> > src/master/allocator/mesos/hierarchical.hpp, line 407
> > 
> >
> > s/iff/if/

"iff" is what was intended 
(https://en.wikipedia.org/wiki/If_and_only_if#Origin_of_iff).


> On Dec. 8, 2015, 3:20 p.m., Qian Zhang wrote:
> > src/master/allocator/mesos/hierarchical.hpp, line 367
> > 
> >
> > Why "Frameworks are removed from this map"? I think it should be "Role" 
> > removed from this map.

Thanks, fixed.


> On Dec. 8, 2015, 3:20 p.m., Qian Zhang wrote:
> > src/master/master.hpp, line 1331
> > 
> >
> > Do we need to put a TODO here to mention that `validRoles` needs to be 
> > removed once "explicit roles" feature is deprecated? I believe at that 
> > time, we will just rely on ACL.

In my opinion, a separate "TODO" is not needed -- the feature is marked as 
deprecated. When it is removed, all the code required to implement it can be 
removed.


> On Dec. 8, 2015, 3:20 p.m., Qian Zhang wrote:
> > src/master/master.cpp, lines 5842-5843
> > 
> >
> > Suggest to erase the role first, and then delete it, like how we remove 
> > offer in `Master::removeOffer()`

Yeah, I considered doing it this way. Why do you think erasing the role first 
is better? (The `removeOffer` case is not quite analogous, because `delete`-ing 
the offer means `offer->id()` is no longer valid).


- Neil


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


On Dec. 8, 2015, 8:33 a.m., Neil Conway wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/41075/
> ---
> 
> (Updated Dec. 8, 2015, 8:33 a.m.)
> 
> 
> Review request for mesos, Adam B, Alexander Rukletsov, Benjamin Hindman, Greg 
> Mann, and Yong Qiao Wang.
> 
> 
> Bugs: MESOS-4085
> https://issues.apache.org/jira/browse/MESOS-4085
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Changed the behavior of the master when the `--roles` flag is NOT
> specified. Previously, this would allow only the `*` role to be used. Now,
> omitting `--roles` means that any role can be used. This is called "implicit
> roles". Configuring which principals can perform operations as which roles
> should be done using ACLs in the authorization system.
> 
> Note that this changes the behavior of the system when `--roles` is not
> specified. This is likely acceptable: if the operator didn't specify `--roles`
> in prior versions of Mesos, they were likely not using roles or authorization 
> at
> that time.
> 
> Another minor behavioral change is that the "/roles" endpoint will now only
> return results for currently "active" roles (those with one or more registered
> frameworks).
> 
> The `--roles` flag is now considered deprecated and will be removed in a 
> future
> version of Mesos.
> 
> 
> Diffs
> -
> 
>   include/mesos/master/allocator.hpp 619ef01c3a7d640560653cfc1838dd09046d1da0 
>   include/mesos/master/allocator.proto 
> 702f56f56c3b1331613cecf26522986f6b572f8c 
>   src/master/allocator/mesos/allocator.hpp 
> 97ee80726ad155917811265a983258b0165d3451 
>   src/master/allocator/mesos/hierarchical.hpp 
> 99c742906874c30c39c159e58a65277ade3c07fd 
>   src/master/allocator/mesos/hierarchical.cpp 
> 5da825a1d578a9ee40b4985378fddb3c5fb3b416 
>   src/master/http.cpp 9097eda47558fb5306af5a61b464937d7ab34e83 
>   src/master/master.hpp 4683fa542a740f9a0b80fff7fbe0e63ec66266f2 
>   src/master/master.cpp 953fa4f14929581b226a7e27d30aea7a5aa1fd7c 
>   src/master/quota_handler.cpp b209da42ace752953686eeda9577007a33556d5d 
>   src/tests/allocator.hpp c7670525765491fe931a4ee38446fa7e9d79af42 
>   src/tests/hierarchical_allocator_tests.cpp 
> fb214a829a57529d3f5c49730ae9733f53e622ca 
> 
> Diff: https://reviews.apache.org/r/41075/diff/
> 
> 
> Testing
> ---
> 
> "make check" on OSX 10.10 and Ubuntu 15.10; `--gtest_repeat=1000` for the 
> more likely role-related tests.
> 
> TODOs:
> 
> * Update documentation
> * Add tests for allocation behavior for weights + implicit roles
> * Add tests for quota + implicit roles?
> 
> Notes:
> 
> * There's two places where we use manual `new`/`delete` where a `unique_ptr` 
> would probably be nicer. I'm inclined to leave this as-is for now though 
> (making use of unique_ptr is a broader issue).
> 
> 
> Thanks,
> 
> Neil Conway
> 
>

Re: Review Request 41075: Added support for implicit roles.

[Neil Conway]

> On Dec. 8, 2015, 5:26 p.m., Anand Mazumdar wrote:
> > Would it be a good idea to split this review into smaller bits for ease of 
> > reviewing ? It would make it easier to commit the individual sub system 
> > component changes.
> > 
> > One possible split can be:
> > 
> > 1. Removal of `RoleInfo` protobuf.
> > 2. Changes to Allocator + Tests. (`src/master/allocator/*`)
> > 3. Changes to Master (`src/master/master.cpp`)
> > 4. Changes to quota related code to handle Implicit Roles. 
> > (`src/master/quota_handler.cpp`)
> > 5. Changes to HTTP handler code `/roles` on master (`src/master/http.cpp`)
> > 
> > What do you think ?

I'm happy to do this if that's what the reviewers would like. I didn't use a 
fine-grained split like this initially for a few reasons:

* My understanding is that we want each commit/RR to be atomic and 
self-contained -- i.e., each commit should compile and pass the unit tests. If 
that is true, it is hard to disentangle most of these changes into separate 
commits.
* Personally, I find it more difficult to review a chain of commits like 
suggested above than a single, coherent commit that makes all the changes 
necessary to implement a feature. But maybe that's just personal preference.

FWIW, if all you're doing is splitting a large commit into smaller commits 
where each small commit touches a single file, that doesn't seem useful to me 
-- you can just review the changes to each file one at a time in the large 
commit.


- Neil


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


On Dec. 8, 2015, 8:33 a.m., Neil Conway wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/41075/
> ---
> 
> (Updated Dec. 8, 2015, 8:33 a.m.)
> 
> 
> Review request for mesos, Adam B, Alexander Rukletsov, Benjamin Hindman, Greg 
> Mann, and Yong Qiao Wang.
> 
> 
> Bugs: MESOS-4085
> https://issues.apache.org/jira/browse/MESOS-4085
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Changed the behavior of the master when the `--roles` flag is NOT
> specified. Previously, this would allow only the `*` role to be used. Now,
> omitting `--roles` means that any role can be used. This is called "implicit
> roles". Configuring which principals can perform operations as which roles
> should be done using ACLs in the authorization system.
> 
> Note that this changes the behavior of the system when `--roles` is not
> specified. This is likely acceptable: if the operator didn't specify `--roles`
> in prior versions of Mesos, they were likely not using roles or authorization 
> at
> that time.
> 
> Another minor behavioral change is that the "/roles" endpoint will now only
> return results for currently "active" roles (those with one or more registered
> frameworks).
> 
> The `--roles` flag is now considered deprecated and will be removed in a 
> future
> version of Mesos.
> 
> 
> Diffs
> -
> 
>   include/mesos/master/allocator.hpp 619ef01c3a7d640560653cfc1838dd09046d1da0 
>   include/mesos/master/allocator.proto 
> 702f56f56c3b1331613cecf26522986f6b572f8c 
>   src/master/allocator/mesos/allocator.hpp 
> 97ee80726ad155917811265a983258b0165d3451 
>   src/master/allocator/mesos/hierarchical.hpp 
> 99c742906874c30c39c159e58a65277ade3c07fd 
>   src/master/allocator/mesos/hierarchical.cpp 
> 5da825a1d578a9ee40b4985378fddb3c5fb3b416 
>   src/master/http.cpp 9097eda47558fb5306af5a61b464937d7ab34e83 
>   src/master/master.hpp 4683fa542a740f9a0b80fff7fbe0e63ec66266f2 
>   src/master/master.cpp 953fa4f14929581b226a7e27d30aea7a5aa1fd7c 
>   src/master/quota_handler.cpp b209da42ace752953686eeda9577007a33556d5d 
>   src/tests/allocator.hpp c7670525765491fe931a4ee38446fa7e9d79af42 
>   src/tests/hierarchical_allocator_tests.cpp 
> fb214a829a57529d3f5c49730ae9733f53e622ca 
> 
> Diff: https://reviews.apache.org/r/41075/diff/
> 
> 
> Testing
> ---
> 
> "make check" on OSX 10.10 and Ubuntu 15.10; `--gtest_repeat=1000` for the 
> more likely role-related tests.
> 
> TODOs:
> 
> * Update documentation
> * Add tests for allocation behavior for weights + implicit roles
> * Add tests for quota + implicit roles?
> 
> Notes:
> 
> * There's two places where we use manual `new`/`delete` where a `unique_ptr` 
> would probably be nicer. I'm inclined to leave this as-is for now though 
> (making use of unique_ptr is a broader issue).
> 
> 
> Thanks,
> 
> Neil Conway
> 
>

Re: Review Request 40168: [3/7] Added 'CreateVolume' and 'DestroyVolume' ACL support to the authorizer.

[Greg Mann]

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

(Updated Dec. 8, 2015, 6:07 p.m.)


Review request for mesos, Jie Yu, Michael Park, and Neil Conway.


Changes
---

Rebase, updated comment.


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


Repository: mesos


Description
---

Added 'CreateVolume' and 'DestroyVolume' ACL support to the authorizer.


Diffs (updated)
-

  include/mesos/authorizer/authorizer.hpp 
ec3d3e9c9bae461c8cd136f17cfda58e0cbd4e1b 
  src/authorizer/local/authorizer.hpp 965658b53d21734a2dbf3713d02d44d728fd6cca 
  src/authorizer/local/authorizer.cpp 1ac6a41d20454515a3ef2b84e296494a4592b3ea 
  src/tests/authorization_tests.cpp 32beeea3584f093bcac24e0c160235de0e3abb28 
  src/tests/mesos.hpp 2429ac5cbcd9c1a3949c11de94b542108a3c13d8 
  src/tests/mesos.cpp a501f04dfbf9c93c2196e83b512519e925a67c97 

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


Testing (updated)
---

This is the third in a chain of 7 patches. New tests were added to 
`authorization_tests.cpp` in order to test the authorizer's handling of both 
successful and failed authorization attempts using `ACL::CreateVolume` and 
`ACL::DestroyVolume`. `make check` was used to test after all patches were 
applied.


Thanks,

Greg Mann

Re: Review Request 39781: Update ModuleTest to not assume dynamic dlopen search.

[James Peach]

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

(Updated Dec. 8, 2015, 6:52 p.m.)


Review request for mesos, Benjamin Bannier, Kapil Arya, Niklas Nielsen, and 
Till Toenshoff.


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


Repository: mesos


Description
---

Update ModuleTest to not assume dynamic dlopen search.


Diffs (updated)
-

  src/tests/module_tests.cpp a3271a3267647e0964dd3decb3ca8384417dd559 

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


Testing
---

make check


Thanks,

James Peach

Re: Review Request 40255: [5/7] Added framework authorization for persistent volumes.

[Greg Mann]

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

(Updated Dec. 8, 2015, 7:03 p.m.)


Review request for mesos, Jie Yu, Michael Park, and Neil Conway.


Changes
---

Added manual clock control to tests.


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


Repository: mesos


Description
---

Added framework authorization for persistent volumes.


Diffs (updated)
-

  src/master/master.cpp 953fa4f14929581b226a7e27d30aea7a5aa1fd7c 
  src/tests/persistent_volume_tests.cpp 
01b3c13751a5558d5f06edb8f650c8644dc54486 

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


Testing (updated)
---

This is the fifth in a chain of 7 patches. New tests were added to 
`persistent_volume_tests.cpp` in order to test a framework attempting both 
successful and failed authorizations for `CREATE` and `DESTROY` offer 
operations. `make check` was used to test after all patches were applied.


Thanks,

Greg Mann

Re: Review Request 40256: [6/7] Fixed handling of multiple offer operations in PersistentVolumeTest.SendingCheckpointResourcesMessage.

[Greg Mann]

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

(Updated Dec. 8, 2015, 7:24 p.m.)


Review request for mesos, Jie Yu, Michael Park, and Neil Conway.


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


Repository: mesos


Description
---

Fixed handling of multiple offer operations in 
PersistentVolumeTest.SendingCheckpointResourcesMessage.


Diffs (updated)
-

  src/tests/persistent_volume_tests.cpp 
01b3c13751a5558d5f06edb8f650c8644dc54486 

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


Testing (updated)
---

This is the sixth in a chain of 7 patches. This patch was required in order to 
fix the `PersistentVolumeTest.SendingCheckpointResourcesMessage` test, which 
was broken by the addition of authorization to the `CREATE` and `DESTROY` offer 
operations. The test was previously both creating and destroying a persistent 
volume in a single `acceptOffer` message. However, our `validate` method for 
`DESTROY` offer operations correctly enforces that in order to delete a volume, 
the volume should be present in the checkpointed resources of the relevant 
Agent. This is likely not the case if the `CREATE` and `DESTROY` operations are 
both in the same message.

`make check` was used to test after all patches were applied.


Thanks,

Greg Mann

Review Request 41098: Fixed punctuation usage in some error messages.

[Neil Conway]

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

Review request for mesos and Jie Yu.


Repository: mesos


Description
---

Error messages should generally not end in punctuation.


Diffs
-

  src/master/http.cpp 9097eda47558fb5306af5a61b464937d7ab34e83 
  src/master/validation.cpp 6a0296b17ffbc3b0f7b8b8dca172e143684fde14 
  src/slave/containerizer/mesos/isolators/cgroups/perf_event.cpp 
b7ba00bc495001380f01737e46e8671ffe1c2ef7 

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


Testing
---


Thanks,

Neil Conway

Re: Review Request 41001: Improved 'ReservationTest.ACLMultipleOperations'.

[Mesos ReviewBot]

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


Patch looks great!

Reviews applied: [40999, 41001]

Passed command: export OS=ubuntu:14.04;export CONFIGURATION="--verbose";export 
COMPILER=gcc; ./support/docker_build.sh

- Mesos ReviewBot


On Dec. 8, 2015, 5:14 p.m., Greg Mann wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/41001/
> ---
> 
> (Updated Dec. 8, 2015, 5:14 p.m.)
> 
> 
> Review request for mesos, Jie Yu and Michael Park.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Improved 'ReservationTest.ACLMultipleOperations'.
> 
> 
> Diffs
> -
> 
>   src/tests/reservation_tests.cpp eccbb8f8a02b65b26f34e020e736afe0445a6d0d 
> 
> Diff: https://reviews.apache.org/r/41001/diff/
> 
> 
> Testing
> ---
> 
> `GTEST_FILTER="ReservationTest.ACLMultipleOperations" bin/mesos-tests.sh 
> --gtest_repeat=1000 --gtest_break_on_failure`
> 
> This test was originally written to test the interaction of multiple 
> RESERVE/UNRESERVE/LAUNCH operations in a single `acceptOffers()` call when 
> authorization is enabled. In order to probe the effect of authorization on 
> this interaction, some operations should fail due to failed authorization. 
> However, this test included operations that failed simply because they were 
> malformed. I altered the test so that now operations will fail due to failed 
> authorization, which tests the desired functionality more precisely.
> 
> 
> Thanks,
> 
> Greg Mann
> 
>

Re: Review Request 40169: [4/7] Added 'Master::authorize{Destroy, Create}Volume' to create/destroy persistent volumes.

[Greg Mann]

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

(Updated Dec. 8, 2015, 6:44 p.m.)


Review request for mesos, Jie Yu, Michael Park, and Neil Conway.


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


Repository: mesos


Description
---

Added 'Master::authorize{Destroy,Create}Volume' to create/destroy persistent 
volumes.


Diffs (updated)
-

  src/master/master.hpp 4683fa542a740f9a0b80fff7fbe0e63ec66266f2 
  src/master/master.cpp 953fa4f14929581b226a7e27d30aea7a5aa1fd7c 

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


Testing (updated)
---

This is the fourth in a chain of 7 patches. `make check` was used to test after 
all patches were applied.


Thanks,

Greg Mann

Re: Review Request 41076: Added tests for implicit roles.

[Neil Conway]

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

(Updated Dec. 8, 2015, 7:18 p.m.)


Review request for mesos, Adam B, Alexander Rukletsov, Benjamin Hindman, Greg 
Mann, and Yong Qiao Wang.


Changes
---

Address review comments, rebase.


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


Repository: mesos


Description
---

Also added tests for the "/role" HTTP endpoint.


Diffs (updated)
-

  src/tests/role_tests.cpp PRE-CREATION 

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


Testing
---


Thanks,

Neil Conway

Re: Review Request 41075: Added support for implicit roles.

[Neil Conway]

> On Dec. 8, 2015, 4:45 p.m., Alexander Rukletsov wrote:
> > src/master/allocator/mesos/hierarchical.hpp, line 367
> > 
> >
> > s/Frameworks/Roles?

Thanks, fixed.


> On Dec. 8, 2015, 4:45 p.m., Alexander Rukletsov wrote:
> > src/master/master.hpp, lines 1332-1333
> > 
> >
> > Do you want to add a `TODO` here to remove this hashtable once the 
> > deprecation cycle is over?

I put a TODO (along with a warning) in master.cpp when we parse `--roles`.


> On Dec. 8, 2015, 4:45 p.m., Alexander Rukletsov wrote:
> > src/master/master.cpp, lines 5837-5838
> > 
> >
> > Fits one line.

You mean

```
  CHECK(roles.contains(role))
<< "Unknown role " << role << " of framework " << *framework;
```

I believe this would violate the "anti-jaggedness" suggestion in the style 
guide.


> On Dec. 8, 2015, 4:45 p.m., Alexander Rukletsov wrote:
> > src/master/http.cpp, lines 1543-1545
> > 
> >
> > Why don't we consider roles without frameworks but with a non-default 
> > weight active? Or roles with quota but without frameworks?
> > 
> > Or if you would like to reserve the term "active" for roles with 
> > frameworks, how about expanding the terminology to something like:
> > ```
> > Visible = {active} U {weighted} U {quota'ed} U {with-dyn-res}
> > ```
> > 
> > Anyway, my point is that we should show all "visible" roles here.

Yeah, this is a good point. Do we think that a user wants to see `{active} U 
{weighted} U {quota'ed} U {with-dyn-res}`?

That does seem reasonable to me, although with the way the data structures are 
organized, it will be somewhat ugly to implement :-\


- Neil


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


On Dec. 8, 2015, 8:33 a.m., Neil Conway wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/41075/
> ---
> 
> (Updated Dec. 8, 2015, 8:33 a.m.)
> 
> 
> Review request for mesos, Adam B, Alexander Rukletsov, Benjamin Hindman, Greg 
> Mann, and Yong Qiao Wang.
> 
> 
> Bugs: MESOS-4085
> https://issues.apache.org/jira/browse/MESOS-4085
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Changed the behavior of the master when the `--roles` flag is NOT
> specified. Previously, this would allow only the `*` role to be used. Now,
> omitting `--roles` means that any role can be used. This is called "implicit
> roles". Configuring which principals can perform operations as which roles
> should be done using ACLs in the authorization system.
> 
> Note that this changes the behavior of the system when `--roles` is not
> specified. This is likely acceptable: if the operator didn't specify `--roles`
> in prior versions of Mesos, they were likely not using roles or authorization 
> at
> that time.
> 
> Another minor behavioral change is that the "/roles" endpoint will now only
> return results for currently "active" roles (those with one or more registered
> frameworks).
> 
> The `--roles` flag is now considered deprecated and will be removed in a 
> future
> version of Mesos.
> 
> 
> Diffs
> -
> 
>   include/mesos/master/allocator.hpp 619ef01c3a7d640560653cfc1838dd09046d1da0 
>   include/mesos/master/allocator.proto 
> 702f56f56c3b1331613cecf26522986f6b572f8c 
>   src/master/allocator/mesos/allocator.hpp 
> 97ee80726ad155917811265a983258b0165d3451 
>   src/master/allocator/mesos/hierarchical.hpp 
> 99c742906874c30c39c159e58a65277ade3c07fd 
>   src/master/allocator/mesos/hierarchical.cpp 
> 5da825a1d578a9ee40b4985378fddb3c5fb3b416 
>   src/master/http.cpp 9097eda47558fb5306af5a61b464937d7ab34e83 
>   src/master/master.hpp 4683fa542a740f9a0b80fff7fbe0e63ec66266f2 
>   src/master/master.cpp 953fa4f14929581b226a7e27d30aea7a5aa1fd7c 
>   src/master/quota_handler.cpp b209da42ace752953686eeda9577007a33556d5d 
>   src/tests/allocator.hpp c7670525765491fe931a4ee38446fa7e9d79af42 
>   src/tests/hierarchical_allocator_tests.cpp 
> fb214a829a57529d3f5c49730ae9733f53e622ca 
> 
> Diff: https://reviews.apache.org/r/41075/diff/
> 
> 
> Testing
> ---
> 
> "make check" on OSX 10.10 and Ubuntu 15.10; `--gtest_repeat=1000` for the 
> more likely role-related tests.
> 
> TODOs:
> 
> * Update documentation
> * Add tests for allocation behavior for weights + implicit roles
> * Add tests for quota + implicit roles?
> 
> Notes:
> 
> * There's two places where we use manual `new`/`delete` where a `unique_ptr` 
> 

Re: Review Request 39782: Add a comment for os::libraries::setPaths.

[James Peach]

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

(Updated Dec. 8, 2015, 6:52 p.m.)


Review request for mesos, Benjamin Bannier, Kapil Arya, Niklas Nielsen, and 
Till Toenshoff.


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


Repository: mesos


Description
---

Add a comment for os::libraries::setPaths.


Diffs (updated)
-

  3rdparty/libprocess/3rdparty/stout/include/stout/os.hpp 
03e6f75850561b5eb92da4771fbe18e4057ad520 

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


Testing
---

No code changes.


Thanks,

James Peach

Re: Review Request 40995: Added test cases for role behavior.

[Neil Conway]

> On Dec. 8, 2015, 2:55 p.m., Alexander Rukletsov wrote:
> > One thing we also agreed is to disallow empty string roles. Do you think it 
> > makes sense to extend this patch with a test for that or do it separately?

I think we should do this separately, perhaps as part of fixing MESOS-2210.


> On Dec. 8, 2015, 2:55 p.m., Alexander Rukletsov wrote:
> > src/tests/role_tests.cpp, line 36
> > 
> >
> > I believe we use third person singular in such cases.

Fixed.


> On Dec. 8, 2015, 2:55 p.m., Alexander Rukletsov wrote:
> > src/tests/role_tests.cpp, lines 76-77
> > 
> >
> > I think a comment here expaining why you reduce the allocation interval 
> > will be good for posterity. Alternatively, you can use 
> > `Clock::advance(flags.allocation_interval)` before `AWAIT_READY(offers)` to 
> > achieve the same. Perhaps it's a cleaner solution.

Yeah, it would probably be cleaner to use `advance` instead of reducing the 
allocation interval (although that applies to many other tests as well). I'll 
take a look at doing that.


- Neil


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


On Dec. 8, 2015, 8:28 a.m., Neil Conway wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/40995/
> ---
> 
> (Updated Dec. 8, 2015, 8:28 a.m.)
> 
> 
> Review request for mesos, Adam B, Alexander Rukletsov, Greg Mann, and Yong 
> Qiao Wang.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added test cases for role behavior.
> 
> 
> Diffs
> -
> 
>   src/Makefile.am e96e0ec41e11acff00fbfb3e86427b48a0625bd2 
>   src/tests/reservation_tests.cpp 3fdf5e121840fe99057e917cca48f1425eff6624 
>   src/tests/role_tests.cpp PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/40995/diff/
> 
> 
> Testing
> ---
> 
> make check.
> 
> 
> Thanks,
> 
> Neil Conway
> 
>

Re: Review Request 41075: Added support for implicit roles.

[Neil Conway]

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

(Updated Dec. 8, 2015, 7:18 p.m.)


Review request for mesos, Adam B, Alexander Rukletsov, Benjamin Hindman, Greg 
Mann, and Yong Qiao Wang.


Changes
---

Address review comments, rebase.


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


Repository: mesos


Description
---

Changed the behavior of the master when the `--roles` flag is NOT
specified. Previously, this would allow only the `*` role to be used. Now,
omitting `--roles` means that any role can be used. This is called "implicit
roles". Configuring which principals can perform operations as which roles
should be done using ACLs in the authorization system.

Note that this changes the behavior of the system when `--roles` is not
specified. This is likely acceptable: if the operator didn't specify `--roles`
in prior versions of Mesos, they were likely not using roles or authorization at
that time.

Another minor behavioral change is that the "/roles" endpoint will now only
return results for currently "active" roles (those with one or more registered
frameworks).

The `--roles` flag is now considered deprecated and will be removed in a future
version of Mesos.


Diffs (updated)
-

  include/mesos/master/allocator.hpp 619ef01c3a7d640560653cfc1838dd09046d1da0 
  include/mesos/master/allocator.proto 702f56f56c3b1331613cecf26522986f6b572f8c 
  src/master/allocator/mesos/allocator.hpp 
97ee80726ad155917811265a983258b0165d3451 
  src/master/allocator/mesos/hierarchical.hpp 
99c742906874c30c39c159e58a65277ade3c07fd 
  src/master/allocator/mesos/hierarchical.cpp 
5da825a1d578a9ee40b4985378fddb3c5fb3b416 
  src/master/http.cpp 9097eda47558fb5306af5a61b464937d7ab34e83 
  src/master/master.hpp 4683fa542a740f9a0b80fff7fbe0e63ec66266f2 
  src/master/master.cpp 953fa4f14929581b226a7e27d30aea7a5aa1fd7c 
  src/master/quota_handler.cpp b209da42ace752953686eeda9577007a33556d5d 
  src/tests/allocator.hpp c7670525765491fe931a4ee38446fa7e9d79af42 
  src/tests/hierarchical_allocator_tests.cpp 
fb214a829a57529d3f5c49730ae9733f53e622ca 

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


Testing
---

"make check" on OSX 10.10 and Ubuntu 15.10; `--gtest_repeat=1000` for the more 
likely role-related tests.

TODOs:

* Update documentation
* Add tests for allocation behavior for weights + implicit roles
* Add tests for quota + implicit roles?

Notes:

* There's two places where we use manual `new`/`delete` where a `unique_ptr` 
would probably be nicer. I'm inclined to leave this as-is for now though 
(making use of unique_ptr is a broader issue).


Thanks,

Neil Conway

Re: Review Request 40995: Added test cases for role behavior.

[Neil Conway]

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

(Updated Dec. 8, 2015, 7:18 p.m.)


Review request for mesos, Adam B, Alexander Rukletsov, Greg Mann, and Yong Qiao 
Wang.


Changes
---

Addressed review comments, rebase.


Repository: mesos


Description
---

Added test cases for role behavior.


Diffs (updated)
-

  src/Makefile.am e96e0ec41e11acff00fbfb3e86427b48a0625bd2 
  src/tests/reservation_tests.cpp eccbb8f8a02b65b26f34e020e736afe0445a6d0d 
  src/tests/role_tests.cpp PRE-CREATION 

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


Testing
---

make check.


Thanks,

Neil Conway

Re: Review Request 40271: [7/7] Added documentation for RESERVE, UNRESERVE, CREATE, and DESTROY authorization.

[Greg Mann]

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

(Updated Dec. 8, 2015, 7:31 p.m.)


Review request for mesos, Jie Yu, Michael Park, and Neil Conway.


Changes
---

Addressed comments.


Bugs: MESOS-3062 and MESOS-3065
https://issues.apache.org/jira/browse/MESOS-3062
https://issues.apache.org/jira/browse/MESOS-3065


Repository: mesos


Description
---

Added documentation for RESERVE, UNRESERVE, CREATE, and DESTROY authorization.


Diffs (updated)
-

  docs/authorization.md f5ed75fcd0785fde38058917354fcf6d668dcccb 
  docs/persistent-volume.md cf7a6bbb6044e460293dd66066df87aded0b4fb8 
  docs/reservation.md de447664f10b1d73211805b725f2284b07c609f6 

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


Testing
---

This is the last in a chain of 7 patches.

This documentation was tested by viewing with the Mesos Website Container 
(https://github.com/mesosphere/mesos-website-container).

`make check` was run after all patches in the chain were applied.


Thanks,

Greg Mann

Re: Review Request 40271: [7/7] Added documentation for RESERVE, UNRESERVE, CREATE, and DESTROY authorization.

[Greg Mann]

> On Dec. 2, 2015, 5:19 p.m., Alexander Rukletsov wrote:
> > Do you think it makes sense to update "src/master/flags.cpp" as well?

I assume you're referring to the help string for the `--acls` flag? I think we 
could skip adding to that, since it already includes examples for three 
different types of ACLs.


- Greg


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


On Dec. 8, 2015, 7:31 p.m., Greg Mann wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/40271/
> ---
> 
> (Updated Dec. 8, 2015, 7:31 p.m.)
> 
> 
> Review request for mesos, Jie Yu, Michael Park, and Neil Conway.
> 
> 
> Bugs: MESOS-3062 and MESOS-3065
> https://issues.apache.org/jira/browse/MESOS-3062
> https://issues.apache.org/jira/browse/MESOS-3065
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added documentation for RESERVE, UNRESERVE, CREATE, and DESTROY authorization.
> 
> 
> Diffs
> -
> 
>   docs/authorization.md f5ed75fcd0785fde38058917354fcf6d668dcccb 
>   docs/persistent-volume.md cf7a6bbb6044e460293dd66066df87aded0b4fb8 
>   docs/reservation.md de447664f10b1d73211805b725f2284b07c609f6 
> 
> Diff: https://reviews.apache.org/r/40271/diff/
> 
> 
> Testing
> ---
> 
> This is the last in a chain of 7 patches.
> 
> This documentation was tested by viewing with the Mesos Website Container 
> (https://github.com/mesosphere/mesos-website-container).
> 
> `make check` was run after all patches in the chain were applied.
> 
> 
> Thanks,
> 
> Greg Mann
> 
>

Re: Review Request 41098: Fixed punctuation usage in some error messages.

[Jie Yu]

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

Ship it!


Ship It!

- Jie Yu


On Dec. 8, 2015, 7:29 p.m., Neil Conway wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/41098/
> ---
> 
> (Updated Dec. 8, 2015, 7:29 p.m.)
> 
> 
> Review request for mesos and Jie Yu.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Error messages should generally not end in punctuation.
> 
> 
> Diffs
> -
> 
>   src/master/http.cpp 9097eda47558fb5306af5a61b464937d7ab34e83 
>   src/master/validation.cpp 6a0296b17ffbc3b0f7b8b8dca172e143684fde14 
>   src/slave/containerizer/mesos/isolators/cgroups/perf_event.cpp 
> b7ba00bc495001380f01737e46e8671ffe1c2ef7 
> 
> Diff: https://reviews.apache.org/r/41098/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Neil Conway
> 
>

Re: Review Request 39781: Update ModuleTest to not assume dynamic dlopen search.

[James Peach]

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

(Updated Dec. 8, 2015, 8:58 p.m.)


Review request for mesos, Benjamin Bannier, Kapil Arya, Niklas Nielsen, and 
Till Toenshoff.


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


Repository: mesos


Description
---

Update ModuleTest to not assume dynamic dlopen search.


Diffs (updated)
-

  src/tests/module_tests.cpp a3271a3267647e0964dd3decb3ca8384417dd559 

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


Testing
---

make check


Thanks,

James Peach

Re: Review Request 41100: Fixed a broken test: HealthTest.ObserveEndpoint.

[Jie Yu]

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

Ship it!


Ship It!

- Jie Yu


On Dec. 8, 2015, 8:55 p.m., Neil Conway wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/41100/
> ---
> 
> (Updated Dec. 8, 2015, 8:55 p.m.)
> 
> 
> Review request for mesos and Jie Yu.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Broken by the error message punctuation changes committed recently.
> 
> 
> Diffs
> -
> 
>   src/tests/repair_tests.cpp a3ea8720faf17e6fb20cd62048f1c421eb6fa08b 
> 
> Diff: https://reviews.apache.org/r/41100/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Neil Conway
> 
>

Re: Review Request 39781: Update ModuleTest to not assume dynamic dlopen search.

[James Peach]

> On Nov. 30, 2015, 8:51 a.m., Benjamin Bannier wrote:
> > src/tests/module_tests.cpp, line 123
> > 
> >
> > None of these functions need to be (mutating!) member functions if you 
> > simply inject `libraryDirectory`; please consider converting them to free 
> > functions, e.g. implemented in an anon namespace in this translation unit.

What exactly do you mean by "inject"? Pass the ```libraryDirectory``` as a 
parameter to these free functions? Or use some Google Test or Mock injection 
technique?

I'd prefer not to clutter this change with additional refactorings if that's OK.


- James


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


On Dec. 8, 2015, 6:52 p.m., James Peach wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/39781/
> ---
> 
> (Updated Dec. 8, 2015, 6:52 p.m.)
> 
> 
> Review request for mesos, Benjamin Bannier, Kapil Arya, Niklas Nielsen, and 
> Till Toenshoff.
> 
> 
> Bugs: MESOS-3725
> https://issues.apache.org/jira/browse/MESOS-3725
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Update ModuleTest to not assume dynamic dlopen search.
> 
> 
> Diffs
> -
> 
>   src/tests/module_tests.cpp a3271a3267647e0964dd3decb3ca8384417dd559 
> 
> Diff: https://reviews.apache.org/r/39781/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> James Peach
> 
>

Re: Review Request 38000: Introduced support for user interaction with HTTP AuthenticationRouter.

[Ben Mahler]

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


This is starting to shape up, thanks Alexander. As we discussed offline, just 
wanted to surface the main bits of feedback so you can start addressing them. I 
will follow up with feedback for the tests, wasn't able to do a complete pass 
in one sitting.


3rdparty/libprocess/include/process/process.hpp (lines 65 - 86)


Why isn't this in the http header? It looks like firewall belongs there too 
since it's more aptly namespaced as 'http::Firewall'?



3rdparty/libprocess/include/process/process.hpp (lines 68 - 73)


How about the following to be a bit more succinct?

```
/**
 * Sets (or overwrites) the authenticator for the realm.
 * 
 * Every incoming HTTP request to an endpoint associated
 * with the realm will be authenticated with the given
 * authenticator.
 */
```



3rdparty/libprocess/include/process/process.hpp (lines 79 - 82)


Not sure we need to say "still", how about:

```
/**
 * Unsets the authenticator for the realm.
 * 
 * Any endpoint mapped to the realm will no
 * longer be authenticated.
 */
```



3rdparty/libprocess/include/process/process.hpp (lines 261 - 263)


I'm guessing this is about introducing a type to make this more explicit in 
the signature, not type safety:

```
TODO(arojas): Consider introducing an `authentication::Principal` type.
```

Similarly, we probably need a TODO on top of the `route()` declarations for 
an `authentication::Realm` type?



3rdparty/libprocess/include/process/process.hpp (lines 261 - 264)


Looks like this needs javadoc like the existing typedef, then you can 
remove the `/* principal */` that you've added here.



3rdparty/libprocess/include/process/process.hpp (lines 372 - 374)


There is a typo in here :)

Also, could you try to avoid the run on sentence? How about saying 
something about http request handlers are equivalent to authenticated http 
request handlers with a none principal, so we convert them.



3rdparty/libprocess/src/authentication_router.hpp (lines 52 - 56)


The second sentence here confused me (e.g. the current implementation of 
what?), so how about we just write the first sentence and let the reader figure 
out the implications? Note that the implications could change over time so it's 
possible for it to go out of sync and cause more confusion.



3rdparty/libprocess/src/process.cpp (lines 231 - 232)


We really need to help the reader understand this!

```
// Http pipelining requires that when we perform http
// authentication, we need to ensure that authentication
// results are processed in sequential order. Otherwise,
// we may send responses out-of-order!
// 
// The ordering needs to be respected within a Process, but
// not across Processes since dispatches are asynchronous.
//
// Note that this needs to be done explicitly here
// because the authentication router does not guarantee
// ordered completion of its Futures (it doesn't have the
// knowledge of UPIDs necessary to do it in a fine-grained
// manner).
hashmap authenticationPipelines; // Or,
// hashmap authenticationSequences;
```

Also, it would be great to manage this correctly and erase entries when 
they are not relevant any more. As we discussed:

* Store the last future from the sequence alongside here.
* Set up a callback on future completion, the callback should check for 
equality against the current last. If equal, the entry can be erased.

Anything I'm missing there?



3rdparty/libprocess/src/process.cpp (line 2399)


"within a Process' execution context"



3rdparty/libprocess/src/process.cpp (lines 2400 - 2402)


Why are you dispatching here rather than calling directly? Note though that 
I'm not sure we should keep `schedule`.



3rdparty/libprocess/src/process.cpp (line 2447)






3rdparty/libprocess/src/process.cpp (lines 2447 - 2455)


The first comment here should match the first comment from the other 
blocks, no? How about:

Re: Review Request 40326: libprocess: Marked Boost as a "system" header.

[Joris Van Remoortere]

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

Ship it!


Ship It!

- Joris Van Remoortere


On Nov. 16, 2015, 8:47 a.m., Neil Conway wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/40326/
> ---
> 
> (Updated Nov. 16, 2015, 8:47 a.m.)
> 
> 
> Review request for mesos, Benjamin Bannier and Joris Van Remoortere.
> 
> 
> Bugs: MESOS-3799
> https://issues.apache.org/jira/browse/MESOS-3799
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This instructs the compiler to ignore warnings in the Boost headers. This is
> useful, because those headers often contain hard-to-silence warnings that are
> not otherwise useful to display. See MESOS-3799.
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/3rdparty/Makefile.am 
> 0adbe539afaf683e4a85582463a2930049a63998 
>   3rdparty/libprocess/Makefile.am cdefa37528ea69422978a9772f955042e882dde4 
>   3rdparty/libprocess/configure.ac 40801653a7fb9a943dfe33913161d28ef24040c3 
> 
> Diff: https://reviews.apache.org/r/40326/diff/
> 
> 
> Testing
> ---
> 
> Re-bootstrapped and tested compilation with Ubuntu Wily and OSX 10.10.
> 
> Note that the cmake build still emits boost-related warnings: I didn't try to 
> fix this, since cmake emits other warnings that autotools suppresses (e.g., 
> no-unused-local-typedefs).
> 
> 
> Thanks,
> 
> Neil Conway
> 
>

Re: Review Request 39780: Update OversubscriptionTest to not assume dynamic dlopen search.

[James Peach]

> On Nov. 30, 2015, 8:51 a.m., Benjamin Bannier wrote:
> > src/tests/oversubscription_tests.cpp, line 117
> > 
> >
> > This isn't really too nice, and e.g. cmake does not put build libraries 
> > into these paths (i.e. what you write will only work with automake).
> > 
> > Like I already pointed out on #40553, you should really lift the level 
> > of abstraction here, and use a function to get this path.
> 
> James Peach wrote:
> The intent of the original code was clearly to use this path. When 
> running in the context of the automake wrapper scripts, automake constructs 
> LD_LIBRARY_PATH so that we don't really need to do anything here. What does 
> cmake do? Can you point me to any documentation of how it handles library 
> search paths when executing artifacts from the build directory?
> 
> Benjamin Bannier wrote:
> You are of course right that this wasn't yours.
> 
> My point is that in #40553 you introduce abstractions like `findModule` 
> that should be perfectly good to capture what was written here.
> 
> James Peach wrote:
> Do you recommend rebasing this onto the subsequent patches? Or adding a 
> new ```getX``` helper here and rebasing the later patches on to it? Or 
> something else that will work for both automake and cmake?

If we want and abstraction like ```findModule``` then IMHO #40553 or a separate 
change is the right way to do that. Let's keep this change within the current 
scope.


- James


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


On Nov. 26, 2015, 2:03 p.m., James Peach wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/39780/
> ---
> 
> (Updated Nov. 26, 2015, 2:03 p.m.)
> 
> 
> Review request for mesos, Benjamin Bannier, Kapil Arya, Niklas Nielsen, and 
> Till Toenshoff.
> 
> 
> Bugs: MESOS-3725
> https://issues.apache.org/jira/browse/MESOS-3725
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Update OversubscriptionTest to not assume dynamic dlopen search.
> 
> 
> Diffs
> -
> 
>   src/tests/oversubscription_tests.cpp 
> 0d0bf7e0b9a4028ed116e00b56d59b670510c5ce 
> 
> Diff: https://reviews.apache.org/r/39780/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> James Peach
> 
>

Re: Review Request 39780: Update OversubscriptionTest to not assume dynamic dlopen search.

[James Peach]

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

(Updated Dec. 8, 2015, 9 p.m.)


Review request for mesos, Benjamin Bannier, Kapil Arya, Niklas Nielsen, and 
Till Toenshoff.


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


Repository: mesos


Description
---

Update OversubscriptionTest to not assume dynamic dlopen search.


Diffs (updated)
-

  src/tests/oversubscription_tests.cpp 0333281c247dd182860a49f39be791c00679bf6b 

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


Testing
---

make check


Thanks,

James Peach

Re: Review Request 39782: Add a comment for os::libraries::setPaths.

[James Peach]

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

(Updated Dec. 8, 2015, 9:01 p.m.)


Review request for mesos, Benjamin Bannier, Kapil Arya, Niklas Nielsen, and 
Till Toenshoff.


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


Repository: mesos


Description
---

Add a comment for os::libraries::setPaths.


Diffs (updated)
-

  3rdparty/libprocess/3rdparty/stout/include/stout/os.hpp 
03e6f75850561b5eb92da4771fbe18e4057ad520 

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


Testing
---

No code changes.


Thanks,

James Peach

Re: Review Request 41092: Added CMake file for agent executable build.

[Mesos ReviewBot]

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


Bad patch!

Reviews applied: [40951, 41090]

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

Error:
 2015-12-08 20:01:21 URL:https://reviews.apache.org/r/41090/diff/raw/ 
[4071/4071] -> "41090.patch" [1]
41090.patch:117: new blank line at EOF.
+
warning: 1 line adds whitespace errors.
Successfully applied: Second iteration of changes for cmake build on linux.

Second iteration of changes for cmake build on linux.


Review: https://reviews.apache.org/r/41090
src/slave/cmake/FindCurl.cmake:111: new blank line at EOF.
Failed to commit patch

- Mesos ReviewBot


On Dec. 8, 2015, 5:57 p.m., Diana Arroyo wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/41092/
> ---
> 
> (Updated Dec. 8, 2015, 5:57 p.m.)
> 
> 
> Review request for mesos, Artem Harutyunyan, Alex Clemmer, Joris Van 
> Remoortere, and Joseph Wu.
> 
> 
> Bugs: MESOS-3843
> https://issues.apache.org/jira/browse/MESOS-3843
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added CMake file for agent executable build.
> 
> 
> Diffs
> -
> 
>   src/slave/CMakeLists.txt PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/41092/diff/
> 
> 
> Testing
> ---
> 
> Tested on Ubuntu and OSX.
> 
> 
> Thanks,
> 
> Diana Arroyo
> 
>

Re: Review Request 41075: Added support for implicit roles.

[Neil Conway]

> On Dec. 8, 2015, 4:45 p.m., Alexander Rukletsov wrote:
> > src/master/http.cpp, lines 1543-1545
> > 
> >
> > Why don't we consider roles without frameworks but with a non-default 
> > weight active? Or roles with quota but without frameworks?
> > 
> > Or if you would like to reserve the term "active" for roles with 
> > frameworks, how about expanding the terminology to something like:
> > ```
> > Visible = {active} U {weighted} U {quota'ed} U {with-dyn-res}
> > ```
> > 
> > Anyway, my point is that we should show all "visible" roles here.
> 
> Neil Conway wrote:
> Yeah, this is a good point. Do we think that a user wants to see 
> `{active} U {weighted} U {quota'ed} U {with-dyn-res}`?
> 
> That does seem reasonable to me, although with the way the data 
> structures are organized, it will be somewhat ugly to implement :-\

Thinking about this a bit more:

* `/roles` currently doesn't show quotas.
* It also doesn't show dynamically reserved resources.
* If you set ACLs that involve a role, you could argue that it should show up 
in `/roles` as well.

Given that most of these issues are not related to implicit roles as such, I'm 
inclined to say we should defer making these changes -- perhaps as part of the 
work on dynamic weights/ACLs, we can reorganize the data structures to make 
this easier to implement anyway. I created 
https://issues.apache.org/jira/browse/MESOS-4097 to track this.


- Neil


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


On Dec. 8, 2015, 7:18 p.m., Neil Conway wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/41075/
> ---
> 
> (Updated Dec. 8, 2015, 7:18 p.m.)
> 
> 
> Review request for mesos, Adam B, Alexander Rukletsov, Benjamin Hindman, Greg 
> Mann, and Yong Qiao Wang.
> 
> 
> Bugs: MESOS-4085
> https://issues.apache.org/jira/browse/MESOS-4085
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Changed the behavior of the master when the `--roles` flag is NOT
> specified. Previously, this would allow only the `*` role to be used. Now,
> omitting `--roles` means that any role can be used. This is called "implicit
> roles". Configuring which principals can perform operations as which roles
> should be done using ACLs in the authorization system.
> 
> Note that this changes the behavior of the system when `--roles` is not
> specified. This is likely acceptable: if the operator didn't specify `--roles`
> in prior versions of Mesos, they were likely not using roles or authorization 
> at
> that time.
> 
> Another minor behavioral change is that the "/roles" endpoint will now only
> return results for currently "active" roles (those with one or more registered
> frameworks).
> 
> The `--roles` flag is now considered deprecated and will be removed in a 
> future
> version of Mesos.
> 
> 
> Diffs
> -
> 
>   include/mesos/master/allocator.hpp 619ef01c3a7d640560653cfc1838dd09046d1da0 
>   include/mesos/master/allocator.proto 
> 702f56f56c3b1331613cecf26522986f6b572f8c 
>   src/master/allocator/mesos/allocator.hpp 
> 97ee80726ad155917811265a983258b0165d3451 
>   src/master/allocator/mesos/hierarchical.hpp 
> 99c742906874c30c39c159e58a65277ade3c07fd 
>   src/master/allocator/mesos/hierarchical.cpp 
> 5da825a1d578a9ee40b4985378fddb3c5fb3b416 
>   src/master/http.cpp 9097eda47558fb5306af5a61b464937d7ab34e83 
>   src/master/master.hpp 4683fa542a740f9a0b80fff7fbe0e63ec66266f2 
>   src/master/master.cpp 953fa4f14929581b226a7e27d30aea7a5aa1fd7c 
>   src/master/quota_handler.cpp b209da42ace752953686eeda9577007a33556d5d 
>   src/tests/allocator.hpp c7670525765491fe931a4ee38446fa7e9d79af42 
>   src/tests/hierarchical_allocator_tests.cpp 
> fb214a829a57529d3f5c49730ae9733f53e622ca 
> 
> Diff: https://reviews.apache.org/r/41075/diff/
> 
> 
> Testing
> ---
> 
> "make check" on OSX 10.10 and Ubuntu 15.10; `--gtest_repeat=1000` for the 
> more likely role-related tests.
> 
> TODOs:
> 
> * Update documentation
> * Add tests for allocation behavior for weights + implicit roles
> * Add tests for quota + implicit roles?
> 
> Notes:
> 
> * There's two places where we use manual `new`/`delete` where a `unique_ptr` 
> would probably be nicer. I'm inclined to leave this as-is for now though 
> (making use of unique_ptr is a broader issue).
> 
> 
> Thanks,
> 
> Neil Conway
> 
>

Re: Review Request 41097: Fixed hyphen usage in comments.

[Jie Yu]

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

Ship it!


Ship It!

- Jie Yu


On Dec. 8, 2015, 7:29 p.m., Neil Conway wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/41097/
> ---
> 
> (Updated Dec. 8, 2015, 7:29 p.m.)
> 
> 
> Review request for mesos and Jie Yu.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Fixed hyphen usage in comments.
> 
> 
> Diffs
> -
> 
>   src/cmake/MesosProtobuf.cmake d5e1e13bcf38ff2a211b8dea4f7ec147301553db 
>   src/examples/java/TestFramework.java 
> 55c6ee6fdfe8fa001799096388e4909de6a55227 
>   src/examples/java/TestLog.java 8fa81582695468734c66f959f63be19d4811e48b 
>   src/jvm/jvm.hpp 75262b21b5d9cb0d88696da790f6feceb6e9895f 
>   src/tests/authorization_tests.cpp 32beeea3584f093bcac24e0c160235de0e3abb28 
> 
> Diff: https://reviews.apache.org/r/41097/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Neil Conway
> 
>

Re: Review Request 40327: mesos: Marked Boost as a "system" header.

[Joris Van Remoortere]

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

Ship it!


Ship It!

- Joris Van Remoortere


On Nov. 16, 2015, 8:48 a.m., Neil Conway wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/40327/
> ---
> 
> (Updated Nov. 16, 2015, 8:48 a.m.)
> 
> 
> Review request for mesos, Benjamin Bannier and Joris Van Remoortere.
> 
> 
> Bugs: MESOS-3799
> https://issues.apache.org/jira/browse/MESOS-3799
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This instructs the compiler to ignore warnings in the Boost headers. This is
> useful, because those headers often contain hard-to-silence warnings that are
> not otherwise useful to display. See MESOS-3799.
> 
> 
> Diffs
> -
> 
>   src/Makefile.am 92d07c3da897c7664c63e2af91d45921d66f64aa 
> 
> Diff: https://reviews.apache.org/r/40327/diff/
> 
> 
> Testing
> ---
> 
> Re-bootstrapped and tested compilation with Ubuntu Wily and OSX 10.10.
> 
> Note that the cmake build still emits boost-related warnings: I didn't try to 
> fix this, since cmake emits other warnings that autotools suppresses (e.g., 
> no-unused-local-typedefs).
> 
> 
> Thanks,
> 
> Neil Conway
> 
>

Re: Review Request 39597: Add Newbie guide.

[Neil Conway]

> On Nov. 2, 2015, 8:59 p.m., Neil Conway wrote:
> > docs/NewbieQuickStart.md, line 86
> > 
> >
> > Links to other docs pages should take the form "(anchor-text)[foo.md]", 
> > rather than using the full URL.
> 
> Diana Arroyo wrote:
> I didn't add the above suggestion because it doesn't seem to work with 
> the docs online.  See: 
> http://mesos.apache.org/documentation/latest/architecture/ and scroll down to 
> the hotlink "App/Framework development guide".  If you select the link it 
> fails.  The underlying text in the architecture.md file is: (see the 
> [App/Framework development guide](app-framework-development-guide.md).

Hi Diana, that is a separate/unrelated issue (see 
https://issues.apache.org/jira/browse/MESOS-1521 -- should be fixed when the 
site is next regenerated). Your change should follow the same link format used 
by the rest of the documentation pages (i.e., it should not use an absolute 
URL).


- Neil


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


On Oct. 30, 2015, 9 p.m., Diana Arroyo wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/39597/
> ---
> 
> (Updated Oct. 30, 2015, 9 p.m.)
> 
> 
> Review request for mesos, Timothy Chen and Vinod Kone.
> 
> 
> Bugs: MESOS-3224
> https://issues.apache.org/jira/browse/MESOS-3224
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Add Newbie guide.
> 
> 
> Diffs
> -
> 
>   docs/NewbieQuickStart.md PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/39597/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Diana Arroyo
> 
>

Re: Review Request 38000: Introduced support for user interaction with HTTP AuthenticationRouter.

[Ben Mahler]

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


Second part of my earlier review, this time I went throug the test code.


3rdparty/libprocess/src/tests/http_tests.cpp (lines 1214 - 1216)


Looking at this, it's a little strange that realm comes after help.

Also, what's "mesos"? :)

In the interest of making this as readable as possible, how about we just 
use "realm" in the tests here? Why do anything less direct?



3rdparty/libprocess/src/tests/http_tests.cpp (lines 1226 - 1250)


After you re-use HttpProxy, the only thing that should be in this fixture 
is setting the authenticator during setup and unsetting during teardown.

How about naming this fixture 'HttpAuthenticationTest'? I can't quite tell 
why some names have http as the test case name, and some don't:

```
AuthenticationTest. HTTPAuthenticationNoAuthenticator
AuthenticationTest, AuthenticationNoCredentials
AuthenticationTest, AuthenticationSuccess
AuthenticationTest, DelayedAuthentication
```

How about the following, also note I tried not to repeat authentication 
since it's clear from the test fixture name:

```
HttpAuthenticationTest, Disabled
HttpAuthenticationTest, Unauthorized
HttpAuthenticationTest, Forbidden
HttpAuthenticationTest, Authenticated
HttpAuthenticationTest, Pipelining
```



3rdparty/libprocess/src/tests/http_tests.cpp (lines 1253 - 1254)


An endpoint doesn't really request authentication, it just specifies a 
realm, no? I imagine that the wiring of authenticators to realms isn't tightly 
tied to the route() calls.

How about:

```
Ensures that when there is no authenticator for
a realm, requests are not authenticated (i.e. the
principal is None).
```



3rdparty/libprocess/src/tests/http_tests.cpp (lines 1258 - 1259)


Ugh.

I suspected I was going to see something like this when I saw your test 
fixture. If we're going to put things in test fixtures I would suggest making 
them flexible and explicit, rather than implicit and static.

For example, rather than statically deciding to add an authenticator to the 
"mesos" realm (which let's just call "realm"!), we can have an explicit method 
for adding an authenticator:

```
addAuthenticator("realm", Owned(new MockAuthenticator());
```

When a test calls this, the test fixture will keep track of the set of 
realms that need to be unset, so that we can clean up in TearDown.

Sound good?



3rdparty/libprocess/src/tests/http_tests.cpp (line 1270)


Have you looked at gtest.hpp in libprocess?

We have: AWAIT_EXPECT_RESPONSE_STATUS_EQ

We should update that assertion to check both the code and status, not sure 
why we manually check both all over the place.



3rdparty/libprocess/src/tests/http_tests.cpp (line 1273)


No CHECKs in tests please :(

Also, you don't need to do this AFAICT, you can just do the following above 
to simplify this:

```
EXPECT_CALL((*process.get()), handler1(_, Option::none()));
```

Does that not work?



3rdparty/libprocess/src/tests/http_tests.cpp (line 1281)


Let's avoid the "auth" abbreviation entirely, can you do a sweep? I also 
see it in the tests below.



3rdparty/libprocess/src/tests/http_tests.cpp (lines 1295 - 1297)


Why is this EXPECT_SOME_EQ instead of EXPECT_EQ? Aren't both of these 
options?



3rdparty/libprocess/src/tests/http_tests.cpp (line 1305)


In the interest of making this more readable:

s/"foo"/"principal"/



3rdparty/libprocess/src/tests/http_tests.cpp (line 1309)


Why is there an underscore here?



3rdparty/libprocess/src/tests/http_tests.cpp (lines 1310 - 1311)


Do you need to capture? It looks like you can just place the value in the 
expectation:

```
EXPECT_CALL((*process.get()), handler1(_, "principal"));
```

Not sure if it can implicitly construct the option here, might need to do 
that explicitly.



3rdparty/libprocess/src/tests/http_tests.cpp (line 1316)


Can you just do "user:password" here? The comment seems sufficient to me

Re: Review Request 40339: Added a flag to master to enable oversubscription for reservations.

[Guangya Liu]

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

(Updated 十二月 8, 2015, 11:53 p.m.)


Review request for mesos, Artem Harutyunyan, Joris Van Remoortere, Joseph Wu, 
and Klaus Ma.


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


Repository: mesos


Description
---

Added a flag to master to enable optimistic offers.


Diffs
-

  include/mesos/master/allocator.hpp 619ef01c3a7d640560653cfc1838dd09046d1da0 
  src/master/allocator/mesos/allocator.hpp 
97ee80726ad155917811265a983258b0165d3451 
  src/master/allocator/mesos/hierarchical.hpp 
99c742906874c30c39c159e58a65277ade3c07fd 
  src/master/allocator/mesos/hierarchical.cpp 
5da825a1d578a9ee40b4985378fddb3c5fb3b416 
  src/master/flags.hpp 9af6c68eef6bcf39d5776809fab6c66dc95da6b2 
  src/master/flags.cpp f9223edb33483cb5619e7dd75221f735306462c6 
  src/master/master.cpp 953fa4f14929581b226a7e27d30aea7a5aa1fd7c 
  src/tests/allocator.hpp c7670525765491fe931a4ee38446fa7e9d79af42 
  src/tests/hierarchical_allocator_tests.cpp 
fb214a829a57529d3f5c49730ae9733f53e622ca 
  src/tests/master_allocator_tests.cpp cba7c36471f93b678d94e1da0251a28a893696b1 
  src/tests/master_quota_tests.cpp 12820fcf7c0d2a791071464ad8ed738664ad85de 
  src/tests/persistent_volume_endpoints_tests.cpp 
0a03b5f1ac7dec14bd99c31768f86100f2b60616 
  src/tests/reservation_endpoints_tests.cpp 
c3833d0949df42c9f8dadf3c6e6b0b49e6cbdce9 
  src/tests/reservation_tests.cpp 3fdf5e121840fe99057e917cca48f1425eff6624 
  src/tests/resource_offers_tests.cpp 0bad45dd1dabecc88fef1ab46e8ea26718070b33 
  src/tests/slave_recovery_tests.cpp c0e4ff75b35c9e806741aab5696771e66d2c2ea8 

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


Testing
---

Ubuntu 14.04
make
make check


Thanks,

Guangya Liu

Re: Review Request 41114: Showed disk resources in the WebUI.

[Mesos ReviewBot]

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


Patch looks great!

Reviews applied: [41114]

Passed command: export OS=ubuntu:14.04;export CONFIGURATION="--verbose";export 
COMPILER=gcc; ./support/docker_build.sh

- Mesos ReviewBot


On Dec. 9, 2015, 1:36 a.m., Vinod Kone wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/41114/
> ---
> 
> (Updated Dec. 9, 2015, 1:36 a.m.)
> 
> 
> Review request for mesos, Ben Mahler and Michael Park.
> 
> 
> Bugs: MESOS-4103
> https://issues.apache.org/jira/browse/MESOS-4103
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Lots of places in the WebUI only shows cpus and mem, but not disk. This fixes 
> it.
> 
> Also, we were showing "0" disk used when the statistics received from the 
> slave didn't contain disk metrics, but I think it's misleading. I fixed it to 
> instead show nothing to make it clear that the value is unknown.
> 
> 
> Diffs
> -
> 
>   src/webui/master/static/framework.html 
> 8c6e5e4e02a769fd0098150b0dcba69f43b20232 
>   src/webui/master/static/frameworks.html 
> 44ef2e55c1e37bb74f769842fe1babffe31a8ea9 
>   src/webui/master/static/home.html 4fc64c0aea558fca18083dc317f8370670d7a4d3 
>   src/webui/master/static/js/controllers.js 
> ccf5c31715e298e96f493cce58921bfe6b16b779 
>   src/webui/master/static/js/dashboard.js 
> 5b1769c8dc731d85a7badfaa44794774da95000c 
>   src/webui/master/static/js/services.js 
> d41bc7142ae0182e25b2ae9dce960ee38ab22361 
>   src/webui/master/static/offers.html 
> cc199352fcf2074dcd0e8d24bb49651b38c41d86 
>   src/webui/master/static/slave.html a1446bce827944609faf10f72e788f33d275d6f9 
>   src/webui/master/static/slave_executor.html 
> 7c66405090f46f89bdd29806a58c05dc76c0ad23 
>   src/webui/master/static/slave_framework.html 
> 90eff2f3b4de08f98283b27e1635ba23a855e488 
> 
> Diff: https://reviews.apache.org/r/41114/diff/
> 
> 
> Testing
> ---
> 
> Tested locally by running a local master, slave and mesos-execute with disk 
> resources.
> 
> 
> Thanks,
> 
> Vinod Kone
> 
>

Re: Review Request 39597: Add Newbie guide.

[Diana Arroyo]

> On Nov. 6, 2015, 5:18 p.m., Vaibhav Khanduja wrote:
> > docs/NewbieQuickStart.md, line 108
> > 
> >
> > I am a "newbie" so was referring to your wip guide. I tried to run this 
> > command, but looks like it did not work me.
> > 
> > a) the options --gtest-filter, --gtest-shuffle and --gtest-repeat are 
> > having "underscore" and not "hyphen" 
> > 
> > --gtest_filter, --gtest_shuffle and --gtest_repeat
> > 
> > b) The command did not filter on Docker testcases, but ran all test 
> > cases. 
> > 
> > sudo GLOG_v=1 ./bin/mesos-tests.sh  –gtest_filter=”*DOCKER*” 
> > --break-on-error –gtest_shuffle –gtest_repeat=100
> > 
> > is there something wrong here?
> 
> Timothy Chen wrote:
> You need two dashes (--gtest_filter), otherwise it should work.
> 
> Vaibhav Khanduja wrote:
> Thanks Tim ... suggestions in "a)" should be made .. the options have 
> underscore between than hyphen e.g. "--gtest_filter" and not "--gtest-filter"

I corrected the "--gtest_filter" parameter in the document and added a "sudo" 
at the beginning as well.  Thanks Vaibhav!


- Diana


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


On Oct. 30, 2015, 9 p.m., Diana Arroyo wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/39597/
> ---
> 
> (Updated Oct. 30, 2015, 9 p.m.)
> 
> 
> Review request for mesos, Timothy Chen and Vinod Kone.
> 
> 
> Bugs: MESOS-3224
> https://issues.apache.org/jira/browse/MESOS-3224
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Add Newbie guide.
> 
> 
> Diffs
> -
> 
>   docs/NewbieQuickStart.md PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/39597/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Diana Arroyo
> 
>

Re: Review Request 40339: Added a flag to master to enable oversubscription for reservations.

[Guangya Liu]

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

(Updated 十二月 8, 2015, 11:55 p.m.)


Review request for mesos, Artem Harutyunyan, Joris Van Remoortere, Joseph Wu, 
and Klaus Ma.


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


Repository: mesos


Description
---

Added a flag to master to enable optimistic offers.


Diffs
-

  include/mesos/master/allocator.hpp 619ef01c3a7d640560653cfc1838dd09046d1da0 
  src/master/allocator/mesos/allocator.hpp 
97ee80726ad155917811265a983258b0165d3451 
  src/master/allocator/mesos/hierarchical.hpp 
99c742906874c30c39c159e58a65277ade3c07fd 
  src/master/allocator/mesos/hierarchical.cpp 
5da825a1d578a9ee40b4985378fddb3c5fb3b416 
  src/master/flags.hpp 9af6c68eef6bcf39d5776809fab6c66dc95da6b2 
  src/master/flags.cpp f9223edb33483cb5619e7dd75221f735306462c6 
  src/master/master.cpp 953fa4f14929581b226a7e27d30aea7a5aa1fd7c 
  src/tests/allocator.hpp c7670525765491fe931a4ee38446fa7e9d79af42 
  src/tests/hierarchical_allocator_tests.cpp 
fb214a829a57529d3f5c49730ae9733f53e622ca 
  src/tests/master_allocator_tests.cpp cba7c36471f93b678d94e1da0251a28a893696b1 
  src/tests/master_quota_tests.cpp 12820fcf7c0d2a791071464ad8ed738664ad85de 
  src/tests/persistent_volume_endpoints_tests.cpp 
0a03b5f1ac7dec14bd99c31768f86100f2b60616 
  src/tests/reservation_endpoints_tests.cpp 
c3833d0949df42c9f8dadf3c6e6b0b49e6cbdce9 
  src/tests/reservation_tests.cpp 3fdf5e121840fe99057e917cca48f1425eff6624 
  src/tests/resource_offers_tests.cpp 0bad45dd1dabecc88fef1ab46e8ea26718070b33 
  src/tests/slave_recovery_tests.cpp c0e4ff75b35c9e806741aab5696771e66d2c2ea8 

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


Testing
---

Ubuntu 14.04
make
make check


Thanks,

Guangya Liu

Re: Review Request 40731: Adding the test framework submitted by Mandeep (@mchadha) https://reviews.apache.org/r/39056/

[Avinash sridharan]

> On Dec. 2, 2015, 6:32 p.m., Bernd Mathiske wrote:
> > src/tests/reservation_tests.cpp, line 248
> > 
> >
> > More customary indentation:
> > 
> >  EXPECT_CALL(sched, resourceOffers(, _))
> >.WillOnce(FutureArg<1>());
> 
> Avinash sridharan wrote:
> Was following this particular style guide:
> http://mesos.apache.org/documentation/latest/c++-style-guide/
> 
> The "Function Definition/Invocation" section wants us to follow styles 
> 1,4, 5 and sometimes 3 and 'never' 2 hence the change?

Anand had a similar comment. I was trying to follow the style guide, but I 
guess there are already inconsistencies in the existing code and enforcing the 
style guide will create an outlier that might make it worse. So just sticking 
with whatever is followed in the current file.


> On Dec. 2, 2015, 6:32 p.m., Bernd Mathiske wrote:
> > src/tests/reservation_tests.cpp, line 266
> > 
> >
> > Missing blanks after "//".

Remove this comment since Anand felt it was redundant.


- Avinash


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


On Dec. 2, 2015, 10:38 p.m., Avinash sridharan wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/40731/
> ---
> 
> (Updated Dec. 2, 2015, 10:38 p.m.)
> 
> 
> Review request for mesos, Bernd Mathiske, Klaus Ma, Mandeep Chadha, and Neil 
> Conway.
> 
> 
> Bugs: MESOS-3552
> https://issues.apache.org/jira/browse/MESOS-3552
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Adding the test framework submitted by Mandeep (@mchadha) 
> https://reviews.apache.org/r/39056/
> 
> 
> Diffs
> -
> 
>   src/tests/reservation_tests.cpp e7f14079e766ce0a8bad2da646776347e4a17169 
> 
> Diff: https://reviews.apache.org/r/40731/diff/
> 
> 
> Testing
> ---
> 
> Ran make check after adding Mandeep's test case to the GTEST framework.
> 
> 
> Thanks,
> 
> Avinash sridharan
> 
>

Re: Review Request 39597: Add Newbie guide.

[Diana Arroyo]

> On Oct. 23, 2015, 4:07 p.m., James Peach wrote:
> > I'm pretty sure that Mesos only builds with the bundled GLog and GTest. At 
> > least I've always been unsuccessful in building against unbundled versions 
> > of those components.

Hi James, do you think we should change some content based on your review 
comment above?  Please advise.  Thanks.


- Diana


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


On Oct. 30, 2015, 9 p.m., Diana Arroyo wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/39597/
> ---
> 
> (Updated Oct. 30, 2015, 9 p.m.)
> 
> 
> Review request for mesos, Timothy Chen and Vinod Kone.
> 
> 
> Bugs: MESOS-3224
> https://issues.apache.org/jira/browse/MESOS-3224
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Add Newbie guide.
> 
> 
> Diffs
> -
> 
>   docs/NewbieQuickStart.md PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/39597/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Diana Arroyo
> 
>

Re: Review Request 39597: Add Newbie guide.

[James Peach]

> On Oct. 23, 2015, 4:07 p.m., James Peach wrote:
> > I'm pretty sure that Mesos only builds with the bundled GLog and GTest. At 
> > least I've always been unsuccessful in building against unbundled versions 
> > of those components.
> 
> Diana Arroyo wrote:
> Hi James, do you think we should change some content based on your review 
> comment above?  Please advise.  Thanks.

Consider removing them from the list of tooling prerequisites? They are bundled 
with Mesos so using external versions seems like an advanced task, not 
something that you'd want to deal with right away.


- James


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


On Dec. 8, 2015, 11:40 p.m., Diana Arroyo wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/39597/
> ---
> 
> (Updated Dec. 8, 2015, 11:40 p.m.)
> 
> 
> Review request for mesos, Timothy Chen and Vinod Kone.
> 
> 
> Bugs: MESOS-3224
> https://issues.apache.org/jira/browse/MESOS-3224
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Add Newbie guide.
> 
> 
> Diffs
> -
> 
>   docs/NewbieQuickStart.md PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/39597/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Diana Arroyo
> 
>

Re: Review Request 39597: Add Newbie guide.

[Diana Arroyo]

> On Nov. 2, 2015, 7:01 p.m., Timothy Chen wrote:
> > Thanks for doing this Diana! Sorry for the delay, we need to figure out a 
> > place to put this on the website as well. You have suggestions?

How about in this page: http://mesos.apache.org/community/ under the title: 
"Contribute a core patch"?


- Diana


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


On Oct. 30, 2015, 9 p.m., Diana Arroyo wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/39597/
> ---
> 
> (Updated Oct. 30, 2015, 9 p.m.)
> 
> 
> Review request for mesos, Timothy Chen and Vinod Kone.
> 
> 
> Bugs: MESOS-3224
> https://issues.apache.org/jira/browse/MESOS-3224
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Add Newbie guide.
> 
> 
> Diffs
> -
> 
>   docs/NewbieQuickStart.md PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/39597/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Diana Arroyo
> 
>

Review Request 41108: Add curl, sasl and dl link flags and add protobuf library directory

[Diana Arroyo]

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

Review request for mesos, Artem Harutyunyan, Alex Clemmer, Joris Van 
Remoortere, and Joseph Wu.


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


Repository: mesos


Description
---

Add curl, sasl and dl link flags and add protobuf library directory


Diffs
-

  src/slave/cmake/SlaveConfigure.cmake fbdfdaa27fbd8c7429861eea5baf401a221f748b 

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


Testing
---

Tested on Ubuntu and OSX.


Thanks,

Diana Arroyo

Re: Review Request 39597: Add Newbie guide.

[Diana Arroyo]

> On Nov. 21, 2015, 1:05 a.m., Timothy Chen wrote:
> > Diana are you still able to finish the comments?
> 
> Diana Arroyo wrote:
> Hey Tim, Yes, I'll finish them up.

Done.  I only have to outstanding items from Neil I didn't fix but provided a 
comment instead.  Do we wait for Neil to agree to the comments before we merge? 
 Please advise.


- Diana


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


On Oct. 30, 2015, 9 p.m., Diana Arroyo wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/39597/
> ---
> 
> (Updated Oct. 30, 2015, 9 p.m.)
> 
> 
> Review request for mesos, Timothy Chen and Vinod Kone.
> 
> 
> Bugs: MESOS-3224
> https://issues.apache.org/jira/browse/MESOS-3224
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Add Newbie guide.
> 
> 
> Diffs
> -
> 
>   docs/NewbieQuickStart.md PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/39597/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Diana Arroyo
> 
>

Review Request 41114: Showed disk resources in the WebUI.

[Vinod Kone]

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

Review request for mesos, Ben Mahler and Michael Park.


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


Repository: mesos


Description
---

Lots of places in the WebUI only shows cpus and mem, but not disk. This fixes 
it.

Also, we were showing "0" disk used when the statistics received from the slave 
didn't contain disk metrics, but I think it's misleading. I fixed it to instead 
show nothing to make it clear that the value is unknown.


Diffs
-

  src/webui/master/static/framework.html 
8c6e5e4e02a769fd0098150b0dcba69f43b20232 
  src/webui/master/static/frameworks.html 
44ef2e55c1e37bb74f769842fe1babffe31a8ea9 
  src/webui/master/static/home.html 4fc64c0aea558fca18083dc317f8370670d7a4d3 
  src/webui/master/static/js/controllers.js 
ccf5c31715e298e96f493cce58921bfe6b16b779 
  src/webui/master/static/js/dashboard.js 
5b1769c8dc731d85a7badfaa44794774da95000c 
  src/webui/master/static/js/services.js 
d41bc7142ae0182e25b2ae9dce960ee38ab22361 
  src/webui/master/static/offers.html cc199352fcf2074dcd0e8d24bb49651b38c41d86 
  src/webui/master/static/slave.html a1446bce827944609faf10f72e788f33d275d6f9 
  src/webui/master/static/slave_executor.html 
7c66405090f46f89bdd29806a58c05dc76c0ad23 
  src/webui/master/static/slave_framework.html 
90eff2f3b4de08f98283b27e1635ba23a855e488 

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


Testing
---

Tested locally by running a local master, slave and mesos-execute with disk 
resources.


Thanks,

Vinod Kone

Re: Review Request 40211: Add mesos provisioner doc.

[Jie Yu]

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

Ship it!


Looks good overall. Thanks Tim!


docs/mesos-provisioner.md (line 5)


It's not just container filesystem spec, right? It also includes runtime 
specification. Can you just say:

```
There are multiple container specifications, ...
```

For runC, it's only for runtime right? So your next sentense does not sound 
so right. How about this:

```
Most of the container specifications, to varying degrees, conflate image 
format specification with other components of a container, including...
```



docs/mesos-provisioner.md (line 7)


s/R/r/

s/different formats/different image formats/



docs/mesos-provisioner.md (line 13)


I wouldn't mention the runtime configuration here since it's pretty 
confusing. I would rather add a section later to explain the runtime 
configuration part (but not in this review probably).



docs/mesos-provisioner.md (line 51)


Why this limitation. I think we can still use image-in-volume even if 
command line executor is used, right?



docs/mesos-provisioner.md (line 59)


you probably want to explain the backend flag (or add a TODO) since it's 
not quite intuitive what that is.



docs/mesos-provisioner.md (line 63)


Kill extra blank line



docs/mesos-provisioner.md (line 67)


kill extra blank line


- Jie Yu


On Dec. 4, 2015, 9:26 a.m., Timothy Chen wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/40211/
> ---
> 
> (Updated Dec. 4, 2015, 9:26 a.m.)
> 
> 
> Review request for mesos, Jie Yu and Jojy Varghese.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Add mesos provisioner doc.
> 
> 
> Diffs
> -
> 
>   docs/mesos-provisioner.md PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/40211/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Timothy Chen
> 
>

Re: Review Request 40224: Fix wrong flags infos in /state and /flags

[Jian Qiu]

> On Nov. 23, 2015, 4:08 p.m., haosdent huang wrote:
> > src/master/http.cpp, line 331
> > 
> >
> > Seems miss credentials, whitelist

These two flags returns as Path instead of protobuf.


- Jian


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


On Nov. 23, 2015, 5:56 a.m., Jian Qiu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/40224/
> ---
> 
> (Updated Nov. 23, 2015, 5:56 a.m.)
> 
> 
> Review request for mesos, Ben Mahler and haosdent huang.
> 
> 
> Bugs: MESOS-3792
> https://issues.apache.org/jira/browse/MESOS-3792
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Fix wrong flags infos in /state and /flags. We should convert protobuf type 
> flag to JSON object explicitly instead of relying on stringify
> 
> 
> Diffs
> -
> 
>   src/master/http.cpp b0bec97ee69413bb70c2673c4ae49e74988796bf 
> 
> Diff: https://reviews.apache.org/r/40224/diff/
> 
> 
> Testing
> ---
> 
> make & make check
> 
> ./mesos-master.sh --work_dir=/tmp/mesos --acls='{"register_frameworks": 
> [{"principals": { "type": "ANY" },"roles": { "values": ["a"] }}],"run_tasks": 
> [{"principals": { "values": ["a", "b"] },"users": { "values": ["c"] 
> }}],"shutdown_frameworks": [{"principals": { "values": ["a", "b"] 
> },"framework_principals": { "values": ["c"] }}]}'
> 
> curl http://master:5050/state
> curl http://master:5050/flag
> 
> 
> Thanks,
> 
> Jian Qiu
> 
>

Re: Review Request 40224: Fix wrong flags infos in /state and /flags

[Jian Qiu]

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

(Updated Dec. 9, 2015, 2:20 a.m.)


Review request for mesos, Ben Mahler and haosdent huang.


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


Repository: mesos


Description
---

Fix wrong flags infos in /state and /flags. We should convert protobuf type 
flag to JSON object explicitly instead of relying on stringify


Diffs (updated)
-

  src/master/http.cpp 9097eda47558fb5306af5a61b464937d7ab34e83 
  src/slave/http.cpp cef568d77da430b96e8f97487a8d0406dc0a0116 

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


Testing
---

make & make check

./mesos-master.sh --work_dir=/tmp/mesos --acls='{"register_frameworks": 
[{"principals": { "type": "ANY" },"roles": { "values": ["a"] }}],"run_tasks": 
[{"principals": { "values": ["a", "b"] },"users": { "values": ["c"] 
}}],"shutdown_frameworks": [{"principals": { "values": ["a", "b"] 
},"framework_principals": { "values": ["c"] }}]}'

curl http://master:5050/state
curl http://master:5050/flag


Thanks,

Jian Qiu

Re: Review Request 37541: Add trace event API

[Cong Wang]

> On Nov. 24, 2015, 12:53 a.m., Vinod Kone wrote:
> > src/linux/perf.hpp, lines 108-110
> > 
> >
> > why are these public?

Easy to access, instead of adding a get/set for each of them.


> On Nov. 24, 2015, 12:53 a.m., Vinod Kone wrote:
> > src/linux/perf.cpp, lines 1125-1128
> > 
> >
> > this could use some comments.

What comments do you expect here? Some comment to explain what ID is? I thought 
the code is clear.


> On Nov. 24, 2015, 12:53 a.m., Vinod Kone wrote:
> > src/tests/containerizer/cgroups_tests.cpp, line 1105
> > 
> >
> > no need for this?

We need this, otherwise we would still hold a ref to the cgroup which causes a 
failure to destroy it later.


> On Nov. 24, 2015, 12:53 a.m., Vinod Kone wrote:
> > src/linux/perf.cpp, lines 974-1029
> > 
> >
> > this could use some comments.

I will add some ASCII arts here to draw the ring buffer.


- Cong


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


On Sept. 30, 2015, 12:14 a.m., Cong Wang wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/37541/
> ---
> 
> (Updated Sept. 30, 2015, 12:14 a.m.)
> 
> 
> Review request for mesos, Chi Zhang, Ian Downes, Jie Yu, and Vinod Kone.
> 
> 
> Bugs: MESOS-2769
> https://issues.apache.org/jira/browse/MESOS-2769
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Based on the PerfEvent API's, add API for Linux kernel trace events, 
> especially the schedule trace events.
> 
> 
> Diffs
> -
> 
>   src/linux/perf.hpp d10968ca670eb516bae08385fd0ddde8e8ad83b5 
>   src/linux/perf.cpp f7035ddb2507a7646d88dd517d048018f695448a 
>   src/tests/containerizer/cgroups_tests.cpp 
> 75a3bc0009c037dc18ce319db2eb44630f083e8c 
>   src/tests/containerizer/perf_tests.cpp 
> ed5212ee31b8aa47339b8b8fab184bbdf85be82a 
> 
> Diff: https://reviews.apache.org/r/37541/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Cong Wang
> 
>

Re: Review Request 39597: Add Newbie guide.

[Diana Arroyo]

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

(Updated Dec. 8, 2015, 11:40 p.m.)


Review request for mesos, Timothy Chen and Vinod Kone.


Changes
---

Changes as a result of reviews.


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


Repository: mesos


Description
---

Add Newbie guide.


Diffs (updated)
-

  docs/NewbieQuickStart.md PRE-CREATION 

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


Testing
---


Thanks,

Diana Arroyo

Re: Review Request 40951: Initial set of source files missing for cmake agent binary.

[Joseph Wu]

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

Ship it!


Ship It!

- Joseph Wu


On Dec. 8, 2015, 7:59 a.m., Diana Arroyo wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/40951/
> ---
> 
> (Updated Dec. 8, 2015, 7:59 a.m.)
> 
> 
> Review request for mesos, Artem Harutyunyan, Alex Clemmer, Joris Van 
> Remoortere, and Joseph Wu.
> 
> 
> Bugs: MESOS-3843
> https://issues.apache.org/jira/browse/MESOS-3843
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Initial set of source files missing for cmake agent binary.
> 
> 
> Diffs
> -
> 
>   src/CMakeLists.txt c0d77c745eb5b12dd6d9d7afaba7e820f8d848ef 
> 
> Diff: https://reviews.apache.org/r/40951/diff/
> 
> 
> Testing
> ---
> 
> Tested to make sure library builds successfully.
> 
> 
> Thanks,
> 
> Diana Arroyo
> 
>

Re: Review Request 40951: Initial set of source files missing for cmake agent binary.

[Diana Arroyo]

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

(Updated Dec. 9, 2015, 3:45 a.m.)


Review request for mesos, Artem Harutyunyan, Alex Clemmer, Joris Van 
Remoortere, and Joseph Wu.


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


Repository: mesos


Description
---

Initial set of source files missing for cmake agent binary.


Diffs (updated)
-

  src/CMakeLists.txt c0d77c745eb5b12dd6d9d7afaba7e820f8d848ef 

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


Testing
---

Tested to make sure library builds successfully.


Thanks,

Diana Arroyo

Re: Review Request 41090: Second iteration of changes for cmake build on linux.

[Diana Arroyo]

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

(Updated Dec. 9, 2015, 3:45 a.m.)


Review request for mesos, Artem Harutyunyan, Alex Clemmer, Joris Van 
Remoortere, and Joseph Wu.


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


Repository: mesos


Description
---

Second iteration of changes for cmake build on linux.


Diffs (updated)
-

  src/slave/cmake/FindCurl.cmake PRE-CREATION 

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


Testing
---

Tested on Ubuntu and OSX.


Thanks,

Diana Arroyo

Re: Review Request 39597: Add Newbie guide.

[Mesos ReviewBot]

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


Patch looks great!

Reviews applied: [39597]

Passed command: export OS=ubuntu:14.04;export CONFIGURATION="--verbose";export 
COMPILER=gcc; ./support/docker_build.sh

- Mesos ReviewBot


On Dec. 8, 2015, 11:40 p.m., Diana Arroyo wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/39597/
> ---
> 
> (Updated Dec. 8, 2015, 11:40 p.m.)
> 
> 
> Review request for mesos, Timothy Chen and Vinod Kone.
> 
> 
> Bugs: MESOS-3224
> https://issues.apache.org/jira/browse/MESOS-3224
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Add Newbie guide.
> 
> 
> Diffs
> -
> 
>   docs/NewbieQuickStart.md PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/39597/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Diana Arroyo
> 
>

Re: Review Request 40999: Fixed flakiness in ReservationTest.ACLMultipleOperations.

[Michael Park]

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

Ship it!


Ship It!

- Michael Park


On Dec. 8, 2015, 4:57 p.m., Greg Mann wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/40999/
> ---
> 
> (Updated Dec. 8, 2015, 4:57 p.m.)
> 
> 
> Review request for mesos, Jie Yu and Michael Park.
> 
> 
> Bugs: MESOS-4067
> https://issues.apache.org/jira/browse/MESOS-4067
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Fixed flakiness in ReservationTest.ACLMultipleOperations.
> 
> 
> Diffs
> -
> 
>   src/tests/reservation_tests.cpp eccbb8f8a02b65b26f34e020e736afe0445a6d0d 
> 
> Diff: https://reviews.apache.org/r/40999/diff/
> 
> 
> Testing
> ---
> 
> `GTEST_FILTER="ReservationTest.ACLMultipleOperations" bin/mesos-tests.sh 
> --gtest_repeat=2000 --gtest_break_on_failure`
> 
> To reproduce the original test failure, you may have to set 
> `allocation_interval` to a shorter period (5ms, for example).
> 
> 
> Thanks,
> 
> Greg Mann
> 
>

Re: Review Request 40995: Added test cases for role behavior.

[Neil Conway]

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

(Updated Dec. 9, 2015, 5:53 a.m.)


Review request for mesos, Adam B, Alexander Rukletsov, Greg Mann, and Yong Qiao 
Wang.


Changes
---

Rebase.


Repository: mesos


Description
---

Added test cases for role behavior.


Diffs (updated)
-

  src/Makefile.am 9762f8567e32d70b8df2d694a1fef5c692fc730e 
  src/tests/reservation_tests.cpp eccbb8f8a02b65b26f34e020e736afe0445a6d0d 
  src/tests/role_tests.cpp PRE-CREATION 

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


Testing
---

make check.


Thanks,

Neil Conway

Re: Review Request 40271: [7/7] Added documentation for RESERVE, UNRESERVE, CREATE, and DESTROY authorization.

[Mesos ReviewBot]

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


Patch looks great!

Reviews applied: [39985, 39986, 39987, 39988, 39989, 40118, 40167, 40168, 
40169, 40255, 40256, 40271]

Passed command: export OS=ubuntu:14.04;export CONFIGURATION="--verbose";export 
COMPILER=gcc; ./support/docker_build.sh

- Mesos ReviewBot


On Dec. 8, 2015, 7:31 p.m., Greg Mann wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/40271/
> ---
> 
> (Updated Dec. 8, 2015, 7:31 p.m.)
> 
> 
> Review request for mesos, Jie Yu, Michael Park, and Neil Conway.
> 
> 
> Bugs: MESOS-3062 and MESOS-3065
> https://issues.apache.org/jira/browse/MESOS-3062
> https://issues.apache.org/jira/browse/MESOS-3065
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added documentation for RESERVE, UNRESERVE, CREATE, and DESTROY authorization.
> 
> 
> Diffs
> -
> 
>   docs/authorization.md f5ed75fcd0785fde38058917354fcf6d668dcccb 
>   docs/persistent-volume.md cf7a6bbb6044e460293dd66066df87aded0b4fb8 
>   docs/reservation.md de447664f10b1d73211805b725f2284b07c609f6 
> 
> Diff: https://reviews.apache.org/r/40271/diff/
> 
> 
> Testing
> ---
> 
> This is the last in a chain of 7 patches.
> 
> This documentation was tested by viewing with the Mesos Website Container 
> (https://github.com/mesosphere/mesos-website-container).
> 
> `make check` was run after all patches in the chain were applied.
> 
> 
> Thanks,
> 
> Greg Mann
> 
>

Re: Review Request 41092: Added CMake file for agent executable build.

[Joseph Wu]

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


Initial review.  I haven't run this code yet.

---

What do you think about using "agent" in the variable names and comments?


src/slave/CMakeLists.txt (line 39)


Nit: trim this line.



src/slave/CMakeLists.txt (line 42)


The pre-commit hooks will complain about this line.


- Joseph Wu


On Dec. 8, 2015, 9:57 a.m., Diana Arroyo wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/41092/
> ---
> 
> (Updated Dec. 8, 2015, 9:57 a.m.)
> 
> 
> Review request for mesos, Artem Harutyunyan, Alex Clemmer, Joris Van 
> Remoortere, and Joseph Wu.
> 
> 
> Bugs: MESOS-3843
> https://issues.apache.org/jira/browse/MESOS-3843
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added CMake file for agent executable build.
> 
> 
> Diffs
> -
> 
>   src/slave/CMakeLists.txt PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/41092/diff/
> 
> 
> Testing
> ---
> 
> Tested on Ubuntu and OSX.
> 
> 
> Thanks,
> 
> Diana Arroyo
> 
>