Re: Review Request 56753: Implemented the JWT authenticator.

[Greg Mann]

> On March 1, 2017, 7:41 a.m., Greg Mann wrote:
> > Note that the upstream `AuthenticationContext` patches have been updated to 
> > rename the new type to `Principal`.

Ah right, looks like the diff has already been updated to use `Principal`, 
thx!! Could you update the Description as well?


- Greg


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


On Feb. 28, 2017, 1:36 p.m., Jan Schlicht wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/56753/
> ---
> 
> (Updated Feb. 28, 2017, 1:36 p.m.)
> 
> 
> Review request for mesos, Alexander Rojas and Greg Mann.
> 
> 
> Bugs: MESOS-7001
> https://issues.apache.org/jira/browse/MESOS-7001
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This HTTP authenticator extracts a JWT from the requests' authorization
> header using the 'Bearer' schema and validates it against a secret using
> HMAC SHA256. The 'sub' claim of the JWT is the extracted principal, all
> other claims will be additional labels of the 'AuthenticationContext'.
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/Makefile.am 75386184108214e67a58c328258ec204099d638c 
>   3rdparty/libprocess/include/process/authenticator.hpp 
> e5489c6cb4adc8a822e7dd4515542618c36136f9 
>   3rdparty/libprocess/src/authenticator.cpp 
> cfedb6f7674e0f6690e77a633cdd1bd494c7d2c7 
>   3rdparty/libprocess/src/jwt_authenticator.cpp PRE-CREATION 
>   3rdparty/libprocess/src/tests/http_tests.cpp 
> fb4da9aecff0370d97a15269c5d8fffb30e0478f 
> 
> 
> Diff: https://reviews.apache.org/r/56753/diff/3/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Jan Schlicht
> 
>

Re: Review Request 56753: Implemented the JWT authenticator.

[Greg Mann]

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



Note that the upstream `AuthenticationContext` patches have been updated to 
rename the new type to `Principal`.


3rdparty/libprocess/include/process/authenticator.hpp
Lines 186-189 (patched)


`override` keyword implies `virtual`, so the `virtual` here can be removed



3rdparty/libprocess/src/tests/http_tests.cpp
Lines 2002-2003 (patched)


Fits on one line.



3rdparty/libprocess/src/tests/http_tests.cpp
Lines 2011-2013 (patched)


Fits on one line.



3rdparty/libprocess/src/tests/http_tests.cpp
Lines 2025-2027 (patched)


Fits on one line.



3rdparty/libprocess/src/tests/http_tests.cpp
Lines 2038-2039 (patched)


Why split these across two lines?



3rdparty/libprocess/src/tests/http_tests.cpp
Lines 2044-2046 (patched)


Fits on one line.



3rdparty/libprocess/src/tests/http_tests.cpp
Lines 2055-2057 (patched)


Fits on one line.



3rdparty/libprocess/src/tests/http_tests.cpp
Lines 2060-2063 (patched)


Is this `JWT` used for anything? It might be nice to use this to create the 
token string rather than relying on a hard-coded string literal?



3rdparty/libprocess/src/tests/http_tests.cpp
Lines 2067-2068 (patched)


Ditto as above - can we consolidate to one line?


- Greg Mann


On Feb. 28, 2017, 1:36 p.m., Jan Schlicht wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/56753/
> ---
> 
> (Updated Feb. 28, 2017, 1:36 p.m.)
> 
> 
> Review request for mesos, Alexander Rojas and Greg Mann.
> 
> 
> Bugs: MESOS-7001
> https://issues.apache.org/jira/browse/MESOS-7001
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This HTTP authenticator extracts a JWT from the requests' authorization
> header using the 'Bearer' schema and validates it against a secret using
> HMAC SHA256. The 'sub' claim of the JWT is the extracted principal, all
> other claims will be additional labels of the 'AuthenticationContext'.
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/Makefile.am 75386184108214e67a58c328258ec204099d638c 
>   3rdparty/libprocess/include/process/authenticator.hpp 
> e5489c6cb4adc8a822e7dd4515542618c36136f9 
>   3rdparty/libprocess/src/authenticator.cpp 
> cfedb6f7674e0f6690e77a633cdd1bd494c7d2c7 
>   3rdparty/libprocess/src/jwt_authenticator.cpp PRE-CREATION 
>   3rdparty/libprocess/src/tests/http_tests.cpp 
> fb4da9aecff0370d97a15269c5d8fffb30e0478f 
> 
> 
> Diff: https://reviews.apache.org/r/56753/diff/3/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Jan Schlicht
> 
>

Re: Review Request 56667: Added support for JSON Web Tokens.

[Greg Mann]

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



Sorry for not noticing this before: since this is a libprocess patch, all 
function and variable names should be in snake_case. CamelCase is used only for 
struct/class names in libprocess.


3rdparty/libprocess/src/jwt.cpp
Lines 36 (patched)


Could we make this a private member function?



3rdparty/libprocess/src/jwt.cpp
Lines 54 (patched)


This could be a private member function as well?

Also, should use snake_case in libprocess for function/variable names I 
believe. See note at top of review.



3rdparty/libprocess/src/jwt.cpp
Lines 59-60 (patched)


Fits on one line.



3rdparty/libprocess/src/jwt.cpp
Lines 67 (patched)


As long as you're touching these, maybe names like `typ_json` would be more 
descriptive?



3rdparty/libprocess/src/jwt.cpp
Lines 91 (patched)


Does this need to be an `Option`? Looks like it could just be `JWT::Alg 
alg;`.



3rdparty/libprocess/src/jwt.cpp
Lines 94-95 (patched)


Join on one line:

`} else if ( ... ) {`

Could also use a `switch` statement here instead.



3rdparty/libprocess/src/jwt.cpp
Lines 126-127 (patched)


Fits on one line.



3rdparty/libprocess/src/jwt.cpp
Lines 173-175 (patched)


Could fit onto just two lines.


- Greg Mann


On Feb. 27, 2017, 11:37 a.m., Jan Schlicht wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/56667/
> ---
> 
> (Updated Feb. 27, 2017, 11:37 a.m.)
> 
> 
> Review request for mesos, Alexander Rojas and Greg Mann.
> 
> 
> Bugs: MESOS-7001
> https://issues.apache.org/jira/browse/MESOS-7001
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> JSON Web Tokens can be used to create claim-based access tokens and is
> typically used for HTTP authentication.
> This implementation is intended for internal use, e.g. Mesos is supposed
> to only parse tokens that it also created. It doesn't fully comply with
> RFC 7519. Currently the only supported cryptographic algorithm is HMAC
> with SHA-256.
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/Makefile.am 75386184108214e67a58c328258ec204099d638c 
>   3rdparty/libprocess/include/process/jwt.hpp PRE-CREATION 
>   3rdparty/libprocess/src/jwt.cpp PRE-CREATION 
>   3rdparty/libprocess/src/tests/jwt_tests.cpp PRE-CREATION 
> 
> 
> Diff: https://reviews.apache.org/r/56667/diff/4/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Jan Schlicht
> 
>

Re: Review Request 57167: Updated quota handler logic for hierarchical roles.

[Mesos Reviewbot]

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



Bad review!

Reviews applied: [57167, 57166, 56805, 57165, 57164]

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

- Mesos Reviewbot


On Feb. 28, 2017, 8:23 p.m., Neil Conway wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/57167/
> ---
> 
> (Updated Feb. 28, 2017, 8:23 p.m.)
> 
> 
> Review request for mesos and Michael Park.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> The quota'd resources for a nested role are "included" within the
> quota'd resources for that role's parent. Hence, the quota of a node
> must always be greater than or equal to the sum of the quota'd resources
> of that role's children.
> 
> When creating and removing quota, we must ensure that this invariant is
> not violated.
> 
> When computing the cluster capacity heuristic, we must ensure that we do
> not "double-count" quota'd resources: e.g., if the cluster has a total
> capacity of 100 cpus, role "x" has a quota guarantee of 60 CPUs, and
> role "x/y" has a quota guarantee of 55 CPUs, this does NOT violate the
> cluster capacity heuristic.
> 
> 
> Diffs
> -
> 
>   src/master/quota_handler.cpp 3ad28e4a9363a877d0610b529a6c17fb30ece37a 
>   src/tests/hierarchical_allocator_tests.cpp 
> cdf1f15b7802439b28405ca8f6634ce83e886630 
>   src/tests/master_quota_tests.cpp 91219d6693fdd119ed3b0bf734eaa55da9c58b0a 
> 
> Diff: https://reviews.apache.org/r/57167/diff/
> 
> 
> Testing
> ---
> 
> `make check`
> 
> 
> Thanks,
> 
> Neil Conway
> 
>

Re: Review Request 57052: CMake: Add `build_config.hpp.in` for `BUILD_TIME` variables.

[Joseph Wu]

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


Ship it!




I'll make the changes and ship this if you agree with the renaming (see below).


cmake/CompilationConfigure.cmake (line 291)


Not yours, but looks like this one actually doesn't work on Posix.

It ends up as:
```
#define BUILD_TIME "%s"
```

I'll fix it while I'm modifying this part of the build system:
```
  execute_process(
COMMAND date +%s
OUTPUT_VARIABLE BUILD_TIME
OUTPUT_STRIP_TRAILING_WHITESPACE
  )
```



cmake/CompilationConfigure.cmake (line 297)


Would be better with a comment :D
```
# Emit the BUILD_DATE, BUILD_TIME, and BUILD_USER variables into a file.
# This will be updated each time `cmake` is run.
```



cmake/CompilationConfigure.cmake (line 299)


Just for symmetry, let's put this in

"${CMAKE_BINARY_DIR}/src/common/build_config.hpp"



cmake/CompilationConfigure.cmake (line 312)


What do you feel about "USE_CMAKE_BUILD_CONFIG" instead?  
"HAVE_BUILD_CONFIG_H" seems too much like a header guard.



src/common/build.cpp (line 26)


Would help to have a nice beefy comment :)
```
// NOTE: On CMake, instead of defining `BUILD_DATE|TIME|USER` as
// compiler flags, we instead emit a header file with the definitions.
// This facilitates incremental builds as the compiler flags will
// no longer change with every invocation of the build.
// TODO(josephw): After deprecating autotools, remove this guard.
```


- Joseph Wu


On Feb. 28, 2017, 4:42 p.m., Andrew Schwartzmeyer wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/57052/
> ---
> 
> (Updated Feb. 28, 2017, 4:42 p.m.)
> 
> 
> Review request for mesos, Alex Clemmer, Joseph Wu, and Michael Park.
> 
> 
> Bugs: MESOS-7172
> https://issues.apache.org/jira/browse/MESOS-7172
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Commit c7fc1377b introduced a bug that prevented any incremental
> recompilation. By defining the `BUILD_TIME` in `MESOS_CPPFLAGS`,
> every build was seen as having a root dependency (the flags)
> changed, causing a rebuild of every single source in Mesos.
> 
> This patch introduces a CMake `configure_file` directive which
> takes in `build_config.hpp.in` and emits `build_config.hpp` with the
> `BUILD_TIME`, `BUILD_DATE`, and `BUILD_USER` variables defined.
> It also sets `HAVE_BUILD_CONFIG_H` which `build.cpp` uses to `#include`
> the configuration file. The result is that the date, time, and user
> are set at the point of configuration (invocation of CMake) instead
> of at build, thus allowing for incremental rebuilds.
> 
> 
> Diffs
> -
> 
>   cmake/CompilationConfigure.cmake 7808d1a0086a791af8dcb2dd796b9424c7d5 
>   src/common/build.cpp 090b59fb22bfc00516d65f501f8f18cd85a5b4cd 
>   src/common/build_config.hpp.in PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/57052/diff/
> 
> 
> Testing
> ---
> 
> cmake; make && make check on Linux
> cmake; VS build stout-tests and mesos-tests, then repeat and see no 
> compilation take place a second time. Also support\windows-build.bat twice.
> 
> 
> Thanks,
> 
> Andrew Schwartzmeyer
> 
>

Re: Review Request 57121: Renamed 'SubprocessInfo' to 'ContainerIO' in 'ContainerLogger'.

[Jie Yu]

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


Ship it!




Ship It!

- Jie Yu


On March 1, 2017, 1:41 a.m., Kevin Klues wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/57121/
> ---
> 
> (Updated March 1, 2017, 1:41 a.m.)
> 
> 
> Review request for mesos, Jie Yu and Joseph Wu.
> 
> 
> Bugs: MESOS-7050
> https://issues.apache.org/jira/browse/MESOS-7050
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Renamed 'SubprocessInfo' to 'ContainerIO' in 'ContainerLogger'.
> 
> 
> Diffs
> -
> 
>   include/mesos/slave/container_logger.hpp 
> a3f619b79ca0188df9e231c600dfa396f39ab29a 
>   src/slave/container_loggers/lib_logrotate.hpp 
> e37d99cb268bb3286e312d2ebdbaf84d3fd4bf91 
>   src/slave/container_loggers/lib_logrotate.cpp 
> b257f48f819985e339a5a7fd8066ffa9f39df7a6 
>   src/slave/container_loggers/sandbox.hpp 
> 4ec090cbfc5834ead45ec39c3a646f491fe892cb 
>   src/slave/container_loggers/sandbox.cpp 
> b55e089877f205bab482ae4ebe5a2010aeebeb47 
>   src/slave/containerizer/docker.cpp 7d801fb17565a5298e8e3c5b430e070e12473680 
>   src/slave/containerizer/mesos/io/switchboard.hpp 
> 5b1fa25d5f577ce3c232fdf5324c7f9c837a64ce 
>   src/slave/containerizer/mesos/io/switchboard.cpp 
> 820d53bc12b1bf6018132660e4b7c5eae8c1e2ee 
>   src/tests/container_logger_tests.cpp 
> 589d6a9df7ce964052355be41597ef11677ca03d 
> 
> Diff: https://reviews.apache.org/r/57121/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Kevin Klues
> 
>

Re: Review Request 57176: Pulled 'ContainerIO' out of the 'ContainerLogger'.

[Jie Yu]

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


Ship it!




Ship It!

- Jie Yu


On March 1, 2017, 1:43 a.m., Kevin Klues wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/57176/
> ---
> 
> (Updated March 1, 2017, 1:43 a.m.)
> 
> 
> Review request for mesos and Jie Yu.
> 
> 
> Bugs: MESOS-7050
> https://issues.apache.org/jira/browse/MESOS-7050
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Pulled 'ContainerIO' out of the 'ContainerLogger'.
> 
> 
> Diffs
> -
> 
>   include/mesos/slave/container_logger.hpp 
> a3f619b79ca0188df9e231c600dfa396f39ab29a 
>   include/mesos/slave/containerizer.hpp 
> d0096b9e0478478cc8184b1ea6bfecafb1a8620f 
>   src/slave/container_loggers/lib_logrotate.hpp 
> e37d99cb268bb3286e312d2ebdbaf84d3fd4bf91 
>   src/slave/container_loggers/lib_logrotate.cpp 
> b257f48f819985e339a5a7fd8066ffa9f39df7a6 
>   src/slave/container_loggers/sandbox.hpp 
> 4ec090cbfc5834ead45ec39c3a646f491fe892cb 
>   src/slave/container_loggers/sandbox.cpp 
> b55e089877f205bab482ae4ebe5a2010aeebeb47 
>   src/slave/containerizer/docker.cpp 7d801fb17565a5298e8e3c5b430e070e12473680 
>   src/slave/containerizer/mesos/containerizer.hpp 
> 10a9b57660388ac2409458a4d07af64cc3b185e2 
>   src/slave/containerizer/mesos/containerizer.cpp 
> d2b4f75a55dbe4746bc2dfc180335fa831a554ef 
>   src/slave/containerizer/mesos/io/switchboard.hpp 
> 5b1fa25d5f577ce3c232fdf5324c7f9c837a64ce 
>   src/slave/containerizer/mesos/io/switchboard.cpp 
> 820d53bc12b1bf6018132660e4b7c5eae8c1e2ee 
>   src/tests/container_logger_tests.cpp 
> 589d6a9df7ce964052355be41597ef11677ca03d 
> 
> Diff: https://reviews.apache.org/r/57176/diff/
> 
> 
> Testing
> ---
> 
> GTEST_FILTER="" make -j check
> src/mesos-tests
> 
> 
> Thanks,
> 
> Kevin Klues
> 
>

Re: Review Request 57121: Renamed 'SubprocessInfo' to 'ContainerIO' in 'ContainerLogger'.

[Jie Yu]

> On Feb. 28, 2017, 10:19 p.m., Jie Yu wrote:
> > include/mesos/slave/container_logger.hpp, line 58
> > 
> >
> > I would pull this to top level, rather than nested inside container 
> > logger as it'll be used by io switchboard as well.
> > 
> > Probably put that in `include/mesos/slave/containerizer.hpp`
> 
> Kevin Klues wrote:
> I think I'd rather do this as a follow-on patch to 
> https://reviews.apache.org/r/56195. Otherwise we will have to compress all of 
> these patches into 1 because the nee 'ContainerIO' class will conflict with 
> the 'ContainerIO' protobuf definition (which we remove in 56195).
> 
> Sound good?

Sounds good!


- Jie


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


On March 1, 2017, 1:41 a.m., Kevin Klues wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/57121/
> ---
> 
> (Updated March 1, 2017, 1:41 a.m.)
> 
> 
> Review request for mesos, Jie Yu and Joseph Wu.
> 
> 
> Bugs: MESOS-7050
> https://issues.apache.org/jira/browse/MESOS-7050
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Renamed 'SubprocessInfo' to 'ContainerIO' in 'ContainerLogger'.
> 
> 
> Diffs
> -
> 
>   include/mesos/slave/container_logger.hpp 
> a3f619b79ca0188df9e231c600dfa396f39ab29a 
>   src/slave/container_loggers/lib_logrotate.hpp 
> e37d99cb268bb3286e312d2ebdbaf84d3fd4bf91 
>   src/slave/container_loggers/lib_logrotate.cpp 
> b257f48f819985e339a5a7fd8066ffa9f39df7a6 
>   src/slave/container_loggers/sandbox.hpp 
> 4ec090cbfc5834ead45ec39c3a646f491fe892cb 
>   src/slave/container_loggers/sandbox.cpp 
> b55e089877f205bab482ae4ebe5a2010aeebeb47 
>   src/slave/containerizer/docker.cpp 7d801fb17565a5298e8e3c5b430e070e12473680 
>   src/slave/containerizer/mesos/io/switchboard.hpp 
> 5b1fa25d5f577ce3c232fdf5324c7f9c837a64ce 
>   src/slave/containerizer/mesos/io/switchboard.cpp 
> 820d53bc12b1bf6018132660e4b7c5eae8c1e2ee 
>   src/tests/container_logger_tests.cpp 
> 589d6a9df7ce964052355be41597ef11677ca03d 
> 
> Diff: https://reviews.apache.org/r/57121/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Kevin Klues
> 
>

