Re: Review Request 71533: Cleaned up `HTTPTest.WWWAuthenticateHeader`.

2019-09-27 Thread Benno Evers

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


Ship it!




Ship It!

- Benno Evers


On Sept. 23, 2019, 11:02 a.m., Benjamin Bannier wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/71533/
> ---
> 
> (Updated Sept. 23, 2019, 11:02 a.m.)
> 
> 
> Review request for mesos, Benno Evers, Greg Mann, and Till Toenshoff.
> 
> 
> Bugs: MESOS-9968
> https://issues.apache.org/jira/browse/MESOS-9968
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This patch removes a number of error-prone temporaries previously reused
> in the test.
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/src/tests/http_tests.cpp 
> 8cb5f163f2fc34baeabfb1ef640fe39bf6709872 
> 
> 
> Diff: https://reviews.apache.org/r/71533/diff/1/
> 
> 
> Testing
> ---
> 
> `make check`
> 
> 
> Thanks,
> 
> Benjamin Bannier
> 
>



Re: Review Request 71522: Added missing member initialization in `JSON::WriterProxy`.

2019-09-20 Thread Benno Evers

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


Fix it, then Ship it!





3rdparty/stout/include/stout/jsonify.hpp
Line 658 (original), 658 (patched)
<https://reviews.apache.org/r/71522/#comment305267>

For consistency, I'd suggest either adding this to the constructor of 
`WriterProxy` or adding a member initializer for `writer_` in the line above as 
well.


- Benno Evers


On Sept. 20, 2019, 7:53 a.m., Benjamin Bannier wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/71522/
> ---
> 
> (Updated Sept. 20, 2019, 7:53 a.m.)
> 
> 
> Review request for mesos, Benno Evers and Benjamin Mahler.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added missing member initialization in `JSON::WriterProxy`.
> 
> 
> Diffs
> -
> 
>   3rdparty/stout/include/stout/jsonify.hpp 
> e5402b788dd8d539a7889c390a66abb3b36faca9 
> 
> 
> Diff: https://reviews.apache.org/r/71522/diff/1/
> 
> 
> Testing
> ---
> 
> `make check`
> 
> 
> Thanks,
> 
> Benjamin Bannier
> 
>



Re: Review Request 71519: Fixed inefficient `hashmap` access patterns.

2019-09-20 Thread Benno Evers

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


Ship it!




The review looks good, but this sentence from the commit message seems to be a 
bit mangled:

>  It was never not intended and is inefficient
as contains itself (e.g., via hashmap::get::isSome), and for cases
where only access to parts of the value in the hashmap is required
(e.g., to access a member of an optional value).

- Benno Evers


