Re: Review Request 40532: Added notion of evictable task to RunTaskMessage.

[Mesos ReviewBot]

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


Patch looks great!

Reviews applied: [40339, 40529, 40532]

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

- Mesos ReviewBot


On Dec. 3, 2015, 4:35 a.m., Guangya Liu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/40532/
> ---
> 
> (Updated Dec. 3, 2015, 4:35 a.m.)
> 
> 
> Review request for mesos, Artem Harutyunyan, Joris Van Remoortere, Joseph Wu, 
> and Klaus Ma.
> 
> 
> Bugs: MESOS-3890
> https://issues.apache.org/jira/browse/MESOS-3890
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added notion of evictable task to RunTaskMessage.
> 
> 
> Diffs
> -
> 
>   src/messages/messages.proto 178d757a76f14829da6daab97ec678843717cf5a 
> 
> Diff: https://reviews.apache.org/r/40532/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Guangya Liu
> 
>

Re: Review Request 38878: Added test for the Subscribe->Subscribed workflow for the Executor HTTP API

[Anand Mazumdar]

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

(Updated Dec. 3, 2015, 6:48 a.m.)


Review request for mesos, Ben Mahler, Isabel Jimenez, and Vinod Kone.


Changes
---

Review comments from Vinod


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


Repository: mesos


Description
---

This change adds a basic test to validate the implementation for 
Subscribe->Subscribed workflow on the `api/v1/executor` endpoint on Agent.


Diffs (updated)
-

  src/tests/executor_http_api_tests.cpp 
fe9df1f4d68babaf0960a3b689ffbe60704b8ad5 

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


Testing
---

make check. Would add more agent recovery tests/executor reconnect tests in a 
separate patch.


Thanks,

Anand Mazumdar

Re: Review Request 38878: Added test for the Subscribe->Subscribed workflow for the Executor HTTP API

[Anand Mazumdar]

> On Dec. 2, 2015, 10:29 p.m., Vinod Kone wrote:
> > src/tests/executor_http_api_tests.cpp, line 757
> > 
> >
> > Hmm. this test is a bit convoluted. you start a pid based executor 
> > first, wait for its update to be received by the scheduler and then start a 
> > subscribe request from http executor?
> > 
> > i'm assuming you can't simply send a subscribe request because the 
> > slave wouldn't have an executor struct in its map. is it possible to stop 
> > the pid executor from starting at all? maybe by using testing 
> > isolator/containerizer?

Yes, The reasoning for doing this is as you specified. I could not send a 
`Subscribe` request directly as the slave did not have the `executor` struct in 
its map.

1. I had previously looked into using the `Containerizer` directly along with a 
`MockExecutor` but there did not seem to be currently an overload of 
`StartSlave(...)` that existed 
https://github.com/apache/mesos/blob/master/src/tests/mesos.hpp#L156 . Hence, 
we would still need a scheduler to do a `launchTask(...)` similar to what is 
being done here or am I missing something ?
2. We already have an existing instance in this file that also uses the above 
trick. 
https://github.com/apache/mesos/blob/master/src/tests/executor_http_api_tests.cpp#L324

That being said, I may be missing something here and I am open to alternate 
methods that can simplify this test.


- Anand


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


On Dec. 3, 2015, 6:48 a.m., Anand Mazumdar wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/38878/
> ---
> 
> (Updated Dec. 3, 2015, 6:48 a.m.)
> 
> 
> Review request for mesos, Ben Mahler, Isabel Jimenez, and Vinod Kone.
> 
> 
> Bugs: MESOS-3515
> https://issues.apache.org/jira/browse/MESOS-3515
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This change adds a basic test to validate the implementation for 
> Subscribe->Subscribed workflow on the `api/v1/executor` endpoint on Agent.
> 
> 
> Diffs
> -
> 
>   src/tests/executor_http_api_tests.cpp 
> fe9df1f4d68babaf0960a3b689ffbe60704b8ad5 
> 
> Diff: https://reviews.apache.org/r/38878/diff/
> 
> 
> Testing
> ---
> 
> make check. Would add more agent recovery tests/executor reconnect tests in a 
> separate patch.
> 
> 
> Thanks,
> 
> Anand Mazumdar
> 
>

Re: Review Request 40435: Fixed pointer alignment error in IP::create().

[Neil Conway]

> On Dec. 3, 2015, 4:26 a.m., Michael Park wrote:
> > Could you also create a clean-up patch to get rid of the unnecessary 
> > `struct` disambiguators in this file?

I kinda like leaving "struct" as-is, because we're interfacing with C-style 
APIs where "struct" is syntactically significant -- when reading C code, I 
expect to see "struct sockaddr", not just "sockaddr". What do you think?


- Neil


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


On Dec. 3, 2015, 6:28 a.m., Neil Conway wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/40435/
> ---
> 
> (Updated Dec. 3, 2015, 6:28 a.m.)
> 
> 
> Review request for mesos, Benjamin Bannier, Ben Mahler, Joris Van Remoortere, 
> Michael Park, and Niklas Nielsen.
> 
> 
> Bugs: MESOS-3939
> https://issues.apache.org/jira/browse/MESOS-3939
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> The previous code took the address of a "struct sockaddr", and then cast the
> resulting pointer to "struct sockaddr_storage *". The alignment requirements 
> for
> "struct sockaddr_storage *" are more strict than for "struct sockaddr *", and
> hence this code produced undefined behavior per ubsan in GCC 5.2.
> 
> Along the way, tweak the code to use reinterpret_cast rather than a C-style
> cast, and not to unnecessarily cast-away constness.
> 
> MESOS-3939.
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/3rdparty/stout/include/stout/ip.hpp 
> 3e506e1e94ee32e3a19870e1fd72d9968c5f8e67 
> 
> Diff: https://reviews.apache.org/r/40435/diff/
> 
> 
> Testing
> ---
> 
> "make check" on Linux/AMD64 + GCC 5.2 + ubsan; without fix, ubsan reports an 
> error. With fix, ubsan does not report (this) error.
> 
> 
> Thanks,
> 
> Neil Conway
> 
>

Re: Review Request 40435: Fixed pointer alignment error in IP::create().

[Neil Conway]

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

(Updated Dec. 3, 2015, 6:28 a.m.)


Review request for mesos, Benjamin Bannier, Ben Mahler, Joris Van Remoortere, 
Michael Park, and Niklas Nielsen.


Changes
---

Address mpark's comments.


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


Repository: mesos


Description
---

The previous code took the address of a "struct sockaddr", and then cast the
resulting pointer to "struct sockaddr_storage *". The alignment requirements for
"struct sockaddr_storage *" are more strict than for "struct sockaddr *", and
hence this code produced undefined behavior per ubsan in GCC 5.2.

Along the way, tweak the code to use reinterpret_cast rather than a C-style
cast, and not to unnecessarily cast-away constness.

MESOS-3939.


Diffs (updated)
-

  3rdparty/libprocess/3rdparty/stout/include/stout/ip.hpp 
3e506e1e94ee32e3a19870e1fd72d9968c5f8e67 

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


Testing
---

"make check" on Linux/AMD64 + GCC 5.2 + ubsan; without fix, ubsan reports an 
error. With fix, ubsan does not report (this) error.


Thanks,

Neil Conway

Re: Review Request 40632: WIP: Enabled oversubscribed resources for reservations in allocator.

[Mesos ReviewBot]

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


Bad patch!

Reviews applied: [40632]

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

Error:
 2015-12-03 05:57:49 URL:https://reviews.apache.org/r/40632/diff/raw/ 
[1068/1068] -> "40632.patch" [1]
error: patch failed: src/master/allocator/mesos/hierarchical.cpp:1031
error: src/master/allocator/mesos/hierarchical.cpp: patch does not apply
Failed to apply patch

- Mesos ReviewBot


On Dec. 3, 2015, 4:34 a.m., Guangya Liu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/40632/
> ---
> 
> (Updated Dec. 3, 2015, 4:34 a.m.)
> 
> 
> Review request for mesos and Klaus Ma.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> WIP: Enabled oversubscribed resources for reservations in allocator.
> 
> 
> Diffs
> -
> 
>   src/master/allocator/mesos/hierarchical.cpp 
> aee8ced1fbfec8cf30cb939ff67fadfc6b08f37a 
> 
> Diff: https://reviews.apache.org/r/40632/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Guangya Liu
> 
>

Re: Review Request 40529: Added helper function to get stateless resources.

[Guangya Liu]

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

(Updated 十二月 3, 2015, 4:35 a.m.)


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


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


Repository: mesos


Description
---

Added helper function to get stateless resources.


Diffs (updated)
-

  include/mesos/resources.hpp 9c8bfb0e7e1e40ca99b4cc75079e5b950630243d 
  include/mesos/v1/resources.hpp db87c992e2ec466cb1d4d17775bf6423a2fb15aa 
  src/common/resources.cpp 5a7981744726a0544435cbbd5007487a43a01211 
  src/tests/resources_tests.cpp ce47bac73937a6a608016f46121137d6116c1399 
  src/v1/resources.cpp d300842576b2fb8fe75f5b18699f23c1fd324717 

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


Testing
---


Thanks,

Guangya Liu

Re: Review Request 40532: Added notion of evictable task to RunTaskMessage.

[Guangya Liu]

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

(Updated 十二月 3, 2015, 4:35 a.m.)


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


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


Repository: mesos


Description
---

Added notion of evictable task to RunTaskMessage.


Diffs (updated)
-

  src/messages/messages.proto 178d757a76f14829da6daab97ec678843717cf5a 

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


Testing
---


Thanks,

Guangya Liu

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 十二月 3, 2015, 4:35 a.m.)


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


Changes
---

Rebase


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


Repository: mesos


Description
---

Added a flag to master to enable optimistic offers.


Diffs (updated)
-

  include/mesos/master/allocator.hpp 619ef01c3a7d640560653cfc1838dd09046d1da0 
  src/master/allocator/mesos/allocator.hpp 
97ee80726ad155917811265a983258b0165d3451 
  src/master/allocator/mesos/hierarchical.hpp 
1cd8d16661568010901e74705375e7719cdfb8a0 
  src/master/allocator/mesos/hierarchical.cpp 
a8f65b72488505afd3677ecdeb9b821ea27af7e0 
  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 
a45b3dd0a8237fdd080536cb9ab600e71ad939d3 
  src/tests/master_allocator_tests.cpp cba7c36471f93b678d94e1da0251a28a893696b1 
  src/tests/master_quota_tests.cpp c9d78cffc30539ce2d29360b231fcaf9e5b592ea 
  src/tests/persistent_volume_endpoints_tests.cpp 
ac46806a712113170b349a969a9f1132723116f0 
  src/tests/reservation_endpoints_tests.cpp 
142b7c61e08804417df639551926bf7fe3da9680 
  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 40632: WIP: Enabled oversubscribed resources for reservations in allocator.

[Guangya Liu]

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

(Updated 十二月 3, 2015, 4:34 a.m.)


Review request for mesos and Klaus Ma.


Repository: mesos


Description
---

WIP: Enabled oversubscribed resources for reservations in allocator.


Diffs
-

  src/master/allocator/mesos/hierarchical.cpp 
aee8ced1fbfec8cf30cb939ff67fadfc6b08f37a 

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


Testing
---


Thanks,

Guangya Liu

Re: Review Request 40532: Added notion of evictable task to RunTaskMessage.

[Guangya Liu]

> On 十二月 1, 2015, 1:31 a.m., Joseph Wu wrote:
> > src/messages/messages.proto, lines 326-329
> > 
> >
> > The design doc comment doesn't quite work without the context 
> > surrounding it.  :)
> > 
> > ---
> > Suggestion:
> > 
> > When a task is launched on reserved resources, this list contains some 
> > executors that have oversubscribed the same reserved resources.  The agent 
> > can evict these executors to make room for the task.
> > 
> > This list is always empty when the task launch does not involve 
> > reserved resources (oversubscribed reserved resources are considered 
> > "revocable resources").

Done, thanks Joseph!


- Guangya


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


On 十一月 25, 2015, 7:31 a.m., Guangya Liu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/40532/
> ---
> 
> (Updated 十一月 25, 2015, 7:31 a.m.)
> 
> 
> Review request for mesos, Artem Harutyunyan, Joris Van Remoortere, Joseph Wu, 
> and Klaus Ma.
> 
> 
> Bugs: MESOS-3890
> https://issues.apache.org/jira/browse/MESOS-3890
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added notion of evictable task to RunTaskMessage.
> 
> 
> Diffs
> -
> 
>   src/messages/messages.proto 178d757a76f14829da6daab97ec678843717cf5a 
> 
> Diff: https://reviews.apache.org/r/40532/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Guangya Liu
> 
>

Re: Review Request 40435: Fixed pointer alignment error in IP::create().

[Michael Park]

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

Ship it!


Could you also create a clean-up patch to get rid of the unnecessary `struct` 
disambiguators in this file?


3rdparty/libprocess/3rdparty/stout/include/stout/ip.hpp (lines 214 - 229)


Great comment!

Could you wrap some of the code segments in there in backquotes?



3rdparty/libprocess/3rdparty/stout/include/stout/ip.hpp (lines 230 - 233)


We should use `auto` here, since we already specify the type on the right.

```cpp
  const auto* addr = reinterpret_cast(&_storage);
  return create(*addr);
```



3rdparty/libprocess/3rdparty/stout/include/stout/ip.hpp (lines 241 - 244)


`auto` here as well:

```cpp
  const auto* addr = reinterpret_cast(&_storage);
  return IP(addr->sin_addr);
```



3rdparty/libprocess/3rdparty/stout/include/stout/ip.hpp (lines 247 - 249)


Not yours, but this fits in one line.


- Michael Park


On Dec. 3, 2015, 1:45 a.m., Neil Conway wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/40435/
> ---
> 
> (Updated Dec. 3, 2015, 1:45 a.m.)
> 
> 
> Review request for mesos, Benjamin Bannier, Ben Mahler, Joris Van Remoortere, 
> Michael Park, and Niklas Nielsen.
> 
> 
> Bugs: MESOS-3939
> https://issues.apache.org/jira/browse/MESOS-3939
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> The previous code took the address of a "struct sockaddr", and then cast the
> resulting pointer to "struct sockaddr_storage *". The alignment requirements 
> for
> "struct sockaddr_storage *" are more strict than for "struct sockaddr *", and
> hence this code produced undefined behavior per ubsan in GCC 5.2.
> 
> Along the way, tweak the code to use reinterpret_cast rather than a C-style
> cast, and not to unnecessarily cast-away constness.
> 
> MESOS-3939.
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/3rdparty/stout/include/stout/ip.hpp 
> 3e506e1e94ee32e3a19870e1fd72d9968c5f8e67 
> 
> Diff: https://reviews.apache.org/r/40435/diff/
> 
> 
> Testing
> ---
> 
> "make check" on Linux/AMD64 + GCC 5.2 + ubsan; without fix, ubsan reports an 
> error. With fix, ubsan does not report (this) error.
> 
> 
> Thanks,
> 
> Neil Conway
> 
>

Re: Review Request 40487: MESOS-3959: show slave hostname on executor page

[Mesos ReviewBot]

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


Bad review!

Reviews applied: []

Error:
 No reviewers specified. Please find a reviewer by asking on JIRA or the 
mailing list.

- Mesos ReviewBot


On Nov. 20, 2015, 5 p.m., Ian Babrou wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/40487/
> ---
> 
> (Updated Nov. 20, 2015, 5 p.m.)
> 
> 
> Review request for mesos.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> MESOS-3959: show slave hostname on executor page
> 
> 
> Diffs
> -
> 
>   src/webui/master/static/slave_executor.html 
> 7c66405090f46f89bdd29806a58c05dc76c0ad23 
> 
> Diff: https://reviews.apache.org/r/40487/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Ian Babrou
> 
>

Re: Review Request 40445: Added linter for license headers in some file types.

[Michael Park]

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



support/mesos-style.py (lines 126 - 127)


Why not `print`?


- Michael Park


On Dec. 3, 2015, 12:02 a.m., Benjamin Bannier wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/40445/
> ---
> 
> (Updated Dec. 3, 2015, 12:02 a.m.)
> 
> 
> Review request for mesos, Benjamin Hindman, Marco Massenzio, and Michael Park.
> 
> 
> Bugs: MESOS-3581
> https://issues.apache.org/jira/browse/MESOS-3581
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Review: https://reviews.apache.org/r/40445
> 
> 
> Diffs
> -
> 
>   support/mesos-style.py 66b45692c3c04f68358b63d52e4d87934f241bd7 
> 
> Diff: https://reviews.apache.org/r/40445/diff/
> 
> 
> Testing
> ---
> 
> Ran the a whole clean checkout through the linter with only one expected 
> failure (`3rdparty/libprocess/stout/tests/protobuf_tests.proto` which lacks a 
> license).
> 
> 
> NOTE TO THE COMMITTER
> -
> 
> Before committing this, it is probably a good idea to check the whole code 
> base again and fix any new files which do not follow the current license 
> style. The commits which originally fixed this were
> 
> * fa36917 (mesos),
> * dc23756 (stout), and
> * 3539b7a (libprocess).
> 
> 
> Thanks,
> 
> Benjamin Bannier
> 
>