Review Request 57177: Clarified naming rules in C++ style guide.

[Greg Mann]

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

Review request for mesos and Joseph Wu.


Repository: mesos


Description
---

This patch clarifies the differences in naming rules between
Mesos application code and the libprocess/stout libraries. It
also updates Wikipedia links to point to more instructive
portions of the relevant entries.


Diffs
-

  docs/c++-style-guide.md 176562d61340492383359a3697a8543806f47f2b 

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


Testing
---


Thanks,

Greg Mann

Review Request 57176: Pulled 'ContainerIO' out of the 'ContainerLogger'.

[Kevin Klues]

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

Review request for mesos and Jie Yu.


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


Repository: mesos


Description
---

Pulled 'ContainerIO' out of the 'ContainerLogger'.


Diffs
-

  include/mesos/slave/container_logger.hpp 
a3f619b79ca0188df9e231c600dfa396f39ab29a 
  include/mesos/slave/containerizer.hpp 
d0096b9e0478478cc8184b1ea6bfecafb1a8620f 
  src/slave/container_loggers/lib_logrotate.hpp 
e37d99cb268bb3286e312d2ebdbaf84d3fd4bf91 
  src/slave/container_loggers/lib_logrotate.cpp 
b257f48f819985e339a5a7fd8066ffa9f39df7a6 
  src/slave/container_loggers/sandbox.hpp 
4ec090cbfc5834ead45ec39c3a646f491fe892cb 
  src/slave/container_loggers/sandbox.cpp 
b55e089877f205bab482ae4ebe5a2010aeebeb47 
  src/slave/containerizer/docker.cpp 7d801fb17565a5298e8e3c5b430e070e12473680 
  src/slave/containerizer/mesos/containerizer.hpp 
10a9b57660388ac2409458a4d07af64cc3b185e2 
  src/slave/containerizer/mesos/containerizer.cpp 
d2b4f75a55dbe4746bc2dfc180335fa831a554ef 
  src/slave/containerizer/mesos/io/switchboard.hpp 
5b1fa25d5f577ce3c232fdf5324c7f9c837a64ce 
  src/slave/containerizer/mesos/io/switchboard.cpp 
820d53bc12b1bf6018132660e4b7c5eae8c1e2ee 
  src/tests/container_logger_tests.cpp 589d6a9df7ce964052355be41597ef11677ca03d 

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


Testing
---

GTEST_FILTER="" make -j check
src/mesos-tests


Thanks,

Kevin Klues

Re: Review Request 56195: Fixed ContainerLogger / IOSwitchboard FD leaks.

[Kevin Klues]

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

(Updated March 1, 2017, 1:43 a.m.)


Review request for mesos, Alexander Rukletsov, Gastón Kleiman, Gilbert Song, 
Jie Yu, Joseph Wu, and Vinod Kone.


Changes
---

Updated with rebase on previous reviews.


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


Repository: mesos


Description
---

Previously, the containizer launch path would leak FDs if the
containerizer launch path failed between successfully calling
prepare() on either the ContainerLogger (in the case of the Docker
containerizer) or the IOSwitchboard (in the case of the mesos
containerizer) and forking the actual container.

These components relied on the Subprocess call inside launcher->fork()
to close these FDS on their behalf. If the containerizer launch path
failed somewhere between calling prepare() and making this fork()
call, these FDs would never be closed.

In the case of the IOSwitchboard, this would lead to deadlock in the
destroy path because the future returned by the IOSwitchboard's
cleanup function would never be satisfied. The IOSwitchboard doesn't
shutdown until the FDs it allocates to the container have been closed.

This commit fixes this problem by updating the
ContainerLogger::ContainerIO::FD abstraction to change the way it
manages FDS. Instead of tagging each FD with the Subprocess::IO::OWNED
label and forcing the launcher->fork() call to deal with closing the
FDs once it's forked a new subprocess, we now do things slightly
differently now.

We now keep the default DUP label on each FD (instead fo changing it
to OWNED) to cause launcher->fork() to dup it before mapping it onto
the stdin/stdout/stderr of the subprocess. It then only closes the
duped FD, leaving the original one open.

In doing so, it's now the containerizer's responsibility to ensure
that these FDs are closed properly (whether that's between a
successful prepare() call and launcher->fork()) or after
launcher->fork() has completed successfully). While this has the
potential to complicate things slightly on the SUCCESS path,
at least it is now the containerizers's responsibility to close these
FDS in *all* cases, rather than splitting that responsibility across
components.

In order to simplify this, we've also modified the
ContainerLogger::ContainerIO::FD abstraction to hold a Shared
pointer to its underlying file descriptor and (optionally) close it on
destruction. With this, we can ensure that all file descriptors
created through this abstraction will be automatically closed onced
their final reference goes out of scope (even if its been copied
around several times).

In essence, this releases the containerizer from the burden of manually
closing these FDS itself. So long as it holds the final reference to
these FDs (which it does), they will be automatically closed along
*any* path out of containerizer->launch(). These are exactly the
semantics we want to achieve.

In the case of the the ContainerLogger, ownership of these FDs (and
thus their final reference) is passed to the containerizer in the
ContainerIO struct returned by prepare(). In the case of the
IOSwitchboard, we had to add a new API call to transfer ownership
(since it is an isolator and prepare() can only return a protobuf),
but the end result is the same.


Diffs (updated)
-

  include/mesos/slave/container_logger.hpp 
a3f619b79ca0188df9e231c600dfa396f39ab29a 
  include/mesos/slave/containerizer.proto 
76fde8af8dfe3acedde8fe5c9facc202c92b8411 
  src/slave/containerizer/mesos/containerizer.hpp 
10a9b57660388ac2409458a4d07af64cc3b185e2 
  src/slave/containerizer/mesos/containerizer.cpp 
d2b4f75a55dbe4746bc2dfc180335fa831a554ef 
  src/slave/containerizer/mesos/io/switchboard.hpp 
5b1fa25d5f577ce3c232fdf5324c7f9c837a64ce 
  src/slave/containerizer/mesos/io/switchboard.cpp 
820d53bc12b1bf6018132660e4b7c5eae8c1e2ee 

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


Testing
---

Linux CentOS 7:
```
GTEST_FILTER="" make -j check
sudo src/mesos-tests
```


Thanks,

Kevin Klues

Re: Review Request 57122: Added STDIN IO to 'ContainerLogger::ContainerIO' class.

[Kevin Klues]

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

(Updated March 1, 2017, 1:42 a.m.)


Review request for mesos, Jie Yu and Joseph Wu.


Changes
---

Updated to reflect @jie's comments from 57121


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


Repository: mesos


Description
---

Added STDIN IO to 'ContainerLogger::ContainerIO' class.


Diffs (updated)
-

  include/mesos/slave/container_logger.hpp 
a3f619b79ca0188df9e231c600dfa396f39ab29a 

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


Testing
---


Thanks,

Kevin Klues

Re: Review Request 57121: Renamed 'SubprocessInfo' to 'ContainerIO' in 'ContainerLogger'.

[Kevin Klues]

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

(Updated March 1, 2017, 1:41 a.m.)


Review request for mesos, Jie Yu and Joseph Wu.


Changes
---

Updated based on @jie's comments.


Summary (updated)
-

Renamed 'SubprocessInfo' to 'ContainerIO' in 'ContainerLogger'.


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


Repository: mesos


Description (updated)
---

Renamed 'SubprocessInfo' to 'ContainerIO' in 'ContainerLogger'.


Diffs (updated)
-

  include/mesos/slave/container_logger.hpp 
a3f619b79ca0188df9e231c600dfa396f39ab29a 
  src/slave/container_loggers/lib_logrotate.hpp 
e37d99cb268bb3286e312d2ebdbaf84d3fd4bf91 
  src/slave/container_loggers/lib_logrotate.cpp 
b257f48f819985e339a5a7fd8066ffa9f39df7a6 
  src/slave/container_loggers/sandbox.hpp 
4ec090cbfc5834ead45ec39c3a646f491fe892cb 
  src/slave/container_loggers/sandbox.cpp 
b55e089877f205bab482ae4ebe5a2010aeebeb47 
  src/slave/containerizer/docker.cpp 7d801fb17565a5298e8e3c5b430e070e12473680 
  src/slave/containerizer/mesos/io/switchboard.hpp 
5b1fa25d5f577ce3c232fdf5324c7f9c837a64ce 
  src/slave/containerizer/mesos/io/switchboard.cpp 
820d53bc12b1bf6018132660e4b7c5eae8c1e2ee 
  src/tests/container_logger_tests.cpp 589d6a9df7ce964052355be41597ef11677ca03d 

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


Testing
---


Thanks,

Kevin Klues

Re: Review Request 57121: Renamed 'ContainerLogger::SubprocessInfo' to 'ContainerIO'.

[Kevin Klues]

> On Feb. 28, 2017, 10:19 p.m., Jie Yu wrote:
> > include/mesos/slave/container_logger.hpp, line 58
> > 
> >
> > I would pull this to top level, rather than nested inside container 
> > logger as it'll be used by io switchboard as well.
> > 
> > Probably put that in `include/mesos/slave/containerizer.hpp`

I think I'd rather do this as a follow-on patch to 
https://reviews.apache.org/r/56195. Otherwise we will have to compress all of 
these patches into 1 because the nee 'ContainerIO' class will conflict with the 
'ContainerIO' protobuf definition (which we remove in 56195).

Sound good?


- Kevin


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


On Feb. 28, 2017, 5:09 a.m., Kevin Klues wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/57121/
> ---
> 
> (Updated Feb. 28, 2017, 5:09 a.m.)
> 
> 
> Review request for mesos, Jie Yu and Joseph Wu.
> 
> 
> Bugs: MESOS-7050
> https://issues.apache.org/jira/browse/MESOS-7050
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Renamed 'ContainerLogger::SubprocessInfo' to 'ContainerIO'.
> 
> 
> Diffs
> -
> 
>   include/mesos/slave/container_logger.hpp 
> a3f619b79ca0188df9e231c600dfa396f39ab29a 
>   src/slave/container_loggers/lib_logrotate.hpp 
> e37d99cb268bb3286e312d2ebdbaf84d3fd4bf91 
>   src/slave/container_loggers/lib_logrotate.cpp 
> b257f48f819985e339a5a7fd8066ffa9f39df7a6 
>   src/slave/container_loggers/sandbox.hpp 
> 4ec090cbfc5834ead45ec39c3a646f491fe892cb 
>   src/slave/container_loggers/sandbox.cpp 
> b55e089877f205bab482ae4ebe5a2010aeebeb47 
>   src/slave/containerizer/docker.cpp 7d801fb17565a5298e8e3c5b430e070e12473680 
>   src/slave/containerizer/mesos/io/switchboard.hpp 
> 5b1fa25d5f577ce3c232fdf5324c7f9c837a64ce 
>   src/slave/containerizer/mesos/io/switchboard.cpp 
> 820d53bc12b1bf6018132660e4b7c5eae8c1e2ee 
>   src/tests/container_logger_tests.cpp 
> 589d6a9df7ce964052355be41597ef11677ca03d 
> 
> Diff: https://reviews.apache.org/r/57121/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Kevin Klues
> 
>

Re: Review Request 57052: CMake: Add `build_config.hpp.in` for `BUILD_TIME` variables.

[Andrew Schwartzmeyer]

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

(Updated March 1, 2017, 12:42 a.m.)


Review request for mesos, Alex Clemmer, Joseph Wu, and Michael Park.


Changes
---

Emit build_config.hpp into build directory, instead of source directory. Rename 
config to build_config.


Summary (updated)
-

CMake: Add `build_config.hpp.in` for `BUILD_TIME` variables.


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


Repository: mesos


Description (updated)
---

Commit c7fc1377b introduced a bug that prevented any incremental
recompilation. By defining the `BUILD_TIME` in `MESOS_CPPFLAGS`,
every build was seen as having a root dependency (the flags)
changed, causing a rebuild of every single source in Mesos.

This patch introduces a CMake `configure_file` directive which
takes in `build_config.hpp.in` and emits `build_config.hpp` with the
`BUILD_TIME`, `BUILD_DATE`, and `BUILD_USER` variables defined.
It also sets `HAVE_BUILD_CONFIG_H` which `build.cpp` uses to `#include`
the configuration file. The result is that the date, time, and user
are set at the point of configuration (invocation of CMake) instead
of at build, thus allowing for incremental rebuilds.


Diffs (updated)
-

  cmake/CompilationConfigure.cmake 7808d1a0086a791af8dcb2dd796b9424c7d5 
  src/common/build.cpp 090b59fb22bfc00516d65f501f8f18cd85a5b4cd 
  src/common/build_config.hpp.in PRE-CREATION 

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


Testing
---

cmake; make && make check on Linux
cmake; VS build stout-tests and mesos-tests, then repeat and see no compilation 
take place a second time. Also support\windows-build.bat twice.


Thanks,

Andrew Schwartzmeyer

Re: Review Request 57158: Added default parameter value to master validation function.

[Mesos Reviewbot]

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



Patch looks great!

Reviews applied: [56623, 56617, 56618, 56901, 57054, 57153, 56619, 56812, 
56813, 56624, 56621, 57158]

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

- Mesos Reviewbot


On Feb. 28, 2017, 7:28 p.m., Greg Mann wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/57158/
> ---
> 
> (Updated Feb. 28, 2017, 7:28 p.m.)
> 
> 
> Review request for mesos, Alexander Rojas and Vinod Kone.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> The master's validation function for RESERVE operations previously
> did not set a default parameter value for its final optional
> parameter, requiring callsites to explicitly specify `None()`. This
> patch adds the default value.
> 
> 
> Diffs
> -
> 
>   src/master/http.cpp 6e5178eeea6cc6b90ae253840da22be13444b088 
>   src/master/validation.hpp f03b3280704083be1ca074ca07c69edbb49dae19 
> 
> Diff: https://reviews.apache.org/r/57158/diff/
> 
> 
> Testing
> ---
> 
> `make check`
> 
> 
> Thanks,
> 
> Greg Mann
> 
>

Re: Review Request 57161: Cleaned up header includes.

[Neil Conway]

> On March 1, 2017, 12:04 a.m., Michael Park wrote:
> > src/linux/capabilities.cpp, line 23
> > 
> >
> > I don't think we need this?

We use `std::set` in that `.cpp` file. (We previously had `using std::set;` as 
well, but I removed it, because it is confusing with the `set` member functions 
implemented by that file.)


- Neil


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


On Feb. 28, 2017, 8:07 p.m., Neil Conway wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/57161/
> ---
> 
> (Updated Feb. 28, 2017, 8:07 p.m.)
> 
> 
> Review request for mesos and Michael Park.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Cleaned up header includes.
> 
> 
> Diffs
> -
> 
>   src/linux/capabilities.hpp 7c6342745a7db1ecf8664c64be77b10a54d5037a 
>   src/linux/capabilities.cpp 9f46bd0a34d35d8295b1899a7ba67d85c3c24348 
> 
> Diff: https://reviews.apache.org/r/57161/diff/
> 
> 
> Testing
> ---
> 
> `make check`
> 
> 
> Thanks,
> 
> Neil Conway
> 
>

Re: Review Request 57161: Cleaned up header includes.

[Michael Park]

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


Ship it!





src/linux/capabilities.cpp (line 23)


I don't think we need this?


- Michael Park


On Feb. 28, 2017, 12:07 p.m., Neil Conway wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/57161/
> ---
> 
> (Updated Feb. 28, 2017, 12:07 p.m.)
> 
> 
> Review request for mesos and Michael Park.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Cleaned up header includes.
> 
> 
> Diffs
> -
> 
>   src/linux/capabilities.hpp 7c6342745a7db1ecf8664c64be77b10a54d5037a 
>   src/linux/capabilities.cpp 9f46bd0a34d35d8295b1899a7ba67d85c3c24348 
> 
> Diff: https://reviews.apache.org/r/57161/diff/
> 
> 
> Testing
> ---
> 
> `make check`
> 
> 
> Thanks,
> 
> Neil Conway
> 
>

Re: Review Request 57012: Added unit tests for SHARE and UNSHARE offer operations.

[Anindya Sinha]

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

(Updated Feb. 28, 2017, 11:58 p.m.)


Review request for mesos and Jiang Yan Xu.


Changes
---

Rebased.


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


Repository: mesos


Description
---

Added unit tests for SHARE and UNSHARE offer operations.


Diffs (updated)
-

  src/tests/hierarchical_allocator_tests.cpp 
cdf1f15b7802439b28405ca8f6634ce83e886630 
  src/tests/master_validation_tests.cpp 
5a84c8d03cacd150ac61213a7c2d8613fd1780e7 
  src/tests/mesos.hpp b450a04dfbf3bbeeb6b605fb78097dca390cbdbe 
  src/tests/persistent_volume_tests.cpp 
7ac82862363af7a33a52b8570149cf2237b3519b 

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


Testing
---

All tests passed.


Thanks,

Anindya Sinha

Re: Review Request 57011: Add support for SHARE and UNSHARE operations.

[Anindya Sinha]

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

(Updated Feb. 28, 2017, 11:58 p.m.)


Review request for mesos and Jiang Yan Xu.


Changes
---

Rebased.


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


Repository: mesos


Description
---

SHARE and UNSHARE is allowed for persistent volumes currently.
Only a non-shared resource can be SHAREd, and a shared resource can be
UNSHAREd. UNSHARE is processed only if the resources are not in use
by a running task, or has not been requested by a pending task. If
the UNSHARE is processed, the pending offers containing the resources
to be unshared are rescinded from all frameworks to which this shared
resource has been offered.


Diffs (updated)
-

  include/mesos/resources.hpp 73e44be8aa03cfd167ff47bbff5d708fb545c01b 
  include/mesos/v1/resources.hpp 19132298cf9f51a60f700ed5ac8c3fbe93e8c40f 
  src/common/protobuf_utils.cpp 944263bbaa87a65005fd924bccfadb7293312fa0 
  src/common/resources.cpp 388e3ef3eabea0dd8d1300e56d493b92b70c75e3 
  src/master/master.hpp 764adb18ddf09b62529c5c96f8e4dfaf7803483e 
  src/master/master.cpp ae36bf477851bf2fe11eb7913c580e3e0b9cbbe5 
  src/master/validation.hpp 50a3e6ec6a6d44c9ad449edfbd5d2cf8540b3a73 
  src/master/validation.cpp 41ef0d072ca53d9695c16b8dd98c319186f11f75 
  src/slave/slave.cpp fc480ae23ffa5cdeeb79b3621a08e1f8703bc01a 
  src/v1/resources.cpp e47c4d49e69b915d37e40a91fef69f75dff52463 

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