On Sept. 19, 2019, 1:44 p.m., Benjamin Bannier wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/71519/
> ---
> 
> (Updated Sept. 19, 2019, 1:44 p.m.)
> 
> 
> Review request for mesos, Benno Evers and Benjamin Mahler.
> 
> 
> Bugs: MESOS-9948
> https://issues.apache.org/jira/browse/MESOS-9948
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This patch fixes some inefficient access patterns around `hashmap::get`.
> Since this function returns an `Option` it can be used as a shorthand
> for a `contains` check and subsequent creation of a value (`Option`
> always contains a value). It was never not intended and is inefficient
> as `contains` itself (e.g., via `hashmap::get::isSome`), and for cases
> where only access to parts of the value in the `hashmap` is required
> (e.g., to access a member of an optional value). In both these cases we
> neither want nor need to create a temporary, and should instead either
> just use `contains`, or access the value with `hashmap::at` after a
> `contains` check; otherwise we might spend a lot of time creating
> unnecessary temporary values.
> 
> This patch fixes some easily identifiable cases found from skimming the
> result of the following clang-query command:
> 
> match cxxMemberCallExpr(
>   on(hasType(cxxRecordDecl(hasName("hashmap",
>   unless(
> hasParent(cxxBindTemporaryExpr(
>   hasParent(materializeTemporaryExpr(
> hasParent(exprWithCleanups(
>   hasParent(varDecl(),
>   callee(cxxMethodDecl(hasName("get"
> 
> This most probably misses a lot of cases. Given how easy it is to misuse
> `hashmap::get` we should consider whether it makes sense to get rid of
> it completely in lieu of an inlined form trading some additional lookups
> for temporary avoidance,
> 
> Option x = map.contains(k) ? Some(map.at(k)) : Option::none();
> 
> 
> Diffs
> -
> 
>   src/exec/exec.cpp 67e082e709ef803f49646da5c36147158330f725 
>   src/executor/executor.cpp 664a2f1e0723f1afd9220e86e47e86cda67f6b9a 
>   src/files/files.cpp f200d5e797084a2a5137ac8f8ede1f6243f12cfb 
>   src/master/master.cpp a2c289a8b6e30eac690f77c7fc1b12b16f62f541 
>   src/master/metrics.cpp 20d7231888e263cb3c1759407a2476291e515d4a 
>   src/slave/containerizer/fetcher.cpp 
> 8ae789a9f260645e574bbe46a108c3b8cab44cc4 
>   src/slave/containerizer/mesos/isolators/posix.hpp 
> 1ff942cfd1f6829bdc3661b9260dd8e53732d023 
>   src/slave/containerizer/mesos/launcher.cpp 
> 413cc621ad49150a6ddaf689bb75b9dc44741563 
>   src/slave/containerizer/mesos/provisioner/provisioner.cpp 
> bf3908d274f8b6f4a57998cbb0a62312b71e3856 
>   src/slave/slave.cpp 96890d37ec024fc33d2403b32576776888bcd9f7 
>   src/state/log.cpp d3bf2ccceff1934785889ddc9507a754cce445fe 
>   src/tests/slave_recovery_tests.cpp d99752ab082f1aca9fb77659378d0bca5a0598eb 
>   src/tests/task_status_update_manager_tests.cpp 
> 0c8b0586a760683ff0ab0f11bf658073087eac12 
> 
> 
> Diff: https://reviews.apache.org/r/71519/diff/2/
> 
> 
> Testing
> ---
> 
> `make check`
> 
> 
> Thanks,
> 
> Benjamin Bannier
> 
>



Re: Review Request 71523: Bumped mesos-tidy onto upstream release_90.

2019-09-20 Thread Benno Evers

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


Ship it!




Ship It!

- Benno Evers


On Sept. 20, 2019, 7:53 a.m., Benjamin Bannier wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/71523/
> ---
> 
> (Updated Sept. 20, 2019, 7:53 a.m.)
> 
> 
> Review request for mesos and Benno Evers.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Bumped mesos-tidy onto upstream release_90.
> 
> 
> Diffs
> -
> 
>   support/clang-tidy 27879a6eb7b43c767c68315c7d21f007da0c9875 
>   support/mesos-tidy/Dockerfile a6ca38a0396fc48ff9f815d52c675eca2eae9ec8 
> 
> 
> Diff: https://reviews.apache.org/r/71523/diff/1/
> 
> 
> Testing
> ---
> 
> `./support/mesos-tidy.sh` with a locally built image
> 
> 
> Thanks,
> 
> Benjamin Bannier
> 
>



Re: Review Request 71497: Introduced new names for SSL-related libprocess flags.

2019-09-19 Thread Benno Evers

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

(Updated Sept. 19, 2019, 2:35 p.m.)


Review request for mesos, Greg Mann and Till Toenshoff.


Changes
---

Revert usage of libprocess DeprecatedName machinery


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


Repository: mesos


Description
---

The `LIBPROCESS_SSL_REQUIRE_CERT` flag was renamed to
`LIBPROCESS_SSL_REQUIRE_CLIENT_CERT`.

The `LIBPROCESS_SSL_VERIFY_CERT` flag was renamed to
`LIBPROCESS_SSL_VERIFY_SERVER_CERT`.

The new names better describe the actual effect of both flags, and
make upgrades easier by allowing operators to only enable verification
on agents that are new enough to contain the updated hostname
validation code paths.


Diffs (updated)
-

  3rdparty/libprocess/include/process/ssl/flags.hpp 
1a0e3820cc8cd1459625f46a54b194133500f11e 
  3rdparty/libprocess/src/openssl.hpp 271cc95238d287c06df36478554502e8b7205b09 
  3rdparty/libprocess/src/openssl.cpp 5854711971c9ebc4d676edc43af5ab5cfd5ea4c6 
  3rdparty/libprocess/src/tests/ssl_tests.cpp 
9d5ab679165a709f7c3740020961ec89a7db4f54 
  docs/ssl.md 90a2eb9800b7d8d9aa9d7b1060a6e5eb9e124b02 
  docs/upgrades.md e630731c332fdd7df788f96644a8084f30b5c621 


Diff: https://reviews.apache.org/r/71497/diff/4/

Changes: https://reviews.apache.org/r/71497/diff/3-4/


Testing
---


Thanks,

Benno Evers



Re: Review Request 71497: Introduced new names for SSL-related libprocess flags.

2019-09-18 Thread Benno Evers


> On Sept. 17, 2019, 5:39 p.m., Vinod Kone wrote:
> > 3rdparty/libprocess/src/openssl.cpp
> > Lines 118 (patched)
> > <https://reviews.apache.org/r/71497/diff/2/?file=2165534#file2165534line118>
> >
> > can you use the `alias` argument in `add()` for these?

Actually, after having implemented the change I'm considering reverting back to 
the previous version.

The `alias()` mechanism is strict about allowing users to specify only one 
version of the command name:

```
E0918 11:04:41.622146 62400 openssl.cpp:454] EXIT with status 1: Failed to load 
flags from environment variables prefixed by LIBPROCESS_SSL_ or SSL_ 
(deprecated): Flag 'verify_server_cert' is already loaded via name 'verify_cert'
```

This makes sense in general, but it can lead to problems when an operator 
manually wants to enable server certificate validation by setting 
`LIBPROCESS_SSL_VERIFY_CERT=true` and a newer agent leaks its own configuration 
`LIBPROCESS_SSL_VERIFY_SERVER_CERT=true` into the process environment, 
accidentally killing the task.


- Benno


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


On Sept. 18, 2019, 12:35 p.m., Benno Evers wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/71497/
> ---
> 
> (Updated Sept. 18, 2019, 12:35 p.m.)
> 
> 
> Review request for mesos, Greg Mann and Till Toenshoff.
> 
> 
> Bugs: MESOS-9972
> https://issues.apache.org/jira/browse/MESOS-9972
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> The `LIBPROCESS_SSL_REQUIRE_CERT` flag was renamed to
> `LIBPROCESS_SSL_REQUIRE_CLIENT_CERT`.
> 
> The `LIBPROCESS_SSL_VERIFY_CERT` flag was renamed to
> `LIBPROCESS_SSL_VERIFY_SERVER_CERT`.
> 
> The new names better describe the actual effect of both flags, and
> make upgrades easier by allowing operators to only enable verification
> on agents that are new enough to contain the updated hostname
> validation code paths.
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/include/process/ssl/flags.hpp 
> 1a0e3820cc8cd1459625f46a54b194133500f11e 
>   3rdparty/libprocess/src/openssl.hpp 
> 271cc95238d287c06df36478554502e8b7205b09 
>   3rdparty/libprocess/src/openssl.cpp 
> 5854711971c9ebc4d676edc43af5ab5cfd5ea4c6 
>   3rdparty/libprocess/src/tests/ssl_tests.cpp 
> 9d5ab679165a709f7c3740020961ec89a7db4f54 
>   docs/ssl.md 90a2eb9800b7d8d9aa9d7b1060a6e5eb9e124b02 
>   docs/upgrades.md e630731c332fdd7df788f96644a8084f30b5c621 
> 
> 
> Diff: https://reviews.apache.org/r/71497/diff/3/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Benno Evers
> 
>



Re: Review Request 71497: Introduced new names for SSL-related libprocess flags.

2019-09-18 Thread Benno Evers


> On Sept. 17, 2019, 5:39 p.m., Vinod Kone wrote:
> > docs/ssl.md
> > Lines 100 (patched)
> > <https://reviews.apache.org/r/71497/diff/2/?file=2165536#file2165536line100>
> >
> > These deprecations need to be documented in CHANGELOG and upgrades.md.

I updated `upgrades.md`, but I think the `CHANGELOG` updates are usually done 
in a separate patch.


- Benno


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


On Sept. 18, 2019, 12:35 p.m., Benno Evers wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/71497/
> ---
> 
> (Updated Sept. 18, 2019, 12:35 p.m.)
> 
> 
> Review request for mesos, Greg Mann and Till Toenshoff.
> 
> 
> Bugs: MESOS-9972
> https://issues.apache.org/jira/browse/MESOS-9972
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> The `LIBPROCESS_SSL_REQUIRE_CERT` flag was renamed to
> `LIBPROCESS_SSL_REQUIRE_CLIENT_CERT`.
> 
> The `LIBPROCESS_SSL_VERIFY_CERT` flag was renamed to
> `LIBPROCESS_SSL_VERIFY_SERVER_CERT`.
> 
> The new names better describe the actual effect of both flags, and
> make upgrades easier by allowing operators to only enable verification
> on agents that are new enough to contain the updated hostname
> validation code paths.
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/include/process/ssl/flags.hpp 
> 1a0e3820cc8cd1459625f46a54b194133500f11e 
>   3rdparty/libprocess/src/openssl.hpp 
> 271cc95238d287c06df36478554502e8b7205b09 
>   3rdparty/libprocess/src/openssl.cpp 
> 5854711971c9ebc4d676edc43af5ab5cfd5ea4c6 
>   3rdparty/libprocess/src/tests/ssl_tests.cpp 
> 9d5ab679165a709f7c3740020961ec89a7db4f54 
>   docs/ssl.md 90a2eb9800b7d8d9aa9d7b1060a6e5eb9e124b02 
>   docs/upgrades.md e630731c332fdd7df788f96644a8084f30b5c621 
> 
> 
> Diff: https://reviews.apache.org/r/71497/diff/3/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Benno Evers
> 
>



Re: Review Request 71497: Introduced new names for SSL-related libprocess flags.

2019-09-18 Thread Benno Evers


> On Sept. 18, 2019, 9:50 a.m., Till Toenshoff wrote:
> > 3rdparty/libprocess/src/openssl.cpp
> > Lines 545-547 (patched)
> > <https://reviews.apache.org/r/71497/diff/1/?file=2165499#file2165499line545>
> >
> > We are stating that there would be a deprecation; does that mean at 
> > some point this flag won't work anymore? If so when?
> > 
> > Our experience with such changes shows that we will very likely never 
> > kill the old name to make sure we stay reliably compatible. The only 
> > realistic option appears to be Mesos 2.0 for a removal of the old flag. 
> > Let's create a blocker ticket for 2.0, add that as a comment referencing 
> > the JIRA and we are golden.

https://issues.apache.org/jira/browse/MESOS-9973


- Benno


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


On Sept. 18, 2019, 12:35 p.m., Benno Evers wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/71497/
> ---
> 
> (Updated Sept. 18, 2019, 12:35 p.m.)
> 
> 
> Review request for mesos, Greg Mann and Till Toenshoff.
> 
> 
> Bugs: MESOS-9972
> https://issues.apache.org/jira/browse/MESOS-9972
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> The `LIBPROCESS_SSL_REQUIRE_CERT` flag was renamed to
> `LIBPROCESS_SSL_REQUIRE_CLIENT_CERT`.
> 
> The `LIBPROCESS_SSL_VERIFY_CERT` flag was renamed to
> `LIBPROCESS_SSL_VERIFY_SERVER_CERT`.
> 
> The new names better describe the actual effect of both flags, and
> make upgrades easier by allowing operators to only enable verification
> on agents that are new enough to contain the updated hostname
> validation code paths.
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/include/process/ssl/flags.hpp 
> 1a0e3820cc8cd1459625f46a54b194133500f11e 
>   3rdparty/libprocess/src/openssl.hpp 
> 271cc95238d287c06df36478554502e8b7205b09 
>   3rdparty/libprocess/src/openssl.cpp 
> 5854711971c9ebc4d676edc43af5ab5cfd5ea4c6 
>   3rdparty/libprocess/src/tests/ssl_tests.cpp 
> 9d5ab679165a709f7c3740020961ec89a7db4f54 
>   docs/ssl.md 90a2eb9800b7d8d9aa9d7b1060a6e5eb9e124b02 
>   docs/upgrades.md e630731c332fdd7df788f96644a8084f30b5c621 
> 
> 
> Diff: https://reviews.apache.org/r/71497/diff/3/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Benno Evers
> 
>



Re: Review Request 71497: Introduced new names for SSL-related libprocess flags.

2019-09-18 Thread Benno Evers

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

(Updated Sept. 18, 2019, 12:35 p.m.)


Review request for mesos, Greg Mann and Till Toenshoff.


Changes
---

Use built-in libprocess alias mechanism.


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


Repository: mesos


Description
---

The `LIBPROCESS_SSL_REQUIRE_CERT` flag was renamed to
`LIBPROCESS_SSL_REQUIRE_CLIENT_CERT`.

The `LIBPROCESS_SSL_VERIFY_CERT` flag was renamed to
`LIBPROCESS_SSL_VERIFY_SERVER_CERT`.

The new names better describe the actual effect of both flags, and
make upgrades easier by allowing operators to only enable verification
on agents that are new enough to contain the updated hostname
validation code paths.


Diffs (updated)
-

  3rdparty/libprocess/include/process/ssl/flags.hpp 
1a0e3820cc8cd1459625f46a54b194133500f11e 
  3rdparty/libprocess/src/openssl.hpp 271cc95238d287c06df36478554502e8b7205b09 
  3rdparty/libprocess/src/openssl.cpp 5854711971c9ebc4d676edc43af5ab5cfd5ea4c6 
  3rdparty/libprocess/src/tests/ssl_tests.cpp 
9d5ab679165a709f7c3740020961ec89a7db4f54 
  docs/ssl.md 90a2eb9800b7d8d9aa9d7b1060a6e5eb9e124b02 
  docs/upgrades.md e630731c332fdd7df788f96644a8084f30b5c621 


Diff: https://reviews.apache.org/r/71497/diff/3/

Changes: https://reviews.apache.org/r/71497/diff/2-3/


Testing
---


Thanks,

Benno Evers



Re: Review Request 71503: Manually created clang-tidy config in mesos-tidy setup.

2019-09-18 Thread Benno Evers

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


Ship it!




Ship It!

- Benno Evers


On Sept. 18, 2019, 12:09 p.m., Benjamin Bannier wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/71503/
> ---
> 
> (Updated Sept. 18, 2019, 12:09 p.m.)
> 
> 
> Review request for mesos and Benno Evers.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Manually created clang-tidy config in mesos-tidy setup.
> 
> 
> Diffs
> -
> 
>   support/mesos-tidy/entrypoint.sh a8408678ec7dfda398a117788d5f5f955e304bc4 
> 
> 
> Diff: https://reviews.apache.org/r/71503/diff/1/
> 
> 
> Testing
> ---
> 
> Ran `support/mesos-tidy.sh` with local docker image.
> 
> 
> Thanks,
> 
> Benjamin Bannier
> 
>



Review Request 71497: Introduced new names for SSL-related libprocess flags.

2019-09-17 Thread Benno Evers

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

Review request for mesos, Greg Mann and Till Toenshoff.


Repository: mesos


Description
---

The `LIBPROCESS_SSL_REQUIRE_CERT` flag was renamed to
`LIBPROCESS_SSL_REQUIRE_CLIENT_CERT`.

The `LIBPROCESS_SSL_VERIFY_CERT` flag was renamed to
`LIBPROCESS_SSL_VERIFY_SERVER_CERT`.

The new names better describe the actual effect of both flags, and
make upgrades easier by allowing operators to only enable verification
on agents that are new enough to contain the updated hostname
validation code paths.


Diffs
-

  3rdparty/libprocess/include/process/ssl/flags.hpp 
1a0e3820cc8cd1459625f46a54b194133500f11e 
  3rdparty/libprocess/src/openssl.hpp 271cc95238d287c06df36478554502e8b7205b09 
  3rdparty/libprocess/src/openssl.cpp 5854711971c9ebc4d676edc43af5ab5cfd5ea4c6 
  3rdparty/libprocess/src/tests/ssl_tests.cpp 
9d5ab679165a709f7c3740020961ec89a7db4f54 
  docs/ssl.md 90a2eb9800b7d8d9aa9d7b1060a6e5eb9e124b02 


Diff: https://reviews.apache.org/r/71497/diff/1/


Testing
---


Thanks,

Benno Evers



Review Request 71496: Removed an outdated reference to the 'libprocess' hostname validation.

2019-09-17 Thread Benno Evers

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

Review request for mesos and Till Toenshoff.


Repository: mesos


Description
---

Removed a reference to the 'libprocess' hostname validation scheme,
which was removed to 'legacy' during development.


Diffs
-

  3rdparty/libprocess/src/openssl.cpp 5854711971c9ebc4d676edc43af5ab5cfd5ea4c6 


Diff: https://reviews.apache.org/r/71496/diff/1/


Testing
---


Thanks,

Benno Evers



Re: Review Request 71457: Activated tests for `src/python/lib`.

2019-09-09 Thread Benno Evers

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


Fix it, then Ship it!





configure.ac
Lines 2505 (patched)
<https://reviews.apache.org/r/71457/#comment304942>

I think you have to quote `"x$tox"` to guard against whitespace inside the 
variable.



src/Makefile.am
Lines 2942 (patched)
<https://reviews.apache.org/r/71457/#comment304944>

Is it intentional that the autotools version specifies the directory but 
the cmake version specifices the location of `tox.ini`?


- Benno Evers


On Sept. 9, 2019, 4:34 p.m., Benjamin Bannier wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/71457/
> ---
> 
> (Updated Sept. 9, 2019, 4:34 p.m.)
> 
> 
> Review request for mesos, Armand Grillet, Benno Evers, and Kevin Klues.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This directory already had a tox config, but this setup was never
> excercised automatically. This patch adds a test running `tox` for that
> directory. We require `tox` to be present in the system and add
> configure-time checks for that.
> 
> 
> Diffs
> -
> 
>   cmake/CompilationConfigure.cmake c8da0895d7e932b51b711be0bf28e01548b94bf6 
>   configure.ac 1814bc67fd7c683285ed44d20273de02e7a6f251 
>   src/CMakeLists.txt 0ec4442245f09afd6a8281f58a4a4d29c2216fe7 
>   src/Makefile.am 5daee632d52e882acc15b90f655a53dab23eaaf6 
>   src/python/lib/CMakeLists.txt PRE-CREATION 
> 
> 
> Diff: https://reviews.apache.org/r/71457/diff/3/
> 
> 
> Testing
> ---
> 
> `make check` in both autotools & cmake build
> 
> 
> Thanks,
> 
> Benjamin Bannier
> 
>



Re: Review Request 69411: Added new interface for constructing `cluster::Master`.

2019-09-09 Thread Benno Evers

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

(Updated Sept. 9, 2019, 1:56 p.m.)


Review request for mesos, Andrei Budnik, Benjamin Bannier, Benjamin Mahler, 
James Peach, and Joseph Wu.


Changes
---

Update review for parity with `SlaveOptions` refactoring.


Repository: mesos


Description (updated)
---

This review adds a new `struct MasterOptions` and a new overload
`StartMaster(const MasterOptions&)` analogous to the refactoring
done for `SlaveOptions`.

This deprecates the other, existing overloads for `StartMaster()`.


Diffs (updated)
-

  src/tests/mesos.hpp ecde51828cb74b34d8ee338259e3ece3944a17f1 
  src/tests/mesos.cpp e77db2231cd81a272c2b7143a48ee193e63e54ab 


Diff: https://reviews.apache.org/r/69411/diff/3/

Changes: https://reviews.apache.org/r/69411/diff/2-3/


Testing
---


Thanks,

Benno Evers



Re: Review Request 71424: Added `SlaveOptions` for wrapping all parameters of `StartSlave`.

2019-09-05 Thread Benno Evers

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


Ship it!




Looks great.

A few more things to think about would be renaming `mock` to something more 
immediately understandable (or even split this in a separate function, since 
`bool` parameters are usually a bit of a code smell), however there's no reason 
that this would need to be done as part of this review.

- Benno Evers


On Sept. 5, 2019, 2 p.m., Andrei Budnik wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/71424/
> ---
> 
> (Updated Sept. 5, 2019, 2 p.m.)
> 
> 
> Review request for mesos, Benjamin Bannier, Benno Evers, Benjamin Mahler, 
> Gilbert Song, Greg Mann, James Peach, Joseph Wu, Meng Zhu, Qian Zhang, and 
> Till Toenshoff.
> 
> 
> Bugs: MESOS-9914
> https://issues.apache.org/jira/browse/MESOS-9914
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This patch introduces a `SlaveOptions` struct which holds optional
> parameters accepted by `cluster::Slave::create`. Added an overload
> of `StartSlave` that accepts `SlaveOptions`. It's a preferred way of
> creating and starting an instance of `cluster::Slave` in tests, since
> underlying `cluster::Slave::create` accepts a long list of optional
> arguments, which might be extended in the future.
> 
> 
> Diffs
> -
> 
>   src/tests/mesos.hpp 25359a24955fbe6413b3354aabf444818b58cb76 
>   src/tests/mesos.cpp 0396ce78b13e76cfe2596f0f92ddf9d82fca 
> 
> 
> Diff: https://reviews.apache.org/r/71424/diff/2/
> 
> 
> Testing
> ---
> 
> `make check`
> 
> 
> Thanks,
> 
> Andrei Budnik
> 
>



Re: Review Request 71241: Added a `distcheck` target to the cmake build.

2019-09-03 Thread Benno Evers

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




cmake/distcheck.sh
Lines 22 (patched)
<https://reviews.apache.org/r/71241/#comment304796>

Probably makes no difference, but `mesos-${VERSION}` looks a bit safer.



cmake/distcheck.sh
Lines 25 (patched)
<https://reviews.apache.org/r/71241/#comment304795>

I don't think we should unconditionally override the users environment like 
this in a support script - there's no guarantee that `clang` or `ccache` are 
even installed.

If we have to specify `CMAKE_CXX_COMPILER` etc. at all, we should probably 
use `${CXX}` as default value and make sure that cmake sets the correct values 
when invoking the script.


- Benno Evers


On Aug. 6, 2019, 7:28 p.m., Benjamin Bannier wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/71241/
> ---
> 
> (Updated Aug. 6, 2019, 7:28 p.m.)
> 
> 
> Review request for mesos, Benjamin Mahler and Till Toenshoff.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This patch adds a `distcheck` target to the cmake build which mimics the
> target of the same name provided by the autotools build. `distcheck`
> depends on the `dist` target to create a distribution archive and then
> ensures that with the distributed sources the `check` targets succeeds.
> 
> 
> Diffs
> -
> 
>   CMakeLists.txt 65bfbac0e2b6aec26a6d98ff96615661210ceac2 
>   cmake/distcheck.sh PRE-CREATION 
> 
> 
> Diff: https://reviews.apache.org/r/71241/diff/1/
> 
> 
> Testing
> ---
> 
> `ninja distcheck` passes, no artifacts left around but tarball.
> 
> 
> Thanks,
> 
> Benjamin Bannier
> 
>



Re: Review Request 71240: Added a `dist` target to the cmake build.

2019-09-03 Thread Benno Evers

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




cmake/dist.sh
Lines 8 (patched)
<https://reviews.apache.org/r/71240/#comment304789>

Why this restriction if we're going to create the tarball from a clean 
checkout anyways?



cmake/dist.sh
Lines 19 (patched)
<https://reviews.apache.org/r/71240/#comment304793>

I'm not sure if this is written down anywhere, but in general it's a good 
shell-script best practice to use `SCREAMING_CASE` for variables that can be 
overriden from outside and `lower_case` for variables that are internal to the 
script.



cmake/dist.sh
Lines 21 (patched)
<https://reviews.apache.org/r/71240/#comment304790>

Can we find a more speaking name than `D` for this? Also, since this is 
used as a temporary directory it might make sense to use `mktemp -d` rather 
than hardcoding the directory name.



cmake/dist.sh
Lines 29 (patched)
<https://reviews.apache.org/r/71240/#comment304797>

Can you explain why in-tree builds are not supported, i.e. why do we need 
to create a separate build directory here?



cmake/dist.sh
Lines 33 (patched)
<https://reviews.apache.org/r/71240/#comment304791>

I assume `nproc` is some magic cmake variable that is set automatically?


- Benno Evers


On Aug. 6, 2019, 7:28 p.m., Benjamin Bannier wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/71240/
> ---
> 
> (Updated Aug. 6, 2019, 7:28 p.m.)
> 
> 
> Review request for mesos, Benjamin Mahler and Till Toenshoff.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This patch adds a `dist` target to the cmake build, analogous to the
> target provided by the autotools build.
> 
> While cmake already provides a `package_source` target to create a
> source archive it does not take care of only including relevant files,
> but instead adds all files to the archive, e.g., including build or
> backup files which should not be part of a release. For that reason this
> patch introduces a script which performs a temporary `git clone` from
> the checked out git repository and creates the archive from that clean
> tree.
> 
> This patch also removes a hardcoded list of ignored files which was
> by design not exhaustive.
> 
> 
> Diffs
> -
> 
>   CMakeLists.txt 65bfbac0e2b6aec26a6d98ff96615661210ceac2 
>   cmake/dist.sh PRE-CREATION 
> 
> 
> Diff: https://reviews.apache.org/r/71240/diff/1/
> 
> 
> Testing
> ---
> 
> `ninja dist` produces a tarball, no artifacts left around but tarball.
> 
> 
> Thanks,
> 
> Benjamin Bannier
> 
>



Re: Review Request 71414: Gracefully handled duplicated volumes from non-conforming CSI plugins.

2019-08-30 Thread Benno Evers

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


Fix it, then Ship it!




Looks good in general. Given the high urgency of resolving this issue and the 
fact that all raised issued are very minor, I'm going to commit this unmodified 
since this version of the commit was already extensively tested and I don't 
want to risk introducing any careless last-minute mistakes.

If we want we can go back and fix up the issues (in particular stopping the 
clock in the test) in a follow-up commit, maybe after the 1.9 release.


src/resource_provider/storage/provider.cpp
Line 1114 (original), 1135 (patched)
<https://reviews.apache.org/r/71414/#comment304762>

It seems like this could be moved to the beginning of the loop, so we don't 
even have to create the raw disk resource at all in case of a duplicated volume 
id.



src/resource_provider/storage/provider.cpp
Line 1145 (original), 1167 (patched)
<https://reviews.apache.org/r/71414/#comment304761>

Probably the name doesnt quite match anymore, now that the conversion 
converts more than just metadata.



src/tests/storage_local_resource_provider_tests.cpp
Lines 1379 (patched)
<https://reviews.apache.org/r/71414/#comment304763>

Can this test be run when the clock is stopped? If not, can we document in 
a comment why not? Given the many moving parts here, I fear that there's a high 
chance this test will become flaky.


- Benno Evers


On Aug. 30, 2019, 12:48 a.m., Chun-Hung Hsiao wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/71414/
> ---
> 
> (Updated Aug. 30, 2019, 12:48 a.m.)
> 
> 
> Review request for mesos, Benjamin Bannier and Jan Schlicht.
> 
> 
> Bugs: MESOS-9956
> https://issues.apache.org/jira/browse/MESOS-9956
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> If the SLRP uses a plugin that does not conform to the CSI spec and
> reports duplicated volumes, the duplicate would be removed.
> 
> 
> Diffs
> -
> 
>   src/resource_provider/storage/provider.cpp 
> 6d632606f411d3ca99d3573a57c9f68b02ba8072 
>   src/tests/storage_local_resource_provider_tests.cpp 
> 69b59d48ceefebbb7accefe411c54ac5cecff1c3 
> 
> 
> Diff: https://reviews.apache.org/r/71414/diff/1/
> 
> 
> Testing
> ---
> 
> make check
> 
> The test would fail without the fix.
> 
> 
> Thanks,
> 
> Chun-Hung Hsiao
> 
>



Re: Review Request 71389: Added CHANGELOG entry for agent draining.

2019-08-28 Thread Benno Evers


> On Aug. 28, 2019, 2:02 p.m., Greg Mann wrote:
> > CHANGELOG
> > Lines 5-11 (patched)
> > <https://reviews.apache.org/r/71389/diff/1/?file=2163055#file2163055line5>
> >
> > Let's remove this since https://reviews.apache.org/r/71378/ added a 
> > changelog entry.

In addition, I moved the entry from r/71378 under the `Maintenance` subheading.


- Benno


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


On Aug. 28, 2019, 2:24 p.m., Benno Evers wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/71389/
> ---
> 
> (Updated Aug. 28, 2019, 2:24 p.m.)
> 
> 
> Review request for mesos, Greg Mann, Joseph Wu, and Till Toenshoff.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added CHANGELOG entry for agent draining.
> 
> 
> Diffs
> -
> 
>   CHANGELOG 935463849cc937e82d01254624ca10a11b9ad5ee 
>   docs/upgrades.md 63eb1bbe401d4fb36e11e595a6b1954793265b33 
> 
> 
> Diff: https://reviews.apache.org/r/71389/diff/2/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Benno Evers
> 
>



Re: Review Request 71389: Added CHANGELOG entry for agent draining.

2019-08-28 Thread Benno Evers

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

(Updated Aug. 28, 2019, 2:24 p.m.)


Review request for mesos, Greg Mann, Joseph Wu, and Till Toenshoff.


Repository: mesos


Description
---

Added CHANGELOG entry for agent draining.


Diffs (updated)
-

  CHANGELOG 935463849cc937e82d01254624ca10a11b9ad5ee 
  docs/upgrades.md 63eb1bbe401d4fb36e11e595a6b1954793265b33 


Diff: https://reviews.apache.org/r/71389/diff/2/

Changes: https://reviews.apache.org/r/71389/diff/1-2/


Testing
---


Thanks,

Benno Evers



Review Request 71389: Added CHANGELOG entry for agent draining.

2019-08-28 Thread Benno Evers

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

Review request for mesos, Greg Mann, Joseph Wu, and Till Toenshoff.


Repository: mesos


Description
---

Added CHANGELOG entry for agent draining.


Diffs
-

  CHANGELOG 935463849cc937e82d01254624ca10a11b9ad5ee 
  docs/upgrades.md 63eb1bbe401d4fb36e11e595a6b1954793265b33 


Diff: https://reviews.apache.org/r/71389/diff/1/


Testing
---


Thanks,

Benno Evers



Re: Review Request 71338: Moved OpenSSL-related ifdef to a central location.

2019-08-21 Thread Benno Evers

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

(Updated Aug. 21, 2019, 1:34 p.m.)


Review request for mesos, Benjamin Bannier and Till Toenshoff.


Repository: mesos


Description
---

A previous commit introduced an preprocessor directive that would
split up code between brackets, confusing syntax highlighters and
making the logic hard to read.


Diffs (updated)
-

  3rdparty/libprocess/src/openssl.cpp 4aeab855b29837dc6bf93104afc62f111c38626c 


Diff: https://reviews.apache.org/r/71338/diff/2/

Changes: https://reviews.apache.org/r/71338/diff/1-2/


Testing
---


Thanks,

Benno Evers



Review Request 71338: Moved OpenSSL-related ifdef to a central location.

2019-08-21 Thread Benno Evers

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

Review request for mesos, Benjamin Bannier and Till Toenshoff.


Repository: mesos


Description
---

A previous commit introduced an preprocessor directive that would
split up code between brackets, confusing syntax highlighters and
making the logic hard to read.


Diffs
-

  3rdparty/libprocess/src/openssl.cpp 4aeab855b29837dc6bf93104afc62f111c38626c 


Diff: https://reviews.apache.org/r/71338/diff/1/


Testing
---


Thanks,

Benno Evers



Re: Review Request 71337: Switched on full debug logging and stack trace reporting for Maven.

2019-08-21 Thread Benno Evers

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


Ship it!




Ship It!

- Benno Evers


On Aug. 21, 2019, 12:50 p.m., Andrei Sekretenko wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/71337/
> ---
> 
> (Updated Aug. 21, 2019, 12:50 p.m.)
> 
> 
> Review request for mesos, Benjamin Bannier and Benno Evers.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This is done in attempt to track down the origin of Java build errors
> sporadically observed on CIs. The added volume of logs is around 15K
> lines, which is still much smaller than that of the logs of the tests.
> Developers who perform local build without Java bindings should
> not be affected.
> 
> 
> Diffs
> -
> 
>   src/Makefile.am 0377c8dc4a0f876f972797556c98d88aaab86589 
>   src/java/CMakeLists.txt 29422e90b10647dc1bf78237725e89b15c69e6aa 
> 
> 
> Diff: https://reviews.apache.org/r/71337/diff/1/
> 
> 
> Testing
> ---
> 
> Ran tests in CI.
> 
> 
> Thanks,
> 
> Andrei Sekretenko
> 
>



Review Request 71329: Updated CentOS 6 dockerfile to properly install devtoolset-7.

2019-08-20 Thread Benno Evers

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

Review request for mesos and Till Toenshoff.


Repository: mesos


Description
---

The previously used yum command was not able to properly install
the required `devtoolset-7` package since the containing repository
was only visible after the first `yum` invocation finished.

This would result in `yum` silently ignoring the package, producing
a broken CentOS 6 docker image.


Diffs
-

  support/packaging/centos/centos6.dockerfile 
bb8d591ce6d41a70bde64020d18aac739021e88e 


Diff: https://reviews.apache.org/r/71329/diff/1/


Testing
---


Thanks,

Benno Evers



Review Request 71309: Added more details to the Bintray release guide.

2019-08-19 Thread Benno Evers

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

Review request for mesos, Greg Mann, Joseph Wu, Till Toenshoff, and Vinod Kone.


Repository: mesos


Description
---

Added more details to the Bintray release guide.


Diffs
-

  docs/release-guide.md 88281d691210307a1a6d6c5a9fd71f1607982694 


Diff: https://reviews.apache.org/r/71309/diff/1/


Testing
---

Uploaded 1.8.1 package to bintray according to these instructions.


Thanks,

Benno Evers



Re: Review Request 71297: Fixed a flaky operation reconciliation test.

2019-08-19 Thread Benno Evers


> On Aug. 16, 2019, 2:43 p.m., Andrei Sekretenko wrote:
> > src/tests/operation_reconciliation_tests.cpp
> > Lines 1842 (patched)
> > <https://reviews.apache.org/r/71297/diff/1/?file=2161041#file2161041line1842>
> >
> > We are restarting the master once, but the scheduler gets 
> > connected/disconnected event pair twice during the master restart... is 
> > this guaranteed by TestMesos + StandaloneMasterDetector, a stable 
> > coincidence, or just the most likely outcome of some other race? 
> > 
> > IMO, this is at least worth a comment - but what about preventing it? 
> > (Will something like `detector->appoint(None())` + 
> > `AWAIT_READY(disconnected)` before killing the master help?)

Great idea, thanks!


- Benno


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


On Aug. 19, 2019, 12:36 p.m., Benno Evers wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/71297/
> ---
> 
> (Updated Aug. 19, 2019, 12:36 p.m.)
> 
> 
> Review request for mesos, Andrei Sekretenko, Greg Mann, Joseph Wu, and Till 
> Toenshoff.
> 
> 
> Bugs: MESOS-9928
> https://issues.apache.org/jira/browse/MESOS-9928
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> The FrameworkReconciliationRaceWithUpdateSlave test from the
> operation reconciliation tests was flaky since we did not wait
> for the scheduler to reconnect before attempting to send a
> subscribe call.
> 
> 
> Diffs
> -
> 
>   src/tests/operation_reconciliation_tests.cpp 
> 9d084c027ec2f910515cafebf715f7428c43f1a9 
> 
> 
> Diff: https://reviews.apache.org/r/71297/diff/2/
> 
> 
> Testing
> ---
> 
> `./src/mesos-tests 
> --gtest_filter="*FrameworkReconciliationRaceWithUpdateSlaveMessage*" 
> --gtest_repeat=200` while simultaneously running `stress-ng` in the 
> background.
> 
> 
> Thanks,
> 
> Benno Evers
> 
>



Re: Review Request 71297: Fixed a flaky operation reconciliation test.

2019-08-19 Thread Benno Evers

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

(Updated Aug. 19, 2019, 12:36 p.m.)


Review request for mesos, Andrei Sekretenko, Greg Mann, Joseph Wu, and Till 
Toenshoff.


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


Repository: mesos


Description (updated)
---

The FrameworkReconciliationRaceWithUpdateSlave test from the
operation reconciliation tests was flaky since we did not wait
for the scheduler to reconnect before attempting to send a
subscribe call.


Diffs (updated)
-

  src/tests/operation_reconciliation_tests.cpp 
9d084c027ec2f910515cafebf715f7428c43f1a9 


Diff: https://reviews.apache.org/r/71297/diff/2/

Changes: https://reviews.apache.org/r/71297/diff/1-2/


Testing
---

`./src/mesos-tests 
--gtest_filter="*FrameworkReconciliationRaceWithUpdateSlaveMessage*" 
--gtest_repeat=200` while simultaneously running `stress-ng` in the background.


Thanks,

Benno Evers



Re: Review Request 71208: Revert "Updated cpplint to be compatible with Python 3."

2019-08-19 Thread Benno Evers

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


Ship it!




Ship It!

- Benno Evers


On July 30, 2019, 9:01 p.m., Benjamin Bannier wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/71208/
> ---
> 
> (Updated July 30, 2019, 9:01 p.m.)
> 
> 
> Review request for mesos and Till Toenshoff.
> 
> 
> Bugs: MESOS-9630
> https://issues.apache.org/jira/browse/MESOS-9630
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This reverts commit 89db66e3df831eaa50fffb4149a3894097505c14.
> 
> This patch was necessary when we were running cpplint in the python3
> environment used e.g., also for bindings and other scripts. With
> pre-commit we have freedom to choose the Python environment needed so we
> can undo our adjustments here to stay closer to upstream.
> 
> 
> Diffs
> -
> 
>   support/cpplint.py 66ec8b3636a8d3ba57becd8560b4fe394e7119d8 
> 
> 
> Diff: https://reviews.apache.org/r/71208/diff/2/
> 
> 
> Testing
> ---
> 
> * used for development for a couple of months
> * reports no issues in the current source tree
> 
> 
> Thanks,
> 
> Benjamin Bannier
> 
>



Re: Review Request 71207: Revert "Updated cpplint.py to be less verbose when there is no linting issue."

2019-08-19 Thread Benno Evers

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


Ship it!




Ship It!

- Benno Evers


On July 30, 2019, 9:01 p.m., Benjamin Bannier wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/71207/
> ---
> 
> (Updated July 30, 2019, 9:01 p.m.)
> 
> 
> Review request for mesos and Till Toenshoff.
> 
> 
> Bugs: MESOS-9630
> https://issues.apache.org/jira/browse/MESOS-9630
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This reverts commit c0f8f56d5a93f3fb870e448fedfd22f1491356ca.
> 
> This patch was necessary when we were running cpplint via
> `support/mesos-style.py` to prevent it from cluttering up the hook
> output. When running under pre-commit linter output is not shown if no
> errors occur so we can undo our change to stay closer to upstream.
> 
> 
> Diffs
> -
> 
>   support/cpplint.py 66ec8b3636a8d3ba57becd8560b4fe394e7119d8 
> 
> 
> Diff: https://reviews.apache.org/r/71207/diff/2/
> 
> 
> Testing
> ---
> 
> * used for development for a couple of months
> * reports no issues in the current source tree
> 
> 
> Thanks,
> 
> Benjamin Bannier
> 
>



Re: Review Request 71205: Switched commit hooks to pre-commit.

2019-08-19 Thread Benno Evers

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


Ship it!




Ship It!

- Benno Evers


On Aug. 19, 2019, 7:20 a.m., Benjamin Bannier wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/71205/
> ---
> 
> (Updated Aug. 19, 2019, 7:20 a.m.)
> 
> 
> Review request for mesos and Till Toenshoff.
> 
> 
> Bugs: MESOS-9630
> https://issues.apache.org/jira/browse/MESOS-9630
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This patch switches commit hooks to be orchestrated by the pre-commit
> tool mirroring the previous linters invoked through git commit
> hooks (orchestrated by `support/mesos-style.py` or standalone hooks).
> 
> Using pre-commit removes the burden of maintaining
> `support/mesos-style.py`, making sure that hooks have the expected
> environment (e.g., Python version, Node installed). Additionally,
> upstream provides a number of additional linters which are not hard to
> add to Mesos' hooks.
> 
> 
> Diffs
> -
> 
>   docs/advanced-contribution.md 573138d3642e901f46ee35db58299627f188b94c 
>   support/gitignore 7218eda0d78b8e6fc4568f215016961bd7a11a1b 
>   support/pre-commit-config.yaml PRE-CREATION 
>   support/setup-dev.bat 584b5c3ca228ff485b22473cd167d3f454d6dad4 
>   support/setup-dev.sh PRE-CREATION 
> 
> 
> Diff: https://reviews.apache.org/r/71205/diff/7/
> 
> 
> Testing
> ---
> 
> * used successfully for a couple of months
> 
> 
> Thanks,
> 
> Benjamin Bannier
> 
>



Re: Review Request 71209: Enabled a number of additional pre-commit checks.

2019-08-19 Thread Benno Evers

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


Ship it!




Ship It!

- Benno Evers


On Aug. 19, 2019, 7:20 a.m., Benjamin Bannier wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/71209/
> ---
> 
> (Updated Aug. 19, 2019, 7:20 a.m.)
> 
> 
> Review request for mesos and Till Toenshoff.
> 
> 
> Bugs: MESOS-9630
> https://issues.apache.org/jira/browse/MESOS-9630
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Enabled a number of additional pre-commit checks.
> 
> 
> Diffs
> -
> 
>   support/pre-commit-config.yaml PRE-CREATION 
> 
> 
> Diff: https://reviews.apache.org/r/71209/diff/5/
> 
> 
> Testing
> ---
> 
> * used for development for a couple of months
> * reports no issues in the current source tree as indentified issues were 
> fixed
> 
> 
> Thanks,
> 
> Benjamin Bannier
> 
>



Re: Review Request 70096: Moved cpplint configuration into dedicated file.

2019-08-19 Thread Benno Evers

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


Ship it!




Ship It!

- Benno Evers


On Aug. 14, 2019, 11:25 a.m., Benjamin Bannier wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/70096/
> ---
> 
> (Updated Aug. 14, 2019, 11:25 a.m.)
> 
> 
> Review request for mesos and Benjamin Mahler.
> 
> 
> Bugs: MESOS-9630
> https://issues.apache.org/jira/browse/MESOS-9630
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> With this change we not only reduce the amount of code in
> `support/mesos-style.py` in favor of a configuration supported by
> upstream, but we also make it easier to interoperate with editor
> integrations for cpplint.
> 
> 
> Diffs
> -
> 
>   bootstrap 7be2cc95c7ace55d710315427f38284cc5b2af4c 
>   bootstrap.bat 584b5c3ca228ff485b22473cd167d3f454d6dad4 
>   support/CPPLINT.cfg PRE-CREATION 
>   support/gitignore 7218eda0d78b8e6fc4568f215016961bd7a11a1b 
>   support/mesos-style.py cd490bd3deeb8477295bbad010f36d984ce17d27 
> 
> 
> Diff: https://reviews.apache.org/r/70096/diff/7/
> 
> 
> Testing
> ---
> 
> * confirmed that `./support/mesos-style.py src/executor/executor.cpp` still 
> does what is expected
> * no new warnings when running over the whole codebase
> 
> 
> Thanks,
> 
> Benjamin Bannier
> 
>



Re: Review Request 71203: Added check script to check for license headers.

2019-08-19 Thread Benno Evers

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


Ship it!




Ship It!

- Benno Evers


On Aug. 19, 2019, 7:20 a.m., Benjamin Bannier wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/71203/
> ---
> 
> (Updated Aug. 19, 2019, 7:20 a.m.)
> 
> 
> Review request for mesos and Till Toenshoff.
> 
> 
> Bugs: MESOS-9630
> https://issues.apache.org/jira/browse/MESOS-9630
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This check adds a script which validates that source files have valid
> license headers. This will allow us to reuse this functionality with
> e.g., the pre-commit tool.
> 
> At the moment the code added here is not invoked from
> `support/mesos-style.py` since it will be removed in a follow-up commit.
> 
> 
> Diffs
> -
> 
>   support/check-license.py PRE-CREATION 
> 
> 
> Diff: https://reviews.apache.org/r/71203/diff/5/
> 
> 
> Testing
> ---
> 
> * tested against files with license headers present or absent
> * tested against all Python and C++ source files in the repo
> 
> 
> Thanks,
> 
> Benjamin Bannier
> 
>



Re: Review Request 71204: Added gitlint config.

2019-08-19 Thread Benno Evers

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


Ship it!




Ship It!

- Benno Evers


On Aug. 14, 2019, 11:24 a.m., Benjamin Bannier wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/71204/
> ---
> 
> (Updated Aug. 14, 2019, 11:24 a.m.)
> 
> 
> Review request for mesos and Till Toenshoff.
> 
> 
> Bugs: MESOS-9630
> https://issues.apache.org/jira/browse/MESOS-9630
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This patch adds a config for the gitlint tool which is slated to replace
> a custom commit-msg hook once we switch our hook infrastructure to the
> pre-commit tool.
> 
> 
> Diffs
> -
> 
>   bootstrap 7be2cc95c7ace55d710315427f38284cc5b2af4c 
>   bootstrap.bat 584b5c3ca228ff485b22473cd167d3f454d6dad4 
>   support/gitignore 7218eda0d78b8e6fc4568f215016961bd7a11a1b 
>   support/gitlint PRE-CREATION 
> 
> 
> Diff: https://reviews.apache.org/r/71204/diff/7/
> 
> 
> Testing
> ---
> 
> n/a
> 
> 
> Thanks,
> 
> Benjamin Bannier
> 
>



Re: Review Request 71206: Removed old mesos-style and references.

2019-08-19 Thread Benno Evers

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


Fix it, then Ship it!





docs/c++-style-guide.md
Line 10 (original), 10 (patched)
<https://reviews.apache.org/r/71206/#comment304576>

s/script/command/



support/mesos-style.py
Line 28 (original), 25 (patched)
<https://reviews.apache.org/r/71206/#comment304577>

This is probably going to be the first point of contact with the new 
linting system for most developers, so it might pay to be a bit more verbose, 
e.g. mention that you can install `pre-commit` via pip and add links to the 
pre-commit website, mailing list or JIRA tickets, etc.

However, it's essentially a judgment call so I'll leave it up to you :D


- Benno Evers


On Aug. 16, 2019, 7:42 p.m., Benjamin Bannier wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/71206/
> ---
> 
> (Updated Aug. 16, 2019, 7:42 p.m.)
> 
> 
> Review request for mesos and Till Toenshoff.
> 
> 
> Bugs: MESOS-9630
> https://issues.apache.org/jira/browse/MESOS-9630
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This patch removes references to `support/mesos-style.py` which was
> replaced with a pre-commit setup in a previous commit. We also remove
> the tool itself.
> 
> 
> Diffs
> -
> 
>   docs/c++-style-guide.md 8a48afe780f23736c9b7abeb7337977521cecfa5 
>   support/build-virtualenv 7dc03b054f7663979e4eb4b11ad51d759b7f1ad3 
>   support/hooks/commit-msg a0c218deee3fb4b7594fe39b76c1025045ba0725 
>   support/hooks/post-rewrite 1ab14abf711d1923a7ae69beb33581317009a94a 
>   support/hooks/pre-commit 519567bf5f20a74b273c8d8514577fe4342dc45d 
>   support/mesos-split.py 0a77c257386ffe576abd12f59f926640836ad900 
>   support/mesos-style.py cd490bd3deeb8477295bbad010f36d984ce17d27 
> 
> 
> Diff: https://reviews.apache.org/r/71206/diff/4/
> 
> 
> Testing
> ---
> 
> n/a
> 
> 
> Thanks,
> 
> Benjamin Bannier
> 
>



Re: Review Request 71299: Added separate script to install developer setup.

2019-08-19 Thread Benno Evers

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


Fix it, then Ship it!





docs/advanced-contribution.md
Line 69 (original), 69 (patched)
<https://reviews.apache.org/r/71299/#comment304575>

This became a bit ambiguous now, i.e. is only the second step or the whole 
step only required if building from git?

(also, I'm not sure why we have this qualification at all in a contributors 
guide)


- Benno Evers


On Aug. 19, 2019, 7:19 a.m., Benjamin Bannier wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/71299/
> ---
> 
> (Updated Aug. 19, 2019, 7:19 a.m.)
> 
> 
> Review request for mesos, Benno Evers and Till Toenshoff.
> 
> 
> Bugs: MESOS-9630
> https://issues.apache.org/jira/browse/MESOS-9630
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This patch breaks the installation of developer tools (i.e., linter
> configuration files and git hooks) out of `./bootstrap`. This not only
> simplifies and streamlines the setup, but will allow us to add
> developer-only features without breaking users who are just interested
> in building a distribution tarball.
> 
> 
> Diffs
> -
> 
>   bootstrap 7be2cc95c7ace55d710315427f38284cc5b2af4c 
>   bootstrap.bat 584b5c3ca228ff485b22473cd167d3f454d6dad4 
>   docs/advanced-contribution.md 573138d3642e901f46ee35db58299627f188b94c 
>   docs/beginner-contribution.md 471f5dd0da30d921cc3b29484d2b02f164f8ca75 
>   docs/clang-format.md 4289813c2ca78e1b963aa22a596abd6e8aa3a28b 
>   support/gitignore 7218eda0d78b8e6fc4568f215016961bd7a11a1b 
>   support/llvm/README.md 188930224ba8ff53dbef8770bc56625be936bb76 
>   support/setup-dev.sh PRE-CREATION 
> 
> 
> Diff: https://reviews.apache.org/r/71299/diff/4/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Benjamin Bannier
> 
>



Review Request 71297: Fixed a flaky operation reconciliation test.

2019-08-16 Thread Benno Evers

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

Review request for mesos, Andrei Sekretenko, Greg Mann, Joseph Wu, and Till 
Toenshoff.


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


Repository: mesos


Description
---

The FrameworkReconciliationRaceWithUpdateSlave test from the
operation reconciliation tests was flaky since we did not wait
for the scheduler to reconnect before advancing the clock to
trigger reregistration.


Diffs
-

  src/tests/operation_reconciliation_tests.cpp 
9d084c027ec2f910515cafebf715f7428c43f1a9 


Diff: https://reviews.apache.org/r/71297/diff/1/


Testing
---

`./src/mesos-tests 
--gtest_filter="*FrameworkReconciliationRaceWithUpdateSlaveMessage*" 
--gtest_repeat=200` while simultaneously running `stress-ng` in the background.


Thanks,

Benno Evers



Re: Review Request 70096: Moved cpplint configuration into dedicated file.

2019-08-14 Thread Benno Evers

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



Can you maybe clarify in the commit description what the advantage of having 
this in a separate file is?

This is going to clutter the source dir with a very visible, screaming-case 
`CPPLINT.cfg` file, so all other things being equal it seems like passing these 
settings via command-line instead would be preferrable?

- Benno Evers


On March 3, 2019, 1:01 a.m., Benjamin Bannier wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/70096/
> ---
> 
> (Updated March 3, 2019, 1:01 a.m.)
> 
> 
> Review request for mesos and Benjamin Mahler.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Moved cpplint configuration into dedicated file.
> 
> 
> Diffs
> -
> 
>   bootstrap 7be2cc95c7ace55d710315427f38284cc5b2af4c 
>   support/CPPLINT.cfg PRE-CREATION 
>   support/mesos-style.py b14820a1efe32c9c6e093b6f9cea93cb55ec97e4 
> 
> 
> Diff: https://reviews.apache.org/r/70096/diff/2/
> 
> 
> Testing
> ---
> 
> * confirmed that `./support/mesos-style.py src/executor/executor.cpp` still 
> does what is expected
> * no new warnings when running over the whole codebase
> 
> 
> Thanks,
> 
> Benjamin Bannier
> 
>



Re: Review Request 71205: Switch commit hooks to pre-commit.

2019-08-14 Thread Benno Evers

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




bootstrap
Line 55 (original), 55 (patched)
<https://reviews.apache.org/r/71205/#comment304493>

Copying my comment from slack here so the discussion isn't split over too 
many places:

As we discussed privately yesterday, I think installing this in bootstrap 
is a bit problematic because that is also part of the source tarball, used by 
non-developers to build mesos w/o even having a git repository. Additionally, I 
think I'd be not particularly amused if I cloned a random open-source project 
and the first thing it does is install a bunch of stuff in my home directory.

I'm not sure if it's possible to implement, but imho the ideal workflow 
would be something like this:

```
$ git commit -m "Foo the bar."
WARNING: Your commit touched a `.cpp` file, but `cpplint` is not installed 
on your system. It is highly recommended to install it to avoid embarrassing 
mistakes.

You can also run `pre-commit ` to automatically install a usable 
version of `cpplint` in you home directory.
    ```


- Benno Evers


On July 30, 2019, 9:01 p.m., Benjamin Bannier wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/71205/
> ---
> 
> (Updated July 30, 2019, 9:01 p.m.)
> 
> 
> Review request for mesos and Till Toenshoff.
> 
> 
> Bugs: MESOS-9360
> https://issues.apache.org/jira/browse/MESOS-9360
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This patch switches commit hooks to be orchestrated by the pre-commit
> tool mirroring the previous linters invoked through git commit
> hooks (orchestrated by `support/mesos-style.py` or standalone hooks).
> 
> Using pre-commit removes the burden of maintaining
> `support/mesos-style.py`, making sure that hooks have the expected
> environment (e.g., Python version, Node installed). Additionally,
> upstream provides a number of additional linters which are not hard to
> add to Mesos' hooks.
> 
> 
> Diffs
> -
> 
>   .pre-commit-config.yaml PRE-CREATION 
>   bootstrap 7be2cc95c7ace55d710315427f38284cc5b2af4c 
> 
> 
> Diff: https://reviews.apache.org/r/71205/diff/1/
> 
> 
> Testing
> ---
> 
> * used successfully for a couple of months
> 
> 
> Thanks,
> 
> Benjamin Bannier
> 
>



Re: Review Request 71206: Removed old mesos-style and references.

2019-08-14 Thread Benno Evers

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



After applying this series for testing purposes, I ran into a problem where 
`mesos-style.py` was still referenced from a git hook, breaking my ability to 
commit without non-trivial investigation of the problem. (and I had the 
advantage of *knowing* that I just applied this patch series, if we commit it 
to master some other developers will be caught by suprise, no matter how 
heavily it is advertised.)

I'd suggest instead of removing `mesos-style.py` outright, we replace it with a 
simple bash script that just prints something like `WARNING: 'mesos-style.py' 
is not supported anymore and does nothing, please fix whatever was calling this 
script. See MESOS-9630.`

- Benno Evers


On July 30, 2019, 9:01 p.m., Benjamin Bannier wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/71206/
> ---
> 
> (Updated July 30, 2019, 9:01 p.m.)
> 
> 
> Review request for mesos and Till Toenshoff.
> 
> 
> Bugs: MESOS-9360
> https://issues.apache.org/jira/browse/MESOS-9360
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This patch removes references to `support/mesos-style.py` which was
> replaced with a pre-commit setup in a previous commit. We also remove
> the tool itself.
> 
> 
> Diffs
> -
> 
>   docs/c++-style-guide.md 8a48afe780f23736c9b7abeb7337977521cecfa5 
>   support/build-virtualenv 7dc03b054f7663979e4eb4b11ad51d759b7f1ad3 
>   support/hooks/commit-msg a0c218deee3fb4b7594fe39b76c1025045ba0725 
>   support/hooks/post-rewrite 1ab14abf711d1923a7ae69beb33581317009a94a 
>   support/hooks/pre-commit 519567bf5f20a74b273c8d8514577fe4342dc45d 
>   support/mesos-style.py cd490bd3deeb8477295bbad010f36d984ce17d27 
> 
> 
> Diff: https://reviews.apache.org/r/71206/diff/1/
> 
> 
> Testing
> ---
> 
> n/a
> 
> 
> Thanks,
> 
> Benjamin Bannier
> 
>



Re: Review Request 71275: Fixed agent draining when returning from unreachable state.

2019-08-13 Thread Benno Evers

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




src/master/master.cpp
Lines 7959 (patched)
<https://reviews.apache.org/r/71275/#comment304448>

Can we already decide at this point if the agent is drained? E.g. 
`checkAndTransitionDrainingAgent()` seems to look at pending operations, which 
only get sent in the first `UpdateSlaveMessage` after the reregistration is 
completed.


- Benno Evers


On Aug. 13, 2019, 2:35 a.m., Joseph Wu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/71275/
> ---
> 
> (Updated Aug. 13, 2019, 2:35 a.m.)
> 
> 
> Review request for mesos, Benjamin Bannier, Benno Evers, and Greg Mann.
> 
> 
> Bugs: MESOS-9934
> https://issues.apache.org/jira/browse/MESOS-9934
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This logic was missing from the initial implementation of agent
> draining.  When an agent became unreachable, and then reregistered
> with the master, the master would not properly deactivate or drain
> the agent.
> 
> 
> Diffs
> -
> 
>   src/master/master.cpp 31c7c97abecb92591369b417e2ef38a99cc09bac 
>   src/tests/api_tests.cpp c2099674e742eaa08134c5e0a7cdab1734808119 
> 
> 
> Diff: https://reviews.apache.org/r/71275/diff/1/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Joseph Wu
> 
>



Re: Review Request 70885: Renamed 'libprocess::network::Address::hostname()'.

2019-08-12 Thread Benno Evers


> On June 24, 2019, 4:20 p.m., Benjamin Mahler wrote:
> > 3rdparty/libprocess/include/process/address.hpp
> > Lines 65-68 (original), 70-74 (patched)
> > <https://reviews.apache.org/r/70885/diff/1/?file=2151346#file2151346line70>
> >
> > It looks a little strange that stout's functions are 
> > hostname/getHostname but libprocess' is lookup_hostname.
> > 
> > Rather than change the name to signal the lookup, having this return a 
> > Future seems sufficient to signal the asynchronous nature of this operation 
> > (per the existing TODO):
> > 
> > ```
> > Future hostname = hostname();
> > 
> > // or
> > 
> > Future> hostnames = hostnames();
> > ```
> > 
> > That way, stout's `hostname()` is the synchronous blocking way to get 
> > it, and libprocess' `hostname()` asynchronous non-blocking way to get it.
> > 
> > I think that's generally the naming convention we'd like to stick to.

I think that changing the signature would not address the core of the problem, 
which is that in code like


```
host = address.hostname(); // `host` is some member variable
```

it would still be very hard for a reader to catch what's going on, unless he 
already has some prior knowledge that `host` is a future or `hostname()` will 
do a lookup.

Even in this version, it is very easy to overlook the `.get()`, or assume it 
belongs to a `Try`, `Option`, `shared_ptr`, etc.:

```
string host = address.hostname().get();
```

Renaming, on the other hand, is unambigous and impossible to miss.

Regarding stout, `net::hostname()` actually does something *different* than the 
function we're touching here, in particular it does **not** do a lookup. So it 
seems like the existing naming was confusing enough to fool even you ;)


- Benno


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


On June 19, 2019, 2:44 p.m., Benno Evers wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/70885/
> ---
> 
> (Updated June 19, 2019, 2:44 p.m.)
> 
> 
> Review request for mesos, Joseph Wu and Till Toenshoff.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Renamed member function `hostname()` to `lookup_hostname()`,
> since the former name hides the fact that a call to this
> function involves a synchronous network access in order
> to make a reverse DNS lookup.
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/include/process/address.hpp 
> e740e840c38381bafd7a1a7fcde5f963832ac1fb 
>   3rdparty/libprocess/src/tests/http_tests.cpp 
> 97aaf3ed3d4fab6d717d5c9b6d12402562ac6b46 
>   3rdparty/libprocess/src/tests/ssl_tests.cpp 
> 6b8496aeeed79ae1bd39d7013f4f403b248fdd4c 
> 
> 
> Diff: https://reviews.apache.org/r/70885/diff/1/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Benno Evers
> 
>



Review Request 71123: Made openssl configuration in 'configure.ac' occur earlier.

2019-07-19 Thread Benno Evers

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

Review request for mesos, Greg Mann, Joseph Wu, and Till Toenshoff.


Repository: mesos


Description
---

The `AC_CHECK_LIBS()` call has a side effect of adding the
found libraries to the linker command line by default. In
addition, our custom configuration code also adds any
non-default include and library search paths to `LDFLAGS`
and `CPPFLAGS`.

Since many of our third-party dependencies depend themselves
on openssl, the configuration check for openssl should be
early during the configuration so that later libraries can
be built against the same version of openssl that is used
for the Mesos build itself.

According to the instructions given at

  https://www.gnu.org/software/autoconf/manual/autoconf-2.67
/html_node/Libraries.html

reordering the calls to `AC_CHECK_LIB()` is a preferrable solution
to passing in `-lssl -lcrypto` as the `other-libraries` argument.


Diffs
-

  configure.ac 4635bb38b6b65ea0454cfbde31b681a6ce232e10 


Diff: https://reviews.apache.org/r/71123/diff/1/


Testing
---


Thanks,

Benno Evers



Re: Review Request 71092: Added additionaly synchronization to a previously flaky test.

2019-07-17 Thread Benno Evers

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


Ship it!




Ship It!

- Benno Evers


On July 17, 2019, 12:02 p.m., Benjamin Bannier wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/71092/
> ---
> 
> (Updated July 17, 2019, 12:02 p.m.)
> 
> 
> Review request for mesos, Benno Evers, Greg Mann, and Joseph Wu.
> 
> 
> Bugs: MESOS-9895
> https://issues.apache.org/jira/browse/MESOS-9895
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This test did not make sure that the agent's task status update manager
> had received a task status update acknowledgemtn before launching a
> subsequent task. The agent then, given its state, correctly rejected the
> task.
> 
> This patch adds additional synchronization so the agent has finished
> processing the task status update like expected.
> 
> 
> Diffs
> -
> 
>   src/tests/slave_tests.cpp 1ed59ca5af69a6b53ceb8be035e608286299c2f1 
> 
> 
> Diff: https://reviews.apache.org/r/71092/diff/1/
> 
> 
> Testing
> ---
> 
> This test previously failed about 4% of the time in ~400 iterations (~20 
> failures) under a lot of additional system stress. With this patch it did not 
> fail once in 500 iterations.
> 
> 
> Thanks,
> 
> Benjamin Bannier
> 
>



Re: Review Request 71035: Added test to verify that Docker executor can override kill policy.

2019-07-10 Thread Benno Evers

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


Ship it!




Ship It!

- Benno Evers


On July 8, 2019, 6:28 p.m., Greg Mann wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/71035/
> ---
> 
> (Updated July 8, 2019, 6:28 p.m.)
> 
> 
> Review request for mesos, Benno Evers, Benjamin Mahler, and Joseph Wu.
> 
> 
> Bugs: MESOS-9853
> https://issues.apache.org/jira/browse/MESOS-9853
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This adds a test which verifies that when a scheduler attemps to
> override a task's default kill policy, the Docker executor will
> honor that override.
> 
> 
> Diffs
> -
> 
>   src/internal/evolve.hpp ffbb342a42ec3501383500dee55dff3588725478 
>   src/internal/evolve.cpp 81de15e192580d2f35237e124c11902e1fc67a1d 
>   src/tests/containerizer/docker_containerizer_tests.cpp 
> a6217581e20168c5114f733323e927a83ac59fd0 
> 
> 
> Diff: https://reviews.apache.org/r/71035/diff/1/
> 
> 
> Testing
> ---
> 
> `make check`
> `sudo bin/mesos-tests.sh --gtest_filter="*ROOT_DOCKER_OverrideKillPolicy*" 
> --gtest_repeat=-1 --gtest_break_on_failure`
> 
> Verified that this new test fails on current master, and passes when the 
> preceding patches in this chain are applied.
> 
> 
> Thanks,
> 
> Greg Mann
> 
>



Re: Review Request 71035: Added test to verify that Docker executor can override kill policy.

2019-07-10 Thread Benno Evers

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




src/tests/containerizer/docker_containerizer_tests.cpp
Lines 5250 (patched)
<https://reviews.apache.org/r/71035/#comment303717>

The test itself looks good, but is it possible to run it with a stopped 
clock? Given the various timers involved in the body, this feels like a prime 
candidate to become flaky sooner or later.


- Benno Evers


On July 8, 2019, 6:28 p.m., Greg Mann wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/71035/
> ---
> 
> (Updated July 8, 2019, 6:28 p.m.)
> 
> 
> Review request for mesos, Benno Evers, Benjamin Mahler, and Joseph Wu.
> 
> 
> Bugs: MESOS-9853
> https://issues.apache.org/jira/browse/MESOS-9853
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This adds a test which verifies that when a scheduler attemps to
> override a task's default kill policy, the Docker executor will
> honor that override.
> 
> 
> Diffs
> -
> 
>   src/internal/evolve.hpp ffbb342a42ec3501383500dee55dff3588725478 
>   src/internal/evolve.cpp 81de15e192580d2f35237e124c11902e1fc67a1d 
>   src/tests/containerizer/docker_containerizer_tests.cpp 
> a6217581e20168c5114f733323e927a83ac59fd0 
> 
> 
> Diff: https://reviews.apache.org/r/71035/diff/1/
> 
> 
> Testing
> ---
> 
> `make check`
> `sudo bin/mesos-tests.sh --gtest_filter="*ROOT_DOCKER_OverrideKillPolicy*" 
> --gtest_repeat=-1 --gtest_break_on_failure`
> 
> Verified that this new test fails on current master, and passes when the 
> preceding patches in this chain are applied.
> 
> 
> Thanks,
> 
> Greg Mann
> 
>



Re: Review Request 71034: Enabled the Docker executor to accept kill policy overrides.

2019-07-10 Thread Benno Evers

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


Fix it, then Ship it!





src/docker/executor.cpp
Line 401 (original), 404 (patched)
<https://reviews.apache.org/r/71034/#comment303715>

Should we print the override value here if it is provided?



src/docker/executor.cpp
Lines 948 (patched)
<https://reviews.apache.org/r/71034/#comment303713>

Unnecessary `move`?



src/docker/executor.cpp
Lines 992 (patched)
<https://reviews.apache.org/r/71034/#comment303714>

Same as above, unnecessary `std::move()`.



src/exec/exec.cpp
Lines 374 (patched)
<https://reviews.apache.org/r/71034/#comment303716>

I believe the "standard" way to write this would be

```
auto* dockerExecutor = dynamic_cast(executor);
if (dockerExecutor) {
  // `executor` was a `DockerExecutor`
} else {
  // `executor` was some other `Executor`
}
```

This way, classes derived from `DockerExecutor` will also get the correct 
overload.


- Benno Evers


On July 8, 2019, 6:25 p.m., Greg Mann wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/71034/
> ---
> 
> (Updated July 8, 2019, 6:25 p.m.)
> 
> 
> Review request for mesos, Benno Evers, Benjamin Mahler, and Joseph Wu.
> 
> 
> Bugs: MESOS-9853
> https://issues.apache.org/jira/browse/MESOS-9853
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This adds a new `killTask()` overload to the Docker executor
> and updates the Mesos executor driver to call into that
> overload when the loaded executor is the Docker executor.
> 
> This allows the executor driver to pass the kill policy
> override, when present, into the Docker executor.
> 
> 
> Diffs
> -
> 
>   src/docker/executor.hpp f21e84c71f646e84404c65fc2ded64bcaff482ef 
>   src/docker/executor.cpp f638e4b65155bcca1be36424b7061ea26a3d6ca3 
>   src/exec/exec.cpp c0fa3b61667da96bc4395bae9956c54446268122 
> 
> 
> Diff: https://reviews.apache.org/r/71034/diff/1/
> 
> 
> Testing
> ---
> 
> Details at the end of this chain.
> 
> 
> Thanks,
> 
> Greg Mann
> 
>



Re: Review Request 71033: Moved the Docker executor declaration into a header.

2019-07-10 Thread Benno Evers

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


Ship it!




Ship It!

- Benno Evers


On July 8, 2019, 6:22 p.m., Greg Mann wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/71033/
> ---
> 
> (Updated July 8, 2019, 6:22 p.m.)
> 
> 
> Review request for mesos, Benno Evers, Benjamin Mahler, and Joseph Wu.
> 
> 
> Bugs: MESOS-9853
> https://issues.apache.org/jira/browse/MESOS-9853
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This moves the declaration of the Docker executor into the
> Docker executor header file and moves the code for the Docker
> executor binary into a new launcher implementation file.
> 
> This change will enable the Mesos executor driver
> implementation to make use of the `DockerExecutor` symbol.
> 
> 
> Diffs
> -
> 
>   src/Makefile.am 761dde1d63e0f4f1ac4ab86f129f84f3746d3153 
>   src/docker/executor.hpp f21e84c71f646e84404c65fc2ded64bcaff482ef 
>   src/docker/executor.cpp f638e4b65155bcca1be36424b7061ea26a3d6ca3 
>   src/launcher/docker_executor.cpp PRE-CREATION 
> 
> 
> Diff: https://reviews.apache.org/r/71033/diff/1/
> 
> 
> Testing
> ---
> 
> Details at the end of this chain.
> 
> 
> Thanks,
> 
> Greg Mann
> 
>



Re: Review Request 70528: Updated release guide.

2019-07-10 Thread Benno Evers

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

(Updated July 10, 2019, 2:58 p.m.)


Review request for mesos, Greg Mann, Joseph Wu, and Till Toenshoff.


Summary (updated)
-

Updated release guide.


Repository: mesos


Description (updated)
---

Updated the release guide in `docs/release-guide.md` to match
the de-facto process we're using to release new versions.

Also mentioned some additional third-party tooling to the post-release
desription.


Diffs (updated)
-

  docs/release-guide.md a3ad2668a1953a7f20dd7209e122481ad8b30f17 


Diff: https://reviews.apache.org/r/70528/diff/3/

Changes: https://reviews.apache.org/r/70528/diff/2-3/


Testing (updated)
---

Released Mesos 1.8.0 according to this procedure.


Thanks,

Benno Evers



Re: Review Request 70993: Added warnings about known problems with libevent epoll backend.

2019-07-05 Thread Benno Evers

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

(Updated July 5, 2019, 1:54 p.m.)


Review request for mesos, Benjamin Mahler and Till Toenshoff.


Changes
---

Renamed `libprocess` -> `legacy`.


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


Repository: mesos


Description
---

Some SSL options are known to cause issues in combination with
older versions of libevent. Detect and warn about this situation.

See MESOS-9867 for details.


Diffs (updated)
-

  3rdparty/libprocess/Makefile.am 851a842114bb19abb00a318c85824067d68d71f6 
  3rdparty/libprocess/src/openssl.cpp 19d25a89f7dda1f6c66dd1ffc5051e35457d26b0 


Diff: https://reviews.apache.org/r/70993/diff/2/

Changes: https://reviews.apache.org/r/70993/diff/1-2/


Testing
---


Thanks,

Benno Evers



Re: Review Request 70921: Added OpenSSL-related changes to CHANGELOG.

2019-07-05 Thread Benno Evers


> On July 4, 2019, 3:12 p.m., Till Toenshoff wrote:
> > docs/upgrades.md
> > Lines 527 (patched)
> > <https://reviews.apache.org/r/70921/diff/2/?file=2153126#file2153126line527>
> >
> > This is really hard to parse but right now I am unable to come up with 
> > something easier for the eye -- lets try to improve this with some native 
> > speaker help.

Since all native speakers I know where unavailable due to June 4th festivities, 
I committed this as-is for now, but will make sure to follow up with one to 
improve on it later on.


- Benno


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


On July 4, 2019, 5:34 p.m., Benno Evers wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/70921/
> ---
> 
> (Updated July 4, 2019, 5:34 p.m.)
> 
> 
> Review request for mesos, Benjamin Mahler, Joseph Wu, and Till Toenshoff.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added OpenSSL-related changes to CHANGELOG.
> 
> 
> Diffs
> -
> 
>   CHANGELOG ecd01d6f415859eb6b9cfa3468db02d1810a3998 
>   docs/upgrades.md 4a818df4993093770b49efb675b8078c42144241 
> 
> 
> Diff: https://reviews.apache.org/r/70921/diff/3/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Benno Evers
> 
>



Re: Review Request 70883: Added optional 'peer_hostname' argument to Socket::connect().

2019-07-05 Thread Benno Evers


> On July 3, 2019, 3:38 p.m., Benjamin Mahler wrote:
> > Can you discard this patch in favor of the agreed upon interface?
> > 
> > This patch looks pretty small outside of the interface changes, so it 
> > should be easy to re-work into the new approach? We don't want to commit a 
> > confusing interface only to immediately rework it, so let's just go with 
> > the better interface.

Left it as is for now, as discussed over Slack.


- Benno


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


On July 2, 2019, 5:29 p.m., Benno Evers wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/70883/
> ---
> 
> (Updated July 2, 2019, 5:29 p.m.)
> 
> 
> Review request for mesos, Benjamin Mahler, Joseph Wu, and Till Toenshoff.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> The Socket::connect() function now takes an optional string
> as an additional argument. This is to prepare support for proper
> TLS hostname validation.
> 
> With TCP, a connection is always made to a specific IP address,
> with the hostname just serving as an artifact to help humans
> remember that address.
> 
> With TLS, the roles are switched: A connection is made to a
> specific hostname (which is recorded in a TLS certificate),
> with the IP address just being a network-layer artifact to
> help packets route to that hostname.
> 
> Therefore, a connecting TLS socket must be aware of the
> hostname it is supposed to connect to.
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/include/process/http.hpp 
> 029605eaff72e80206cb7dfd64c2f898084155e0 
>   3rdparty/libprocess/include/process/socket.hpp 
> 4f0f6e9aa6e95e826e3de96e518a7200ad7a8f83 
>   3rdparty/libprocess/src/http.cpp 3e73ee936f5c6329f41704a179f3d88ab65dfb6d 
>   3rdparty/libprocess/src/poll_socket.hpp 
> 15b7902ba2b10fad63e2ba7b8d5081d4b9e2d1c7 
>   3rdparty/libprocess/src/posix/libevent/libevent_ssl_socket.hpp 
> 6ef5a86566af3439cfe0b06ab3576076623f7be0 
>   3rdparty/libprocess/src/posix/libevent/libevent_ssl_socket.cpp 
> 7e2229a9ed815727500bd457356e5531607fa6cf 
>   3rdparty/libprocess/src/posix/poll_socket.cpp 
> 74acb6942682a9d9626df81b303eba0a1c24ecf7 
>   3rdparty/libprocess/src/process.cpp 
> 799666f03d6a78708aa9336c2dd04bc9b5023aa0 
>   3rdparty/libprocess/src/windows/poll_socket.cpp 
> 565b0088dc2b270193e615655f57f48419eb2c12 
> 
> 
> Diff: https://reviews.apache.org/r/70883/diff/3/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Benno Evers
> 
>



Re: Review Request 70884: Added optional 'host' string member to UPID.

2019-07-05 Thread Benno Evers


> On July 3, 2019, 4:07 p.m., Benjamin Mahler wrote:
> > Was the benchmark done with an optimized build?
> > Can you expand on the performance implications of this change in the 
> > description? (e.g. compare the min, 25th percentile, median, 75th 
> > percentile, max of at least 10 runs).

Sorry, I did not see this comment before committing. The benchmark above was 
done on my laptop, I think using an optimized build but I can't prove it 
anymore. I can repeat the benchmark on one of our build machines and record the 
results in some JIRA instead.


> On July 3, 2019, 4:07 p.m., Benjamin Mahler wrote:
> > 3rdparty/libprocess/include/process/pid.hpp
> > Lines 204 (patched)
> > <https://reviews.apache.org/r/70884/diff/4/?file=2153428#file2153428line204>
> >
> > Hm.. hostname or host?

I opted for `host` here because that's the name that was already used in the 
PID-parsing code. However, since this is an internal field there should be no 
problem updating the name if we decide something else would be more appropriate.


- Benno


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


On June 26, 2019, 1:37 p.m., Benno Evers wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/70884/
> ---
> 
> (Updated June 26, 2019, 1:37 p.m.)
> 
> 
> Review request for mesos, Benjamin Mahler, Joseph Wu, and Till Toenshoff.
> 
> 
> Bugs: MESOS-9858
> https://issues.apache.org/jira/browse/MESOS-9858
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This allows client code to access the original hostname
> that was used to specify a libprocess address.
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/include/process/pid.hpp 
> 9f09ab46fa3ceaeac09b0fbf9f532728c4ed2d7a 
>   3rdparty/libprocess/src/pid.cpp fdc61b5ab6c75b33ce33de7edd11e9302550f300 
> 
> 
> Diff: https://reviews.apache.org/r/70884/diff/4/
> 
> 
> Testing
> ---
> 
> Results from running the `Process_BENCHMARK_ThroughputPerformance` with 
> `--gtest_repeat=10`. There seems to be a slight degradation of performance on 
> average, but the benchmark runtime seems to be dominated by other factors, as 
> indicated by the large variance and presence of outliers on both sides.
> 
> 
> # Master
> 2519083.849973
> 2401163.451671
> 2322734.358626
> 2285763.292828
> 1883268.801707
> 2179436.004374
> 2071825.301714
> 2105588.897964
> 2067228.706673
> 
> 
> # Including UPID changes
> 2305142.571762
> 2194902.761484
> 2139113.533129
> 2024036.049603
> 1765327.993699
> 2172592.534808
> 2053217.096855
> 2103817.232509
> 1480080.596818
> 1808977.015807
> 
> 
> Thanks,
> 
> Benno Evers
> 
>



Re: Review Request 70991: Added ability to pass custom SSL context to `Socket::connect()`.

2019-07-05 Thread Benno Evers

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

(Updated July 5, 2019, 9:03 a.m.)


Review request for mesos, Benjamin Mahler, Joseph Wu, and Till Toenshoff.


Changes
---

Add some wording changes.


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


Repository: mesos


Description
---

Users of libprocess can now pass a custom SSL context when
connecting a generic socket via the `Socket::connect()`
function.

Additionally the API of `Socket::connect()` was also reworked
according to the following boundary conditions requested by
libprocess maintainers:

 * When libprocess is compiled without SSL support, neither the
   declaration of the TLS configuration object nor the `connnect()`
   overload that accepts the TLS configuration should be available.
 * Passing just the servername is not an acceptable short-hand for
   using the default TLS configuration together with that servername.
 * When the incorrect overload is selected (i.e. passing TLS config
   to a poll socket or omitting TLS configuration for a TLS socket),
   the program should abort.

This following changes are introduced according to the requirements
above:

 * A new class `openssl::TLSClientConfig` is introduced when libprocess
   is compiled with ssl support.
 * A new overload
   `Socket::connect(const Address&, const TLSClientConfig&)` is
   introduced when libprocess is compiled with ssl support.
 * All call sites are adjusted to check the socket kind before calling
   `connect()`.


Diffs (updated)
-

  3rdparty/libprocess/include/Makefile.am 
1ddcc2d5a30f7bf3914138e497a9b228b515cd29 
  3rdparty/libprocess/include/process/socket.hpp 
4f0f6e9aa6e95e826e3de96e518a7200ad7a8f83 
  3rdparty/libprocess/include/process/ssl/tls_config.hpp PRE-CREATION 
  3rdparty/libprocess/src/http.cpp 3e73ee936f5c6329f41704a179f3d88ab65dfb6d 
  3rdparty/libprocess/src/openssl.hpp 17bec246e516261f8d772f1647c17f092fae82d1 
  3rdparty/libprocess/src/openssl.cpp 19d25a89f7dda1f6c66dd1ffc5051e35457d26b0 
  3rdparty/libprocess/src/poll_socket.hpp 
15b7902ba2b10fad63e2ba7b8d5081d4b9e2d1c7 
  3rdparty/libprocess/src/posix/libevent/libevent_ssl_socket.hpp 
6ef5a86566af3439cfe0b06ab3576076623f7be0 
  3rdparty/libprocess/src/posix/libevent/libevent_ssl_socket.cpp 
7e2229a9ed815727500bd457356e5531607fa6cf 
  3rdparty/libprocess/src/posix/poll_socket.cpp 
74acb6942682a9d9626df81b303eba0a1c24ecf7 
  3rdparty/libprocess/src/process.cpp 799666f03d6a78708aa9336c2dd04bc9b5023aa0 
  3rdparty/libprocess/src/tests/http_tests.cpp 
4d372943a2d417d24d06444ec2e72909fb348017 
  3rdparty/libprocess/src/tests/socket_tests.cpp 
b09ae23a551c6587656b2d5f6f58c5267e8e0088 
  3rdparty/libprocess/src/tests/ssl_client.cpp 
de87b3b89c84d17f2ebba1f09e9ec682f139aace 
  3rdparty/libprocess/src/tests/ssl_tests.cpp 
5d360221937e68da185754f0633fa41a217c7107 


Diff: https://reviews.apache.org/r/70991/diff/3/

Changes: https://reviews.apache.org/r/70991/diff/2-3/


Testing
---


Thanks,

Benno Evers



Re: Review Request 70797: Added unit tests for hostname validation.

2019-07-04 Thread Benno Evers

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

(Updated July 4, 2019, 7 p.m.)


Review request for mesos, Alexander Rukletsov, Benjamin Mahler, Joseph Wu, and 
Till Toenshoff.


Changes
---

Renamed `libprocess` -> `legacy`.


Repository: mesos


Description (updated)
---

While going through the existing tests to look for candidates
that would benefit from being tested for both hostname validation
schemes, I noticed a number of existing tests where test setup
did not quite match the comment or test name. I fixed these up
in the same review.


Diffs (updated)
-

  3rdparty/libprocess/include/process/ssl/gtest.hpp 
6cdd7815f4389cd398defe56260a73eb710a4d8f 
  3rdparty/libprocess/src/tests/ssl_client.cpp 
de87b3b89c84d17f2ebba1f09e9ec682f139aace 
  3rdparty/libprocess/src/tests/ssl_tests.cpp 
5d360221937e68da185754f0633fa41a217c7107 


Diff: https://reviews.apache.org/r/70797/diff/6/

Changes: https://reviews.apache.org/r/70797/diff/5-6/


Testing
---


Thanks,

Benno Evers



Re: Review Request 70991: Added ability to pass custom SSL context to `Socket::connect()`.

2019-07-04 Thread Benno Evers

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

(Updated July 4, 2019, 7 p.m.)


Review request for mesos, Benjamin Mahler, Joseph Wu, and Till Toenshoff.


Changes
---

Addressed review comments.


Summary (updated)
-

Added ability to pass custom SSL context to `Socket::connect()`.


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


Repository: mesos


Description (updated)
---

Users of libprocess can now pass a custom SSL context when
connecting a generic socket via the `Socket::connect()`
function.

Additionally the API of `Socket::connect()` was also reworked
according to the following boundary conditions requested by
libprocess maintainers:

 * When libprocess is compiled without SSL support, neither the
   declaration of the TLS configuration object nor the `connnect()`
   overload that accepts the TLS configuration should be available.
 * Passing just the servername is not an acceptable short-hand for
   using the default TLS configuration together with that servername.
 * When the incorrect overload is selected (i.e. passing TLS config
   to a poll socket or omitting TLS configuration for a TLS socket),
   the program should abort.

This following changes are introduced according to the requirements
above:

 * A new class `openssl::TLSClientConfig` is introduced when libprocess
   is compiled with ssl support.
 * A new overload
   `Socket::connect(const Address&, const TLSClientConfig&)` is
   introduced when libprocess is compiled with ssl support.
 * All call sites are adjusted to check the socket kind before calling
   `connect()`.


Diffs (updated)
-

  3rdparty/libprocess/include/Makefile.am 
1ddcc2d5a30f7bf3914138e497a9b228b515cd29 
  3rdparty/libprocess/include/process/socket.hpp 
4f0f6e9aa6e95e826e3de96e518a7200ad7a8f83 
  3rdparty/libprocess/include/process/ssl/tls_config.hpp PRE-CREATION 
  3rdparty/libprocess/src/http.cpp 3e73ee936f5c6329f41704a179f3d88ab65dfb6d 
  3rdparty/libprocess/src/openssl.hpp 17bec246e516261f8d772f1647c17f092fae82d1 
  3rdparty/libprocess/src/openssl.cpp 19d25a89f7dda1f6c66dd1ffc5051e35457d26b0 
  3rdparty/libprocess/src/poll_socket.hpp 
15b7902ba2b10fad63e2ba7b8d5081d4b9e2d1c7 
  3rdparty/libprocess/src/posix/libevent/libevent_ssl_socket.hpp 
6ef5a86566af3439cfe0b06ab3576076623f7be0 
  3rdparty/libprocess/src/posix/libevent/libevent_ssl_socket.cpp 
7e2229a9ed815727500bd457356e5531607fa6cf 
  3rdparty/libprocess/src/posix/poll_socket.cpp 
74acb6942682a9d9626df81b303eba0a1c24ecf7 
  3rdparty/libprocess/src/process.cpp 799666f03d6a78708aa9336c2dd04bc9b5023aa0 
  3rdparty/libprocess/src/tests/http_tests.cpp 
4d372943a2d417d24d06444ec2e72909fb348017 
  3rdparty/libprocess/src/tests/socket_tests.cpp 
b09ae23a551c6587656b2d5f6f58c5267e8e0088 
  3rdparty/libprocess/src/tests/ssl_client.cpp 
de87b3b89c84d17f2ebba1f09e9ec682f139aace 
  3rdparty/libprocess/src/tests/ssl_tests.cpp 
5d360221937e68da185754f0633fa41a217c7107 


Diff: https://reviews.apache.org/r/70991/diff/2/

Changes: https://reviews.apache.org/r/70991/diff/1-2/


Testing
---


Thanks,

Benno Evers



Re: Review Request 70795: Updated SSL docs to include new libprocess flag.

2019-07-04 Thread Benno Evers

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

(Updated July 4, 2019, 6:58 p.m.)


Review request for mesos, Alexander Rukletsov, Benjamin Mahler, Jan-Philip 
Gehrcke, Joseph Wu, and Till Toenshoff.


Changes
---

Renamed `libprocess` -> `legacy`.


Repository: mesos


Description
---

Added a description of the new `--hostname_validation_scheme` flag
and corresponding `LIBPROCESS_SSL_HOSTNAME_VALIDATION_SCHEME`
environment variable.


Diffs (updated)
-

  docs/ssl.md ce5058896144aa7824986d40d996899d92cb7c1c 


Diff: https://reviews.apache.org/r/70795/diff/5/

Changes: https://reviews.apache.org/r/70795/diff/4-5/


Testing
---


Thanks,

Benno Evers



Re: Review Request 70749: Introduced RFC6125-compliant hostname validation scheme.

2019-07-04 Thread Benno Evers

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

(Updated July 4, 2019, 6:52 p.m.)


Review request for mesos, Alexander Rukletsov, Benjamin Mahler, Joseph Wu, and 
Till Toenshoff.


Changes
---

Renamed 'libprocess' -> 'legacy'


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


Repository: mesos


Description (updated)
---

This commit introduces a new libprocess SSL flag
`hostname_validation_scheme`, which can be set to 'legacy'
to select the previous hostname validation behaviour or to
'openssl' to use standardized OpenSSL algorithms to handle
hostname validation as part of the TLS handshake.

As a nice side-effect, the new scheme gets rid of reverse DNS
lookups during TLS connection establishment, which used to be
a common source of hard-to-debug unresponsiveness in Mesos
components.

See `docs/ssl.md` in the follow-up commit for details of and
differences between the schemes.


Diffs (updated)
-

  3rdparty/libprocess/include/process/ssl/flags.hpp 
f3483f97f93bb29117b2c78f0f2ed9735d9c4b3a 
  3rdparty/libprocess/src/openssl.hpp 17bec246e516261f8d772f1647c17f092fae82d1 
  3rdparty/libprocess/src/openssl.cpp 19d25a89f7dda1f6c66dd1ffc5051e35457d26b0 
  3rdparty/libprocess/src/posix/libevent/libevent_ssl_socket.hpp 
6ef5a86566af3439cfe0b06ab3576076623f7be0 
  3rdparty/libprocess/src/posix/libevent/libevent_ssl_socket.cpp 
7e2229a9ed815727500bd457356e5531607fa6cf 


Diff: https://reviews.apache.org/r/70749/diff/9/

Changes: https://reviews.apache.org/r/70749/diff/8-9/


Testing
---

See added unit tests later in this chain.


Thanks,

Benno Evers



Re: Review Request 70921: Added OpenSSL-related changes to CHANGELOG.

2019-07-04 Thread Benno Evers

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

(Updated July 4, 2019, 5:34 p.m.)


Review request for mesos, Benjamin Mahler, Joseph Wu, and Till Toenshoff.


Repository: mesos


Description
---

Added OpenSSL-related changes to CHANGELOG.


Diffs (updated)
-

  CHANGELOG ecd01d6f415859eb6b9cfa3468db02d1810a3998 
  docs/upgrades.md 4a818df4993093770b49efb675b8078c42144241 


Diff: https://reviews.apache.org/r/70921/diff/3/

Changes: https://reviews.apache.org/r/70921/diff/2-3/


Testing
---


Thanks,

Benno Evers



Re: Review Request 70992: Recorded Socket API change in CHANGELOG.

2019-07-04 Thread Benno Evers

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

(Updated July 4, 2019, 2:28 p.m.)


Review request for mesos, Benjamin Mahler, Joseph Wu, and Till Toenshoff.


Summary (updated)
-

Recorded Socket API change in CHANGELOG.


Repository: mesos


Description (updated)
---

Recorded Socket API change in CHANGELOG.


Diffs (updated)
-

  CHANGELOG ecd01d6f415859eb6b9cfa3468db02d1810a3998 


Diff: https://reviews.apache.org/r/70992/diff/2/

Changes: https://reviews.apache.org/r/70992/diff/1-2/


Testing
---


Thanks,

Benno Evers



Re: Review Request 70748: Changed semantics of TLS certificate verification flags.

2019-07-02 Thread Benno Evers


> On May 31, 2019, 3:05 p.m., Alexander Rukletsov wrote:
> > 3rdparty/libprocess/src/tests/ssl_tests.cpp
> > Lines 287-290 (patched)
> > <https://reviews.apache.org/r/70748/diff/1/?file=2147040#file2147040line287>
> >
> > Can we also test that a client must *not* present a cert? I believe the 
> > answer is _no_, but a TODO or a NOTE could be helpful.
> 
> Benno Evers wrote:
> I guess we could let `curl` make a request against a server with 
> `verify_cert=true`, but that seems unrelated to the change in this review, so 
> it should probably become a separate review later in the chain.

Created https://issues.apache.org/jira/browse/MESOS-9879 for tracking this.


- Benno


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


On July 2, 2019, 5:28 p.m., Benno Evers wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/70748/
> ---
> 
> (Updated July 2, 2019, 5:28 p.m.)
> 
> 
> Review request for mesos, Alexander Rukletsov, Benjamin Mahler, Jan-Philip 
> Gehrcke, Joseph Wu, and Till Toenshoff.
> 
> 
> Bugs: MESOS-9810
> https://issues.apache.org/jira/browse/MESOS-9810
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This commit slightly updates the semants of the
> `LIBPROCESS_SSL_VERIFY_CERT` and `LIBPROCESS_SSL_REQUIRE_CERT`
> environment variables. The former now only applies to connections
> in client mode and the latter now only applies to connections in
> server mode.
> 
> In particular, in TLS server mode we now *only* verify client
> certificates when `LIBPROCESS_SSL_REQUIRE_CERT` is set to `true`,
> regardless of the value of `LIBPROCESS_SSL_VERIFY_CERT`.
> 
> In addtion, when in SSL client mode and  `LIBPROCESS_SSL_VERIFY_CERT`
> has been set to `true`, enforce that the server actually presents a
> certificate that can be verified. Note that this is expected to be
> not a behavioural change in practice, since the TLS specification
> already states that a server MUST always send a certificate unless an
> anonymous cipher is used, and most TLS ciphersuites are configured to
> exclude anonymous ciphers.
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/src/openssl.hpp 
> 17bec246e516261f8d772f1647c17f092fae82d1 
>   3rdparty/libprocess/src/openssl.cpp 
> 19d25a89f7dda1f6c66dd1ffc5051e35457d26b0 
>   3rdparty/libprocess/src/posix/libevent/libevent_ssl_socket.cpp 
> 7e2229a9ed815727500bd457356e5531607fa6cf 
>   3rdparty/libprocess/src/tests/ssl_tests.cpp 
> 5d360221937e68da185754f0633fa41a217c7107 
> 
> 
> Diff: https://reviews.apache.org/r/70748/diff/9/
> 
> 
> Testing
> ---
> 
> See end of this chain.
> 
> 
> Thanks,
> 
> Benno Evers
> 
>



Re: Review Request 70797: Added unit tests for hostname validation.

2019-07-02 Thread Benno Evers

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

(Updated July 2, 2019, 5:53 p.m.)


Review request for mesos, Alexander Rukletsov, Benjamin Mahler, Joseph Wu, and 
Till Toenshoff.


Repository: mesos


Description (updated)
---

This commit adds some unit tests to verify the newly added 
`hostname_validation_scheme` flag is working as intended.

While going through the existing tests to look for candidates
that would benefit from being tested for both hostname validation
schemes, I noticed a number of existing tests where test setup
did not quite match the comment or test name. I fixed these up
in this review as well.


Diffs (updated)
-

  3rdparty/libprocess/include/process/ssl/gtest.hpp 
6cdd7815f4389cd398defe56260a73eb710a4d8f 
  3rdparty/libprocess/src/tests/ssl_client.cpp 
de87b3b89c84d17f2ebba1f09e9ec682f139aace 
  3rdparty/libprocess/src/tests/ssl_tests.cpp 
5d360221937e68da185754f0633fa41a217c7107 


Diff: https://reviews.apache.org/r/70797/diff/5/

Changes: https://reviews.apache.org/r/70797/diff/4-5/


Testing
---


Thanks,

Benno Evers



Review Request 70993: Added warnings about known problems with libevent epoll backend.

2019-07-02 Thread Benno Evers

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

Review request for mesos, Benjamin Mahler and Till Toenshoff.


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


Repository: mesos


Description
---

Some SSL options are known to cause issues in combination with
older versions of libevent. Detect and warn about this situation.

See MESOS-9867 for details.


Diffs
-

  3rdparty/libprocess/Makefile.am 851a842114bb19abb00a318c85824067d68d71f6 
  3rdparty/libprocess/src/openssl.cpp 19d25a89f7dda1f6c66dd1ffc5051e35457d26b0 


Diff: https://reviews.apache.org/r/70993/diff/1/


Testing
---


Thanks,

Benno Evers



Review Request 70992: Record Socket API change in CHANGELOG.

2019-07-02 Thread Benno Evers

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

Review request for mesos, Benjamin Mahler, Joseph Wu, and Till Toenshoff.


Repository: mesos


Description
---

Record Socket API change in CHANGELOG.


Diffs
-

  CHANGELOG ecd01d6f415859eb6b9cfa3468db02d1810a3998 


Diff: https://reviews.apache.org/r/70992/diff/1/


Testing
---


Thanks,

Benno Evers



Review Request 70991: Updated `Socket::connect()` API according to maintainer feedback.

2019-07-02 Thread Benno Evers

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

Review request for mesos, Benjamin Mahler, Joseph Wu, and Till Toenshoff.


Repository: mesos


Description
---

Reworked the API of `Socket::connect()` according to the following
boundary conditions requested by libprocess maintainers:

 * It shall be possible to use custom connection options when
   connecting with a SSL socket.
 * When libprocess is compiled without SSL support, neither the
   declaration of the TLS configuration object nor the `connnect()`
   overload that accepts the TLS configuration should be available.
 * Passing just the servername is not an acceptable short-hand for
   using the default TLS configuration together with that servername.
 * When the incorrect overload is selected (i.e. passing TLS config
   to a poll socket or omitting TLS configuration for a TLS socket),
   the program should abort.

This commit changes the API of `Socket::connect()` according to the
requirements above. In particular:

 * A new class `openssl::TLSClientConfig` is introduced when libprocess
   is compiled with ssl support.
 * A new overload
   `Socket::connect(const Address&, const TLSClientConfig&)` is
   introduced when libprocess is compiled with ssl support.
 * All call sites are adjusted to check the socket kind before calling
   `connect()`.


Diffs
-

  3rdparty/libprocess/include/Makefile.am 
1ddcc2d5a30f7bf3914138e497a9b228b515cd29 
  3rdparty/libprocess/include/process/socket.hpp 
4f0f6e9aa6e95e826e3de96e518a7200ad7a8f83 
  3rdparty/libprocess/include/process/ssl/tls_config.hpp PRE-CREATION 
  3rdparty/libprocess/src/http.cpp 3e73ee936f5c6329f41704a179f3d88ab65dfb6d 
  3rdparty/libprocess/src/openssl.hpp 17bec246e516261f8d772f1647c17f092fae82d1 
  3rdparty/libprocess/src/openssl.cpp 19d25a89f7dda1f6c66dd1ffc5051e35457d26b0 
  3rdparty/libprocess/src/poll_socket.hpp 
15b7902ba2b10fad63e2ba7b8d5081d4b9e2d1c7 
  3rdparty/libprocess/src/posix/libevent/libevent_ssl_socket.hpp 
6ef5a86566af3439cfe0b06ab3576076623f7be0 
  3rdparty/libprocess/src/posix/libevent/libevent_ssl_socket.cpp 
7e2229a9ed815727500bd457356e5531607fa6cf 
  3rdparty/libprocess/src/posix/poll_socket.cpp 
74acb6942682a9d9626df81b303eba0a1c24ecf7 
  3rdparty/libprocess/src/process.cpp 799666f03d6a78708aa9336c2dd04bc9b5023aa0 
  3rdparty/libprocess/src/tests/http_tests.cpp 
4d372943a2d417d24d06444ec2e72909fb348017 
  3rdparty/libprocess/src/tests/socket_tests.cpp 
b09ae23a551c6587656b2d5f6f58c5267e8e0088 
  3rdparty/libprocess/src/tests/ssl_client.cpp 
de87b3b89c84d17f2ebba1f09e9ec682f139aace 
  3rdparty/libprocess/src/tests/ssl_tests.cpp 
5d360221937e68da185754f0633fa41a217c7107 


Diff: https://reviews.apache.org/r/70991/diff/1/


Testing
---


Thanks,

Benno Evers



Re: Review Request 70749: Introduced RFC6125-compliant hostname validation scheme.

2019-07-02 Thread Benno Evers

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

(Updated July 2, 2019, 5:49 p.m.)


Review request for mesos, Alexander Rukletsov, Benjamin Mahler, Joseph Wu, and 
Till Toenshoff.


Changes
---

Rebased onto latest master; changed title.


Summary (updated)
-

Introduced RFC6125-compliant hostname validation scheme.


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


Repository: mesos


Description
---

This commit introduces a new libprocess SSL flag
`hostname_validation_scheme`, which can be used to select
between the previous hostname validation behaviour and a new
option to use standardized OpenSSL algorithms to handle
hostname validation as part of the

As a nice side-effect, the new scheme gets rid of reverse DNS
lookups during TLS connection establishment, which used to be
a common source of hard-to-debug unresponsiveness in Mesos
components.

See `docs/ssl.md` in the follow-up commit for details of and
differences between the schemes.


Diffs (updated)
-

  3rdparty/libprocess/include/process/ssl/flags.hpp 
f3483f97f93bb29117b2c78f0f2ed9735d9c4b3a 
  3rdparty/libprocess/src/openssl.hpp 17bec246e516261f8d772f1647c17f092fae82d1 
  3rdparty/libprocess/src/openssl.cpp 19d25a89f7dda1f6c66dd1ffc5051e35457d26b0 
  3rdparty/libprocess/src/posix/libevent/libevent_ssl_socket.hpp 
6ef5a86566af3439cfe0b06ab3576076623f7be0 
  3rdparty/libprocess/src/posix/libevent/libevent_ssl_socket.cpp 
7e2229a9ed815727500bd457356e5531607fa6cf 


Diff: https://reviews.apache.org/r/70749/diff/8/

Changes: https://reviews.apache.org/r/70749/diff/7-8/


Testing (updated)
---

See added unit tests later in this chain.


Thanks,

Benno Evers



Re: Review Request 70883: Added optional 'peer_hostname' argument to Socket::connect().

2019-07-02 Thread Benno Evers

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

(Updated July 2, 2019, 5:29 p.m.)


Review request for mesos, Benjamin Mahler, Joseph Wu, and Till Toenshoff.


Changes
---

Passed missing hostname in `send()`.


Repository: mesos


Description
---

The Socket::connect() function now takes an optional string
as an additional argument. This is to prepare support for proper
TLS hostname validation.

With TCP, a connection is always made to a specific IP address,
with the hostname just serving as an artifact to help humans
remember that address.

With TLS, the roles are switched: A connection is made to a
specific hostname (which is recorded in a TLS certificate),
with the IP address just being a network-layer artifact to
help packets route to that hostname.

Therefore, a connecting TLS socket must be aware of the
hostname it is supposed to connect to.


Diffs (updated)
-

  3rdparty/libprocess/include/process/http.hpp 
029605eaff72e80206cb7dfd64c2f898084155e0 
  3rdparty/libprocess/include/process/socket.hpp 
4f0f6e9aa6e95e826e3de96e518a7200ad7a8f83 
  3rdparty/libprocess/src/http.cpp 3e73ee936f5c6329f41704a179f3d88ab65dfb6d 
  3rdparty/libprocess/src/poll_socket.hpp 
15b7902ba2b10fad63e2ba7b8d5081d4b9e2d1c7 
  3rdparty/libprocess/src/posix/libevent/libevent_ssl_socket.hpp 
6ef5a86566af3439cfe0b06ab3576076623f7be0 
  3rdparty/libprocess/src/posix/libevent/libevent_ssl_socket.cpp 
7e2229a9ed815727500bd457356e5531607fa6cf 
  3rdparty/libprocess/src/posix/poll_socket.cpp 
74acb6942682a9d9626df81b303eba0a1c24ecf7 
  3rdparty/libprocess/src/process.cpp 799666f03d6a78708aa9336c2dd04bc9b5023aa0 
  3rdparty/libprocess/src/windows/poll_socket.cpp 
565b0088dc2b270193e615655f57f48419eb2c12 


Diff: https://reviews.apache.org/r/70883/diff/2/

Changes: https://reviews.apache.org/r/70883/diff/1-2/


Testing
---


Thanks,

Benno Evers



Re: Review Request 70748: Changed semantics of TLS certificate verification flags.

2019-07-02 Thread Benno Evers

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

(Updated July 2, 2019, 5:28 p.m.)


Review request for mesos, Alexander Rukletsov, Benjamin Mahler, Jan-Philip 
Gehrcke, Joseph Wu, and Till Toenshoff.


Changes
---

Rebased on master; moved a message to prevent illogical warnings in logs.


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


Repository: mesos


Description
---

This commit slightly updates the semants of the
`LIBPROCESS_SSL_VERIFY_CERT` and `LIBPROCESS_SSL_REQUIRE_CERT`
environment variables. The former now only applies to connections
in client mode and the latter now only applies to connections in
server mode.

In particular, in TLS server mode we now *only* verify client
certificates when `LIBPROCESS_SSL_REQUIRE_CERT` is set to `true`,
regardless of the value of `LIBPROCESS_SSL_VERIFY_CERT`.

In addtion, when in SSL client mode and  `LIBPROCESS_SSL_VERIFY_CERT`
has been set to `true`, enforce that the server actually presents a
certificate that can be verified. Note that this is expected to be
not a behavioural change in practice, since the TLS specification
already states that a server MUST always send a certificate unless an
anonymous cipher is used, and most TLS ciphersuites are configured to
exclude anonymous ciphers.


Diffs (updated)
-

  3rdparty/libprocess/src/openssl.hpp 17bec246e516261f8d772f1647c17f092fae82d1 
  3rdparty/libprocess/src/openssl.cpp 19d25a89f7dda1f6c66dd1ffc5051e35457d26b0 
  3rdparty/libprocess/src/posix/libevent/libevent_ssl_socket.cpp 
7e2229a9ed815727500bd457356e5531607fa6cf 
  3rdparty/libprocess/src/tests/ssl_tests.cpp 
5d360221937e68da185754f0633fa41a217c7107 


Diff: https://reviews.apache.org/r/70748/diff/9/

Changes: https://reviews.apache.org/r/70748/diff/8-9/


Testing
---

See end of this chain.


Thanks,

Benno Evers



Re: Review Request 70971: Bumped mesos-tidy to 8.0.0.

2019-06-28 Thread Benno Evers

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


Ship it!




Ship It!

- Benno Evers


On June 28, 2019, 1:37 p.m., Benjamin Bannier wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/70971/
> ---
> 
> (Updated June 28, 2019, 1:37 p.m.)
> 
> 
> Review request for mesos and Benno Evers.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Bumped mesos-tidy to 8.0.0.
> 
> 
> Diffs
> -
> 
>   support/mesos-tidy/Dockerfile c6f9d83b0c2bdf239c34f584cf521c08a6526efc 
> 
> 
> Diff: https://reviews.apache.org/r/70971/diff/1/
> 
> 
> Testing
> ---
> 
> With another patch to use a locally built image:
> 
> * `CMAKE_ARGS='-DENABLE_LIBEVENT=ON -DENABLE_SSL=ON' ./support/mesos-tidy.sh`
> * `CMAKE_ARGS='-DENABLE_LIBEVENT=OFF -DENABLE_SSL=OFF' 
> ./support/mesos-tidy.sh`
> 
> 
> Thanks,
> 
> Benjamin Bannier
> 
>



Re: Review Request 70970: Deactivated clang-tidy check `clang-analyzer-cplusplus.Move`.

2019-06-28 Thread Benno Evers

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


Ship it!




Ship It!

- Benno Evers


On June 28, 2019, 1:37 p.m., Benjamin Bannier wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/70970/
> ---
> 
> (Updated June 28, 2019, 1:37 p.m.)
> 
> 
> Review request for mesos and Benno Evers.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This check which will be enabled by default with upstreams 8.0.0 release
> has false positives in our codebase, see
> https://bugs.llvm.org/show_bug.cgi?id=42433.
> 
> 
> Diffs
> -
> 
>   support/clang-tidy 8cb1662104e5cd41e62c9d573b33df55bc6c5d07 
> 
> 
> Diff: https://reviews.apache.org/r/70970/diff/1/
> 
> 
> Testing
> ---
> 
> `./support/mesos-tidy.sh`
> 
> 
> Thanks,
> 
> Benjamin Bannier
> 
>



Re: Review Request 70884: Added optional 'host' string member to UPID.

2019-06-26 Thread Benno Evers

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

(Updated June 26, 2019, 1:37 p.m.)


Review request for mesos, Benjamin Mahler, Joseph Wu, and Till Toenshoff.


Changes
---

Added benchmark results.


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


Repository: mesos


Description
---

This allows client code to access the original hostname
that was used to specify a libprocess address.


Diffs
-

  3rdparty/libprocess/include/process/pid.hpp 
9f09ab46fa3ceaeac09b0fbf9f532728c4ed2d7a 
  3rdparty/libprocess/src/pid.cpp fdc61b5ab6c75b33ce33de7edd11e9302550f300 
  3rdparty/libprocess/src/process.cpp 799666f03d6a78708aa9336c2dd04bc9b5023aa0 


Diff: https://reviews.apache.org/r/70884/diff/3/


Testing (updated)
---

Results from running the `Process_BENCHMARK_ThroughputPerformance` with 
`--gtest_repeat=10`. There seems to be a slight degradation of performance on 
average, but the benchmark runtime seems to be dominated by other factors, as 
indicated by the large variance and presence of outliers on both sides.


# Master
2519083.849973
2401163.451671
2322734.358626
2285763.292828
1883268.801707
2179436.004374
2071825.301714
2105588.897964
2067228.706673


# Including UPID changes
2305142.571762
2194902.761484
2139113.533129
2024036.049603
1765327.993699
2172592.534808
2053217.096855
2103817.232509
1480080.596818
1808977.015807


Thanks,

Benno Evers



Re: Review Request 70748: Changed semantics of TLS certificate verification flags.

2019-06-26 Thread Benno Evers

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

(Updated June 26, 2019, 10:26 a.m.)


Review request for mesos, Alexander Rukletsov, Jan-Philip Gehrcke, Joseph Wu, 
and Till Toenshoff.


Changes
---

Adjusted info messages to new semantics.


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


Repository: mesos


Description
---

This commit slightly updates the semants of the
`LIBPROCESS_SSL_VERIFY_CERT` and `LIBPROCESS_SSL_REQUIRE_CERT`
environment variables. The former now only applies to connections
in client mode and the latter now only applies to connections in
server mode.

In particular, in TLS server mode we now *only* verify client
certificates when `LIBPROCESS_SSL_REQUIRE_CERT` is set to `true`,
regardless of the value of `LIBPROCESS_SSL_VERIFY_CERT`.

In addtion, when in SSL client mode and  `LIBPROCESS_SSL_VERIFY_CERT`
has been set to `true`, enforce that the server actually presents a
certificate that can be verified. Note that this is expected to be
not a behavioural change in practice, since the TLS specification
already states that a server MUST always send a certificate unless an
anonymous cipher is used, and most TLS ciphersuites are configured to
exclude anonymous ciphers.


Diffs (updated)
-

  3rdparty/libprocess/src/openssl.hpp 17bec246e516261f8d772f1647c17f092fae82d1 
  3rdparty/libprocess/src/openssl.cpp e7dbd67913fa8e7fbbf60dee428e7e38895f86ce 
  3rdparty/libprocess/src/posix/libevent/libevent_ssl_socket.cpp 
29a1bf71c1df9d80370455a6269ecea0ec4193b0 
  3rdparty/libprocess/src/tests/ssl_tests.cpp 
6b8496aeeed79ae1bd39d7013f4f403b248fdd4c 


Diff: https://reviews.apache.org/r/70748/diff/8/

Changes: https://reviews.apache.org/r/70748/diff/7-8/


Testing (updated)
---

See end of this chain.


Thanks,

Benno Evers



Re: Review Request 70797: Added unit tests for hostname validation.

2019-06-25 Thread Benno Evers

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

(Updated June 26, 2019, 12:18 a.m.)


Review request for mesos, Alexander Rukletsov, Joseph Wu, and Till Toenshoff.


Summary (updated)
-

Added unit tests for hostname validation.


Repository: mesos


Description (updated)
---

While going through the existing tests to look for candidates
that would benefit from being tested for both hostname validation
schemes, I noticed a number of existing tests where test setup
did not quite match the comment or test name. I fixed these up
in the same review.


Diffs (updated)
-

  3rdparty/libprocess/src/tests/ssl_tests.cpp 
6b8496aeeed79ae1bd39d7013f4f403b248fdd4c 


Diff: https://reviews.apache.org/r/70797/diff/4/

Changes: https://reviews.apache.org/r/70797/diff/3-4/


Testing
---


Thanks,

Benno Evers



Re: Review Request 70933: Moved an inline duration for slow DNS logging into a const variable.

2019-06-25 Thread Benno Evers


> On June 24, 2019, 4:50 p.m., Benjamin Mahler wrote:
> > The commit summary "Refactored raw duration into const variable." could be 
> > clearer, how about:
> > 
> > ```
> > Moved an inline duration for slow DNS logging into a const variable.
> > ```
> > 
> > With this I can immediately understand the change from the commit summary, 
> > I wouldn't need to read the description / diff to get a sense of the change.

Sounds good, updated.


- Benno


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


On June 24, 2019, 12:22 p.m., Benno Evers wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/70933/
> ---
> 
> (Updated June 24, 2019, 12:22 p.m.)
> 
> 
> Review request for mesos, Benjamin Mahler and Till Toenshoff.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Moved the 100ms threshold that was used for printing warning
> messages about slow reverse DNS lookups into a named variable.
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/src/openssl.cpp 
> e7dbd67913fa8e7fbbf60dee428e7e38895f86ce 
> 
> 
> Diff: https://reviews.apache.org/r/70933/diff/1/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Benno Evers
> 
>



Re: Review Request 70884: Added optional 'host' string member to UPID.

2019-06-24 Thread Benno Evers

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

(Updated June 24, 2019, 1:48 p.m.)


Review request for mesos, Benjamin Mahler, Joseph Wu, and Till Toenshoff.


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


Repository: mesos


Description
---

This allows client code to access the original hostname
that was used to specify a libprocess address.


Diffs (updated)
-

  3rdparty/libprocess/include/process/pid.hpp 
9f09ab46fa3ceaeac09b0fbf9f532728c4ed2d7a 
  3rdparty/libprocess/src/pid.cpp fdc61b5ab6c75b33ce33de7edd11e9302550f300 
  3rdparty/libprocess/src/process.cpp 799666f03d6a78708aa9336c2dd04bc9b5023aa0 


Diff: https://reviews.apache.org/r/70884/diff/3/

Changes: https://reviews.apache.org/r/70884/diff/2-3/


Testing
---


Thanks,

Benno Evers



Re: Review Request 70795: Updated SSL docs to include new libprocess flag.

2019-06-24 Thread Benno Evers


> On June 21, 2019, 1:34 a.m., Till Toenshoff wrote:
> > docs/ssl.md
> > Lines 194 (patched)
> > <https://reviews.apache.org/r/70795/diff/3/?file=2151430#file2151430line194>
> >
> > I wonder if we should already start a deprecation of the `libprocess` 
> > scheme - that would be:
> > - announcing that `openssl` will be standard soon on compatible boxes
> > - announcing it to be gone at some point
> >     
> > Or am I too eager for unification here?
> 
> Benno Evers wrote:
> It's actually a pretty big change - the 'libprocess' behaviour was built, 
> I assume, to "magically" work with normal certificates w/o IP addresses 
> despite libprocess only knowing about IP addresses. In DC/OS we don't notice 
> most of it, since there all our certificates *do* contain the correct IP 
> address, but at least quite a few unit tests will break by switching the 
> default.
> 
> So I actually agree we should do this deprecation, but I'm not sure about 
> the timeline.

Created https://issues.apache.org/jira/browse/MESOS-9857 to track the change.


- Benno


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


On June 21, 2019, 3:05 p.m., Benno Evers wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/70795/
> ---
> 
> (Updated June 21, 2019, 3:05 p.m.)
> 
> 
> Review request for mesos, Alexander Rukletsov, Jan-Philip Gehrcke, Joseph Wu, 
> and Till Toenshoff.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added a description of the new `--hostname_validation_scheme` flag
> and corresponding `LIBPROCESS_SSL_HOSTNAME_VALIDATION_SCHEME`
> environment variable.
> 
> 
> Diffs
> -
> 
>   docs/ssl.md ce5058896144aa7824986d40d996899d92cb7c1c 
> 
> 
> Diff: https://reviews.apache.org/r/70795/diff/4/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Benno Evers
> 
>



Re: Review Request 70749: Introduced optional new scheme for hostname validation.

2019-06-24 Thread Benno Evers


> On June 21, 2019, 1:32 a.m., Till Toenshoff wrote:
> > 3rdparty/libprocess/src/openssl.cpp
> > Lines 808 (patched)
> > <https://reviews.apache.org/r/70749/diff/4/?file=2151450#file2151450line808>
> >
> > I know you just moved it, but where do these 100ms come from and how 
> > could we be more explicit about that choice?
> > 
> > I would suggest to use a const with some explaining comment how that 
> > value was chosen - can we please? :D
> 
> Benno Evers wrote:
> I'm afraid I have no idea where the 100ms come from. The suggestion 
> sounds good, but I think the changes will fit better in a separate review, 
> since they're not really related to hostname validation.

Fixed in https://reviews.apache.org/r/70933/


- Benno


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


On June 21, 2019, 3:32 p.m., Benno Evers wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/70749/
> ---
> 
> (Updated June 21, 2019, 3:32 p.m.)
> 
> 
> Review request for mesos, Alexander Rukletsov, Joseph Wu, and Till Toenshoff.
> 
> 
> Bugs: MESOS-9809
> https://issues.apache.org/jira/browse/MESOS-9809
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This commit introduces a new libprocess SSL flag
> `hostname_validation_scheme`, which can be used to select
> between the previous hostname validation behaviour and a new
> option to use standardized OpenSSL algorithms to handle
> hostname validation as part of the
> 
> As a nice side-effect, the new scheme gets rid of reverse DNS
> lookups during TLS connection establishment, which used to be
> a common source of hard-to-debug unresponsiveness in Mesos
> components.
> 
> See `docs/ssl.md` in the follow-up commit for details of and
> differences between the schemes.
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/include/process/ssl/flags.hpp 
> f3483f97f93bb29117b2c78f0f2ed9735d9c4b3a 
>   3rdparty/libprocess/src/openssl.hpp 
> 17bec246e516261f8d772f1647c17f092fae82d1 
>   3rdparty/libprocess/src/openssl.cpp 
> e7dbd67913fa8e7fbbf60dee428e7e38895f86ce 
>   3rdparty/libprocess/src/posix/libevent/libevent_ssl_socket.hpp 
> 6ef5a86566af3439cfe0b06ab3576076623f7be0 
>   3rdparty/libprocess/src/posix/libevent/libevent_ssl_socket.cpp 
> 29a1bf71c1df9d80370455a6269ecea0ec4193b0 
> 
> 
> Diff: https://reviews.apache.org/r/70749/diff/5/
> 
> 
> Testing
> ---
> 
> Todo!
> 
> 
> Thanks,
> 
> Benno Evers
> 
>



Review Request 70933: Refactored raw duration into const variable.

2019-06-24 Thread Benno Evers

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

Review request for mesos, Benjamin Mahler and Till Toenshoff.


Repository: mesos


Description
---

Moved the 100ms threshold that was used for printing warning
messages about slow reverse DNS lookups into a named variable.


Diffs
-

  3rdparty/libprocess/src/openssl.cpp e7dbd67913fa8e7fbbf60dee428e7e38895f86ce 


Diff: https://reviews.apache.org/r/70933/diff/1/


Testing
---


Thanks,

Benno Evers



Re: Review Request 70884: Added optional 'host' string member to UPID.

2019-06-21 Thread Benno Evers

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




3rdparty/libprocess/src/pid.cpp
Lines 74 (patched)
<https://reviews.apache.org/r/70884/#comment303056>

Note: I'm a bit unsure about this. On the one hand, it feels super-useful 
to print UPID's the same way they were received, and I've been annoyed more 
often than not that I have to manually resolve an IP address from some Mesos 
log.

On the other hand, this can lead to slightly confusing messages like:
```
W0622 00:09:43.911963  5825 slave.cpp:1584] Ignoring re-registration 
message from master@127.0.1.1:5050 because it is not the expected master: 
master@localhost:5050

```
where it's not immediately obvious that `localhost` is actually ignored by 
the code in question and the problem is actually that it resolved to 
`127.0.0.1` and not `127.0.1.1`.


- Benno Evers


On June 21, 2019, 10:25 p.m., Benno Evers wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/70884/
> ---
> 
> (Updated June 21, 2019, 10:25 p.m.)
> 
> 
> Review request for mesos, Joseph Wu and Till Toenshoff.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This allows client code to access the original hostname
> that was used to specify a libprocess address.
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/include/process/pid.hpp 
> 9f09ab46fa3ceaeac09b0fbf9f532728c4ed2d7a 
>   3rdparty/libprocess/src/pid.cpp fdc61b5ab6c75b33ce33de7edd11e9302550f300 
>   3rdparty/libprocess/src/process.cpp 
> 799666f03d6a78708aa9336c2dd04bc9b5023aa0 
> 
> 
> Diff: https://reviews.apache.org/r/70884/diff/2/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Benno Evers
> 
>



Re: Review Request 70884: Added optional 'host' string member to UPID.

2019-06-21 Thread Benno Evers

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

(Updated June 21, 2019, 10:25 p.m.)


Review request for mesos, Joseph Wu and Till Toenshoff.


Summary (updated)
-

Added optional 'host' string member to UPID.


Repository: mesos


Description (updated)
---

This allows client code to access the original hostname
that was used to specify a libprocess address.


Diffs (updated)
-

  3rdparty/libprocess/include/process/pid.hpp 
9f09ab46fa3ceaeac09b0fbf9f532728c4ed2d7a 
  3rdparty/libprocess/src/pid.cpp fdc61b5ab6c75b33ce33de7edd11e9302550f300 
  3rdparty/libprocess/src/process.cpp 799666f03d6a78708aa9336c2dd04bc9b5023aa0 


Diff: https://reviews.apache.org/r/70884/diff/2/

Changes: https://reviews.apache.org/r/70884/diff/1-2/


Testing
---


Thanks,

Benno Evers



Re: Review Request 70749: Introduced optional new scheme for hostname validation.

2019-06-21 Thread Benno Evers

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

(Updated June 21, 2019, 3:32 p.m.)


Review request for mesos, Alexander Rukletsov, Joseph Wu, and Till Toenshoff.


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


Repository: mesos


Description (updated)
---

This commit introduces a new libprocess SSL flag
`hostname_validation_scheme`, which can be used to select
between the previous hostname validation behaviour and a new
option to use standardized OpenSSL algorithms to handle
hostname validation as part of the

As a nice side-effect, the new scheme gets rid of reverse DNS
lookups during TLS connection establishment, which used to be
a common source of hard-to-debug unresponsiveness in Mesos
components.

See `docs/ssl.md` in the follow-up commit for details of and
differences between the schemes.


Diffs (updated)
-

  3rdparty/libprocess/include/process/ssl/flags.hpp 
f3483f97f93bb29117b2c78f0f2ed9735d9c4b3a 
  3rdparty/libprocess/src/openssl.hpp 17bec246e516261f8d772f1647c17f092fae82d1 
  3rdparty/libprocess/src/openssl.cpp e7dbd67913fa8e7fbbf60dee428e7e38895f86ce 
  3rdparty/libprocess/src/posix/libevent/libevent_ssl_socket.hpp 
6ef5a86566af3439cfe0b06ab3576076623f7be0 
  3rdparty/libprocess/src/posix/libevent/libevent_ssl_socket.cpp 
29a1bf71c1df9d80370455a6269ecea0ec4193b0 


Diff: https://reviews.apache.org/r/70749/diff/5/

Changes: https://reviews.apache.org/r/70749/diff/4-5/


Testing
---

Todo!


Thanks,

Benno Evers



Re: Review Request 70749: Introduced optional new scheme for hostname validation.

2019-06-21 Thread Benno Evers


> On June 21, 2019, 1:32 a.m., Till Toenshoff wrote:
> > 3rdparty/libprocess/src/openssl.cpp
> > Lines 808 (patched)
> > <https://reviews.apache.org/r/70749/diff/4/?file=2151450#file2151450line808>
> >
> > I know you just moved it, but where do these 100ms come from and how 
> > could we be more explicit about that choice?
> > 
> > I would suggest to use a const with some explaining comment how that 
> > value was chosen - can we please? :D

I'm afraid I have no idea where the 100ms come from. The suggestion sounds 
good, but I think the changes will fit better in a separate review, since 
they're not really related to hostname validation.


> On June 21, 2019, 1:32 a.m., Till Toenshoff wrote:
> > 3rdparty/libprocess/src/openssl.cpp
> > Lines 984-985 (patched)
> > <https://reviews.apache.org/r/70749/diff/4/?file=2151450#file2151450line984>
> >
> > This sounds like a great idea worth spending 1 more cycle on -- can we 
> > create and reference a ticket that explains this jazz as nicely as we do 
> > here in the code?
> > 
> > My thought is that being open about this idea in JIRA,  we would 
> > provide more chances of getting community support for it.

Opened https://issues.apache.org/jira/browse/MESOS-9855


> On June 21, 2019, 1:32 a.m., Till Toenshoff wrote:
> > 3rdparty/libprocess/src/openssl.cpp
> > Lines 989 (patched)
> > <https://reviews.apache.org/r/70749/diff/4/?file=2151450#file2151450line989>
> >
> > Shall we explain why this is the `right` way - aka best practices?

Huh, I actually removed this now: I originally had a look at the OpenSSL 
example code and at RFC6125, but missed that partial wildcards are only 
disallowed in *internationalized* domain names. (and for these, openssl already 
does the correct thing regardless of this flag.)

With this removed, we're a bit more loose than what a web browser would accept, 
but Mesos is not a web browser so that seems fine.


- Benno


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


On June 20, 2019, 5:48 p.m., Benno Evers wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/70749/
> ---
> 
> (Updated June 20, 2019, 5:48 p.m.)
> 
> 
> Review request for mesos, Alexander Rukletsov, Joseph Wu, and Till Toenshoff.
> 
> 
> Bugs: MESOS-9809
> https://issues.apache.org/jira/browse/MESOS-9809
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This commit introduces a new libprocess SSL flag
> `hostname_validation_scheme`, which can be used to select
> between the previous hostname validation behaviour and a new
> option to use standardized OpenSSL algorithms to handle
> hostname validation as part of the handshake.
> 
> As a nice side-effect, the new scheme gets rid of reverse DNS
> lookups during TLS connection establishment, which used to be
> a common source of hard-to-debug unresponsiveness in Mesos
> components.
> 
> See `docs/ssl.md` in the follow-up commit for details of and
> differences between the schemes.
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/include/process/ssl/flags.hpp 
> f3483f97f93bb29117b2c78f0f2ed9735d9c4b3a 
>   3rdparty/libprocess/src/openssl.hpp 
> 17bec246e516261f8d772f1647c17f092fae82d1 
>   3rdparty/libprocess/src/openssl.cpp 
> e7dbd67913fa8e7fbbf60dee428e7e38895f86ce 
>   3rdparty/libprocess/src/posix/libevent/libevent_ssl_socket.hpp 
> 6ef5a86566af3439cfe0b06ab3576076623f7be0 
>   3rdparty/libprocess/src/posix/libevent/libevent_ssl_socket.cpp 
> 29a1bf71c1df9d80370455a6269ecea0ec4193b0 
> 
> 
> Diff: https://reviews.apache.org/r/70749/diff/4/
> 
> 
> Testing
> ---
> 
> Todo!
> 
> 
> Thanks,
> 
> Benno Evers
> 
>



Re: Review Request 70795: Updated SSL docs to include new libprocess flag.

2019-06-21 Thread Benno Evers

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

(Updated June 21, 2019, 3:05 p.m.)


Review request for mesos, Alexander Rukletsov, Jan-Philip Gehrcke, Joseph Wu, 
and Till Toenshoff.


Repository: mesos


Description (updated)
---

Added a description of the new `--hostname_validation_scheme` flag
and corresponding `LIBPROCESS_SSL_HOSTNAME_VALIDATION_SCHEME`
environment variable.


Diffs (updated)
-

  docs/ssl.md ce5058896144aa7824986d40d996899d92cb7c1c 


Diff: https://reviews.apache.org/r/70795/diff/4/

Changes: https://reviews.apache.org/r/70795/diff/3-4/


Testing
---


Thanks,

Benno Evers



Review Request 70921: Add OpenSSL related changes to CHANGELOG.

2019-06-21 Thread Benno Evers

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

Review request for mesos, Joseph Wu and Till Toenshoff.


Repository: mesos


Description
---

Add OpenSSL related changes to CHANGELOG.


Diffs
-

  CHANGELOG 197d7912fe229e3b5b52f97330fb327d7b2b9394 
  docs/upgrades.md 4a818df4993093770b49efb675b8078c42144241 


Diff: https://reviews.apache.org/r/70921/diff/1/


Testing
---


Thanks,

Benno Evers



Re: Review Request 70748: Changed semantics of libprocess TLS flags.

2019-06-21 Thread Benno Evers

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

(Updated June 21, 2019, 3:01 p.m.)


Review request for mesos, Alexander Rukletsov, Jan-Philip Gehrcke, Joseph Wu, 
and Till Toenshoff.


Summary (updated)
-

Changed semantics of libprocess TLS flags.


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


Repository: mesos


Description (updated)
---

This commit slightly updates the semants of the
`LIBPROCESS_SSL_VERIFY_CERT` and `LIBPROCESS_SSL_REQUIRE_CERT`
environment variables. The former now only applies to connections
in client mode and the latter now only applies to connections in
server mode.

In particular, in TLS server mode we now *only* verify client
certificates when `LIBPROCESS_SSL_REQUIRE_CERT` is set to `true`,
regardless of the value of `LIBPROCESS_SSL_VERIFY_CERT`.

In addtion, when in SSL client mode and  `LIBPROCESS_SSL_VERIFY_CERT`
has been set to `true`, enforce that the server actually presents a
certificate that can be verified. Note that this is expected to be
not a behavioural change in practice, since the TLS specification
already states that a server MUST always send a certificate unless an
anonymous cipher is used, and most TLS ciphersuites are configured to
exclude anonymous ciphers.


Diffs (updated)
-

  3rdparty/libprocess/src/openssl.hpp 17bec246e516261f8d772f1647c17f092fae82d1 
  3rdparty/libprocess/src/openssl.cpp e7dbd67913fa8e7fbbf60dee428e7e38895f86ce 
  3rdparty/libprocess/src/posix/libevent/libevent_ssl_socket.cpp 
29a1bf71c1df9d80370455a6269ecea0ec4193b0 
  3rdparty/libprocess/src/tests/ssl_tests.cpp 
6b8496aeeed79ae1bd39d7013f4f403b248fdd4c 


Diff: https://reviews.apache.org/r/70748/diff/5/

Changes: https://reviews.apache.org/r/70748/diff/4-5/


Testing
---

So far, mostly manual testing.


Thanks,

Benno Evers



Re: Review Request 70748: Changed semantics of some libprocess TLS flags.

2019-06-21 Thread Benno Evers


> On June 21, 2019, 1:06 a.m., Till Toenshoff wrote:
> > 3rdparty/libprocess/src/openssl.cpp
> > Line 96 (original), 96 (patched)
> > <https://reviews.apache.org/r/70748/diff/4/?file=2151330#file2151330line96>
> >
> > Additional to the updates above, we also need to call this out in the 
> > CHANGELOG. Can you please include that in this chain?

I added the `CHANGELOG` changes in a new review 
https://reviews.apache.org/r/70921/


- Benno


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


On June 19, 2019, 2:28 p.m., Benno Evers wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/70748/
> ---
> 
> (Updated June 19, 2019, 2:28 p.m.)
> 
> 
> Review request for mesos, Alexander Rukletsov, Jan-Philip Gehrcke, Joseph Wu, 
> and Till Toenshoff.
> 
> 
> Bugs: MESOS-9810
> https://issues.apache.org/jira/browse/MESOS-9810
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This commit slightly updates the semants of the
> `LIBPROCESS_SSL_VERIFY_CERT` and `LIBPROCESS_SSL_REQUIRE_CERT`
> environment variables. The former now only applies to connections
> in client mode and the latter now only applies to connections in
> server mode.
> 
> In particular, in TLS server mode we now *only* verify client
> certificates when `LIBPROCESS_SSL_REQUIRE_CERT` is set to `true`,
> regardless of the value of `LIBPROCESS_SSL_VERIFY_CERT`.
> 
> In addition, when in SSL client mode and  `LIBPROCESS_SSL_VERIFY_CERT`
> has been set to `true`, enforce that the server actually presents a
> certificate that can be verified. Note that this is expected to be
> not a behavioural change in practice, since the TLS specification
> already states that a server MUST always send a certificate unless an
> anonymous cipher is used, and most TLS ciphersuites are configured to
> exclude anonymous ciphers.
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/src/openssl.hpp 
> 17bec246e516261f8d772f1647c17f092fae82d1 
>   3rdparty/libprocess/src/openssl.cpp 
> e7dbd67913fa8e7fbbf60dee428e7e38895f86ce 
>   3rdparty/libprocess/src/posix/libevent/libevent_ssl_socket.cpp 
> 29a1bf71c1df9d80370455a6269ecea0ec4193b0 
>   3rdparty/libprocess/src/tests/ssl_tests.cpp 
> 6b8496aeeed79ae1bd39d7013f4f403b248fdd4c 
> 
> 
> Diff: https://reviews.apache.org/r/70748/diff/4/
> 
> 
> Testing
> ---
> 
> So far, mostly manual testing.
> 
> 
> Thanks,
> 
> Benno Evers
> 
>



Re: Review Request 70795: Updated SSL docs to include new libprocess flag.

2019-06-21 Thread Benno Evers


> On June 21, 2019, 1:34 a.m., Till Toenshoff wrote:
> > docs/ssl.md
> > Lines 194 (patched)
> > <https://reviews.apache.org/r/70795/diff/3/?file=2151430#file2151430line194>
> >
> > I wonder if we should already start a deprecation of the `libprocess` 
> > scheme - that would be:
> > - announcing that `openssl` will be standard soon on compatible boxes
> > - announcing it to be gone at some point
> > 
> > Or am I too eager for unification here?

It's actually a pretty big change - the 'libprocess' behaviour was built, I 
assume, to "magically" work with normal certificates w/o IP addresses despite 
libprocess only knowing about IP addresses. In DC/OS we don't notice most of 
it, since there all our certificates *do* contain the correct IP address, but 
at least quite a few unit tests will break by switching the default.

So I actually agree we should do this deprecation, but I'm not sure about the 
timeline.


- Benno


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


On June 20, 2019, 5:27 p.m., Benno Evers wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/70795/
> ---
> 
> (Updated June 20, 2019, 5:27 p.m.)
> 
> 
> Review request for mesos, Alexander Rukletsov, Jan-Philip Gehrcke, Joseph Wu, 
> and Till Toenshoff.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added a description of the new `--hostname_validation_algorithm` flag
> and corresponding `LIBPROCESS_SSL_HOSTNAME_VALIDATION_ALGORITHM`
> environment variable.
> 
> 
> Diffs
> -
> 
>   docs/ssl.md ce5058896144aa7824986d40d996899d92cb7c1c 
> 
> 
> Diff: https://reviews.apache.org/r/70795/diff/3/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Benno Evers
> 
>



Re: Review Request 70749: Introduced optional new scheme for hostname validation.

2019-06-20 Thread Benno Evers

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

(Updated June 20, 2019, 5:48 p.m.)


Review request for mesos, Alexander Rukletsov, Joseph Wu, and Till Toenshoff.


Changes
---

Renamed algorithm -> scheme


Summary (updated)
-

Introduced optional new scheme for hostname validation.


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


Repository: mesos


Description (updated)
---

This commit introduces a new libprocess SSL flag
`hostname_validation_scheme`, which can be used to select
between the previous hostname validation behaviour and a new
option to use standardized OpenSSL algorithms to handle
hostname validation as part of the

As a nice side-effect, the new scheme gets rid of reverse DNS
lookups during TLS connection establishment, which used to be
a common source of hard-to-debug unresponsiveness in Mesos
components.

See `docs/ssl.md` in the follow-up commit for details of and
differences between the schemes.


Diffs (updated)
-

  3rdparty/libprocess/include/process/ssl/flags.hpp 
f3483f97f93bb29117b2c78f0f2ed9735d9c4b3a 
  3rdparty/libprocess/src/openssl.hpp 17bec246e516261f8d772f1647c17f092fae82d1 
  3rdparty/libprocess/src/openssl.cpp e7dbd67913fa8e7fbbf60dee428e7e38895f86ce 
  3rdparty/libprocess/src/posix/libevent/libevent_ssl_socket.hpp 
6ef5a86566af3439cfe0b06ab3576076623f7be0 
  3rdparty/libprocess/src/posix/libevent/libevent_ssl_socket.cpp 
29a1bf71c1df9d80370455a6269ecea0ec4193b0 


Diff: https://reviews.apache.org/r/70749/diff/4/

Changes: https://reviews.apache.org/r/70749/diff/3-4/


Testing
---

Todo!


Thanks,

Benno Evers



Re: Review Request 70810: Updated SSL docs with suggested runtime configuration.

2019-06-20 Thread Benno Evers


> On June 20, 2019, 12:49 p.m., Jan-Philip Gehrcke wrote:
> > docs/ssl.md
> > Lines 90 (patched)
> > <https://reviews.apache.org/r/70810/diff/2/?file=2151333#file2151333line90>
> >
> > Let's define "and contain the correct hostname", let's refer to the 
> > available algorithms.

I did that in the other review, since strictly speaking this commit does not 
yet contain the hostname validation option.


- Benno


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


On June 20, 2019, 5:32 p.m., Benno Evers wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/70810/
> ---
> 
> (Updated June 20, 2019, 5:32 p.m.)
> 
> 
> Review request for mesos, Alexander Rukletsov, Joseph Wu, and Till Toenshoff.
> 
> 
> Bugs: MESOS-9791
> https://issues.apache.org/jira/browse/MESOS-9791
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Updated SSL docs with suggested runtime configuration.
> 
> 
> Diffs
> -
> 
>   docs/ssl.md ce5058896144aa7824986d40d996899d92cb7c1c 
> 
> 
> Diff: https://reviews.apache.org/r/70810/diff/3/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Benno Evers
> 
>



Re: Review Request 70810: Updated SSL docs with suggested runtime configuration.

2019-06-20 Thread Benno Evers

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

(Updated June 20, 2019, 5:32 p.m.)


Review request for mesos, Alexander Rukletsov, Joseph Wu, and Till Toenshoff.


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


Repository: mesos


Description (updated)
---

Updated SSL docs with suggested runtime configuration.


Diffs (updated)
-

  docs/ssl.md ce5058896144aa7824986d40d996899d92cb7c1c 


Diff: https://reviews.apache.org/r/70810/diff/3/

Changes: https://reviews.apache.org/r/70810/diff/2-3/


Testing
---


Thanks,

Benno Evers



Re: Review Request 70795: Updated SSL docs to include new libprocess flag.

2019-06-20 Thread Benno Evers


> On June 20, 2019, 1:05 p.m., Jan-Philip Gehrcke wrote:
> > docs/ssl.md
> > Lines 145 (patched)
> > <https://reviews.apache.org/r/70795/diff/2/?file=2151355#file2151355line145>
> >
> > "via DNS name" instead of "via hostname"?

I know that "DNS name" is more common in the context of x509 certification, but 
here I think it may be confusing for readers: It's the first only time a "DNS 
name" is mentioned in this document, and also there's no requirement that the 
DNS name must be resolved via DNS or stored in any DNS server.


- Benno


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


On June 20, 2019, 5:27 p.m., Benno Evers wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/70795/
> ---
> 
> (Updated June 20, 2019, 5:27 p.m.)
> 
> 
> Review request for mesos, Alexander Rukletsov, Jan-Philip Gehrcke, Joseph Wu, 
> and Till Toenshoff.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added a description of the new `--hostname_validation_algorithm` flag
> and corresponding `LIBPROCESS_SSL_HOSTNAME_VALIDATION_ALGORITHM`
> environment variable.
> 
> 
> Diffs
> -
> 
>   docs/ssl.md ce5058896144aa7824986d40d996899d92cb7c1c 
> 
> 
> Diff: https://reviews.apache.org/r/70795/diff/3/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Benno Evers
> 
>



Re: Review Request 70795: Updated SSL docs to include new libprocess flag.

2019-06-20 Thread Benno Evers

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

(Updated June 20, 2019, 5:27 p.m.)


Review request for mesos, Alexander Rukletsov, Jan-Philip Gehrcke, Joseph Wu, 
and Till Toenshoff.


Repository: mesos


Description
---

Added a description of the new `--hostname_validation_algorithm` flag
and corresponding `LIBPROCESS_SSL_HOSTNAME_VALIDATION_ALGORITHM`
environment variable.


Diffs (updated)
-

  docs/ssl.md ce5058896144aa7824986d40d996899d92cb7c1c 


Diff: https://reviews.apache.org/r/70795/diff/3/

Changes: https://reviews.apache.org/r/70795/diff/2-3/


Testing
---


Thanks,

Benno Evers



Review Request 70886: WIP: Override source address for executors.

2019-06-19 Thread Benno Evers

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

Review request for mesos, Jan-Philip Gehrcke, Joseph Wu, and Till Toenshoff.


Repository: mesos


Description
---

Ignore an executor's reported IP address and enforce the hostname
"localhost" instead for TLS hostname validation.

NOTE: This change is for illustration purposes only, and is not
intended to be upstreamed.


Diffs
-

  src/slave/slave.cpp 30039b0857a4d85b4b96fa95d7f8724d57cdec6e 


Diff: https://reviews.apache.org/r/70886/diff/1/


Testing
---


Thanks,

Benno Evers



Re: Review Request 70797: WIP: Unit tests for hostname validation.

2019-06-19 Thread Benno Evers

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

(Updated June 19, 2019, 2:47 p.m.)


Review request for mesos, Alexander Rukletsov, Joseph Wu, and Till Toenshoff.


Repository: mesos


Description
---

WIP: Unit tests for hostname validation.


Diffs (updated)
-

  3rdparty/libprocess/src/tests/ssl_tests.cpp 
6b8496aeeed79ae1bd39d7013f4f403b248fdd4c 


Diff: https://reviews.apache.org/r/70797/diff/2/

Changes: https://reviews.apache.org/r/70797/diff/1-2/


Testing
---


Thanks,

Benno Evers



Re: Review Request 70795: Updated SSL docs to include new libprocess flag.

2019-06-19 Thread Benno Evers

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

(Updated June 19, 2019, 2:46 p.m.)


Review request for mesos, Alexander Rukletsov, Jan-Philip Gehrcke, Joseph Wu, 
and Till Toenshoff.


Summary (updated)
-

Updated SSL docs to include new libprocess flag.


Repository: mesos


Description (updated)
---

Added a description of the new `--hostname_validation_algorithm` flag
and corresponding `LIBPROCESS_SSL_HOSTNAME_VALIDATION_ALGORITHM`
environment variable.


Diffs (updated)
-

  docs/ssl.md ce5058896144aa7824986d40d996899d92cb7c1c 


Diff: https://reviews.apache.org/r/70795/diff/2/

Changes: https://reviews.apache.org/r/70795/diff/1-2/


Testing
---


Thanks,

Benno Evers



Re: Review Request 70749: Introduced optional new algorithm for hostname validation.

2019-06-19 Thread Benno Evers

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

(Updated June 19, 2019, 2:45 p.m.)


Review request for mesos, Alexander Rukletsov, Joseph Wu, and Till Toenshoff.


Summary (updated)
-

Introduced optional new algorithm for hostname validation.


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


Repository: mesos


Description
---

This commit introduces a new libprocess SSL flag
`hostname_validation_algorithm`, which can be used to select
between the previous hostname validation behaviour and a new
option to use standardized OpenSSL algorithms to handle
hostname validation as part of the

As a nice side-effect, the new algorithm gets rid of reverse DNS
lookups during TLS connection establishment, which used to be
a common source of hard-to-debug unresponsiveness in Mesos
components.

See `docs/ssl.md` in the follow-up commit for details of and
differences between the algorithms.


Diffs (updated)
-

  3rdparty/libprocess/include/process/ssl/flags.hpp 
f3483f97f93bb29117b2c78f0f2ed9735d9c4b3a 
  3rdparty/libprocess/src/openssl.hpp 17bec246e516261f8d772f1647c17f092fae82d1 
  3rdparty/libprocess/src/openssl.cpp e7dbd67913fa8e7fbbf60dee428e7e38895f86ce 
  3rdparty/libprocess/src/posix/libevent/libevent_ssl_socket.hpp 
6ef5a86566af3439cfe0b06ab3576076623f7be0 
  3rdparty/libprocess/src/posix/libevent/libevent_ssl_socket.cpp 
29a1bf71c1df9d80370455a6269ecea0ec4193b0 


Diff: https://reviews.apache.org/r/70749/diff/3/

Changes: https://reviews.apache.org/r/70749/diff/2-3/


Testing
---

Todo!


Thanks,

Benno Evers



Re: Review Request 70796: Fixed Mesos unit tests after Address API change.

2019-06-19 Thread Benno Evers

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

(Updated June 19, 2019, 2:44 p.m.)


Review request for mesos, Alexander Rukletsov, Joseph Wu, and Till Toenshoff.


Summary (updated)
-

Fixed Mesos unit tests after Address API change.


Repository: mesos


Description (updated)
---

Adjusted some Mesos unit tests after the renaming
of `Address::hostname()` -> `Address::lookup_hostname()`.


Diffs (updated)
-

  src/tests/master_tests.cpp 097f1b77a59e29c6690210773d1556ebf2bb701e 


Diff: https://reviews.apache.org/r/70796/diff/2/

Changes: https://reviews.apache.org/r/70796/diff/1-2/


Testing
---


Thanks,

Benno Evers



Review Request 70885: Renamed 'libprocess::network::Address::hostname()'.

2019-06-19 Thread Benno Evers

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

Review request for mesos, Joseph Wu and Till Toenshoff.


Repository: mesos


Description
---

Renamed member function `hostname()` to `lookup_hostname()`,
since the former name hides the fact that a call to this
function involves a synchronous network access in order
to make a reverse DNS lookup.


Diffs
-

  3rdparty/libprocess/include/process/address.hpp 
e740e840c38381bafd7a1a7fcde5f963832ac1fb 
  3rdparty/libprocess/src/tests/http_tests.cpp 
97aaf3ed3d4fab6d717d5c9b6d12402562ac6b46 
  3rdparty/libprocess/src/tests/ssl_tests.cpp 
6b8496aeeed79ae1bd39d7013f4f403b248fdd4c 


Diff: https://reviews.apache.org/r/70885/diff/1/


Testing
---


Thanks,

Benno Evers



Review Request 70884: WIP: Added optional hostname field to UPID.

2019-06-19 Thread Benno Evers

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

Review request for mesos, Joseph Wu and Till Toenshoff.


Repository: mesos


Description
---

This allows client code to access the original hostname
that was used to specify a libprocess address.

NOTE: Right now, this does not yet contain the code to
set the hostname when creating a UPID from a string
description.


Diffs
-

  3rdparty/libprocess/include/process/pid.hpp 
9f09ab46fa3ceaeac09b0fbf9f532728c4ed2d7a 
  3rdparty/libprocess/src/process.cpp 799666f03d6a78708aa9336c2dd04bc9b5023aa0 


Diff: https://reviews.apache.org/r/70884/diff/1/


Testing
---


Thanks,

Benno Evers



  1   2   3   4   5   6   7   8   >