Re: Review Request 70749: WIP: Use openssl hostname validation.
> On June 4, 2019, 11:39 a.m., Alexander Rukletsov wrote: > > 3rdparty/libprocess/src/posix/libevent/libevent_ssl_socket.cpp > > Lines 530-533 (patched) > > <https://reviews.apache.org/r/70749/diff/1/?file=2147044#file2147044line530> > > > > Does it mean that hostname validation with the help of openssl is not > > supported for clients? > > Benno Evers wrote: > I'm not sure I understand your question? This comment is basically > talking about the issue addressed in the follow-up review in the chain. > > It is supported for clients, but does not add as much security as > intended until this TODO is fixed. What I meant is that currently, i.e., `legacy`, hostname validation is also performed in the server mode, i.e., for clients. With `hostname_validation_algorithm=openssl` and `require_cert=true`, will hostname validation be performed for client certificates as it would be with `hostname_validation_algorithm=legacy`? - Alexander --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/70749/#review215673 --- On May 31, 2019, 3:47 p.m., Benno Evers wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/70749/ > ------- > > (Updated May 31, 2019, 3:47 p.m.) > > > Review request for mesos, Alexander Rukletsov and Joseph Wu. > > > Bugs: MESOS-9809 > https://issues.apache.org/jira/browse/MESOS-9809 > > > Repository: mesos > > > Description > --- > > WIP: Use openssl hostname validation. > > > Diffs > - > > 3rdparty/libprocess/include/process/ssl/flags.hpp > f3483f97f93bb29117b2c78f0f2ed9735d9c4b3a > 3rdparty/libprocess/src/openssl.hpp > 17bec246e516261f8d772f1647c17f092fae82d1 > 3rdparty/libprocess/src/openssl.cpp > e7dbd67913fa8e7fbbf60dee428e7e38895f86ce > 3rdparty/libprocess/src/posix/libevent/libevent_ssl_socket.cpp > 29a1bf71c1df9d80370455a6269ecea0ec4193b0 > > > Diff: https://reviews.apache.org/r/70749/diff/1/ > > > Testing > --- > > > Thanks, > > Benno Evers > >
Re: Review Request 70749: WIP: Use openssl hostname validation.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/70749/#review215673 --- 3rdparty/libprocess/src/openssl.cpp Lines 142 (patched) <https://reviews.apache.org/r/70749/#comment302457> Looks incomplete 3rdparty/libprocess/src/openssl.cpp Lines 553-557 (patched) <https://reviews.apache.org/r/70749/#comment302458> Please explain in the comment and also in the flag description why this choice. If you keep auto option, please log the changes to the flag value. Also it might make sense to keep a separate variable for the actual value and keep user input unchanged (which is not quite what we have done here). 3rdparty/libprocess/src/openssl.cpp Lines 565-567 (patched) <https://reviews.apache.org/r/70749/#comment302460> Hm, this is unfortunate. I wonder if we can use https://www.openssl.org/docs/manmaster/man3/SSL_get_verify_result.html in combination with `SSL_VERIFY_NONE` to mimic the OR behaviour we currently have? Another question is whether we need to support OR at all. 3rdparty/libprocess/src/posix/libevent/libevent_ssl_socket.cpp Lines 530-533 (patched) <https://reviews.apache.org/r/70749/#comment302461> Does it mean that hostname validation with the help of openssl is not supported for clients? - Alexander Rukletsov On May 31, 2019, 3:47 p.m., Benno Evers wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/70749/ > --- > > (Updated May 31, 2019, 3:47 p.m.) > > > Review request for mesos, Alexander Rukletsov and Joseph Wu. > > > Bugs: MESOS-9809 > https://issues.apache.org/jira/browse/MESOS-9809 > > > Repository: mesos > > > Description > --- > > WIP: Use openssl hostname validation. > > > Diffs > - > > 3rdparty/libprocess/include/process/ssl/flags.hpp > f3483f97f93bb29117b2c78f0f2ed9735d9c4b3a > 3rdparty/libprocess/src/openssl.hpp > 17bec246e516261f8d772f1647c17f092fae82d1 > 3rdparty/libprocess/src/openssl.cpp > e7dbd67913fa8e7fbbf60dee428e7e38895f86ce > 3rdparty/libprocess/src/posix/libevent/libevent_ssl_socket.cpp > 29a1bf71c1df9d80370455a6269ecea0ec4193b0 > > > Diff: https://reviews.apache.org/r/70749/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/#review215616 --- The code looks good to me. Thanks! As part of this change, either here or in separate reviews, let us: - call out the modification in the changelog; - mention taken approach in MESOS-9791 and explain why (based on the discussion we had with Jan-Philip Gehrcke); - update SSL flags description, in particular calling out that `REQUIRE_CERT` is applicable for client mode only; - update SSL documentation, in particular introduce suggested configuration and clarify the behaviour in client and server modes based on set arguments (based on the table you showed me offline); - send a message to the user list regarding the use of anonymous ciphers 3rdparty/libprocess/src/openssl.cpp Lines 742-743 (patched) <https://reviews.apache.org/r/70748/#comment302368> blank comment line please 3rdparty/libprocess/src/posix/libevent/libevent_ssl_socket.cpp Lines 74-75 (patched) <https://reviews.apache.org/r/70748/#comment302369> ouch? 3rdparty/libprocess/src/tests/ssl_tests.cpp Lines 287-290 (patched) <https://reviews.apache.org/r/70748/#comment302371> Can we also test that a client must *not* present a cert? I believe the answer is _no_, but a TODO or a NOTE could be helpful. 3rdparty/libprocess/src/tests/ssl_tests.cpp Lines 290 (patched) <https://reviews.apache.org/r/70748/#comment302372> s/VerifyAnonymousCipher/NoAnonymousCipherIfVerify/ 3rdparty/libprocess/src/tests/ssl_tests.cpp Lines 296 (patched) <https://reviews.apache.org/r/70748/#comment302373> Let's mention that `ADH-AES256-SHA` is an anonymous cipher and maybe even link https://www.openssl.org/docs/man1.0.2/man1/ciphers.html ? - Alexander Rukletsov On May 31, 2019, 2:34 p.m., Benno Evers wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/70748/ > --- > > (Updated May 31, 2019, 2:34 p.m.) > > > Review request for mesos, Alexander Rukletsov, Jan-Philip Gehrcke, and Till > Toenshoff. > > > Bugs: MESOS-9791 > https://issues.apache.org/jira/browse/MESOS-9791 > > > 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 says 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/1/ > > > 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/#review215615 --- Ship it! Ship It! - Alexander Rukletsov 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 70549: Added authorization for `UpdateQuota` call in the master.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/70549/#review215484 --- include/mesos/authorizer/authorizer.proto Line 139 (original), 141-147 (patched) <https://reviews.apache.org/r/70549/#comment302197> How about something like: ``` // TODO(mzhu): Remove this action after associated API calls `SET_QUOTA` and `REMOVE_QUOTA` are no longer supported. // // NOTE: We cannot reuse this action for the `UPDATE_QUOTA` API call, because the associated `QuotaConfig` message contains more information than `QuotaInfo`. ``` include/mesos/authorizer/authorizer.proto Lines 149-150 (patched) <https://reviews.apache.org/r/70549/#comment302198> I would like us to challenge the necessity of passing `QuotaConfig` here. The built-in authorizer only looks at `role`, ignoring any information about resources. One might say that a custom authorizer might utilize that extra information, however, that extra information might not be enough to make a decision, because it does not include the current state or the state change, e.g., resource delta. Imagine an authorizer that allows decreasing quota for a number of principals, but only a few are allowed to increase. Passing `QuotaConfig` does not really help that authorizer to make a decision. Note that authorizer cannot tract previous requests to deduce the current quota state because previous request could have been dropped after successful authorization. I tend to keep it simple and use just the role. We should consult Till Tönshoff and Jan-Philip Gehrcke as maintainers of DC/OS authorizer for more input. - Alexander Rukletsov On May 22, 2019, 12:48 p.m., Meng Zhu wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/70549/ > --- > > (Updated May 22, 2019, 12:48 p.m.) > > > Review request for mesos, Alexander Rukletsov, Andrei Sekretenko, and > Benjamin Mahler. > > > Bugs: MESOS-9640 > https://issues.apache.org/jira/browse/MESOS-9640 > > > Repository: mesos > > > Description > --- > > A new authorizable action `UPDATE_QUOTA_WITH_CONFIG` is added. > This disambiguates with the old action `UPDATE_QUOTA` which > are used for the old `SetQuota` and `RemoveQuota` calls. > `UPDATE_QUOTA` action requires `QuotaInfo` as the object while > the new `UpdatedQuota` call uses `QuotaConfig`. To keep it compatible > with any external authorization modules, a new action is introduced. > > > Diffs > - > > include/mesos/authorizer/authorizer.proto > e2740c402732bb37db991ec92b9301e58b33215b > src/master/master.hpp 5ad128d00d3cdf0dca89eb637ae196987bdce412 > src/master/quota_handler.cpp a18d8bafda5604d1844f7f7ed31d4ea80fbf6d04 > src/tests/master_authorization_tests.cpp > ee69910a34416728bf14ed23f4a6faae6c1204a0 > > > Diff: https://reviews.apache.org/r/70549/diff/3/ > > > Testing > --- > > make check > > > Thanks, > > Meng Zhu > >
Re: Review Request 70691: Added more comments regarding `message QuotaConfig`.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/70691/#review215483 --- Ship it! Thank you for adding clarity, Meng! - Alexander Rukletsov On May 21, 2019, 3:23 p.m., Meng Zhu wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/70691/ > --- > > (Updated May 21, 2019, 3:23 p.m.) > > > Review request for mesos, Alexander Rukletsov and Benjamin Mahler. > > > Repository: mesos > > > Description > --- > > Added more comments regarding `message QuotaConfig`. > > > Diffs > - > > include/mesos/v1/quota/quota.proto b109ca2a9c7c4b8f1444cc654298c9e580d0ca42 > > > Diff: https://reviews.apache.org/r/70691/diff/1/ > > > Testing > --- > > N/A > > > Thanks, > > Meng Zhu > >
Re: Review Request 70589: Logged when `/__processes__` returns.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/70589/ --- (Updated May 23, 2019, 10:59 a.m.) Review request for mesos, Benjamin Mahler and Till Toenshoff. Bugs: MESOS-9766 https://issues.apache.org/jira/browse/MESOS-9766 Repository: mesos Description --- Adds a log entry when a response with generated by `/__processes__` is about to be returned to the client. Diffs (updated) - 3rdparty/libprocess/src/process.cpp a47b5709095665bbe6141b7531b956aeb911d04a Diff: https://reviews.apache.org/r/70589/diff/2/ Changes: https://reviews.apache.org/r/70589/diff/1-2/ Testing --- built & run on Mac OS 10.13.6 ``` I0503 13:13:00.885133 59219968 process.cpp:3599] Handling HTTP event for process '__processes__' with path: '/__processes__' I0503 13:13:00.886037 59219968 process.cpp:3412] HTTP GET for /__processes__ from 192.168.178.47:59548: '200 OK' after 1.06214ms ``` Thanks, Alexander Rukletsov
Re: Review Request 70549: Added authorization for `UpdateQuota` call in the master.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/70549/#review215160 --- include/mesos/authorizer/authorizer.proto Lines 138-140 (original), 139-144 (patched) <https://reviews.apache.org/r/70549/#comment301682> If my understanding is correct, both `UPDATE_QUOTA` and `UPDATE_QUOTA_CONFIG` are per role but use different objects in payload. Can we convert `QuotaConfig` to `QuotaInfo` for the purpose of authorization and spare extra action altogether? I see that authorization implementation is identical and does not rely on differences between `QuotaConfig` and `QuotaInfo`. include/mesos/authorizer/authorizer.proto Lines 143 (patched) <https://reviews.apache.org/r/70549/#comment301683> If you do plan to keep this action, the naming convention in this file suggests: `UPDATE_QUOTA_WITH_CONFIG` or `UPDATE_QUOTA_WITH_QUOTA_CONFIG`. - Alexander Rukletsov On May 6, 2019, 11 p.m., Meng Zhu wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/70549/ > --- > > (Updated May 6, 2019, 11 p.m.) > > > Review request for mesos, Andrei Sekretenko and Benjamin Mahler. > > > Bugs: MESOS-9640 > https://issues.apache.org/jira/browse/MESOS-9640 > > > Repository: mesos > > > Description > --- > > A new authorizable action `update_quota_configs` is added. > This disambiguates with the old action `update_quota` > which are used for the old `SetQuota` and > `RemoveQuota` calls. `update_quota` action requires > `QuotaInfo` as the object while the new `UpdatedQuota` call > uses `QuotaConfig`. To keep it compatible with any external > authorization modules, a new action `update_quota_config` > is introduced. > > > Diffs > - > > include/mesos/authorizer/authorizer.proto > e2740c402732bb37db991ec92b9301e58b33215b > src/master/master.hpp 7d9732f1e432f6f0290d234242864cbdbf381fa8 > src/master/quota_handler.cpp 5d449e6f027a69ccaa0ac3473ea4cf57441601f3 > > > Diff: https://reviews.apache.org/r/70549/diff/2/ > > > Testing > --- > > make check > > > Thanks, > > Meng Zhu > >
Re: Review Request 70589: Logged when `/__processes__` returns.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/70589/ --- (Updated May 9, 2019, 1:09 p.m.) Review request for mesos, Benjamin Mahler and Till Toenshoff. Bugs: MESOS-9766 https://issues.apache.org/jira/browse/MESOS-9766 Repository: mesos Description --- Adds a log entry when a response with generated by `/__processes__` is about to be returned to the client. Diffs - 3rdparty/libprocess/src/process.cpp 124836472313721a5dbfe4b1ca55f0da3cecd66b Diff: https://reviews.apache.org/r/70589/diff/1/ Testing --- built & run on Mac OS 10.13.6 ``` I0503 13:13:00.885133 59219968 process.cpp:3599] Handling HTTP event for process '__processes__' with path: '/__processes__' I0503 13:13:00.886037 59219968 process.cpp:3412] HTTP GET for /__processes__ from 192.168.178.47:59548: '200 OK' after 1.06214ms ``` Thanks, Alexander Rukletsov
Re: Review Request 70595: Adds a regression test for MESOS-9766.
> On May 3, 2019, 11:31 p.m., Chun-Hung Hsiao wrote:
> > 3rdparty/libprocess/src/tests/process_tests.cpp
> > Lines 2092-2108 (patched)
> > <https://reviews.apache.org/r/70595/diff/1/?file=2142838#file2142838line2092>
> >
> > I tried this unit test w/o the fix in the following settings:
> > 1. `--gtest_repeat=100`, which resulted in 3 passes and 97 failures.
> > 2. Invoked the test w/o repetition in a for-loop, which resulted in 60
> > passes and 40 failures.
> >
> > So it seems to me that this test passes fairly easily if it only runs
> > once.
> >
> > Then I replace this snippet with the following code, which just used 1
> > actor but failed every time in the above two settings:
> > ```
> > UPID pid = spawn(new ProcessBase(), true);
> >
> > dispatch(pid, [] { os::sleep(Milliseconds(50)); });
> >
> > http::URL url = http::URL(
> > "http",
> > process::address().ip,
> > process::address().port,
> > "/__processes__");
> >
> > Future response = http::get(url);
> >
> > terminate(pid, true);
> > ```
Agree with Chun, we should avoid races as much as we can. Several suggestions
to further improve Chun's version:
- Pause the clock in the beginnig. We can then sleep for 42 minutes to make
sure the first message is still being processes when `http::get()` is invoked
(will of course require `settle()` before).
- `settle()` after `http::get` to ensure `/__processes__` handler is put into
the `pid`'s mailbaox.
- Check that the resulted JSON does not include the entry for `pid`, i.e., is
empty.
- Alexander
---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/70595/#review215035
---
On May 3, 2019, 7:58 p.m., Benjamin Mahler wrote:
>
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/70595/
> ---
>
> (Updated May 3, 2019, 7:58 p.m.)
>
>
> Review request for mesos, Alexander Rukletsov and Chun-Hung Hsiao.
>
>
> Bugs: MESOS-9766
> https://issues.apache.org/jira/browse/MESOS-9766
>
>
> Repository: mesos
>
>
> Description
> ---
>
> This test fails on master prior to applying the fix for MESOS-9766.
> It attempts to ensure that processes are terminated after the
> /__processes__ handler dispatches to them.
>
>
> Diffs
> -
>
> 3rdparty/libprocess/src/tests/process_tests.cpp
> 60f3dd653153c2b2ccf9c3a7eae8c75fd6ff025c
>
>
> Diff: https://reviews.apache.org/r/70595/diff/1/
>
>
> Testing
> ---
>
> Ran in repetition, although it appears to consistently fail on master without
> repetition needed.
>
>
> Thanks,
>
> Benjamin Mahler
>
>
Re: Review Request 70594: Fixed an issue where /__processes__ never returns a response.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/70594/#review215059 --- Ship it! Slick solution, thanks, Ben! - Alexander Rukletsov On May 3, 2019, 7:58 p.m., Benjamin Mahler wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/70594/ > --- > > (Updated May 3, 2019, 7:58 p.m.) > > > Review request for mesos, Alexander Rukletsov and Chun-Hung Hsiao. > > > Bugs: MESOS-9766 > https://issues.apache.org/jira/browse/MESOS-9766 > > > Repository: mesos > > > Description > --- > > It's possible for /__processes__ to never return a response if > a process is terminated after the /__processes__ handler dispatches > to it, thus leading the Future to be abandoned. > > This patch ensures we ignore those cases, rather than wait forever. > > See MESOS-9766. > > > Diffs > - > > 3rdparty/libprocess/src/process.cpp > 124836472313721a5dbfe4b1ca55f0da3cecd66b > > > Diff: https://reviews.apache.org/r/70594/diff/1/ > > > Testing > --- > > Added a test in the subsequent patch. > > > Thanks, > > Benjamin Mahler > >
Review Request 70589: Logged when `/__processes__` returns.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/70589/ --- Review request for mesos, Benjamin Mahler and Till Toenshoff. Repository: mesos Description --- Adds a log entry when a response with generated by `/__processes__` is about to be returned to the client. Diffs - 3rdparty/libprocess/src/process.cpp 124836472313721a5dbfe4b1ca55f0da3cecd66b Diff: https://reviews.apache.org/r/70589/diff/1/ Testing --- built & run on Mac OS 10.13.6 ``` I0503 13:13:00.885133 59219968 process.cpp:3599] Handling HTTP event for process '__processes__' with path: '/__processes__' I0503 13:13:00.886037 59219968 process.cpp:3412] HTTP GET for /__processes__ from 192.168.178.47:59548: '200 OK' after 1.06214ms ``` Thanks, Alexander Rukletsov
Re: Review Request 70566: Prevented logging of coverity token in CI.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/70566/#review214962 --- Ship it! Ship It! - Alexander Rukletsov On April 29, 2019, 9:12 a.m., Benjamin Bannier wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/70566/ > --- > > (Updated April 29, 2019, 9:12 a.m.) > > > Review request for mesos, Alexander Rukletsov and Till Toenshoff. > > > Repository: mesos > > > Description > --- > > Prevented logging of coverity token in CI. > > > Diffs > - > > support/docker-build.sh 0c6988d246dee711bb78580efd7b690940c9bb63 > > > Diff: https://reviews.apache.org/r/70566/diff/1/ > > > Testing > --- > > > Thanks, > > Benjamin Bannier > >
Re: Review Request 70098: Removed `support/cpplint.patch`.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/70098/#review213514 --- Ship it! Ship It! - Alexander Rukletsov 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/70098/ > --- > > (Updated March 3, 2019, 1:01 a.m.) > > > Review request for mesos and Armand Grillet. > > > Repository: mesos > > > Description > --- > > This change removes explicit tracking of a patch against upstream > cpplint in the tree and replaces it with a clear reference against the > upstream repository and revision used in the modified file. > > The patch was never used by us against any upstream version, and was > not e.g., a legal requirement. The process around creating the patch > was cumbersome and error prone. Removing the patch from the tree > should not only make it easier to evolve our modifications going > forward, but also remove a lot of noise from changes. > > > Diffs > - > > support/cpplint.patch 2427be87559e6dd21c8841f1719064b5d46e4fc7 > support/cpplint.py d3e7aaf71255cf4efd0239434f7630afd6ea47d0 > > > Diff: https://reviews.apache.org/r/70098/diff/2/ > > > Testing > --- > > N/A > > > Thanks, > > Benjamin Bannier > >
Re: Review Request 70093: Added license header to some Python source files.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/70093/#review213487 --- Fix it, then Ship it! src/python/scheduler/src/mesos/__init__.py Lines 17-32 (patched) <https://reviews.apache.org/r/70093/#comment299421> Why doubled? - Alexander Rukletsov 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/70093/ > --- > > (Updated March 3, 2019, 1:01 a.m.) > > > Review request for mesos, Armand Grillet, haosdent huang, and Steve Niemitz. > > > Repository: mesos > > > Description > --- > > Added license header to some Python source files. > > > Diffs > - > > mpi/mpiexec-mesos.py d86c85bd68e75f2ddd190254da4f520e9f126d6f > src/python/cli/src/mesos/__init__.py > 3fcba01e3801cab7b21cbfc32972e9ab2810ddda > src/python/executor/src/mesos/__init__.py > 3fcba01e3801cab7b21cbfc32972e9ab2810ddda > src/python/interface/src/mesos/__init__.py > 3fcba01e3801cab7b21cbfc32972e9ab2810ddda > src/python/interface/src/mesos/v1/__init__.py > e69de29bb2d1d6434b8b29ae775ad8c2e48c5391 > src/python/interface/src/mesos/v1/interface/__init__.py > e69de29bb2d1d6434b8b29ae775ad8c2e48c5391 > src/python/native/src/mesos/__init__.py > 3fcba01e3801cab7b21cbfc32972e9ab2810ddda > src/python/protocol/src/mesos/__init__.py > 3fcba01e3801cab7b21cbfc32972e9ab2810ddda > src/python/scheduler/src/mesos/__init__.py > 3fcba01e3801cab7b21cbfc32972e9ab2810ddda > src/python/src/mesos/__init__.py f48ad10528712b2b8960f1863d156b88ed1ce311 > > > Diff: https://reviews.apache.org/r/70093/diff/1/ > > > Testing > --- > > `make check` > > > Thanks, > > Benjamin Bannier > >
Re: Review Request 70094: Fixed garbled license header.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/70094/#review213488 --- Ship it! Ship It! - Alexander Rukletsov 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/70094/ > --- > > (Updated March 3, 2019, 1:01 a.m.) > > > Review request for mesos, Jie Yu, Jojy Varghese, and Till Toenshoff. > > > Repository: mesos > > > Description > --- > > Fixed garbled license header. > > > Diffs > - > > src/slave/containerizer/mesos/provisioner/appc/fetcher.cpp > f5e90527e6ace45a7745ca1ba1a9eeef938d2a68 > > > Diff: https://reviews.apache.org/r/70094/diff/1/ > > > Testing > --- > > `make check` > > > Thanks, > > Benjamin Bannier > >
Re: Review Request 70095: Fixed cpplint issues in the Java bindings.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/70095/#review213484 --- Fix it, then Ship it! src/java/jni/convert.cpp Line 55 (original), 55-56 (patched) <https://reviews.apache.org/r/70095/#comment299419> ``` // Initialized in `JNI_OnLoad` later in this file. jweak mesosClassLoader = nullptr; ``` - Alexander Rukletsov 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/70095/ > --- > > (Updated March 3, 2019, 1:01 a.m.) > > > Review request for mesos and Gastón Kleiman. > > > Repository: mesos > > > Description > --- > > Fixed cpplint issues in the Java bindings. > > > Diffs > - > > src/java/jni/construct.cpp 2645d9e5282f352bd663dfeb71d5244662773704 > src/java/jni/convert.cpp 338eb963cf843dbffc2d59d13171eb987ac2bc65 > src/java/jni/org_apache_mesos_Log.cpp > 1ffbacff7584a0b70e03588ad16918f136af1937 > src/java/jni/org_apache_mesos_MesosExecutorDriver.cpp > 9d1d45667317048916fbdf6537a07061215cac2c > src/java/jni/org_apache_mesos_MesosSchedulerDriver.cpp > 762fe85e181f0e2c9002321d07f8ad4f91638e92 > src/java/jni/org_apache_mesos_state_AbstractState.cpp > 0f12f4f7763727da51edb72f8e16cc520a783e42 > src/java/jni/org_apache_mesos_state_LevelDBState.cpp > b06956f5f16cdcbc242e7a0fce39ddb7f214994c > src/java/jni/org_apache_mesos_state_ZooKeeperState.cpp > b2734c6154a700d8f96216209f4b1aeee883e1ae > src/java/jni/org_apache_mesos_v1_scheduler_V0Mesos.cpp > 4e4dd54007d9a4da88acc9e38bd48d1e599d74d3 > support/mesos-style.py 11d5f96d4ca534a7d51ed93d2d6b0c528d31fad4 > > > Diff: https://reviews.apache.org/r/70095/diff/2/ > > > Testing > --- > > `make check` with enabled Java bindings > > > Thanks, > > Benjamin Bannier > >
Re: Review Request 70099: Parameterized cpplint extension list via config instead of via patch.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/70099/#review213481 --- Ship it! Ship It! - Alexander Rukletsov On March 3, 2019, 1:02 a.m., Benjamin Bannier wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/70099/ > --- > > (Updated March 3, 2019, 1:02 a.m.) > > > Review request for mesos and Benjamin Mahler. > > > Repository: mesos > > > Description > --- > > Parameterized cpplint extension list via config instead of via patch. > > > Diffs > - > > support/cpplint.patch 2427be87559e6dd21c8841f1719064b5d46e4fc7 > support/cpplint.py b8b3b1a14d3ac56fa4c8e44b271b32e3308cb4e1 > support/mesos-style.py 11d5f96d4ca534a7d51ed93d2d6b0c528d31fad4 > > > Diff: https://reviews.apache.org/r/70099/diff/1/ > > > Testing > --- > > ./support/mesos-style.py > > > Thanks, > > Benjamin Bannier > >
Re: Review Request 70102: Moved NULL check from cpplint into clang-tidy.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/70102/#review213480 --- Ship it! Semantics over syntax! - Alexander Rukletsov On March 3, 2019, 1:02 a.m., Benjamin Bannier wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/70102/ > --- > > (Updated March 3, 2019, 1:02 a.m.) > > > Review request for mesos, Benjamin Mahler and Tomasz Janiszewski. > > > Repository: mesos > > > Description > --- > > Moved NULL check from cpplint into clang-tidy. > > > Diffs > - > > support/CPPLINT.cfg PRE-CREATION > support/clang-tidy d448314b245a043ced0c4816628cd7c05ea8a68d > support/cpplint.patch 2427be87559e6dd21c8841f1719064b5d46e4fc7 > support/cpplint.py b8b3b1a14d3ac56fa4c8e44b271b32e3308cb4e1 > > > Diff: https://reviews.apache.org/r/70102/diff/1/ > > > Testing > --- > > `make check` > > > 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/#review213483 --- Ship it! Ship It! - Alexander Rukletsov On March 3, 2019, 1:01 a.m., Benjamin Bannier wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/70096/ > --- > > (Updated March 3, 2019, 1:01 a.m.) > > > Review request for mesos and Benjamin Mahler. > > > Repository: mesos > > > Description > --- > > Moved cpplint configuration into dedicated file. > > > Diffs > - > > bootstrap 7be2cc95c7ace55d710315427f38284cc5b2af4c > support/CPPLINT.cfg PRE-CREATION > support/mesos-style.py 11d5f96d4ca534a7d51ed93d2d6b0c528d31fad4 > > > Diff: https://reviews.apache.org/r/70096/diff/1/ > > > 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 70098: Made cpplint.patch reflect our modifications.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/70098/#review213482 --- Ship it! Ship It! - Alexander Rukletsov 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/70098/ > --- > > (Updated March 3, 2019, 1:01 a.m.) > > > Review request for mesos and Armand Grillet. > > > Repository: mesos > > > Description > --- > > This patch was created by performing a `git diff` of our current > version agains our upstream version `43d512ba130`. > > This patch restores trailing but significant whitespaces to the patch > file, which are flagged by our commit hooks. When committing we need to > skip these hooks. > > > Diffs > - > > support/cpplint.patch 2427be87559e6dd21c8841f1719064b5d46e4fc7 > > > Diff: https://reviews.apache.org/r/70098/diff/1/ > > > Testing > --- > > N/A > > > Thanks, > > Benjamin Bannier > >
Re: Review Request 70100: Skipped pylint for cpplint.py per config instead of per patch.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/70100/#review213478 --- Ship it! Ship It! - Alexander Rukletsov On March 3, 2019, 1:02 a.m., Benjamin Bannier wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/70100/ > --- > > (Updated March 3, 2019, 1:02 a.m.) > > > Review request for mesos, Armand Grillet and Kevin Klues. > > > Repository: mesos > > > Description > --- > > Skipped pylint for cpplint.py per config instead of per patch. > > > Diffs > - > > support/cpplint.patch 2427be87559e6dd21c8841f1719064b5d46e4fc7 > support/cpplint.py b8b3b1a14d3ac56fa4c8e44b271b32e3308cb4e1 > support/pylint.config af25dd90cb2d467c688ea4b060dc4640040a068b > > > Diff: https://reviews.apache.org/r/70100/diff/1/ > > > Testing > --- > > `./support/mesos-style.py support/cpplint.py` still skips the file > > > Thanks, > > Benjamin Bannier > >
Re: Review Request 70101: Removed manual author list cpplint.patch.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/70101/#review213479 --- Ship it! I can't think of a legal reason, i.e., license requirements, for this to be needed there — blessing for removal! - Alexander Rukletsov On March 3, 2019, 1:02 a.m., Benjamin Bannier wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/70101/ > --- > > (Updated March 3, 2019, 1:02 a.m.) > > > Review request for mesos and Benjamin Mahler. > > > Repository: mesos > > > Description > --- > > Removed manual author list cpplint.patch. > > > Diffs > - > > support/cpplint.patch 2427be87559e6dd21c8841f1719064b5d46e4fc7 > support/cpplint.py b8b3b1a14d3ac56fa4c8e44b271b32e3308cb4e1 > > > Diff: https://reviews.apache.org/r/70101/diff/1/ > > > Testing > --- > > N/A > > > Thanks, > > Benjamin Bannier > >
Re: Review Request 70099: Parameterized cpplint extension list via config instead of via patch.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/70099/#review213477 --- support/mesos-style.py Line 266 (original), 266 (patched) <https://reviews.apache.org/r/70099/#comment299418> This might reduce the files we check, and it's not reflected in the summary. May I ask you to either extract it into a separate patch or update the summary? - Alexander Rukletsov On March 3, 2019, 1:02 a.m., Benjamin Bannier wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/70099/ > --- > > (Updated March 3, 2019, 1:02 a.m.) > > > Review request for mesos and Benjamin Mahler. > > > Repository: mesos > > > Description > --- > > Parameterized cpplint extension list via config instead of via patch. > > > Diffs > - > > support/cpplint.patch 2427be87559e6dd21c8841f1719064b5d46e4fc7 > support/cpplint.py b8b3b1a14d3ac56fa4c8e44b271b32e3308cb4e1 > support/mesos-style.py 11d5f96d4ca534a7d51ed93d2d6b0c528d31fad4 > > > Diff: https://reviews.apache.org/r/70099/diff/1/ > > > Testing > --- > > ./support/mesos-style.py > > > Thanks, > > Benjamin Bannier > >
Re: Review Request 70098: Made cpplint.patch reflect our modifications.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/70098/#review213470 --- Looking at this deeper, I'm not sure we should explicitly maintain the patch. Of course, ideally we would have a copy of a specific version of cpplint plus the patch, but it is unclear when to apply the patch (on bootstrap? on build?). To make things simpler and have only one source of truth, let's **remove** the patch altogether and leave a comment in the **modified** version of `cpplint.py` with 1) SHA of the base `cpplint.py` to be able to restore the diff and 2) a quick overview of our modifications. support/cpplint.patch Lines 1-4 (original), 1-4 (patched) <https://reviews.apache.org/r/70098/#comment299402> These are not whitespace changes : ) - Alexander Rukletsov 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/70098/ > --- > > (Updated March 3, 2019, 1:01 a.m.) > > > Review request for mesos and Armand Grillet. > > > Repository: mesos > > > Description > --- > > This patch was created by performing a `git diff` of our current > version agains our upstream version `43d512ba130`. > > This patch restores trailing but significant whitespaces to the patch > file, which are flagged by our commit hooks. When committing we need to > skip these hooks. > > > Diffs > - > > support/cpplint.patch 2427be87559e6dd21c8841f1719064b5d46e4fc7 > > > Diff: https://reviews.apache.org/r/70098/diff/1/ > > > Testing > --- > > N/A > > > Thanks, > > Benjamin Bannier > >
Re: Review Request 70097: Reverted untracked and unneeded cpplint modification.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/70097/#review213469 --- Ship it! Ship It! - Alexander Rukletsov 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/70097/ > --- > > (Updated March 3, 2019, 1:01 a.m.) > > > Review request for mesos and Armand Grillet. > > > Repository: mesos > > > Description > --- > > Reverted untracked and unneeded cpplint modification. > > > Diffs > - > > support/cpplint.py b8b3b1a14d3ac56fa4c8e44b271b32e3308cb4e1 > > > Diff: https://reviews.apache.org/r/70097/diff/1/ > > > Testing > --- > > `./support/mesos-style.py` > > > Thanks, > > Benjamin Bannier > >
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/ --- 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 69986: Added a log line to isolator for simplified debugging.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/69986/ --- (Updated Feb. 15, 2019, 8:54 a.m.) Review request for mesos, Andrei Budnik and Gilbert Song. Summary (updated) - Added a log line to isolator for simplified debugging. Repository: mesos Description (updated) --- Added a log line to isolator 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 69986: Added a log line for simplified debugging.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/69986/ --- 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
Re: Review Request 69860: Disabled parallel test execution for reviewbot.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/69860/#review212442 --- Ship it! Ship It! - Alexander Rukletsov On Jan. 30, 2019, 8:09 a.m., Benjamin Bannier wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/69860/ > --- > > (Updated Jan. 30, 2019, 8:09 a.m.) > > > Review request for mesos, Alexander Rukletsov and Benjamin Mahler. > > > Repository: mesos > > > Description > --- > > While we globally disabled parallel test execution for reviewbot jobs > in `37532fae01d`, we missed that the configuration was overwritten > downstream. > > This patch disables parallel tests also in the script ultimately > performing the final configuration. > > > Diffs > - > > support/verify-reviews.py 71326d34bb649e27a3a2901867d31a2a1fffd4e9 > > > Diff: https://reviews.apache.org/r/69860/diff/1/ > > > Testing > --- > > > Thanks, > > Benjamin Bannier > >
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/#review212327 --- Fix it, then Ship it! src/scheduler/scheduler.cpp Line 234 (original), 234 (patched) <https://reviews.apache.org/r/69839/#comment298088> I would move it closer to the invokation of `authenticate()` for clarity. - Alexander Rukletsov On Jan. 25, 2019, 3:41 p.m., Till Toenshoff wrote: > > --- > 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. > > > Bugs: MESOS-9210 > https://issues.apache.org/jira/browse/MESOS-9210 > > > Repository: mesos > > > Description > --- > > 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 > >
Re: Review Request 69839: Fixed scheduler library on multiple SUBSCRIBE requests per connection.
> On Jan. 25, 2019, 4:01 p.m., Vinod Kone wrote: > > src/scheduler/scheduler.cpp > > Line 234 (original), 234 (patched) > > <https://reviews.apache.org/r/69839/diff/1/?file=2122187#file2122187line234> > > > > s/Augmenting/Authenticating/ Please no: authenticatee does not really authenticate, but adds authentication-related headers to the request. > On Jan. 25, 2019, 4:01 p.m., Vinod Kone wrote: > > src/scheduler/scheduler.cpp > > Line 587 (original), 573 (patched) > > <https://reviews.apache.org/r/69839/diff/1/?file=2122187#file2122187line587> > > > > s/to augment request// > > > > I didn't know "<< future" is thing! I would keep this for clarity. - Alexander --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/69839/#review212325 --- On Jan. 25, 2019, 3:41 p.m., Till Toenshoff wrote: > > --- > 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. > > > Bugs: MESOS-9210 > https://issues.apache.org/jira/browse/MESOS-9210 > > > Repository: mesos > > > Description > --- > > 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 > >
Re: Review Request 69839: Fixed scheduler library on multiple SUBSCRIBE requests per connection.
> On Jan. 25, 2019, 4:07 p.m., Alexander Rukletsov wrote: > > src/scheduler/scheduler.cpp > > Line 234 (original), 234 (patched) > > <https://reviews.apache.org/r/69839/diff/1/?file=2122187#file2122187line234> > > > > I would move it closer to the invokation of `authenticate()` for > > clarity. Ups, didn't mean it to be an issue, just a mere suggestion; feel free to ignore. - Alexander --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/69839/#review212327 --- On Jan. 25, 2019, 3:41 p.m., Till Toenshoff wrote: > > --- > 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. > > > Bugs: MESOS-9210 > https://issues.apache.org/jira/browse/MESOS-9210 > > > Repository: mesos > > > Description > --- > > 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 > >
Re: Review Request 66815: Removed an unconditional .get() in DefaultExecutor.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/66815/#review211188 --- Fix it, then Ship it! src/launcher/default_executor.cpp Line 938 (original), 939-944 (patched) <https://reviews.apache.org/r/66815/#comment296119> Can we make it a one-liner? For example, ``` LOG(INFO) << "Child container " << container->containerId << " of task '" << taskId << "' completed in state " << stringify(taskState) << (message.isSome() ? ": " + message.get() : ""); ``` - Alexander Rukletsov On April 26, 2018, 3:24 p.m., Benno Evers wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/66815/ > --- > > (Updated April 26, 2018, 3:24 p.m.) > > > Review request for mesos and Gilbert Song. > > > Bugs: MESOS-8797 > https://issues.apache.org/jira/browse/MESOS-8797 > > > Repository: mesos > > > Description > --- > > This would fail in some cases where the optional > message is `None`, e.g. when the container is in > killing state. > > > Diffs > - > > src/launcher/default_executor.cpp ea0d425bb60e970f209f411632e1d486c279259b > > > Diff: https://reviews.apache.org/r/66815/diff/1/ > > > Testing > --- > > `./src/mesos-tests > --gtest_filter=MesosContainerizer/DefaultExecutorTest.TaskUsesExecutor/0 > --gtest_repeat=500 --gtest_break_on_failure` > > > Thanks, > > Benno Evers > >
Re: Review Request 69464: Made the `createTask` helper work for both v0 and v1 API.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/69464/#review210929 --- Ship it! Ship It! - Alexander Rukletsov On Nov. 27, 2018, 11:05 p.m., Chun-Hung Hsiao wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/69464/ > --- > > (Updated Nov. 27, 2018, 11:05 p.m.) > > > Review request for mesos, Alexander Rukletsov, Benjamin Mahler, Greg Mann, > and Till Toenshoff. > > > Bugs: MESOS-9419 > https://issues.apache.org/jira/browse/MESOS-9419 > > > Repository: mesos > > > Description > --- > > The patched `createTask` helper originally uses the `slave_id` accessor, > which does not apply to the v1 API. This patch fixes this problem. > > > Diffs > - > > src/tests/mesos.hpp 576f4bde88c069ee2fa0dd33912a034437338e7e > > > Diff: https://reviews.apache.org/r/69464/diff/1/ > > > Testing > --- > > make check > > > Thanks, > > Chun-Hung Hsiao > >
Re: Review Request 69451: Fixed master crash when executors send messages to recovered frameworks.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/69451/#review210928 --- src/master/master.hpp Line 2609 (original), 2621 (patched) <https://reviews.apache.org/r/69451/#comment295764> Does it make sense to add a log line in case neither `http` not `pid` are set? - Alexander Rukletsov On Nov. 28, 2018, 12:40 a.m., Chun-Hung Hsiao wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/69451/ > --- > > (Updated Nov. 28, 2018, 12:40 a.m.) > > > Review request for mesos, Alexander Rukletsov, Benjamin Mahler, Greg Mann, > and Till Toenshoff. > > > Bugs: MESOS-9419 > https://issues.apache.org/jira/browse/MESOS-9419 > > > Repository: mesos > > > Description > --- > > The `Framework::send` function assumes that either `http` or `pid` is > set, which is not true for a framework that hasn't yet reregistered yet > but recovered from a reregistered agent. As a result, the master would > crash when a recovered executor tries to send a message to such a > framework (see MESOS-9419). This patch fixes this crash bug. > > > Diffs > - > > src/master/master.hpp 3b3c1a4e61de9503c8d038dd3bee623ded5914c9 > src/master/master.cpp b4b02d8b4d7d6d1aabda1f97b9bf824419f76a9e > > > Diff: https://reviews.apache.org/r/69451/diff/3/ > > > Testing > --- > > make check > > > Thanks, > > Chun-Hung Hsiao > >
Re: Review Request 69405: Refactored createAuthorizationCallbacks into common/authorization.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/69405/#review210723 --- Fix it, then Ship it! Lovely! src/common/authorization.hpp Lines 34 (patched) <https://reviews.apache.org/r/69405/#comment295456> please move `stout/hashset` include here - Alexander Rukletsov On Nov. 20, 2018, 2:35 a.m., Till Toenshoff wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/69405/ > --- > > (Updated Nov. 20, 2018, 2:35 a.m.) > > > Review request for mesos, Alexander Rukletsov, Alexander Rojas, and Greg Mann. > > > Repository: mesos > > > Description > --- > > Moves 'createAuthorizationCallbacks' out of common/http into > common/authorization. > > > Diffs > - > > src/authorizer/local/authorizer.cpp > 83944b9a7322707609c2d4e13b84ab3a44cf4e25 > src/common/authorization.hpp PRE-CREATION > src/common/authorization.cpp PRE-CREATION > src/common/http.hpp 6ca54a641d1ea1625c70518f8870516664741e9a > src/common/http.cpp d9703f80880850b8a1ec9ddd2b1e8f125f36 > src/master/main.cpp 2c7b1bb492a0655dec9280e98ff942a15e2ae8f0 > src/slave/main.cpp e774092ff2c3941f17cdebfb26d80c05a26497c6 > src/tests/cluster.cpp 2b351ca70d8e80008e49722aa7d46918b5ecd9b0 > src/tests/logging_tests.cpp 30bcdc7f4aa9d6a39c5ef6e825357815bf4f9f19 > > > Diff: https://reviews.apache.org/r/69405/diff/1/ > > > Testing > --- > > `make check` and internal CI > > > Thanks, > > Till Toenshoff > >
Re: Review Request 69386: Added test for ACCESS_MESOS_LOG authorization.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/69386/#review210650 --- Ship it! Appreciate you cleaning up things around you even though they are not directly related to the ticket you're working on. Thank you, Till! - Alexander Rukletsov On Nov. 18, 2018, 11:11 p.m., Till Toenshoff wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/69386/ > --- > > (Updated Nov. 18, 2018, 11:11 p.m.) > > > Review request for mesos, Alexander Rukletsov and Alexander Rojas. > > > Repository: mesos > > > Description > --- > > Added test for ACCESS_MESOS_LOG authorization. > > > Diffs > - > > src/tests/authorization_tests.cpp ac52181aa29bb5a5e4197cd90f32330aeb59385f > > > Diff: https://reviews.apache.org/r/69386/diff/1/ > > > Testing > --- > > `make check` and internal CI > > > Thanks, > > Till Toenshoff > >
Re: Review Request 69384: Introduced common/authorization and refactored collectAuthorizations.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/69384/#review210645 --- Fix it, then Ship it! src/master/weights_handler.cpp Lines 33 (patched) <https://reviews.apache.org/r/69384/#comment295371> This should go before "master/..." - Alexander Rukletsov On Nov. 18, 2018, 10:52 p.m., Till Toenshoff wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/69384/ > --- > > (Updated Nov. 18, 2018, 10:52 p.m.) > > > Review request for mesos, Alexander Rukletsov, Alexander Rojas, and Benjamin > Bannier. > > > Bugs: MESOS-9317 > https://issues.apache.org/jira/browse/MESOS-9317 > > > Repository: mesos > > > Description > --- > > Adds a new collection of authorization specific helper/s to reduce code > duplication and increase efficient test coverage. > > Moves the newly introduced 'collectAuthorizations' helper into this new > authorization source unit. > > > Diffs > - > > src/CMakeLists.txt 732368293049b7d9d6f62057344d433637ad44e8 > src/Makefile.am c17eae4ff1d019d515f67d81821e933ecb5dc190 > src/common/authorization.hpp PRE-CREATION > src/common/authorization.cpp PRE-CREATION > src/master/master.hpp e77babf22126838c63cd05e483875c9beb3ac5ff > src/master/master.cpp 9458ff10999d48e40a8596ec4edf1243591bc0d4 > src/master/weights_handler.cpp 222ec754e216da195250d1895a728294a076ee5d > src/tests/master_tests.cpp ac6bf379c5906cf9612284911c121c9457f648a0 > > > Diff: https://reviews.apache.org/r/69384/diff/1/ > > > Testing > --- > > make check and private ci > > > Thanks, > > Till Toenshoff > >
Re: Review Request 69385: Refactored createSubject and authorizeLogAccess to common/authorization.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/69385/#review210648 --- Fix it, then Ship it! src/common/http.cpp Lines 905-906 (original), 880-881 (patched) <https://reviews.apache.org/r/69385/#comment295373> Looks like another candidate for "common/authorization.hpp". Mind leaving a TODO? src/master/master.cpp Line 1077 (original), 1077 (patched) <https://reviews.apache.org/r/69385/#comment295374> Let's leave a TODO here (it will also help explaining why you do this): ``` // TODO(tillt): Use lambda named captures for // these cached values once it is available. ``` src/slave/slave.cpp Line 839 (original), 840 (patched) <https://reviews.apache.org/r/69385/#comment295375> Ditto - Alexander Rukletsov On Nov. 18, 2018, 11:10 p.m., Till Toenshoff wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/69385/ > --- > > (Updated Nov. 18, 2018, 11:10 p.m.) > > > Review request for mesos, Alexander Rukletsov, Alexander Rojas, Benjamin > Bannier, and Greg Mann. > > > Repository: mesos > > > Description > --- > > Moves 'createSubject' out of common/http into common/authorization. > > Removes duplicate 'authorizeLogAccess' out of master.cpp and slave.cpp. > Introduces 'authorizeLogAccess' within common/authorization. > > > Diffs > - > > src/common/authorization.hpp PRE-CREATION > src/common/authorization.cpp PRE-CREATION > src/common/http.hpp 6ca54a641d1ea1625c70518f8870516664741e9a > src/common/http.cpp d9703f80880850b8a1ec9ddd2b1e8f125f36 > src/master/http.cpp 75ab6ea422a77e25049cd6afe007507d11650a06 > src/master/master.hpp e77babf22126838c63cd05e483875c9beb3ac5ff > src/master/master.cpp 9458ff10999d48e40a8596ec4edf1243591bc0d4 > src/master/quota_handler.cpp a4975fd50a2be20f3af8bbcfa81255b5dcc64fed > src/slave/http.cpp 816aed1607bb7d25851052662b6ffa306c62b983 > src/slave/slave.hpp 0bd340176e2a8cefdfa7ef71e059441fb171aff6 > src/slave/slave.cpp 74f6fb9036a9ac4f587f53ec2df04eeb4c167bfb > > > Diff: https://reviews.apache.org/r/69385/diff/1/ > > > Testing > --- > > `make check` and internal CI validation > > > Thanks, > > Till Toenshoff > >
Re: Review Request 68131: Added MasterActorResponsiveness_BENCHMARK_Test.
> On Nov. 6, 2018, 12:19 p.m., Benno Evers wrote:
> > src/tests/master_benchmarks.cpp
> > Line 640 (original), 674-677 (patched)
> > <https://reviews.apache.org/r/68131/diff/3-4/?file=2070789#file2070789line679>
> >
> > I'm a bit confused by the intention behind the stop condition:
> >
> > My best guess is that you want to ensure that you have constant load
> > during the whole duration of the benchmark. However, in that case it seems
> > like the requests to `/state` should not be limited to a specific number
> > but continue indefinitely, and `stop` should be set to true after the
> > required number of requests to the indicator endpoint have happened.
> >
> > On the other hand, if the current implementation is as intended, I
> > think the message should read `launching *up to* {numRequests} requests`,
> > because there's no guarantee that the loop with requests for the indicator
> > endpoint will be the one finishing first. Also, in this case I think the
> > comments could be a bit more specific about the intention.
I've swapped `indicator` and the `state` endpoints in regard to stopping
condition. I've also adjusted the comment. Marking as read; let me know if you
think something else should be done additionally.
- Alexander
---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/68131/#review210347
---
On Nov. 18, 2018, 8:10 p.m., Alexander Rukletsov wrote:
>
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/68131/
> ---
>
> (Updated Nov. 18, 2018, 8:10 p.m.)
>
>
> Review request for mesos, Benno Evers and Benjamin Mahler.
>
>
> Bugs: MESOS-8975
> https://issues.apache.org/jira/browse/MESOS-8975
>
>
> Repository: mesos
>
>
> Description
> ---
>
> See summary.
>
>
> Diffs
> -
>
> src/tests/master_benchmarks.cpp fbfffb69930c30b038f74e0b831fc0ae41c820f0
>
>
> Diff: https://reviews.apache.org/r/68131/diff/5/
>
>
> Testing
> ---
>
> See https://reviews.apache.org/r/68132/
>
>
> Thanks,
>
> Alexander Rukletsov
>
>
Re: Review Request 68131: Added MasterActorResponsiveness_BENCHMARK_Test.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/68131/ --- (Updated Nov. 18, 2018, 8:10 p.m.) Review request for mesos, Benno Evers and Benjamin Mahler. Bugs: MESOS-8975 https://issues.apache.org/jira/browse/MESOS-8975 Repository: mesos Description --- See summary. Diffs (updated) - src/tests/master_benchmarks.cpp fbfffb69930c30b038f74e0b831fc0ae41c820f0 Diff: https://reviews.apache.org/r/68131/diff/5/ Changes: https://reviews.apache.org/r/68131/diff/4-5/ Testing --- See https://reviews.apache.org/r/68132/ Thanks, Alexander Rukletsov
Re: Review Request 68131: Added MasterActorResponsiveness_BENCHMARK_Test.
> On Nov. 6, 2018, 12:19 p.m., Benno Evers wrote:
> > src/tests/master_benchmarks.cpp
> > Lines 616 (patched)
> > <https://reviews.apache.org/r/68131/diff/3-4/?file=2070789#file2070789line620>
> >
> > Last time we tried running this benchmark, we discovered a dead-lock
> > caused by the interaction between the use of libprocess worker threads in
> > both our test code and in the mesos code.
> >
> > Ideally we wouldn't use libprocess at all here, but to avoid another
> > big change late in the review cycle, adding a check like this should be
> > sufficient:
> > ```
> > if (numClients >= LIBPROCESS_NUM_WORKER_THREADS - 3) {
> > cerr << "Not enough worker threads for the desired number of
> > clients.";
> > exit(1);
> > }
> > ```
Though I'm in favour of the idea, I'm not sure we can reliably get
`LIBPROCESS_NUM_WORKER_THREADS` in tests. Do you think we should try find some
substitute or punt on this and leave a comment?
> On Nov. 6, 2018, 12:19 p.m., Benno Evers wrote:
> > src/tests/master_benchmarks.cpp
> > Line 646 (original), 702 (patched)
> > <https://reviews.apache.org/r/68131/diff/3-4/?file=2070789#file2070789line707>
> >
> > Given that we're capturing and printing baseline statistics anyways,
> > would it make sense to print the relative slowdown between baseline
> > performance and performance under load here, to get a metric that is
> > comparable across Mesos versions?
I'm not sure what a formual for a standard relative slowdown would be : ). Do
you have a suggestion? I'm inclining to leave it as is.
- Alexander
---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/68131/#review210347
---
On Nov. 4, 2018, 4:31 a.m., Alexander Rukletsov wrote:
>
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/68131/
> ---
>
> (Updated Nov. 4, 2018, 4:31 a.m.)
>
>
> Review request for mesos, Benno Evers and Benjamin Mahler.
>
>
> Bugs: MESOS-8975
> https://issues.apache.org/jira/browse/MESOS-8975
>
>
> Repository: mesos
>
>
> Description
> ---
>
> See summary.
>
>
> Diffs
> -
>
> src/tests/master_benchmarks.cpp b6d6dc7c1752491e2da854018966374b624d6682
>
>
> Diff: https://reviews.apache.org/r/68131/diff/4/
>
>
> Testing
> ---
>
> See https://reviews.apache.org/r/68132/
>
>
> Thanks,
>
> Alexander Rukletsov
>
>
Re: Review Request 69369: Added collectAuthorizations helper to master.hpp.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/69369/#review210624 --- Fix it, then Ship it! src/master/master.hpp Lines 2362 (patched) <https://reviews.apache.org/r/69369/#comment295358> Please include - Alexander Rukletsov On Nov. 18, 2018, 1:25 a.m., Till Toenshoff wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/69369/ > --- > > (Updated Nov. 18, 2018, 1:25 a.m.) > > > Review request for mesos, Alexander Rukletsov, Alexander Rojas, and Benjamin > Bannier. > > > Bugs: MESOS-9317 > https://issues.apache.org/jira/browse/MESOS-9317 > > > Repository: mesos > > > Description > --- > > Adds the helper function 'collectAuthorizations' to master.hpp. This > function allows for a simple way to collect authorization futures and > only if all supplied futures result in an approved authorization will > the returned future return true. > > All identified areas that were formally triggering MESOS-9317 are > being updated to make use of this new helper. > > A helper function has been chosen and preferred over copying this > pattern into the areas that needed a fix to allow for an efficient and > complete test coverage. > > Additionally we are adding a test validating that new helper. > > > Diffs > - > > src/master/master.hpp e77babf22126838c63cd05e483875c9beb3ac5ff > src/master/master.cpp 1e326ec42a7f79a0835529a4655e7ec272f1cf40 > src/master/weights_handler.cpp 222ec754e216da195250d1895a728294a076ee5d > src/tests/master_tests.cpp ac6bf379c5906cf9612284911c121c9457f648a0 > > > Diff: https://reviews.apache.org/r/69369/diff/2/ > > > Testing > --- > > `make check` and internal CI validation. > > > Thanks, > > Till Toenshoff > >
Re: Review Request 69368: Added test reproducing crash on authorization failure.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/69368/#review210623 --- Ship it! src/tests/master_tests.cpp Lines 10101 (patched) <https://reviews.apache.org/r/69368/#comment295356> We usually back tick variable, type names. src/tests/master_tests.cpp Lines 10109 (patched) <https://reviews.apache.org/r/69368/#comment295357> why is "Static" capitalized? - Alexander Rukletsov On Nov. 18, 2018, 1:23 a.m., Till Toenshoff wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/69368/ > --- > > (Updated Nov. 18, 2018, 1:23 a.m.) > > > Review request for mesos, Alexander Rukletsov, Alexander Rojas, and Benjamin > Bannier. > > > Bugs: MESOS-9317 > https://issues.apache.org/jira/browse/MESOS-9317 > > > Repository: mesos > > > Description > --- > > This test reproduces the scenario as described in MESOS-9317. The test > attempts to create a persistent volume by a web request to the > authorized V1 operator endpoint. The test assures that the underlying > authorization request fails as it can in production due to failures in > the authorization backend. > > Without fixing MESOS-9317, this test crashes the master process as the > code-path involved will attempt to access the contents of the awaited > future even though the future had failed. > > > Diffs > - > > src/tests/master_tests.cpp ac6bf379c5906cf9612284911c121c9457f648a0 > > > Diff: https://reviews.apache.org/r/69368/diff/2/ > > > Testing > --- > > `make check` before applying the fix for MESOS-9317 and after doing so. > > > Thanks, > > Till Toenshoff > >
Re: Review Request 69267: Fixed flaky SchedulerTest.MasterFailover.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/69267/#review210396 --- Ship it! Ship It! - Alexander Rukletsov On Nov. 7, 2018, 1:26 a.m., Joseph Wu wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/69267/ > --- > > (Updated Nov. 7, 2018, 1:26 a.m.) > > > Review request for mesos, Alexander Rukletsov and Greg Mann. > > > Bugs: MESOS-6949 > https://issues.apache.org/jira/browse/MESOS-6949 > > > Repository: mesos > > > Description > --- > > This test was flaky because there is a double-master-detection race > after the master fails over. This test uses the Standalone master > detector, which keeps a single Master PID in memory and always returns > that one PID as the leader. This means there is almost no delay > between failing over the master and detecting a new leader. > > The scheduler in this test tries to send a SUBSCRIBE call to the master > as soon as the master is detected. Normally, there will only be two > total SUBSCRIBE calls during the test, before and after the master > failover. However, the test also manually appoints the leader after > failing over the master. This step races against the scheduler's own > retry logic, and can potentially cause a third SUBSCRIBE if the second > SUBSCRIBE has already started. > > Because the scheduler in this test does not enable checkpointing, the > third SUBSCRIBE will actively disconnect the framework, causing the > master to remove the framework. This removal also prevents the > framework from ever registering again, and thereby times out the test. > > This fixes the test to prevent excess master detection events. > > We could also change the HTTP scheduler driver to ignore these extra > master detection events when the master in question has not changed. > > > Diffs > - > > src/tests/scheduler_tests.cpp 0ee5b77e5a667e37ac13553e15f634b2cb19ea65 > > > Diff: https://reviews.apache.org/r/69267/diff/1/ > > > Testing > --- > > make check > > GLOG_v=1 src/mesos-tests --gtest_filter="*SchedulerTest.MasterFailover*" > --gtest_repeat=-1 --gtest_break_on_failure --verbose > > > Thanks, > > Joseph Wu > >
Re: Review Request 68131: Added MasterActorResponsiveness_BENCHMARK_Test.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/68131/ --- (Updated Nov. 4, 2018, 4:31 a.m.) Review request for mesos, Benno Evers and Benjamin Mahler. Bugs: MESOS-8975 https://issues.apache.org/jira/browse/MESOS-8975 Repository: mesos Description --- See summary. Diffs (updated) - src/tests/master_benchmarks.cpp b6d6dc7c1752491e2da854018966374b624d6682 Diff: https://reviews.apache.org/r/68131/diff/4/ Changes: https://reviews.apache.org/r/68131/diff/3-4/ Testing --- See https://reviews.apache.org/r/68132/ Thanks, Alexander Rukletsov
Re: Review Request 69224: Fixed a test flake in `HealthCheckTest.HealthyTaskNonShell`.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/69224/#review210250 --- Ship it! Ship It! - Alexander Rukletsov On Nov. 1, 2018, 3:15 a.m., Chun-Hung Hsiao wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/69224/ > --- > > (Updated Nov. 1, 2018, 3:15 a.m.) > > > Review request for mesos, Alexander Rukletsov and Benno Evers. > > > Bugs: MESOS-9366 > https://issues.apache.org/jira/browse/MESOS-9366 > > > Repository: mesos > > > Description > --- > > Fixed a test flake in `HealthCheckTest.HealthyTaskNonShell`. > > > Diffs > - > > src/tests/health_check_tests.cpp da64dc2b4de6e1526628522af34ee83b8ef9f469 > > > Diff: https://reviews.apache.org/r/69224/diff/1/ > > > Testing > --- > > make check > > > Thanks, > > Chun-Hung Hsiao > >
Re: Review Request 68812: Added example framework for inverse-offers.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/68812/#review209970 --- Fix it, then Ship it! Please also add it to CMakeLists! src/examples/inverse_offer_framework.cpp Lines 495 (patched) <https://reviews.apache.org/r/68812/#comment294612> Let's extract it into `constexpr char FRAMEWORK_METRICS_PREFIX[]`. src/examples/inverse_offer_framework.cpp Lines 598 (patched) <https://reviews.apache.org/r/68812/#comment294613> How about pulling it into a constant, e.g., `constexpr char FRAMEWORK_NAME[]`? - Alexander Rukletsov On Sept. 23, 2018, 12:08 a.m., Till Toenshoff wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/68812/ > --- > > (Updated Sept. 23, 2018, 12:08 a.m.) > > > Review request for mesos and Joseph Wu. > > > Bugs: MESOS-5827 > https://issues.apache.org/jira/browse/MESOS-5827 > > > Repository: mesos > > > Description > --- > > Adds an example framework displaying how to handle inverse offers. > This example is based on the original review request > https://reviews.apache.org/r/50010/ by Joseph Wu. > Some changes were applied adding framework authentication > capabilites, updated PullGauge metrics and other minor adaptations > following the other example frameworks. > > > Diffs > - > > src/Makefile.am 5795c7097a9ed1f659e169ad81a9f2c09481aa81 > src/examples/inverse_offer_framework.cpp PRE-CREATION > > > Diff: https://reviews.apache.org/r/68812/diff/1/ > > > Testing > --- > > make check > > > Thanks, > > Till Toenshoff > >
Re: Review Request 69110: Added task health check definitions to master API responses.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/69110/#review209914 --- src/tests/health_check_tests.cpp Lines 460-466 (patched) <https://reviews.apache.org/r/69110/#comment294552> What do you think about moving this blob to `AgentAPITest::GetState`? It will be consistent with the `TASK_ADDED` test. - Alexander Rukletsov On Oct. 22, 2018, 6:37 p.m., Greg Mann wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/69110/ > --- > > (Updated Oct. 22, 2018, 6:37 p.m.) > > > Review request for mesos, Alexander Rukletsov, Gastón Kleiman, and Vinod Kone. > > > Bugs: MESOS-8780 > https://issues.apache.org/jira/browse/MESOS-8780 > > > Repository: mesos > > > Description > --- > > The Task protobuf message is updated to include the health check > definition of a task when it is specified. Associated helpers are > also updated along with a test which verifies that this field is > reflected in master API responses. > > > Diffs > - > > include/mesos/mesos.proto 72966da75bc90d4b5d891e209e786e5326155d17 > include/mesos/v1/mesos.proto c6c1dae5494d9270a7a43a2d838769574ae47872 > src/common/http.cpp 80848aafe27eb0c35222ad236e671a32c4ab10aa > src/common/protobuf_utils.cpp 77139d8a3931dc1e9e00fcea83d77d96244a34f3 > src/tests/api_tests.cpp c681b9b0b83f7472312083730a5433e3d6f0efc0 > src/tests/health_check_tests.cpp c972b9003196b4b4272d7c34311c84c117b1ae3d > > > Diff: https://reviews.apache.org/r/69110/diff/3/ > > > Testing > --- > > `make check` > > > Thanks, > > Greg Mann > >
Re: Review Request 69110: Added task health check definitions to master API responses.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/69110/#review209842 --- Fix it, then Ship it! Let's make sure we also add a test for the contents of `TASK_ADDED` soon and add expose check definitions as well. include/mesos/mesos.proto Lines 2200-2201 (patched) <https://reviews.apache.org/r/69110/#comment294455> Let's add a `TODO` here for `CheckInfo`. Here and below. - Alexander Rukletsov On Oct. 22, 2018, 5:35 a.m., Greg Mann wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/69110/ > --- > > (Updated Oct. 22, 2018, 5:35 a.m.) > > > Review request for mesos, Alexander Rukletsov, Gastón Kleiman, and Vinod Kone. > > > Bugs: MESOS-8780 > https://issues.apache.org/jira/browse/MESOS-8780 > > > Repository: mesos > > > Description > --- > > The Task protobuf message is updated to include the health check > definition of a task when it is specified. Associated helpers are > also updated along with a test which verifies that this field is > reflected in master API responses. > > > Diffs > - > > include/mesos/mesos.proto 72966da75bc90d4b5d891e209e786e5326155d17 > include/mesos/v1/mesos.proto c6c1dae5494d9270a7a43a2d838769574ae47872 > src/common/http.cpp 80848aafe27eb0c35222ad236e671a32c4ab10aa > src/common/protobuf_utils.cpp 77139d8a3931dc1e9e00fcea83d77d96244a34f3 > src/tests/health_check_tests.cpp c972b9003196b4b4272d7c34311c84c117b1ae3d > > > Diff: https://reviews.apache.org/r/69110/diff/2/ > > > Testing > --- > > `make check` > > > Thanks, > > Greg Mann > >
Re: Review Request 68993: Introduced `logResponse` for http handlers.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/68993/ --- (Updated Oct. 16, 2018, 11:57 a.m.) Review request for mesos, Benjamin Hindman, Benno Evers, Benjamin Mahler, Gastón Kleiman, and Till Toenshoff. Repository: mesos Description --- Currently we use `logRequest` function to log requests to endpoints in Mesos `MasterProcess`, `SlaveProcess`, and `FilesProcess`. Each request is logged when it is first fetched from the actor mailbox. However this obviously does not allow for logging the request processing time. This patch introduces a `logResponse` function which logs the request together with the corresponding response status and the time spent generating the response. Diffs (updated) - src/common/http.hpp 4f994a0744f098363327b785df56e877c9624e2a src/common/http.cpp 9070071bbb42703b3f62e0cf50b31da943da015c Diff: https://reviews.apache.org/r/68993/diff/2/ Changes: https://reviews.apache.org/r/68993/diff/1-2/ Testing --- See https://reviews.apache.org/r/68994/ Thanks, Alexander Rukletsov
Re: Review Request 68994: Logged request processing time for some endpoints.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/68994/ --- (Updated Oct. 16, 2018, 11:57 a.m.) Review request for mesos, Benjamin Hindman, Benno Evers, Benjamin Mahler, Gastón Kleiman, and Till Toenshoff. Repository: mesos Description --- This patch leverages `logResponse()` function to print response status code together with the request processing time for some endpoints on the master and agent. Not all endpoints are participating to avoid unnecessary log pollution; only these that are know to be "slow" in generating the response. Note that requests are still logged when they are fetched from the actor mailbox, i.e., affected endpoints now log twice per request: when the processing starts and when the response is ready to be sent. Diffs (updated) - src/master/master.cpp 06d769aeba16586a020729d454f4d00688b78c78 src/slave/slave.cpp 9d84dcb52e5b7e507fc375f184c5f77be08d70a2 Diff: https://reviews.apache.org/r/68994/diff/2/ Changes: https://reviews.apache.org/r/68994/diff/1-2/ Testing --- `make check` on Mac OS 10.13.6 and various Linux distros. Manual testing == Started a mesos-master process locally: `./bin/mesos-master.sh --work_dir=m` 1. `/state` request: `http http://192.168.1.3:5050/state` Relevant snippet from the master log: ``` I1011 16:07:05.978631 10211328 http.cpp:1178] HTTP GET for /master/state from 192.168.1.3:56510 with User-Agent='HTTPie/0.9.3' I1011 16:07:05.981534 13430784 http.cpp:1195] HTTP GET for /master/state from 192.168.1.3:56510: '200 OK' after 3.260928ms ``` 2. `/flags` request: `http http://192.168.1.3:5050/flags` Relevant snippet from the master log: ``` I1011 16:08:51.012370 63365120 http.cpp:1178] HTTP GET for /master/flags from 192.168.1.3:56629 with User-Agent='HTTPie/0.9.3' ``` Thanks, Alexander Rukletsov
Re: Review Request 68992: Added 'received' timestamp into `process::Request`.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/68992/ --- (Updated Oct. 16, 2018, 11:56 a.m.) Review request for mesos, Benjamin Hindman, Benno Evers, Benjamin Mahler, Gastón Kleiman, and Till Toenshoff. Repository: mesos Description --- This allows us to note when a request comes in and hence calculate request processing time. Diffs (updated) - 3rdparty/libprocess/include/process/http.hpp bbcd0bac8bab51da2dae6c052896d11a86753744 Diff: https://reviews.apache.org/r/68992/diff/2/ Changes: https://reviews.apache.org/r/68992/diff/1-2/ Testing --- See https://reviews.apache.org/r/68994/ Thanks, Alexander Rukletsov
Re: Review Request 68992: Added 'received' timestamp into `process::Request`.
> On Oct. 16, 2018, 11:41 a.m., Till Toenshoff wrote: > > 3rdparty/libprocess/include/process/http.hpp > > Lines 568-569 (patched) > > <https://reviews.apache.org/r/68992/diff/1/?file=2096383#file2096383line568> > > > > As discussed out of band, let's come up with a proper story here in a > > JIRA first and then get back to it. Agreed — this suggests one possible solution, I'll remove the TODO. - Alexander --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/68992/#review209605 --- On Oct. 11, 2018, 2:16 p.m., Alexander Rukletsov wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/68992/ > --- > > (Updated Oct. 11, 2018, 2:16 p.m.) > > > Review request for mesos, Benjamin Hindman, Benno Evers, Benjamin Mahler, > Gastón Kleiman, and Till Toenshoff. > > > Repository: mesos > > > Description > --- > > This allows us to note when a request comes in and > hence calculate request processing time. > > > Diffs > - > > 3rdparty/libprocess/include/process/http.hpp > bbcd0bac8bab51da2dae6c052896d11a86753744 > > > Diff: https://reviews.apache.org/r/68992/diff/1/ > > > Testing > --- > > See https://reviews.apache.org/r/68994/ > > > Thanks, > > Alexander Rukletsov > >
Re: Review Request 67840: Renamed committer checklist into committer guidelines.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/67840/ --- (Updated Oct. 15, 2018, 5:43 p.m.) Review request for mesos, Gastón Kleiman and James Peach. Repository: mesos Description --- Renamed committer checklist into committer guidelines. Diffs (updated) - docs/committer-candidate-checklist.md bf67473445d135800d5b54587f2fb5513ca6b9e4 docs/committers.md 67034dcaba94b61433464558d5a5876e43c7bbaf docs/home.md 029b8cd1f5b1c6403aa653b496f6a7067d1bc053 Diff: https://reviews.apache.org/r/67840/diff/2/ Changes: https://reviews.apache.org/r/67840/diff/1-2/ Testing --- None: not a functional change. Thanks, Alexander Rukletsov
Re: Review Request 67841: Updated committer candidate guidelines.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/67841/ --- (Updated Oct. 15, 2018, 5:43 p.m.) Review request for mesos, Gastón Kleiman and James Peach. Repository: mesos Description --- Updated committer candidate guidelines. Diffs (updated) - docs/committer-candidate-guidelines.md bf67473445d135800d5b54587f2fb5513ca6b9e4 Diff: https://reviews.apache.org/r/67841/diff/2/ Changes: https://reviews.apache.org/r/67841/diff/1-2/ Testing --- None: not a functional change. Thanks, Alexander Rukletsov
Review Request 68993: Introduced `logResponse` for http handlers.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/68993/ --- Review request for mesos, Benjamin Hindman, Benno Evers, Benjamin Mahler, Gastón Kleiman, and Till Toenshoff. Repository: mesos Description --- Currently we use `logRequest` function to log requests to endpoints in Mesos `MasterProcess`, `SlaveProcess`, and `FilesProcess`. Each request is logged when it is first fetched from the actor mailbox. However this obviously does not allow for logging the request processing time. This patch introduces a `logResponse` function which logs the request together with the corresponding response status and the time spent generating the response. Diffs - src/common/http.hpp 4f994a0744f098363327b785df56e877c9624e2a src/common/http.cpp 9070071bbb42703b3f62e0cf50b31da943da015c Diff: https://reviews.apache.org/r/68993/diff/1/ Testing --- See https://reviews.apache.org/r/68994/ Thanks, Alexander Rukletsov
Review Request 68994: Logged request processing time for some endpoints.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/68994/ --- Review request for mesos, Benjamin Hindman, Benno Evers, Benjamin Mahler, Gastón Kleiman, and Till Toenshoff. Repository: mesos Description --- This patch leverages `logResponse()` function to print response status code together with the request processing time for some endpoints on the master and agent. Not all endpoints are participating to avoid unnecessary log pollution; only these that are know to be "slow" in generating the response. Note that requests are still logged when they are fetched from the actor mailbox, i.e., affected endpoints now log twice per request: when the processing starts and when the response is ready to be sent. Diffs - src/master/master.cpp 06d769aeba16586a020729d454f4d00688b78c78 src/slave/slave.cpp 9d84dcb52e5b7e507fc375f184c5f77be08d70a2 Diff: https://reviews.apache.org/r/68994/diff/1/ Testing --- `make check` on Mac OS 10.13.6 and various Linux distros. Manual testing == Started a mesos-master process locally: `./bin/mesos-master.sh --work_dir=m` 1. `/state` request: `http http://192.168.1.3:5050/state` Relevant snippet from the master log: ``` I1011 16:07:05.978631 10211328 http.cpp:1178] HTTP GET for /master/state from 192.168.1.3:56510 with User-Agent='HTTPie/0.9.3' I1011 16:07:05.981534 13430784 http.cpp:1195] HTTP GET for /master/state from 192.168.1.3:56510: '200 OK' after 3.260928ms ``` 2. `/flags` request: `http http://192.168.1.3:5050/flags` Relevant snippet from the master log: ``` I1011 16:08:51.012370 63365120 http.cpp:1178] HTTP GET for /master/flags from 192.168.1.3:56629 with User-Agent='HTTPie/0.9.3' ``` Thanks, Alexander Rukletsov
Review Request 68992: Added 'received' timestamp into `process::Request`.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/68992/ --- Review request for mesos, Benjamin Hindman, Benno Evers, Benjamin Mahler, Gastón Kleiman, and Till Toenshoff. Repository: mesos Description --- This allows us to note when a request comes in and hence calculate request processing time. Diffs - 3rdparty/libprocess/include/process/http.hpp bbcd0bac8bab51da2dae6c052896d11a86753744 Diff: https://reviews.apache.org/r/68992/diff/1/ Testing --- See https://reviews.apache.org/r/68994/ Thanks, Alexander Rukletsov
Re: Review Request 61818: Adjusted a comment and a log message around container termination.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/61818/ --- (Updated Oct. 9, 2018, 3:50 p.m.) Review request for mesos, Gilbert Song, Ian Downes, Jie Yu, and Vinod Kone. Repository: mesos Description --- When the container launch is aborted and the container is cleaned up in the containerizer before `executorLaunched()` is invoked, `containerizer->wait()` called in `executorLaunched()` does not hold a termination object. This case should not be logged as an error. Diffs - src/slave/slave.cpp 50d2a10cd68f6611efd4e691e5325e6e0c06f33a Diff: https://reviews.apache.org/r/61818/diff/1/ Testing --- None: not a functional change. Thanks, Alexander Rukletsov
Re: Review Request 68945: Fused constructors of `MethodNotAllowed` into one.
> On Oct. 8, 2018, 10:49 a.m., Benno Evers wrote:
> > 3rdparty/libprocess/include/process/http.hpp
> > Lines 777 (patched)
> > <https://reviews.apache.org/r/68945/diff/2/?file=2095314#file2095314line784>
> >
> > I'd probably consider using `strings::format()` here.
It is indeed a bit nicer, but I'd say the readability benefit does not outweigh
the cost of including an extra header.
```
"405 Method Not Allowed. Expecting one of { '" +
strings::join("', '", allowedMethods) + "' }" +
(requestMethod.isSome()
? ", but received '" + requestMethod.get() + "'"
: "") +
".";
```
vs.
```
strings::format(
"405 Method Not Allowed. Expecting one of { '%s' }%s.",
strings::join("', '", allowedMethods),
requestMethod.isSome()
? ", but received '" + requestMethod.get() + "'"
: "");
```
- Alexander
-------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/68945/#review209305
---
On Oct. 8, 2018, 7:50 a.m., Alexander Rukletsov wrote:
>
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/68945/
> ---
>
> (Updated Oct. 8, 2018, 7:50 a.m.)
>
>
> Review request for mesos and Benno Evers.
>
>
> Repository: mesos
>
>
> Description
> ---
>
> There is no good reason to provide two c-tors for `MethodNotAllowed`,
> with one taking `requestMethod` and one not. Instead, an `Option<>`
> can be used. This also removes the need for copy-paste in the c-tor
> body.
>
>
> Diffs
> -
>
> 3rdparty/libprocess/include/process/http.hpp
> cef511a1563a26e170ce7f4a49de12776b4512e7
>
>
> Diff: https://reviews.apache.org/r/68945/diff/2/
>
>
> Testing
> ---
>
> `make check` on various linux distros
>
>
> Thanks,
>
> Alexander Rukletsov
>
>
Re: Review Request 68945: Fused constructors of `MethodNotAllowed` into one.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/68945/ --- (Updated Oct. 8, 2018, 7:50 a.m.) Review request for mesos and Benno Evers. Repository: mesos Description --- There is no good reason to provide two c-tors for `MethodNotAllowed`, with one taking `requestMethod` and one not. Instead, an `Option<>` can be used. This also removes the need for copy-paste in the c-tor body. Diffs (updated) - 3rdparty/libprocess/include/process/http.hpp cef511a1563a26e170ce7f4a49de12776b4512e7 Diff: https://reviews.apache.org/r/68945/diff/2/ Changes: https://reviews.apache.org/r/68945/diff/1-2/ Testing --- `make check` on various linux distros Thanks, Alexander Rukletsov
Review Request 68944: Used delegating constructors in `Response` types.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/68944/ --- Review request for mesos and Benno Evers. Repository: mesos Description --- For clarity and brevity, use delegating constructors (available since C++11) in descendants of the `Response` class. Diffs - 3rdparty/libprocess/include/process/http.hpp cef511a1563a26e170ce7f4a49de12776b4512e7 Diff: https://reviews.apache.org/r/68944/diff/1/ Testing --- See https://reviews.apache.org/r/68945/ Thanks, Alexander Rukletsov
Review Request 68945: Fused constructors of `MethodNotAllowed` into one.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/68945/ --- Review request for mesos and Benno Evers. Repository: mesos Description --- There is no good reason to provide two c-tors for `MethodNotAllowed`, with one taking `requestMethod` and one not. Instead, an `Option<>` can be used. This also removes the need for copy-paste in the c-tor body. Diffs - 3rdparty/libprocess/include/process/http.hpp cef511a1563a26e170ce7f4a49de12776b4512e7 Diff: https://reviews.apache.org/r/68945/diff/1/ Testing --- `make check` on various linux distros Thanks, Alexander Rukletsov
Re: Review Request 67414: Added default message bodies to libprocess HTTP error responses.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/67414/#review209290 --- Fix it, then Ship it! Thanks! I'll fix the outstanding issues and commit shorty. 3rdparty/libprocess/include/process/http.hpp Lines 778-779 (original), 778-779 (patched) <https://reviews.apache.org/r/67414/#comment293581> It makes sense to add the same text here, right? 3rdparty/libprocess/include/process/http.hpp Lines 820-831 (original), 824-838 (patched) <https://reviews.apache.org/r/67414/#comment293580> Even though both are apparently allowed by our style guide, I'd go for local consistency. - Alexander Rukletsov On Oct. 5, 2018, 4:45 p.m., Benno Evers wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/67414/ > --- > > (Updated Oct. 5, 2018, 4:45 p.m.) > > > Review request for mesos and Alexander Rukletsov. > > > Bugs: MESOS-8999 > https://issues.apache.org/jira/browse/MESOS-8999 > > > Repository: mesos > > > Description > --- > > By default on error libprocess would only return a response > with the correct status code and no response body. > > However, most browsers do not visually indicate the response > status code, making it hard for the user to figure out what > exactly the problem was. > > A rudimentary text-only body is added to the default body of HTTP > error responses, mirroring the approach taken by e.g. the Go > standard library or Python's http.server module. > > > Diffs > - > > 3rdparty/libprocess/include/process/http.hpp > fa66b0fe2ed72e218b235ac4b697a2a294027527 > > > Diff: https://reviews.apache.org/r/67414/diff/2/ > > > Testing > --- > > Started `make check` on various different platforms (Run #4563) > > > Thanks, > > Benno Evers > >
Re: Review Request 67414: Added default message bodies to libprocess HTTP error responses.
> On June 5, 2018, 10:46 a.m., Alexander Rukletsov wrote:
> > 3rdparty/libprocess/include/process/http.hpp
> > Line 701 (original), 701-702 (patched)
> > <https://reviews.apache.org/r/67414/diff/1/?file=2034492#file2034492line701>
> >
> > Why not using `process::http::Status::string()`
> > https://github.com/apache/mesos/blob/2198b961d24b788564d36490cf52f78d7ec07655/3rdparty/libprocess/src/http.cpp#L176-L180
> > instead?
> >
> > Maybe then you can even put a single line into `Response` c-tor instead
> > of modifying individual classes.
>
> Benno Evers wrote:
> I didn't know about this function, but looking at it it seems to be in
> direct violation of our style guide at
> (https://google.github.io/styleguide/cppguide.html#Static_and_Global_Variables)
> so we should probably look into deprecating it rather than promoting further
> usage.
>
> But even w/o that issue, I'm not sure that
>
> ```
> BadRequest()
> : Response(Status::string(Status::BAD_REQUEST), Status::BAD_REQUEST)
> {}
> ```
>
> is better than
>
> ```
> BadRequest()
> : Response("400 Bad Request.", Status::BAD_REQUEST) {}
> ```
>
>
> I dont think putting it in the default constructor of `Response` is a
> good idea, because we more or less endorse constructing responses like this
>
> ```
> process::http::OK response;
> response.type = response.PATH;
> response.path = path;
> response.headers["Content-Type"] = "application/octet-stream";
> response.headers["Content-Disposition"] =
> strings::format("attachment; filename=%s", path).get();
> ```
>
> and we would need to audit all usages of such responses to ensure they're
> not confused by suddenly having an additional body.
>
> Alexander Rukletsov wrote:
> What I don't like about the current approach is the duplication: we
> already have `code -> string` mapping in http.cpp, it would be good to reuse
> it here so that they don't go out of sync.
>
> Benno Evers wrote:
> I guess I don't really see why it would be a problem if the two go out of
> sync - many pages have very elaborate 404 error pages including images,
> animations and lots of text, so I don't think any tool will have the
> assumption that the response body equals the stringified HTTP status code.
Discussed with Benno offline. I'm convinced that the string does not have to be
auto generated, so I'm dropping the issue.
- Alexander
---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/67414/#review204313
---
On June 1, 2018, 3:14 p.m., Benno Evers wrote:
>
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/67414/
> ---
>
> (Updated June 1, 2018, 3:14 p.m.)
>
>
> Review request for mesos.
>
>
> Bugs: MESOS-8999
> https://issues.apache.org/jira/browse/MESOS-8999
>
>
> Repository: mesos
>
>
> Description
> ---
>
> By default on error libprocess would only return a response
> with the correct status code and no response body.
>
> However, most browsers do not visually indicate the response
> status code, making it hard for the user to figure out what
> exactly the problem was.
>
>
> Diffs
> -
>
> 3rdparty/libprocess/include/process/http.hpp
> 055447e13117c0a3ba79d0fc326ece657e8f064f
>
>
> Diff: https://reviews.apache.org/r/67414/diff/1/
>
>
> Testing
> ---
>
>
> Thanks,
>
> Benno Evers
>
>
Re: Review Request 68866: Waited for TEARDOWN response in v1 Java scheduler shim.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/68866/ --- (Updated Oct. 4, 2018, 10:02 a.m.) Review request for mesos, Gastón Kleiman, Greg Mann, and Nicholas Parker. Bugs: MESOS-9274 https://issues.apache.org/jira/browse/MESOS-9274 Repository: mesos Description --- Destruction of the library is not always under our control. For example, the JVM can call JNI `finalize()` if the Java scheduler nullifies its reference to the V1Mesos library immediately after sending `TEARDOWN`. We want to make sure that `TEARDOWN` is sent before the Mesos library is destructed. Diffs (updated) - src/java/jni/org_apache_mesos_v1_scheduler_V1Mesos.cpp 2a5fbd79ac7bad933067cd96e38186849af8edc4 Diff: https://reviews.apache.org/r/68866/diff/4/ Changes: https://reviews.apache.org/r/68866/diff/3-4/ Testing --- `make check` on various Linux distro Thanks, Alexander Rukletsov
Re: Review Request 68865: Put `TerminateEvent` at the end of the queue in the Mesos library.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/68865/ --- (Updated Oct. 4, 2018, 9:38 a.m.) Review request for mesos, Gastón Kleiman, Greg Mann, and Nicholas Parker. Changes --- Addressed comments. Bugs: MESOS-9274 https://issues.apache.org/jira/browse/MESOS-9274 Repository: mesos Description --- This is to ensure all pending dispatches are processed. However multistage events, e.g., `Call`, might still be dropped, because a continuation (stage) of such event can be dispatched after the termination event. Diffs (updated) - src/scheduler/scheduler.cpp 471152945d6af660c8983324b38702d872657f89 Diff: https://reviews.apache.org/r/68865/diff/2/ Changes: https://reviews.apache.org/r/68865/diff/1-2/ Testing --- See https://reviews.apache.org/r/68866/ Thanks, Alexander Rukletsov
Re: Review Request 68866: Waited for TEARDOWN response in v1 Java scheduler shim.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/68866/ --- (Updated Oct. 4, 2018, 9:42 a.m.) Review request for mesos, Gastón Kleiman, Greg Mann, and Nicholas Parker. Bugs: MESOS-9274 https://issues.apache.org/jira/browse/MESOS-9274 Repository: mesos Description --- Destruction of the library is not always under our control. For example, the JVM can call JNI `finalize()` if the Java scheduler nullifies its reference to the V1Mesos library immediately after sending `TEARDOWN`. We want to make sure that `TEARDOWN` is sent before the Mesos library is destructed. Diffs (updated) - src/java/jni/org_apache_mesos_v1_scheduler_V1Mesos.cpp 2a5fbd79ac7bad933067cd96e38186849af8edc4 Diff: https://reviews.apache.org/r/68866/diff/3/ Changes: https://reviews.apache.org/r/68866/diff/2-3/ Testing --- `make check` on various Linux distro Thanks, Alexander Rukletsov
Re: Review Request 68866: Waited for TEARDOWN response in v1 Java scheduler shim.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/68866/ --- (Updated Oct. 4, 2018, 9:39 a.m.) Review request for mesos, Gastón Kleiman, Greg Mann, and Nicholas Parker. Changes --- Addressed feedback. Bugs: MESOS-9274 https://issues.apache.org/jira/browse/MESOS-9274 Repository: mesos Description --- Destruction of the library is not always under our control. For example, the JVM can call JNI `finalize()` if the Java scheduler nullifies its reference to the V1Mesos library immediately after sending `TEARDOWN`. We want to make sure that `TEARDOWN` is sent before the Mesos library is destructed. Diffs (updated) - src/java/jni/org_apache_mesos_v1_scheduler_V1Mesos.cpp 2a5fbd79ac7bad933067cd96e38186849af8edc4 Diff: https://reviews.apache.org/r/68866/diff/2/ Changes: https://reviews.apache.org/r/68866/diff/1-2/ Testing (updated) --- `make check` on various Linux distro Thanks, Alexander Rukletsov
Re: Review Request 68866: Waited for TEARDOWN response in v1 Java scheduler shim.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/68866/ --- (Updated Oct. 4, 2018, 9:33 a.m.) Review request for mesos, Gastón Kleiman, Greg Mann, and Nicholas Parker. Bugs: MESOS-9274 https://issues.apache.org/jira/browse/MESOS-9274 Repository: mesos Description (updated) --- Destruction of the library is not always under our control. For example, the JVM can call JNI `finalize()` if the Java scheduler nullifies its reference to the V1Mesos library immediately after sending `TEARDOWN`. We want to make sure that `TEARDOWN` is sent before the Mesos library is destructed. Diffs - src/java/jni/org_apache_mesos_v1_scheduler_V1Mesos.cpp 2a5fbd79ac7bad933067cd96e38186849af8edc4 Diff: https://reviews.apache.org/r/68866/diff/1/ Testing --- `make check`on various Linux distro Thanks, Alexander Rukletsov
Re: Review Request 68865: Put `TerminateEvent` at the end of the queue in the Mesos library.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/68865/ --- (Updated Oct. 4, 2018, 9:07 a.m.) Review request for mesos, Gastón Kleiman, Greg Mann, and Nicholas Parker. Bugs: MESOS-9274 https://issues.apache.org/jira/browse/MESOS-9274 Repository: mesos Description (updated) --- This is to ensure all pending dispatches are processed. However multistage events, e.g., `Call`, might still be dropped, because a continuation (stage) of such event can be dispatched after the termination event. Diffs - src/scheduler/scheduler.cpp 471152945d6af660c8983324b38702d872657f89 Diff: https://reviews.apache.org/r/68865/diff/1/ Testing --- See https://reviews.apache.org/r/68866/ Thanks, Alexander Rukletsov
Re: Review Request 68903: Avoid deadlock-prone blocking in master's parallel endpoint serving.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/68903/#review209216 --- src/master/http.cpp Lines 2433-2440 (patched) <https://reviews.apache.org/r/68903/#comment293537> The interesting scenario is when the only available working thread is the one currently attached to the master actor. In this scenario, is the expectation here that each `process::async(worker)` will be spawned and executed on this single worker thread before master actor proceeds to launching the last worker? - Alexander Rukletsov On Oct. 3, 2018, 12:07 a.m., Benjamin Mahler wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/68903/ > --- > > (Updated Oct. 3, 2018, 12:07 a.m.) > > > Review request for mesos, Alexander Rukletsov and Benno Evers. > > > Repository: mesos > > > Description > --- > > The approach taken to serve endpoints in parallel was to call > `process::async` for each request and block until they finish. > Blocking in this case assumes that there will be enough additional > non-blocked worker threads to execute all the requests. When the > number of blocking Processes is equal or greater than the number > of worker threads, deadlock become possible. The parallel endpoint > serving approach added another blocking Process. > > While we could make blocking safe (see MESOS-8256) by, for example, > detaching a worker thread prior to blocking (a la golang), such a > change is more involved. > > In lieu of making blocking safe, this patch updates the Master to > block only when it's guaranteed to be safe. This is accomplished > by: > > (1) using a "work queue" (just a shared atomic "next index") and > having workers pop items from this "queue", > (2) we execute one of the workers on the master Process directly, > (3) when this worker completes, we then know that all requests > are either processed or being processed (i.e. the `async` > `Process` is executing on a worker thread), > (4) the master can then block on a count down latch, knowing > that it will unblock because of (3). > > > Diffs > - > > src/master/http.cpp d912789bcfdcf6eaefc29dba7b878fa4e02f9276 > > > Diff: https://reviews.apache.org/r/68903/diff/1/ > > > Testing > --- > > > Thanks, > > Benjamin Mahler > >
Re: Review Request 68866: Waited for TEARDOWN response in v1 Java scheduler shim.
> On Sept. 27, 2018, 6:23 p.m., Vinod Kone wrote: > > src/java/jni/org_apache_mesos_v1_scheduler_V1Mesos.cpp > > Lines 294 (patched) > > <https://reviews.apache.org/r/68866/diff/1/?file=2092453#file2092453line294> > > > > Looks like you are doing `call` here instead of `send` so that you have > > a future to wait on? Skipping `send` which does validation and > > authentication seems wrong. Let's not do that. > > > > I would recommend putting a sleep in the client code instead of here > > for now. But `call` does [validation](https://github.com/apache/mesos/blob/6e21e94ddca5b776d44636fe3eba8500bf88dc25/src/scheduler/scheduler.cpp#L269-L270) and [authentication](https://github.com/apache/mesos/blob/6e21e94ddca5b776d44636fe3eba8500bf88dc25/src/scheduler/scheduler.cpp#L301) too. Not sure I follow, Vinod. - Alexander --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/68866/#review209074 ------- On Sept. 27, 2018, 5:41 p.m., Alexander Rukletsov wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/68866/ > --- > > (Updated Sept. 27, 2018, 5:41 p.m.) > > > Review request for mesos, Gastón Kleiman, Greg Mann, and Nicholas Parker. > > > Bugs: MESOS-9274 > https://issues.apache.org/jira/browse/MESOS-9274 > > > Repository: mesos > > > Description > --- > > See summary. > > > Diffs > - > > src/java/jni/org_apache_mesos_v1_scheduler_V1Mesos.cpp > 2a5fbd79ac7bad933067cd96e38186849af8edc4 > > > Diff: https://reviews.apache.org/r/68866/diff/1/ > > > Testing > --- > > `make check`on various Linux distro > > > Thanks, > > Alexander Rukletsov > >
Re: Review Request 68866: Waited for TEARDOWN response in v1 Java scheduler shim.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/68866/ --- (Updated Sept. 27, 2018, 5:41 p.m.) Review request for mesos, Greg Mann and Nicholas Parker. Bugs: MESOS-9274 https://issues.apache.org/jira/browse/MESOS-9274 Repository: mesos Description --- See summary. Diffs - src/java/jni/org_apache_mesos_v1_scheduler_V1Mesos.cpp 2a5fbd79ac7bad933067cd96e38186849af8edc4 Diff: https://reviews.apache.org/r/68866/diff/1/ Testing (updated) --- `make check`on various Linux distro Thanks, Alexander Rukletsov
Re: Review Request 68865: Put `TerminateEvent` at the end of the queue in the Mesos library.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/68865/ --- (Updated Sept. 27, 2018, 5:41 p.m.) Review request for mesos, Gastón Kleiman, Greg Mann, and Nicholas Parker. Bugs: MESOS-9274 https://issues.apache.org/jira/browse/MESOS-9274 Repository: mesos Description --- See summary. Diffs - src/scheduler/scheduler.cpp 471152945d6af660c8983324b38702d872657f89 Diff: https://reviews.apache.org/r/68865/diff/1/ Testing (updated) --- See https://reviews.apache.org/r/68866/ Thanks, Alexander Rukletsov
Review Request 68866: Waited for TEARDOWN response in v1 Java scheduler shim.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/68866/ --- Review request for mesos, Greg Mann and Nicholas Parker. Bugs: MESOS-9274 https://issues.apache.org/jira/browse/MESOS-9274 Repository: mesos Description --- See summary. Diffs - src/java/jni/org_apache_mesos_v1_scheduler_V1Mesos.cpp 2a5fbd79ac7bad933067cd96e38186849af8edc4 Diff: https://reviews.apache.org/r/68866/diff/1/ Testing --- Thanks, Alexander Rukletsov
Review Request 68865: Put `TerminateEvent` at the end of the queue in the Mesos library.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/68865/ --- Review request for mesos, Greg Mann and Nicholas Parker. Bugs: MESOS-9274 https://issues.apache.org/jira/browse/MESOS-9274 Repository: mesos Description --- See summary. Diffs - src/scheduler/scheduler.cpp 471152945d6af660c8983324b38702d872657f89 Diff: https://reviews.apache.org/r/68865/diff/1/ Testing --- Thanks, Alexander Rukletsov
Re: Review Request 68839: Disabled flaky LaunchNestedContainerSessionsInParallel test.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/68839/#review209050 --- Ship it! Ship It! - Alexander Rukletsov On Sept. 25, 2018, 3:36 p.m., Andrei Budnik wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/68839/ > --- > > (Updated Sept. 25, 2018, 3:36 p.m.) > > > Review request for mesos and Alexander Rukletsov. > > > Bugs: MESOS-9257 > https://issues.apache.org/jira/browse/MESOS-9257 > > > Repository: mesos > > > Description > --- > > See summary. > > > Diffs > - > > src/tests/api_tests.cpp b3fec4e6e184a635f7de676b8f35fa3ec5900284 > > > Diff: https://reviews.apache.org/r/68839/diff/1/ > > > Testing > --- > > > Thanks, > > Andrei Budnik > >
Re: Review Request 68784: Fixed broken pipe error in IOSwitchboard.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/68784/#review208854 --- src/slave/containerizer/mesos/io/switchboard.cpp Lines 1757-1761 (patched) <https://reviews.apache.org/r/68784/#comment293089> Thanks, Andrei! An advice I'd like to share: prefer explicit capture. In this case, when something changes in the code flow, the compiler notifies you happily : ) - Alexander Rukletsov On Sept. 21, 2018, 12:12 p.m., Andrei Budnik wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/68784/ > --- > > (Updated Sept. 21, 2018, 12:12 p.m.) > > > Review request for mesos and Alexander Rukletsov. > > > Bugs: MESOS-8545 > https://issues.apache.org/jira/browse/MESOS-8545 > > > Repository: mesos > > > Description > --- > > Previous attempt to fix `HTTP 500` "broken pipe" in review /r/62187/ > was not correct: after IOSwitchboard sends a response to the agent for > the `ATTACH_CONTAINER_INPUT` call, the socket is closed immediately, > thus causing the error on the agent. This patch adds a delay after > IO redirects are finished and before IOSwitchboard forcibly send a > response. > > > Diffs > - > > src/slave/containerizer/mesos/io/switchboard.cpp > 5bb21e7c2bf331c44a097ce8f83d8ca7e967332a > > > Diff: https://reviews.apache.org/r/68784/diff/2/ > > > Testing > --- > > 1. sudo make check (Fedora 25) > 2. internal CI > > > Thanks, > > Andrei Budnik > >
Re: Review Request 68768: Fixed disconnection while sending acknowledgment to IOSwitchboard.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/68768/#review208853 --- Ship it! Ship It! - Alexander Rukletsov On Sept. 19, 2018, 4:14 p.m., Andrei Budnik wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/68768/ > --- > > (Updated Sept. 19, 2018, 4:14 p.m.) > > > Review request for mesos and Alexander Rukletsov. > > > Bugs: MESOS-8545 > https://issues.apache.org/jira/browse/MESOS-8545 > > > Repository: mesos > > > Description > --- > > Previously, an HTTP connection to the IOSwitchboard could be garbage > collected before the agent sent an acknowledgment to the IOSwitchboard > via this connection. This patch fixes the issue by keeping a reference > count to the connection in a lambda callback until disconnection > occurs. > > > Diffs > - > > src/slave/http.cpp bf34ef75f8f25cf895d1c4d52923b2d6254f1ecc > > > Diff: https://reviews.apache.org/r/68768/diff/1/ > > > Testing > --- > > 1. sudo make check (Fedora 25) > 2. internal CI > > > Thanks, > > Andrei Budnik > >
Re: Review Request 68784: Fixed broken pipe error in IOSwitchboard.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/68784/#review208852 --- Ship it! Ship It! - Alexander Rukletsov On Sept. 21, 2018, 12:12 p.m., Andrei Budnik wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/68784/ > --- > > (Updated Sept. 21, 2018, 12:12 p.m.) > > > Review request for mesos and Alexander Rukletsov. > > > Bugs: MESOS-8545 > https://issues.apache.org/jira/browse/MESOS-8545 > > > Repository: mesos > > > Description > --- > > Previous attempt to fix `HTTP 500` "broken pipe" in review /r/62187/ > was not correct: after IOSwitchboard sends a response to the agent for > the `ATTACH_CONTAINER_INPUT` call, the socket is closed immediately, > thus causing the error on the agent. This patch adds a delay after > IO redirects are finished and before IOSwitchboard forcibly send a > response. > > > Diffs > - > > src/slave/containerizer/mesos/io/switchboard.cpp > 5bb21e7c2bf331c44a097ce8f83d8ca7e967332a > > > Diff: https://reviews.apache.org/r/68784/diff/2/ > > > Testing > --- > > 1. sudo make check (Fedora 25) > 2. internal CI > > > Thanks, > > Andrei Budnik > >
Re: Review Request 62187: Fixed broken pipe error in IOSwitchboard.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/62187/#review208704 --- Ship it! Ship It! - Alexander Rukletsov On Sept. 10, 2018, 6 p.m., Andrei Budnik wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/62187/ > --- > > (Updated Sept. 10, 2018, 6 p.m.) > > > Review request for mesos, Andrei Budnik and Alexander Rukletsov. > > > Bugs: MESOS-8545 > https://issues.apache.org/jira/browse/MESOS-8545 > > > Repository: mesos > > > Description > --- > > We force IOSwitchboard to return a final response to the client for the > `ATTACH_CONTAINER_INPUT` call after IO redirects are finished. In this > case, we don't read remaining messages from the input stream. So the > agent might send an acknowledgment for the request before IOSwitchboard > has received remaining messages. We need to delay termination of > IOSwitchboard to give it a chance to read the remaining messages. > Otherwise, the agent might get `HTTP 500` "broken pipe" while > attempting to write the final message. > > > Diffs > - > > src/slave/containerizer/mesos/io/switchboard.cpp > 52b0e521ed1c651c90b3a3df7c4df576288bf400 > > > Diff: https://reviews.apache.org/r/62187/diff/5/ > > > Testing > --- > > internal CI (3x times) > > > Thanks, > > Andrei Budnik > >
Re: Review Request 65168: Fixed HTTP errors caused by dropped HTTP responses by IOSwitchboard.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/65168/#review208703 --- Fix it, then Ship it! src/slave/containerizer/mesos/io/switchboard.cpp Lines 1379-1389 (patched) <https://reviews.apache.org/r/65168/#comment292858> Let's extract it into a separate method src/slave/http.cpp Lines 3138 (patched) <https://reviews.apache.org/r/65168/#comment292855> s/Since/After src/slave/http.cpp Lines 3157 (patched) <https://reviews.apache.org/r/65168/#comment292857> please `snake_case` instead of `camelCase` - Alexander Rukletsov On Sept. 6, 2018, 3:22 p.m., Andrei Budnik wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/65168/ > --- > > (Updated Sept. 6, 2018, 3:22 p.m.) > > > Review request for mesos, Alexander Rukletsov and Qian Zhang. > > > Bugs: MESOS-8545 > https://issues.apache.org/jira/browse/MESOS-8545 > > > Repository: mesos > > > Description > --- > > Previously, IOSwitchboard process could terminate before all HTTP > responses had been sent to the agent. In the case of > `ATTACH_CONTAINER_INPUT` call, we could drop a final HTTP `200 OK` > response, so the agent got broken HTTP connection for the call. > This patch introduces an acknowledgment for the received response > for the `ATTACH_CONTAINER_INPUT` call. This acknowledgment is a new > type of control messages for the `ATTACH_CONTAINER_INPUT` call. When > IOSwitchboard receives an acknowledgment, and io redirects are > finished, it terminates itself. That guarantees that the agent always > receives a response for the `ATTACH_CONTAINER_INPUT` call. > > > Diffs > - > > src/slave/containerizer/mesos/io/switchboard.cpp > 52b0e521ed1c651c90b3a3df7c4df576288bf400 > src/slave/http.hpp 78200874cb03a3d5af4ee0443c03a1d94cd494cc > src/slave/http.cpp fb923686e92ff635e99cc2ccc6316f0282ab3e3a > src/tests/containerizer/io_switchboard_tests.cpp > c00f6a9018801e6b8144ff3beae549c15c7d3917 > > > Diff: https://reviews.apache.org/r/65168/diff/10/ > > > Testing > --- > > internal CI (3x times) > > > Thanks, > > Andrei Budnik > >
Re: Review Request 68230: Added `AgentAPITest.LaunchNestedContainerSessionKillTask` test.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/68230/#review208560 --- Fix it, then Ship it! src/tests/api_tests.cpp Lines 6670 (patched) <https://reviews.apache.org/r/68230/#comment292599> Kill this line src/tests/api_tests.cpp Lines 6679 (patched) <https://reviews.apache.org/r/68230/#comment292600> Kill this line src/tests/api_tests.cpp Lines 6714-6721 (patched) <https://reviews.apache.org/r/68230/#comment292601> Instead, you can piggy back on connection success, something like this: ``` EXPECT_CALL(*scheduler, connected(_)) .WillOnce(v1::scheduler::SendSubscribe(v1::DEFAULT_FRAMEWORK_INFO)); ``` src/tests/api_tests.cpp Lines 6752-6753 (patched) <https://reviews.apache.org/r/68230/#comment292602> `MesosTest::defaultTaskResourcesString` src/tests/api_tests.cpp Lines 6755 (patched) <https://reviews.apache.org/r/68230/#comment292603> `SLEEP_COMMAND()` src/tests/api_tests.cpp Lines 6766-6784 (patched) <https://reviews.apache.org/r/68230/#comment292607> Not yours, but please—for me and my sanity!—let's pull it into a general helper in `tests/mesos.hpp`. Quick grepping shows there are more instances where this helper can be applied (can you even maybe fix those in a separate patch?) https://github.com/apache/mesos/blob/af82f887db535c44b559e0d19cbddfb2dbd6134a/src/tests/master_tests.cpp#L9568-L9586 https://github.com/apache/mesos/blob/af82f887db535c44b559e0d19cbddfb2dbd6134a/src/tests/scheduler_tests.cpp#L612-L630 src/tests/api_tests.cpp Lines 6787 (patched) <https://reviews.apache.org/r/68230/#comment292608> Kill this line src/tests/api_tests.cpp Lines 6833-6835 (patched) <https://reviews.apache.org/r/68230/#comment292609> Update the comment please! src/tests/api_tests.cpp Lines 6843-6854 (patched) <https://reviews.apache.org/r/68230/#comment292610> Does it make sense to make helpers out of these as well? - Alexander Rukletsov On Aug. 31, 2018, 1:11 p.m., Andrei Budnik wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/68230/ > --- > > (Updated Aug. 31, 2018, 1:11 p.m.) > > > Review request for mesos, Alexander Rukletsov, Gilbert Song, and Qian Zhang. > > > Bugs: MESOS-9131 > https://issues.apache.org/jira/browse/MESOS-9131 > > > Repository: mesos > > > Description > --- > > This test verifies that IOSwitchboard, which holds an open HTTP input > connection, terminates once IO redirects finish for the corresponding > nested container. > > > Diffs > - > > src/tests/api_tests.cpp 6be0dfa28333a15b1650268798046dd5e76717c5 > > > Diff: https://reviews.apache.org/r/68230/diff/5/ > > > Testing > --- > > > Thanks, > > Andrei Budnik > >
Re: Review Request 65168: Fixed HTTP errors caused by dropped HTTP responses by IOSwitchboard.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/65168/#review208563 --- src/slave/containerizer/mesos/io/switchboard.cpp Line 1224 (original), 1230 (patched) <https://reviews.apache.org/r/65168/#comment292613> no "a" src/slave/http.cpp Lines 3152-3153 (patched) <https://reviews.apache.org/r/65168/#comment292612> fits one line - Alexander Rukletsov On Sept. 6, 2018, 3:22 p.m., Andrei Budnik wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/65168/ > --- > > (Updated Sept. 6, 2018, 3:22 p.m.) > > > Review request for mesos, Alexander Rukletsov and Qian Zhang. > > > Bugs: MESOS-8545 > https://issues.apache.org/jira/browse/MESOS-8545 > > > Repository: mesos > > > Description > --- > > Previously, IOSwitchboard process could terminate before all HTTP > responses had been sent to the agent. In the case of > `ATTACH_CONTAINER_INPUT` call, we could drop a final HTTP `200 OK` > response, so the agent got broken HTTP connection for the call. > This patch introduces an acknowledgment for the received response > for the `ATTACH_CONTAINER_INPUT` call. This acknowledgment is a new > type of control messages for the `ATTACH_CONTAINER_INPUT` call. When > IOSwitchboard receives an acknowledgment, and io redirects are > finished, it terminates itself. That guarantees that the agent always > receives a response for the `ATTACH_CONTAINER_INPUT` call. > > > Diffs > - > > include/mesos/agent/agent.proto 74488e873cbf99ca487403b70691912cf3788288 > src/slave/containerizer/mesos/io/switchboard.cpp > 52b0e521ed1c651c90b3a3df7c4df576288bf400 > src/slave/http.hpp 78200874cb03a3d5af4ee0443c03a1d94cd494cc > src/slave/http.cpp fb923686e92ff635e99cc2ccc6316f0282ab3e3a > src/tests/containerizer/io_switchboard_tests.cpp > c00f6a9018801e6b8144ff3beae549c15c7d3917 > > > Diff: https://reviews.apache.org/r/65168/diff/8/ > > > Testing > --- > > > Thanks, > > Andrei Budnik > >
Re: Review Request 68231: Added `AgentAPITest.AttachContainerInputRepeat` test.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/68231/#review208562 --- Ship it! - Alexander Rukletsov On Aug. 31, 2018, 2:15 p.m., Andrei Budnik wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/68231/ > --- > > (Updated Aug. 31, 2018, 2:15 p.m.) > > > Review request for mesos, Alexander Rukletsov, Gilbert Song, and Qian Zhang. > > > Repository: mesos > > > Description > --- > > This test verifies that we can call `ATTACH_CONTAINER_INPUT` more > than once. We send a short message first then we send a long message > in chunks. > > > Diffs > - > > src/tests/api_tests.cpp 6be0dfa28333a15b1650268798046dd5e76717c5 > > > Diff: https://reviews.apache.org/r/68231/diff/5/ > > > Testing > --- > > > Thanks, > > Andrei Budnik > >
Re: Review Request 68232: Fixed IOSwitchboard waiting EOF from attach container input request.
---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/68232/#review208558
---
Fix it, then Ship it!
src/slave/containerizer/mesos/io/switchboard.cpp
Line 1225 (original), 1225 (patched)
<https://reviews.apache.org/r/68232/#comment292594>
... set `redirectFinished` promise which triggers a callback...
src/slave/containerizer/mesos/io/switchboard.cpp
Line 1227 (original), 1227 (patched)
<https://reviews.apache.org/r/68232/#comment292595>
.. the EOF message.
src/slave/containerizer/mesos/io/switchboard.cpp
Lines 1707-1708 (patched)
<https://reviews.apache.org/r/68232/#comment292598>
```
//
// TODO(abudnik): Ideally we would have used `process::select()` to capture
a transition into a terminal state for any of `{readLoop, redirectFinished}`.
However, `select()` select currently does not capture a future that has failed.
Another alternative would be to allow `promise::associate()` to accept multiple
source futures.
```
src/slave/containerizer/mesos/io/switchboard.cpp
Lines 1710 (patched)
<https://reviews.apache.org/r/68232/#comment292596>
[promise]
- Alexander Rukletsov
On Aug. 31, 2018, 11:48 a.m., Andrei Budnik wrote:
>
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/68232/
> ---
>
> (Updated Aug. 31, 2018, 11:48 a.m.)
>
>
> Review request for mesos, Alexander Rukletsov, Benjamin Mahler, Gilbert Song,
> and Qian Zhang.
>
>
> Bugs: MESOS-9131
> https://issues.apache.org/jira/browse/MESOS-9131
>
>
> Repository: mesos
>
>
> Description
> ---
>
> Previously, when a corresponding nested container terminated, while the
> user was attached to the container's stdin via `ATTACH_CONTAINER_INPUT`
> IOSwitchboard didn't terminate immediately. IOSwitchboard was waiting
> for EOF message from the input HTTP connection. Since the IOSwitchboard
> was stuck, the corresponding nested container was also stuck in
> `DESTROYING` state.
>
> This patch fixes the aforementioned issue by sending 200 `OK` response
> for `ATTACH_CONTAINER_INPUT` call in the case when io redirect is
> finished while reading from the HTTP input connection is not.
>
>
> Diffs
> -
>
> src/slave/containerizer/mesos/io/switchboard.cpp
> 52b0e521ed1c651c90b3a3df7c4df576288bf400
>
>
> Diff: https://reviews.apache.org/r/68232/diff/8/
>
>
> Testing
> ---
>
> 1. internal Mesosphere CI
> 2. sudo make check (Fedora 25)
>
> This test fixes `LaunchNestedContainerSessionKillTask` test, which can be
> found in the first patch of this patch chain.
>
>
> Thanks,
>
> Andrei Budnik
>
>
Re: Review Request 68568: Added '/roles' to the set of batched master endpoints.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/68568/#review208406 --- Fix it, then Ship it! src/master/http.cpp Line 2605 (original), 2562 (patched) <https://reviews.apache.org/r/68568/#comment292310> I'll move it to `master.cpp`. src/master/master.hpp Lines 1614 (patched) <https://reviews.apache.org/r/68568/#comment292311> I think this can be removed now. - Alexander Rukletsov On Sept. 5, 2018, 12:18 p.m., Benno Evers wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/68568/ > --- > > (Updated Sept. 5, 2018, 12:18 p.m.) > > > Review request for mesos and Alexander Rukletsov. > > > Bugs: MESOS-9194 > https://issues.apache.org/jira/browse/MESOS-9194 > > > Repository: mesos > > > Description > --- > > For improved consistency, the '/roles' endpoint on the > master is now marked as read-only and uses the batching > mechanism shared by the other read-only endpoints. > > > Diffs > - > > src/master/http.cpp e074a93666d82944774e7b0c3fa32d7021d93c97 > src/master/master.hpp eecb66c8826b2b681ef94e6457c2651fc63c724b > src/master/readonly_handler.cpp 47d7de5bce4f6b21134596fe53dd02e457f9c069 > > > Diff: https://reviews.apache.org/r/68568/diff/2/ > > > Testing > --- > > Successful internal CI run executing `make check` on several platforms. > (Jenkins id #4290) > > For rev. 2, internal CI run #4340. > > > Thanks, > > Benno Evers > >
Re: Review Request 68236: Fixed `LaunchNestedContainerSessionsInParallel` test.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/68236/#review208405 --- Ship it! Ship It! - Alexander Rukletsov On Sept. 3, 2018, 4:58 p.m., Andrei Budnik wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/68236/ > --- > > (Updated Sept. 3, 2018, 4:58 p.m.) > > > Review request for mesos and Alexander Rukletsov. > > > Bugs: MESOS-9116 > https://issues.apache.org/jira/browse/MESOS-9116 > > > Repository: mesos > > > Description > --- > > Previously, we sent `ATTACH_CONTAINER_OUTPUT` to attach to a > short-living nested container. An attempt to attach to a terminated > nested container leads to HTTP 500 error. This patch gets rid of > `ATTACH_CONTAINER_OUTPUT` in favor of `LAUNCH_NESTED_CONTAINER_SESSION` > so that we can read the container's output without using an extra call. > > > Diffs > - > > src/tests/api_tests.cpp 43541af732df271177f0fd56ddc419adec461110 > > > Diff: https://reviews.apache.org/r/68236/diff/1/ > > > Testing > --- > > 1. sudo make check (Fedora 25) > 2. internal CI > > > Thanks, > > Andrei Budnik > >
Re: Review Request 68627: Brought site dependencies up to date.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/68627/#review208343 --- Ship it! Ship It! - Alexander Rukletsov On Sept. 5, 2018, 8:53 a.m., Benjamin Bannier wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/68627/ > --- > > (Updated Sept. 5, 2018, 8:53 a.m.) > > > Review request for mesos and Alexander Rukletsov. > > > Repository: mesos > > > Description > --- > > This commit bumps versions of indirect site dependencies to their > latest version without bumping any pinned direct dependencies. > > > Diffs > - > > site/Gemfile.lock 5143ee5b7671cf22ec7fb1d80d74d3efa18eb218 > > > Diff: https://reviews.apache.org/r/68627/diff/1/ > > > Testing > --- > > Manually tested. > > $ bundle install > $ bundle exec rake > $ bundle exec middleman server > > > Thanks, > > Benjamin Bannier > >
Re: Review Request 68567: Restructured /roles code.
> On Sept. 4, 2018, 9:39 a.m., Alexander Rukletsov wrote: > > src/master/http.cpp > > Line 2648 (original), 2644 (patched) > > <https://reviews.apache.org/r/68567/diff/1/?file=2079012#file2079012line2648> > > > > `std::move()` Reading https://www.ibm.com/developerworks/community/blogs/5894415f-be62-4bc0-81c5-3956e82276f3/entry/RVO_V_S_std_move?lang=en_us it seems that casting to rvalue prevents compilers from doing NRVO => dropping. - Alexander --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/68567/#review208297 --- On Sept. 4, 2018, 9:39 a.m., Benno Evers wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/68567/ > --- > > (Updated Sept. 4, 2018, 9:39 a.m.) > > > Review request for mesos and Alexander Rukletsov. > > > Repository: mesos > > > Description > --- > > Reworked the structure of the role handling in both > v0 and v1 APIs in preparation for the subsequent commit. > > As a nice bonus on top, this also saves one trip > through the master queue for every call of this endpoint. > > > Diffs > - > > src/master/http.cpp e074a93666d82944774e7b0c3fa32d7021d93c97 > src/master/master.hpp eecb66c8826b2b681ef94e6457c2651fc63c724b > > > Diff: https://reviews.apache.org/r/68567/diff/1/ > > > Testing > --- > > See https://reviews.apache.org/r/68568/ > > > Thanks, > > Benno Evers > >
Re: Review Request 68568: Added '/roles' to the set of batched master endpoints.
---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/68568/#review208299
---
src/master/master.hpp
Lines 1611 (patched)
<https://reviews.apache.org/r/68568/#comment292039>
What about moving `_filterRoles` directly to the master? We already have
helpers there for roles:
```
bool isWhitelistedRole(const std::string& name) const;
```
and using approvers:
```
process::Future authorizeLogAccess(
const Option& principal);
```
src/master/readonly_handler.cpp
Lines 745 (patched)
<https://reviews.apache.org/r/68568/#comment292038>
const
- Alexander Rukletsov
On Sept. 4, 2018, 9:39 a.m., Benno Evers wrote:
>
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/68568/
> ---
>
> (Updated Sept. 4, 2018, 9:39 a.m.)
>
>
> Review request for mesos and Alexander Rukletsov.
>
>
> Bugs: MESOS-9194
> https://issues.apache.org/jira/browse/MESOS-9194
>
>
> Repository: mesos
>
>
> Description
> ---
>
> To improve consistency, the '/roles' endpoint on the
> master is now marked as read-only and uses the batching
> mechanism shared by the other read-only endpoints.
>
>
> Diffs
> -
>
> src/master/http.cpp e074a93666d82944774e7b0c3fa32d7021d93c97
> src/master/master.hpp eecb66c8826b2b681ef94e6457c2651fc63c724b
> src/master/readonly_handler.cpp 47d7de5bce4f6b21134596fe53dd02e457f9c069
>
>
> Diff: https://reviews.apache.org/r/68568/diff/1/
>
>
> Testing
> ---
>
> Successful internal CI run executing `make check` on several platforms.
> (Jenkins id #4290)
>
>
> Thanks,
>
> Benno Evers
>
>
Re: Review Request 68568: Added '/roles' to the set of batched master endpoints.
> On Aug. 30, 2018, 11:34 a.m., Benno Evers wrote: > > src/master/readonly_handler.cpp > > Lines 753 (patched) > > <https://reviews.apache.org/r/68568/diff/1/?file=2079016#file2079016line753> > > > > Note that this line had to be changed from `weights[name]` to > > `weights.at(name)` during copying to make this compile as a const member > > function. Thanks for calling this out! - Alexander --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/68568/#review208125 --- On Sept. 4, 2018, 9:39 a.m., Benno Evers wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/68568/ > --- > > (Updated Sept. 4, 2018, 9:39 a.m.) > > > Review request for mesos and Alexander Rukletsov. > > > Bugs: MESOS-9194 > https://issues.apache.org/jira/browse/MESOS-9194 > > > Repository: mesos > > > Description > --- > > To improve consistency, the '/roles' endpoint on the > master is now marked as read-only and uses the batching > mechanism shared by the other read-only endpoints. > > > Diffs > - > > src/master/http.cpp e074a93666d82944774e7b0c3fa32d7021d93c97 > src/master/master.hpp eecb66c8826b2b681ef94e6457c2651fc63c724b > src/master/readonly_handler.cpp 47d7de5bce4f6b21134596fe53dd02e457f9c069 > > > Diff: https://reviews.apache.org/r/68568/diff/1/ > > > Testing > --- > > Successful internal CI run executing `make check` on several platforms. > (Jenkins id #4290) > > > Thanks, > > Benno Evers > >
Re: Review Request 68567: Restructured /roles code.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/68567/#review208297 --- Fix it, then Ship it! src/master/http.cpp Line 2648 (original), 2644 (patched) <https://reviews.apache.org/r/68567/#comment292034> `std::move()` src/master/http.cpp Lines 2671 (patched) <https://reviews.apache.org/r/68567/#comment292035> const src/master/http.cpp Lines 2765 (patched) <https://reviews.apache.org/r/68567/#comment292036> const - Alexander Rukletsov On Aug. 30, 2018, 11:33 a.m., Benno Evers wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/68567/ > --- > > (Updated Aug. 30, 2018, 11:33 a.m.) > > > Review request for mesos and Alexander Rukletsov. > > > Repository: mesos > > > Description > --- > > Reworked the structure of the role handling in both > v0 and v1 APIs in preparation for the subsequent commit. > > As a nice bonus on top, this also saves one trip > through the master queue for every call of this endpoint. > > > Diffs > - > > src/master/http.cpp e074a93666d82944774e7b0c3fa32d7021d93c97 > src/master/master.hpp eecb66c8826b2b681ef94e6457c2651fc63c724b > > > Diff: https://reviews.apache.org/r/68567/diff/1/ > > > Testing > --- > > > Thanks, > > Benno Evers > >
Re: Review Request 68555: Made checker library retry to remove the previous check container.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/68555/#review208259 --- Ship it! Good catch and good fix, Qian! - Alexander Rukletsov On Aug. 29, 2018, 7:22 a.m., Qian Zhang wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/68555/ > --- > > (Updated Aug. 29, 2018, 7:22 a.m.) > > > Review request for mesos, Andrei Budnik, Alexander Rukletsov, Gastón Kleiman, > and Gilbert Song. > > > Bugs: MESOS-8568 > https://issues.apache.org/jira/browse/MESOS-8568 > > > Repository: mesos > > > Description > --- > > Previously when checker library fails to remove the previous check > container, it will discard the promise and launch a new check container > which will cause two problems: > 1. The discarded promise is used to launch the new check container, > that means even the new check container is launched successfully, > we still have no chance to process its check result since the > promise has already been discarded. > 2. The previous check container will never get a chance to be removed > which is leak, i.e., its runtime directory and sandbox directory > will not be removed. > > Now in this patch, when checker library fails to remove the previous > check container, we make it remove the previous check container again. > > > Diffs > - > > src/checks/checker_process.cpp 77a76f465fe57eab89f027b5acb74c2339551678 > > > Diff: https://reviews.apache.org/r/68555/diff/1/ > > > Testing > --- > > sudo make check > > > Thanks, > > Qian Zhang > >
Re: Review Request 68495: Made command check always waits before removing the nested container.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/68495/#review208258 --- Ship it! Modulo my comment in the previous review. - Alexander Rukletsov On Aug. 24, 2018, 9:54 a.m., Qian Zhang wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/68495/ > --- > > (Updated Aug. 24, 2018, 9:54 a.m.) > > > Review request for mesos, Andrei Budnik, Alexander Rukletsov, Gastón Kleiman, > and Gilbert Song. > > > Bugs: MESOS-8568 > https://issues.apache.org/jira/browse/MESOS-8568 > > > Repository: mesos > > > Description > --- > > Made command check always waits before removing the nested container. > > > Diffs > - > > src/checks/checker_process.cpp 77a76f465fe57eab89f027b5acb74c2339551678 > > > Diff: https://reviews.apache.org/r/68495/diff/1/ > > > Testing > --- > > sudo make check > > > Thanks, > > Qian Zhang > >
Re: Review Request 68495: Made command check always waits before removing the nested container.
> On Aug. 27, 2018, 5:16 p.m., Alexander Rukletsov wrote:
> > src/checks/checker_process.cpp
> > Lines 878-889 (original), 890-901 (patched)
> > <https://reviews.apache.org/r/68495/diff/1/?file=2077041#file2077041line890>
> >
> > It looks like we should always call `waitNestedContainer()` after we
> > said `previousCheckContainerId = checkContainerId;`. For example here.
> >
> > Maybe it makes sense to call `waitNestedContainer()` right in the
> > beginning? We can end up calling it twice, but I think it's fine?
>
> Qian Zhang wrote:
> > Maybe it makes sense to call waitNestedContainer() right in the
> beginning? We can end up calling it twice, but I think it's fine?
>
> That means we will call agent API `WAIT_NESTED_CONTAINER` twice for each
> successful launch of check container, I think that might be a burden for
> agent in a large scale env. So I'd still prefer to call it only in the places
> where we have to do it.
>
> Qian Zhang wrote:
> And for the case (L879:L888, i.e., the connection to agent failed) that
> you pointed out above, I think when the connection to agent is back (e.g.,
> agent starts up again), the check container will be treated as orphan
> container and destroyed by agent, and then we will remove it here:
> https://github.com/apache/mesos/blob/1.6.1/src/checks/checker_process.cpp#L616:L638.
> However I am going to post another patch to change these codes
> (https://github.com/apache/mesos/blob/1.6.1/src/checks/checker_process.cpp#L660:L664)
> to something like:
> ```
> promise->discard();
> } else {
> previousCheckContainerId = None();
> _nestedCommandCheck(promise, cmd, nested);
> }
> ```
> In this way, if we fail to remove the check container (e.g., due to agent
> has not finished recovery, or the check container is still in `DESTROYING`
> state), we will try to remove it again.
>
> Qian Zhang wrote:
> I posted another patch https://reviews.apache.org/r/68555/ as I mentioned
> above.
So the assumption you're making is that if `Connection::send()` fails, the
container will not start and hence we don't need to call `wait()` before we
destroy? It is not obvious to a reader and requires some non-local knowledge
about how the UCR works. Can you please leave a comment documenting this
assumption so that when it changes and someone will be trying to figure out why
checks sometimes started fail, they have a hint : )
- Alexander
---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/68495/#review207984
---
On Aug. 24, 2018, 9:54 a.m., Qian Zhang wrote:
>
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/68495/
> ---
>
> (Updated Aug. 24, 2018, 9:54 a.m.)
>
>
> Review request for mesos, Andrei Budnik, Alexander Rukletsov, Gastón Kleiman,
> and Gilbert Song.
>
>
> Bugs: MESOS-8568
> https://issues.apache.org/jira/browse/MESOS-8568
>
>
> Repository: mesos
>
>
> Description
> ---
>
> Made command check always waits before removing the nested container.
>
>
> Diffs
> -
>
> src/checks/checker_process.cpp 77a76f465fe57eab89f027b5acb74c2339551678
>
>
> Diff: https://reviews.apache.org/r/68495/diff/1/
>
>
> Testing
> ---
>
> sudo make check
>
>
> Thanks,
>
> Qian Zhang
>
>
Re: Review Request 68131: Added MasterActorResponsiveness_BENCHMARK_Test.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/68131/ --- (Updated Aug. 30, 2018, 8:47 p.m.) Review request for mesos, Benno Evers and Benjamin Mahler. Bugs: MESOS-8975 https://issues.apache.org/jira/browse/MESOS-8975 Repository: mesos Description --- See summary. Diffs - src/tests/master_benchmarks.cpp b6d6dc7c1752491e2da854018966374b624d6682 Diff: https://reviews.apache.org/r/68131/diff/3/ Testing --- See https://reviews.apache.org/r/68132/ Thanks, Alexander Rukletsov