Testing
---

All tests passed.


Thanks,

Anindya Sinha

Re: Review Request 57010: Added protobuf changes for `SHARE` and `UNSHARE` offer operations.

[Anindya Sinha]

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

(Updated Feb. 28, 2017, 11:58 p.m.)


Review request for mesos and Jiang Yan Xu.


Changes
---

Rebased.


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


Repository: mesos


Description
---

Added protobuf changes for `SHARE` and `UNSHARE` offer operations.


Diffs (updated)
-

  include/mesos/mesos.proto 030e79c003f6560e9c0627db12fb1baba411151d 
  include/mesos/v1/mesos.proto 7f6f858a7d9387d202510730d400e490298e6574 
  src/common/protobuf_utils.cpp 944263bbaa87a65005fd924bccfadb7293312fa0 
  src/common/resources.cpp 388e3ef3eabea0dd8d1300e56d493b92b70c75e3 
  src/master/master.cpp ae36bf477851bf2fe11eb7913c580e3e0b9cbbe5 
  src/v1/resources.cpp e47c4d49e69b915d37e40a91fef69f75dff52463 

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


Testing
---

All tests passed.


Thanks,

Anindya Sinha

Re: Review Request 56666: Added a HMAC SHA256 generator.

[Greg Mann]

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




3rdparty/libprocess/include/process/ssl/utilities.hpp (lines 114 - 122)


In this comment, it might be good to introduce HMAC by saying "Generates a 
keyed-hash message authentication code (HMAC) with SHA256."

Then, you can use "HMAC" in the `@return` comment below.



3rdparty/libprocess/include/process/ssl/utilities.hpp (lines 124 - 125)


Could we use `key` instead of `secret` for the second parameter name here? 
Seems more consistent with the terminology in this file, as well as the OpenSSL 
library.

I also have a slight preference for `message` instead of `input`, but up to 
you.


- Greg Mann


On Feb. 27, 2017, 11:37 a.m., Jan Schlicht wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/5/
> ---
> 
> (Updated Feb. 27, 2017, 11:37 a.m.)
> 
> 
> Review request for mesos, Alexander Rojas and Greg Mann.
> 
> 
> Bugs: MESOS-7001
> https://issues.apache.org/jira/browse/MESOS-7001
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> HMAC SHA256 can be used to create or verify message signatures.
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/include/process/ssl/utilities.hpp 
> c2f64a91a505675d568ddf5aa081125e4e32fe17 
>   3rdparty/libprocess/src/ssl/utilities.cpp 
> 8aec613312eee3dd11d9df8c3828a5185407e073 
> 
> Diff: https://reviews.apache.org/r/5/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Jan Schlicht
> 
>

Re: Review Request 53666: Refactor cgroups cleanup to allow cleanup to continue on an error.

[Anindya Sinha]

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

(Updated Feb. 28, 2017, 11:16 p.m.)


Review request for mesos, Jie Yu and Jiang Yan Xu.


Changes
---

Rebased.


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


Repository: mesos


Description
---

Added a `continueOnError` attribute to cgroups `Destroyer` to allow
cgroups cleanup to continue incase of any error. The linux launcher
continues cleanup of cgroups even if there is an error. However, if
the top level cgroup is actually removed, it treats it as a success.


Diffs (updated)
-

  src/linux/cgroups.hpp cfce09cb57501f2c988a8d997d7c6150280ed53d 
  src/linux/cgroups.cpp 3a853297eaccc86e55888da21f292f5e37c88733 
  src/slave/containerizer/mesos/linux_launcher.cpp 
a3409dc64435e002072b9b458a5fda0564214f28 

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


Testing
---

All tests passed.


Thanks,

Anindya Sinha

Re: Review Request 51257: Add external process container logger.

[Joseph Wu]

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



* You may also want to add this to the CMake build.
* There are a couple of extraneous spaces in this patch (like `{ task}` in the 
test).  Not really a big deal.


src/slave/container_loggers/lib_externallogger.hpp (lines 58 - 66)


Nit: Ordering of validation here and some wording suggestions

```
if (isExecutable.isError()) {
  return Error(
  "Failed to stat file '" + executablePath + 
  "': " + isExecutable.error());
}

if (!isExecutable.get()) {
  return Error("File is not an executable: " + executablePath);
}
```



src/slave/container_loggers/lib_externallogger.hpp (line 79)


Suggestion:

Name of the environment variable used to store the stream identifier 
("stdout" or "stderr").



src/slave/container_loggers/lib_externallogger.hpp (line 87)


You could remove the leading `MESOS_` in the default.



src/slave/container_loggers/lib_externallogger.cpp (lines 99 - 100)


Nit: Inconsistent use of `.insert` vs `operator[]` in this file.

I would prefer the operator.


- Joseph Wu


On Feb. 13, 2017, 2:41 p.m., Will Rouesnel wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/51257/
> ---
> 
> (Updated Feb. 13, 2017, 2:41 p.m.)
> 
> 
> Review request for mesos and Joseph Wu.
> 
> 
> Bugs: MESOS-6003
> https://issues.apache.org/jira/browse/MESOS-6003
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Adds the external process container logger. This functions like the
> logrotate container logger, but instead calls a custom host binary
> (or script) and passes the executorInfo as JSON via environment
> variables. This makes it very easy for users to configure custom
> logging solutions, without needing to write and maintain logging
> modules.
> 
> Tests passing and complete.
> 
> 
> Diffs
> -
> 
>   src/Makefile.am c21a073bdbefbb4547e45ed13b7a8a563854fd82 
>   src/slave/container_loggers/lib_externallogger.hpp PRE-CREATION 
>   src/slave/container_loggers/lib_externallogger.cpp PRE-CREATION 
>   src/tests/container_logger_external.sh PRE-CREATION 
>   src/tests/container_logger_tests.cpp 
> 589d6a9df7ce964052355be41597ef11677ca03d 
>   src/tests/module.hpp e661d95fa44fc1aedfe83c564c826d5b7d32c85b 
>   src/tests/module.cpp 5b83fd6358ddea4c9d849b8992e1a6040ef74505 
> 
> Diff: https://reviews.apache.org/r/51257/diff/
> 
> 
> Testing
> ---
> 
> Adds ContainerLoggerTest.EXTERNAL_RecieveEnvironment which tests all major 
> parameters of the change.
> 
> A synthetic external container logger is provided by the script 
> tests/container_logger_external.sh which is setup to fail if any important 
> output is unavailable to the logging process. 
> 
> The other basic checks are duplicated from the Logrotate container logger, 
> from where this change inherits a lot of its code.
> 
> 
> Thanks,
> 
> Will Rouesnel
> 
>

Re: Review Request 57109: Re-checkpointed the `Executor`s and `Task`s during agent recovery.

[Benjamin Mahler]

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




src/slave/slave.cpp (lines 5311 - 5315)


We could clarify here that in order to support of a scheduler upgrading to 
MULTI_ROLE and then changing its roles we need to do .



src/slave/slave.cpp (lines 6948 - 6950)


It would be nice to avoid checkpointing every time we recover the agent, 
since we only need to re-checkpoint if any allocation info injection took place.

I was also going to suggest a comment here but I think once updated to 
conditional checkpointing it will be a bit more clear that we're doing this in 
support of the multi-role upgrade case (if not we probably want to clarify 
this).


- Benjamin Mahler


On Feb. 27, 2017, 10:18 p.m., Michael Park wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/57109/
> ---
> 
> (Updated Feb. 27, 2017, 10:18 p.m.)
> 
> 
> Review request for mesos and Benjamin Mahler.
> 
> 
> Bugs: MESOS-7061
> https://issues.apache.org/jira/browse/MESOS-7061
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Re-checkpointed the tasks and executors during agent recovery by calling
> `checkpointX` to `recoverX` functions for tasks and executors.
> 
> 
> Diffs
> -
> 
>   src/slave/slave.hpp 3b0aea4e3e9a17501077beccbccaab4abbe11af2 
>   src/slave/slave.cpp fc480ae23ffa5cdeeb79b3621a08e1f8703bc01a 
> 
> Diff: https://reviews.apache.org/r/57109/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Michael Park
> 
>

Re: Review Request 57160: Added 'override' to function declarations in 'LibeventSSLSocketImpl'.

[Greg Mann]

> On Feb. 28, 2017, 10:31 p.m., Joseph Wu wrote:
> > 3rdparty/libprocess/src/libevent_ssl_socket.hpp, lines 47-48
> > 
> >
> > With the removal of `virtual`, this fits on one line again :)

doh! thx. I was about to update, but I see you've already submitted :)


- Greg


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


On Feb. 28, 2017, 8:51 p.m., Greg Mann wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/57160/
> ---
> 
> (Updated Feb. 28, 2017, 8:51 p.m.)
> 
> 
> Review request for mesos, Benjamin Bannier and Joseph Wu.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Inconsistent use of the `override` keyword in
> `LibeventSSLSocketImpl` was causing warnings during
> clang builds. This patch makes use of the keyword
> across all relevant declarations in the class.
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/src/libevent_ssl_socket.hpp 
> e589a04d14378f265a8fca871c9f5b0c577f5713 
> 
> Diff: https://reviews.apache.org/r/57160/diff/
> 
> 
> Testing
> ---
> 
> `make check`
> 
> 
> Thanks,
> 
> Greg Mann
> 
>

Re: Review Request 57132: Updated WebUI to display roles.

[Mesos Reviewbot]

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



Patch looks great!

Reviews applied: [57013, 57132]

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

- Mesos Reviewbot


On Feb. 28, 2017, 5:25 p.m., Jay Guo wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/57132/
> ---
> 
> (Updated Feb. 28, 2017, 5:25 p.m.)
> 
> 
> Review request for mesos and Benjamin Mahler.
> 
> 
> Bugs: MESOS-6657
> https://issues.apache.org/jira/browse/MESOS-6657
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> With support of multi-tenancy in Mesos, WebUI needs to be updated
> to correctly display role information.
> 
> 
> Diffs
> -
> 
>   src/webui/master/static/agent.html 7a4599367ea8ff802591adfd86d4058cdc51f297 
>   src/webui/master/static/agent_executor.html 
> 95e921d1c3ca0f005e3eb3f79eb0d9a70a3b3f91 
>   src/webui/master/static/agent_framework.html 
> 806ab06063dcf62cd6d1d7913840ed83141bd1eb 
>   src/webui/master/static/framework.html 
> 37e0b31ac90e877b70f315143b7411fadec6 
>   src/webui/master/static/frameworks.html 
> 0c6fc1c37e0ab49fed64aaa861dfcbaab7f38e0e 
>   src/webui/master/static/js/controllers.js 
> 2ea8275cb5fffa2344474ebfcf9277fe92165a92 
>   src/webui/master/static/offers.html 
> 181fc2deabef3faec36785e7fcdbc37465668bec 
> 
> Diff: https://reviews.apache.org/r/57132/diff/
> 
> 
> Testing
> ---
> 
> manually test by launching new master with both new/old agent, and inspect 
> WebUI to see roles.
> 
> 
> Thanks,
> 
> Jay Guo
> 
>

Re: Review Request 57160: Added 'override' to function declarations in 'LibeventSSLSocketImpl'.

[Joseph Wu]

> On Feb. 28, 2017, 12:27 p.m., Benjamin Bannier wrote:
> > This lgtm. I believe that even applying `override` incompletely can prevent 
> > bad mistakes, but am unsure if we are willing to tolerate inconsistencies 
> > in the Mesos code base at all. What's your stance on this Joseph?

Since the plan is to eventually have `override` on all overriden functions, I 
find it better to get there incrementally than not-at-all.


- Joseph


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


On Feb. 28, 2017, 12:51 p.m., Greg Mann wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/57160/
> ---
> 
> (Updated Feb. 28, 2017, 12:51 p.m.)
> 
> 
> Review request for mesos, Benjamin Bannier and Joseph Wu.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Inconsistent use of the `override` keyword in
> `LibeventSSLSocketImpl` was causing warnings during
> clang builds. This patch makes use of the keyword
> across all relevant declarations in the class.
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/src/libevent_ssl_socket.hpp 
> e589a04d14378f265a8fca871c9f5b0c577f5713 
> 
> Diff: https://reviews.apache.org/r/57160/diff/
> 
> 
> Testing
> ---
> 
> `make check`
> 
> 
> Thanks,
> 
> Greg Mann
> 
>

Re: Review Request 57160: Added 'override' to function declarations in 'LibeventSSLSocketImpl'.

[Joseph Wu]

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


Fix it, then Ship it!





3rdparty/libprocess/src/libevent_ssl_socket.hpp (lines 47 - 48)


With the removal of `virtual`, this fits on one line again :)



3rdparty/libprocess/src/libevent_ssl_socket.hpp (lines 51 - 54)


Same whitespace with this line.


- Joseph Wu


On Feb. 28, 2017, 12:51 p.m., Greg Mann wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/57160/
> ---
> 
> (Updated Feb. 28, 2017, 12:51 p.m.)
> 
> 
> Review request for mesos, Benjamin Bannier and Joseph Wu.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Inconsistent use of the `override` keyword in
> `LibeventSSLSocketImpl` was causing warnings during
> clang builds. This patch makes use of the keyword
> across all relevant declarations in the class.
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/src/libevent_ssl_socket.hpp 
> e589a04d14378f265a8fca871c9f5b0c577f5713 
> 
> Diff: https://reviews.apache.org/r/57160/diff/
> 
> 
> Testing
> ---
> 
> `make check`
> 
> 
> Thanks,
> 
> Greg Mann
> 
>

Re: Review Request 55657: Added cpack to create source package.

[Joseph Wu]

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




CMakeLists.txt (lines 73 - 81)


The full list should ignore some of the bootstrap artifacts, docs, support 
files, and autotools files, as well as the build directory:
```
# List of sources we will ignore when generating a source package.
# We assume there are no artifacts from an autotools build.
#
# NOTE: An in-source build is not supported. The build directory
# must be different than the source directory.
set(CPACK_SOURCE_IGNORE_FILES
  .clang-format;
  .gitignore;
  .reviewboardrc;
  bootstrap.bat;
  bootstrap;
  configure.ac;
  Doxyfile;
  mesos.pc.in;
  /.git/;
  /docs/;
  /m4/;
  /mpi/;
  /site/;
  /support/;
  ${CMAKE_BINARY_DIR};
  )

# Convert the ignore list to a string (as required by CPack).
set(CPACK_SOURCE_IGNORE_FILES "${CPACK_SOURCE_IGNORE_FILES}")
```



CMakeLists.txt (line 83)


Remove this line, as the vendor should define this when invoking CMake.



CMakeLists.txt (line 84)


To affect the binary packages, `CPACK_PACKAGE_FILE_NAME` should be set too.



CMakeLists.txt (line 87)


Instead of specifying the package format, we can leave the format of the 
package up to the end-user.

For binary packages, they can define:

CPACK_BINARY_(BUNDLE|DEB|DRAGNDROP|IFW|NSIS|OSXX11|PACKAGEMAKER|RPM|STGZ|TBZ2|TGZ|TXZ)

For source packages:
CPACK_SOURCE_(TGZ|TBZ2|TXZ|TZ|ZIP)

For convenience, I'll enable CPACK_SOURCE_TGZ and leave all the other ones 
disabled by default.



CMakeLists.txt (line 89)


At the moment, there's no need to mirror the autotools target name.  So 
this line can be removed.


- Joseph Wu


On Jan. 18, 2017, 2:25 p.m., Srinivas Brahmaroutu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/55657/
> ---
> 
> (Updated Jan. 18, 2017, 2:25 p.m.)
> 
> 
> Review request for mesos, Alex Clemmer and Joseph Wu.
> 
> 
> Bugs: MESOS-4245
> https://issues.apache.org/jira/browse/MESOS-4245
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added cpack to create source package.
> 
> 
> Diffs
> -
> 
>   CMakeLists.txt c6b1eb6a7c7e6ed3650847acbec0ddd3101bb96f 
> 
> Diff: https://reviews.apache.org/r/55657/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Srinivas Brahmaroutu
> 
>

Re: Review Request 56195: Fixed ContainerLogger / IOSwitchboard FD leaks.

[Jie Yu]

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


Fix it, then Ship it!





src/slave/containerizer/mesos/io/switchboard.hpp (line 131)


I would suggest adding a new line above.


- Jie Yu


