Re: Review Request 71018: Fixed a race between status updates and acknowledgements in SLRP tests.

[Mesos Reviewbot]

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



Patch looks great!

Reviews applied: [71018]

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

- Mesos Reviewbot


On July 5, 2019, 11:56 p.m., Chun-Hung Hsiao wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/71018/
> ---
> 
> (Updated July 5, 2019, 11:56 p.m.)
> 
> 
> Review request for mesos, Benjamin Bannier, Benjamin Mahler, and Greg Mann.
> 
> 
> Bugs: MESOS-9881
> https://issues.apache.org/jira/browse/MESOS-9881
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> SLRP tests `RetryOperationStatusUpdate*` check that no operation update
> will be sent by SLRP once acknowledgements are sent by master. However,
> since the acknowledgements are delivered via HTTP, it is possible that
> operation status updates race with acknowledgements that are still in
> HTTP pipes. This patch fixes this race condition.
> 
> 
> Diffs
> -
> 
>   src/tests/storage_local_resource_provider_tests.cpp 
> 38233053452259571743317326a6efc74d97bd29 
> 
> 
> Diff: https://reviews.apache.org/r/71018/diff/1/
> 
> 
> Testing
> ---
> 
> Ran the tests under stress 200 times.
> 
> 
> Thanks,
> 
> Chun-Hung Hsiao
> 
>

Review Request 71018: Fixed a race between status updates and acknowledgements in SLRP tests.

[Chun-Hung Hsiao]

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

Review request for mesos, Benjamin Bannier, Benjamin Mahler, and Greg Mann.


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


Repository: mesos


Description
---

SLRP tests `RetryOperationStatusUpdate*` check that no operation update
will be sent by SLRP once acknowledgements are sent by master. However,
since the acknowledgements are delivered via HTTP, it is possible that
operation status updates race with acknowledgements that are still in
HTTP pipes. This patch fixes this race condition.


Diffs
-

  src/tests/storage_local_resource_provider_tests.cpp 
38233053452259571743317326a6efc74d97bd29 


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


Testing
---

Ran the tests under stress 200 times.


Thanks,

Chun-Hung Hsiao

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

[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.

[Benno Evers]

> On July 4, 2019, 3:12 p.m., Till Toenshoff wrote:
> > docs/upgrades.md
> > Lines 527 (patched)
> > 
> >
> > 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().

[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.

[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)
> > 
> >
> > 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 70886: Override source address for executors.

[Mesos Reviewbot]

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



Bad review!

Reviews applied: [70886, 70993, 70933, 70797, 70992, 70991, 70921, 70795, 
70749, 70883, 70884, 70810, 70748]

Error:
2019-07-05 09:32:53 URL:https://reviews.apache.org/r/70992/diff/raw/ [812/812] 
-> "70992.patch" [1]
error: patch failed: CHANGELOG:27
error: CHANGELOG: patch does not apply

- Mesos Reviewbot


On June 19, 2019, 2:48 p.m., Benno Evers wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/70886/
> ---
> 
> (Updated June 19, 2019, 2:48 p.m.)
> 
> 
> Review request for mesos, Benjamin Mahler, Jan-Philip Gehrcke, Joseph Wu, and 
> Till Toenshoff.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Ignore an executor's reported IP address and enforce the hostname
> "localhost" instead.
> 
> This is intended to enable TLS connections to executors running
> in bridge-mode or virtual-mode docker containers, which can not
> present a valid certificate containing their source IP which is
> dynamically generated and not known until the executor connects.
> 
> 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/3/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Benno Evers
> 
>

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

[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