Re: Review Request 40445: Added linter for license headers in some file types.

[Michael Park]

> On Dec. 2, 2015, 11:31 p.m., Michael Park wrote:
> > support/mesos-style.py, line 6
> > 
> >
> > Why the introduction of the `print` function? Can't we just use `print` 
> > and `.format`?
> 
> Benjamin Bannier wrote:
> This was some drive-by future proofing. 
> 
> In python-3.0 `print` can only be called as a function (`print("foo")`) 
> and the python-2.X syntax used around here (`print "foo"`) isn't available 
> anymore. The `print` function becomes available in python-2.6.0, but in the 
> shebang we pick whatever python interpreter is in `$PATH`. If we can drop 
> support for python-2.5 or earlier (not sure if it is still supported with 
> what is in here) we don't need to import it from `__future__`.

Can we take this on as a separate ticket/task? We have a bunch of python 
scripts, all of them seem to assume Python 2.


- Michael


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


On Dec. 3, 2015, 12:02 a.m., Benjamin Bannier wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/40445/
> ---
> 
> (Updated Dec. 3, 2015, 12:02 a.m.)
> 
> 
> Review request for mesos, Benjamin Hindman, Marco Massenzio, and Michael Park.
> 
> 
> Bugs: MESOS-3581
> https://issues.apache.org/jira/browse/MESOS-3581
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Review: https://reviews.apache.org/r/40445
> 
> 
> Diffs
> -
> 
>   support/mesos-style.py 66b45692c3c04f68358b63d52e4d87934f241bd7 
> 
> Diff: https://reviews.apache.org/r/40445/diff/
> 
> 
> Testing
> ---
> 
> Ran the a whole clean checkout through the linter with only one expected 
> failure (`3rdparty/libprocess/stout/tests/protobuf_tests.proto` which lacks a 
> license).
> 
> 
> NOTE TO THE COMMITTER
> -
> 
> Before committing this, it is probably a good idea to check the whole code 
> base again and fix any new files which do not follow the current license 
> style. The commits which originally fixed this were
> 
> * fa36917 (mesos),
> * dc23756 (stout), and
> * 3539b7a (libprocess).
> 
> 
> Thanks,
> 
> Benjamin Bannier
> 
>

Re: Review Request 40435: Fixed pointer alignment error in IP::create().

[Mesos ReviewBot]

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


Patch looks great!

Reviews applied: [40435]

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

- Mesos ReviewBot


On Dec. 3, 2015, 1:45 a.m., Neil Conway wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/40435/
> ---
> 
> (Updated Dec. 3, 2015, 1:45 a.m.)
> 
> 
> Review request for mesos, Benjamin Bannier, Ben Mahler, Joris Van Remoortere, 
> Michael Park, and Niklas Nielsen.
> 
> 
> Bugs: MESOS-3939
> https://issues.apache.org/jira/browse/MESOS-3939
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> The previous code took the address of a "struct sockaddr", and then cast the
> resulting pointer to "struct sockaddr_storage *". The alignment requirements 
> for
> "struct sockaddr_storage *" are more strict than for "struct sockaddr *", and
> hence this code produced undefined behavior per ubsan in GCC 5.2.
> 
> Along the way, tweak the code to use reinterpret_cast rather than a C-style
> cast, and not to unnecessarily cast-away constness.
> 
> MESOS-3939.
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/3rdparty/stout/include/stout/ip.hpp 
> 3e506e1e94ee32e3a19870e1fd72d9968c5f8e67 
> 
> Diff: https://reviews.apache.org/r/40435/diff/
> 
> 
> Testing
> ---
> 
> "make check" on Linux/AMD64 + GCC 5.2 + ubsan; without fix, ubsan reports an 
> error. With fix, ubsan does not report (this) error.
> 
> 
> Thanks,
> 
> Neil Conway
> 
>

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 十二月 3, 2015, 2:33 a.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 (updated)
-

  include/mesos/master/allocator.hpp 619ef01c3a7d640560653cfc1838dd09046d1da0 
  src/master/allocator/mesos/allocator.hpp 
97ee80726ad155917811265a983258b0165d3451 
  src/master/allocator/mesos/hierarchical.hpp 
1cd8d16661568010901e74705375e7719cdfb8a0 
  src/master/allocator/mesos/hierarchical.cpp 
a8f65b72488505afd3677ecdeb9b821ea27af7e0 
  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 
a45b3dd0a8237fdd080536cb9ab600e71ad939d3 
  src/tests/master_allocator_tests.cpp cba7c36471f93b678d94e1da0251a28a893696b1 
  src/tests/master_quota_tests.cpp c9d78cffc30539ce2d29360b231fcaf9e5b592ea 
  src/tests/persistent_volume_endpoints_tests.cpp 
ac46806a712113170b349a969a9f1132723116f0 
  src/tests/reservation_endpoints_tests.cpp 
142b7c61e08804417df639551926bf7fe3da9680 
  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 40445: Added linter for license headers in some file types.

[Mesos ReviewBot]

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


Patch looks great!

Reviews applied: [39590, 39591, 39592, 40445]

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

- Mesos ReviewBot


On Dec. 3, 2015, 12:02 a.m., Benjamin Bannier wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/40445/
> ---
> 
> (Updated Dec. 3, 2015, 12:02 a.m.)
> 
> 
> Review request for mesos, Benjamin Hindman, Marco Massenzio, and Michael Park.
> 
> 
> Bugs: MESOS-3581
> https://issues.apache.org/jira/browse/MESOS-3581
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Review: https://reviews.apache.org/r/40445
> 
> 
> Diffs
> -
> 
>   support/mesos-style.py 66b45692c3c04f68358b63d52e4d87934f241bd7 
> 
> Diff: https://reviews.apache.org/r/40445/diff/
> 
> 
> Testing
> ---
> 
> Ran the a whole clean checkout through the linter with only one expected 
> failure (`3rdparty/libprocess/stout/tests/protobuf_tests.proto` which lacks a 
> license).
> 
> 
> NOTE TO THE COMMITTER
> -
> 
> Before committing this, it is probably a good idea to check the whole code 
> base again and fix any new files which do not follow the current license 
> style. The commits which originally fixed this were
> 
> * fa36917 (mesos),
> * dc23756 (stout), and
> * 3539b7a (libprocess).
> 
> 
> Thanks,
> 
> Benjamin Bannier
> 
>

Re: Review Request 40888: Add volume driver plugin to mesos v1 proto.

[Guangya Liu]

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

Ship it!


Ship It!

- Guangya Liu


On 十二月 2, 2015, 11:48 p.m., Gilbert Song wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/40888/
> ---
> 
> (Updated 十二月 2, 2015, 11:48 p.m.)
> 
> 
> Review request for mesos, haosdent huang, Artem Harutyunyan, and Timothy Chen.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Add volume driver plugin to mesos v1 proto.
> 
> 
> Diffs
> -
> 
>   include/mesos/v1/mesos.proto 9acefd55603a5a4f3f08a879a380ff927fd1e0dd 
> 
> Diff: https://reviews.apache.org/r/40888/diff/
> 
> 
> Testing
> ---
> 
> make check(ubuntu14.04 + clang-3.6)
> 
> 
> Thanks,
> 
> Gilbert Song
> 
>

Re: Review Request 40487: MESOS-3959: show slave hostname on executor page

[Ben Mahler]

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


Hey Ian thanks for your patch!

While this is pretty trivial, would you mind sharing a screenshot before I 
commit this so that I know you've actually checked how this looks?

- Ben Mahler


On Nov. 20, 2015, 5 p.m., Ian Babrou wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/40487/
> ---
> 
> (Updated Nov. 20, 2015, 5 p.m.)
> 
> 
> Review request for mesos.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> MESOS-3959: show slave hostname on executor page
> 
> 
> Diffs
> -
> 
>   src/webui/master/static/slave_executor.html 
> 7c66405090f46f89bdd29806a58c05dc76c0ad23 
> 
> Diff: https://reviews.apache.org/r/40487/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Ian Babrou
> 
>

Re: Review Request 40435: Fixed pointer alignment error in IP::create().

[Neil Conway]

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

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


Review request for mesos, Benjamin Bannier, Ben Mahler, Joris Van Remoortere, 
Michael Park, and Niklas Nielsen.


Changes
---

Avoid duplicating code, per mpark. Add comment, per joris.


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


Repository: mesos


Description
---

The previous code took the address of a "struct sockaddr", and then cast the
resulting pointer to "struct sockaddr_storage *". The alignment requirements for
"struct sockaddr_storage *" are more strict than for "struct sockaddr *", and
hence this code produced undefined behavior per ubsan in GCC 5.2.

Along the way, tweak the code to use reinterpret_cast rather than a C-style
cast, and not to unnecessarily cast-away constness.

MESOS-3939.


Diffs (updated)
-

  3rdparty/libprocess/3rdparty/stout/include/stout/ip.hpp 
3e506e1e94ee32e3a19870e1fd72d9968c5f8e67 

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


Testing
---

"make check" on Linux/AMD64 + GCC 5.2 + ubsan; without fix, ubsan reports an 
error. With fix, ubsan does not report (this) error.


Thanks,

Neil Conway

Re: Review Request 40888: Add volume driver plugin to mesos v1 proto.

[Mesos ReviewBot]

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


Patch looks great!

Reviews applied: [40888]

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

- Mesos ReviewBot


On Dec. 2, 2015, 11:48 p.m., Gilbert Song wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/40888/
> ---
> 
> (Updated Dec. 2, 2015, 11:48 p.m.)
> 
> 
> Review request for mesos, haosdent huang, Artem Harutyunyan, and Timothy Chen.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Add volume driver plugin to mesos v1 proto.
> 
> 
> Diffs
> -
> 
>   include/mesos/v1/mesos.proto 9acefd55603a5a4f3f08a879a380ff927fd1e0dd 
> 
> Diff: https://reviews.apache.org/r/40888/diff/
> 
> 
> Testing
> ---
> 
> make check(ubuntu14.04 + clang-3.6)
> 
> 
> Thanks,
> 
> Gilbert Song
> 
>

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 Dec. 3, 2015, 1:05 a.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 (updated)
-

  include/mesos/master/allocator.hpp 619ef01c3a7d640560653cfc1838dd09046d1da0 
  src/master/allocator/mesos/allocator.hpp 
97ee80726ad155917811265a983258b0165d3451 
  src/master/allocator/mesos/hierarchical.hpp 
1cd8d16661568010901e74705375e7719cdfb8a0 
  src/master/allocator/mesos/hierarchical.cpp 
a8f65b72488505afd3677ecdeb9b821ea27af7e0 
  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 
a45b3dd0a8237fdd080536cb9ab600e71ad939d3 
  src/tests/master_allocator_tests.cpp cba7c36471f93b678d94e1da0251a28a893696b1 
  src/tests/master_quota_tests.cpp c9d78cffc30539ce2d29360b231fcaf9e5b592ea 
  src/tests/reservation_endpoints_tests.cpp 
142b7c61e08804417df639551926bf7fe3da9680 
  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 40339: Added a flag to master to enable oversubscription for reservations.

[Guangya Liu]

> On Dec. 1, 2015, 1:03 a.m., Joseph Wu wrote:
> > include/mesos/master/allocator.hpp, line 96
> > 
> >
> > Suggestion: rename the variable to `enableReservationOversubscription`.

Done.


> On Dec. 1, 2015, 1:03 a.m., Joseph Wu wrote:
> > src/master/flags.cpp, lines 439-443
> > 
> >
> > Suggestion:
> > 
> > Whether to enable oversubscription for reservations. If enabled, the 
> > allocator will optimistically offer reserved-but-unallocated resources to 
> > other frameworks as revocable resources.  Tasks running on revocable 
> > resources will be evicted when the reserved resources are allocated.

Done, thanks Joesph!


- Guangya


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


On Nov. 24, 2015, 5:40 a.m., Guangya Liu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/40339/
> ---
> 
> (Updated Nov. 24, 2015, 5:40 a.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 
> 2a21364fdcaa4ec5e5567b9f367c14a1579b9a49 
>   src/master/allocator/mesos/hierarchical.cpp 
> aee8ced1fbfec8cf30cb939ff67fadfc6b08f37a 
>   src/master/flags.hpp 9af6c68eef6bcf39d5776809fab6c66dc95da6b2 
>   src/master/flags.cpp 2d58fcd56317897786a8b832c4ccf7f3d7b566e5 
>   src/master/master.cpp 370980edfc80d1e52134fdaf3ce49177b6528b02 
>   src/tests/allocator.hpp c7670525765491fe931a4ee38446fa7e9d79af42 
>   src/tests/hierarchical_allocator_tests.cpp 
> a45b3dd0a8237fdd080536cb9ab600e71ad939d3 
>   src/tests/master_allocator_tests.cpp 
> cba7c36471f93b678d94e1da0251a28a893696b1 
>   src/tests/master_quota_tests.cpp 330e591f81c7ece7f401042ad159bd6b55881a84 
>   src/tests/reservation_endpoints_tests.cpp 
> f4e332327049944000baccd3e607201240a8c5f4 
>   src/tests/reservation_tests.cpp 1d0a65d5b4c2cb03e49f302176084ef5d602569f 
>   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 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 Dec. 3, 2015, 1:01 a.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 (updated)
-

  include/mesos/master/allocator.hpp 619ef01c3a7d640560653cfc1838dd09046d1da0 
  src/master/allocator/mesos/allocator.hpp 
97ee80726ad155917811265a983258b0165d3451 
  src/master/allocator/mesos/hierarchical.hpp 
1cd8d16661568010901e74705375e7719cdfb8a0 
  src/master/allocator/mesos/hierarchical.cpp 
a8f65b72488505afd3677ecdeb9b821ea27af7e0 
  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 
a45b3dd0a8237fdd080536cb9ab600e71ad939d3 
  src/tests/master_allocator_tests.cpp cba7c36471f93b678d94e1da0251a28a893696b1 
  src/tests/master_quota_tests.cpp c9d78cffc30539ce2d29360b231fcaf9e5b592ea 
  src/tests/reservation_endpoints_tests.cpp 
142b7c61e08804417df639551926bf7fe3da9680 
  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 40266: Libprocess Reinitialization: Cleanup SocketManager along side ProcessManager.

[Joseph Wu]

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

(Updated Dec. 2, 2015, 4:50 p.m.)


Review request for mesos, Artem Harutyunyan and Joris Van Remoortere.


Changes
---

Addressed one comment.  Found and fixed a bug while doing so.


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


Repository: mesos


Description
---

The `SocketManager` and `ProcessManager` are highly inter-dependent, which 
requires some untangling in `process::finalize`.

* Logic originally found in `~ProcessManager` has been split into 
`ProcessManager::finalize` due to what happens during cleanup.
* Some additional cleanup was added for dangling pointers.
* The future from `__s__->accept()` must be explicitly discarded as libevent 
does not detect a locally closed socket.


Diffs (updated)
-

  3rdparty/libprocess/src/process.cpp a7af4671efa2f370137ed4a749e5247c91e6f95e 

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


Testing
---

`make check` (libev)
`make check` (--enable-libevent --enable-ssl)


Thanks,

Joseph Wu

Re: Review Request 40266: Libprocess Reinitialization: Cleanup SocketManager along side ProcessManager.

[Joseph Wu]

> On Dec. 2, 2015, 8:08 a.m., Benjamin Bannier wrote:
> > 3rdparty/libprocess/src/process.cpp, line 2200
> > 
> >
> > I find adding another manual iteration index manipulation here makes 
> > this even harder to read (e.g., do we really iterate over all elements? Are 
> > there assumptions about ordering (hopefully not)?, ...). 
> > 
> > You could e.g., factor out a `synchronized` helper to get the next 
> > not-ignored element (or a `nullptr` if nothing is left); the whole existing 
> > loop could then collapse to
> > 
> > while (true) {
> >   ProcessBase* process = next_cleanup(processes));
> >   if (!process) {
> > break;
> >   }
> >   process::terminate(process, false);
> >   process::wait(process);
> > }
> > 
> > This would also make it clear that we intent over all not-ignored 
> > processes (which currently is implicit through `wait` removing processes 
> > one after the other, and `processes` not containing any `nullptr` elements).
> > 
> > The helper `next_cleanup` can be implemented without querying size 
> > information, e.g., it could iterate `processes` until the process doesn't 
> > pattern-match with ignored processes.

This comment actually helped me uncover another edge case in the finalization 
code :)  
As it turns out, the current patch **is dependent** on ordering.