On Feb. 28, 2017, 5:09 a.m., Kevin Klues wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/56195/
> ---
> 
> (Updated Feb. 28, 2017, 5:09 a.m.)
> 
> 
> Review request for mesos, Alexander Rukletsov, Gastón Kleiman, Gilbert Song, 
> Jie Yu, Joseph Wu, and Vinod Kone.
> 
> 
> Bugs: MESOS-7050
> https://issues.apache.org/jira/browse/MESOS-7050
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Previously, the containizer launch path would leak FDs if the
> containerizer launch path failed between successfully calling
> prepare() on either the ContainerLogger (in the case of the Docker
> containerizer) or the IOSwitchboard (in the case of the mesos
> containerizer) and forking the actual container.
> 
> These components relied on the Subprocess call inside launcher->fork()
> to close these FDS on their behalf. If the containerizer launch path
> failed somewhere between calling prepare() and making this fork()
> call, these FDs would never be closed.
> 
> In the case of the IOSwitchboard, this would lead to deadlock in the
> destroy path because the future returned by the IOSwitchboard's
> cleanup function would never be satisfied. The IOSwitchboard doesn't
> shutdown until the FDs it allocates to the container have been closed.
> 
> This commit fixes this problem by updating the
> ContainerLogger::ContainerIO::FD abstraction to change the way it
> manages FDS. Instead of tagging each FD with the Subprocess::IO::OWNED
> label and forcing the launcher->fork() call to deal with closing the
> FDs once it's forked a new subprocess, we now do things slightly
> differently now.
> 
> We now keep the default DUP label on each FD (instead fo changing it
> to OWNED) to cause launcher->fork() to dup it before mapping it onto
> the stdin/stdout/stderr of the subprocess. It then only closes the
> duped FD, leaving the original one open.
> 
> In doing so, it's now the containerizer's responsibility to ensure
> that these FDs are closed properly (whether that's between a
> successful prepare() call and launcher->fork()) or after
> launcher->fork() has completed successfully). While this has the
> potential to complicate things slightly on the SUCCESS path,
> at least it is now the containerizers's responsibility to close these
> FDS in *all* cases, rather than splitting that responsibility across
> components.
> 
> In order to simplify this, we've also modified the
> ContainerLogger::ContainerIO::FD abstraction to hold a Shared
> pointer to its underlying file descriptor and (optionally) close it on
> destruction. With this, we can ensure that all file descriptors
> created through this abstraction will be automatically closed onced
> their final reference goes out of scope (even if its been copied
> around several times).
> 
> In essence, this releases the containerizer from the burden of manually
> closing these FDS itself. So long as it holds the final reference to
> these FDs (which it does), they will be automatically closed along
> *any* path out of containerizer->launch(). These are exactly the
> semantics we want to achieve.
> 
> In the case of the the ContainerLogger, ownership of these FDs (and
> thus their final reference) is passed to the containerizer in the
> ContainerIO struct returned by prepare(). In the case of the
> IOSwitchboard, we had to add a new API call to transfer ownership
> (since it is an isolator and prepare() can only return a protobuf),
> but the end result is the same.
> 
> 
> Diffs
> -
> 
>   include/mesos/slave/container_logger.hpp 
> a3f619b79ca0188df9e231c600dfa396f39ab29a 
>   include/mesos/slave/containerizer.proto 
> 76fde8af8dfe3acedde8fe5c9facc202c92b8411 
>   src/slave/containerizer/mesos/containerizer.hpp 
> 10a9b57660388ac2409458a4d07af64cc3b185e2 
>   src/slave/containerizer/mesos/containerizer.cpp 
> d2b4f75a55dbe4746bc2dfc180335fa831a554ef 
>   src/slave/containerizer/mesos/io/switchboard.hpp 
> 5b1fa25d5f577ce3c232fdf5324c7f9c837a64ce 
>   src/slave/containerizer/mesos/io/switchboard.cpp 
> 820d53bc12b1bf6018132660e4b7c5eae8c1e2ee 
> 
> Diff: https://reviews.apache.org/r/56195/diff/
> 
> 
> Testing
> ---
> 
> Linux CentOS 7:
> ```
> GTEST_FILTER="" make -j check
> sudo src/mesos-tests
> ```
> 
> 
> Thanks,
> 
> Kevin Klues
> 
>

Re: Review Request 57122: Added STDIN IO to 'ContainerLogger::ContainerIO' class.

[Jie Yu]

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


Ship it!




Ship It!

- Jie Yu


On Feb. 28, 2017, 5:09 a.m., Kevin Klues wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/57122/
> ---
> 
> (Updated Feb. 28, 2017, 5:09 a.m.)
> 
> 
> Review request for mesos, Jie Yu and Joseph Wu.
> 
> 
> Bugs: MESOS-7050
> https://issues.apache.org/jira/browse/MESOS-7050
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added STDIN IO to 'ContainerLogger::ContainerIO' class.
> 
> 
> Diffs
> -
> 
>   include/mesos/slave/container_logger.hpp 
> a3f619b79ca0188df9e231c600dfa396f39ab29a 
> 
> Diff: https://reviews.apache.org/r/57122/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Kevin Klues
> 
>

Re: Review Request 57121: Renamed 'ContainerLogger::SubprocessInfo' to 'ContainerIO'.

[Jie Yu]

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




include/mesos/slave/container_logger.hpp (line 58)


I would pull this to top level, rather than nested inside container logger 
as it'll be used by io switchboard as well.

Probably put that in `include/mesos/slave/containerizer.hpp`



include/mesos/slave/container_logger.hpp (line 130)


No need for `ContainerIO` here?



include/mesos/slave/container_logger.hpp (line 135)


Ditto.



include/mesos/slave/container_logger.hpp (line 166)


the container


- Jie Yu


On Feb. 28, 2017, 5:09 a.m., Kevin Klues wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/57121/
> ---
> 
> (Updated Feb. 28, 2017, 5:09 a.m.)
> 
> 
> Review request for mesos, Jie Yu and Joseph Wu.
> 
> 
> Bugs: MESOS-7050
> https://issues.apache.org/jira/browse/MESOS-7050
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Renamed 'ContainerLogger::SubprocessInfo' to 'ContainerIO'.
> 
> 
> Diffs
> -
> 
>   include/mesos/slave/container_logger.hpp 
> a3f619b79ca0188df9e231c600dfa396f39ab29a 
>   src/slave/container_loggers/lib_logrotate.hpp 
> e37d99cb268bb3286e312d2ebdbaf84d3fd4bf91 
>   src/slave/container_loggers/lib_logrotate.cpp 
> b257f48f819985e339a5a7fd8066ffa9f39df7a6 
>   src/slave/container_loggers/sandbox.hpp 
> 4ec090cbfc5834ead45ec39c3a646f491fe892cb 
>   src/slave/container_loggers/sandbox.cpp 
> b55e089877f205bab482ae4ebe5a2010aeebeb47 
>   src/slave/containerizer/docker.cpp 7d801fb17565a5298e8e3c5b430e070e12473680 
>   src/slave/containerizer/mesos/io/switchboard.hpp 
> 5b1fa25d5f577ce3c232fdf5324c7f9c837a64ce 
>   src/slave/containerizer/mesos/io/switchboard.cpp 
> 820d53bc12b1bf6018132660e4b7c5eae8c1e2ee 
>   src/tests/container_logger_tests.cpp 
> 589d6a9df7ce964052355be41597ef11677ca03d 
> 
> Diff: https://reviews.apache.org/r/57121/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Kevin Klues
> 
>

Re: Review Request 56665: Added a URL-safe base64 implementation.

[Greg Mann]

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


Fix it, then Ship it!





3rdparty/stout/include/stout/base64.hpp (line 32)


I think we prefer `constexpr char` for constants like this: 
http://mesos.apache.org/documentation/latest/c++-style-guide/

As long as you're touching this, could you change to `constexpr`?



3rdparty/stout/include/stout/base64.hpp (line 37)


ditto



3rdparty/stout/tests/base64_tests.cpp (lines 48 - 69)


Could we use strings for these tests that include both of the modified 
alphabet characters, '-' and '_'?


- Greg Mann


On Feb. 27, 2017, 11:37 a.m., Jan Schlicht wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/56665/
> ---
> 
> (Updated Feb. 27, 2017, 11:37 a.m.)
> 
> 
> Review request for mesos, Alexander Rojas and Greg Mann.
> 
> 
> Bugs: MESOS-7001
> https://issues.apache.org/jira/browse/MESOS-7001
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Base64 has many variants that use different alphabets for encoding.
> "Base 64 Encoding with URL and Filename Safe Alphabet" is a variant
> described in RFC 4648.
> 
> 
> Diffs
> -
> 
>   3rdparty/stout/include/stout/base64.hpp 
> 2ac04c4602bc919633a2a480dd2168b7aa301bd7 
>   3rdparty/stout/tests/base64_tests.cpp 
> 32e516861d44c7e3a36e1a29b4d1fe5960684e3b 
> 
> Diff: https://reviews.apache.org/r/56665/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Jan Schlicht
> 
>

Re: Review Request 57052: CMake: Add `config.hpp.in` for `BUILD_TIME` variables.

[Andrew Schwartzmeyer]

> On Feb. 28, 2017, 6:05 p.m., Andrew Schwartzmeyer wrote:
> > support/gitignore, line 36
> > 
> >
> > Ah, note to myself: update the output to go only into `build` instead 
> > of into `${CMAKE_SOURCE_DIR}`.
> 
> Joseph Wu wrote:
> `${CMAKE_BINARY_DIR}` is the magic variable you'll want to use.

Yes indeed it is; sorry, a bit busy with other stuff today.


- Andrew


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


On Feb. 25, 2017, 12:31 a.m., Andrew Schwartzmeyer wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/57052/
> ---
> 
> (Updated Feb. 25, 2017, 12:31 a.m.)
> 
> 
> Review request for mesos, Alex Clemmer, Joseph Wu, and Michael Park.
> 
> 
> Bugs: MESOS-7172
> https://issues.apache.org/jira/browse/MESOS-7172
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Commit c7fc1377b introduced a bug that prevented any incremental
> recompilation. By defining the `BUILD_TIME` in `MESOS_CPPFLAGS`,
> every build was seen as having a root dependency (the flags)
> changed, causing a rebuild of every single file (including
> dependencies).
> 
> This patch introduces a CMake `configure_file` directive which
> takes in `config.hpp.in` and emits `config.hpp` with the
> `BUILD_TIME`, `BUILD_DATE`, and `BUILD_USER` variables defined.
> It also sets `HAVE_CONFIG_H` which `build.cpp` uses to `#include`
> the configuration file. The result is that the date, time, and user
> are set at the point of configuration (invocation of CMake) instead
> of at build, thus allowing for incremental rebuilds.
> 
> As `config.hpp` is auto-generated by the CMake configuration,
> it is ignored by Git.
> 
> 
> Diffs
> -
> 
>   cmake/CompilationConfigure.cmake ed727e6a679e718f88f158faba9fecc3061ac700 
>   src/common/build.cpp 090b59fb22bfc00516d65f501f8f18cd85a5b4cd 
>   src/common/config.hpp.in PRE-CREATION 
>   support/gitignore 90b6697d19a5e0a68805b23b587b362731a1df25 
> 
> Diff: https://reviews.apache.org/r/57052/diff/
> 
> 
> Testing
> ---
> 
> cmake; make && make check on Linux
> cmake; VS build stout-tests and mesos-tests, then repeat and see no 
> compilation take place a second time. Also support\windows-build.bat twice.
> 
> 
> Thanks,
> 
> Andrew Schwartzmeyer
> 
>

Re: Review Request 55686: Added custom target for 'make distcheck'.

[Joseph Wu]

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



I'm going to discard this, mostly because there is no canonical way for 
`${CMAKE_MAKE_PROGRAM}` to inherit the options of the caller.  i.e. `make -j8 
...`  This is further complicated by the number of `CMAKE_GENERATOR`s that 
exist (Ninja, MSVC, etc).

Basically, we would currently be better off running `distcheck` manually, 
because the caller can specify parallelism or other options as needed.

- Joseph Wu


On Jan. 18, 2017, 2:25 p.m., Srinivas Brahmaroutu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/55686/
> ---
> 
> (Updated Jan. 18, 2017, 2:25 p.m.)
> 
> 
> Review request for mesos, Alex Clemmer and Joseph Wu.
> 
> 
> Bugs: MESOS-5433
> https://issues.apache.org/jira/browse/MESOS-5433
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added custom target for 'make distcheck'.
> 
> 
> Diffs
> -
> 
>   CMakeLists.txt c6b1eb6a7c7e6ed3650847acbec0ddd3101bb96f 
> 
> Diff: https://reviews.apache.org/r/55686/diff/
> 
> 
> Testing
> ---
> 
> make dist
> 
> 
> Thanks,
> 
> Srinivas Brahmaroutu
> 
>

Re: Review Request 57118: Added debugging APIs to agent API docs.

[Kevin Klues]

> On Feb. 28, 2017, 11:34 a.m., Gastón Kleiman wrote:
> > docs/operator-http-api.md, line 3441
> > 
> >
> > Not a native English speaker, but isn't "i.e." usually followed by a 
> > comma?

I tend to follow the rule where I put a comma after it if it's part of the 
sentence itself, but not if it's part of a paranthetical (i.e. something 
enclosed in parantheses). Apparently, the rules are a bit iffy anyway though: 
https://jakubmarian.com/comma-after-i-e-and-e-g/


- Kevin


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


On Feb. 28, 2017, 1:35 a.m., Kevin Klues wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/57118/
> ---
> 
> (Updated Feb. 28, 2017, 1:35 a.m.)
> 
> 
> Review request for mesos, Adam B and Vinod Kone.
> 
> 
> Bugs: MESOS-7188
> https://issues.apache.org/jira/browse/MESOS-7188
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added debugging APIs to agent API docs.
> 
> 
> Diffs
> -
> 
>   docs/operator-http-api.md 94a5477af60de0f9e8e79366fe93adb5299e470c 
> 
> Diff: https://reviews.apache.org/r/57118/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Kevin Klues
> 
>

Re: Review Request 57160: Added 'override' to function declarations in 'LibeventSSLSocketImpl'.

[Greg Mann]

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

(Updated Feb. 28, 2017, 8:51 p.m.)


Review request for mesos, Benjamin Bannier and Joseph Wu.


Changes
---

Addressed comments.


Repository: mesos


Description
---

Inconsistent use of the `override` keyword in
`LibeventSSLSocketImpl` was causing warnings during
clang builds. This patch makes use of the keyword
across all relevant declarations in the class.


Diffs (updated)
-

  3rdparty/libprocess/src/libevent_ssl_socket.hpp 
e589a04d14378f265a8fca871c9f5b0c577f5713 

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


Testing
---

`make check`


Thanks,

Greg Mann

Re: Review Request 56945: Removed inconsistent use of override.

[Greg Mann]

> On Feb. 22, 2017, 6:58 p.m., Greg Mann wrote:
> > Is the motivation here to eliminate the clang warnings? IIUC having the 
> > `override` here, while inconsistent, is strictly an improvement from the 
> > previous state without `override`?
> 
> Benjamin Bannier wrote:
> Yes, we currently emit warnings in the clang build without being 
> interested in fixing this, potentially causing other more interesting 
> warnings to slip through (libprocess is not built with `-Werror`). An 
> alternative fix would be to use `override` correctly here, i.e., mark all 
> overriding methods with as `override` and to drop the now completely 
> redundant `virtual`.
> 
> I agree that this is a strict improvement over the previous state, but I 
> feel implementing MESOS-4871 instead would be the proper fix instead of 
> introducing it incompletely (incompletely both on the code base, and even 
> incompletely on a class here).

I see you already found the new review I posted, but I'll leave this here for 
posterity :)
Would prefer to increase usage of `override` rather than decrease. I posted a 
review here to make it consistent across the `LibeventSSLSocketImpl` class; let 
me know what you think! https://reviews.apache.org/r/57160/


- Greg


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


On Feb. 22, 2017, 6:50 p.m., Benjamin Bannier wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/56945/
> ---
> 
> (Updated Feb. 22, 2017, 6:50 p.m.)
> 
> 
> Review request for mesos, Benjamin Hindman, Greg Mann, and Joseph Wu.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This removes inconsistent use of the 'override' keyword where the
> 'LibeventSSLSocketImpl' class contained both a method explicitly
> marked 'override' and implicitly 'override' functions. This is
> diagnosed as an inconsistency by clang-4.0, e.g.,
> 
> /PATH/libevent_ssl_socket.hpp|43 col 27| warning: \
>   'connect' overrides a member function but is not marked \
>   'override' [-Winconsistent-missing-override]
>   virtual Future connect(const Address& address);
>   ^
> /PATH/socket.hpp|148 col 27| note: \
>   overridden virtual function is here
>   virtual Future connect(const Address& address) = 0;
> 
> A proper fix will be implemented as part of MESOS-4871.
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/src/libevent_ssl_socket.hpp 
> e589a04d14378f265a8fca871c9f5b0c577f5713 
> 
> Diff: https://reviews.apache.org/r/56945/diff/
> 
> 
> Testing
> ---
> 
> make check (OS X, clang/trunk).
> 
> 
> Thanks,
> 
> Benjamin Bannier
> 
>

Re: Review Request 57160: Added 'override' to function declarations in 'LibeventSSLSocketImpl'.

[Benjamin Bannier]

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


Fix it, then Ship it!




This lgtm. I believe that even applying `override` incompletely can prevent bad 
mistakes, but am unsure if we are willing to tolerate inconsistencies in the 
Mesos code base at all. What's your stance on this Joseph?


3rdparty/libprocess/src/libevent_ssl_socket.hpp (line 40)


We should apply `override` here and drop `virtual`.



3rdparty/libprocess/src/libevent_ssl_socket.hpp (lines 43 - 51)


Since you are about to set a precedent, please drop `virtual` everywhere 
here. A function which is `override` already necessarily implements a `virtual` 
function.



3rdparty/libprocess/src/libevent_ssl_socket.hpp (line 63)


Drop `virtual` here as well.


- Benjamin Bannier


On Feb. 28, 2017, 8:55 p.m., Greg Mann wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/57160/
> ---
> 
> (Updated Feb. 28, 2017, 8:55 p.m.)
> 
> 
> Review request for mesos, Benjamin Bannier and Joseph Wu.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Inconsistent use of the `override` keyword in
> `LibeventSSLSocketImpl` was causing warnings during
> clang builds. This patch makes use of the keyword
> across all relevant declarations in the class.
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/src/libevent_ssl_socket.hpp 
> e589a04d14378f265a8fca871c9f5b0c577f5713 
> 
> Diff: https://reviews.apache.org/r/57160/diff/
> 
> 
> Testing
> ---
> 
> `make check`
> 
> 
> Thanks,
> 
> Greg Mann
> 
>

Re: Review Request 56805: Simplified interface for setting weights in allocator.

[Neil Conway]

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

(Updated Feb. 28, 2017, 8:24 p.m.)


Review request for mesos, Adam B and Yongqiao Wang.


Changes
---

Rebase.


Repository: mesos


Description
---

Now that weights can change dynamically, passing a list of weights to
the allocator at initialization-time is unnecessary and confusing (e.g.,
the initial weights might be wrong, if a different set of weight values
are recovered from the registry).

Instead, require that weights are communicated to the allocator via the
existing `updateWeights` method.


Diffs (updated)
-

  include/mesos/allocator/allocator.hpp 
25ed5f36b186f2bd257dd0bdb366d0b21a795622 
  src/master/allocator/mesos/allocator.hpp 
1defb59b686c9cd8d403a0ed6825219f62a7801d 
  src/master/allocator/mesos/hierarchical.hpp 
0bb24be2761bde6d1baad69f5654029f3ceed553 
  src/master/allocator/mesos/hierarchical.cpp 
696815795dc391b4ec7538892b1224b812482d34 
  src/master/master.cpp ae36bf477851bf2fe11eb7913c580e3e0b9cbbe5 
  src/tests/allocator.hpp b6b0022d581bd688900aaf5beb0af7ce6e0129a1 
  src/tests/api_tests.cpp 607392ff6b714a9e812c2802f4d1465e8f71ad09 
  src/tests/hierarchical_allocator_tests.cpp 
cdf1f15b7802439b28405ca8f6634ce83e886630 
  src/tests/master_allocator_tests.cpp 7b0b786f1c6c53616fd7ae1f7f765752d94a4f83 
  src/tests/master_quota_tests.cpp 91219d6693fdd119ed3b0bf734eaa55da9c58b0a 
  src/tests/persistent_volume_endpoints_tests.cpp 
1cc6c9d01a3a473f5a44210ea725310ea5931ff6 
  src/tests/reservation_endpoints_tests.cpp 
