Re: Review Request 71986: Removed redundant calls to `c_str` flagged by mesos-tidy.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/71986/#review219234 --- Ship it! Ship It! - Till Toenshoff On Jan. 13, 2020, 9:49 a.m., Benjamin Bannier wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/71986/ > --- > > (Updated Jan. 13, 2020, 9:49 a.m.) > > > Review request for mesos and Benno Evers. > > > Repository: mesos > > > Description > --- > > These calls were flagged by the upstream > `readability-redundant-string-cstr` check. > > > Diffs > - > > src/linux/systemd.cpp d9cb5662cbf8e4d3531518af8bb633d9bb12d038 > > > Diff: https://reviews.apache.org/r/71986/diff/1/ > > > Testing > --- > > confirmed that `clang-tidy` does not report anything from these lines anymore > > > Thanks, > > Benjamin Bannier > >
Re: Review Request 71985: Properly initialized dummy variable.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/71985/#review219233 --- Ship it! Ship It! - Till Toenshoff On Jan. 13, 2020, 9:48 a.m., Benjamin Bannier wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/71985/ > --- > > (Updated Jan. 13, 2020, 9:48 a.m.) > > > Review request for mesos and Benno Evers. > > > Repository: mesos > > > Description > --- > > Since `Try` does not have a default constructor we need to provide a > dummy value. > > > Diffs > - > > src/slave/main.cpp 9e40743a9d379d5b1ff9f97372826d4296167ce3 > > > Diff: https://reviews.apache.org/r/71985/diff/1/ > > > Testing > --- > > `make check` on macos > > > Thanks, > > Benjamin Bannier > >
Re: Review Request 71934: Bumped site's rack to rack-1.16.12.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/71934/#review219130 --- Ship it! Ship It! - Till Toenshoff On Dec. 20, 2019, 11:51 p.m., Benjamin Bannier wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/71934/ > --- > > (Updated Dec. 20, 2019, 11:51 p.m.) > > > Review request for mesos, Benno Evers and Vinod Kone. > > > Repository: mesos > > > Description > --- > > This addresses CVE-2019-16782 which should not affect us. > > > Diffs > - > > site/Gemfile.lock 87d825c4e4056c33e0702b3c429a48b01cc1b035 > > > Diff: https://reviews.apache.org/r/71934/diff/1/ > > > Testing > --- > > Confirmed that the website still builds. > > > Thanks, > > Benjamin Bannier > >
Re: Review Request 71663: SSL Wrapper: Windows: Made sure windows.hpp is included before OpenSSL.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/71663/#review218779 --- Ship it! Ship It! - Till Toenshoff On Oct. 23, 2019, 7:37 p.m., Joseph Wu wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/71663/ > --- > > (Updated Oct. 23, 2019, 7:37 p.m.) > > > Review request for mesos, Benno Evers, Benjamin Mahler, Greg Mann, and Till > Toenshoff. > > > Bugs: MESOS-10009 > https://issues.apache.org/jira/browse/MESOS-10009 > > > Repository: mesos > > > Description > --- > > Depending on the version of OpenSSL and what headers of OpenSSL > are included, the order of certain Windows headers may be included > in the "wrong" order. This results in hundreds of cryptic syntax > errors from MSVC. > > This sweeps through the existing sites with OpenSSL headers and > makes sure to include the Windows headers above them. > > > Diffs > - > > 3rdparty/libprocess/include/process/jwt.hpp > 62bf5c2ac3fcb6b76c88cd691d001358a4cb0aa6 > 3rdparty/libprocess/include/process/ssl/gtest.hpp > 4a14a8ef7937e8c6adf3a78db3e9558333700a1b > 3rdparty/libprocess/include/process/ssl/tls_config.hpp > 18c51a8baabf07143c7892dbee0ad2d856457d8d > 3rdparty/libprocess/src/openssl.cpp > bd05866950e1043d9585a7c5fdc7b2147a233fd3 > 3rdparty/libprocess/src/ssl/utilities.cpp > c4a8ab4f83939554ca3eb7b4eb86c42c8a539b48 > > > Diff: https://reviews.apache.org/r/71663/diff/2/ > > > Testing > --- > > cmake --build . (Windows) > > > Thanks, > > Joseph Wu > >
Re: Review Request 71535: Fixed certificate write functions for windows.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/71535/ --- (Updated Sept. 24, 2019, 12:41 a.m.) Review request for mesos, Greg Mann and Joseph Wu. Repository: mesos Description (updated) --- The former implementation failed on Windows. By switching to the recommended `BIO_` API for serializing the x509 data to disk, we avoid that problem. Diffs - 3rdparty/libprocess/src/ssl/utilities.cpp c4a8ab4f83939554ca3eb7b4eb86c42c8a539b48 Diff: https://reviews.apache.org/r/71535/diff/2/ Testing --- Tested "manually" within a private project on Windows. Thanks, Till Toenshoff
Re: Review Request 71535: Fixed certificate write functions for windows.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/71535/ --- (Updated Sept. 24, 2019, 12:40 a.m.) Review request for mesos, Greg Mann and Joseph Wu. Repository: mesos Description (updated) --- Fixed certificate write functions for windows. Diffs (updated) - 3rdparty/libprocess/src/ssl/utilities.cpp c4a8ab4f83939554ca3eb7b4eb86c42c8a539b48 Diff: https://reviews.apache.org/r/71535/diff/2/ Changes: https://reviews.apache.org/r/71535/diff/1-2/ Testing --- Tested "manually" within a private project on Windows. Thanks, Till Toenshoff
Review Request 71535: Fixed certificate write functions for windows.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/71535/ --- Review request for mesos, Greg Mann and Joseph Wu. Repository: mesos Description --- The former implementation failed on Windows. By switching to the recommended `BIO_` API for serializing the x509 data to disk, we avoid that problem. Diffs - 3rdparty/libprocess/src/ssl/utilities.cpp c4a8ab4f83939554ca3eb7b4eb86c42c8a539b48 Diff: https://reviews.apache.org/r/71535/diff/1/ Testing --- Tested "manually" within a private project on Windows. Thanks, Till Toenshoff
Re: Review Request 71497: Introduced new names for SSL-related libprocess flags.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/71497/#review217884 --- Ship it! Ship It! - Till Toenshoff On Sept. 19, 2019, 2:35 p.m., Benno Evers wrote: > > --- > 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. > > > 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/4/ > > > Testing > --- > > > Thanks, > > Benno Evers > >
Re: Review Request 71478: Windows: Moved definition out of inline function call.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/71478/#review217850 --- Ship it! Ship It! - Till Toenshoff On Sept. 12, 2019, 7:35 p.m., Joseph Wu wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/71478/ > --- > > (Updated Sept. 12, 2019, 7:35 p.m.) > > > Review request for mesos, Greg Mann and Till Toenshoff. > > > Repository: mesos > > > Description > --- > > MSVC does not deal with #ifdefs from inside function calls. > So here, the `#if defined(...)` was taken literally and is > considered a syntax error by MSVC. > > > Diffs > - > > 3rdparty/libprocess/src/openssl.cpp > 5854711971c9ebc4d676edc43af5ab5cfd5ea4c6 > > > Diff: https://reviews.apache.org/r/71478/diff/1/ > > > Testing > --- > > make check > > > Thanks, > > Joseph Wu > >
Re: Review Request 71509: Windows: Fixed AllocationRoleEnvironmentVariable tests.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/71509/#review217836 --- Ship it! Ship It! - Till Toenshoff On Sept. 18, 2019, 10:57 p.m., Joseph Wu wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/71509/ > --- > > (Updated Sept. 18, 2019, 10:57 p.m.) > > > Review request for mesos, Gilbert Song, Greg Mann, Qian Zhang, and Till > Toenshoff. > > > Bugs: MESOS-9874 > https://issues.apache.org/jira/browse/MESOS-9874 > > > Repository: mesos > > > Description > --- > > These tests were using a shell script, which can easily be > converted to a batch script on Windows. > > > Diffs > - > > src/tests/command_executor_tests.cpp > e9726b0b6c4623d5196186cd43722952f77669da > src/tests/containerizer/docker_containerizer_tests.cpp > 5e31a95149bdc0de64eeab3c289f7816077c82c4 > src/tests/default_executor_tests.cpp > e5c0bf2df4b35fb3920104e1b72bea7797724933 > > > Diff: https://reviews.apache.org/r/71509/diff/1/ > > > Testing > --- > > cmake --build . --target check (Windows) > > > Thanks, > > Joseph Wu > >
Re: Review Request 71497: Introduced new names for SSL-related libprocess flags.
---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/71497/#review217787
---
3rdparty/libprocess/src/openssl.cpp
Lines 545-547 (patched)
<https://reviews.apache.org/r/71497/#comment305172>
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.
3rdparty/libprocess/src/openssl.cpp
Line 593 (original), 615 (patched)
<https://reviews.apache.org/r/71497/#comment305173>
s/compatility/compatibility/
3rdparty/libprocess/src/openssl.cpp
Line 594 (original), 616 (patched)
<https://reviews.apache.org/r/71497/#comment305176>
s/be//
docs/ssl.md
Lines 100 (patched)
<https://reviews.apache.org/r/71497/#comment305174>
s/below//
docs/ssl.md
Lines 117 (patched)
<https://reviews.apache.org/r/71497/#comment305175>
s/below//
- Till Toenshoff
On Sept. 17, 2019, 12:50 p.m., Benno Evers wrote:
>
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/71497/
> ---
>
> (Updated Sept. 17, 2019, 12:50 p.m.)
>
>
> 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/2/
>
>
> Testing
> ---
>
>
> Thanks,
>
> Benno Evers
>
>
Re: Review Request 71430: Included new Python CLI in distribution tarball.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/71430/#review217789 --- Ship it! Ship It! - Till Toenshoff On Sept. 4, 2019, 7:48 a.m., Benjamin Bannier wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/71430/ > --- > > (Updated Sept. 4, 2019, 7:48 a.m.) > > > Review request for mesos, Benno Evers and Till Toenshoff. > > > Bugs: MESOS-9958 > https://issues.apache.org/jira/browse/MESOS-9958 > > > Repository: mesos > > > Description > --- > > Included new Python CLI in distribution tarball. > > > Diffs > - > > src/Makefile.am 5daee632d52e882acc15b90f655a53dab23eaaf6 > > > Diff: https://reviews.apache.org/r/71430/diff/1/ > > > Testing > --- > > * confirmed that the new CLI source files are contained in the distribution > tarball > * one can currently still not run `make distcheck` with `--enable-new-cli` > since the Mesos lib egg build process puts output files into the source dir > > > Thanks, > > Benjamin Bannier > >
Re: Review Request 71496: Removed an outdated reference to the 'libprocess' hostname validation.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/71496/#review217788 --- Ship it! Ship It! - Till Toenshoff On Sept. 17, 2019, 12:35 p.m., Benno Evers wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/71496/ > --- > > (Updated Sept. 17, 2019, 12:35 p.m.) > > > 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 71395: Implemented snapshot bot.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/71395/#review217588 --- support/mesos-snapshot.sh Lines 23 (patched) <https://reviews.apache.org/r/71395/#comment304859> Very good point and I missed it in my review. - Till Toenshoff On Aug. 28, 2019, 4:41 p.m., Vinod Kone wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/71395/ > --- > > (Updated Aug. 28, 2019, 4:41 p.m.) > > > Review request for mesos, Benjamin Bannier and Till Toenshoff. > > > Bugs: MESOS-9955 > https://issues.apache.org/jira/browse/MESOS-9955 > > > Repository: mesos > > > Description > --- > > Dockerized the snapshot building process. > > > Diffs > - > > support/jenkins/snapshotbot.sh PRE-CREATION > support/mesos-snapshot.sh PRE-CREATION > > > Diff: https://reviews.apache.org/r/71395/diff/1/ > > > Testing > --- > > https://builds.apache.org/view/M-R/view/Mesos/job/Mesos-Snapshot-Jar-Test/16/console > > > Thanks, > > Vinod Kone > >
Re: Review Request 71395: Implemented snapshot bot.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/71395/#review217587 --- Ship it! Ship It! - Till Toenshoff On Aug. 28, 2019, 4:41 p.m., Vinod Kone wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/71395/ > --- > > (Updated Aug. 28, 2019, 4:41 p.m.) > > > Review request for mesos, Benjamin Bannier and Till Toenshoff. > > > Bugs: MESOS-9955 > https://issues.apache.org/jira/browse/MESOS-9955 > > > Repository: mesos > > > Description > --- > > Dockerized the snapshot building process. > > > Diffs > - > > support/jenkins/snapshotbot.sh PRE-CREATION > support/mesos-snapshot.sh PRE-CREATION > > > Diff: https://reviews.apache.org/r/71395/diff/1/ > > > Testing > --- > > https://builds.apache.org/view/M-R/view/Mesos/job/Mesos-Snapshot-Jar-Test/16/console > > > Thanks, > > Vinod Kone > >
Re: Review Request 71393: Skiped GPG signing during `maven-install` in Makefile.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/71393/#review217585 --- Ship it! Ship It! - Till Toenshoff On Aug. 28, 2019, 4:40 p.m., Vinod Kone wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/71393/ > --- > > (Updated Aug. 28, 2019, 4:40 p.m.) > > > Review request for mesos, Benjamin Bannier and Till Toenshoff. > > > Bugs: MESOS-9955 > https://issues.apache.org/jira/browse/MESOS-9955 > > > Repository: mesos > > > Description > --- > > GPG signing is not needed for snapshot JARs (since they are > typically done by CI). But our snapshot building workflow, > i.e., support/snapshot.sh, relies on `make maven-install` which forces > GPG signing to happen. This allows our snapshot script to skip GPG > signing by doing `mvn deploy -Dgpg.skip`. > > Note that GPG signing still happens for RCs and release JARs (which > are typically done by humans) when `mvn deploy` (without `-Dgpg.skip`) > is called (e.g., in support/vote.sh). > > > Diffs > - > > src/Makefile.am 5daee632d52e882acc15b90f655a53dab23eaaf6 > > > Diff: https://reviews.apache.org/r/71393/diff/1/ > > > Testing > --- > > https://builds.apache.org/view/M-R/view/Mesos/job/Mesos-Snapshot-Jar-Test/16/console > > > Thanks, > > Vinod Kone > >
Re: Review Request 71394: Skipped GPG signing for snapshot builds.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/71394/#review217586 --- Ship it! Ship It! - Till Toenshoff On Aug. 28, 2019, 4:40 p.m., Vinod Kone wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/71394/ > --- > > (Updated Aug. 28, 2019, 4:40 p.m.) > > > Review request for mesos, Benjamin Bannier and Till Toenshoff. > > > Bugs: MESOS-9955 > https://issues.apache.org/jira/browse/MESOS-9955 > > > Repository: mesos > > > Description > --- > > Skipped GPG signing for snapshot builds. > > > Diffs > - > > support/snapshot.sh c30aca02ec999320e2b3f0f3ce36c04aa0425ae3 > > > Diff: https://reviews.apache.org/r/71394/diff/1/ > > > Testing > --- > > https://builds.apache.org/view/M-R/view/Mesos/job/Mesos-Snapshot-Jar-Test/16/console > > > Thanks, > > Vinod Kone > >
Re: Review Request 71392: Improved snapshot script to deduce the Mesos version.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/71392/#review217582 --- support/snapshot.sh Line 36 (original), 34 (patched) <https://reviews.apache.org/r/71392/#comment304858> This is a bit unfortunate I feel as "SNAPSHOT" does not help that much determining the version --- how about we use the HEAD SHA instead? ``` TAG=$VERSION-$SHA ``` support/snapshot.sh Line 56 (original), 53 (patched) <https://reviews.apache.org/r/71392/#comment304855> Why are we hardcoding `-j3` here and below instead of using e.g. `$(nproc)`? - Till Toenshoff On Aug. 28, 2019, 4:39 p.m., Vinod Kone wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/71392/ > --- > > (Updated Aug. 28, 2019, 4:39 p.m.) > > > Review request for mesos, Benjamin Bannier and Till Toenshoff. > > > Bugs: MESOS-9955 > https://issues.apache.org/jira/browse/MESOS-9955 > > > Repository: mesos > > > Description > --- > > Previously the script expected the version to be provided as an > argument. > > > Diffs > - > > support/snapshot.sh c30aca02ec999320e2b3f0f3ce36c04aa0425ae3 > > > Diff: https://reviews.apache.org/r/71392/diff/1/ > > > Testing > --- > > https://builds.apache.org/view/M-R/view/Mesos/job/Mesos-Snapshot-Jar-Test/16/console > > > Thanks, > > Vinod Kone > >
Re: Review Request 71390: Fixed the snapshot script to be non-interactive.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/71390/#review217581 --- Ship it! Ship It! - Till Toenshoff On Aug. 28, 2019, 4:38 p.m., Vinod Kone wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/71390/ > --- > > (Updated Aug. 28, 2019, 4:38 p.m.) > > > Review request for mesos, Benjamin Bannier and Till Toenshoff. > > > Bugs: MESOS-9955 > https://issues.apache.org/jira/browse/MESOS-9955 > > > Repository: mesos > > > Description > --- > > Fixed the snapshot script to be non-interactive. > > > Diffs > - > > support/snapshot.sh c30aca02ec999320e2b3f0f3ce36c04aa0425ae3 > > > Diff: https://reviews.apache.org/r/71390/diff/1/ > > > Testing > --- > > https://builds.apache.org/view/M-R/view/Mesos/job/Mesos-Snapshot-Jar-Test/16/console > > > Thanks, > > Vinod Kone > >
Re: Review Request 71241: Added a `distcheck` target to the cmake build.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/71241/#review217580 --- Ship it! Ship It! - Till Toenshoff On Sept. 3, 2019, 2:51 p.m., Benjamin Bannier wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/71241/ > --- > > (Updated Sept. 3, 2019, 2:51 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 dc50dd457585c14a456ca710da4a16a0d45ef17a > cmake/distcheck.sh PRE-CREATION > > > Diff: https://reviews.apache.org/r/71241/diff/3/ > > > 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.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/71240/#review217579 --- cmake/dist.sh Lines 30 (patched) <https://reviews.apache.org/r/71240/#comment304854> Will fail with my setup as my repo-dir is called different than "mesos". - Till Toenshoff On Sept. 3, 2019, 1:52 p.m., Benjamin Bannier wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/71240/ > --- > > (Updated Sept. 3, 2019, 1:52 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 dc50dd457585c14a456ca710da4a16a0d45ef17a > cmake/dist.sh PRE-CREATION > > > Diff: https://reviews.apache.org/r/71240/diff/2/ > > > Testing > --- > > `ninja dist` produces a tarball, no artifacts left around but tarball. > > > Thanks, > > Benjamin Bannier > >
Re: Review Request 71240: Added a `dist` target to the cmake build.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/71240/#review217578 --- cmake/dist.sh Lines 37 (patched) <https://reviews.apache.org/r/71240/#comment304853> s/Sucessfully/Successfully/ - Till Toenshoff On Sept. 3, 2019, 1:52 p.m., Benjamin Bannier wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/71240/ > --- > > (Updated Sept. 3, 2019, 1:52 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 dc50dd457585c14a456ca710da4a16a0d45ef17a > cmake/dist.sh PRE-CREATION > > > Diff: https://reviews.apache.org/r/71240/diff/2/ > > > Testing > --- > > `ninja dist` produces a tarball, no artifacts left around but tarball. > > > Thanks, > > Benjamin Bannier > >
Re: Review Request 71240: Added a `dist` target to the cmake build.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/71240/#review217577 --- Ship it! Ship It! - Till Toenshoff On Sept. 3, 2019, 1:52 p.m., Benjamin Bannier wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/71240/ > --- > > (Updated Sept. 3, 2019, 1:52 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 dc50dd457585c14a456ca710da4a16a0d45ef17a > cmake/dist.sh PRE-CREATION > > > Diff: https://reviews.apache.org/r/71240/diff/2/ > > > Testing > --- > > `ninja dist` produces a tarball, no artifacts left around but tarball. > > > Thanks, > > Benjamin Bannier > >
Re: Review Request 71207: Revert "Updated cpplint.py to be less verbose when there is no linting issue."
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/71207/#review217441 --- Ship it! - Till Toenshoff On Aug. 20, 2019, 11:49 a.m., Benjamin Bannier wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/71207/ > --- > > (Updated Aug. 20, 2019, 11:49 a.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/3/ > > > Testing > --- > > * used for development for a couple of months > * reports no issues in the current source tree > > > Thanks, > > Benjamin Bannier > >
Re: Review Request 71209: Enabled a number of additional pre-commit checks.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/71209/#review217443 --- Ship it! I'ld love to see the commit description to tell us which checks now got activated exactly - the more we explain, the better i feel. - Till Toenshoff 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 71300: Removed mesos-style transition script.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/71300/#review217438 --- Ship it! - Till Toenshoff On Aug. 26, 2019, 4:40 p.m., Benjamin Bannier wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/71300/ > --- > > (Updated Aug. 26, 2019, 4:40 p.m.) > > > Review request for mesos, Benno Evers and Till Toenshoff. > > > Bugs: MESOS-9630 > https://issues.apache.org/jira/browse/MESOS-9630 > > > Repository: mesos > > > Description > --- > > Removed mesos-style transition script. > > > Diffs > - > > support/mesos-style.py cd490bd3deeb8477295bbad010f36d984ce17d27 > > > Diff: https://reviews.apache.org/r/71300/diff/3/ > > > Testing > --- > > n/a > > THIS PATCH SHOULD ONLY BE COMMITTED AFTER THE PRECEEDING CHAIN HAS BEEN > LANDED FOR SOME TIME TO GIVE CONTRIBUTORS A CHANCE TO ADJUST THEIR WORKFLOW. > > > Thanks, > > Benjamin Bannier > >
Re: Review Request 71208: Revert "Updated cpplint to be compatible with Python 3."
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/71208/#review217442 --- Ship it! - Till Toenshoff 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/3/ > > > Testing > --- > > * used for development for a couple of months > * reports no issues in the current source tree > > > Thanks, > > Benjamin Bannier > >
Re: Review Request 71206: Removed old mesos-style and references.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/71206/#review217444 --- Ship it! - Till Toenshoff On Aug. 20, 2019, 11:49 a.m., Benjamin Bannier wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/71206/ > --- > > (Updated Aug. 20, 2019, 11:49 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 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/5/ > > > Testing > --- > > n/a > > > Thanks, > > Benjamin Bannier > >
Re: Review Request 71205: Switched commit hooks to pre-commit.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/71205/#review217439 --- Fix it, then Ship it! When running the script, I noticed this ``` [INFO] Installing environment for local. [INFO] Once installed this environment will be reused. [INFO] This may take a few minutes... [INFO] Installing environment for local. [INFO] Once installed this environment will be reused. [INFO] This may take a few minutes... ``` Why do we seemingly repeat that step? After this all further steps were non duplicated. docs/advanced-contribution.md Line 67 (original), 68 (patched) <https://reviews.apache.org/r/71205/#comment304711> "The hooks are set up with `./support/setup_dev.sh`" We have to invoke it, so it does not really do much automatically, I'ld argue. support/setup-dev.sh Lines 57 (patched) <https://reviews.apache.org/r/71205/#comment304712> We generally strive for capitalizing product names - so let's have it this `Mesos` way everywhere please :) support/setup-dev.sh Lines 62 (patched) <https://reviews.apache.org/r/71205/#comment304710> Shouldnt there be an `exit(1)` here so we spare our users three more error messages? - Till Toenshoff On Aug. 20, 2019, 11:48 a.m., Benjamin Bannier wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/71205/ > --- > > (Updated Aug. 20, 2019, 11:48 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/8/ > > > Testing > --- > > * used successfully for a couple of months > > > Thanks, > > Benjamin Bannier > >
Re: Review Request 71299: Added separate script to install developer setup.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/71299/#review217434 --- Ship it! Excellent structure - pretty much a move code only review, making it easy for us - Thanks!! support/setup-dev.sh Lines 29-48 (patched) <https://reviews.apache.org/r/71299/#comment304709> These still are rather unfortunate, I feel. - Till Toenshoff On Aug. 20, 2019, 11:48 a.m., Benjamin Bannier wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/71299/ > --- > > (Updated Aug. 20, 2019, 11:48 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/5/ > > > Testing > --- > > > Thanks, > > Benjamin Bannier > >
Re: Review Request 71204: Added gitlint config.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/71204/#review217433 --- Ship it! - Till Toenshoff 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 71203: Added check script to check for license headers.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/71203/#review217432 --- Ship it! support/check-license.py Lines 25-26 (patched) <https://reviews.apache.org/r/71203/#comment304708> Nifty python code :) - Till Toenshoff 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 70096: Moved cpplint configuration into dedicated file.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/70096/#review217431 --- Ship it! bootstrap Lines 84 (patched) <https://reviews.apache.org/r/70096/#comment304707> Too bad cpplint does not allow for hidden config files like e.g. `.cpplintrc` - but this code tells us; https://github.com/cpplint/cpplint/blob/master/cpplint.py#L6220-L6228 - Till Toenshoff 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 71338: Moved OpenSSL-related ifdef to a central location.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/71338/#review217353 --- Fix it, then Ship it! Thanks Benno for fixing my ugly patch. 3rdparty/libprocess/src/openssl.cpp Lines 55 (patched) <https://reviews.apache.org/r/71338/#comment304641> As nicely noted by BenjaminB in my previous less beautiful RR, we might want to explain what version this was; "(OpenSSL 1.1.0)" as done above. - Till Toenshoff On Aug. 21, 2019, 1:34 p.m., Benno Evers wrote: > > --- > 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 > - > > 3rdparty/libprocess/src/openssl.cpp > 4aeab855b29837dc6bf93104afc62f111c38626c > > > Diff: https://reviews.apache.org/r/71338/diff/2/ > > > Testing > --- > > > Thanks, > > Benno Evers > >
Re: Review Request 71336: Fixed deprecation warning when building against OpenSSL 1.1.x.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/71336/ --- (Updated Aug. 21, 2019, 11:45 a.m.) Review request for mesos, Benno Evers and Benjamin Mahler. Bugs: MESOS-9353 https://issues.apache.org/jira/browse/MESOS-9353 Repository: mesos Description --- OpenSSL 1.1.x deprecates 'ASN1_STRING_get_data' and replaces it by 'ASN1_STRING_get0_data'. Diffs - 3rdparty/libprocess/src/openssl.cpp ee9a049d06bdf6365930aa92873dd97af197eba0 Diff: https://reviews.apache.org/r/71336/diff/1/ Testing --- ``` cmake .. -DUNBUNDLED_ZOOKEEPER=ON -DUNBUNDLED_LIBEVENT=ON -DUNBUNDLED_LEVELDB=ON -DUNBUNDLED_LIBARCHIVE=ON -DENABLE_SSL=ON -DENABLE_LIBEVENT=ON -DOPENSSL_ROOT_DIR=/usr/local/opt/openssl@1.1 -DLIBEVENT_ROOT_DIR=/Users/till/usr -DCMAKE_EXPORT_COMPILE_COMMANDS=1 -G Ninja ninja tests -j12 ``` Thanks, Till Toenshoff
Review Request 71336: Fixed deprecation warning when building against OpenSSL 1.1.x.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/71336/ --- Review request for mesos, Benno Evers and Benjamin Mahler. Repository: mesos Description --- OpenSSL 1.1.x deprecates 'ASN1_STRING_get_data' and replaces it by 'ASN1_STRING_get0_data'. Diffs - 3rdparty/libprocess/src/openssl.cpp ee9a049d06bdf6365930aa92873dd97af197eba0 Diff: https://reviews.apache.org/r/71336/diff/1/ Testing --- ``` cmake .. -DUNBUNDLED_ZOOKEEPER=ON -DUNBUNDLED_LIBEVENT=ON -DUNBUNDLED_LEVELDB=ON -DUNBUNDLED_LIBARCHIVE=ON -DENABLE_SSL=ON -DENABLE_LIBEVENT=ON -DOPENSSL_ROOT_DIR=/usr/local/opt/openssl@1.1 -DLIBEVENT_ROOT_DIR=/Users/till/usr -DCMAKE_EXPORT_COMPILE_COMMANDS=1 -G Ninja ninja tests -j12 ``` Thanks, Till Toenshoff
Re: Review Request 71232: Renamed cmake parameter for parallel test execution.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/71232/#review217050 --- Fix it, then Ship it! cmake/MesosConfigure.cmake Line 49 (original), 49 (patched) <https://reviews.apache.org/r/71232/#comment304271> s/no/not/ - Till Toenshoff On Aug. 2, 2019, 7:15 a.m., Benjamin Bannier wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/71232/ > --- > > (Updated Aug. 2, 2019, 7:15 a.m.) > > > Review request for mesos, Benno Evers and Till Toenshoff. > > > Bugs: MESOS-6382 > https://issues.apache.org/jira/browse/MESOS-6382 > > > Repository: mesos > > > Description > --- > > The Jenkins setup (which uses `support/docker-build.sh` under the > covers) is parameterized with the `CONFIGURATION` environment variable. > While in we pass configure-style flags for both autotools and cmake > builds to it in the Jenkins > configuration, the script performs transformations so that > configure-style flags are transformed to cmake-style (replace `_` with > `-`, uppercase flags, replace `--` with `-D`). > > We disable parallel test execution in Jenkins by passing > `--disable-parallel-test-execution` which with the transformations in > `support/docker-build.sh` leads to a cmake arg > `-DDISABLE_PARALLEL_TEST_EXECUTION=1`. This patch renames the cmake arg > from a default enabled `ENABLE_PARALLEL_TEST_EXECUTION` to a default > disabled `DISABLE_PARALLEL_TEST_EXECUTION` to support this workflow. > > > Diffs > - > > cmake/MesosConfigure.cmake ca8d406e1be9d6ab50c6c9dc4d2de97189b17b9a > > > Diff: https://reviews.apache.org/r/71232/diff/1/ > > > Testing > --- > > Tested ctest behavior with both the new flag enabled and disabled. > > > Thanks, > > Benjamin Bannier > >
Review Request 71138: Updated configure.ac to correct openssl/libevent setup.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/71138/ --- Review request for mesos and Vinod Kone. Repository: mesos Description --- Fixes a problem introduced by configure.ac reordering in commit 4df2b62. Diffs - configure.ac 0e3058c2bcd730a0992ca497809ab09c58ed6fa1 Diff: https://reviews.apache.org/r/71138/diff/1/ Testing --- Tested within DC/OS build CI which failed without this fix. Thanks, Till Toenshoff
Re: Review Request 71123: Made openssl configuration in 'configure.ac' occur earlier.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/71123/#review216765 --- Ship it! Ship It! - Till Toenshoff On July 19, 2019, 10:49 a.m., Benno Evers wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/71123/ > --- > > (Updated July 19, 2019, 10:49 a.m.) > > > 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 70991: Added ability to pass custom SSL context to `Socket::connect()`.
> On July 4, 2019, 11:53 p.m., Till Toenshoff wrote: > > 3rdparty/libprocess/src/openssl.cpp > > Line 806 (original), 807 (patched) > > <https://reviews.apache.org/r/70991/diff/2/?file=2153776#file2153776line807> > > > > s/libprocess/legacy/ Actually, please make sure you dont end up with ``` // i.e. the lagacy 'legacy' scheme ``` :D - Till --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/70991/#review216396 --- On July 4, 2019, 7 p.m., Benno Evers wrote: > > --- > 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. > > > 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 > - > > 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/ > > > Testing > --- > > > Thanks, > > Benno Evers > >
Re: Review Request 70991: Added ability to pass custom SSL context to `Socket::connect()`.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/70991/#review216396 --- Fix it, then Ship it! Awesome work Benno - thanks! Can't wait to see these great additions landing. 3rdparty/libprocess/include/process/ssl/tls_config.hpp Lines 59 (patched) <https://reviews.apache.org/r/70991/#comment303611> ``` (See also the comments on the typedefs above.) ``` Looking at the callback type comments is somewhat implicit, i feel - we can do without that part. 3rdparty/libprocess/include/process/ssl/tls_config.hpp Lines 65-69 (patched) <https://reviews.apache.org/r/70991/#comment303613> I would suggest to split this up into smaller digestable sentences - native speaker please?! Otherwise maybe like this? ``` // Returns `TLSClientConfig` object configured with provided `servername` as // well as the global libprocess SSL context. The callbacks for `verify` and // `configure_socket` get setup following the SSL behaviour configured via // the `LIBPROCESS_SSL_*` environment variables. ``` 3rdparty/libprocess/include/process/ssl/tls_config.hpp Lines 80 (patched) <https://reviews.apache.org/r/70991/#comment303614> s/libprocess/legacy/ 3rdparty/libprocess/src/openssl.cpp Line 806 (original), 807 (patched) <https://reviews.apache.org/r/70991/#comment303615> s/libprocess/legacy/ - Till Toenshoff On July 4, 2019, 7 p.m., Benno Evers wrote: > > --- > 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. > > > 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 > - > > 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/ > > > Testing > --- > > > Thanks, > > Benno Evers > >
Re: Review Request 70991: Updated `Socket::connect()` API according to maintainer feedback.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/70991/#review216383 --- 3rdparty/libprocess/src/poll_socket.hpp Lines 16-18 (patched) <https://reviews.apache.org/r/70991/#comment303604> No need for #ifdef. 3rdparty/libprocess/src/poll_socket.hpp Lines 16-18 (patched) <https://reviews.apache.org/r/70991/#comment303605> No need for #ifdef. - Till Toenshoff On July 2, 2019, 5:50 p.m., Benno Evers wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/70991/ > --- > > (Updated July 2, 2019, 5:50 p.m.) > > > Review request for mesos, Benjamin Mahler, Joseph Wu, and Till Toenshoff. > > > Bugs: MESOS-9878 > https://issues.apache.org/jira/browse/MESOS-9878 > > > 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 70921: Added OpenSSL-related changes to CHANGELOG.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/70921/#review216382 --- Fix it, then Ship it! Ship It! CHANGELOG Lines 9 (patched) <https://reviews.apache.org/r/70921/#comment303598> s/should/will/ CHANGELOG Lines 27 (patched) <https://reviews.apache.org/r/70921/#comment303599> How about: ... will now forego TLS certificate verification ... CHANGELOG Lines 27 (patched) <https://reviews.apache.org/r/70921/#comment303600> How about: ... will now forego TLS certificate verification ... docs/upgrades.md Lines 516 (patched) <https://reviews.apache.org/r/70921/#comment303601> s/change/configure/ docs/upgrades.md Lines 526 (patched) <https://reviews.apache.org/r/70921/#comment303602> How about? ``` * For incoming connections, certificates are not verified unless `LIBPROCESS_SSL_REQUIRE_CERT` is set to true. ``` docs/upgrades.md Lines 527 (patched) <https://reviews.apache.org/r/70921/#comment303603> 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. - Till Toenshoff On June 21, 2019, 3:02 p.m., Benno Evers wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/70921/ > --- > > (Updated June 21, 2019, 3:02 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/2/ > > > Testing > --- > > > Thanks, > > Benno Evers > >
Re: Review Request 70993: Added warnings about known problems with libevent epoll backend.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/70993/#review216381 --- Ship it! Ship It! - Till Toenshoff On July 2, 2019, 5:53 p.m., Benno Evers wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/70993/ > --- > > (Updated July 2, 2019, 5:53 p.m.) > > > 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 > >
Re: Review Request 70933: Moved an inline duration for slow DNS logging into a const variable.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/70933/#review216380 --- Ship it! Ship It! - Till Toenshoff 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 > 19d25a89f7dda1f6c66dd1ffc5051e35457d26b0 > > > Diff: https://reviews.apache.org/r/70933/diff/2/ > > > Testing > --- > > > Thanks, > > Benno Evers > >
Re: Review Request 70797: Added unit tests for hostname validation.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/70797/#review216379 --- Ship it! Ship It! - Till Toenshoff On July 2, 2019, 5:53 p.m., Benno Evers wrote: > > --- > 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 > --- > > 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 > - > > 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/ > > > Testing > --- > > > Thanks, > > Benno Evers > >
Re: Review Request 70797: Added unit tests for hostname validation.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/70797/#review216378 --- 3rdparty/libprocess/src/tests/ssl_tests.cpp Line 318 (original), 368 (patched) <https://reviews.apache.org/r/70797/#comment303597> s/incorrect/not matching/ - Till Toenshoff On July 2, 2019, 5:53 p.m., Benno Evers wrote: > > --- > 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 > --- > > 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 > - > > 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/ > > > Testing > --- > > > Thanks, > > Benno Evers > >
Re: Review Request 70992: Recorded Socket API change in CHANGELOG.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/70992/#review216377 --- Ship it! Ship It! - Till Toenshoff On July 4, 2019, 2:28 p.m., Benno Evers wrote: > > --- > 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. > > > Repository: mesos > > > Description > --- > > Recorded Socket API change in CHANGELOG. > > > Diffs > - > > CHANGELOG ecd01d6f415859eb6b9cfa3468db02d1810a3998 > > > Diff: https://reviews.apache.org/r/70992/diff/2/ > > > Testing > --- > > > Thanks, > > Benno Evers > >
Re: Review Request 70795: Updated SSL docs to include new libprocess flag.
> 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. > > Benno Evers wrote: > Created https://issues.apache.org/jira/browse/MESOS-9857 to track the > change. Great - next we would update all relevant documentation with a deprecation note and a reference of that ticket. Right now I am contemplating doing this in a single run, right away, instead of multiple phases. Multiple phases would which would allow us to have that `libprocess` default without having to warn about it. What do you think? We would ... - add a comment note here - add an SSL flags description note - possibly have the flags validation output a deprecation warning - anything I forgot here? - Till --- 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, Benjamin Mahler, 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 70948: Attempting to get Jenkins pipeline build expiry fixed.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/70948/ --- (Updated June 26, 2019, 12:16 a.m.) Review request for mesos, Benjamin Bannier, Gilbert Song, and Vinod Kone. Repository: mesos Description --- Attempting to get Jenkins pipeline build expiry fixed. Diffs (updated) - support/jenkins/Jenkinsfile-packaging-centos aa8f37f6cd661e767e55ca75095a91827ebedb21 Diff: https://reviews.apache.org/r/70948/diff/2/ Changes: https://reviews.apache.org/r/70948/diff/1-2/ Testing --- none - cannot test this without committing it right now info for the option in use is taken from: https://stackoverflow.com/a/53141684/91282 Thanks, Till Toenshoff
Review Request 70948: Attempting to get Jenkins pipeline build expiry fixed.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/70948/ --- Review request for mesos, Benjamin Bannier, Gilbert Song, and Vinod Kone. Repository: mesos Description --- Attempting to get Jenkins pipeline build expiry fixed. Diffs - support/jenkins/Jenkinsfile-packaging-centos aa8f37f6cd661e767e55ca75095a91827ebedb21 Diff: https://reviews.apache.org/r/70948/diff/1/ Testing --- none - cannot test this without committing it right now info for the option in use is taken from: https://stackoverflow.com/a/53141684/91282 Thanks, Till Toenshoff
Re: Review Request 70795: Updated SSL docs to include new libprocess flag.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/70795/#review216020 --- Fix it, then Ship it! Great! Please don't forget to update the review description to match the changed naming. docs/ssl.md Lines 176 (patched) <https://reviews.apache.org/r/70795/#comment302972> s/can not/cannot/ docs/ssl.md Lines 194 (patched) <https://reviews.apache.org/r/70795/#comment302973> 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? - Till Toenshoff 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.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/70749/#review216017 --- 3rdparty/libprocess/src/openssl.cpp Lines 147 (patched) <https://reviews.apache.org/r/70749/#comment302961> s/algorithm/scheme/? 3rdparty/libprocess/src/openssl.cpp Lines 153 (patched) <https://reviews.apache.org/r/70749/#comment302962> s/algorithm/scheme/? Also, we should tell the user which version he needs at least here, nice touch ;) 3rdparty/libprocess/src/openssl.cpp Lines 565-566 (patched) <https://reviews.apache.org/r/70749/#comment302963> Should we ``` EXIT(EXIT_FAILURE) << "Unknown value for hostname_validation_scheme: " << ssl_flags->hostname_validation_scheme; ``` instead here and below to be perfectly explicit and consistent? 3rdparty/libprocess/src/openssl.cpp Lines 576 (patched) <https://reviews.apache.org/r/70749/#comment302964> s/algorithm/scheme/? 3rdparty/libprocess/src/openssl.cpp Line 755 (original), 788 (patched) <https://reviews.apache.org/r/70749/#comment302965> Maybe ``` // When using the OpenSSL built-in scheme,... ``` 3rdparty/libprocess/src/openssl.cpp Lines 795 (patched) <https://reviews.apache.org/r/70749/#comment302966> s/algorithm/scheme/ 3rdparty/libprocess/src/openssl.cpp Lines 808 (patched) <https://reviews.apache.org/r/70749/#comment302967> 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 3rdparty/libprocess/src/openssl.cpp Lines 984-985 (patched) <https://reviews.apache.org/r/70749/#comment302968> 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. 3rdparty/libprocess/src/openssl.cpp Lines 989 (patched) <https://reviews.apache.org/r/70749/#comment302969> Shall we explain why this is the `right` way - aka best practices? 3rdparty/libprocess/src/openssl.cpp Lines 1016 (patched) <https://reviews.apache.org/r/70749/#comment302970> s/ip/IP/ 3rdparty/libprocess/src/posix/libevent/libevent_ssl_socket.cpp Lines 444-448 (patched) <https://reviews.apache.org/r/70749/#comment302999> Great comment - thanks! 3rdparty/libprocess/src/posix/libevent/libevent_ssl_socket.cpp Lines 569 (patched) <https://reviews.apache.org/r/70749/#comment303000> Now that we log the `peer_hostname`, I wonder if it made sense to also log the `peer_ip` like this - what do you think? If you agree, then we should even try to render a single log-line out of those two - it will be noisy enough on level 2. 3rdparty/libprocess/src/posix/libevent/libevent_ssl_socket.cpp Lines 1129 (patched) <https://reviews.apache.org/r/70749/#comment303002> I feel `peername` comes in a bit surprising here if one does not know about `getpeername()`. Maybe s/peername/IP address/? 3rdparty/libprocess/src/posix/libevent/libevent_ssl_socket.cpp Lines 1136 (patched) <https://reviews.apache.org/r/70749/#comment303001> As mentioned in some other place of this chain, let's add a neat JIRA on this plan. - Till Toenshoff 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 > -
Re: Review Request 70796: Fixed Mesos unit tests after Address API change.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/70796/#review216016 --- Ship it! - Till Toenshoff 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/70796/ > --- > > (Updated June 19, 2019, 2:44 p.m.) > > > Review request for mesos, Alexander Rukletsov, Joseph Wu, and Till Toenshoff. > > > Repository: mesos > > > Description > --- > > Adjusts some Mesos unit tests after renaming > `Address::hostname()` to `Address::lookup_hostname()`. > > > Diffs > - > > src/tests/master_tests.cpp 097f1b77a59e29c6690210773d1556ebf2bb701e > > > Diff: https://reviews.apache.org/r/70796/diff/2/ > > > Testing > --- > > > Thanks, > > Benno Evers > >
Re: Review Request 70885: Renamed 'libprocess::network::Address::hostname()'.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/70885/#review216015 --- Ship it! Thanks a bunch - much better signal for a pricy function now :) - Till Toenshoff 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 > >
Re: Review Request 70883: Added optional 'peer_hostname' argument to Socket::connect().
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/70883/#review216013 --- Ship it! We have discussed this patch a bit out of band and agreed that passing the `peer_hostname` as an additional argument into the `process::connect` function and socket stack below was the most sensible we can do. Alternatives like passing as a new `network::Address` member has surprising conversion/typecasting results while exploiting the idea of a network address. Adding a setter to our `Socket` might be another option but here we would have to supply a noop member of the non SSL variant. With both variants we would have widened the scope of this argument to areas where it does not play a role. 3rdparty/libprocess/include/process/socket.hpp Lines 371 (patched) <https://reviews.apache.org/r/70883/#comment302959> Too bad we can't default. - Till Toenshoff On June 19, 2019, 2:34 p.m., Benno Evers wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/70883/ > --- > > (Updated June 19, 2019, 2:34 p.m.) > > > Review request for mesos, 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 > 29a1bf71c1df9d80370455a6269ecea0ec4193b0 > 3rdparty/libprocess/src/posix/poll_socket.cpp > 74acb6942682a9d9626df81b303eba0a1c24ecf7 > 3rdparty/libprocess/src/windows/poll_socket.cpp > 565b0088dc2b270193e615655f57f48419eb2c12 > > > Diff: https://reviews.apache.org/r/70883/diff/1/ > > > Testing > --- > > > Thanks, > > Benno Evers > >
Re: Review Request 70810: Updated SSL docs with suggested runtime configuration.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/70810/#review216011 --- Ship it! Awesome - thanks! - Till Toenshoff 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 70748: Changed semantics of some libprocess TLS flags.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/70748/#review216009 --- Fix it, then Ship it! Great - thanks so much! 3rdparty/libprocess/src/openssl.cpp Line 90 (original), 90 (patched) <https://reviews.apache.org/r/70748/#comment302996> `verify_cert` effectively becomes a TLS client mode specific setting - we should explain that here. In my mind, in TLS server mode, `verify_cert` stricly follows `require_cert` in its state. 3rdparty/libprocess/src/openssl.cpp Line 95 (original), 95 (patched) <https://reviews.apache.org/r/70748/#comment302994> This is a TLS server mode only setting by now - we need to call that out I feel. In TLS client mode, we have `require_cert` enabled implicitly. 3rdparty/libprocess/src/openssl.cpp Line 96 (original), 96 (patched) <https://reviews.apache.org/r/70748/#comment302997> Additional to the updates above, we also need to call this out in the CHANGELOG. Can you please include that in this chain? 3rdparty/libprocess/src/openssl.cpp Lines 743-745 (patched) <https://reviews.apache.org/r/70748/#comment302956> This is so important, I would like to add more meat to it; Let's clarify that the OpenSSL handshake itself will do this, before we even reach our additional verification. Also, the use of `remote` confuses me - do local servers have an exception here? Maybe something like this? ``` // NOTE: Even without this check, the OpenSSL handshake will // not complete when connecting to servers that do not present // a certificate, unless an anonymous cipher is used. ``` 3rdparty/libprocess/src/tests/ssl_tests.cpp Lines 296-297 (patched) <https://reviews.apache.org/r/70748/#comment302957> Nice, thank you! - Till Toenshoff 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 70747: Updated some flag description for libprocess SSL flags.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/70747/#review215968 --- Ship it! Ship It! - Till Toenshoff On May 29, 2019, 1:50 p.m., Benno Evers wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/70747/ > --- > > (Updated May 29, 2019, 1:50 p.m.) > > > Review request for mesos and Alexander Rukletsov. > > > Repository: mesos > > > Description > --- > > Changed `SSL` to `TLS` in some flag descriptions where the > flag was referring to versions of the TLS protocol. > > > Diffs > - > > 3rdparty/libprocess/src/openssl.cpp > e7dbd67913fa8e7fbbf60dee428e7e38895f86ce > > > Diff: https://reviews.apache.org/r/70747/diff/1/ > > > Testing > --- > > > Thanks, > > Benno Evers > >
Re: Review Request 70748: Disallow verification of empty TLS server certificates.
---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/70748/#review215878
---
3rdparty/libprocess/src/openssl.cpp
Line 738 (original), 745 (patched)
<https://reviews.apache.org/r/70748/#comment302818>
This means that when a certificate gets provided, we always validate it.
Entirely straightforward but may be too much of a potential problem source in
the real world, I fear.
IIUC then client certificate validation is exactly a non goal of this
review-chain as client certificates in real world scenarios often times do not
match. My feeling is that we have a great chance of reducing potential problem
sources by working more like this;
CLIENT mode (we initiate a connection)
- when we receive a peer certificate, we verify it
- when we receive no peer certificate, we error out
SERVER mode (we accept a connection)
- `require` is set to false
- when we receive a peer certificate, we ignore it
- when we receive no peer certificate, we ignore it
We would return early as done for verify above like this;
```
if (mode == Mode::SERVER && !ssl_flags->require_cert) {
return Nothing();
}
```
Then relax this part towards;
```
if (cert == nullptr) {
return Error("Peer did not provide certificate");
}
```
- Till Toenshoff
On June 6, 2019, 11:12 p.m., Benno Evers wrote:
>
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/70748/
> ---
>
> (Updated June 6, 2019, 11:12 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
> ---
>
> When in SSL client mode and `LIBPROCESS_SSL_VERIFY_CERT=true` has
> been set, enforce that the server actually presents a certificate
> that can be verified.
>
> Note that in most cases, the TLS stack would rejected the connection
> before the code ever reaches `openssl::verify()`, since the TLS
> specification that a server MUST always send a certificate unless
> an anonymous cipher is used.
>
>
> 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/2/
>
>
> Testing
> ---
>
>
> Thanks,
>
> Benno Evers
>
>
Re: Review Request 70811: Fixed compilation error on Mac OS.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/70811/#review215797 --- Ship it! Ship It! - Till Toenshoff On June 7, 2019, 5:21 p.m., Andrei Budnik wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/70811/ > --- > > (Updated June 7, 2019, 5:21 p.m.) > > > Review request for mesos, Benjamin Bannier, Gilbert Song, James Peach, and > Jiang Yan Xu. > > > Repository: mesos > > > Description > --- > > This patch adds missing switch case to fix compilation error introduced > in `bc5a57122635`. > > > Diffs > - > > src/slave/containerizer/mesos/launch.cpp > a69a68823842171bb15ddf34a504e5ce4af232b0 > > > Diff: https://reviews.apache.org/r/70811/diff/2/ > > > Testing > --- > > internal CI > > > Thanks, > > Andrei Budnik > >
Re: Review Request 70811: Fixed compilation error on Mac OS.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/70811/#review215793 --- src/slave/containerizer/mesos/launch.cpp Line 508 (original), 509 (patched) <https://reviews.apache.org/r/70811/#comment302672> I am 100% with BenB here - silently ignoring seems a ticket for hard to explain / debug problems. Let's please return an `Error` suggesting that this setting was not supported on macOS instead. - Till Toenshoff On June 7, 2019, 5:21 p.m., Andrei Budnik wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/70811/ > --- > > (Updated June 7, 2019, 5:21 p.m.) > > > Review request for mesos, Benjamin Bannier, Gilbert Song, James Peach, and > Jiang Yan Xu. > > > Repository: mesos > > > Description > --- > > This patch adds missing switch case to fix compilation error introduced > in `bc5a57122635`. > > > Diffs > - > > src/slave/containerizer/mesos/launch.cpp > a69a68823842171bb15ddf34a504e5ce4af232b0 > > > Diff: https://reviews.apache.org/r/70811/diff/1/ > > > Testing > --- > > internal CI > > > Thanks, > > Andrei Budnik > >
Review Request 70829: Added logrotate option to force Jenkins into removing older artefacts.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/70829/ --- Review request for mesos, Benjamin Bannier and Vinod Kone. Repository: mesos Description --- See summary. Diffs - support/jenkins/Jenkinsfile-packaging-centos de688f45f6530c48f13930cbd78f772873a1bc84 Diff: https://reviews.apache.org/r/70829/diff/1/ Testing --- Untested but based on documentation and hope: https://support.cloudbees.com/hc/en-us/articles/115000237071-How-do-I-set-discard-old-builds-for-a-Multi-Branch-Pipeline-Job- This should remove old logfiles and artefacts after 30 days. Thanks, Till Toenshoff
Re: Review Request 70562: Add LIBPROCESS_SSL_ENABLE_TLS_V1_3
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/70562/#review214949 --- Ship it! Thanks so much Stephane. Looks slick -- Benno will go ahead and land this with the small fix he mentioned already (thanks Benno :)) - Till Toenshoff On April 29, 2019, 6:43 a.m., Stéphane Cottin wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/70562/ > --- > > (Updated April 29, 2019, 6:43 a.m.) > > > Review request for mesos and Benno Evers. > > > Bugs: MESOS-9730 > https://issues.apache.org/jira/browse/MESOS-9730 > > > Repository: mesos > > > Description > --- > > When building mesos with libopenssl >= 1.1.1, TLS1.3 is enabled by default. > This causes major communication issues between executors and agents. > > This patch adds new `LIBPROCESS_SSL_ENABLE_TLS_V1_3` env var, disabled by > default. > Should be changed to enabled by default when full openssl >= 1.1 support will > land. > > > Diffs > - > > 3rdparty/libprocess/include/process/ssl/flags.hpp 3806266fb > 3rdparty/libprocess/include/process/ssl/gtest.hpp e173b3224 > 3rdparty/libprocess/src/openssl.hpp 0c4192f90 > 3rdparty/libprocess/src/openssl.cpp a4d503642 > 3rdparty/libprocess/src/tests/ssl_tests.cpp 5e994498d > > > Diff: https://reviews.apache.org/r/70562/diff/1/ > > > Testing > --- > > > Thanks, > > Stéphane Cottin > >
Re: Review Request 70563: document LIBPROCESS_SSL_ENABLE_TLS_V1_3
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/70563/#review214948 --- Ship it! Ship It! - Till Toenshoff On April 29, 2019, 6:43 a.m., Stéphane Cottin wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/70563/ > --- > > (Updated April 29, 2019, 6:43 a.m.) > > > Review request for mesos and Benno Evers. > > > Bugs: MESOS-9730 > https://issues.apache.org/jira/browse/MESOS-9730 > > > Repository: mesos > > > Description > --- > > update documentation about `LIBPROCESS_SSL_ENABLE_TLS_V1_3` and TLS1.3 > > > Diffs > - > > docs/ssl.md e6c118121 > > > Diff: https://reviews.apache.org/r/70563/diff/1/ > > > Testing > --- > > > Thanks, > > Stéphane Cottin > >
Re: Review Request 70526: Rearranged 'Downloads' page and updated bintray URL.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/70526/#review214854 --- Ship it! As discussed out of band, we will try to preserve all the older releases when transitioning to the new URL - thanks a bunch Benno! - Till Toenshoff On April 23, 2019, 2:32 p.m., Benno Evers wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/70526/ > --- > > (Updated April 23, 2019, 2:32 p.m.) > > > Review request for mesos, Greg Mann and Till Toenshoff. > > > Bugs: MESOS-9697 > https://issues.apache.org/jira/browse/MESOS-9697 > > > Repository: mesos > > > Description > --- > > Updated the bintray link on the `Downloads` page to point > to the `apache/mesos` account instead of the `mesos` > account. > > In addition, several minor formatting changes were done: > > * Added a space after the colon in the `Getting older > Mesos binaries` section. > > * Moved links to the getting started guide to the top > of the document. > > * Used a list to present the download links to the latest > stable release. > > * Used `` instead of `` tags for the link to the > ASF git repository. > > > Diffs > - > > site/source/downloads.html.erb d5a165c71378a7c268b9c2e73b60902d632463fb > > > Diff: https://reviews.apache.org/r/70526/diff/1/ > > > Testing > --- > > Visual inspection after using Firefox to edit the HTML: > https://drive.google.com/file/d/1PuLGmb3h3pj3xNXciVGWPQzGBHJqHvk5/view?usp=sharing > > > Thanks, > > Benno Evers > >
Re: Review Request 70489: Documented docker manifest v2 schema2 support.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/70489/#review214707 --- docs/container-image.md Lines 188-190 (patched) <https://reviews.apache.org/r/70489/#comment300926> maybe a bit more like this? ``` For privacte registries, the operator needs to configure `curl` with the location of required CA certificates. ``` - Till Toenshoff On April 16, 2019, 11:39 p.m., Gilbert Song wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/70489/ > --- > > (Updated April 16, 2019, 11:39 p.m.) > > > Review request for mesos, Andrei Budnik, Jie Yu, and Qian Zhang. > > > Repository: mesos > > > Description > --- > > Documented docker manifest v2 schema2 support. > > > Diffs > - > > docs/container-image.md 3c4520ad4a5391b1202319bb8db27b4d1c07a96b > > > Diff: https://reviews.apache.org/r/70489/diff/1/ > > > Testing > --- > > > Thanks, > > Gilbert Song > >
Re: Review Request 70323: Bumped nokogiri and rack site dependencies.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/70323/#review214115 --- Ship it! Ship It! - Till Toenshoff On March 27, 2019, 4:21 p.m., Benjamin Bannier wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/70323/ > --- > > (Updated March 27, 2019, 4:21 p.m.) > > > Review request for mesos, Benjamin Mahler and Till Toenshoff. > > > Repository: mesos > > > Description > --- > > This bumps nokogiri to >=1.8.5 for CVE-2018-16471 and rack to > >=1.6.11 for CVE-2018-16471. > > > Diffs > - > > site/Gemfile.lock b3f364945b7dea7cd4696cbb7e956c3ed7bfb59a > > > Diff: https://reviews.apache.org/r/70323/diff/1/ > > > Testing > --- > > `bundle install && bundle exec middleman` > > > Thanks, > > Benjamin Bannier > >
Re: Review Request 70233: Allowed for optionally unbundled ZooKeeper for CMake builds.
> On March 19, 2019, 11:07 p.m., Joseph Wu wrote: > > 3rdparty/cmake/FindZOOKEEPER.cmake > > Lines 50 (patched) > > <https://reviews.apache.org/r/70233/diff/1/?file=2132719#file2132719line50> > > > > I wonder if it is worth adding a note here, restating bits of the blurb > > here: > > > > https://github.com/apache/zookeeper/blob/master/zookeeper-client/zookeeper-client-c/README#L26-L32 > > > > We don't have a note in the autotools build. Yeah, why not - its a good note... > On March 19, 2019, 11:07 p.m., Joseph Wu wrote: > > cmake/CompilationConfigure.cmake > > Lines 115 (patched) > > <https://reviews.apache.org/r/70233/diff/1/?file=2132720#file2132720line115> > > > > Using a Windows style path here is... a bit funny to me. Nothing wrong > > with it though. You are perfectly right, the last few times I followed this pattern, I smiled :D. - Till --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/70233/#review213810 --- On March 19, 2019, 12:11 a.m., Till Toenshoff wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/70233/ > --- > > (Updated March 19, 2019, 12:11 a.m.) > > > Review request for mesos, Benjamin Bannier and Joseph Wu. > > > Bugs: MESOS-9662 > https://issues.apache.org/jira/browse/MESOS-9662 > > > Repository: mesos > > > Description > --- > > Allowed for optionally unbundled ZooKeeper for CMake builds. > > > Diffs > - > > 3rdparty/CMakeLists.txt 1999dd20964da96bc5acfbd47cb80d4ca6f734b9 > 3rdparty/cmake/FindZOOKEEPER.cmake PRE-CREATION > cmake/CompilationConfigure.cmake d9c1e40a59eebbb295aa993682edb8ccbf2b48f7 > > > Diff: https://reviews.apache.org/r/70233/diff/3/ > > > Testing > --- > > > Thanks, > > Till Toenshoff > >
Re: Review Request 70233: Allowed for optionally unbundled ZooKeeper for CMake builds.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/70233/ --- (Updated March 19, 2019, 12:11 a.m.) Review request for mesos, Benjamin Bannier and Joseph Wu. Bugs: MESOS-9662 https://issues.apache.org/jira/browse/MESOS-9662 Repository: mesos Description --- Allowed for optionally unbundled ZooKeeper for CMake builds. Diffs (updated) - 3rdparty/CMakeLists.txt 1999dd20964da96bc5acfbd47cb80d4ca6f734b9 3rdparty/cmake/FindZOOKEEPER.cmake PRE-CREATION cmake/CompilationConfigure.cmake d9c1e40a59eebbb295aa993682edb8ccbf2b48f7 Diff: https://reviews.apache.org/r/70233/diff/3/ Changes: https://reviews.apache.org/r/70233/diff/2-3/ Testing --- Thanks, Till Toenshoff
Re: Review Request 70233: Allowed for optionally unbundled ZooKeeper for CMake builds.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/70233/ --- (Updated March 19, 2019, 12:03 a.m.) Review request for mesos, Benjamin Bannier and Joseph Wu. Bugs: MESOS-9662 https://issues.apache.org/jira/browse/MESOS-9662 Repository: mesos Description --- Allowed for optionally unbundled ZooKeeper for CMake builds. Diffs (updated) - 3rdparty/CMakeLists.txt 1999dd20964da96bc5acfbd47cb80d4ca6f734b9 3rdparty/cmake/FindZOOKEEPER.cmake PRE-CREATION cmake/CompilationConfigure.cmake d9c1e40a59eebbb295aa993682edb8ccbf2b48f7 Diff: https://reviews.apache.org/r/70233/diff/2/ Changes: https://reviews.apache.org/r/70233/diff/1-2/ Testing --- Thanks, Till Toenshoff
Review Request 70233: Allowed for optionally unbundled ZooKeeper for CMake builds.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/70233/ --- Review request for mesos, Benjamin Bannier and Joseph Wu. Bugs: MESOS-9662 https://issues.apache.org/jira/browse/MESOS-9662 Repository: mesos Description --- Allowed for optionally unbundled ZooKeeper for CMake builds. Diffs - 3rdparty/CMakeLists.txt 1999dd20964da96bc5acfbd47cb80d4ca6f734b9 3rdparty/cmake/FindZOOKEEPER.cmake PRE-CREATION cmake/CompilationConfigure.cmake d9c1e40a59eebbb295aa993682edb8ccbf2b48f7 Diff: https://reviews.apache.org/r/70233/diff/1/ Testing --- Thanks, Till Toenshoff
Re: Review Request 70131: Added a comment around recovery for `ContainerLogger`s.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/70131/#review213494 --- Ship it! Ship It! - Till Toenshoff On March 5, 2019, 10:42 p.m., Alexander Rukletsov wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/70131/ > --- > > (Updated March 5, 2019, 10:42 p.m.) > > > Review request for mesos, Gilbert Song, Joseph Wu, and Till Toenshoff. > > > Repository: mesos > > > Description > --- > > `ContainerLogger` interface does not currently use a > "prepare-recover-cleanup" pattern and hence neither allows > stateful loggers nor provides synchronization on container > termination. Capture this in a comment to the interface. > > > Diffs > - > > include/mesos/slave/container_logger.hpp > 4e6d15a503e533a82bfac393da447f6f129dd66d > > > Diff: https://reviews.apache.org/r/70131/diff/1/ > > > Testing > --- > > None: not a functional change. > > > Thanks, > > Alexander Rukletsov > >
Re: Review Request 70047: Updated build specific artefact generation.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/70047/ --- (Updated March 6, 2019, 1:12 a.m.) Review request for mesos, Benjamin Bannier and Joseph Wu. Bugs: MESOS-9605 https://issues.apache.org/jira/browse/MESOS-9605 Repository: mesos Description --- For autotools, we extracted additional build info like the git branch and sha during the automake phase handing them into libbuild via commandline defines. CMake builds however used a configuration file for this purpose. This patch updates both build systems to make use of build_git_config.hpp.in for build specific git information. Diffs (updated) - cmake/CompilationConfigure.cmake c330324e2e3dea6e71980ae8c9ed71632ebb018e configure.ac ee29fc784e53ebaf1bb016c33136b200c646ee9b src/Makefile.am 283d5ed89b36d74da36f38c26aec03c6129d6261 src/common/build.cpp f5271d87d33ac429fb94093a347be1d6c25d3432 src/common/build_config.hpp.in 4cce2403c1d7a5feee8fd2fffa7cf4308507cd0c src/common/git_version.hpp.in PRE-CREATION Diff: https://reviews.apache.org/r/70047/diff/6/ Changes: https://reviews.apache.org/r/70047/diff/5-6/ Testing --- Manually tested both cmake and autotools. First configure run: ``` [...] checking for src/common/build_git_config.hpp... no configure: creating src/common/build_git_config.hpp [...] ``` Subsequent configure runs: ``` [...] checking for src/common/build_git_config.hpp... yes [...] ``` Final build from `support/packaging/centos/build-docker-rpmbuild.sh` installed and ran agent; ``` Installed: mesos.x86_64 0:1.8.0-0.1.pre.20190226gitc125541.el7 Complete! [root@9b0d899ff4c6 ~]# mesos-slave --work_dir=/tmp --master=127.0.0.1:5050 I0226 01:00:48.581748 157 main.cpp:350] Build: 2019-02-26 00:36:49 by centos I0226 01:00:48.581817 157 main.cpp:351] Version: 1.8.0 I0226 01:00:48.581823 157 main.cpp:358] Git SHA: c125541b8e4f2c2f6f56fe7e1c2e0b26d5bbfc62 I0226 01:00:48.587003 157 systemd.cpp:240] systemd version `219` detected I0226 01:00:48.587026 157 main.cpp:453] Initializing systemd state ``` Thanks, Till Toenshoff
Re: Review Request 70049: Assured a set USER within RPM build.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/70049/ --- (Updated March 6, 2019, 1:01 a.m.) Review request for mesos, Benjamin Bannier and Jie Yu. Bugs: MESOS-9605 https://issues.apache.org/jira/browse/MESOS-9605 Repository: mesos Description --- Assured a set USER within RPM build. Diffs (updated) - support/packaging/centos/build_rpm.sh 4c5b9617cfcef591b33d11f7f84170c0781d1fa1 Diff: https://reviews.apache.org/r/70049/diff/2/ Changes: https://reviews.apache.org/r/70049/diff/1-2/ Testing --- Installed resulting package and ran agent; ``` [root@9b0d899ff4c6 ~]# mesos-slave --work_dir=/tmp --master=127.0.0.1:5050 I0225 02:30:16.11352897 main.cpp:350] Build: 2019-02-25 01:59:55 by centos I0225 02:30:16.11360697 main.cpp:351] Version: 1.8.0 ``` Thanks, Till Toenshoff
Re: Review Request 65116: Getting Started using the Mesos Binaries.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/65116/#review213447 --- Ship it! Thanks Senthil! Fixing below while landing docs/binary-packages.md Lines 29 (patched) <https://reviews.apache.org/r/65116/#comment299391> s/Subsitute/Substitute/ site/source/downloads.html.erb Lines 47 (patched) <https://reviews.apache.org/r/65116/#comment299390> s/mesos/Mesos/g - Till Toenshoff On March 5, 2019, 10:30 p.m., Senthil Kumaran wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/65116/ > --- > > (Updated March 5, 2019, 10:30 p.m.) > > > Review request for mesos, Greg Mann and Kapil Arya. > > > Repository: mesos > > > Description > --- > > Getting Started using the Mesos Binaries. > > > Diffs > - > > docs/binary-packages.md PRE-CREATION > site/source/downloads.html.erb 259ca43a7 > site/source/getting-started.html.md 2b1b18f61 > > > Diff: https://reviews.apache.org/r/65116/diff/3/ > > > Testing > --- > > ``` > $ curl -L > "https://dl.bintray.com/mesos/el//7/x86_64/mesos-1.4.1-1.el7.x86_64.rpm; -o > mesos-1.4.1-1.el7.x86_64.rpm > % Total% Received % Xferd Average Speed TimeTime Time > Current > Dload Upload Total SpentLeft Speed > 0 00 00 0 0 0 --:--:-- --:--:-- --:--:-- 0 > 100 43.4M 100 43.4M0 0 49.5M 0 --:--:-- --:--:-- --:--:-- 49.5M > ``` > > And > > ``` > $ sudo yum install mesos-1.4.1-1.el7.x86_64.rpm > Loaded plugins: langpacks, product-id, search-disabled-repos > Examining mesos-1.4.1-1.el7.x86_64.rpm: mesos-1.4.1-1.el7.x86_64 > Marking mesos-1.4.1-1.el7.x86_64.rpm to be installed > Resolving Dependencies > --> Running transaction check > ---> Package mesos.x86_64 0:1.4.1-1.el7 will be installed > --> Processing Dependency: cyrus-sasl-md5 for package: > mesos-1.4.1-1.el7.x86_64 > rhui-microsoft-azure-rhel7 > | 2.9 kB 00:00:00 > rhui-rhel-7-server-dotnet-rhui-debug-rpms > | 3.8 kB 00:00:00 > rhui-rhel-7-server-dotnet-rhui-rpms > | 4.0 kB 00:00:00 > rhui-rhel-7-server-dotnet-rhui-source-rpms > | 3.8 kB 00:00:00 > rhui-rhel-7-server-rhui-debug-rpms > | 3.3 kB 00:00:00 > rhui-rhel-7-server-rhui-extras-debug-rpms > | 3.2 kB 00:00:00 > rhui-rhel-7-server-rhui-extras-rpms > | 3.4 kB 00:00:00 > rhui-rhel-7-server-rhui-extras-source-rpms > | 3.8 kB 00:00:00 > rhui-rhel-7-server-rhui-optional-debug-rpms > | 3.3 kB 00:00:00 > rhui-rhel-7-server-rhui-optional-rpms > | 3.5 kB 00:00:00 > rhui-rhel-7-server-rhui-optional-source-rpms > | 3.8 kB 00:00:00 > rhui-rhel-7-server-rhui-rh-common-debug-rpms > | 3.8 kB 00:00:00 > rhui-rhel-7-server-rhui-rh-common-rpms > | 3.8 kB 00:00:00 > rhui-rhel-7-server-rhui-rh-common-source-rpms > | 3.8 kB 00:00:00 > rhui-rhel-7-server-rhui-rpms > | 3.5 kB 00:00:00 > rhui-rhel-7-server-rhui-source-rpms > | 3.8 kB 00:00:00 > rhui-rhel-7-server-rhui-supplementary-debug-rpms > | 3.7 kB 00:00:00 > rhui-rhel-7-server-rhui-supplementary-rpms > | 3.4 kB 00:00:00 > rhui-rhel-7-server-rhui-supplementary-source-rpms >
Re: Review Request 70047: Updated build specific artefact generation.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/70047/ --- (Updated March 5, 2019, 6:46 p.m.) Review request for mesos, Benjamin Bannier and Joseph Wu. Changes --- addressed comments Bugs: MESOS-9605 https://issues.apache.org/jira/browse/MESOS-9605 Repository: mesos Description --- For autotools, we extracted additional build info like the git branch and sha during the automake phase handing them into libbuild via commandline defines. CMake builds however used a configuration file for this purpose. This patch updates both build systems to make use of build_git_config.hpp.in for build specific git information. Diffs (updated) - cmake/CompilationConfigure.cmake c330324e2e3dea6e71980ae8c9ed71632ebb018e configure.ac ee29fc784e53ebaf1bb016c33136b200c646ee9b src/Makefile.am 283d5ed89b36d74da36f38c26aec03c6129d6261 src/common/build.cpp f5271d87d33ac429fb94093a347be1d6c25d3432 src/common/build_config.hpp.in 4cce2403c1d7a5feee8fd2fffa7cf4308507cd0c src/common/git_version.hpp.in PRE-CREATION Diff: https://reviews.apache.org/r/70047/diff/5/ Changes: https://reviews.apache.org/r/70047/diff/4-5/ Testing --- Manually tested both cmake and autotools. First configure run: ``` [...] checking for src/common/build_git_config.hpp... no configure: creating src/common/build_git_config.hpp [...] ``` Subsequent configure runs: ``` [...] checking for src/common/build_git_config.hpp... yes [...] ``` Final build from `support/packaging/centos/build-docker-rpmbuild.sh` installed and ran agent; ``` Installed: mesos.x86_64 0:1.8.0-0.1.pre.20190226gitc125541.el7 Complete! [root@9b0d899ff4c6 ~]# mesos-slave --work_dir=/tmp --master=127.0.0.1:5050 I0226 01:00:48.581748 157 main.cpp:350] Build: 2019-02-26 00:36:49 by centos I0226 01:00:48.581817 157 main.cpp:351] Version: 1.8.0 I0226 01:00:48.581823 157 main.cpp:358] Git SHA: c125541b8e4f2c2f6f56fe7e1c2e0b26d5bbfc62 I0226 01:00:48.587003 157 systemd.cpp:240] systemd version `219` detected I0226 01:00:48.587026 157 main.cpp:453] Initializing systemd state ``` Thanks, Till Toenshoff
Re: Review Request 70089: Removed non-ascii source code characters.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/70089/#review213369 --- Ship it! Thanks - these keep creeping in. - Till Toenshoff 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/70089/ > --- > > (Updated March 3, 2019, 1:01 a.m.) > > > Review request for mesos and Till Toenshoff. > > > Repository: mesos > > > Description > --- > > Removed non-ascii source code characters. > > > Diffs > - > > 3rdparty/patch.exe.manifest cebcdadc49ab8e9a919fd3c50c4e823900b8fe37 > CHANGELOG c1ce30c21dd89e1b9dad4673c1505a3d8f3b0e97 > LICENSE 592b4d1369ea358a9889d768675a98f0466e2c9c > docs/csi.md ae6079d9de566859963453429d8bdaee8ad8e898 > docs/isolators/cgroups-cpu.md ce29a890ee01f01eb01b2fe1f64c8b4ad83b4be7 > docs/quota.md 28021f481ba7c637519b3822c7dbe88dba440e0f > include/mesos/scheduler/scheduler.proto > f6a780a7b75878b9e74402a28a25bb868f7ac36f > include/mesos/v1/scheduler/scheduler.proto > fcfec5e417463103e98dd6917722b4fde41cac7c > > > Diff: https://reviews.apache.org/r/70089/diff/1/ > > > Testing > --- > > * `make check` > * regenerated and inspected site > > > Thanks, > > Benjamin Bannier > >
Re: Review Request 70049: Assured a set USER within RPM build.
> On Feb. 28, 2019, 8:52 p.m., Benjamin Bannier wrote:
> > support/packaging/centos/build_rpm.sh
> > Lines 29 (patched)
> > <https://reviews.apache.org/r/70049/diff/1/?file=2126568#file2126568line29>
> >
> > What about only setting this if unset?
> >
> > export USER=${USER:-centos}
> >
> > That way this script would do the expected thing if invoked by somebody
> > building her own packages, and we wouldn't necessarily assume that we run
> > this in a Docker container.
magificient.
- Till
---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/70049/#review213309
---
On Feb. 25, 2019, 2:31 a.m., Till Toenshoff wrote:
>
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/70049/
> ---
>
> (Updated Feb. 25, 2019, 2:31 a.m.)
>
>
> Review request for mesos, Benjamin Bannier and Jie Yu.
>
>
> Bugs: MESOS-9605
> https://issues.apache.org/jira/browse/MESOS-9605
>
>
> Repository: mesos
>
>
> Description
> ---
>
> Assured a set USER within RPM build.
>
>
> Diffs
> -
>
> support/packaging/centos/build_rpm.sh
> 4c5b9617cfcef591b33d11f7f84170c0781d1fa1
>
>
> Diff: https://reviews.apache.org/r/70049/diff/1/
>
>
> Testing
> ---
>
> Installed resulting package and ran agent;
> ```
> [root@9b0d899ff4c6 ~]# mesos-slave --work_dir=/tmp --master=127.0.0.1:5050
> I0225 02:30:16.11352897 main.cpp:350] Build: 2019-02-25 01:59:55 by centos
> I0225 02:30:16.11360697 main.cpp:351] Version: 1.8.0
> ```
>
>
> Thanks,
>
> Till Toenshoff
>
>
Re: Review Request 70047: Updated build specific artefact generation.
> On Feb. 26, 2019, 8:22 p.m., Joseph Wu wrote: > > cmake/CompilationConfigure.cmake > > Lines 630-637 (original), 663-667 (patched) > > <https://reviews.apache.org/r/70047/diff/4/?file=2126973#file2126973line663> > > > > I don't think the CMake build needs to follow the autotools one here. > > For one, the packaging for CMake does not need to configure more than once > > (assuming we finish the CMake packaging ;). > > > > A comment mentioning the difference should be enough. i.e. > > ``` > > # NOTE: The `build_git_config.hpp.in` file is specific to the autotools > > build. > > # `build_config.hpp.in` includes the definitions for the CMake build. > > ``` > > Till Toenshoff wrote: > There is indeed only one reason to do this, it is because we might want > zero suprises when finally switching over -- waiting for my shepherd to make > a call here :) I totally omitted the source-distribution argument when stating the above -- so while academically speaking we wont need this for cmake based package creation (as done via CPack and not rpmbuild), we will still need it to include a git sha artefact in our source tarballs/packages. - Till --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/70047/#review213230 --- On Feb. 26, 2019, 11:05 a.m., Till Toenshoff wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/70047/ > --- > > (Updated Feb. 26, 2019, 11:05 a.m.) > > > Review request for mesos, Benjamin Bannier and Joseph Wu. > > > Bugs: MESOS-9605 > https://issues.apache.org/jira/browse/MESOS-9605 > > > Repository: mesos > > > Description > --- > > For autotools, we extracted additional build info like the git branch > and sha during the automake phase handing them into libbuild via > commandline defines. > > CMake builds however used a configuration file for this purpose. > > This patch updates both build systems to make use of > build_git_config.hpp.in for build specific git information. > > > Diffs > - > > cmake/CompilationConfigure.cmake c330324e2e3dea6e71980ae8c9ed71632ebb018e > configure.ac ee29fc784e53ebaf1bb016c33136b200c646ee9b > src/Makefile.am 283d5ed89b36d74da36f38c26aec03c6129d6261 > src/common/build.cpp f5271d87d33ac429fb94093a347be1d6c25d3432 > src/common/build_config.hpp.in 4cce2403c1d7a5feee8fd2fffa7cf4308507cd0c > src/common/build_git_config.hpp.in PRE-CREATION > > > Diff: https://reviews.apache.org/r/70047/diff/4/ > > > Testing > --- > > Manually tested both cmake and autotools. > > First configure run: > ``` > [...] > checking for src/common/build_git_config.hpp... no > configure: creating src/common/build_git_config.hpp > [...] > > ``` > > Subsequent configure runs: > ``` > [...] > checking for src/common/build_git_config.hpp... yes > [...] > ``` > > Final build from `support/packaging/centos/build-docker-rpmbuild.sh` > installed and ran agent; > ``` > Installed: > mesos.x86_64 0:1.8.0-0.1.pre.20190226gitc125541.el7 > > Complete! > [root@9b0d899ff4c6 ~]# mesos-slave --work_dir=/tmp --master=127.0.0.1:5050 > I0226 01:00:48.581748 157 main.cpp:350] Build: 2019-02-26 00:36:49 by centos > I0226 01:00:48.581817 157 main.cpp:351] Version: 1.8.0 > I0226 01:00:48.581823 157 main.cpp:358] Git SHA: > c125541b8e4f2c2f6f56fe7e1c2e0b26d5bbfc62 > I0226 01:00:48.587003 157 systemd.cpp:240] systemd version `219` detected > I0226 01:00:48.587026 157 main.cpp:453] Initializing systemd state > ``` > > > Thanks, > > Till Toenshoff > >
Re: Review Request 70047: Updated build specific artefact generation.
> On March 1, 2019, 12:02 a.m., Benjamin Bannier wrote:
> > cmake/CompilationConfigure.cmake
> > Lines 630-637 (original), 663-667 (patched)
> > <https://reviews.apache.org/r/70047/diff/4/?file=2126973#file2126973line663>
> >
> > The idea (at least on the autotools side which currently supports
> > packaging) is to generate an artifact specifiying git info which is
> > included with the distribution tarball.
> >
> > AFAICT, unless we completely rip out autotools while implementing cmake
> > distribution tarballs, we'll need to implement something like this to
> > support showing git info when building from distribution tarballs, even in
> > the cmake build (which would probably emit this also during packaging time).
> >
> > Suggest to keep as is.
Yup!
> On March 1, 2019, 12:02 a.m., Benjamin Bannier wrote:
> > configure.ac
> > Lines 2829 (patched)
> > <https://reviews.apache.org/r/70047/diff/4/?file=2126974#file2126974line2830>
> >
> > Lets use `presence` instead of `for`,
> >
> > checking src/common/build_git_config.hpp presence ...
ok
> On March 1, 2019, 12:02 a.m., Benjamin Bannier wrote:
> > configure.ac
> > Lines 2831 (patched)
> > <https://reviews.apache.org/r/70047/diff/4/?file=2126974#file2126974line2832>
> >
> > Even though there is some nesting I'd prefer more general `AS_IF` and
> > friends.
> >
> > Here and below.
ok
> On March 1, 2019, 12:02 a.m., Benjamin Bannier wrote:
> > configure.ac
> > Lines 2836 (patched)
> > <https://reviews.apache.org/r/70047/diff/4/?file=2126974#file2126974line2837>
> >
> > `s/creating/generating/`
ok
> On March 1, 2019, 12:02 a.m., Benjamin Bannier wrote:
> > configure.ac
> > Lines 2843 (patched)
> > <https://reviews.apache.org/r/70047/diff/4/?file=2126974#file2126974line2844>
> >
> > Do you understand why this command could print something to stderr? It
> > seems it should be possible to treat the absent case as error and define
> > the variable with a value immediately.
> >
> > Also, what do you think about instead
> > ```
> > git --git-dir=${srcdir}"/.git/ rev-parse HEAD ...
> > ```
> >
> > We could put the git dir into a variable.
> >
> > Here and below.
> Do you understand why this command could print something to stderr?
Yes - well not the head SHA, that should indeed always work.
Let me take the tag for an example;
```
lobomacpro4:~/Development/mesos-private (ci/till/build-config-revamp ?) $ git
describe --exact --tags
fatal: no tag exactly matches '9ed7e160f003b5e22bf9c41e0104e0efca1df682'
lobomacpro4:~/Development/mesos-private (ci/till/build-config-revamp ?) $ git
describe --exact --tags 2>/dev/null
lobomacpro4:~/Development/mesos-private (ci/till/build-config-revamp ?) $ echo
$?
128
```
Then imagine a detached checkout, that would similarly fail for the branch
detection.
```
lobomacpro4:~/Development/mesos-private (no branch! ?) $ git symbolic-ref HEAD
fatal: ref HEAD is not a symbolic ref
lobomacpro4:~/Development/mesos-private (no branch! ?) $ git symbolic-ref HEAD
2>/dev/null
lobomacpro4:~/Development/mesos-private (no branch! ?) $ echo $?
128
```
So I would argue that the current approach for error handling is rather well
adapted.
> > `git --git-dir=${srcdir}"/.git/ rev-parse HEAD ...`
Much better, thanks!
> On March 1, 2019, 12:02 a.m., Benjamin Bannier wrote:
> > src/common/build_git_config.hpp.in
> > Lines 1 (patched)
> > <https://reviews.apache.org/r/70047/diff/4/?file=2126978#file2126978line1>
> >
> > Since this isn't about the build but git version, what do you think
> > abot calling this file e.g., `git_version.hpp.in` or similar?
Yes - good point.
> On March 1, 2019, 12:02 a.m., Benjamin Bannier wrote:
> > src/common/build_git_config.hpp.in
> > Lines 23-25 (patched)
> > <https://reviews.apache.org/r/70047/diff/4/?file=2126978#file2126978line23>
> >
> > Not really a fan of how we directly emit a CPP macro from autoconf, but
> > without config headers I cannot really think of a objectively better
> > approach :(
I wish you could! :(
- Till
---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/70047/#review213313
---
On Feb. 26, 2019, 11:05 a.m., Till Toenshoff wrote:
>
> ---
> This is an automatically generated e-m
Re: Review Request 70047: Updated build specific artefact generation.
> On Feb. 26, 2019, 8:22 p.m., Joseph Wu wrote: > > cmake/CompilationConfigure.cmake > > Lines 630-637 (original), 663-667 (patched) > > <https://reviews.apache.org/r/70047/diff/4/?file=2126973#file2126973line663> > > > > I don't think the CMake build needs to follow the autotools one here. > > For one, the packaging for CMake does not need to configure more than once > > (assuming we finish the CMake packaging ;). > > > > A comment mentioning the difference should be enough. i.e. > > ``` > > # NOTE: The `build_git_config.hpp.in` file is specific to the autotools > > build. > > # `build_config.hpp.in` includes the definitions for the CMake build. > > ``` There is indeed only one reason to do this, it is because we might want zero suprises when finally switching over -- waiting for my shepherd to make a call here :) - Till --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/70047/#review213230 ------- On Feb. 26, 2019, 11:05 a.m., Till Toenshoff wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/70047/ > --- > > (Updated Feb. 26, 2019, 11:05 a.m.) > > > Review request for mesos, Benjamin Bannier and Joseph Wu. > > > Bugs: MESOS-9605 > https://issues.apache.org/jira/browse/MESOS-9605 > > > Repository: mesos > > > Description > --- > > For autotools, we extracted additional build info like the git branch > and sha during the automake phase handing them into libbuild via > commandline defines. > > CMake builds however used a configuration file for this purpose. > > This patch updates both build systems to make use of > build_git_config.hpp.in for build specific git information. > > > Diffs > - > > cmake/CompilationConfigure.cmake c330324e2e3dea6e71980ae8c9ed71632ebb018e > configure.ac ee29fc784e53ebaf1bb016c33136b200c646ee9b > src/Makefile.am 283d5ed89b36d74da36f38c26aec03c6129d6261 > src/common/build.cpp f5271d87d33ac429fb94093a347be1d6c25d3432 > src/common/build_config.hpp.in 4cce2403c1d7a5feee8fd2fffa7cf4308507cd0c > src/common/build_git_config.hpp.in PRE-CREATION > > > Diff: https://reviews.apache.org/r/70047/diff/4/ > > > Testing > --- > > Manually tested both cmake and autotools. > > First configure run: > ``` > [...] > checking for src/common/build_git_config.hpp... no > configure: creating src/common/build_git_config.hpp > [...] > > ``` > > Subsequent configure runs: > ``` > [...] > checking for src/common/build_git_config.hpp... yes > [...] > ``` > > Final build from `support/packaging/centos/build-docker-rpmbuild.sh` > installed and ran agent; > ``` > Installed: > mesos.x86_64 0:1.8.0-0.1.pre.20190226gitc125541.el7 > > Complete! > [root@9b0d899ff4c6 ~]# mesos-slave --work_dir=/tmp --master=127.0.0.1:5050 > I0226 01:00:48.581748 157 main.cpp:350] Build: 2019-02-26 00:36:49 by centos > I0226 01:00:48.581817 157 main.cpp:351] Version: 1.8.0 > I0226 01:00:48.581823 157 main.cpp:358] Git SHA: > c125541b8e4f2c2f6f56fe7e1c2e0b26d5bbfc62 > I0226 01:00:48.587003 157 systemd.cpp:240] systemd version `219` detected > I0226 01:00:48.587026 157 main.cpp:453] Initializing systemd state > ``` > > > Thanks, > > Till Toenshoff > >
Re: Review Request 70047: Updated build specific artefact generation.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/70047/ --- (Updated Feb. 26, 2019, 11:05 a.m.) Review request for mesos, Benjamin Bannier and Joseph Wu. Bugs: MESOS-9605 https://issues.apache.org/jira/browse/MESOS-9605 Repository: mesos Description --- For autotools, we extracted additional build info like the git branch and sha during the automake phase handing them into libbuild via commandline defines. CMake builds however used a configuration file for this purpose. This patch updates both build systems to make use of build_git_config.hpp.in for build specific git information. Diffs (updated) - cmake/CompilationConfigure.cmake c330324e2e3dea6e71980ae8c9ed71632ebb018e configure.ac ee29fc784e53ebaf1bb016c33136b200c646ee9b src/Makefile.am 283d5ed89b36d74da36f38c26aec03c6129d6261 src/common/build.cpp f5271d87d33ac429fb94093a347be1d6c25d3432 src/common/build_config.hpp.in 4cce2403c1d7a5feee8fd2fffa7cf4308507cd0c src/common/build_git_config.hpp.in PRE-CREATION Diff: https://reviews.apache.org/r/70047/diff/4/ Changes: https://reviews.apache.org/r/70047/diff/3-4/ Testing --- Manually tested both cmake and autotools. First configure run: ``` [...] checking for src/common/build_git_config.hpp... no configure: creating src/common/build_git_config.hpp [...] ``` Subsequent configure runs: ``` [...] checking for src/common/build_git_config.hpp... yes [...] ``` Final build from `support/packaging/centos/build-docker-rpmbuild.sh` installed and ran agent; ``` Installed: mesos.x86_64 0:1.8.0-0.1.pre.20190226gitc125541.el7 Complete! [root@9b0d899ff4c6 ~]# mesos-slave --work_dir=/tmp --master=127.0.0.1:5050 I0226 01:00:48.581748 157 main.cpp:350] Build: 2019-02-26 00:36:49 by centos I0226 01:00:48.581817 157 main.cpp:351] Version: 1.8.0 I0226 01:00:48.581823 157 main.cpp:358] Git SHA: c125541b8e4f2c2f6f56fe7e1c2e0b26d5bbfc62 I0226 01:00:48.587003 157 systemd.cpp:240] systemd version `219` detected I0226 01:00:48.587026 157 main.cpp:453] Initializing systemd state ``` Thanks, Till Toenshoff
Re: Review Request 70047: Updated build specific artefact generation.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/70047/ --- (Updated Feb. 26, 2019, 2:20 a.m.) Review request for mesos, Benjamin Bannier and Joseph Wu. Bugs: MESOS-9605 https://issues.apache.org/jira/browse/MESOS-9605 Repository: mesos Description (updated) --- For autotools, we extracted additional build info like the git branch and sha during the automake phase handing them into libbuild via commandline defines. CMake builds however used a configuration file for this purpose. This patch updates both build systems to make use of build_git_config.hpp.in for build specific git information. Diffs (updated) - cmake/CompilationConfigure.cmake c330324e2e3dea6e71980ae8c9ed71632ebb018e configure.ac ee29fc784e53ebaf1bb016c33136b200c646ee9b src/Makefile.am 283d5ed89b36d74da36f38c26aec03c6129d6261 src/common/build.cpp f5271d87d33ac429fb94093a347be1d6c25d3432 src/common/build_config.hpp.in 4cce2403c1d7a5feee8fd2fffa7cf4308507cd0c src/common/build_git_config.hpp.in PRE-CREATION Diff: https://reviews.apache.org/r/70047/diff/3/ Changes: https://reviews.apache.org/r/70047/diff/2-3/ Testing --- Manually tested both cmake and autotools. First configure run: ``` [...] checking for src/common/build_git_config.hpp... no configure: creating src/common/build_git_config.hpp [...] ``` Subsequent configure runs: ``` [...] checking for src/common/build_git_config.hpp... yes [...] ``` Final build from `support/packaging/centos/build-docker-rpmbuild.sh` installed and ran agent; ``` Installed: mesos.x86_64 0:1.8.0-0.1.pre.20190226gitc125541.el7 Complete! [root@9b0d899ff4c6 ~]# mesos-slave --work_dir=/tmp --master=127.0.0.1:5050 I0226 01:00:48.581748 157 main.cpp:350] Build: 2019-02-26 00:36:49 by centos I0226 01:00:48.581817 157 main.cpp:351] Version: 1.8.0 I0226 01:00:48.581823 157 main.cpp:358] Git SHA: c125541b8e4f2c2f6f56fe7e1c2e0b26d5bbfc62 I0226 01:00:48.587003 157 systemd.cpp:240] systemd version `219` detected I0226 01:00:48.587026 157 main.cpp:453] Initializing systemd state ``` Thanks, Till Toenshoff
Re: Review Request 70047: Updated build specific artefact generation.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/70047/ --- (Updated Feb. 26, 2019, 2:18 a.m.) Review request for mesos, Benjamin Bannier and Joseph Wu. Bugs: MESOS-9605 https://issues.apache.org/jira/browse/MESOS-9605 Repository: mesos Description (updated) --- For autotools, we extracted additional build info like the git branch and sha during the automake phase handing them into libbuild via commandline supplied defines. CMake builds however used a configuration file for this purpose. This patch updates both build systems to make use of build_git_config.hpp.in for build specific git information. The data is persisted only once for the initial configuration / cmake run. Subsequent invocations will not update build_git_config.hpp. We do this to make sure that snapshots can get built without having git specific information available at build time. Diffs (updated) - cmake/CompilationConfigure.cmake c330324e2e3dea6e71980ae8c9ed71632ebb018e configure.ac ee29fc784e53ebaf1bb016c33136b200c646ee9b src/Makefile.am 283d5ed89b36d74da36f38c26aec03c6129d6261 src/common/build.cpp f5271d87d33ac429fb94093a347be1d6c25d3432 src/common/build_config.hpp.in 4cce2403c1d7a5feee8fd2fffa7cf4308507cd0c src/common/build_git_config.hpp.in PRE-CREATION Diff: https://reviews.apache.org/r/70047/diff/2/ Changes: https://reviews.apache.org/r/70047/diff/1-2/ Testing (updated) --- Manually tested both cmake and autotools. First configure run: ``` [...] checking for src/common/build_git_config.hpp... no configure: creating src/common/build_git_config.hpp [...] ``` Subsequent configure runs: ``` [...] checking for src/common/build_git_config.hpp... yes [...] ``` Final build from `support/packaging/centos/build-docker-rpmbuild.sh` installed and ran agent; ``` Installed: mesos.x86_64 0:1.8.0-0.1.pre.20190226gitc125541.el7 Complete! [root@9b0d899ff4c6 ~]# mesos-slave --work_dir=/tmp --master=127.0.0.1:5050 I0226 01:00:48.581748 157 main.cpp:350] Build: 2019-02-26 00:36:49 by centos I0226 01:00:48.581817 157 main.cpp:351] Version: 1.8.0 I0226 01:00:48.581823 157 main.cpp:358] Git SHA: c125541b8e4f2c2f6f56fe7e1c2e0b26d5bbfc62 I0226 01:00:48.587003 157 systemd.cpp:240] systemd version `219` detected I0226 01:00:48.587026 157 main.cpp:453] Initializing systemd state ``` Thanks, Till Toenshoff
Re: Review Request 70055: Output Review Request URL in Reviewbot output.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/70055/#review213194 --- Ship it! Ship It! - Till Toenshoff On Feb. 25, 2019, 11:44 p.m., Vinod Kone wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/70055/ > --- > > (Updated Feb. 25, 2019, 11:44 p.m.) > > > Review request for mesos, Benjamin Bannier and Till Toenshoff. > > > Repository: mesos > > > Description > --- > > The URL makes it easy to get to the Review from log messages. > > > Diffs > - > > support/verify-reviews.py ab4888ae04ded555abb75c7cea468afa0ccb2ea8 > > > Diff: https://reviews.apache.org/r/70055/diff/1/ > > > Testing > --- > > ./support/verify-reviews.py -u foo -p bar --skip-verify > > > This just made sure I didn't make any syntax errors. > > > Thanks, > > Vinod Kone > >
Re: Review Request 70047: Updated build specific artefact generation.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/70047/ --- (Updated Feb. 25, 2019, 2:35 a.m.) Review request for mesos, Benjamin Bannier and Joseph Wu. Bugs: MESOS-9605 https://issues.apache.org/jira/browse/MESOS-9605 Repository: mesos Description --- For autotools, we extracted additional build info like the git branch and sha during the automake phase handing them into libbuild via commandline defines. CMake builds however used a configuration file for this purpose. This patch updates both build systems to make use of build_config.hpp.in for build specific configuration artefacts. Diffs - cmake/CompilationConfigure.cmake c330324e2e3dea6e71980ae8c9ed71632ebb018e configure.ac ee29fc784e53ebaf1bb016c33136b200c646ee9b src/Makefile.am 283d5ed89b36d74da36f38c26aec03c6129d6261 src/common/build.cpp f5271d87d33ac429fb94093a347be1d6c25d3432 src/common/build_config.hpp.in 4cce2403c1d7a5feee8fd2fffa7cf4308507cd0c Diff: https://reviews.apache.org/r/70047/diff/1/ Testing (updated) --- manually tested both cmake and autotools. WIP: A test using the centos RPM toolchain does not yield the intended result; ``` [root@9b0d899ff4c6 ~]# mesos-slave --work_dir=/tmp --master=127.0.0.1:5050 I0225 02:30:16.11352897 main.cpp:350] Build: 2019-02-25 01:59:55 by centos I0225 02:30:16.11360697 main.cpp:351] Version: 1.8.0 I0225 02:30:16.12117397 systemd.cpp:240] systemd version `219` detected I0225 02:30:16.12119997 main.cpp:453] Initializing systemd state E0225 02:30:16.12125197 main.cpp:462] EXIT with status 1: Failed to initialize systemd: Failed to locate systemd runtime directory: /run/systemd/system ``` It still lacks the SHA! Thanks, Till Toenshoff
Re: Review Request 70049: Assured a set USER within RPM build.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/70049/ --- (Updated Feb. 25, 2019, 2:31 a.m.) Review request for mesos, Benjamin Bannier and Jie Yu. Bugs: MESOS-9605 https://issues.apache.org/jira/browse/MESOS-9605 Repository: mesos Description --- Assured a set USER within RPM build. Diffs - support/packaging/centos/build_rpm.sh 4c5b9617cfcef591b33d11f7f84170c0781d1fa1 Diff: https://reviews.apache.org/r/70049/diff/1/ Testing (updated) --- Installed resulting package and ran agent; ``` [root@9b0d899ff4c6 ~]# mesos-slave --work_dir=/tmp --master=127.0.0.1:5050 I0225 02:30:16.11352897 main.cpp:350] Build: 2019-02-25 01:59:55 by centos I0225 02:30:16.11360697 main.cpp:351] Version: 1.8.0 ``` Thanks, Till Toenshoff
Review Request 70049: Assured a set USER within RPM build.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/70049/ --- Review request for mesos, Benjamin Bannier and Jie Yu. Bugs: MESOS-9605 https://issues.apache.org/jira/browse/MESOS-9605 Repository: mesos Description --- Assured a set USER within RPM build. Diffs - support/packaging/centos/build_rpm.sh 4c5b9617cfcef591b33d11f7f84170c0781d1fa1 Diff: https://reviews.apache.org/r/70049/diff/1/ Testing --- Thanks, Till Toenshoff
Review Request 70047: Updated build specific artefact generation.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/70047/ --- Review request for mesos, Benjamin Bannier and Joseph Wu. Bugs: MESOS-9605 https://issues.apache.org/jira/browse/MESOS-9605 Repository: mesos Description --- For autotools, we extracted additional build info like the git branch and sha during the automake phase handing them into libbuild via commandline defines. CMake builds however used a configuration file for this purpose. This patch updates both build systems to make use of build_config.hpp.in for build specific configuration artefacts. Diffs - cmake/CompilationConfigure.cmake c330324e2e3dea6e71980ae8c9ed71632ebb018e configure.ac ee29fc784e53ebaf1bb016c33136b200c646ee9b src/Makefile.am 283d5ed89b36d74da36f38c26aec03c6129d6261 src/common/build.cpp f5271d87d33ac429fb94093a347be1d6c25d3432 src/common/build_config.hpp.in 4cce2403c1d7a5feee8fd2fffa7cf4308507cd0c Diff: https://reviews.apache.org/r/70047/diff/1/ Testing --- manually tested both cmake and autotools. Thanks, Till Toenshoff
Re: Review Request 70045: Skipped tests when verifying `master` as part of review verification.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/70045/#review213126 --- Ship it! Ship It! - Till Toenshoff On Feb. 24, 2019, 10:03 p.m., Benjamin Bannier wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/70045/ > --- > > (Updated Feb. 24, 2019, 10:03 p.m.) > > > Review request for mesos, Till Toenshoff and Vinod Kone. > > > Repository: mesos > > > Description > --- > > Due to flakes test results reported by reviewbot need manual > verification so we can skip executing tests when validating the > `master` branch. This saves some execution time during validation. > > Incidentally with this patch a previous comment matches now matches > what is happening. > > > Diffs > - > > support/jenkins/reviewbot.sh 3cdd7f1c909c930a561ce2890212118579a47b73 > > > Diff: https://reviews.apache.org/r/70045/diff/2/ > > > Testing > --- > > > Thanks, > > Benjamin Bannier > >
Re: Review Request 70000: Fixed verify-reviews.py to not abort review verification prematurely.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/7/#review212914 --- Fix it, then Ship it! support/verify-reviews.py Lines 182-186 (patched) <https://reviews.apache.org/r/7/#comment298791> We could now reference https://issues.apache.org/jira/browse/MESOS-9583 :) - Till Toenshoff On Feb. 19, 2019, 10:43 a.m., Benjamin Bannier wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/7/ > --- > > (Updated Feb. 19, 2019, 10:43 a.m.) > > > Review request for mesos, Till Toenshoff and Vinod Kone. > > > Bugs: MESOS-9582 > https://issues.apache.org/jira/browse/MESOS-9582 > > > Repository: mesos > > > Description > --- > > Previously `support/verify-reviews.py` would abort prematurely > whenever a review a patch could not be applied. This was due to > the `shell` function used to call `support/apply-reviews.py` invoking > `exit(1)` on the first error which stopped iteration over all > outstanding reviews. > > In this patch that explicit call to `exit` is removed, and instead we > let a possible `subprocess.CalledProcessError` propagate up for it to > be handled at a higher level. Currently this will post a note on the > review in question to (1) notify the submitter, and (2) prevent the > review from being checked again. > > With the changes here the script will not stop verify reviews in such > cases. > > > Diffs > - > > support/verify-reviews.py a88a91f8ecb3794846d2d5eddee27ada770d55b9 > > > Diff: https://reviews.apache.org/r/7/diff/2/ > > > Testing > --- > > Ran the script (with a dummy user and password) on today's reviewboard state. > The script attempted to post a review on the last patch in the chain instead > of aborting (see the `TODO` in the code on why we weren't able to diagnose > the issue in the faulty patch with the current implementation). > > > Thanks, > > Benjamin Bannier > >
Re: Review Request 69999: Fixed some lint issues in verify-reviews.py.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/6/#review212910 --- Ship it! Ship It! - Till Toenshoff On Feb. 17, 2019, 5:48 p.m., Benjamin Bannier wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/6/ > --- > > (Updated Feb. 17, 2019, 5:48 p.m.) > > > Review request for mesos, Till Toenshoff and Vinod Kone. > > > Bugs: MESOS-9582 > https://issues.apache.org/jira/browse/MESOS-9582 > > > Repository: mesos > > > Description > --- > > Fixed some lint issues in verify-reviews.py. > > > Diffs > - > > support/verify-reviews.py a88a91f8ecb3794846d2d5eddee27ada770d55b9 > > > Diff: https://reviews.apache.org/r/6/diff/1/ > > > Testing > --- > > https://reviews.apache.org/r/7/ > > > Thanks, > > Benjamin Bannier > >
Re: Review Request 69998: Reformated verify-reviews.py with yapf.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/69998/#review212909 --- Ship it! Ship It! - Till Toenshoff On Feb. 17, 2019, 5:48 p.m., Benjamin Bannier wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/69998/ > --- > > (Updated Feb. 17, 2019, 5:48 p.m.) > > > Review request for mesos, Till Toenshoff and Vinod Kone. > > > Bugs: MESOS-9582 > https://issues.apache.org/jira/browse/MESOS-9582 > > > Repository: mesos > > > Description > --- > > Reformated verify-reviews.py with yapf. > > > Diffs > - > > support/verify-reviews.py a88a91f8ecb3794846d2d5eddee27ada770d55b9 > > > Diff: https://reviews.apache.org/r/69998/diff/1/ > > > Testing > --- > > Tested as part of https://reviews.apache.org/r/7/ > > > Thanks, > > Benjamin Bannier > >
Re: Review Request 69986: Added a log line for simplified debugging.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/69986/#review212862 --- Ship it! Ship It! - Till Toenshoff On Feb. 14, 2019, 3:24 p.m., Alexander Rukletsov wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/69986/ > --- > > (Updated Feb. 14, 2019, 3:24 p.m.) > > > Review request for mesos, Andrei Budnik and Gilbert Song. > > > Repository: mesos > > > Description > --- > > Added a log line for simplified debugging. > > > Diffs > - > > src/slave/containerizer/mesos/isolators/filesystem/linux.cpp > 2a9ea448d7f963f86e8b2909d83e82b498e4104c > > > Diff: https://reviews.apache.org/r/69986/diff/1/ > > > Testing > --- > > None: not a functional change. > > > Thanks, > > Alexander Rukletsov > >
Review Request 69931: Empty test commit.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/69931/ --- Review request for mesos. Repository: mesos Description --- Empty test commit. Diffs - configure.ac 21115e56e1f5faaff667389f59a357e314f1da8f Diff: https://reviews.apache.org/r/69931/diff/1/ Testing --- Thanks, Till Toenshoff
Re: Review Request 69697: Reverted cleanup step of `verify-reviews.py`.
> On Jan. 16, 2019, 5:04 p.m., Vinod Kone wrote:
> > support/verify-reviews.py
> > Line 174 (original), 171 (patched)
> > <https://reviews.apache.org/r/69697/diff/1/?file=2118600#file2118600line174>
> >
> > Do you guys understand why this was failing in the Azure CI in the
> > first place? Originally, the HEAD variable was calculated outside
> > `cleanup`. But looks like at some point (during refactor for python3) it
> > was pushed inside `cleanup`. Do you know why?
> >
> > Also `git checkout HEAD -- HEAD` is wrong. I don't think that works.
> > Was this tested?
We are assuming this stems from the fact that the working directory of the
invocations differ - https://reviews.apache.org/r/69817 determines the working
directory, we expect/hope/pray that fixes things.
- Till
---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/69697/#review212077
---
On Jan. 9, 2019, 9:30 a.m., Armand Grillet wrote:
>
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/69697/
> ---
>
> (Updated Jan. 9, 2019, 9:30 a.m.)
>
>
> Review request for mesos and Till Toenshoff.
>
>
> Bugs: MESOS-9514
> https://issues.apache.org/jira/browse/MESOS-9514
>
>
> Repository: mesos
>
>
> Description
> ---
>
> Reverts the improvements made on the support script `verify-reviews.py`
> in 3badf7179992e61f30f5a79da9d481dd451c7c2f. Sadly, these changes have
> broken our Azure based Mesos CI as seen in MESOS-9514.
>
> After comparing the code before and after the commit, we can see that
> the line failing in CI, `HEAD = shell("git rev-parse HEAD")`, is
> misplaced. It was previously located in the exit process in the
> `cleanup()` function and it is now back there, surrounded by a
> try/catch for error handling.
>
>
> Diffs
> -
>
> support/verify-reviews.py 72f98b234d9a2a84decb0569998d74e4c730122d
>
>
> Diff: https://reviews.apache.org/r/69697/diff/1/
>
>
> Testing
> ---
>
>
> Thanks,
>
> Armand Grillet
>
>
Re: Review Request 69817: Refactored 'support/verify-reviews.py' to be closer to commit 7412179.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/69817/#review212668 --- Ship it! Ship It! - Till Toenshoff On Jan. 23, 2019, 11:47 a.m., Armand Grillet wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/69817/ > --- > > (Updated Jan. 23, 2019, 11:47 a.m.) > > > Review request for mesos, Till Toenshoff and Vinod Kone. > > > Repository: mesos > > > Description > --- > > The goal of this commit is to make 'verify-reviews.py' as close as > possible to its last known working state (commit > 74121798f24fca372180b8c4bc00b4df07d46240). > > The diff between 'verify-reviews.py' at this commit and 7412179 is > available here: https://www.diffchecker.com/cmfaM83O All the new > features added since 7412179 have been added again and the code > has been made Python 3 compatible again. > > The goal is to have a diff as minimal as possible with improvements > regarding logs so that we do not face CI issues anymore. Changes > have been made regarding how we run commands from the script so > that they are run from the repository instead of where the > script is being called. > > > Diffs > - > > support/verify-reviews.py 71326d34bb649e27a3a2901867d31a2a1fffd4e9 > > > Diff: https://reviews.apache.org/r/69817/diff/1/ > > > Testing > --- > > Tested locally. > > > Thanks, > > Armand Grillet > >
Re: Review Request 69839: Fixed scheduler library on multiple SUBSCRIBE requests per connection.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/69839/ --- (Updated Jan. 30, 2019, 4:34 a.m.) Review request for mesos, Alexander Rukletsov and Vinod Kone. Bugs: MESOS-9210 https://issues.apache.org/jira/browse/MESOS-9210 Repository: mesos Description (updated) --- The HTTP scheduler API dictates that on a single connection, the scheduler may only send a single SUBSCRIBE request. Due to recent authentication related changes, this contract got broken. This patch restores the contract and adds a test validating that the library is enforcing it. Diffs (updated) - src/scheduler/scheduler.cpp cb24ba9c8e1d04b8c62bdf07b12758a61b3bf036 src/tests/scheduler_tests.cpp b571bb1d20744b943580677a26db4c12c7c311d1 Diff: https://reviews.apache.org/r/69839/diff/2/ Changes: https://reviews.apache.org/r/69839/diff/1-2/ Testing --- manual testing; Running the included test without patching `scheduler.cpp` -> fails as the master does in fact receive two SUBSCRIBE requests. `make check` Thanks, Till Toenshoff
Re: Review Request 69839: Fixed scheduler library on multiple SUBSCRIBE requests per connection.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/69839/ --- (Updated Jan. 25, 2019, 3:41 p.m.) Review request for mesos, Alexander Rukletsov and Vinod Kone. Summary (updated) - Fixed scheduler library on multiple SUBSCRIBE requests per connection. Bugs: MESOS-9210 https://issues.apache.org/jira/browse/MESOS-9210 Repository: mesos Description (updated) --- The HTTP scheduler API dictates that on a single connection, the scheduler may only send a single SUBSCRIBE request. Due to recent authentication related changes, this contract got broken. This patch restores the contract and adds a test validating that the library is enforcing it. Diffs - src/scheduler/scheduler.cpp cb24ba9c8e1d04b8c62bdf07b12758a61b3bf036 src/tests/scheduler_tests.cpp b571bb1d20744b943580677a26db4c12c7c311d1 Diff: https://reviews.apache.org/r/69839/diff/1/ Testing --- manual testing; Running the included test without patching `scheduler.cpp` -> fails as the master does in fact receive two SUBSCRIBE requests. `make check` Thanks, Till Toenshoff