---
The existing `process::finalize` (prior to this patch) ends up dereferencing a 
terminated process.  But we don't segfault because the terminated process's 
pointer is never deleted:
1) Run a test like `ProcessTest.Http1`.
2) This leaves two processes behind (bad test cleanup?), a client 
`ConnectionProcess` and an `HttpProxy` on the server side.
3) After the libprocess tests, we call `process::finalize`.
4) We delete in alphabetical order, because `std::map` sorts the processes 
alphabetically.
4) It just so happens that `__gc__` is always the first to die.  This means no 
processes spawned via `spawn(..., true)` will be deleted.
5) The `HttpProxy` (named `__http__`) is deleted next.  We leak this pointer :(
6) The `ConnectionProcess` (named `__http_connection__`) is deleted next.  This 
also fires `process::internal::decode_recv`, which cleans up the socket.
7) During socket cleanup, we terminate the associated `HttpProxy` (which was 
terminated in step 5).  Termination actually requires a dereference (i.e. 
process->pid).  This works because we leaked the pointer.


- Joseph


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


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

Re: Review Request 39989: [5/5] Added framework authorization for dynamic reservation.

[Alexander Rukletsov]

> On Dec. 2, 2015, 4:05 p.m., Alexander Rukletsov wrote:
> > src/master/master.cpp, lines 3041-3043
> > 
> >
> > We validate later on that in `principal` is `None`, reserve is aborted. 
> > IIUC, if `FrameworkInfo` does not specify principal, it cannot reserve 
> > resources. So the question is: do we need to check this first and only then 
> > proceed with authz?
> 
> Jie Yu wrote:
> Alex, from the impl. perspective, we thought about that in the earlier 
> iteration, and found that it's quite difficult to do it cleanly given the 
> current structure of the code. We want validation to be done at the same 
> place (`Master::_accept`).
> 
> ALso, we had some discussion on whether 'principal' in ReservationInfo 
> needs to be required or not (MESOS-3064). In the future, we might want to 
> make it 'optional' so that a framework without principal can also reserve 
> resources (it's reserved resources can be unreserved by anyone).
> 
> So I would suggest we keep the current structure and add a comment here 
> saying that: currently, if framework's principal is not specified, the 
> operation validation will fail in `_accept` even thought authorization might 
> succeed.
> 
> Greg Mann wrote:
> I added a comment along the lines of Jie's suggestion.

Thanks a lot for the explanation and pointing me to the discussions in the 
ticket. It is very valuable for me as we design authz for quota now.


- Alexander


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


On Dec. 2, 2015, 7:55 p.m., Greg Mann wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/39989/
> ---
> 
> (Updated Dec. 2, 2015, 7:55 p.m.)
> 
> 
> Review request for mesos, Adam B, Jie Yu, Michael Park, and Till Toenshoff.
> 
> 
> Bugs: MESOS-3062
> https://issues.apache.org/jira/browse/MESOS-3062
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added framework authorization for dynamic reservation.
> Note: this review is continued from https://reviews.apache.org/r/37127/
> 
> 
> Diffs
> -
> 
>   src/master/master.cpp b918ae4a0e7dc3cd41165fc4b683ae7b6f031821 
>   src/tests/reservation_tests.cpp 15d180f92ec0aea99e6f3a7d0b505c62bd207b61 
> 
> Diff: https://reviews.apache.org/r/39989/diff/
> 
> 
> Testing
> ---
> 
> This is the fifth in a chain of 5 patches. New reservation tests were added 
> to `reservation_tests.cpp` to validate the authentication of framework 
> reserve and unreserve operations using ACLs. `make check` was run to test 
> after all patches were applied.
> 
> 
> Thanks,
> 
> Greg Mann
> 
>

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

[Mesos ReviewBot]

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


Bad patch!

Reviews applied: [40730, 40732, 40731]

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

Error:
 2015-12-03 00:14:47 URL:https://reviews.apache.org/r/40731/diff/raw/ 
[7279/7279] -> "40731.patch" [1]
error: patch failed: src/tests/reservation_tests.cpp:161
error: src/tests/reservation_tests.cpp: patch does not apply
Failed to apply patch

- Mesos ReviewBot


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 40445: Added linter for license headers in some file types.

[Benjamin Bannier]

> On Dec. 2, 2015, 11:31 p.m., Michael Park wrote:
> > support/mesos-style.py, line 6
> > 
> >
> > Why the introduction of the `print` function? Can't we just use `print` 
> > and `.format`?

This was some drive-by future proofing. 

In python-3.0 `print` can only be called as a function (`print("foo")`) and the 
python-2.X syntax used around here (`print "foo"`) isn't available anymore. The 
`print` function becomes available in python-2.6.0, but in the shebang we pick 
whatever python interpreter is in `$PATH`. If we can drop support for 
python-2.5 or earlier (not sure if it is still supported with what is in here) 
we don't need to import it from `__future__`.


> On Dec. 2, 2015, 11:31 p.m., Michael Park wrote:
> > support/mesos-style.py, lines 83-84
> > 
> >
> > Is this meant to be commented out?

I removed this intentionally as it was purely `cpplint` specific (but missed 
nuking the commented code). The idea here was that as `mesos-style` wraps other 
checks we should not need to act like a thin wrapper around `cpplint` anymore. 
I added a more generic header below.


> On Dec. 2, 2015, 11:31 p.m., Michael Park wrote:
> > support/mesos-style.py, lines 95-96
> > 
> >
> > Why the change here?

The old regex matched interesting lines by just looking for a `:` somewhere 
which let the total `cpplint` error count slip through (it was useful for a 
thin wrapper around `cpplint`). We now calculate and output our own, combined 
error count so we should remove the one from `cpplint`.

I replaced this construct with a simple regex.


> On Dec. 2, 2015, 11:31 p.m., Michael Park wrote:
> > support/mesos-style.py, line 112
> > 
> >
> > Do we allow arbitrary whitespace `\s`? or do we want to lock it down to 
> > exactly one space character?

Good point, this can be tightened.


- Benjamin


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


On Dec. 3, 2015, 12:02 a.m., Benjamin Bannier wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/40445/
> ---
> 
> (Updated Dec. 3, 2015, 12:02 a.m.)
> 
> 
> Review request for mesos, Benjamin Hindman, Marco Massenzio, and Michael Park.
> 
> 
> Bugs: MESOS-3581
> https://issues.apache.org/jira/browse/MESOS-3581
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Review: https://reviews.apache.org/r/40445
> 
> 
> Diffs
> -
> 
>   support/mesos-style.py 66b45692c3c04f68358b63d52e4d87934f241bd7 
> 
> Diff: https://reviews.apache.org/r/40445/diff/
> 
> 
> Testing
> ---
> 
> Ran the a whole clean checkout through the linter with only one expected 
> failure (`3rdparty/libprocess/stout/tests/protobuf_tests.proto` which lacks a 
> license).
> 
> 
> NOTE TO THE COMMITTER
> -
> 
> Before committing this, it is probably a good idea to check the whole code 
> base again and fix any new files which do not follow the current license 
> style. The commits which originally fixed this were
> 
> * fa36917 (mesos),
> * dc23756 (stout), and
> * 3539b7a (libprocess).
> 
> 
> Thanks,
> 
> Benjamin Bannier
> 
>

Re: Review Request 40445: Added linter for license headers in some file types.

[Benjamin Bannier]

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

(Updated Dec. 3, 2015, 12:02 a.m.)


Review request for mesos, Benjamin Hindman, Marco Massenzio, and Michael Park.


Changes
---

Addressed review comments.


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


Repository: mesos


Description (updated)
---

Review: https://reviews.apache.org/r/40445


Diffs (updated)
-

  support/mesos-style.py 66b45692c3c04f68358b63d52e4d87934f241bd7 

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


Testing
---

Ran the a whole clean checkout through the linter with only one expected 
failure (`3rdparty/libprocess/stout/tests/protobuf_tests.proto` which lacks a 
license).


NOTE TO THE COMMITTER
-

Before committing this, it is probably a good idea to check the whole code base 
again and fix any new files which do not follow the current license style. The 
commits which originally fixed this were

* fa36917 (mesos),
* dc23756 (stout), and
* 3539b7a (libprocess).


Thanks,

Benjamin Bannier

Review Request 40888: Add volume driver plugin to mesos v1 proto.

[Gilbert Song]

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

Review request for mesos, haosdent huang, Artem Harutyunyan, and Timothy Chen.


Repository: mesos


Description
---

Add volume driver plugin to mesos v1 proto.


Diffs
-

  include/mesos/v1/mesos.proto 9acefd55603a5a4f3f08a879a380ff927fd1e0dd 

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


Testing
---

make check(ubuntu14.04 + clang-3.6)


Thanks,

Gilbert Song

Re: Review Request 40884: Environment variable: Implemented passing user taskinfo and docker image env var for docker containerizer.

[Mesos ReviewBot]

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


Patch looks great!

Reviews applied: [40838, 40884]

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

- Mesos ReviewBot


On Dec. 2, 2015, 10:16 p.m., Gilbert Song wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/40884/
> ---
> 
> (Updated Dec. 2, 2015, 10:16 p.m.)
> 
> 
> Review request for mesos, Artem Harutyunyan and Timothy Chen.
> 
> 
> Bugs: MESOS-4051
> https://issues.apache.org/jira/browse/MESOS-4051
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Environment variable: Implemented passing user taskinfo and docker image env 
> var for docker containerizer.
> 
> 
> Diffs
> -
> 
>   src/slave/containerizer/docker.cpp 2d2dd4e0df36207c5f3cbb4fe2c50df51c0f3e9e 
> 
> Diff: https://reviews.apache.org/r/40884/diff/
> 
> 
> Testing
> ---
> 
> make check (ubuntu14.04 + clang-3.6)
> 
> 
> Thanks,
> 
> Gilbert Song
> 
>

Re: Review Request 40372: Relocate containerizer isolator files under mesos containerizer.

[Jie Yu]

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

Ship it!



src/tests/containerizer/mesos_containerizer_tests.cpp (lines 45 - 48)


why this change?


- Jie Yu


On Dec. 2, 2015, 10:46 p.m., Gilbert Song wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/40372/
> ---
> 
> (Updated Dec. 2, 2015, 10:46 p.m.)
> 
> 
> Review request for mesos, Artem Harutyunyan, Jie Yu, and Timothy Chen.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Relocate containerizer isolator files under mesos containerizer.
> 
> 
> Diffs
> -
> 
>   src/CMakeLists.txt cfe9d26c45ba42852fd1af958549954e7b04d448 
>   src/Makefile.am 0409491b92c9720d60ad76fdbc2edff554fb4965 
>   src/slave/containerizer/isolator.hpp  
>   src/slave/containerizer/isolator.cpp 
> 6cc9b8342138ba76581647c95b1b9bac7e12f64f 
>   src/slave/containerizer/mesos/isolators/cgroups/cpushare.hpp 
> 558071667f6ec9292af6aa571457f4a125ad8eea 
>   src/slave/containerizer/mesos/isolators/cgroups/mem.hpp 
> 9d2a48a47b0628f70a910f9a1e507c4b45032667 
>   src/slave/containerizer/mesos/isolators/cgroups/perf_event.hpp 
> b8809c4040d02a253b4c10af1d0dcd28e816a84c 
>   src/slave/containerizer/mesos/isolators/filesystem/linux.hpp 
> 8f7ff39bdfcfc6e6e443959dbdc145d9ebe92685 
>   src/slave/containerizer/mesos/isolators/filesystem/posix.hpp 
> 3241dbc3dd4283d06f165fd0946418bbf6b88eb5 
>   src/slave/containerizer/mesos/isolators/filesystem/shared.hpp 
> 6b6c8ce755b40ed2c827f6d4dab1af2a158bb2f9 
>   src/slave/containerizer/mesos/isolators/namespaces/pid.hpp 
> 7ad6858692185d04b00fabb21c9fbd3ed6e39225 
>   src/slave/containerizer/mesos/isolators/network/port_mapping.hpp 
> eaea644cc93601af9cf47ba3ac4b2b1c31a00f7a 
>   src/slave/containerizer/mesos/isolators/posix.hpp 
> d683f74f3e2abfcf371fee63a61ff5e28d1a7526 
>   src/slave/containerizer/mesos/isolators/posix/disk.hpp 
> 09836763287dfa6870f187d261bc22c24438dd97 
>   src/tests/containerizer/isolator.hpp 
> e49bc3ab0323ae1cf2cfb81c53f5ee0f57dd0b06 
>   src/tests/containerizer/mesos_containerizer_tests.cpp 
> fe679354d04d68b68e168cd8c4eab23898f6532f 
> 
> Diff: https://reviews.apache.org/r/40372/diff/
> 
> 
> Testing
> ---
> 
> make check(ubuntu14.04 + clang-3.6)
> 
> 
> Thanks,
> 
> Gilbert Song
> 
>

Re: Review Request 40445: Added linter for license headers in some file types.

[Michael Park]

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



support/mesos-style.py (line 5)


Why the introduction of the `print` function? Can't we just use `print` and 
`.format`?



support/mesos-style.py (lines 63 - 64)


Is this meant to be commented out?



support/mesos-style.py (lines 74 - 75)


Why the change here?



support/mesos-style.py (line 91)


Do we allow arbitrary whitespace `\s`? or do we want to lock it down to 
exactly one space character?


- Michael Park


On Dec. 2, 2015, 11:18 a.m., Benjamin Bannier wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/40445/
> ---
> 
> (Updated Dec. 2, 2015, 11:18 a.m.)
> 
> 
> Review request for mesos, Benjamin Hindman, Marco Massenzio, and Michael Park.
> 
> 
> Bugs: MESOS-3581
> https://issues.apache.org/jira/browse/MESOS-3581
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added linter for license headers in some file types.
> 
> 
> Diffs
> -
> 
>   support/mesos-style.py 66b45692c3c04f68358b63d52e4d87934f241bd7 
> 
> Diff: https://reviews.apache.org/r/40445/diff/
> 
> 
> Testing
> ---
> 
> Ran the a whole clean checkout through the linter with only one expected 
> failure (`3rdparty/libprocess/stout/tests/protobuf_tests.proto` which lacks a 
> license).
> 
> 
> NOTE TO THE COMMITTER
> -
> 
> Before committing this, it is probably a good idea to check the whole code 
> base again and fix any new files which do not follow the current license 
> style. The commits which originally fixed this were
> 
> * fa36917 (mesos),
> * dc23756 (stout), and
> * 3539b7a (libprocess).
> 
> 
> Thanks,
> 
> Benjamin Bannier
> 
>

Re: Review Request 40873: SimpleRegistryPullerTest: Moved blob response preparation to the top.

[Jojy Varghese]

> On Dec. 2, 2015, 4:40 p.m., Anand Mazumdar wrote:
> > @jojy, Do we know why preparing the response was taking so long that was 
> > causing the socket to timeout previously ?
> 
> Jojy Varghese wrote:
> @anand: The observed behavior is that the server socket gets a RST. This 
> is ususally when the peer side of the socket is closed. Also observed is that 
> there are asycnhronous socket close being done ( i think from the HTTP 
> layer). This was observed when I added a log inside the dtor of the socket 
> (socket.hpp: L103). For failed test cases, I saw an extra socket close. This 
> change reduces the time between the peer interactions. This is not a 
> "solution" of the problem but an effort to eliminate possible false alarms 
> and be able to focus on the real issue. Since its very difficult to reproduce 
> this issue predictably, we need to elimitate all red herrings.
> 
> Anand Mazumdar wrote:
> Thanks for the reply @jojy. 
> 
> Do we know why the peer side of the socket was closed i.e. it got a RST 
> and why the peer side of the socket was closed in the first place ? Can you 
> also help me understand how are these false alarms ? To me, it suggests a bug 
> inside the `RegistryClient` itself and we are finding a workaround in the 
> tests to mask it, no ?

This fix addresses couple of things:

- Eliminates the assumption that a socket accept call will be invoked before 
the client sends the request.
- Eliminates the assumption of any timeout dependencies of http client socket 
on the processing at the server side.
  
  Assumption no. 1 is clearly a bug in the test that this fix addresses.
  
 @anand: We discussed about this and I agree that we could be hiding an 
underlying issue at the http layer. But we want to address the real issue and 
not red herring. This test case failure is just that. Moreover, these fixes are 
improvements that the test need anyways to eliminate the above mentioned 
assumptions.


- Jojy


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


On Dec. 2, 2015, 4:26 p.m., Jojy Varghese wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/40873/
> ---
> 
> (Updated Dec. 2, 2015, 4:26 p.m.)
> 
> 
> Review request for mesos and Timothy Chen.
> 
> 
> Bugs: MESOS-4025
> https://issues.apache.org/jira/browse/MESOS-4025
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> By moving the preperation task above any test task avoids socket waiting for 
> the response to be prepared. This change has eliminated socket timeouts seen 
> sometimes.
> 
> 
> Diffs
> -
> 
>   src/tests/containerizer/provisioner_docker_tests.cpp 
> c63bf53fee40ef12536a16e11f4d5224c4e4278e 
> 
> Diff: https://reviews.apache.org/r/40873/diff/
> 
> 
> Testing
> ---
> 
> make check (600 count).
> 
> 
> Thanks,
> 
> Jojy Varghese
> 
>

Re: Review Request 40788: Added lambda function in resources.cpp.

[Greg Mann]

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

(Updated Dec. 2, 2015, 11:19 p.m.)


Review request for mesos, Michael Park and Neil Conway.


Changes
---

Extended changes to src/v1/resources.cpp


Repository: mesos


Description
---

Added lambda function in resources.cpp.


Diffs (updated)
-

  src/common/resources.cpp 0a657487c3c544068855146a39238f35ecb44f6a 
  src/v1/resources.cpp 90830c9a09aaf2c5514d9a40dbe9bf147718d132 

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


Testing
---

`make check`


Thanks,

Greg Mann

Re: Review Request 39988: [4/5] Added authorization for dynamic reservation master endpoints.

[Greg Mann]

> On Dec. 2, 2015, 3:05 p.m., Alexander Rukletsov wrote:
> > src/master/http.cpp, lines 1035-1038
> > 
> >
> > Let's leave a comment here, that `principal` matches 
> > `reservation().principal()` for each resource in 
> > `operation.reserve().resources()`, hence it's OK to authorize for 
> > `principal` and use `reservation().principal()` in `unreserve()`. Maybe a 
> > symmetrical comment in `unreserve()` path would also make sense.
> > 
> > Maybe if you validate before authorizing it will be more easy to 
> > understand?
> 
> Jie Yu wrote:
> +1 on validate before authorizing. That could save us a big comment here.

I changed the order of validation with respect to authorization; hopefully that 
does enough to improve readability. Let me know if you think we would benefit 
from an additional comment when we validate, we can add one if necessary.


- Greg


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


On Dec. 2, 2015, 10:47 p.m., Greg Mann wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/39988/
> ---
> 
> (Updated Dec. 2, 2015, 10:47 p.m.)
> 
> 
> Review request for mesos, Adam B, Jie Yu, Michael Park, and Till Toenshoff.
> 
> 
> Bugs: MESOS-3062
> https://issues.apache.org/jira/browse/MESOS-3062
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added authorization for dynamic reservation master endpoints.
> Note: this review is continued from https://reviews.apache.org/r/37126/
> 
> 
> Diffs
> -
> 
>   src/master/http.cpp 9d729ef7f7d7ad6185934648f833e4f8a4f0a123 
>   src/tests/reservation_endpoints_tests.cpp 
> f30ff8bc6a3e9773437fa7fd7c8f569b7d7e2d9d 
> 
> Diff: https://reviews.apache.org/r/39988/diff/
> 
> 
> Testing
> ---
> 
> This is the fourth in a chain of 5 patches. Added new reservation endpoints 
> tests to validate authorization of reserve and unreserve operations using 
> ACLs. `make check` was run to test after all patches were applied.
> 
> 
> Thanks,
> 
> Greg Mann
> 
>

Re: Review Request 39988: [4/5] Added authorization for dynamic reservation master endpoints.

[Greg Mann]

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

(Updated Dec. 2, 2015, 10:47 p.m.)


Review request for mesos, Adam B, Jie Yu, Michael Park, and Till Toenshoff.


Changes
---

Addressed comment, changed order of validation with respect to authorization.


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


Repository: mesos


Description
---

Added authorization for dynamic reservation master endpoints.
Note: this review is continued from https://reviews.apache.org/r/37126/


Diffs (updated)
-

  src/master/http.cpp 9d729ef7f7d7ad6185934648f833e4f8a4f0a123 
  src/tests/reservation_endpoints_tests.cpp 
f30ff8bc6a3e9773437fa7fd7c8f569b7d7e2d9d 

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


Testing
---

This is the fourth in a chain of 5 patches. Added new reservation endpoints 
tests to validate authorization of reserve and unreserve operations using ACLs. 
`make check` was run to test after all patches were applied.


Thanks,

Greg Mann

Re: Review Request 40372: Relocate containerizer isolator files under mesos containerizer.

[Gilbert Song]

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

(Updated Dec. 2, 2015, 2:46 p.m.)


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


Repository: mesos


Description
---

Relocate containerizer isolator files under mesos containerizer.


Diffs (updated)
-

  src/CMakeLists.txt cfe9d26c45ba42852fd1af958549954e7b04d448 
  src/Makefile.am 0409491b92c9720d60ad76fdbc2edff554fb4965 
  src/slave/containerizer/isolator.hpp  
  src/slave/containerizer/isolator.cpp 6cc9b8342138ba76581647c95b1b9bac7e12f64f 
  src/slave/containerizer/mesos/isolators/cgroups/cpushare.hpp 
558071667f6ec9292af6aa571457f4a125ad8eea 
  src/slave/containerizer/mesos/isolators/cgroups/mem.hpp 
9d2a48a47b0628f70a910f9a1e507c4b45032667 
  src/slave/containerizer/mesos/isolators/cgroups/perf_event.hpp 
b8809c4040d02a253b4c10af1d0dcd28e816a84c 
  src/slave/containerizer/mesos/isolators/filesystem/linux.hpp 
8f7ff39bdfcfc6e6e443959dbdc145d9ebe92685 
  src/slave/containerizer/mesos/isolators/filesystem/posix.hpp 
3241dbc3dd4283d06f165fd0946418bbf6b88eb5 
  src/slave/containerizer/mesos/isolators/filesystem/shared.hpp 
6b6c8ce755b40ed2c827f6d4dab1af2a158bb2f9 
  src/slave/containerizer/mesos/isolators/namespaces/pid.hpp 
7ad6858692185d04b00fabb21c9fbd3ed6e39225 
  src/slave/containerizer/mesos/isolators/network/port_mapping.hpp 
eaea644cc93601af9cf47ba3ac4b2b1c31a00f7a 
  src/slave/containerizer/mesos/isolators/posix.hpp 
d683f74f3e2abfcf371fee63a61ff5e28d1a7526 
  src/slave/containerizer/mesos/isolators/posix/disk.hpp 
09836763287dfa6870f187d261bc22c24438dd97 
  src/tests/containerizer/isolator.hpp e49bc3ab0323ae1cf2cfb81c53f5ee0f57dd0b06 
  src/tests/containerizer/mesos_containerizer_tests.cpp 
fe679354d04d68b68e168cd8c4eab23898f6532f 

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


Testing
---

make check(ubuntu14.04 + clang-3.6)


Thanks,

Gilbert Song

Re: Review Request 38877: Added functionality for Subscribe/Subscribed workflow for HTTP executors

[Anand Mazumdar]

> On Dec. 2, 2015, 10:24 p.m., Vinod Kone wrote:
> > I see a very basic test in the next review. Are you planning to write more 
> > comprehensive tests? Is that plan to templatize slave recovery tests for 
> > both pid based and http executors?

For now, I added just a basic test for testing the subscribe->subscribed 
workflow but I would surely be adding more tests around recovery. Currently, 
the existing Slave recovery tests use the `Executor/ExecutorDriver` to do all 
the heavy lifting. Until, have a HTTP based executor library it won't be that 
easy to templatize the slave recovery tests. Hence, for now, I would go ahead 
and add some basic tests in `src/tests/executor_http_apt_tests.cpp`. Does that 
sound reasonable to you ?


> On Dec. 2, 2015, 10:24 p.m., Vinod Kone wrote:
> > src/slave/slave.cpp, lines 2414-2415
> > 
> >
> > I see that 'RECOVERING' is a possible state in `registerExecutor()`. Is 
> > that not possible for http executors?

Yes, the agent cannot be in `state == RECOVERING` for HTTP based executors 
since we already check it inside the `executor(...)` call back here:
https://github.com/apache/mesos/blob/master/src/slave/http.cpp#L218

Hence, it should be fine to fail the assert and crash here.


- Anand


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


On Dec. 2, 2015, 10:41 p.m., Anand Mazumdar wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/38877/
> ---
> 
> (Updated Dec. 2, 2015, 10:41 p.m.)
> 
> 
> Review request for mesos, Ben Mahler, Isabel Jimenez, and Vinod Kone.
> 
> 
> Bugs: MESOS-3515
> https://issues.apache.org/jira/browse/MESOS-3515
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This change adds the functionality for executors to `Subscribe` via the 
> `api/v1/executor` endpoint. It also stores a marker file as part of the 
> `Subscribe` call if framework `checkpointing` is enabled. This can then be 
> used by the agent when recovering to wait for reconnecting back with the 
> executor.
> 
> 
> Diffs
> -
> 
>   src/slave/http.cpp c3247f17e9faed32a46d3ab9ee83c399cd2c8d5e 
>   src/slave/slave.hpp 5ee133ae52998d05c8afabb2ade7095e363c75c5 
>   src/slave/slave.cpp 9055f2a789cb19f3579c15a379ea505dfef0578c 
> 
> Diff: https://reviews.apache.org/r/38877/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Anand Mazumdar
> 
>

Re: Review Request 38877: Added functionality for Subscribe/Subscribed workflow for HTTP executors

[Anand Mazumdar]

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

(Updated Dec. 2, 2015, 10:41 p.m.)


Review request for mesos, Ben Mahler, Isabel Jimenez, and Vinod Kone.


Changes
---

Review comments from Vinod


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


Repository: mesos


Description
---

This change adds the functionality for executors to `Subscribe` via the 
`api/v1/executor` endpoint. It also stores a marker file as part of the 
`Subscribe` call if framework `checkpointing` is enabled. This can then be used 
by the agent when recovering to wait for reconnecting back with the executor.


Diffs (updated)
-

  src/slave/http.cpp c3247f17e9faed32a46d3ab9ee83c399cd2c8d5e 
  src/slave/slave.hpp 5ee133ae52998d05c8afabb2ade7095e363c75c5 
  src/slave/slave.cpp 9055f2a789cb19f3579c15a379ea505dfef0578c 

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


Testing
---

make check


Thanks,

Anand Mazumdar

Re: Review Request 39987: [3/5] Added 'Master::authorize(Un)reserveResources()' for Reserve/Unreserve.

[Greg Mann]

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

(Updated Dec. 2, 2015, 10:38 p.m.)


Review request for mesos, Adam B, Jie Yu, Michael Park, and Till Toenshoff.


Changes
---

Altered comment to match changes in subsequent patches.


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


Repository: mesos


Description
---

Added 'Master::authorize(Un)reserveResources()' for Reserve/Unreserve.
Note: this review is continued from https://reviews.apache.org/r/37125/


Diffs (updated)
-

  src/master/master.hpp 96951e766de32842197506504e5ac67a2caa3efe 
  src/master/master.cpp b918ae4a0e7dc3cd41165fc4b683ae7b6f031821 

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


Testing
---

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


Thanks,

Greg Mann

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

[Avinash sridharan]

---
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 (updated)
-

  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 40524: Used string directly in resources.cpp.

[Joris Van Remoortere]

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

Ship it!


Ship It!

- Joris Van Remoortere


On Dec. 1, 2015, 2:13 a.m., Guangya Liu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/40524/
> ---
> 
> (Updated Dec. 1, 2015, 2:13 a.m.)
> 
> 
> Review request for mesos, Artem Harutyunyan, Joris Van Remoortere, Joseph Wu, 
> and Klaus Ma.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Used string directly in resources.cpp.
> 
> 
> Diffs
> -
> 
>   src/common/resources.cpp 98804a456b0faecc4aa56929bd1b83e5900db5ae 
>   src/v1/resources.cpp 8488c318a987a150fc5fde26b54246e8effb0428 
> 
> Diff: https://reviews.apache.org/r/40524/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Guangya Liu
> 
>

Re: Review Request 40880: Fix flaky MemoryPressureMesosTest.CGROUPS_ROOT_SlaveRecovery test.

[Mesos ReviewBot]

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


Patch looks great!

Reviews applied: [40849, 40880]

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

- Mesos ReviewBot


On Dec. 2, 2015, 8:44 p.m., Joseph Wu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/40880/
> ---
> 
> (Updated Dec. 2, 2015, 8:44 p.m.)
> 
> 
> Review request for mesos, Bernd Mathiske, Artem Harutyunyan, and Till 
> Toenshoff.
> 
> 
> Bugs: MESOS-4047
> https://issues.apache.org/jira/browse/MESOS-4047
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> `MesosContainerizerSlaveRecoveryTest.ResourceStatistics` has very similar 
> logic for restarting an agent, re-registering the executor, and even getting 
> `ResourceStatistics`.  But 
> `MesosContainerizerSlaveRecoveryTest.ResourceStatistics` is stable.
> 
> This patch updates the flaky test's wait-for-agent-recovery logic to match 
> the stable test.
> 
> 
> Diffs
> -
> 
>   src/tests/containerizer/memory_pressure_tests.cpp 
> e18b971c4df26c9b9c103ca73bdad4fd400d6c02 
> 
> Diff: https://reviews.apache.org/r/40880/diff/
> 
> 
> Testing
> ---
> 
> On Ubuntu 14:
> `make check`
> `sudo bin/mesos-tests.sh 
> --gtest_filter="*MemoryPressureMesosTest.CGROUPS_ROOT_SlaveRecovery*" 
> --gtest_repeat=-1 --gtest_break_on_failure`
> 
> ^ Ran the above until satisfied.
> 
> 
> Thanks,
> 
> Joseph Wu
> 
>

Re: Review Request 38878: Added test for the Subscribe->Subscribed workflow for the Executor HTTP API

[Vinod Kone]

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



src/tests/executor_http_api_tests.cpp (line 749)


s/on/by/



src/tests/executor_http_api_tests.cpp (line 757)


Hmm. this test is a bit convoluted. you start a pid based executor first, 
wait for its update to be received by the scheduler and then start a subscribe 
request from http executor?

i'm assuming you can't simply send a subscribe request because the slave 
wouldn't have an executor struct in its map. is it possible to stop the pid 
executor from starting at all? maybe by using testing isolator/containerizer?


- Vinod Kone


On Nov. 30, 2015, 3:56 a.m., Anand Mazumdar wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/38878/
> ---
> 
> (Updated Nov. 30, 2015, 3:56 a.m.)
> 
> 
> Review request for mesos, Ben Mahler, Isabel Jimenez, and Vinod Kone.
> 
> 
> Bugs: MESOS-3515
> https://issues.apache.org/jira/browse/MESOS-3515
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This change adds a basic test to validate the implementation for 
> Subscribe->Subscribed workflow on the `api/v1/executor` endpoint on Agent.
> 
> 
> Diffs
> -
> 
>   src/tests/executor_http_api_tests.cpp 
> fe9df1f4d68babaf0960a3b689ffbe60704b8ad5 
> 
> Diff: https://reviews.apache.org/r/38878/diff/
> 
> 
> Testing
> ---
> 
> make check. Would add more agent recovery tests/executor reconnect tests in a 
> separate patch.
> 
> 
> Thanks,
> 
> Anand Mazumdar
> 
>

Re: Review Request 38877: Added functionality for Subscribe/Subscribed workflow for HTTP executors

[Vinod Kone]

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


I see a very basic test in the next review. Are you planning to write more 
comprehensive tests? Is that plan to templatize slave recovery tests for both 
pid based and http executors?


src/slave/slave.cpp (lines 2414 - 2415)


I see that 'RECOVERING' is a possible state in `registerExecutor()`. Is 
that not possible for http executors?



src/slave/slave.cpp (line 2450)


s/retry/retried/



src/slave/slave.cpp (line 2454)


i don't think 'in lieu of' is correct here. just remove the second part 
starting from "in lieu of...".



src/slave/slave.cpp (line 2475)


s/http/HTTP/


- Vinod Kone


On Nov. 30, 2015, 3:56 a.m., Anand Mazumdar wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/38877/
> ---
> 
> (Updated Nov. 30, 2015, 3:56 a.m.)
> 
> 
> Review request for mesos, Ben Mahler, Isabel Jimenez, and Vinod Kone.
> 
> 
> Bugs: MESOS-3515
> https://issues.apache.org/jira/browse/MESOS-3515
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This change adds the functionality for executors to `Subscribe` via the 
> `api/v1/executor` endpoint. It also stores a marker file as part of the 
> `Subscribe` call if framework `checkpointing` is enabled. This can then be 
> used by the agent when recovering to wait for reconnecting back with the 
> executor.
> 
> 
> Diffs
> -
> 
>   src/slave/http.cpp c3247f17e9faed32a46d3ab9ee83c399cd2c8d5e 
>   src/slave/slave.hpp 5ee133ae52998d05c8afabb2ade7095e363c75c5 
>   src/slave/slave.cpp 9055f2a789cb19f3579c15a379ea505dfef0578c 
> 
> Diff: https://reviews.apache.org/r/38877/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Anand Mazumdar
> 
>

Review Request 40884: Environment variable: Implemented passing user taskinfo and docker image env var for docker containerizer.

[Gilbert Song]

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

Review request for mesos, Artem Harutyunyan and Timothy Chen.


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


Repository: mesos


Description
---

Environment variable: Implemented passing user taskinfo and docker image env 
var for docker containerizer.


Diffs
-

  src/slave/containerizer/docker.cpp 2d2dd4e0df36207c5f3cbb4fe2c50df51c0f3e9e 

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


Testing
---

make check (ubuntu14.04 + clang-3.6)


Thanks,

Gilbert Song

Re: Review Request 40838: Environment variable: Implemented `Env` specified in docker image returned from docker pull.

[Gilbert Song]

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

(Updated Dec. 2, 2015, 2:09 p.m.)


Review request for mesos, Artem Harutyunyan and Timothy Chen.


Summary (updated)
-

Environment variable: Implemented `Env` specified in docker image returned from 
docker pull.


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


Repository: mesos


Description (updated)
---

Environment variable: Implemented `Env` specified in docker image returned from 
docker pull.


Diffs (updated)
-

  src/docker/docker.hpp 33d6fb3e82ff7328ad093648a45546a18ec7b8cb 
  src/docker/docker.cpp 5dc4667d93b143b54841d85606affe3e4926757a 

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


Testing (updated)
---

make check (ubuntu14.04 + clang-3.6)


Thanks,

Gilbert Song

Re: Review Request 39938: Document OS X SIGPIPE delivery.

[Ben Mahler]

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

Ship it!


Thanks!


3rdparty/libprocess/3rdparty/stout/tests/os/signals_tests.cpp (line 44)


One r in triggering.


- Ben Mahler


On Nov. 4, 2015, 5:15 p.m., James Peach wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/39938/
> ---
> 
> (Updated Nov. 4, 2015, 5:15 p.m.)
> 
> 
> Review request for mesos and Ben Mahler.
> 
> 
> Bugs: MESOS-2079
> https://issues.apache.org/jira/browse/MESOS-2079
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Document OS X SIGPIPE delivery.
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/3rdparty/stout/tests/os/signals_tests.cpp 
> 4bb79fdc161483cfc2b7a7a15e5c3ce62ceb493d 
> 
> Diff: https://reviews.apache.org/r/39938/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> James Peach
> 
>

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

[Avinash sridharan]

> On Dec. 2, 2015, 8:02 p.m., Anand Mazumdar wrote:
> > src/tests/reservation_tests.cpp, line 213
> > 
> >
> > Is this comment needed ?

I think this a copy-paste issue. This must have come from the previous test 
case (TEST_F(ReservationTest, ReserveThenUnreserve)) that was copied over. Will 
remove it.


> On Dec. 2, 2015, 8:02 p.m., Anand Mazumdar wrote:
> > src/tests/reservation_tests.cpp, line 231
> > 
> >
> > Can we just do:
> > 
> > `EXPECT_CALL(sched, resourceOffers(&driver, _))
> >  .WillOnce(FutureArg<1>(&offers));`
> >  
> > This is more in sync with the style followed in the rest of the tests. 
> > Can we modify the other ocurrences in this test too ?

The above exceeds the 80 column limit. I was following the style guide here:
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?


> On Dec. 2, 2015, 8:02 p.m., Anand Mazumdar wrote:
> > src/tests/reservation_tests.cpp, line 266
> > 
> >
> > We generally avoid redundant/self-explanatory comments. Is this really 
> > needed at all ? What is it trying to convey ?

The problem is that the check failure would lead to a crash in the agent, and 
wouldn't show up in the mockscheduler. Not sure if we can re-write the test 
case somehow to reflect it. Hence added an explicit comment to explain the 
behavior to the reader.


> On Dec. 2, 2015, 8:02 p.m., Anand Mazumdar wrote:
> > src/tests/reservation_tests.cpp, line 273
> > 
> >
> > Not yours : Can we insert an additional line here. We leave 2 lines 
> > after each global function/test.

ok


- Avinash


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


On Dec. 2, 2015, 8:27 a.m., Avinash sridharan wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/40731/
> ---
> 
> (Updated Dec. 2, 2015, 8:27 a.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 40873: SimpleRegistryPullerTest: Moved blob response preparation to the top.

[Mesos ReviewBot]

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


Patch looks great!

Reviews applied: [40872, 40873]

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

- Mesos ReviewBot


On Dec. 2, 2015, 4:26 p.m., Jojy Varghese wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/40873/
> ---
> 
> (Updated Dec. 2, 2015, 4:26 p.m.)
> 
> 
> Review request for mesos and Timothy Chen.
> 
> 
> Bugs: MESOS-4025
> https://issues.apache.org/jira/browse/MESOS-4025
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> By moving the preperation task above any test task avoids socket waiting for 
> the response to be prepared. This change has eliminated socket timeouts seen 
> sometimes.
> 
> 
> Diffs
> -
> 
>   src/tests/containerizer/provisioner_docker_tests.cpp 
> c63bf53fee40ef12536a16e11f4d5224c4e4278e 
> 
> Diff: https://reviews.apache.org/r/40873/diff/
> 
> 
> Testing
> ---
> 
> make check (600 count).
> 
> 
> Thanks,
> 
> Jojy Varghese
> 
>

Review Request 40880: Fix flaky MemoryPressureMesosTest.CGROUPS_ROOT_SlaveRecovery test.

[Joseph Wu]

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

Review request for mesos, Bernd Mathiske, Artem Harutyunyan, and Till Toenshoff.


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


Repository: mesos


Description
---

`MesosContainerizerSlaveRecoveryTest.ResourceStatistics` has very similar logic 
for restarting an agent, re-registering the executor, and even getting 
`ResourceStatistics`.  But 
`MesosContainerizerSlaveRecoveryTest.ResourceStatistics` is stable.

This patch updates the flaky test's wait-for-agent-recovery logic to match the 
stable test.


Diffs
-

  src/tests/containerizer/memory_pressure_tests.cpp 
e18b971c4df26c9b9c103ca73bdad4fd400d6c02 

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


Testing
---

On Ubuntu 14:
`make check`
`sudo bin/mesos-tests.sh 
--gtest_filter="*MemoryPressureMesosTest.CGROUPS_ROOT_SlaveRecovery*" 
--gtest_repeat=-1 --gtest_break_on_failure`

^ Ran the above until satisfied.


Thanks,

Joseph Wu

Re: Review Request 40873: SimpleRegistryPullerTest: Moved blob response preparation to the top.

[Anand Mazumdar]

> On Dec. 2, 2015, 4:40 p.m., Anand Mazumdar wrote:
> > @jojy, Do we know why preparing the response was taking so long that was 
> > causing the socket to timeout previously ?
> 
> Jojy Varghese wrote:
> @anand: The observed behavior is that the server socket gets a RST. This 
> is ususally when the peer side of the socket is closed. Also observed is that 
> there are asycnhronous socket close being done ( i think from the HTTP 
> layer). This was observed when I added a log inside the dtor of the socket 
> (socket.hpp: L103). For failed test cases, I saw an extra socket close. This 
> change reduces the time between the peer interactions. This is not a 
> "solution" of the problem but an effort to eliminate possible false alarms 
> and be able to focus on the real issue. Since its very difficult to reproduce 
> this issue predictably, we need to elimitate all red herrings.

Thanks for the reply @jojy. 

Do we know why the peer side of the socket was closed i.e. it got a RST and why 
the peer side of the socket was closed in the first place ? Can you also help 
me understand how are these false alarms ? To me, it suggests a bug inside the 
`RegistryClient` itself and we are finding a workaround in the tests to mask 
it, no ?


- Anand


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


On Dec. 2, 2015, 4:26 p.m., Jojy Varghese wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/40873/
> ---
> 
> (Updated Dec. 2, 2015, 4:26 p.m.)
> 
> 
> Review request for mesos and Timothy Chen.
> 
> 
> Bugs: MESOS-4025
> https://issues.apache.org/jira/browse/MESOS-4025
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> By moving the preperation task above any test task avoids socket waiting for 
> the response to be prepared. This change has eliminated socket timeouts seen 
> sometimes.
> 
> 
> Diffs
> -
> 
>   src/tests/containerizer/provisioner_docker_tests.cpp 
> c63bf53fee40ef12536a16e11f4d5224c4e4278e 
> 
> Diff: https://reviews.apache.org/r/40873/diff/
> 
> 
> Testing
> ---
> 
> make check (600 count).
> 
> 
> Thanks,
> 
> Jojy Varghese
> 
>

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

[Anand Mazumdar]

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


Some minor comments modulo comments from Bernd earlier.


src/tests/reservation_tests.cpp (line 205)


Is this comment needed ?



src/tests/reservation_tests.cpp (line 222)


Can we just do:

`EXPECT_CALL(sched, resourceOffers(&driver, _))
 .WillOnce(FutureArg<1>(&offers));`
 
This is more in sync with the style followed in the rest of the tests. Can 
we modify the other ocurrences in this test too ?



src/tests/reservation_tests.cpp (line 254)


We generally avoid redundant/self-explanatory comments. Is this really 
needed at all ? What is it trying to convey ?



src/tests/reservation_tests.cpp (line 261)


Not yours : Can we insert an additional line here. We leave 2 lines after 
each global function/test.


- Anand Mazumdar


On Dec. 2, 2015, 8:27 a.m., Avinash sridharan wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/40731/
> ---
> 
> (Updated Dec. 2, 2015, 8:27 a.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 40746: Quota: Removed quota from registry for remove request.

[Joris Van Remoortere]

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

Ship it!


Ship It!

- Joris Van Remoortere


On Nov. 26, 2015, 12:28 p.m., Alexander Rukletsov wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/40746/
> ---
> 
> (Updated Nov. 26, 2015, 12:28 p.m.)
> 
> 
> Review request for mesos, Joris Van Remoortere and Joseph Wu.
> 
> 
> Bugs: MESOS-4021
> https://issues.apache.org/jira/browse/MESOS-4021
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> See summary.
> 
> 
> Diffs
> -
> 
>   src/master/quota_handler.cpp e9c8791d4bd00e6c1be1844f27d9bee26f722a9b 
> 
> Diff: https://reviews.apache.org/r/40746/diff/
> 
> 
> Testing
> ---
> 
> make check (Mac OS X 10.10.4)
> 
> 
> Thanks,
> 
> Alexander Rukletsov
> 
>

Re: Review Request 40873: SimpleRegistryPullerTest: Moved blob response preparation to the top.

[Jojy Varghese]

> On Dec. 2, 2015, 4:40 p.m., Anand Mazumdar wrote:
> > @jojy, Do we know why preparing the response was taking so long that was 
> > causing the socket to timeout previously ?

@anand: The observed behavior is that the server socket gets a RST. This is 
ususally when the peer side of the socket is closed. Also observed is that 
there are asycnhronous socket close being done ( i think from the HTTP layer). 
This was observed when I added a log inside the dtor of the socket (socket.hpp: 
L103). For failed test cases, I saw an extra socket close. This change reduces 
the time between the peer interactions. This is not a "solution" of the problem 
but an effort to eliminate possible false alarms and be able to focus on the 
real issue. Since its very difficult to reproduce this issue predictably, we 
need to elimitate all red herrings.


- Jojy


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


On Dec. 2, 2015, 4:26 p.m., Jojy Varghese wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/40873/
> ---
> 
> (Updated Dec. 2, 2015, 4:26 p.m.)
> 
> 
> Review request for mesos and Timothy Chen.
> 
> 
> Bugs: MESOS-4025
> https://issues.apache.org/jira/browse/MESOS-4025
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> By moving the preperation task above any test task avoids socket waiting for 
> the response to be prepared. This change has eliminated socket timeouts seen 
> sometimes.
> 
> 
> Diffs
> -
> 
>   src/tests/containerizer/provisioner_docker_tests.cpp 
> c63bf53fee40ef12536a16e11f4d5224c4e4278e 
> 
> Diff: https://reviews.apache.org/r/40873/diff/
> 
> 
> Testing
> ---
> 
> make check (600 count).
> 
> 
> Thanks,
> 
> Jojy Varghese
> 
>

Re: Review Request 39989: [5/5] Added framework authorization for dynamic reservation.

[Greg Mann]

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

(Updated Dec. 2, 2015, 7:55 p.m.)


Review request for mesos, Adam B, Jie Yu, Michael Park, and Till Toenshoff.


Changes
---

Addressed comments.


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


Repository: mesos


Description
---

Added framework authorization for dynamic reservation.
Note: this review is continued from https://reviews.apache.org/r/37127/


Diffs (updated)
-

  src/master/master.cpp b918ae4a0e7dc3cd41165fc4b683ae7b6f031821 
  src/tests/reservation_tests.cpp 15d180f92ec0aea99e6f3a7d0b505c62bd207b61 

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


Testing
---

This is the fifth in a chain of 5 patches. New reservation tests were added to 
`reservation_tests.cpp` to validate the authentication of framework reserve and 
unreserve operations using ACLs. `make check` was run to test after all patches 
were applied.


Thanks,

Greg Mann

Re: Review Request 39989: [5/5] Added framework authorization for dynamic reservation.

[Greg Mann]

> On Dec. 2, 2015, 4:05 p.m., Alexander Rukletsov wrote:
> > src/master/master.cpp, lines 3041-3043
> > 
> >
> > We validate later on that in `principal` is `None`, reserve is aborted. 
> > IIUC, if `FrameworkInfo` does not specify principal, it cannot reserve 
> > resources. So the question is: do we need to check this first and only then 
> > proceed with authz?
> 
> Jie Yu wrote:
> Alex, from the impl. perspective, we thought about that in the earlier 
> iteration, and found that it's quite difficult to do it cleanly given the 
> current structure of the code. We want validation to be done at the same 
> place (`Master::_accept`).
> 
> ALso, we had some discussion on whether 'principal' in ReservationInfo 
> needs to be required or not (MESOS-3064). In the future, we might want to 
> make it 'optional' so that a framework without principal can also reserve 
> resources (it's reserved resources can be unreserved by anyone).
> 
> So I would suggest we keep the current structure and add a comment here 
> saying that: currently, if framework's principal is not specified, the 
> operation validation will fail in `_accept` even thought authorization might 
> succeed.

I added a comment along the lines of Jie's suggestion.


- Greg


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


On Dec. 2, 2015, 9:41 a.m., Greg Mann wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/39989/
> ---
> 
> (Updated Dec. 2, 2015, 9:41 a.m.)
> 
> 
> Review request for mesos, Adam B, Jie Yu, Michael Park, and Till Toenshoff.
> 
> 
> Bugs: MESOS-3062
> https://issues.apache.org/jira/browse/MESOS-3062
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added framework authorization for dynamic reservation.
> Note: this review is continued from https://reviews.apache.org/r/37127/
> 
> 
> Diffs
> -
> 
>   src/master/master.cpp b918ae4a0e7dc3cd41165fc4b683ae7b6f031821 
>   src/tests/reservation_tests.cpp 15d180f92ec0aea99e6f3a7d0b505c62bd207b61 
> 
> Diff: https://reviews.apache.org/r/39989/diff/
> 
> 
> Testing
> ---
> 
> This is the fifth in a chain of 5 patches. New reservation tests were added 
> to `reservation_tests.cpp` to validate the authentication of framework 
> reserve and unreserve operations using ACLs. `make check` was run to test 
> after all patches were applied.
> 
> 
> Thanks,
> 
> Greg Mann
> 
>

Re: Review Request 39988: [4/5] Added authorization for dynamic reservation master endpoints.

[Greg Mann]

> On Dec. 2, 2015, 2:26 p.m., Alexander Rukletsov wrote:
> > src/master/master.hpp, lines 1089-1095
> > 
> >
> > Do you want to do this cleanup as a separate patch? I believe we tend 
> > not to conflate semantic and stylistic changes in one patch.

Sounds good, I'll put these changes in another patch.


- Greg


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


On Dec. 2, 2015, 7:53 p.m., Greg Mann wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/39988/
> ---
> 
> (Updated Dec. 2, 2015, 7:53 p.m.)
> 
> 
> Review request for mesos, Adam B, Jie Yu, Michael Park, and Till Toenshoff.
> 
> 
> Bugs: MESOS-3062
> https://issues.apache.org/jira/browse/MESOS-3062
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added authorization for dynamic reservation master endpoints.
> Note: this review is continued from https://reviews.apache.org/r/37126/
> 
> 
> Diffs
> -
> 
>   src/master/http.cpp 9d729ef7f7d7ad6185934648f833e4f8a4f0a123 
>   src/tests/reservation_endpoints_tests.cpp 
> f30ff8bc6a3e9773437fa7fd7c8f569b7d7e2d9d 
> 
> Diff: https://reviews.apache.org/r/39988/diff/
> 
> 
> Testing
> ---
> 
> This is the fourth in a chain of 5 patches. Added new reservation endpoints 
> tests to validate authorization of reserve and unreserve operations using 
> ACLs. `make check` was run to test after all patches were applied.
> 
> 
> Thanks,
> 
> Greg Mann
> 
>

Re: Review Request 39988: [4/5] Added authorization for dynamic reservation master endpoints.

[Greg Mann]

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

(Updated Dec. 2, 2015, 7:53 p.m.)


Review request for mesos, Adam B, Jie Yu, Michael Park, and Till Toenshoff.


Changes
---

Addressed comments.


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


Repository: mesos


Description
---

Added authorization for dynamic reservation master endpoints.
Note: this review is continued from https://reviews.apache.org/r/37126/


Diffs (updated)
-

  src/master/http.cpp 9d729ef7f7d7ad6185934648f833e4f8a4f0a123 
  src/tests/reservation_endpoints_tests.cpp 
f30ff8bc6a3e9773437fa7fd7c8f569b7d7e2d9d 

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


Testing
---

This is the fourth in a chain of 5 patches. Added new reservation endpoints 
tests to validate authorization of reserve and unreserve operations using ACLs. 
`make check` was run to test after all patches were applied.


Thanks,

Greg Mann

Re: Review Request 39987: [3/5] Added 'Master::authorize(Un)reserveResources()' for Reserve/Unreserve.

[Greg Mann]

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

(Updated Dec. 2, 2015, 7:52 p.m.)


Review request for mesos, Adam B, Jie Yu, Michael Park, and Till Toenshoff.


Changes
---

Addressed comment.


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


Repository: mesos


Description
---

Added 'Master::authorize(Un)reserveResources()' for Reserve/Unreserve.
Note: this review is continued from https://reviews.apache.org/r/37125/


Diffs (updated)
-

  src/master/master.hpp 96951e766de32842197506504e5ac67a2caa3efe 
  src/master/master.cpp b918ae4a0e7dc3cd41165fc4b683ae7b6f031821 

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


Testing
---

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


Thanks,

Greg Mann

Re: Review Request 40745: Cleaned up `RemoveSingleQuota` test and corrected formatting issues.

[Joris Van Remoortere]

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

Ship it!


Ship It!

- Joris Van Remoortere


On Nov. 27, 2015, 2:54 p.m., Alexander Rukletsov wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/40745/
> ---
> 
> (Updated Nov. 27, 2015, 2:54 p.m.)
> 
> 
> Review request for mesos, Joerg Schad and Joris Van Remoortere.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> See summary.
> 
> 
> Diffs
> -
> 
>   src/master/allocator/mesos/hierarchical.cpp 
> 31ed62efb5b1a2edb567f43d37559c5914e0665e 
>   src/master/quota_handler.cpp e9c8791d4bd00e6c1be1844f27d9bee26f722a9b 
>   src/tests/master_quota_tests.cpp 15edb27f3ea5d3813c937614aa217ca999fd2481 
> 
> Diff: https://reviews.apache.org/r/40745/diff/
> 
> 
> Testing
> ---
> 
> make check (Mac OS X 10.10.4)
> 
> 
> Thanks,
> 
> Alexander Rukletsov
> 
>

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(&driver, _))
> >.WillOnce(FutureArg<1>(&offers));

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?


> On Dec. 2, 2015, 6:32 p.m., Bernd Mathiske wrote:
> > src/tests/reservation_tests.cpp, line 257
> > 
> >
> > Where exactly is "over here"? Does the wait statement contain floating 
> > point comparisons?

over here is actually the agent. Would result a crash in the agent. Will update 
the comment accordingly


- Avinash


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


On Dec. 2, 2015, 8:27 a.m., Avinash sridharan wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/40731/
> ---
> 
> (Updated Dec. 2, 2015, 8:27 a.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 40849: Fix flaky MemoryPressureMesosTests

[Joseph Wu]

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

(Updated Dec. 2, 2015, 11:25 a.m.)


Review request for mesos, Bernd Mathiske, Greg Mann, Artem Harutyunyan, Jan 
Schlicht, and Till Toenshoff.


Changes
---

Fixed typo in comment.  Tested on a few more platforms.


Summary (updated)
-

Fix flaky MemoryPressureMesosTests


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


Repository: mesos


Description
---

The existing tests will check that "low" pressure events occur at least as 
often as "medium" pressure events (this is the documented behavior).  However, 
the order of events and the order in which we process said events is not 
guaranteed.  When we collect the pressure events via a counter, there may be 
some events that are in-flight, and thereby not accounted for in the counters.

This patch modifies MemoryPressureMesosTests to wait for memory pressure events 
to stop before checking the counts for correctness.
The tests now stop the memory-pressure-triggering task and then wait for all 
events to be processed before checking the counters.


Diffs (updated)
-

  src/tests/containerizer/memory_pressure_tests.cpp 
e18b971c4df26c9b9c103ca73bdad4fd400d6c02 

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


Testing (updated)
---

On Debian 8, Ubuntu 14, Centos 7 (Thanks Jan!):
`make check`
`sudo bin/mesos-tests.sh --gtest_filter="*MemoryPressureMesosTest*" 
--gtest_repeat=-1 --gtest_break_on_failure`

^ Ran the above for a couple minutes or ~100 times.  It was previously failing 
~1/5 times.

---
Note on Centos 6 (Thanks Greg!):
```
[ RUN  ] MemoryPressureMesosTest.CGROUPS_ROOT_Statistics
../../src/tests/mesos.cpp:849: Failure
Value of: _baseHierarchy.get()
  Actual: "/cgroup"
Expected: baseHierarchy
Which is: "/tmp/mesos_test_cgroup"
-
Multiple cgroups base hierarchies detected:
  '/tmp/mesos_test_cgroup'
  '/cgroup'
Mesos does not support multiple cgroups base hierarchies.
Please unmount the corresponding (or all) subsystems.
-
../../src/tests/mesos.cpp:932: Failure
(cgroups::destroy(hierarchy, cgroup)).failure(): Failed to remove cgroup 
'/tmp/mesos_test_cgroup/perf_event/mesos_test': Device or resource busy
[  FAILED  ] MemoryPressureMesosTest.CGROUPS_ROOT_Statistics (12 ms)
```

---
Note on Ubuntu 14:
There is some other flakiness (in Agent recovery).  This will be tracked in 
https://issues.apache.org/jira/browse/MESOS-4047


Thanks,

Joseph Wu

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

[Bernd Mathiske]

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



src/tests/reservation_tests.cpp (line 237)


More customary indentation:

 EXPECT_CALL(sched, resourceOffers(&driver, _))
   .WillOnce(FutureArg<1>(&offers));



src/tests/reservation_tests.cpp (line 243)


Please put blanks after "//" on theleft.

Typos: 
- calculate
- comparison

Never use apostrophy abbreviations where not necessary:
s/isn't/is not/

Missing period at sentence end after MESOS-3552.



src/tests/reservation_tests.cpp (line 245)


Where exactly is "over here"? Does the wait statement contain floating 
point comparisons?



src/tests/reservation_tests.cpp (line 254)


Missing blanks after "//".


- Bernd Mathiske


On Dec. 2, 2015, 12:27 a.m., Avinash sridharan wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/40731/
> ---
> 
> (Updated Dec. 2, 2015, 12:27 a.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 39987: [3/5] Added 'Master::authorize(Un)reserveResources()' for Reserve/Unreserve.

[Jie Yu]

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



src/master/master.cpp (lines 2804 - 2805)


Do you need add an if check here given that validation is not a 
pre-condition of this method

```
if (isDynamicallyReserved(resource)) {
  ...
}
```


- Jie Yu


On Dec. 2, 2015, 6 p.m., Greg Mann wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/39987/
> ---
> 
> (Updated Dec. 2, 2015, 6 p.m.)
> 
> 
> Review request for mesos, Adam B, Jie Yu, Michael Park, and Till Toenshoff.
> 
> 
> Bugs: MESOS-3062
> https://issues.apache.org/jira/browse/MESOS-3062
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added 'Master::authorize(Un)reserveResources()' for Reserve/Unreserve.
> Note: this review is continued from https://reviews.apache.org/r/37125/
> 
> 
> Diffs
> -
> 
>   src/master/master.hpp 96951e766de32842197506504e5ac67a2caa3efe 
>   src/master/master.cpp b918ae4a0e7dc3cd41165fc4b683ae7b6f031821 
> 
> Diff: https://reviews.apache.org/r/39987/diff/
> 
> 
> Testing
> ---
> 
> This is the third in a chain of 5 patches. `make check` was used to test 
> after all patches were applied.
> 
> 
> Thanks,
> 
> Greg Mann
> 
>

Re: Review Request 39987: [3/5] Added 'Master::authorize(Un)reserveResources()' for Reserve/Unreserve.

[Jie Yu]

> On Dec. 2, 2015, 4:02 p.m., Alexander Rukletsov wrote:
> > src/master/master.cpp, lines 2769-2770
> > 
> >
> > Is my understanding is correct, we do not allow reserving resources 
> > neither for operators nor for frameworks without a principal. Why do we 
> > have a path for `ACL::Entity::Any` here then? If you plan to reserve it for 
> > future use cases or just be more general — it's fine, but let's leave a fat 
> > comment that this is not possible currently (or how it's possible if my 
> > understanding is not correct).
> > 
> > A follow-up question: Is it something we have agreed and maybe even 
> > documented somewhere, that certain actions require principal? I'm thinking 
> > about authz for quota and whether we have to make `principal` required 
> > there.
> 
> Greg Mann wrote:
> Yea this is an artifact of doing validation after authorization. We're 
> doing it in that order here to maintain consistency with the order for LAUNCH 
> operations, which must validate after authorization because their validation 
> requires knowledge of the tasks that may have already been launched within a 
> particular operation.
> 
> You're right that if `!principal.isSome()` here, we know that this 
> operation will fail. The validation routine will catch that and return the 
> proper error, but how best to handle it here? In order to ensure that 
> `validate()` is able to report the correct error, perhaps we should just 
> `return true;` when `!principal.isSome()`. This is less than ideal, but 
> perhaps would suffice along with a fat comment, as you suggested :-)

Alex, thank you for the comments! I think those are great questions.

> whether we have to make principal required there.

I like the idea of making it optional. That gives us more flexibility (see some 
discussion in MESOS-3064): allowing dynamic reserveration without authz.


- Jie


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


On Dec. 2, 2015, 6 p.m., Greg Mann wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/39987/
> ---
> 
> (Updated Dec. 2, 2015, 6 p.m.)
> 
> 
> Review request for mesos, Adam B, Jie Yu, Michael Park, and Till Toenshoff.
> 
> 
> Bugs: MESOS-3062
> https://issues.apache.org/jira/browse/MESOS-3062
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added 'Master::authorize(Un)reserveResources()' for Reserve/Unreserve.
> Note: this review is continued from https://reviews.apache.org/r/37125/
> 
> 
> Diffs
> -
> 
>   src/master/master.hpp 96951e766de32842197506504e5ac67a2caa3efe 
>   src/master/master.cpp b918ae4a0e7dc3cd41165fc4b683ae7b6f031821 
> 
> Diff: https://reviews.apache.org/r/39987/diff/
> 
> 
> Testing
> ---
> 
> This is the third in a chain of 5 patches. `make check` was used to test 
> after all patches were applied.
> 
> 
> Thanks,
> 
> Greg Mann
> 
>

Re: Review Request 39987: [3/5] Added 'Master::authorize(Un)reserveResources()' for Reserve/Unreserve.

[Greg Mann]

> On Dec. 2, 2015, 4:02 p.m., Alexander Rukletsov wrote:
> > src/master/master.cpp, lines 2769-2770
> > 
> >
> > Is my understanding is correct, we do not allow reserving resources 
> > neither for operators nor for frameworks without a principal. Why do we 
> > have a path for `ACL::Entity::Any` here then? If you plan to reserve it for 
> > future use cases or just be more general — it's fine, but let's leave a fat 
> > comment that this is not possible currently (or how it's possible if my 
> > understanding is not correct).
> > 
> > A follow-up question: Is it something we have agreed and maybe even 
> > documented somewhere, that certain actions require principal? I'm thinking 
> > about authz for quota and whether we have to make `principal` required 
> > there.

Yea this is an artifact of doing validation after authorization. We're doing it 
in that order here to maintain consistency with the order for LAUNCH 
operations, which must validate after authorization because their validation 
requires knowledge of the tasks that may have already been launched within a 
particular operation.

You're right that if `!principal.isSome()` here, we know that this operation 
will fail. The validation routine will catch that and return the proper error, 
but how best to handle it here? In order to ensure that `validate()` is able to 
report the correct error, perhaps we should just `return true;` when 
`!principal.isSome()`. This is less than ideal, but perhaps would suffice along 
with a fat comment, as you suggested :-)


- Greg


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


On Dec. 2, 2015, 6 p.m., Greg Mann wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/39987/
> ---
> 
> (Updated Dec. 2, 2015, 6 p.m.)
> 
> 
> Review request for mesos, Adam B, Jie Yu, Michael Park, and Till Toenshoff.
> 
> 
> Bugs: MESOS-3062
> https://issues.apache.org/jira/browse/MESOS-3062
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added 'Master::authorize(Un)reserveResources()' for Reserve/Unreserve.
> Note: this review is continued from https://reviews.apache.org/r/37125/
> 
> 
> Diffs
> -
> 
>   src/master/master.hpp 96951e766de32842197506504e5ac67a2caa3efe 
>   src/master/master.cpp b918ae4a0e7dc3cd41165fc4b683ae7b6f031821 
> 
> Diff: https://reviews.apache.org/r/39987/diff/
> 
> 
> Testing
> ---
> 
> This is the third in a chain of 5 patches. `make check` was used to test 
> after all patches were applied.
> 
> 
> Thanks,
> 
> Greg Mann
> 
>

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

[Bernd Mathiske]

> On Nov. 30, 2015, 6:15 a.m., Bernd Mathiske wrote:
> > src/tests/reservation_tests.cpp, line 248
> > 
> >
> > s/RESERVE/UNRESERVE/ ?
> > 
> > What is the intention here? Please explain.
> 
> Avinash sridharan wrote:
> Can you clarify this comment? The idea here seems to be to generate 
> multiple CPU resource requests. Hence the RESERVE?

The misleading comment near this is gone and a new one is there that addresses 
the issue. Thx


- Bernd


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


On Dec. 2, 2015, 12:27 a.m., Avinash sridharan wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/40731/
> ---
> 
> (Updated Dec. 2, 2015, 12:27 a.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 37540: Add perf event API

[Niklas Nielsen]

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


Ping - Cong, is there anything you need to close the last issues? :)

- Niklas Nielsen


On Sept. 29, 2015, 5:12 p.m., Cong Wang wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/37540/
> ---
> 
> (Updated Sept. 29, 2015, 5:12 p.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
> ---
> 
> Abstract Linux kernel perf event API and provide API to collect schedule 
> events.
> 
> 
> Diffs
> -
> 
>   src/linux/perf.hpp d10968ca670eb516bae08385fd0ddde8e8ad83b5 
>   src/linux/perf.cpp f7035ddb2507a7646d88dd517d048018f695448a 
>   src/tests/containerizer/perf_tests.cpp 
> ed5212ee31b8aa47339b8b8fab184bbdf85be82a 
> 
> Diff: https://reviews.apache.org/r/37540/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Cong Wang
> 
>

Re: Review Request 39989: [5/5] Added framework authorization for dynamic reservation.

[Jie Yu]

> On Dec. 2, 2015, 4:05 p.m., Alexander Rukletsov wrote:
> > src/master/master.cpp, lines 3041-3043
> > 
> >
> > We validate later on that in `principal` is `None`, reserve is aborted. 
> > IIUC, if `FrameworkInfo` does not specify principal, it cannot reserve 
> > resources. So the question is: do we need to check this first and only then 
> > proceed with authz?

Alex, from the impl. perspective, we thought about that in the earlier 
iteration, and found that it's quite difficult to do it cleanly given the 
current structure of the code. We want validation to be done at the same place 
(`Master::_accept`).

ALso, we had some discussion on whether 'principal' in ReservationInfo needs to 
be required or not (MESOS-3064). In the future, we might want to make it 
'optional' so that a framework without principal can also reserve resources 
(it's reserved resources can be unreserved by anyone).

So I would suggest we keep the current structure and add a comment here saying 
that: currently, if framework's principal is not specified, the operation 
validation will fail in `_accept` even thought authorization might succeed.


- Jie


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


On Dec. 2, 2015, 9:41 a.m., Greg Mann wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/39989/
> ---
> 
> (Updated Dec. 2, 2015, 9:41 a.m.)
> 
> 
> Review request for mesos, Adam B, Jie Yu, Michael Park, and Till Toenshoff.
> 
> 
> Bugs: MESOS-3062
> https://issues.apache.org/jira/browse/MESOS-3062
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added framework authorization for dynamic reservation.
> Note: this review is continued from https://reviews.apache.org/r/37127/
> 
> 
> Diffs
> -
> 
>   src/master/master.cpp b918ae4a0e7dc3cd41165fc4b683ae7b6f031821 
>   src/tests/reservation_tests.cpp 15d180f92ec0aea99e6f3a7d0b505c62bd207b61 
> 
> Diff: https://reviews.apache.org/r/39989/diff/
> 
> 
> Testing
> ---
> 
> This is the fifth in a chain of 5 patches. New reservation tests were added 
> to `reservation_tests.cpp` to validate the authentication of framework 
> reserve and unreserve operations using ACLs. `make check` was run to test 
> after all patches were applied.
> 
> 
> Thanks,
> 
> Greg Mann
> 
>

Re: Review Request 39987: [3/5] Added 'Master::authorize(Un)reserveResources()' for Reserve/Unreserve.

[Greg Mann]

> On Dec. 2, 2015, 2:17 p.m., Alexander Rukletsov wrote:
> > src/master/master.cpp, line 2775
> > 
> >
> > I believe you inherited the TODO from MPark. Do you want to preserve 
> > his name or change to yours?

I was going to leave his name, since he's the original author of the TODO :-)


> On Dec. 2, 2015, 2:17 p.m., Alexander Rukletsov wrote:
> > src/master/master.cpp, line 2781
> > 
> >
> > Maybe "' to reserve any resources"? Or mention what resources are 
> > allowed to reserve (similar to what you do in `authorizeUnreserveResources`.

Good call! Changed.


- Greg


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


On Dec. 2, 2015, 6 p.m., Greg Mann wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/39987/
> ---
> 
> (Updated Dec. 2, 2015, 6 p.m.)
> 
> 
> Review request for mesos, Adam B, Jie Yu, Michael Park, and Till Toenshoff.
> 
> 
> Bugs: MESOS-3062
> https://issues.apache.org/jira/browse/MESOS-3062
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added 'Master::authorize(Un)reserveResources()' for Reserve/Unreserve.
> Note: this review is continued from https://reviews.apache.org/r/37125/
> 
> 
> Diffs
> -
> 
>   src/master/master.hpp 96951e766de32842197506504e5ac67a2caa3efe 
>   src/master/master.cpp b918ae4a0e7dc3cd41165fc4b683ae7b6f031821 
> 
> Diff: https://reviews.apache.org/r/39987/diff/
> 
> 
> Testing
> ---
> 
> This is the third in a chain of 5 patches. `make check` was used to test 
> after all patches were applied.
> 
> 
> Thanks,
> 
> Greg Mann
> 
>

Re: Review Request 39987: [3/5] Added 'Master::authorize(Un)reserveResources()' for Reserve/Unreserve.

[Greg Mann]

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

(Updated Dec. 2, 2015, 6 p.m.)


Review request for mesos, Adam B, Jie Yu, Michael Park, and Till Toenshoff.


Changes
---

Addressed comments.


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


Repository: mesos


Description
---

Added 'Master::authorize(Un)reserveResources()' for Reserve/Unreserve.
Note: this review is continued from https://reviews.apache.org/r/37125/


Diffs (updated)
-

  src/master/master.hpp 96951e766de32842197506504e5ac67a2caa3efe 
  src/master/master.cpp b918ae4a0e7dc3cd41165fc4b683ae7b6f031821 

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


Testing
---

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


Thanks,

Greg Mann

Re: Review Request 39988: [4/5] Added authorization for dynamic reservation master endpoints.

[Jie Yu]

> On Dec. 2, 2015, 3:05 p.m., Alexander Rukletsov wrote:
> > src/master/http.cpp, lines 1035-1038
> > 
> >
> > Let's leave a comment here, that `principal` matches 
> > `reservation().principal()` for each resource in 
> > `operation.reserve().resources()`, hence it's OK to authorize for 
> > `principal` and use `reservation().principal()` in `unreserve()`. Maybe a 
> > symmetrical comment in `unreserve()` path would also make sense.
> > 
> > Maybe if you validate before authorizing it will be more easy to 
> > understand?

+1 on validate before authorizing. That could save us a big comment here.


- Jie


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


On Dec. 2, 2015, 9:06 a.m., Greg Mann wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/39988/
> ---
> 
> (Updated Dec. 2, 2015, 9:06 a.m.)
> 
> 
> Review request for mesos, Adam B, Jie Yu, Michael Park, and Till Toenshoff.
> 
> 
> Bugs: MESOS-3062
> https://issues.apache.org/jira/browse/MESOS-3062
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added authorization for dynamic reservation master endpoints.
> Note: this review is continued from https://reviews.apache.org/r/37126/
> 
> 
> Diffs
> -
> 
>   src/master/http.cpp 9d729ef7f7d7ad6185934648f833e4f8a4f0a123 
>   src/master/master.hpp 96951e766de32842197506504e5ac67a2caa3efe 
>   src/tests/reservation_endpoints_tests.cpp 
> f30ff8bc6a3e9773437fa7fd7c8f569b7d7e2d9d 
> 
> Diff: https://reviews.apache.org/r/39988/diff/
> 
> 
> Testing
> ---
> 
> This is the fourth in a chain of 5 patches. Added new reservation endpoints 
> tests to validate authorization of reserve and unreserve operations using 
> ACLs. `make check` was run to test after all patches were applied.
> 
> 
> Thanks,
> 
> Greg Mann
> 
>

Re: Review Request 40818: Fixed flaky test (AvailableResourcesAfterRescinding).

[Joris Van Remoortere]

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

Ship it!


After our discussion offline, I suggest we remove these expectations for now. 
They require a significant amount of documentation, and don't really test 
anything as they only span the length of some helper functions.

- Joris Van Remoortere


On Dec. 1, 2015, 4:52 p.m., Alexander Rukletsov wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/40818/
> ---
> 
> (Updated Dec. 1, 2015, 4:52 p.m.)
> 
> 
> Review request for mesos, Joris Van Remoortere and Vinod Kone.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Addendum to b2b0eedc7371cd294b715fc91312989919d57c7a
> 
> 
> Diffs
> -
> 
>   src/tests/master_quota_tests.cpp 481298361574008b2cf9d213c908deb633393e31 
> 
> Diff: https://reviews.apache.org/r/40818/diff/
> 
> 
> Testing
> ---
> 
> Two test setups on Mac OS X 10.10.4.
> 
> `GTEST_FILTER="*AvailableResourcesAfterRescinding*" ./bin/mesos-tests.sh 
> --gtest_repeat=1000` *with* `allocation_interval` set to `50ms`
> The probability of a batch allocation between rescinding and `Shutdown()` is 
> low.
> 
> 
> `GTEST_FILTER="*AvailableResourcesAfterRescinding*" ./bin/mesos-tests.sh 
> --gtest_repeat=1000` *with* simulation of slow CI
> A batch allocation between rescinding and `Shutdown()` is triggered by 
> inserting the following lines after resources are recovered:
> ```
>   Clock::pause();
>   Clock::advance(flags.allocation_timeout);
>   Clock::settle();
>   Clock::resume();
> ```
> 
> 
> Thanks,
> 
> Alexander Rukletsov
> 
>

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

[Neil Conway]

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



docs/authorization.md (line 13)


"create-volumes" and "destroy-volumes".


- Neil Conway


On Nov. 14, 2015, 12:27 a.m., Greg Mann wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/40271/
> ---
> 
> (Updated Nov. 14, 2015, 12:27 a.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 0951ccb69daaa19b959e11cf3bf972a674a58305 
>   docs/reservation.md 81f21c3755b216b0932876c1ddd9de4d3fbe814a 
> 
> 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 40348: [4/4] Quota Authorization: Documented quota authorization.

[Alexander Rukletsov]

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



docs/authorization.md (lines 9 - 11)


Let's update this section as well.


- Alexander Rukletsov


On Dec. 2, 2015, 11:44 a.m., Jan Schlicht wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/40348/
> ---
> 
> (Updated Dec. 2, 2015, 11:44 a.m.)
> 
> 
> Review request for mesos, Alexander Rukletsov, Alexander Rojas, Joris Van 
> Remoortere, and Till Toenshoff.
> 
> 
> Bugs: MESOS-3862
> https://issues.apache.org/jira/browse/MESOS-3862
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Quota: Documented quota authorization.
> 
> 
> Diffs
> -
> 
>   docs/authorization.md f5ed75fcd0785fde38058917354fcf6d668dcccb 
>   src/master/flags.cpp f9223edb33483cb5619e7dd75221f735306462c6 
> 
> Diff: https://reviews.apache.org/r/40348/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Jan Schlicht
> 
>

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

[Alexander Rukletsov]

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


Do you think it makes sense to update "src/master/flags.cpp" as well?

- Alexander Rukletsov


On Nov. 14, 2015, 12:27 a.m., Greg Mann wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/40271/
> ---
> 
> (Updated Nov. 14, 2015, 12:27 a.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 0951ccb69daaa19b959e11cf3bf972a674a58305 
>   docs/reservation.md 81f21c3755b216b0932876c1ddd9de4d3fbe814a 
> 
> 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.

[Alexander Rukletsov]

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


Minor nits.


docs/authorization.md (line 13)


s/4./5.



docs/authorization.md (line 30)


s/6./7.


inor

- Alexander Rukletsov


On Nov. 14, 2015, 12:27 a.m., Greg Mann wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/40271/
> ---
> 
> (Updated Nov. 14, 2015, 12:27 a.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 0951ccb69daaa19b959e11cf3bf972a674a58305 
>   docs/reservation.md 81f21c3755b216b0932876c1ddd9de4d3fbe814a 
> 
> 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 40874: Restructured comments in authorizer.proto

[Michael Park]

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

Ship it!


Ship It!

- Michael Park


On Dec. 2, 2015, 4:25 p.m., Alexander Rukletsov wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/40874/
> ---
> 
> (Updated Dec. 2, 2015, 4:25 p.m.)
> 
> 
> Review request for mesos, Greg Mann, Michael Park, Jan Schlicht, and Till 
> Toenshoff.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Upcoming modification to "authorizer.proto" 
> (https://reviews.apache.org/r/39985/, https://reviews.apache.org/r/40345/) 
> introduce a clear and readable pattern for ACL comments. Update existing 
> comments to comply to that pattern.
> 
> 
> Diffs
> -
> 
>   include/mesos/authorizer/authorizer.proto 
> 8e72003f405770f00c5d87f318a9e1a8ed7430ee 
> 
> Diff: https://reviews.apache.org/r/40874/diff/
> 
> 
> Testing
> ---
> 
> Not a functional change.
> 
> 
> Thanks,
> 
> Alexander Rukletsov
> 
>

Re: Review Request 40873: SimpleRegistryPullerTest: Moved blob response preparation to the top.

[Anand Mazumdar]

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


@jojy, Do we know why preparing the response was taking so long that was 
causing the socket to timeout previously ?

- Anand Mazumdar


On Dec. 2, 2015, 4:26 p.m., Jojy Varghese wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/40873/
> ---
> 
> (Updated Dec. 2, 2015, 4:26 p.m.)
> 
> 
> Review request for mesos and Timothy Chen.
> 
> 
> Bugs: MESOS-4025
> https://issues.apache.org/jira/browse/MESOS-4025
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> By moving the preperation task above any test task avoids socket waiting for 
> the response to be prepared. This change has eliminated socket timeouts seen 
> sometimes.
> 
> 
> Diffs
> -
> 
>   src/tests/containerizer/provisioner_docker_tests.cpp 
> c63bf53fee40ef12536a16e11f4d5224c4e4278e 
> 
> Diff: https://reviews.apache.org/r/40873/diff/
> 
> 
> Testing
> ---
> 
> make check (600 count).
> 
> 
> Thanks,
> 
> Jojy Varghese
> 
>

Re: Review Request 40874: Restructured comments in authorizer.proto

[Jan Schlicht]

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

Ship it!


Ship It!

- Jan Schlicht


On Dec. 2, 2015, 5:25 p.m., Alexander Rukletsov wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/40874/
> ---
> 
> (Updated Dec. 2, 2015, 5:25 p.m.)
> 
> 
> Review request for mesos, Greg Mann, Michael Park, Jan Schlicht, and Till 
> Toenshoff.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Upcoming modification to "authorizer.proto" 
> (https://reviews.apache.org/r/39985/, https://reviews.apache.org/r/40345/) 
> introduce a clear and readable pattern for ACL comments. Update existing 
> comments to comply to that pattern.
> 
> 
> Diffs
> -
> 
>   include/mesos/authorizer/authorizer.proto 
> 8e72003f405770f00c5d87f318a9e1a8ed7430ee 
> 
> Diff: https://reviews.apache.org/r/40874/diff/
> 
> 
> Testing
> ---
> 
> Not a functional change.
> 
> 
> Thanks,
> 
> Alexander Rukletsov
> 
>

Re: Review Request 40874: Restructured comments in authorizer.proto

[Till Toenshoff]

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

Ship it!


Ship It!

- Till Toenshoff


On Dec. 2, 2015, 4:25 p.m., Alexander Rukletsov wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/40874/
> ---
> 
> (Updated Dec. 2, 2015, 4:25 p.m.)
> 
> 
> Review request for mesos, Greg Mann, Michael Park, Jan Schlicht, and Till 
> Toenshoff.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Upcoming modification to "authorizer.proto" 
> (https://reviews.apache.org/r/39985/, https://reviews.apache.org/r/40345/) 
> introduce a clear and readable pattern for ACL comments. Update existing 
> comments to comply to that pattern.
> 
> 
> Diffs
> -
> 
>   include/mesos/authorizer/authorizer.proto 
> 8e72003f405770f00c5d87f318a9e1a8ed7430ee 
> 
> Diff: https://reviews.apache.org/r/40874/diff/
> 
> 
> Testing
> ---
> 
> Not a functional change.
> 
> 
> Thanks,
> 
> Alexander Rukletsov
> 
>

Review Request 40873: SimpleRegistryPullerTest: Moved blob response preparation to the top.

[Jojy Varghese]

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

Review request for mesos and Timothy Chen.


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


Repository: mesos


Description
---

By moving the preperation task above any test task avoids socket waiting for 
the response to be prepared. This change has eliminated socket timeouts seen 
sometimes.


Diffs
-

  src/tests/containerizer/provisioner_docker_tests.cpp 
c63bf53fee40ef12536a16e11f4d5224c4e4278e 

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


Testing
---

make check (600 count).


Thanks,

Jojy Varghese

Review Request 40874: Restructured comments in authorizer.proto

[Alexander Rukletsov]

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

Review request for mesos, Greg Mann, Michael Park, Jan Schlicht, and Till 
Toenshoff.


Repository: mesos


Description
---

Upcoming modification to "authorizer.proto" 
(https://reviews.apache.org/r/39985/, https://reviews.apache.org/r/40345/) 
introduce a clear and readable pattern for ACL comments. Update existing 
comments to comply to that pattern.


Diffs
-

  include/mesos/authorizer/authorizer.proto 
8e72003f405770f00c5d87f318a9e1a8ed7430ee 

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


Testing
---

Not a functional change.


Thanks,

Alexander Rukletsov

Review Request 40872: Changed RegistryClientTest to use instance work directory.

[Jojy Varghese]

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

Review request for mesos and Timothy Chen.


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


Repository: mesos


Description
---

RegistryClientTest suite uses static variable for is work directory. This has
been changed to model the TemporaryDirectoryTest suite.


Diffs
-

  src/tests/containerizer/provisioner_docker_tests.cpp 
c63bf53fee40ef12536a16e11f4d5224c4e4278e 

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


Testing
---

make check (600 times).


Thanks,

Jojy Varghese

Re: Review Request 40266: Libprocess Reinitialization: Cleanup SocketManager along side ProcessManager.

[Benjamin Bannier]

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



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


While using a raw pointer here lets you not worry about `socket_mutex`' 
lifetime, it does create a future false positive for any leak checks.

Instead you should really try to be explicit. You could e.g., use a 
`shared_ptr`, and, if you feel this adds too much noise to the 
call sites, pass that one to the callbacks as an implicit parameter, i.e. 
declare

void finalize(shared_ptr m = process::socket_mutex);

and similarly for `internal::on_accept`.



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


You should be more explicit about lifetimes here and use `unique_ptrs` of 
.. instead (you can always `reset` in place of `delete` if you need to destroy 
at a certain point).



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


Once you use a proper smart pointer for `process_route` this comment will 
be right (maybe `s/deleted/cleaned up/`).



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


I find adding another manual iteration index manipulation here makes this 
even harder to read (e.g., do we really iterate over all elements? Are there 
assumptions about ordering (hopefully not)?, ...). 

You could e.g., factor out a `synchronized` helper to get the next 
not-ignored element (or a `nullptr` if nothing is left); the whole existing 
loop could then collapse to

while (true) {
  ProcessBase* process = next_cleanup(processes));
  if (!process) {
break;
  }
  process::terminate(process, false);
  process::wait(process);
}

This would also make it clear that we intent over all not-ignored processes 
(which currently is implicit through `wait` removing processes one after the 
other, and `processes` not containing any `nullptr` elements).

The helper `next_cleanup` can be implemented without querying size 
information, e.g., it could iterate `processes` until the process doesn't 
pattern-match with ignored processes.


- Benjamin Bannier


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

Re: Review Request 39989: [5/5] Added framework authorization for dynamic reservation.

[Alexander Rukletsov]

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



src/master/master.cpp (lines 3029 - 3030)


Blank line? Here and below



src/master/master.cpp (lines 3032 - 3034)


We validate later on that in `principal` is `None`, reserve is aborted. 
IIUC, if `FrameworkInfo` does not specify principal, it cannot reserve 
resources. So the question is: do we need to check this first and only then 
proceed with authz?



src/master/master.cpp (line 3180)


s/Testing/Test for consistency, here and below.


- Alexander Rukletsov


On Dec. 2, 2015, 9:41 a.m., Greg Mann wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/39989/
> ---
> 
> (Updated Dec. 2, 2015, 9:41 a.m.)
> 
> 
> Review request for mesos, Adam B, Jie Yu, Michael Park, and Till Toenshoff.
> 
> 
> Bugs: MESOS-3062
> https://issues.apache.org/jira/browse/MESOS-3062
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added framework authorization for dynamic reservation.
> Note: this review is continued from https://reviews.apache.org/r/37127/
> 
> 
> Diffs
> -
> 
>   src/master/master.cpp b918ae4a0e7dc3cd41165fc4b683ae7b6f031821 
>   src/tests/reservation_tests.cpp 15d180f92ec0aea99e6f3a7d0b505c62bd207b61 
> 
> Diff: https://reviews.apache.org/r/39989/diff/
> 
> 
> Testing
> ---
> 
> This is the fifth in a chain of 5 patches. New reservation tests were added 
> to `reservation_tests.cpp` to validate the authentication of framework 
> reserve and unreserve operations using ACLs. `make check` was run to test 
> after all patches were applied.
> 
> 
> Thanks,
> 
> Greg Mann
> 
>

Re: Review Request 39987: [3/5] Added 'Master::authorize(Un)reserveResources()' for Reserve/Unreserve.

[Alexander Rukletsov]

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



src/master/master.cpp (lines 2769 - 2770)


Is my understanding is correct, we do not allow reserving resources neither 
for operators nor for frameworks without a principal. Why do we have a path for 
`ACL::Entity::Any` here then? If you plan to reserve it for future use cases or 
just be more general — it's fine, but let's leave a fat comment that this is 
not possible currently (or how it's possible if my understanding is not 
correct).

A follow-up question: Is it something we have agreed and maybe even 
documented somewhere, that certain actions require principal? I'm thinking 
about authz for quota and whether we have to make `principal` required there.


- Alexander Rukletsov


On Dec. 2, 2015, 9 a.m., Greg Mann wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/39987/
> ---
> 
> (Updated Dec. 2, 2015, 9 a.m.)
> 
> 
> Review request for mesos, Adam B, Jie Yu, Michael Park, and Till Toenshoff.
> 
> 
> Bugs: MESOS-3062
> https://issues.apache.org/jira/browse/MESOS-3062
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added 'Master::authorize(Un)reserveResources()' for Reserve/Unreserve.
> Note: this review is continued from https://reviews.apache.org/r/37125/
> 
> 
> Diffs
> -
> 
>   src/master/master.hpp 96951e766de32842197506504e5ac67a2caa3efe 
>   src/master/master.cpp b918ae4a0e7dc3cd41165fc4b683ae7b6f031821 
> 
> Diff: https://reviews.apache.org/r/39987/diff/
> 
> 
> Testing
> ---
> 
> This is the third in a chain of 5 patches. `make check` was used to test 
> after all patches were applied.
> 
> 
> Thanks,
> 
> Greg Mann
> 
>

Re: Review Request 40347: [3/4] Quota Authorization: Implemented quota request authorization.

[Alexander Rukletsov]

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



include/mesos/quota/quota.proto (lines 43 - 45)


Do you think `principal` should be required? Does it mean quota cannot be 
requested without providing `Authorization` header? I think you do so in 
`authorize()` below.

I think it's also fine to prohibit setting quota without prinicipal, but 
let's be consitent and explicit about it.



src/master/master.hpp (lines 899 - 900)


Do you need the whole `QuotaInfo` or optional `principal` and role would 
suffice?



src/master/quota_handler.cpp (line 82)


Do we need `Credential` here or principal would suffice?



src/master/quota_handler.cpp (lines 94 - 96)


I think this may be buggy: `principal` is required, however we do not 
ensure it is present here, means we may create a protbuf message, that we 
cannot deserialize.



src/master/quota_handler.cpp (lines 236 - 240)


Can we re-write it using a lambda? This way you do not need to inject 
`authorized` into the continuation.

```
authorize(quotaInfo)
  .then(defer(master->self(), [=](bool authorized) -> Future {
if (!authorized) {
  return Unauthorized("Mesos master");
}

_set(quotaInfo);
  }));
```



src/master/quota_handler.cpp (lines 333 - 334)


Blank line?



src/master/quota_handler.cpp (lines 338 - 339)


Blank line?



src/master/quota_handler.cpp (lines 339 - 340)


Let's wrap variable and function names in backticks.

Also, how about re-wording it a bit for brevity? Something like 
"`quotaInfo` is already validated, role is quaranteed to be present".


- Alexander Rukletsov


On Nov. 20, 2015, 10:03 a.m., Jan Schlicht wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/40347/
> ---
> 
> (Updated Nov. 20, 2015, 10:03 a.m.)
> 
> 
> Review request for mesos, Alexander Rukletsov, Alexander Rojas, Joris Van 
> Remoortere, and Till Toenshoff.
> 
> 
> Bugs: MESOS-3862
> https://issues.apache.org/jira/browse/MESOS-3862
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Quota: Implemented quota request authorization.
> 
> 
> Diffs
> -
> 
>   include/mesos/quota/quota.proto 4e4d8ccc92e2bf9a8e5eae8488c0c952f82fdd6d 
>   src/master/master.hpp 5e5a575dc7dd49324f3c837028df8a7f75cd1f80 
>   src/master/quota_handler.cpp 03cef4117c52da7599a2800060f65483ca33bc3f 
> 
> Diff: https://reviews.apache.org/r/40347/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Jan Schlicht
> 
>

Re: Review Request 39988: [4/5] Added authorization for dynamic reservation master endpoints.

[Alexander Rukletsov]

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



src/master/http.cpp (lines 1034 - 1037)


Let's leave a comment here, that `principal` matches 
`reservation().principal()` for each resource in 
`operation.reserve().resources()`, hence it's OK to authorize for `principal` 
and use `reservation().principal()` in `unreserve()`. Maybe a symmetrical 
comment in `unreserve()` path would also make sense.

Maybe if you validate before authorizing it will be more easy to understand?


- Alexander Rukletsov


On Dec. 2, 2015, 9:06 a.m., Greg Mann wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/39988/
> ---
> 
> (Updated Dec. 2, 2015, 9:06 a.m.)
> 
> 
> Review request for mesos, Adam B, Jie Yu, Michael Park, and Till Toenshoff.
> 
> 
> Bugs: MESOS-3062
> https://issues.apache.org/jira/browse/MESOS-3062
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added authorization for dynamic reservation master endpoints.
> Note: this review is continued from https://reviews.apache.org/r/37126/
> 
> 
> Diffs
> -
> 
>   src/master/http.cpp 9d729ef7f7d7ad6185934648f833e4f8a4f0a123 
>   src/master/master.hpp 96951e766de32842197506504e5ac67a2caa3efe 
>   src/tests/reservation_endpoints_tests.cpp 
> f30ff8bc6a3e9773437fa7fd7c8f569b7d7e2d9d 
> 
> Diff: https://reviews.apache.org/r/39988/diff/
> 
> 
> Testing
> ---
> 
> This is the fourth in a chain of 5 patches. Added new reservation endpoints 
> tests to validate authorization of reserve and unreserve operations using 
> ACLs. `make check` was run to test after all patches were applied.
> 
> 
> Thanks,
> 
> Greg Mann
> 
>

Re: Review Request 39988: [4/5] Added authorization for dynamic reservation master endpoints.

[Alexander Rukletsov]

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



src/master/master.hpp (lines 1089 - 1094)


Do you want to do this cleanup as a separate patch? I believe we tend not 
to conflate semantic and stylistic changes in one patch.


- Alexander Rukletsov


On Dec. 2, 2015, 9:06 a.m., Greg Mann wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/39988/
> ---
> 
> (Updated Dec. 2, 2015, 9:06 a.m.)
> 
> 
> Review request for mesos, Adam B, Jie Yu, Michael Park, and Till Toenshoff.
> 
> 
> Bugs: MESOS-3062
> https://issues.apache.org/jira/browse/MESOS-3062
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added authorization for dynamic reservation master endpoints.
> Note: this review is continued from https://reviews.apache.org/r/37126/
> 
> 
> Diffs
> -
> 
>   src/master/http.cpp 9d729ef7f7d7ad6185934648f833e4f8a4f0a123 
>   src/master/master.hpp 96951e766de32842197506504e5ac67a2caa3efe 
>   src/tests/reservation_endpoints_tests.cpp 
> f30ff8bc6a3e9773437fa7fd7c8f569b7d7e2d9d 
> 
> Diff: https://reviews.apache.org/r/39988/diff/
> 
> 
> Testing
> ---
> 
> This is the fourth in a chain of 5 patches. Added new reservation endpoints 
> tests to validate authorization of reserve and unreserve operations using 
> ACLs. `make check` was run to test after all patches were applied.
> 
> 
> Thanks,
> 
> Greg Mann
> 
>

Re: Review Request 39987: [3/5] Added 'Master::authorize(Un)reserveResources()' for Reserve/Unreserve.

[Alexander Rukletsov]

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


A drive-by review, I'm looking into authz and ACLs and learning from your 
patches.


src/master/master.cpp (line 2775)


I believe you inherited the TODO from MPark. Do you want to preserve his 
name or change to yours?



src/master/master.cpp (line 2781)


Maybe "' to reserve any resources"? Or mention what resources are allowed 
to reserve (similar to what you do in `authorizeUnreserveResources`.



src/master/master.cpp (lines 2811 - 2812)


Fits one line (clang-format suggests the same).


- Alexander Rukletsov


On Dec. 2, 2015, 9 a.m., Greg Mann wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/39987/
> ---
> 
> (Updated Dec. 2, 2015, 9 a.m.)
> 
> 
> Review request for mesos, Adam B, Jie Yu, Michael Park, and Till Toenshoff.
> 
> 
> Bugs: MESOS-3062
> https://issues.apache.org/jira/browse/MESOS-3062
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added 'Master::authorize(Un)reserveResources()' for Reserve/Unreserve.
> Note: this review is continued from https://reviews.apache.org/r/37125/
> 
> 
> Diffs
> -
> 
>   src/master/master.hpp 96951e766de32842197506504e5ac67a2caa3efe 
>   src/master/master.cpp b918ae4a0e7dc3cd41165fc4b683ae7b6f031821 
> 
> Diff: https://reviews.apache.org/r/39987/diff/
> 
> 
> Testing
> ---
> 
> This is the third in a chain of 5 patches. `make check` was used to test 
> after all patches were applied.
> 
> 
> Thanks,
> 
> Greg Mann
> 
>

Re: Review Request 40346: [2/4] Quota Authorization: Implemented authorization of quota requests in the authorizer.

[Alexander Rukletsov]

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

Ship it!



include/mesos/authorizer/authorizer.hpp (lines 136 - 141)


Let' reformat the comment to avoid jaggedness. Also, let's backtick 
`request` since it refers to the variable name and remove the artice then.



include/mesos/authorizer/authorizer.hpp (line 143)


Backtick `ACL::SetQuota`

Does reformatting avoid jaggedness here?



include/mesos/authorizer/authorizer.hpp (line 145)


s/with/for



include/mesos/authorizer/authorizer.hpp (lines 147 - 148)


Formatting



src/tests/mesos.hpp (lines 1245 - 1247)


Please move this into .cpp (as part of the rebase).


- Alexander Rukletsov


On Nov. 20, 2015, 10:03 a.m., Jan Schlicht wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/40346/
> ---
> 
> (Updated Nov. 20, 2015, 10:03 a.m.)
> 
> 
> Review request for mesos, Alexander Rukletsov, Alexander Rojas, Joris Van 
> Remoortere, and Till Toenshoff.
> 
> 
> Bugs: MESOS-3862
> https://issues.apache.org/jira/browse/MESOS-3862
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Quota: Implemented authorization of quota requests in the authorizer.
> 
> 
> Diffs
> -
> 
>   include/mesos/authorizer/authorizer.hpp 
> d667a52f90f970a313580446a5a006cec4b5e25b 
>   src/authorizer/local/authorizer.hpp 
> 32de102fd588f029882ef121ca83a7410c65 
>   src/authorizer/local/authorizer.cpp 
> 6d7da87731a438c2180cf91003e09d4aa5a1c773 
>   src/tests/mesos.hpp 25074a0b8d86b83c5820f7a5a5e10b4ba9efb1ed 
> 
> Diff: https://reviews.apache.org/r/40346/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Jan Schlicht
> 
>

Re: Review Request 40348: [4/4] Quota Authorization: Documented quota authorization.

[Mesos ReviewBot]

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


Bad patch!

Reviews applied: [39211, 39018, 39102, 36913, 38059, 39285, 38110, 39288]

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

Error:
 2015-12-02 13:03:55 URL:https://reviews.apache.org/r/39288/diff/raw/ 
[3292/3292] -> "39288.patch" [1]
error: patch failed: src/master/master.hpp:898
error: src/master/master.hpp: patch does not apply
error: patch failed: src/master/quota_handler.cpp:35
error: src/master/quota_handler.cpp: patch does not apply
Failed to apply patch

- Mesos ReviewBot


On Dec. 2, 2015, 11:44 a.m., Jan Schlicht wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/40348/
> ---
> 
> (Updated Dec. 2, 2015, 11:44 a.m.)
> 
> 
> Review request for mesos, Alexander Rukletsov, Alexander Rojas, Joris Van 
> Remoortere, and Till Toenshoff.
> 
> 
> Bugs: MESOS-3862
> https://issues.apache.org/jira/browse/MESOS-3862
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Quota: Documented quota authorization.
> 
> 
> Diffs
> -
> 
>   docs/authorization.md f5ed75fcd0785fde38058917354fcf6d668dcccb 
>   src/master/flags.cpp f9223edb33483cb5619e7dd75221f735306462c6 
> 
> Diff: https://reviews.apache.org/r/40348/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Jan Schlicht
> 
>

Re: Review Request 40445: Added linter for license headers in some file types.

[Mesos ReviewBot]

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


Patch looks great!

Reviews applied: [39590, 39591, 39592, 40445]

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

- Mesos ReviewBot


On Dec. 2, 2015, 11:18 a.m., Benjamin Bannier wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/40445/
> ---
> 
> (Updated Dec. 2, 2015, 11:18 a.m.)
> 
> 
> Review request for mesos, Benjamin Hindman, Marco Massenzio, and Michael Park.
> 
> 
> Bugs: MESOS-3581
> https://issues.apache.org/jira/browse/MESOS-3581
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added linter for license headers in some file types.
> 
> 
> Diffs
> -
> 
>   support/mesos-style.py 66b45692c3c04f68358b63d52e4d87934f241bd7 
> 
> Diff: https://reviews.apache.org/r/40445/diff/
> 
> 
> Testing
> ---
> 
> Ran the a whole clean checkout through the linter with only one expected 
> failure (`3rdparty/libprocess/stout/tests/protobuf_tests.proto` which lacks a 
> license).
> 
> 
> NOTE TO THE COMMITTER
> -
> 
> Before committing this, it is probably a good idea to check the whole code 
> base again and fix any new files which do not follow the current license 
> style. The commits which originally fixed this were
> 
> * fa36917 (mesos),
> * dc23756 (stout), and
> * 3539b7a (libprocess).
> 
> 
> Thanks,
> 
> Benjamin Bannier
> 
>

Re: Review Request 40348: [4/4] Quota Authorization: Documented quota authorization.

[Jan Schlicht]

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

(Updated Dec. 2, 2015, 12:44 p.m.)


Review request for mesos, Alexander Rukletsov, Alexander Rojas, Joris Van 
Remoortere, and Till Toenshoff.


Changes
---

Fix wrong naming.


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


Repository: mesos


Description
---

Quota: Documented quota authorization.


Diffs (updated)
-

  docs/authorization.md f5ed75fcd0785fde38058917354fcf6d668dcccb 
  src/master/flags.cpp f9223edb33483cb5619e7dd75221f735306462c6 

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


Testing
---

make check


Thanks,

Jan Schlicht

Re: Review Request 40345: [1/4] Quota Authorization: Added "SetQuota" message to ACL protobuf.

[Alexander Rukletsov]

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



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


Let's clarify what subjects are, how about
// Subjects: Operator usernames.



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


How about:
// Objects: The list of roles for which a quota can be set.


- Alexander Rukletsov


On Nov. 20, 2015, 4:13 p.m., Jan Schlicht wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/40345/
> ---
> 
> (Updated Nov. 20, 2015, 4:13 p.m.)
> 
> 
> Review request for mesos, Alexander Rukletsov, Alexander Rojas, Joris Van 
> Remoortere, and Till Toenshoff.
> 
> 
> Bugs: MESOS-3862
> https://issues.apache.org/jira/browse/MESOS-3862
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Quota: Added "SetQuota" message to ACL protobuf.
> 
> 
> Diffs
> -
> 
>   include/mesos/authorizer/authorizer.proto 
> 86bbb45f9d91b4098a262e3e50a793f3bb39497e 
> 
> Diff: https://reviews.apache.org/r/40345/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Jan Schlicht
> 
>