345f0457ec1fc00b7033d71227ff178c14e015bb 
  src/tests/reservation_tests.cpp 5c0d01483efb4561b8c0016c3a2fa6ea5574196e 
  src/tests/resource_offers_tests.cpp 74dacf140e49e402a4ad02ce7751e7c7b2f78ee1 
  src/tests/slave_recovery_tests.cpp b5b805868bed61bf482d71322fb1918a0d020d48 

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


Testing
---

`make check`


Thanks,

Neil Conway

Review Request 57167: Updated quota handler logic for hierarchical roles.

[Neil Conway]

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

Review request for mesos and Michael Park.


Repository: mesos


Description
---

The quota'd resources for a nested role are "included" within the
quota'd resources for that role's parent. Hence, the quota of a node
must always be greater than or equal to the sum of the quota'd resources
of that role's children.

When creating and removing quota, we must ensure that this invariant is
not violated.

When computing the cluster capacity heuristic, we must ensure that we do
not "double-count" quota'd resources: e.g., if the cluster has a total
capacity of 100 cpus, role "x" has a quota guarantee of 60 CPUs, and
role "x/y" has a quota guarantee of 55 CPUs, this does NOT violate the
cluster capacity heuristic.


Diffs
-

  src/master/quota_handler.cpp 3ad28e4a9363a877d0610b529a6c17fb30ece37a 
  src/tests/hierarchical_allocator_tests.cpp 
cdf1f15b7802439b28405ca8f6634ce83e886630 
  src/tests/master_quota_tests.cpp 91219d6693fdd119ed3b0bf734eaa55da9c58b0a 

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


Testing
---

`make check`


Thanks,

Neil Conway

Review Request 57166: Updated role validation for hierarchical roles.

[Neil Conway]

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

Review request for mesos, Benjamin Bannier and Michael Park.


Repository: mesos


Description
---

Role names can now contain forward slashes. Each component in a role
name must now be validated separately.


Diffs
-

  src/common/roles.cpp 31774a9b8f99f5efeed35b1c3e3486e05ca00f6a 
  src/tests/role_tests.cpp 77f3d46a544a51ba71476e2f0735bb32758dd9e1 

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


Testing
---

`make check`


Thanks,

Neil Conway

Review Request 57165: Minor cleanup for quota validation code.

[Neil Conway]

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

Review request for mesos, Benjamin Bannier and Michael Park.


Repository: mesos


Description
---

Minor cleanup for quota validation code.


Diffs
-

  src/master/quota.cpp 847ec068f4f3fe94ddabc11ff583d7e76117d375 

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


Testing
---

`make check`


Thanks,

Neil Conway

Review Request 57164: Cleaned up header includes.

[Neil Conway]

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

Review request for mesos and Michael Park.


Repository: mesos


Description
---

Cleaned up header includes.


Diffs
-

  include/mesos/roles.hpp 1e6954447b821c2cf0dfa11373480161a0da5ae6 
  src/common/roles.cpp 31774a9b8f99f5efeed35b1c3e3486e05ca00f6a 
  src/tests/reconciliation_tests.cpp 3573d558ee656ee3e10a25c3b3ed1c36f9fe6a07 
  src/tests/slave_recovery_tests.cpp b5b805868bed61bf482d71322fb1918a0d020d48 
  src/tests/sorter_tests.cpp c93d236b13256f4022a811d019990ef81521aa77 

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


Testing
---

`make check`


Thanks,

Neil Conway

Review Request 57163: Cleaned up sorter test cases.

[Neil Conway]

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

Review request for mesos.


Repository: mesos


Description
---

Cleaned up sorter test cases.


Diffs
-

  src/tests/sorter_tests.cpp c93d236b13256f4022a811d019990ef81521aa77 

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


Testing
---

`make check`


Thanks,

Neil Conway

Review Request 57162: Minor cleanup for slave tests.

[Neil Conway]

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

Review request for mesos and Michael Park.


Repository: mesos


Description
---

Minor cleanup for slave tests.


Diffs
-

  src/tests/slave_tests.cpp 3731c7607d5e49f3000c4863b1999851fac45705 

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


Testing
---

`make check`


Thanks,

Neil Conway

Review Request 57161: Cleaned up header includes.

[Neil Conway]

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

Review request for mesos and Michael Park.


Repository: mesos


Description
---

Cleaned up header includes.


Diffs
-

  src/linux/capabilities.hpp 7c6342745a7db1ecf8664c64be77b10a54d5037a 
  src/linux/capabilities.cpp 9f46bd0a34d35d8295b1899a7ba67d85c3c24348 

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


Testing
---

`make check`


Thanks,

Neil Conway

Re: Review Request 57157: Cleaned up `using` statements.

[Neil Conway]

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

(Updated Feb. 28, 2017, 8:07 p.m.)


Review request for mesos and Michael Park.


Changes
---

Fix compilation on Linux.


Repository: mesos


Description
---

A source file should typically not contain both `using xyz::foo` and
`xyz::foo`.


Diffs (updated)
-

  src/linux/capabilities.cpp 9f46bd0a34d35d8295b1899a7ba67d85c3c24348 
  src/master/weights_handler.cpp 4f6a4cd29d27b64cbc9ee6d8909f79812313b681 
  src/slave/containerizer/docker.cpp 7d801fb17565a5298e8e3c5b430e070e12473680 
  src/slave/containerizer/mesos/isolators/cgroups/subsystems/net_cls.cpp 
2608df1aeabfd2b26f2786eaab3d2a47c2f5710d 
  src/slave/containerizer/mesos/isolators/gpu/allocator.cpp 
2e722691475c84afae14009014ea70cc0fdd0e65 
  
src/slave/containerizer/mesos/isolators/network/cni/plugins/port_mapper/port_mapper.cpp
 d0cf80b82cb4c9d224bd0c9ec1e11077e05ce7f5 
  src/slave/containerizer/mesos/linux_launcher.cpp 
a3409dc64435e002072b9b458a5fda0564214f28 
  src/slave/containerizer/mesos/paths.cpp 
c1770cefe0287ce994eec6979db41201148ef3fb 
  src/slave/flags.cpp 8de2643ea6cd775636e91af989b596fa7a0f9abc 
  src/tests/container_logger_tests.cpp 589d6a9df7ce964052355be41597ef11677ca03d 
  src/tests/containerizer/mesos_containerizer_tests.cpp 
039f197ece893fdf95c25fe60bb827fa2d7a3cbf 
  src/tests/containerizer/rootfs.cpp 513c1b544f9e5239fa2a8e8ebedc3d84a6384ba3 

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


Testing
---

`make check`


Thanks,

Neil Conway

Re: Review Request 56806: Fixed some clang-tidy warnings.

[Neil Conway]

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

(Updated Feb. 28, 2017, 8:07 p.m.)


Review request for mesos and Michael Park.


Changes
---

Address review comments, more minor cleanup.


Repository: mesos


Description
---

Fixed some clang-tidy warnings.


Diffs (updated)
-

  src/common/protobuf_utils.cpp 944263bbaa87a65005fd924bccfadb7293312fa0 
  src/master/detector/detector.cpp 1ebe5af4219a7e1ec1cbf22b440dd3ecaad4b4a9 
  src/slave/slave.cpp fc480ae23ffa5cdeeb79b3621a08e1f8703bc01a 
  src/tests/containerizer/docker_containerizer_tests.cpp 
e760f47b3d8c4054bad8ff65269107d5583c9a90 
  src/tests/fetcher_cache_tests.cpp e5eee9bce56edacb74ad80ff859ddbc5048c73d0 
  src/tests/gc_tests.cpp 293c1461a5fbdb5a3d3098290fa5c1d21e15c85e 
  src/tests/hook_tests.cpp 237df8102941cd143c4d61016963e62a69c43382 
  src/tests/partition_tests.cpp 105157deaa500642a490b8bda0624629035a95a5 
  src/tests/persistent_volume_endpoints_tests.cpp 
1cc6c9d01a3a473f5a44210ea725310ea5931ff6 

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


Testing
---

`make check`


Thanks,

Neil Conway

Review Request 57160: Added 'override' to function declarations in 'LibeventSSLSocketImpl'.

[Greg Mann]

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

Review request for mesos, Benjamin Bannier and Joseph Wu.


Repository: mesos


Description
---

Inconsistent use of the `override` keyword in
`LibeventSSLSocketImpl` was causing warnings during
clang builds. This patch makes use of the keyword
across all relevant declarations in the class.


Diffs
-

  3rdparty/libprocess/src/libevent_ssl_socket.hpp 
e589a04d14378f265a8fca871c9f5b0c577f5713 

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


Testing
---

`make check`


Thanks,

Greg Mann

Re: Review Request 56813: Updated master handlers to use the 'Principal' type.

[Greg Mann]

> On Feb. 28, 2017, 1:38 p.m., Alexander Rojas wrote:
> > src/master/http.cpp, lines 2224-2227
> > 
> >
> > This line didn't need modification. However it does raises the issue of 
> > the signature of `validation::operation::validate()`. The common pattern in 
> > Mesos is to use a default parameter in the declaration, like:
> > 
> > ```c++
> > Option validate(
> > const Offer::Operation::Reserve& reserve,
> > const Option& principal,
> > const Option& frameworkInfo = None())
> > ```
> > 
> > so that the caller doesn't need to do it. Doing a grep on the srouce 
> > code in over a hundred instances of that pattern.
> > 
> > Could you quickly fix that?
> 
> Greg Mann wrote:
> Sure thing, I'll follow-up with another patch and leave a link here.

Follow-up review here: https://reviews.apache.org/r/57158/


- Greg


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


On Feb. 28, 2017, 6:34 p.m., Greg Mann wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/56813/
> ---
> 
> (Updated Feb. 28, 2017, 6:34 p.m.)
> 
> 
> Review request for mesos, Adam B, Alexander Rojas, Jan Schlicht, Till 
> Toenshoff, and Vinod Kone.
> 
> 
> Bugs: MESOS-7003
> https://issues.apache.org/jira/browse/MESOS-7003
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This patch updates the HTTP endpoint handlers in the
> master process to accept the `Principal` type instead
> of an `Option& principal`.
> 
> 
> Diffs
> -
> 
>   src/master/http.cpp 6e5178eeea6cc6b90ae253840da22be13444b088 
>   src/master/master.hpp 764adb18ddf09b62529c5c96f8e4dfaf7803483e 
>   src/master/master.cpp 2ef836536784d5fda23c80dc2304239b176a8942 
>   src/master/quota_handler.cpp 3ad28e4a9363a877d0610b529a6c17fb30ece37a 
>   src/master/registrar.cpp d7134eea34102ab7b24d2f0131363bdd9005cfd3 
>   src/master/weights_handler.cpp 4f6a4cd29d27b64cbc9ee6d8909f79812313b681 
> 
> Diff: https://reviews.apache.org/r/56813/diff/
> 
> 
> Testing
> ---
> 
> Testing details can be found at the end of this review chain.
> 
> 
> Thanks,
> 
> Greg Mann
> 
>

Re: Review Request 56611: Relax perf version check for Arch Linux.

[Neil Conway]

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


Ship it!




Ship It!

- Neil Conway


On Feb. 28, 2017, 5:43 a.m., James Peach wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/56611/
> ---
> 
> (Updated Feb. 28, 2017, 5:43 a.m.)
> 
> 
> Review request for mesos and Neil Conway.
> 
> 
> Bugs: MESOS-6982
> https://issues.apache.org/jira/browse/MESOS-6982
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Relax perf version check for Arch Linux.
> 
> 
> Diffs
> -
> 
>   src/linux/perf.hpp 9c4330b6086abb18f03660fe89a6fb01d8ed 
>   src/linux/perf.cpp 3141e5ee9eee78e974625791f362dc345c682ebb 
>   src/tests/containerizer/perf_tests.cpp 
> d536ecc5cb24787bc3487efb146573cd4f3ded43 
> 
> Diff: https://reviews.apache.org/r/56611/diff/
> 
> 
> Testing
> ---
> 
> make check (Fedora 25)
> 
> 
> Thanks,
> 
> James Peach
> 
>

Review Request 57158: Added default parameter value to master validation function.

[Greg Mann]

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

Review request for mesos, Alexander Rojas and Vinod Kone.


Repository: mesos


Description
---

The master's validation function for RESERVE operations previously
did not set a default parameter value for its final optional
parameter, requiring callsites to explicitly specify `None()`. This
patch adds the default value.


Diffs
-

  src/master/http.cpp 6e5178eeea6cc6b90ae253840da22be13444b088 
  src/master/validation.hpp f03b3280704083be1ca074ca07c69edbb49dae19 

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


Testing
---

`make check`


Thanks,

Greg Mann

Re: Review Request 57052: CMake: Add `config.hpp.in` for `BUILD_TIME` variables.

[Joseph Wu]

> On Feb. 28, 2017, 10:05 a.m., Andrew Schwartzmeyer wrote:
> > support/gitignore, line 36
> > 
> >
> > Ah, note to myself: update the output to go only into `build` instead 
> > of into `${CMAKE_SOURCE_DIR}`.

`${CMAKE_BINARY_DIR}` is the magic variable you'll want to use.


- Joseph


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


On Feb. 24, 2017, 4:31 p.m., Andrew Schwartzmeyer wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/57052/
> ---
> 
> (Updated Feb. 24, 2017, 4:31 p.m.)
> 
> 
> Review request for mesos, Alex Clemmer, Joseph Wu, and Michael Park.
> 
> 
> Bugs: MESOS-7172
> https://issues.apache.org/jira/browse/MESOS-7172
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Commit c7fc1377b introduced a bug that prevented any incremental
> recompilation. By defining the `BUILD_TIME` in `MESOS_CPPFLAGS`,
> every build was seen as having a root dependency (the flags)
> changed, causing a rebuild of every single file (including
> dependencies).
> 
> This patch introduces a CMake `configure_file` directive which
> takes in `config.hpp.in` and emits `config.hpp` with the
> `BUILD_TIME`, `BUILD_DATE`, and `BUILD_USER` variables defined.
> It also sets `HAVE_CONFIG_H` which `build.cpp` uses to `#include`
> the configuration file. The result is that the date, time, and user
> are set at the point of configuration (invocation of CMake) instead
> of at build, thus allowing for incremental rebuilds.
> 
> As `config.hpp` is auto-generated by the CMake configuration,
> it is ignored by Git.
> 
> 
> Diffs
> -
> 
>   cmake/CompilationConfigure.cmake ed727e6a679e718f88f158faba9fecc3061ac700 
>   src/common/build.cpp 090b59fb22bfc00516d65f501f8f18cd85a5b4cd 
>   src/common/config.hpp.in PRE-CREATION 
>   support/gitignore 90b6697d19a5e0a68805b23b587b362731a1df25 
> 
> Diff: https://reviews.apache.org/r/57052/diff/
> 
> 
> Testing
> ---
> 
> cmake; make && make check on Linux
> cmake; VS build stout-tests and mesos-tests, then repeat and see no 
> compilation take place a second time. Also support\windows-build.bat twice.
> 
> 
> Thanks,
> 
> Andrew Schwartzmeyer
> 
>

Re: Review Request 57157: Cleaned up `using` statements.

[Michael Park]

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


Ship it!




Ship It!

- Michael Park


On Feb. 28, 2017, 11:14 a.m., Neil Conway wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/57157/
> ---
> 
> (Updated Feb. 28, 2017, 11:14 a.m.)
> 
> 
> Review request for mesos and Michael Park.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> A source file should typically not contain both `using xyz::foo` and
> `xyz::foo`.
> 
> 
> Diffs
> -
> 
>   src/linux/capabilities.cpp 9f46bd0a34d35d8295b1899a7ba67d85c3c24348 
>   src/master/weights_handler.cpp 4f6a4cd29d27b64cbc9ee6d8909f79812313b681 
>   src/slave/containerizer/docker.cpp 7d801fb17565a5298e8e3c5b430e070e12473680 
>   src/slave/containerizer/mesos/isolators/cgroups/subsystems/net_cls.cpp 
> 2608df1aeabfd2b26f2786eaab3d2a47c2f5710d 
>   src/slave/containerizer/mesos/isolators/gpu/allocator.cpp 
> 2e722691475c84afae14009014ea70cc0fdd0e65 
>   
> src/slave/containerizer/mesos/isolators/network/cni/plugins/port_mapper/port_mapper.cpp
>  d0cf80b82cb4c9d224bd0c9ec1e11077e05ce7f5 
>   src/slave/containerizer/mesos/linux_launcher.cpp 
> a3409dc64435e002072b9b458a5fda0564214f28 
>   src/slave/containerizer/mesos/paths.cpp 
> c1770cefe0287ce994eec6979db41201148ef3fb 
>   src/slave/flags.cpp 8de2643ea6cd775636e91af989b596fa7a0f9abc 
>   src/tests/container_logger_tests.cpp 
> 589d6a9df7ce964052355be41597ef11677ca03d 
>   src/tests/containerizer/mesos_containerizer_tests.cpp 
> 039f197ece893fdf95c25fe60bb827fa2d7a3cbf 
>   src/tests/containerizer/rootfs.cpp 513c1b544f9e5239fa2a8e8ebedc3d84a6384ba3 
> 
> Diff: https://reviews.apache.org/r/57157/diff/
> 
> 
> Testing
> ---
> 
> `make check`
> 
> 
> Thanks,
> 
> Neil Conway
> 
>

Re: Review Request 56806: Fixed some clang-tidy warnings.

[Michael Park]

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


Fix it, then Ship it!





src/tests/containerizer/docker_containerizer_tests.cpp (line 2982)


Looks like this one only has 1 use as well. It seems like rest of the 
places you just replaced the 1 use with `offer.slave_id()`.


- Michael Park


On Feb. 28, 2017, 11:13 a.m., Neil Conway wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/56806/
> ---
> 
> (Updated Feb. 28, 2017, 11:13 a.m.)
> 
> 
> Review request for mesos and Michael Park.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Fixed some clang-tidy warnings.
> 
> 
> Diffs
> -
> 
>   src/common/protobuf_utils.cpp 944263bbaa87a65005fd924bccfadb7293312fa0 
>   src/master/detector/detector.cpp 1ebe5af4219a7e1ec1cbf22b440dd3ecaad4b4a9 
>   src/slave/slave.cpp fc480ae23ffa5cdeeb79b3621a08e1f8703bc01a 
>   src/tests/containerizer/docker_containerizer_tests.cpp 
> e760f47b3d8c4054bad8ff65269107d5583c9a90 
>   src/tests/fetcher_cache_tests.cpp e5eee9bce56edacb74ad80ff859ddbc5048c73d0 
>   src/tests/hook_tests.cpp 237df8102941cd143c4d61016963e62a69c43382 
>   src/tests/persistent_volume_endpoints_tests.cpp 
> 1cc6c9d01a3a473f5a44210ea725310ea5931ff6 
> 
> Diff: https://reviews.apache.org/r/56806/diff/
> 
> 
> Testing
> ---
> 
> `make check`
> 
> 
> Thanks,
> 
> Neil Conway
> 
>

Review Request 57157: Cleaned up `using` statements.

[Neil Conway]

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

Review request for mesos and Michael Park.


Repository: mesos


Description
---

A source file should typically not contain both `using xyz::foo` and
`xyz::foo`.


Diffs
-

  src/linux/capabilities.cpp 9f46bd0a34d35d8295b1899a7ba67d85c3c24348 
  src/master/weights_handler.cpp 4f6a4cd29d27b64cbc9ee6d8909f79812313b681 
  src/slave/containerizer/docker.cpp 7d801fb17565a5298e8e3c5b430e070e12473680 
  src/slave/containerizer/mesos/isolators/cgroups/subsystems/net_cls.cpp 
2608df1aeabfd2b26f2786eaab3d2a47c2f5710d 
  src/slave/containerizer/mesos/isolators/gpu/allocator.cpp 
2e722691475c84afae14009014ea70cc0fdd0e65 
  
src/slave/containerizer/mesos/isolators/network/cni/plugins/port_mapper/port_mapper.cpp
 d0cf80b82cb4c9d224bd0c9ec1e11077e05ce7f5 
  src/slave/containerizer/mesos/linux_launcher.cpp 
a3409dc64435e002072b9b458a5fda0564214f28 
  src/slave/containerizer/mesos/paths.cpp 
c1770cefe0287ce994eec6979db41201148ef3fb 
  src/slave/flags.cpp 8de2643ea6cd775636e91af989b596fa7a0f9abc 
  src/tests/container_logger_tests.cpp 589d6a9df7ce964052355be41597ef11677ca03d 
  src/tests/containerizer/mesos_containerizer_tests.cpp 
039f197ece893fdf95c25fe60bb827fa2d7a3cbf 
  src/tests/containerizer/rootfs.cpp 513c1b544f9e5239fa2a8e8ebedc3d84a6384ba3 

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


Testing
---

`make check`


Thanks,

Neil Conway

Re: Review Request 56806: Fixed some clang-tidy warnings.

[Neil Conway]

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

(Updated Feb. 28, 2017, 7:13 p.m.)


Review request for mesos and Michael Park.


Changes
---

Tweak commit message.


Repository: mesos


Description (updated)
---

Fixed some clang-tidy warnings.


Diffs (updated)
-

  src/common/protobuf_utils.cpp 944263bbaa87a65005fd924bccfadb7293312fa0 
  src/master/detector/detector.cpp 1ebe5af4219a7e1ec1cbf22b440dd3ecaad4b4a9 
  src/slave/slave.cpp fc480ae23ffa5cdeeb79b3621a08e1f8703bc01a 
  src/tests/containerizer/docker_containerizer_tests.cpp 
e760f47b3d8c4054bad8ff65269107d5583c9a90 
  src/tests/fetcher_cache_tests.cpp e5eee9bce56edacb74ad80ff859ddbc5048c73d0 
  src/tests/hook_tests.cpp 237df8102941cd143c4d61016963e62a69c43382 
  src/tests/persistent_volume_endpoints_tests.cpp 
1cc6c9d01a3a473f5a44210ea725310ea5931ff6 

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


Testing
---

`make check`


Thanks,

Neil Conway

Re: Review Request 56812: Updated agent handlers to use the 'Principal' type.

[Greg Mann]

> On Feb. 24, 2017, 2:07 a.m., Vinod Kone wrote:
> > src/slave/http.cpp, line 836
> > 
> >
> > not related to your change, but I'm curious why this method uses the 
> > object approver whereas the `Slave::Http::flags()` doesn't? cc @arojas
> 
> Greg Mann wrote:
> I'm guessing it's just for historical reasons; the old handler probably 
> was not updated when the newer one was added?
> 
> Alexander Rojas wrote:
> As Greg says, it is mostly historical reasons. Once the objectApprover 
> was introduced, it is consider a better option, but we haven't consolidate an 
> effort to move everthing towards the object approver.

I created a ticket to track migrating to the newer interface: 
https://issues.apache.org/jira/browse/MESOS-7190


- Greg


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


On Feb. 28, 2017, 6:37 a.m., Greg Mann wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/56812/
> ---
> 
> (Updated Feb. 28, 2017, 6:37 a.m.)
> 
> 
> Review request for mesos, Adam B, Alexander Rojas, Jan Schlicht, Till 
> Toenshoff, and Vinod Kone.
> 
> 
> Bugs: MESOS-7003
> https://issues.apache.org/jira/browse/MESOS-7003
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This patch updates the HTTP endpoint handlers in the
> agent process to accept the `Principal` type instead
> of an `Option& principal`.
> 
> 
> Diffs
> -
> 
>   src/slave/http.cpp 94731ec883c309cefb811694dc4e39de12d1ac59 
>   src/slave/slave.hpp 3b0aea4e3e9a17501077beccbccaab4abbe11af2 
>   src/slave/slave.cpp fc480ae23ffa5cdeeb79b3621a08e1f8703bc01a 
> 
> Diff: https://reviews.apache.org/r/56812/diff/
> 
> 
> Testing
> ---
> 
> Testing details can be found at the end of this review chain.
> 
> 
> Thanks,
> 
> Greg Mann
> 
>

Re: Review Request 57117: Added nested container launch/wait/kill APIs to agent API docs.

[Vinod Kone]

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


Ship it!




Ship It!

- Vinod Kone


On Feb. 28, 2017, 6:08 p.m., Kevin Klues wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/57117/
> ---
> 
> (Updated Feb. 28, 2017, 6:08 p.m.)
> 
> 
> Review request for mesos, Adam B, Benjamin Mahler, and Vinod Kone.
> 
> 
> Bugs: MESOS-7189
> https://issues.apache.org/jira/browse/MESOS-7189
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added nested container launch/wait/kill APIs to agent API docs.
> 
> 
> Diffs
> -
> 
>   docs/operator-http-api.md 94a5477af60de0f9e8e79366fe93adb5299e470c 
> 
> Diff: https://reviews.apache.org/r/57117/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Kevin Klues
> 
>

Re: Review Request 56813: Updated master handlers to use the 'Principal' type.

[Greg Mann]

> On Feb. 28, 2017, 1:38 p.m., Alexander Rojas wrote:
> > src/master/http.cpp, line 115
> > 
> >
> > wasn't metrics process used at all?

It wasn't - I split this change out into a separate review: 
https://reviews.apache.org/r/57153/


- Greg


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


On Feb. 28, 2017, 6:34 p.m., Greg Mann wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/56813/
> ---
> 
> (Updated Feb. 28, 2017, 6:34 p.m.)
> 
> 
> Review request for mesos, Adam B, Alexander Rojas, Jan Schlicht, Till 
> Toenshoff, and Vinod Kone.
> 
> 
> Bugs: MESOS-7003
> https://issues.apache.org/jira/browse/MESOS-7003
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This patch updates the HTTP endpoint handlers in the
> master process to accept the `Principal` type instead
> of an `Option& principal`.
> 
> 
> Diffs
> -
> 
>   src/master/http.cpp 6e5178eeea6cc6b90ae253840da22be13444b088 
>   src/master/master.hpp 764adb18ddf09b62529c5c96f8e4dfaf7803483e 
>   src/master/master.cpp 2ef836536784d5fda23c80dc2304239b176a8942 
>   src/master/quota_handler.cpp 3ad28e4a9363a877d0610b529a6c17fb30ece37a 
>   src/master/registrar.cpp d7134eea34102ab7b24d2f0131363bdd9005cfd3 
>   src/master/weights_handler.cpp 4f6a4cd29d27b64cbc9ee6d8909f79812313b681 
> 
> Diff: https://reviews.apache.org/r/56813/diff/
> 
> 
> Testing
> ---
> 
> Testing details can be found at the end of this review chain.
> 
> 
> Thanks,
> 
> Greg Mann
> 
>

Re: Review Request 56813: Updated master handlers to use the 'Principal' type.

[Greg Mann]

> On Feb. 28, 2017, 1:38 p.m., Alexander Rojas wrote:
> > src/master/http.cpp, lines 889-891
> > 
> >
> > I'm not sure printing the whole `principal` with name and claims is 
> > such a good idea. The reason is that you only print 
> > `framework->info.principal()`, and to my knowledge, only compare the names. 
> > Perhaps is a better idea to only print `principal->name`?

I agree it would be better to just print the `value`; unfortunately, it's 
possible here that `principal->value` is NONE. We could have another 
conditional for the NONE case but I don't think it's worth it.


> On Feb. 28, 2017, 1:38 p.m., Alexander Rojas wrote:
> > src/master/http.cpp, lines 2224-2227
> > 
> >
> > This line didn't need modification. However it does raises the issue of 
> > the signature of `validation::operation::validate()`. The common pattern in 
> > Mesos is to use a default parameter in the declaration, like:
> > 
> > ```c++
> > Option validate(
> > const Offer::Operation::Reserve& reserve,
> > const Option& principal,
> > const Option& frameworkInfo = None())
> > ```
> > 
> > so that the caller doesn't need to do it. Doing a grep on the srouce 
> > code in over a hundred instances of that pattern.
> > 
> > Could you quickly fix that?

Sure thing, I'll follow-up with another patch and leave a link here.


- Greg


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


On Feb. 28, 2017, 6:34 p.m., Greg Mann wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/56813/
> ---
> 
> (Updated Feb. 28, 2017, 6:34 p.m.)
> 
> 
> Review request for mesos, Adam B, Alexander Rojas, Jan Schlicht, Till 
> Toenshoff, and Vinod Kone.
> 
> 
> Bugs: MESOS-7003
> https://issues.apache.org/jira/browse/MESOS-7003
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This patch updates the HTTP endpoint handlers in the
> master process to accept the `Principal` type instead
> of an `Option& principal`.
> 
> 
> Diffs
> -
> 
>   src/master/http.cpp 6e5178eeea6cc6b90ae253840da22be13444b088 
>   src/master/master.hpp 764adb18ddf09b62529c5c96f8e4dfaf7803483e 
>   src/master/master.cpp 2ef836536784d5fda23c80dc2304239b176a8942 
>   src/master/quota_handler.cpp 3ad28e4a9363a877d0610b529a6c17fb30ece37a 
>   src/master/registrar.cpp d7134eea34102ab7b24d2f0131363bdd9005cfd3 
>   src/master/weights_handler.cpp 4f6a4cd29d27b64cbc9ee6d8909f79812313b681 
> 
> Diff: https://reviews.apache.org/r/56813/diff/
> 
> 
> Testing
> ---
> 
> Testing details can be found at the end of this review chain.
> 
> 
> Thanks,
> 
> Greg Mann
> 
>

Re: Review Request 56813: Updated master handlers to use the 'Principal' type.

[Greg Mann]

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

(Updated Feb. 28, 2017, 6:34 p.m.)


Review request for mesos, Adam B, Alexander Rojas, Jan Schlicht, Till 
Toenshoff, and Vinod Kone.


Changes
---

Addressed comments.


Summary (updated)
-

Updated master handlers to use the 'Principal' type.


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


Repository: mesos


Description
---

This patch updates the HTTP endpoint handlers in the
master process to accept the `Principal` type instead
of an `Option& principal`.


Diffs (updated)
-

  src/master/http.cpp 6e5178eeea6cc6b90ae253840da22be13444b088 
  src/master/master.hpp 764adb18ddf09b62529c5c96f8e4dfaf7803483e 
  src/master/master.cpp 2ef836536784d5fda23c80dc2304239b176a8942 
  src/master/quota_handler.cpp 3ad28e4a9363a877d0610b529a6c17fb30ece37a 
  src/master/registrar.cpp d7134eea34102ab7b24d2f0131363bdd9005cfd3 
  src/master/weights_handler.cpp 4f6a4cd29d27b64cbc9ee6d8909f79812313b681 

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


Testing
---

Testing details can be found at the end of this review chain.


Thanks,

Greg Mann

Re: Review Request 56619: Updated 'Files' handlers to use the 'Principal' type.

[Greg Mann]

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

(Updated Feb. 28, 2017, 6:32 p.m.)


Review request for mesos, Adam B, Alexander Rojas, Jan Schlicht, Till 
Toenshoff, and Vinod Kone.


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


Repository: mesos


Description
---

This patch updates the HTTP endpoint handlers in the
Mesos `Files` process to accept the `Principal` type
instead of an `Option& principal`.


Diffs (updated)
-

  src/files/files.hpp 8cffc26fc7d674187e55663f23f1e10bed40229e 
  src/files/files.cpp 8327f8002fbfa3be77a4bbe4aa83a73d0f170f7a 

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


Testing
---

Testing information can be found at the end of this review chain.


Thanks,

Greg Mann

Review Request 57153: Removed unnecessary 'using' statement in master HTTP code.

[Greg Mann]

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

Review request for mesos, Alexander Rojas, Jan Schlicht, and Vinod Kone.


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


Repository: mesos


Description
---

`MetricsProcess` was previously declared in 'src/master/http.cpp',
but it is not currently used in that file. This patch removes the
declaration.


Diffs
-

  src/master/http.cpp 6e5178eeea6cc6b90ae253840da22be13444b088 

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


Testing
---

Testing details can be found at the end of this patch chain.


Thanks,

Greg Mann

Re: Review Request 56623: Implemented the 'Principal' type in libprocess.

[Greg Mann]

> On Feb. 27, 2017, 11:58 a.m., Alexander Rojas wrote:
> > 3rdparty/libprocess/include/process/authenticator.hpp, lines 65-72
> > 
> >
> > I would prefer if either the definition or the whole function could be 
> > moved to `authenticator.cpp`
> 
> Greg Mann wrote:
> In some of Jan's upcoming patches, he's moving 'authenticator.cpp' to 
> 'basic_authenticator.cpp'. I left this definition in the header because it 
> didn't seem worth it to me to maintain the file 'authenticator.cpp' just for 
> this function, but perhaps we should?
> 
> Alexander Rojas wrote:
> I would prefer that, or at least a TODO mentioning that once stuff from 
> `authenticator.cpp` goes into `basic_authenticator.cpp` this should go with 
> it. Perhaps we even need some stuff in `authenticator.cpp` and this is a 
> prime candidate.

OK I moved these to 'authenticator.cpp'; let's keep that file after Jan's 
changes.


> On Feb. 27, 2017, 11:58 a.m., Alexander Rojas wrote:
> > 3rdparty/libprocess/include/process/authenticator.hpp, line 62
> > 
> >
> > `static` keyword doesn't have that much use in a header.
> 
> Greg Mann wrote:
> In this context, the `static` keyword gives the function internal 
> linkage. I could also use an anonymous namespace; I don't have a strong 
> preference for one or the other.
> 
> Alexander Rojas wrote:
> Anonymous namespace seems to be the preferred way.

Moved to 'authenticator.cpp'.


- Greg


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


On Feb. 28, 2017, 6:28 p.m., Greg Mann wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/56623/
> ---
> 
> (Updated Feb. 28, 2017, 6:28 p.m.)
> 
> 
> Review request for mesos, Adam B, Alexander Rojas, Jan Schlicht, Till 
> Toenshoff, and Vinod Kone.
> 
> 
> Bugs: MESOS-7003
> https://issues.apache.org/jira/browse/MESOS-7003
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This patch adds a new struct, `Principal`, to libprocess
> to represent an authenticated entity in the system.
> The new type contains a string `value` and a map containing
> arbitrary key-value pairs.
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/include/process/authenticator.hpp 
> e5489c6cb4adc8a822e7dd4515542618c36136f9 
>   3rdparty/libprocess/include/process/http.hpp 
> eb2c87dee2a911085592e0e14a7499d526ad0bfc 
>   3rdparty/libprocess/include/process/process.hpp 
> ceedbe86ccba5c24bf1e9e7f032d7dbcf6acb0f3 
>   3rdparty/libprocess/src/authenticator.cpp 
> cfedb6f7674e0f6690e77a633cdd1bd494c7d2c7 
>   3rdparty/libprocess/src/authenticator_manager.cpp 
> a22acd026a001788dc39b8005a56577e33c6800b 
>   3rdparty/libprocess/src/process.cpp 
> 3ad485f9136cd9edf0a6db76ff1f475039236391 
> 
> Diff: https://reviews.apache.org/r/56623/diff/
> 
> 
> Testing
> ---
> 
> Testing information can be found at the end of this review chain.
> 
> 
> Thanks,
> 
> Greg Mann
> 
>

Re: Review Request 56623: Implemented the 'Principal' type in libprocess.

[Greg Mann]

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

(Updated Feb. 28, 2017, 6:28 p.m.)


Review request for mesos, Adam B, Alexander Rojas, Jan Schlicht, Till 
Toenshoff, and Vinod Kone.


Changes
---

Addressed comments.


Summary (updated)
-

Implemented the 'Principal' type in libprocess.


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


Repository: mesos


Description
---

This patch adds a new struct, `Principal`, to libprocess
to represent an authenticated entity in the system.
The new type contains a string `value` and a map containing
arbitrary key-value pairs.


Diffs (updated)
-

  3rdparty/libprocess/include/process/authenticator.hpp 
e5489c6cb4adc8a822e7dd4515542618c36136f9 
  3rdparty/libprocess/include/process/http.hpp 
eb2c87dee2a911085592e0e14a7499d526ad0bfc 
  3rdparty/libprocess/include/process/process.hpp 
ceedbe86ccba5c24bf1e9e7f032d7dbcf6acb0f3 
  3rdparty/libprocess/src/authenticator.cpp 
cfedb6f7674e0f6690e77a633cdd1bd494c7d2c7 
  3rdparty/libprocess/src/authenticator_manager.cpp 
a22acd026a001788dc39b8005a56577e33c6800b 
  3rdparty/libprocess/src/process.cpp 3ad485f9136cd9edf0a6db76ff1f475039236391 

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


Testing
---

Testing information can be found at the end of this review chain.


Thanks,

Greg Mann

Re: Review Request 57117: Added nested container launch/wait/kill APIs to agent API docs.

[Kevin Klues]

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

(Updated Feb. 28, 2017, 6:08 p.m.)


Review request for mesos, Adam B, Benjamin Mahler, and Vinod Kone.


Changes
---

Updated based on Vinod's comments.


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


Repository: mesos


Description
---

Added nested container launch/wait/kill APIs to agent API docs.


Diffs (updated)
-

  docs/operator-http-api.md 94a5477af60de0f9e8e79366fe93adb5299e470c 

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


Testing
---


Thanks,

Kevin Klues

Re: Review Request 57052: CMake: Add `config.hpp.in` for `BUILD_TIME` variables.

[Andrew Schwartzmeyer]

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




cmake/CompilationConfigure.cmake (line 298)


Joseph Wu brought up a point that this _may_ not update the output file 
`config.hpp` on each invocation of CMake, as `config.hpp.in` itself doesn't 
change.

However, I double checked and with CMake 3.7.1 at least, it does indeed 
behave as I expected. Each direction invocation of `cmake` reruns the CMake 
command `configure_file` which has different input due to `BUILD_TIME` 
changing, and so updates the file. 

While the semantics _slightly_ change from 'compilation time' to 
'configuration time', we agree this is acceptable.



support/gitignore (line 36)


Ah, note to myself: update the output to go only into `build` instead of 
into `${CMAKE_SOURCE_DIR}`.


- Andrew Schwartzmeyer


On Feb. 25, 2017, 12:31 a.m., Andrew Schwartzmeyer wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/57052/
> ---
> 
> (Updated Feb. 25, 2017, 12:31 a.m.)
> 
> 
> Review request for mesos, Alex Clemmer, Joseph Wu, and Michael Park.
> 
> 
> Bugs: MESOS-7172
> https://issues.apache.org/jira/browse/MESOS-7172
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Commit c7fc1377b introduced a bug that prevented any incremental
> recompilation. By defining the `BUILD_TIME` in `MESOS_CPPFLAGS`,
> every build was seen as having a root dependency (the flags)
> changed, causing a rebuild of every single file (including
> dependencies).
> 
> This patch introduces a CMake `configure_file` directive which
> takes in `config.hpp.in` and emits `config.hpp` with the
> `BUILD_TIME`, `BUILD_DATE`, and `BUILD_USER` variables defined.
> It also sets `HAVE_CONFIG_H` which `build.cpp` uses to `#include`
> the configuration file. The result is that the date, time, and user
> are set at the point of configuration (invocation of CMake) instead
> of at build, thus allowing for incremental rebuilds.
> 
> As `config.hpp` is auto-generated by the CMake configuration,
> it is ignored by Git.
> 
> 
> Diffs
> -
> 
>   cmake/CompilationConfigure.cmake ed727e6a679e718f88f158faba9fecc3061ac700 
>   src/common/build.cpp 090b59fb22bfc00516d65f501f8f18cd85a5b4cd 
>   src/common/config.hpp.in PRE-CREATION 
>   support/gitignore 90b6697d19a5e0a68805b23b587b362731a1df25 
> 
> Diff: https://reviews.apache.org/r/57052/diff/
> 
> 
> Testing
> ---
> 
> cmake; make && make check on Linux
> cmake; VS build stout-tests and mesos-tests, then repeat and see no 
> compilation take place a second time. Also support\windows-build.bat twice.
> 
> 
> Thanks,
> 
> Andrew Schwartzmeyer
> 
>

Re: Review Request 57118: Added debugging APIs to agent API docs.

[Vinod Kone]

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


Fix it, then Ship it!





docs/operator-http-api.md (line 3429)


s/STDOUT/STDERR/ maybe?



docs/operator-http-api.md (line 3570)


s/STDOUT/STDERR/ maybe?


- Vinod Kone


On Feb. 28, 2017, 1:35 a.m., Kevin Klues wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/57118/
> ---
> 
> (Updated Feb. 28, 2017, 1:35 a.m.)
> 
> 
> Review request for mesos, Adam B and Vinod Kone.
> 
> 
> Bugs: MESOS-7188
> https://issues.apache.org/jira/browse/MESOS-7188
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added debugging APIs to agent API docs.
> 
> 
> Diffs
> -
> 
>   docs/operator-http-api.md 94a5477af60de0f9e8e79366fe93adb5299e470c 
> 
> Diff: https://reviews.apache.org/r/57118/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Kevin Klues
> 
>

Re: Review Request 57117: Added nested container launch/wait/kill APIs to agent API docs.

[Vinod Kone]

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




docs/operator-http-api.md (line 3237)


This API doesn't mandate task groups right?

s/as part of a task group/under an existing parent container/.



docs/operator-http-api.md (line 3306)


can you use a different uuid than the parent's?



docs/operator-http-api.md (line 3348)


can you use a different uuid than the parent's?


- Vinod Kone


On Feb. 28, 2017, 1:35 a.m., Kevin Klues wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/57117/
> ---
> 
> (Updated Feb. 28, 2017, 1:35 a.m.)
> 
> 
> Review request for mesos, Adam B, Benjamin Mahler, and Vinod Kone.
> 
> 
> Bugs: MESOS-7189
> https://issues.apache.org/jira/browse/MESOS-7189
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added nested container launch/wait/kill APIs to agent API docs.
> 
> 
> Diffs
> -
> 
>   docs/operator-http-api.md 94a5477af60de0f9e8e79366fe93adb5299e470c 
> 
> Diff: https://reviews.apache.org/r/57117/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Kevin Klues
> 
>

Review Request 57132: Updated WebUI to display roles.

[Jay Guo]

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

Review request for mesos and Benjamin Mahler.


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


Repository: mesos


Description
---

With support of multi-tenancy in Mesos, WebUI needs to be updated
to correctly display role information.


Diffs
-

  src/webui/master/static/agent.html 7a4599367ea8ff802591adfd86d4058cdc51f297 
  src/webui/master/static/agent_executor.html 
95e921d1c3ca0f005e3eb3f79eb0d9a70a3b3f91 
  src/webui/master/static/agent_framework.html 
806ab06063dcf62cd6d1d7913840ed83141bd1eb 
  src/webui/master/static/framework.html 
37e0b31ac90e877b70f315143b7411fadec6 
  src/webui/master/static/frameworks.html 
0c6fc1c37e0ab49fed64aaa861dfcbaab7f38e0e 
  src/webui/master/static/js/controllers.js 
2ea8275cb5fffa2344474ebfcf9277fe92165a92 
  src/webui/master/static/offers.html 181fc2deabef3faec36785e7fcdbc37465668bec 

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


Testing
---

manually test by launching new master with both new/old agent, and inspect 
WebUI to see roles.


Thanks,

Jay Guo

Re: Review Request 56208: Updated checks library with general check support.

[Alexander Rukletsov]

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

(Updated Feb. 28, 2017, 3:50 p.m.)


Review request for mesos, Gastón Kleiman and Vinod Kone.


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


Repository: mesos


Description
---

Add support for general checks, i.e. defined by CheckInfo, in
checking library. A general check can be either an command or
an HTTP request. The library performs the requested check at
the specified interval and sends the result to the framework
via a task status update. If the current result is the same as
the previous result, no status update is sent.


Diffs
-

  src/checks/checker.hpp dc293f3d3613dec716510d269829f8a6f406c277 
  src/checks/checker.cpp 8716e4cc684e6c4b6b76d8ca53221be06d10b2a6 
  src/checks/health_checker.hpp f1f2834b3429fb00cc49c179fa9a3de328f597b5 
  src/checks/health_checker.cpp 6c97369fd9a567ba16dd92085bf142d43f71eaf1 

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


Testing (updated)
---

https://reviews.apache.org/r/56213/


Thanks,

Alexander Rukletsov

Review Request 57150: Simplified task id procurement in command executor.

[Alexander Rukletsov]

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

Review request for mesos, Gastón Kleiman and Vinod Kone.


Repository: mesos


Description
---

See summary.


Diffs
-

  src/launcher/executor.cpp adcc50fb7df91b9f8dff0f583b61e0efcd6da378 

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


Testing
---

See https://reviews.apache.org/r/56213/


Thanks,

Alexander Rukletsov

Re: Review Request 56213: Added check tests for command executor.

[Alexander Rukletsov]

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

(Updated Feb. 28, 2017, 3:55 p.m.)


Review request for mesos, Gastón Kleiman and Vinod Kone.


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


Repository: mesos


Description
---

See summary.


Diffs
-

  src/tests/check_tests.cpp b5a5d8e80c80b480992a3c8160ee7d0e4443111c 
  src/tests/mesos.hpp b450a04dfbf3bbeeb6b605fb78097dca390cbdbe 

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


Testing
---

See https://reviews.apache.org/r/56218/


Thanks,

Alexander Rukletsov

Re: Review Request 56212: Added support for general checks to command executor.

[Alexander Rukletsov]

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

(Updated Feb. 28, 2017, 3:54 p.m.)


Review request for mesos, Gastón Kleiman and Vinod Kone.


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


Repository: mesos


Description
---

See summary.


Diffs (updated)
-

  src/launcher/executor.cpp adcc50fb7df91b9f8dff0f583b61e0efcd6da378 

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


Testing (updated)
---

See https://reviews.apache.org/r/56213/


Thanks,

Alexander Rukletsov

Re: Review Request 56211: Renamed health checker in command executor for clarity.

[Alexander Rukletsov]

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

(Updated Feb. 28, 2017, 3:54 p.m.)


Review request for mesos, Gastón Kleiman and Vinod Kone.


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


Repository: mesos


Description
---

Also validate internal invariants when health is updated.


Diffs
-

  src/launcher/executor.cpp adcc50fb7df91b9f8dff0f583b61e0efcd6da378 

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


Testing (updated)
---

See https://reviews.apache.org/r/56213/


Thanks,

Alexander Rukletsov

Re: Review Request 56211: Renamed health checker in command executor for clarity.

[Alexander Rukletsov]

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

(Updated Feb. 28, 2017, 3:54 p.m.)


Review request for mesos, Gastón Kleiman and Vinod Kone.


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


Repository: mesos


Description (updated)
---

Also validate internal invariants when health is updated.


Diffs (updated)
-

  src/launcher/executor.cpp adcc50fb7df91b9f8dff0f583b61e0efcd6da378 

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


Testing
---

See https://reviews.apache.org/r/56218/


Thanks,

Alexander Rukletsov

Re: Review Request 56210: Reused previous task status for health updates in command executor.

[Alexander Rukletsov]

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

(Updated Feb. 28, 2017, 3:52 p.m.)


Review request for mesos, Gastón Kleiman and Vinod Kone.


Summary (updated)
-

Reused previous task status for health updates in command executor.


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


Repository: mesos


Description (updated)
---

When a new health task status update is generated in the executor, we
have to make sure specific data is duplicated from the previous one to
avoid shadowing of those data during reconciliation.


Diffs (updated)
-

  src/launcher/executor.cpp adcc50fb7df91b9f8dff0f583b61e0efcd6da378 

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


Testing (updated)
---

See https://reviews.apache.org/r/56213/


Thanks,

Alexander Rukletsov

Review Request 57149: Added a warning if command executor gets an unknown acknowledgement.

[Alexander Rukletsov]

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

Review request for mesos, Gastón Kleiman and Vinod Kone.


Repository: mesos


Description
---

See summary.


Diffs
-

  src/launcher/executor.cpp adcc50fb7df91b9f8dff0f583b61e0efcd6da378 

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


Testing
---

https://reviews.apache.org/r/56213/


Thanks,

Alexander Rukletsov

Re: Review Request 56208: Updated checks library with general check support.

[Alexander Rukletsov]

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

(Updated Feb. 28, 2017, 3:49 p.m.)


Review request for mesos, Gastón Kleiman and Vinod Kone.


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


Repository: mesos


Description
---

Add support for general checks, i.e. defined by CheckInfo, in
checking library. A general check can be either an command or
an HTTP request. The library performs the requested check at
the specified interval and sends the result to the framework
via a task status update. If the current result is the same as
the previous result, no status update is sent.


Diffs (updated)
-

  src/checks/checker.hpp dc293f3d3613dec716510d269829f8a6f406c277 
  src/checks/checker.cpp 8716e4cc684e6c4b6b76d8ca53221be06d10b2a6 
  src/checks/health_checker.hpp f1f2834b3429fb00cc49c179fa9a3de328f597b5 
  src/checks/health_checker.cpp 6c97369fd9a567ba16dd92085bf142d43f71eaf1 

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


Testing
---

See https://reviews.apache.org/r/56218/


Thanks,

Alexander Rukletsov

Re: Review Request 56017: Added a helper for building a task status from an existing one.

[Alexander Rukletsov]

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

(Updated Feb. 28, 2017, 3:48 p.m.)


Review request for mesos, Gastón Kleiman and Vinod Kone.


Summary (updated)
-

Added a helper for building a task status from an existing one.


Repository: mesos


Description
---

See summary.


Diffs (updated)
-

  src/common/protobuf_utils.hpp 09e468c77f0cdd931302d1bdcc192370b6ce3340 
  src/common/protobuf_utils.cpp 944263bbaa87a65005fd924bccfadb7293312fa0 

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


Testing
---

make check


Thanks,

Alexander Rukletsov

Re: Review Request 56208: Updated checks library with general check support.

[Alexander Rukletsov]

> On Feb. 14, 2017, 5:46 p.m., Gastón Kleiman wrote:
> > src/checks/checker.cpp, line 219
> > 
> >
> > I think that it'd be useful to log the `taskId` here, since an executor 
> > might have multiple checkers running at the same time.
> 
> Alexander Rukletsov wrote:
> To do this we have to cache `taskId` in the class. Is it worth it?
> 
> Gastón Kleiman wrote:
> As per offline discussion, I think that we could move this logging 
> statement to `CheckerProcess::finalize()`.
> 
> I see the following benefits:
> 
>  1. It makes the debugging output friendlier =).
>  2. `CheckerProcess::initialize()` already logs the check configuration, 
> so logging something on termination would be symmetrical.
>  3. It makes `Checker` a plain and simple wrapper, except maybe for the 
> extra validation in `Checker::create()`.

I'm convinced!


- Alexander


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


On Feb. 28, 2017, 11:12 a.m., Alexander Rukletsov wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/56208/
> ---
> 
> (Updated Feb. 28, 2017, 11:12 a.m.)
> 
> 
> Review request for mesos, Gastón Kleiman and Vinod Kone.
> 
> 
> Bugs: MESOS-6906
> https://issues.apache.org/jira/browse/MESOS-6906
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Add support for general checks, i.e. defined by CheckInfo, in
> checking library. A general check can be either an command or
> an HTTP request. The library performs the requested check at
> the specified interval and sends the result to the framework
> via a task status update. If the current result is the same as
> the previous result, no status update is sent.
> 
> 
> Diffs
> -
> 
>   src/checks/checker.hpp dc293f3d3613dec716510d269829f8a6f406c277 
>   src/checks/checker.cpp 8716e4cc684e6c4b6b76d8ca53221be06d10b2a6 
>   src/checks/health_checker.hpp f1f2834b3429fb00cc49c179fa9a3de328f597b5 
>   src/checks/health_checker.cpp a5225ff1f4b071ed4182d41fa8ecc705fa4dbe00 
> 
> Diff: https://reviews.apache.org/r/56208/diff/
> 
> 
> Testing
> ---
> 
> See https://reviews.apache.org/r/56218/
> 
> 
> Thanks,
> 
> Alexander Rukletsov
> 
>

Re: Review Request 56212: Added support for general checks to command executor.

[Alexander Rukletsov]

> On Feb. 14, 2017, 6:59 p.m., Gastón Kleiman wrote:
> > src/launcher/executor.cpp, lines 848-854
> > 
> >
> > This method is called to build updates when the task transitions to 
> > `TASK_KILLING`, `TASK_FINISHED`, `TASK_KILLED`, and `TASK_FAILED`.
> > 
> > Do we want to lose the result of the previous check in all those cases? 
> > I think it might make sense to keep it at least for `TASK_KILLING`. It 
> > would also make sense to keep the `health` flag in that case.

I'm not sure what is the best thing to do here. Does it make sense to talk 
about check status when, e.g., the task is being killed?

Moreover, for health checks, keeping health info for terminal states means 
changing current behaviour.


- Alexander


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


On Feb. 8, 2017, 4:56 p.m., Alexander Rukletsov wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/56212/
> ---
> 
> (Updated Feb. 8, 2017, 4:56 p.m.)
> 
> 
> Review request for mesos, Gastón Kleiman and Vinod Kone.
> 
> 
> Bugs: MESOS-6906
> https://issues.apache.org/jira/browse/MESOS-6906
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> See summary.
> 
> 
> Diffs
> -
> 
>   src/launcher/executor.cpp d9417ce1d5b108f5292a66010eab80f11552a969 
> 
> Diff: https://reviews.apache.org/r/56212/diff/
> 
> 
> Testing
> ---
> 
> See https://reviews.apache.org/r/56218/
> 
> 
> Thanks,
> 
> Alexander Rukletsov
> 
>

Re: Review Request 56611: Relax perf version check for Arch Linux.

[Mesos Reviewbot]

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



Patch looks great!

Reviews applied: [56611]

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

- Mesos Reviewbot


On Feb. 28, 2017, 5:43 a.m., James Peach wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/56611/
> ---
> 
> (Updated Feb. 28, 2017, 5:43 a.m.)
> 
> 
> Review request for mesos and Neil Conway.
> 
> 
> Bugs: MESOS-6982
> https://issues.apache.org/jira/browse/MESOS-6982
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Relax perf version check for Arch Linux.
> 
> 
> Diffs
> -
> 
>   src/linux/perf.hpp 9c4330b6086abb18f03660fe89a6fb01d8ed 
>   src/linux/perf.cpp 3141e5ee9eee78e974625791f362dc345c682ebb 
>   src/tests/containerizer/perf_tests.cpp 
> d536ecc5cb24787bc3487efb146573cd4f3ded43 
> 
> Diff: https://reviews.apache.org/r/56611/diff/
> 
> 
> Testing
> ---
> 
> make check (Fedora 25)
> 
> 
> Thanks,
> 
> James Peach
> 
>

Re: Review Request 56448: Shortened continuation name in default executor.

[Alexander Rukletsov]

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

(Updated Feb. 28, 2017, 2:36 p.m.)


Review request for mesos, Anand Mazumdar, Gastón Kleiman, and Vinod Kone.


Repository: mesos


Description
---

See summary.


Diffs
-

  src/launcher/default_executor.cpp e63cf153831088851863d0956455a024e9bc172a 

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


Testing
---

See https://reviews.apache.org/r/56218/


Thanks,

Alexander Rukletsov

Re: Review Request 56449: Moved health checker closer to container in default executor.

[Alexander Rukletsov]

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

(Updated Feb. 28, 2017, 2:35 p.m.)


Review request for mesos, Anand Mazumdar, Gastón Kleiman, and Vinod Kone.


Repository: mesos


Description
---

With the recent introduction of the `Container` struct in the default
executor, tasks' health checkers should be moved to this struct. Also,
health checking is stopped on per-task basis and not on shutdown.


Diffs
-

  src/launcher/default_executor.cpp e63cf153831088851863d0956455a024e9bc172a 

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


Testing
---

See https://reviews.apache.org/r/56218/


Thanks,

Alexander Rukletsov

Re: Review Request 56621: Updated Mesos tests to use the 'Principal' type.

[Alexander Rojas]

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


Ship it!




Ship It!

- Alexander Rojas


On Feb. 28, 2017, 8:07 a.m., Greg Mann wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/56621/
> ---
> 
> (Updated Feb. 28, 2017, 8:07 a.m.)
> 
> 
> Review request for mesos, Adam B, Alexander Rojas, Jan Schlicht, Till 
> Toenshoff, and Vinod Kone.
> 
> 
> Bugs: MESOS-7003
> https://issues.apache.org/jira/browse/MESOS-7003
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This patch updates the Mesos tests to use authenticated
> handlers which accept the `Principal` type instead of an
> `Option principal`.
> 
> 
> Diffs
> -
> 
>   src/tests/files_tests.cpp 6c6353e406249f021803e83909415e9908ded28c 
>   src/tests/http_authentication_tests.cpp 
> 95f01c432f77d1a944dd78c343e21b310676417f 
>   src/tests/master_validation_tests.cpp 
> 45254739e2a62fed7008ceada7a8f64bb9ee4e31 
> 
> Diff: https://reviews.apache.org/r/56621/diff/
> 
> 
> Testing
> ---
> 
> `make check`
> 
> 
> Thanks,
> 
> Greg Mann
> 
>

Re: Review Request 56753: Implemented the JWT authenticator.

[Jan Schlicht]

> On Feb. 25, 2017, 3:26 a.m., Greg Mann wrote:
> > 3rdparty/libprocess/src/jwt_authenticator.cpp, lines 81-102
> > 
> >
> > I was just discussing this claim with Vinod; let's leave it out of our 
> > implementation. We'd like to move toward a world where the new 
> > `AuthenticationContext` is first-class throughout the authN/authZ code, so 
> > that, for example, a scheduler could subscribe using a credential that 
> > resolves to an `AuthenticationContext` which contains claims, but no 
> > principal. In such a world, we would simply use the "sub" field as-is 
> > within the claims, with no need to translate it into the `principal` member.

Agreed. Though this has some consequences for the `SecretGenerator`, because it 
shouldn't map a `Principal::value` to a claim. Not sure yet, but if called with 
a `Principal` that has its `value` set, it should fail. But probably even 
better would be to change `SecretGenerator::generate` to accept a `map` of 
claims instead of `Principal`.


- Jan


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


On Feb. 28, 2017, 2:36 p.m., Jan Schlicht wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/56753/
> ---
> 
> (Updated Feb. 28, 2017, 2:36 p.m.)
> 
> 
> Review request for mesos, Alexander Rojas and Greg Mann.
> 
> 
> Bugs: MESOS-7001
> https://issues.apache.org/jira/browse/MESOS-7001
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This HTTP authenticator extracts a JWT from the requests' authorization
> header using the 'Bearer' schema and validates it against a secret using
> HMAC SHA256. The 'sub' claim of the JWT is the extracted principal, all
> other claims will be additional labels of the 'AuthenticationContext'.
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/Makefile.am 75386184108214e67a58c328258ec204099d638c 
>   3rdparty/libprocess/include/process/authenticator.hpp 
> e5489c6cb4adc8a822e7dd4515542618c36136f9 
>   3rdparty/libprocess/src/authenticator.cpp 
> cfedb6f7674e0f6690e77a633cdd1bd494c7d2c7 
>   3rdparty/libprocess/src/jwt_authenticator.cpp PRE-CREATION 
>   3rdparty/libprocess/src/tests/http_tests.cpp 
> fb4da9aecff0370d97a15269c5d8fffb30e0478f 
> 
> Diff: https://reviews.apache.org/r/56753/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Jan Schlicht
> 
>

Re: Review Request 56753: Implemented the JWT authenticator.

[Jan Schlicht]

> On Feb. 22, 2017, 8:58 p.m., Greg Mann wrote:
> > 3rdparty/libprocess/src/jwt_authenticator.cpp, line 97
> > 
> >
> > Is it possible to use `JSON::String` here directly?

Not possible. Also needed to support multiple types, see the next issue. Hope 
it's okay that I'm dropping this.


> On Feb. 22, 2017, 8:58 p.m., Greg Mann wrote:
> > 3rdparty/libprocess/src/jwt_authenticator.cpp, lines 99-101
> > 
> >
> > Are you sure this won't end up silently ignoring fields which have 
> > numerical values? If the field's value is a JSON number, we should probably 
> > convert it to a string and place it in the map. Similarly with other JSON 
> > types; could we stringify a JSON object and place it into the map as well?

Agreed, we shouldn't ignore any field. I've changed it to call `stringify` an 
any value. All JSON types have implementations for this and `jsonify` 
themselves. Handling of `JSON::String` is a bit different though, because 
`stringify` would return a quoted string. RFC 7519 states that a claim can be 
any JSON object, IMO we're good by return either a string, a stringified number 
or a JSON string of more complicated values.


> On Feb. 22, 2017, 8:58 p.m., Greg Mann wrote:
> > 3rdparty/libprocess/src/jwt_authenticator.cpp, lines 77-89
> > 
> >
> > In these error cases, we could provide additional error information in 
> > the WWW-Authenticate header. See 
> > https://tools.ietf.org/html/rfc6750#section-3
> > 
> > We can use "error=invalid_token, error_description=ERROR_MESSAGE", with 
> > appropriate messages for each case. Perhaps it makes sense to construct and 
> > return the Unauthorized response within each conditional so that we can set 
> > the header appropriately at construction?

Thanks for the hint, wasn't aware of that. Added error information where it was 
appropriate.


- Jan


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


On Feb. 28, 2017, 2:36 p.m., Jan Schlicht wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/56753/
> ---
> 
> (Updated Feb. 28, 2017, 2:36 p.m.)
> 
> 
> Review request for mesos, Alexander Rojas and Greg Mann.
> 
> 
> Bugs: MESOS-7001
> https://issues.apache.org/jira/browse/MESOS-7001
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This HTTP authenticator extracts a JWT from the requests' authorization
> header using the 'Bearer' schema and validates it against a secret using
> HMAC SHA256. The 'sub' claim of the JWT is the extracted principal, all
> other claims will be additional labels of the 'AuthenticationContext'.
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/Makefile.am 75386184108214e67a58c328258ec204099d638c 
>   3rdparty/libprocess/include/process/authenticator.hpp 
> e5489c6cb4adc8a822e7dd4515542618c36136f9 
>   3rdparty/libprocess/src/authenticator.cpp 
> cfedb6f7674e0f6690e77a633cdd1bd494c7d2c7 
>   3rdparty/libprocess/src/jwt_authenticator.cpp PRE-CREATION 
>   3rdparty/libprocess/src/tests/http_tests.cpp 
> fb4da9aecff0370d97a15269c5d8fffb30e0478f 
> 
> Diff: https://reviews.apache.org/r/56753/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Jan Schlicht
> 
>

Re: Review Request 56210: Reused previous task status to generate a new one in command executor.

[Alexander Rukletsov]

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

(Updated Feb. 28, 2017, 2:01 p.m.)


Review request for mesos, Gastón Kleiman and Vinod Kone.


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


Repository: mesos


Description
---

When a new task status update is generated in the executor, we have
to make sure specific data is duplicated from the previous update
to, e.g., avoid shadowing of those data during reconciliation. For
instance, consider a check status being sent; in this status update
we must include the latest known health information.


Diffs
-

  src/launcher/executor.cpp d9417ce1d5b108f5292a66010eab80f11552a969 

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


Testing
---

See https://reviews.apache.org/r/56218/


Thanks,

Alexander Rukletsov

Re: Review Request 56209: Hashed unacknowledged updates by UUID string in command executor.

[Alexander Rukletsov]

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

(Updated Feb. 28, 2017, 2 p.m.)


Review request for mesos, Gastón Kleiman and Vinod Kone.


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


Repository: mesos


Description
---

Update acknowledgements contain UUID as string, a conversion is
anyway necessary. In the future we may not have access to the
original UUID, while string representation is always available in
the protobuf message itself. Hence it seems reasonable to hash
updates by string instead of UUID.


Diffs
-

  src/launcher/executor.cpp d9417ce1d5b108f5292a66010eab80f11552a969 

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


Testing
---

See https://reviews.apache.org/r/56218/


Thanks,

Alexander Rukletsov

Re: Review Request 56208: Updated checks library with general check support.

[Gastón Kleiman]

> On Feb. 14, 2017, 5:46 p.m., Gastón Kleiman wrote:
> > src/checks/checker.cpp, line 219
> > 
> >
> > I think that it'd be useful to log the `taskId` here, since an executor 
> > might have multiple checkers running at the same time.
> 
> Alexander Rukletsov wrote:
> To do this we have to cache `taskId` in the class. Is it worth it?

As per offline discussion, I think that we could move this logging statement to 
`CheckerProcess::finalize()`.

I see the following benefits:

 1. It makes the debugging output friendlier =).
 2. `CheckerProcess::initialize()` already logs the check configuration, so 
logging something on termination would be symmetrical.
 3. It makes `Checker` a plain and simple wrapper, except maybe for the extra 
validation in `Checker::create()`.


- Gastón


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


On Feb. 28, 2017, 11:12 a.m., Alexander Rukletsov wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/56208/
> ---
> 
> (Updated Feb. 28, 2017, 11:12 a.m.)
> 
> 
> Review request for mesos, Gastón Kleiman and Vinod Kone.
> 
> 
> Bugs: MESOS-6906
> https://issues.apache.org/jira/browse/MESOS-6906
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Add support for general checks, i.e. defined by CheckInfo, in
> checking library. A general check can be either an command or
> an HTTP request. The library performs the requested check at
> the specified interval and sends the result to the framework
> via a task status update. If the current result is the same as
> the previous result, no status update is sent.
> 
> 
> Diffs
> -
> 
>   src/checks/checker.hpp dc293f3d3613dec716510d269829f8a6f406c277 
>   src/checks/checker.cpp 8716e4cc684e6c4b6b76d8ca53221be06d10b2a6 
>   src/checks/health_checker.hpp f1f2834b3429fb00cc49c179fa9a3de328f597b5 
>   src/checks/health_checker.cpp a5225ff1f4b071ed4182d41fa8ecc705fa4dbe00 
> 
> Diff: https://reviews.apache.org/r/56208/diff/
> 
> 
> Testing
> ---
> 
> See https://reviews.apache.org/r/56218/
> 
> 
> Thanks,
> 
> Alexander Rukletsov
> 
>

Re: Review Request 56624: Updated libprocess tests to use the 'Principal' type.

[Alexander Rojas]

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


Fix it, then Ship it!





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


`/Option::none()/None()/`

Not so sure if it works in macros, but I did test changing it in the 
previous commit and compiling it.


- Alexander Rojas


On Feb. 28, 2017, 7:59 a.m., Greg Mann wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/56624/
> ---
> 
> (Updated Feb. 28, 2017, 7:59 a.m.)
> 
> 
> Review request for mesos, Adam B, Alexander Rojas, Jan Schlicht, Till 
> Toenshoff, and Vinod Kone.
> 
> 
> Bugs: MESOS-7003
> https://issues.apache.org/jira/browse/MESOS-7003
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This patch updates the HTTP-related libprocess
> tests to use authenticated handlers which accept
> the `Principal` type instead of an
> `Option principal`.
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/src/tests/http_tests.cpp 
> fb4da9aecff0370d97a15269c5d8fffb30e0478f 
> 
> Diff: https://reviews.apache.org/r/56624/diff/
> 
> 
> Testing
> ---
> 
> Testing information can be found at the end of this review chain.
> 
> 
> Thanks,
> 
> Greg Mann
> 
>

Re: Review Request 56754: Implemented a JWT secret generator.

[Jan Schlicht]

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

(Updated Feb. 28, 2017, 2:39 p.m.)


Review request for mesos, Alexander Rojas and Greg Mann.


Changes
---

Rebased.


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


Repository: mesos


Description
---

This can be used to create a 'Secret' from an 'AuthenticationContext'.
The resulting secret will provide a JWT.


Diffs (updated)
-

  src/Makefile.am 76d58a0bbd77fe59ff8a81ba7a68c84dc7169750 
  src/authentication/executor/jwt_secret_generator.hpp PRE-CREATION 
  src/authentication/executor/jwt_secret_generator.cpp PRE-CREATION 
  src/tests/secret_generator_tests.cpp PRE-CREATION 

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


Testing
---

make check


Thanks,

Jan Schlicht

Re: Review Request 56757: Added the SecretGenerator module interface.

[Jan Schlicht]

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

(Updated Feb. 28, 2017, 2:37 p.m.)


Review request for mesos, Alexander Rojas and Greg Mann.


Changes
---

Rebased.


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


Repository: mesos


Description
---

This interface will be used by agents to create credentials for the
default executor.


Diffs (updated)
-

  include/mesos/authentication/secret_generator.hpp PRE-CREATION 
  include/mesos/module/secret_generator.hpp PRE-CREATION 
  src/Makefile.am 76d58a0bbd77fe59ff8a81ba7a68c84dc7169750 

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


Testing
---

make check


Thanks,

Jan Schlicht

Re: Review Request 56813: Updated master handlers to use 'AuthenticationContext'.

[Alexander Rojas]

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




src/master/http.cpp (line 115)


wasn't metrics process used at all?



src/master/http.cpp (lines 805 - 806)


This can be merge into one line.



src/master/http.cpp (lines 884 - 886)


I'm not sure printing the whole `principal` with name and claims is such a 
good idea. The reason is that you only print `framework->info.principal()`, and 
to my knowledge, only compare the names. Perhaps is a better idea to only print 
`principal->name`?



src/master/http.cpp (lines 1104 - 1106)


This line doesn't need change.



src/master/http.cpp (lines 2203 - 2206)


This line didn't need modification. However it does raises the issue of the 
signature of `validation::operation::validate()`. The common pattern in Mesos 
is to use a default parameter in the declaration, like:

```c++
Option validate(
const Offer::Operation::Reserve& reserve,
const Option& principal,
const Option& frameworkInfo = None())
```

so that the caller doesn't need to do it. Doing a grep on the srouce code 
in over a hundred instances of that pattern.

Could you quickly fix that?



src/master/master.cpp (lines 1070 - 1073)


No need for the break and indent.



src/master/master.cpp (line 3880)


/Option::none()/None()/



src/master/master.cpp (line 3893)


/Option::none()/None()/



src/master/master.cpp (line 3919)


/Option::none()/None()/


- Alexander Rojas


On Feb. 28, 2017, 7:56 a.m., Greg Mann wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/56813/
> ---
> 
> (Updated Feb. 28, 2017, 7:56 a.m.)
> 
> 
> Review request for mesos, Adam B, Alexander Rojas, Jan Schlicht, Till 
> Toenshoff, and Vinod Kone.
> 
> 
> Bugs: MESOS-7003
> https://issues.apache.org/jira/browse/MESOS-7003
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This patch updates the HTTP endpoint handlers in the
> master process to accept the `Principal` type instead
> of an `Option& principal`.
> 
> 
> Diffs
> -
> 
>   src/master/http.cpp 6e5178eeea6cc6b90ae253840da22be13444b088 
>   src/master/master.hpp 764adb18ddf09b62529c5c96f8e4dfaf7803483e 
>   src/master/master.cpp 2ef836536784d5fda23c80dc2304239b176a8942 
>   src/master/quota_handler.cpp 3ad28e4a9363a877d0610b529a6c17fb30ece37a 
>   src/master/registrar.cpp d7134eea34102ab7b24d2f0131363bdd9005cfd3 
>   src/master/weights_handler.cpp 4f6a4cd29d27b64cbc9ee6d8909f79812313b681 
> 
> Diff: https://reviews.apache.org/r/56813/diff/
> 
> 
> Testing
> ---
> 
> Testing details can be found at the end of this review chain.
> 
> 
> Thanks,
> 
> Greg Mann
> 
>

Re: Review Request 56753: Implemented the JWT authenticator.

[Jan Schlicht]

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

(Updated Feb. 28, 2017, 2:36 p.m.)


Review request for mesos, Alexander Rojas and Greg Mann.


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


Repository: mesos


Description
---

This HTTP authenticator extracts a JWT from the requests' authorization
header using the 'Bearer' schema and validates it against a secret using
HMAC SHA256. The 'sub' claim of the JWT is the extracted principal, all
other claims will be additional labels of the 'AuthenticationContext'.


Diffs
-

  3rdparty/libprocess/Makefile.am 75386184108214e67a58c328258ec204099d638c 
  3rdparty/libprocess/include/process/authenticator.hpp 
e5489c6cb4adc8a822e7dd4515542618c36136f9 
  3rdparty/libprocess/src/authenticator.cpp 
cfedb6f7674e0f6690e77a633cdd1bd494c7d2c7 
  3rdparty/libprocess/src/jwt_authenticator.cpp PRE-CREATION 
  3rdparty/libprocess/src/tests/http_tests.cpp 
fb4da9aecff0370d97a15269c5d8fffb30e0478f 

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


Testing
---

make check


Thanks,

Jan Schlicht

Re: Review Request 56753: Implemented the JWT authenticator.

[Jan Schlicht]

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

(Updated Feb. 28, 2017, 2:36 p.m.)


Review request for mesos, Alexander Rojas and Greg Mann.


Changes
---

Rebased and addressed issues.


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


Repository: mesos


Description
---

This HTTP authenticator extracts a JWT from the requests' authorization
header using the 'Bearer' schema and validates it against a secret using
HMAC SHA256. The 'sub' claim of the JWT is the extracted principal, all
other claims will be additional labels of the 'AuthenticationContext'.


Diffs (updated)
-

  3rdparty/libprocess/Makefile.am 75386184108214e67a58c328258ec204099d638c 
  3rdparty/libprocess/include/process/authenticator.hpp 
e5489c6cb4adc8a822e7dd4515542618c36136f9 
  3rdparty/libprocess/src/authenticator.cpp 
cfedb6f7674e0f6690e77a633cdd1bd494c7d2c7 
  3rdparty/libprocess/src/jwt_authenticator.cpp PRE-CREATION 
  3rdparty/libprocess/src/tests/http_tests.cpp 
fb4da9aecff0370d97a15269c5d8fffb30e0478f 

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


Testing
---

make check


Thanks,

Jan Schlicht