Re: Review Request 71300: Removed mesos-style transition script.
> On May 6, 2020, 5:52 p.m., Andrei Sekretenko wrote: > > As 1.10 release is near, I have landed this into master (cannot close the > > PR as "submitted"). > > > > Sumbission: > > > > commit 83359534cb1b3303fcbae34af3fadd81b7c8cb85 > > Author: Benjamin Bannier > > Date: Wed May 6 17:47:37 2020 +0200 > > > > Removed mesos-style transition script. > > > > Review: https://reviews.apache.org/r/71300/ Thanks Andrei. Really happy to see this transition wrapping up. - Benjamin --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/71300/#review220659 ------- On Aug. 26, 2019, 6:40 p.m., Benjamin Bannier wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/71300/ > --- > > (Updated Aug. 26, 2019, 6:40 p.m.) > > > Review request for mesos, Benno Evers and Till Toenshoff. > > > Bugs: MESOS-9630 > https://issues.apache.org/jira/browse/MESOS-9630 > > > Repository: mesos > > > Description > --- > > Removed mesos-style transition script. > > > Diffs > - > > support/mesos-style.py cd490bd3deeb8477295bbad010f36d984ce17d27 > > > Diff: https://reviews.apache.org/r/71300/diff/3/ > > > Testing > --- > > n/a > > THIS PATCH SHOULD ONLY BE COMMITTED AFTER THE PRECEEDING CHAIN HAS BEEN > LANDED FOR SOME TIME TO GIVE CONTRIBUTORS A CHANCE TO ADJUST THEIR WORKFLOW. > > > Thanks, > > Benjamin Bannier > >
Re: Review Request 72161: Added patch for RapidJSON.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/72161/#review219677 --- Is the plan to upstream this patch? We usually try to enable building against unbundled dependencies and depending on custom behavior makes that impossible. My hunch would be to not make functional changes like the one in this patch to dependencies (changes for building are usually fine). - Benjamin Bannier On Feb. 25, 2020, 2:07 vorm., Qian Zhang wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/72161/ > --- > > (Updated Feb. 25, 2020, 2:07 vorm.) > > > Review request for mesos, Andrei Budnik and Greg Mann. > > > Bugs: MESOS-10064 > https://issues.apache.org/jira/browse/MESOS-10064 > > > Repository: mesos > > > Description > --- > > This commit updates the writer of RapidJSON to write infinite > floating point numbers as "Infinity" and "-Infinity" (i.e., > with double quotes) rather than Infinity and -Infinity. This > is to ensure the strings converted from JSON objects conform > to the rule defined by Protobuf: > https://developers.google.com/protocol-buffers/docs/proto3#json > > > Diffs > - > > 3rdparty/CMakeLists.txt c45d742684ba4b3b4abc57ae0bcb85a879c68bfd > 3rdparty/rapidjson-1.1.0.patch PRE-CREATION > > > Diff: https://reviews.apache.org/r/72161/diff/1/ > > > Testing > --- > > > Thanks, > > Qian Zhang > >
Re: Review Request 72114: Removed remaining domain socket code from the Windows build.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/72114/#review219545 --- Ship it! For me personally this goes a little far in ifdef'ing out code (the flags could be left in on Windows, even though they are never added; the test could be disabled at runtime with `TEST_F_TEMP_DISABLED_ON_WINDOWS` or sim; only the part in `slave.cpp` we seem to really need to exclude if we don't want to `CHECK` for either the variable being set `||` windows), but it probably still fits with what we do elsewhere. The reason I am weary of ifdef's is that they affect code visibility which can make it harder to work with the code (e.g., _find references_ in some IDE might not show all references anymore). It also affects what code e.g., static analyzers can see. I believe we do too much of this already, but it is probably okay here since we do not select for features, but for a platform (selecting in ifdef's on features leads to a hard to control explosion of flag combinations needed so e.g., a static analyzer can see all code). - Benjamin Bannier On Feb. 11, 2020, 5:38 p.m., Greg Mann wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/72114/ > --- > > (Updated Feb. 11, 2020, 5:38 p.m.) > > > Review request for mesos, Andrei Budnik and Benjamin Bannier. > > > Repository: mesos > > > Description > --- > > These changes are needed to get the tests to run. > > > Diffs > - > > src/slave/flags.hpp 838aaee0b921b1b59d4e2b2fb69d861eec95ba56 > src/slave/slave.cpp 75bf59566a327a05cf7e763409b60f30e9432c86 > src/tests/command_executor_tests.cpp > 73f80063631f1d004be307fdce77d1b405468e14 > > > Diff: https://reviews.apache.org/r/72114/diff/1/ > > > Testing > --- > > `cmake --build . --target tests` > `src\mesos-tests.exe` > > > Thanks, > > Greg Mann > >
Re: Review Request 72107: Fixed Windows command argument stringification.
---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/72107/#review219538
---
3rdparty/stout/include/stout/os/windows/shell.hpp
Lines 218 (patched)
<https://reviews.apache.org/r/72107/#comment307729>
Please add a test for this.
3rdparty/stout/include/stout/os/windows/shell.hpp
Lines 219-223 (patched)
<https://reviews.apache.org/r/72107/#comment307728>
This is a strange loop and it took me a while to digest.
Could we instead just perform the escaping inside the loop above where we
already perform some escaping? You could even introduce a small helper if you
wanted.
- Benjamin Bannier
On Feb. 11, 2020, 9:54 a.m., Greg Mann wrote:
>
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/72107/
> ---
>
> (Updated Feb. 11, 2020, 9:54 a.m.)
>
>
> Review request for mesos, Andrei Budnik and Benjamin Bannier.
>
>
> Bugs: MESOS-10093
> https://issues.apache.org/jira/browse/MESOS-10093
>
>
> Repository: mesos
>
>
> Description
> ---
>
> Fixed Windows command argument stringification.
>
>
> Diffs
> -
>
> 3rdparty/stout/include/stout/os/windows/shell.hpp
> 2f76bbaac9032906c544010c55cdcbe3d7b03d23
>
>
> Diff: https://reviews.apache.org/r/72107/diff/1/
>
>
> Testing
> ---
>
> Tested manually by launching a docker task with the following command:
>
> ```
> python -c "import time\nwhile True:\ntime.sleep(5)\nprint('hello
> world')"
> ```
>
>
> Thanks,
>
> Greg Mann
>
>
Review Request 72070: Fixed the build on Windows.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/72070/ --- Review request for mesos, Greg Mann and Joseph Wu. Bugs: MESOS-10090 https://issues.apache.org/jira/browse/MESOS-10090 Repository: mesos Description --- The addition of executor domain socket support broke the build on Windows. This patch updates preprocessor directives in order to fix the build. Diffs - src/executor/executor.cpp 7cff25845176f0fc893ebb708e7a16974a51681f src/launcher/default_executor.cpp 4369fd0052b2e8496ba63606fa57e17d881ea52c src/local/local.cpp 6a7709de33908eed08dae3a0b8eb2a31dcd7b482 src/slave/flags.cpp 7653c397cbf27490f7b071b5e151bcdf77e6478c src/slave/main.cpp b5715d9f14a7d64ed0b82082bf7d04b97a48cfe4 src/slave/slave.hpp 38448672747f82f2b7ee72aef50cb8eac08f1e8b src/slave/slave.cpp 4c3d4715538742c84c060f5b197eaaae0e561793 src/tests/cluster.cpp ffb7161ed91c4b5b7a200bbca79e3be4b767cebe src/tests/mock_slave.cpp dfbca2a01083072f68d30d6867fb3f487be5ec16 Diff: https://reviews.apache.org/r/72070/diff/1/ Testing --- `make check` macos/Linux; `make tests` msvc Thanks, Benjamin Bannier
Review Request 72054: Switched `verify-reviews.py` to use ubuntu-16.04-based image.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/72054/ --- Review request for mesos and Andrei Budnik. Repository: mesos Description --- This not only uses a more recent platform to verify review, but also works around MESOS-10091 which affects only older systems like e.g., ubuntu-14.04. Diffs - support/verify-reviews.py 045a9ef12eef1cefa5e61938366d814de04835cc Diff: https://reviews.apache.org/r/72054/diff/1/ Testing --- Successfully ran a dummy verify-reviews session. Thanks, Benjamin Bannier
Re: Review Request 67631: [WIP] Added non-const accessor for JSON values.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/67631/#review219398 --- Bad review! Reviews applied: [] Error: No reviewers specified. Please find a reviewer by asking on JIRA or the mailing list. - Benjamin Bannier On June 18, 2018, 3:52 p.m., Benno Evers wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/67631/ > --- > > (Updated June 18, 2018, 3:52 p.m.) > > > Review request for mesos. > > > Repository: mesos > > > Description > --- > > Added a non-const version of Value::as(), > to enable modifying json objects without > having to resort to the underlying boost functions. > > > Diffs > - > > 3rdparty/stout/include/stout/json.hpp > 5e738cf6f72f32b852beb61a16787e48082dab14 > > > Diff: https://reviews.apache.org/r/67631/diff/1/ > > > Testing > --- > > > Thanks, > > Benno Evers > >
Re: Review Request 71421: Switched verify-reviews from docker-build to prepared Docker image.
> On Jan. 27, 2020, 4:16 p.m., Andrei Budnik wrote: > > support/verify-reviews.py > > Lines 245-248 (original), 245-248 (patched) > > <https://reviews.apache.org/r/71421/diff/1/?file=2163242#file2163242line245> > > > > Please, update these comments if needed. Just got rid of this comment. - Benjamin --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/71421/#review219383 --- On Sept. 2, 2019, 11:35 a.m., Benjamin Bannier wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/71421/ > --- > > (Updated Sept. 2, 2019, 11:35 a.m.) > > > Review request for mesos and Benjamin Bannier. > > > Repository: mesos > > > Description > --- > > Switched verify-reviews from docker-build to prepared Docker image. > > > Diffs > - > > support/verify-reviews.py 045a9ef12eef1cefa5e61938366d814de04835cc > > > Diff: https://reviews.apache.org/r/71421/diff/3/ > > > Testing > --- > > > Thanks, > > Benjamin Bannier > >
Re: Review Request 72035: Remembered whether an executor was agent-generated.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/72035/ --- (Updated Jan. 22, 2020, 1:46 p.m.) Review request for mesos, Andrei Sekretenko and Benjamin Mahler. Bugs: MESOS-10084 https://issues.apache.org/jira/browse/MESOS-10084 Repository: mesos Description --- This patch adds code to pass on whether was generated in the agent from the point where the executor is generated to the point where we create an actual `slave::Executor` instance. This allows us to persist this information in the executor state. Diffs (updated) - src/slave/slave.hpp 3d191dcbdd58e851ad5e86fbb01a890407409c64 src/slave/slave.cpp 0005971717f5b90da368b45caad8e209ada95fa5 src/tests/mock_slave.hpp eb31a0f93d21ef859684c26a99effde08348ead6 src/tests/mock_slave.cpp 4940285b183a9513701e616e7d7523801daf113b Diff: https://reviews.apache.org/r/72035/diff/2/ Changes: https://reviews.apache.org/r/72035/diff/1-2/ Testing --- `make check` Thanks, Benjamin Bannier
Re: Review Request 72035: Remembered whether an executor was agent-generated.
> On Jan. 21, 2020, 2:40 p.m., Andrei Sekretenko wrote: > > src/slave/slave.cpp > > Lines 10625 (patched) > > <https://reviews.apache.org/r/72035/diff/1/?file=2209256#file2209256line10625> > > > > Can you avoid spliting initialization of `isGeneratedForCommandTask_` > > and just fully initialize it here? > > After that, it should also be possible to make it `const`. > > > > Btw, it might make sense to move the workaround logic into > > `ExecutorState::recover(...)` so that we don't have Option in the code. Fixed the initialization. Still defined the member as mutable as nothing about it makes it inherently unassignable; if one worries about unintentionally mutating internal state one should instead either work with `const` `Executor` variables in the calling scope, or declare `Executor` member functions as `const`. We don't do the first often enough, but often enough adhere to the second. - Benjamin --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/72035/#review219342 ------- On Jan. 22, 2020, 1:46 p.m., Benjamin Bannier wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/72035/ > --- > > (Updated Jan. 22, 2020, 1:46 p.m.) > > > Review request for mesos, Andrei Sekretenko and Benjamin Mahler. > > > Bugs: MESOS-10084 > https://issues.apache.org/jira/browse/MESOS-10084 > > > Repository: mesos > > > Description > --- > > This patch adds code to pass on whether was generated in the agent from > the point where the executor is generated to the point where we create > an actual `slave::Executor` instance. This allows us to persist this > information in the executor state. > > > Diffs > - > > src/slave/slave.hpp 3d191dcbdd58e851ad5e86fbb01a890407409c64 > src/slave/slave.cpp 0005971717f5b90da368b45caad8e209ada95fa5 > src/tests/mock_slave.hpp eb31a0f93d21ef859684c26a99effde08348ead6 > src/tests/mock_slave.cpp 4940285b183a9513701e616e7d7523801daf113b > > > Diff: https://reviews.apache.org/r/72035/diff/2/ > > > Testing > --- > > `make check` > > > Thanks, > > Benjamin Bannier > >
Re: Review Request 72034: Sync'd whether an executor was generated to and from disk.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/72034/ --- (Updated Jan. 22, 2020, 1:43 p.m.) Review request for mesos, Andrei Sekretenko and Benjamin Mahler. Bugs: MESOS-10084 https://issues.apache.org/jira/browse/MESOS-10084 Repository: mesos Description --- This patch introduces an `ExecutorState` variable signifying whether an executor was generated by the agent (and is thus unknown to the master). Currently we still detect agent-generated executors with a heuristic, but will adapt that heuristic in a follow-up path making us of the additional state we now persist. Diffs (updated) - src/slave/paths.hpp d7fcb1321b12a3d8ac96f3267a9bae82dc9cb6c4 src/slave/paths.cpp af2679f59de56a77859cb07299528f6699327796 src/slave/slave.cpp 0005971717f5b90da368b45caad8e209ada95fa5 src/slave/state.hpp 6d6ae0168ae1e4c999a099c0505344dac2fac71c src/slave/state.cpp 821132ba81c955714db2e809cca764262997639a Diff: https://reviews.apache.org/r/72034/diff/2/ Changes: https://reviews.apache.org/r/72034/diff/1-2/ Testing --- `make check` Thanks, Benjamin Bannier
Re: Review Request 72033: Decoupled detection of generated executors from Mesos install location.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/72033/ --- (Updated Jan. 21, 2020, 4:39 p.m.) Review request for mesos, Andrei Sekretenko and Benjamin Mahler. Bugs: MESOS-10084 https://issues.apache.org/jira/browse/MESOS-10084 Repository: mesos Description (updated) --- We previously were detecting executors generated for command tasks by checking whether their command matched the full path of `mesos-executor`. This approach can lead to misdetection if e.g., during an upgrade a new installation location is choosen. This patch adjusts the heuristic by now only relying on the fact that the executor command should end in `mesos-executor`. In order to cut down on false positives we now additionally check that the executor name looks similar to the ones we generate for command tasks. Diffs (updated) - src/slave/slave.cpp 0005971717f5b90da368b45caad8e209ada95fa5 Diff: https://reviews.apache.org/r/72033/diff/2/ Changes: https://reviews.apache.org/r/72033/diff/1-2/ Testing --- `make check` Thanks, Benjamin Bannier
Review Request 72034: Sync'd whether an executor was generated to and from disk.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/72034/ --- Review request for mesos, Andrei Sekretenko and Benjamin Mahler. Bugs: MESOS-10084 https://issues.apache.org/jira/browse/MESOS-10084 Repository: mesos Description --- This patch introduces an `ExecutorState` variable signifying whether an executor was generated by the agent (and is thus unknown to the master). Currently we still detect agent-generated executors with a heuristic, but will adapt that heuristic in a follow-up path making us of the additional state we now persist. Diffs - src/slave/paths.hpp d7fcb1321b12a3d8ac96f3267a9bae82dc9cb6c4 src/slave/paths.cpp af2679f59de56a77859cb07299528f6699327796 src/slave/slave.cpp 0005971717f5b90da368b45caad8e209ada95fa5 src/slave/state.hpp 6d6ae0168ae1e4c999a099c0505344dac2fac71c src/slave/state.cpp 821132ba81c955714db2e809cca764262997639a Diff: https://reviews.apache.org/r/72034/diff/1/ Testing --- `make check` Thanks, Benjamin Bannier
Review Request 72033: Decoupled detection of generated executors from Mesos install location.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/72033/ --- Review request for mesos, Andrei Sekretenko and Benjamin Mahler. Bugs: MESOS-10084 https://issues.apache.org/jira/browse/MESOS-10084 Repository: mesos Description --- We previously were detecting executors generated for command tasks by checking whether their command matched the full path of `mesos-executor`. This approach can lead to misdetection if e.g., during an upgrade a new installation location is choosen. This patch adjusts the heuristic by now only relying on the fact that the executor command should end in `mesos-execute`. In order to cut down on false positives we now additionally check that the executor name looks similar to the ones we generate for command tasks. Diffs - src/slave/slave.cpp 0005971717f5b90da368b45caad8e209ada95fa5 Diff: https://reviews.apache.org/r/72033/diff/1/ Testing --- `make check` Thanks, Benjamin Bannier
Review Request 72035: Remembered whether an executor was agent-generated.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/72035/ --- Review request for mesos, Andrei Sekretenko and Benjamin Mahler. Bugs: MESOS-10084 https://issues.apache.org/jira/browse/MESOS-10084 Repository: mesos Description --- This patch adds code to pass on whether was generated in the agent from the point where the executor is generated to the point where we create an actual `slave::Executor` instance. This allows us to persist this information in the executor state. Diffs - src/slave/slave.hpp 3d191dcbdd58e851ad5e86fbb01a890407409c64 src/slave/slave.cpp 0005971717f5b90da368b45caad8e209ada95fa5 src/tests/mock_slave.hpp eb31a0f93d21ef859684c26a99effde08348ead6 src/tests/mock_slave.cpp 4940285b183a9513701e616e7d7523801daf113b Diff: https://reviews.apache.org/r/72035/diff/1/ Testing --- `make check` Thanks, Benjamin Bannier
Review Request 72011: Removed unintentional copies of for loop iteration variables.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/72011/ --- Review request for mesos and Andrei Sekretenko. Repository: mesos Description --- Removed unintentional copies of for loop iteration variables. Diffs - src/master/validation.cpp d628bb3443b32acac38e29a15a49be688b35b8b3 src/slave/container_loggers/lib_logrotate.cpp 3ecd83ef42a5bff9ec082024bdabaf6cb4a50de0 src/slave/containerizer/mesos/provisioner/backends/copy.cpp 4afef7f75092067caac35459805d670a9651afa0 src/slave/gc.cpp 450bbef66e317a403d566bb0ac5ccf5fb0d2329e src/tests/sorter_tests.cpp 472691d1aba9d476bf0cf35ca82106d880cdc415 Diff: https://reviews.apache.org/r/72011/diff/1/ Testing --- `make check` Thanks, Benjamin Bannier
Re: Review Request 72002: Added `ExecutorInfo` field to tag agent-generated executors.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/72002/ --- (Updated Jan. 16, 2020, 2:51 p.m.) Review request for mesos, Andrei Sekretenko and Benjamin Mahler. Bugs: MESOS-10084 https://issues.apache.org/jira/browse/MESOS-10084 Repository: mesos Description --- For command tasks the agent will generate an `ExecutorInfo` which was previously detected with a path-based heuristic. If the installation location of Mesos changes, e.g., in an upgrade this can lead to misdetections of generated executors. The flag added in this patch will be set and persisted in a follow-up patch which will allow for a more stable solution. Diffs (updated) - include/mesos/mesos.proto b0f590545df720df644d049e9d8f1e81bdbe814c include/mesos/v1/mesos.proto 53a7b9bc6190d826868a1633c11c9a0ecf9acf0a src/master/validation.cpp d628bb3443b32acac38e29a15a49be688b35b8b3 Diff: https://reviews.apache.org/r/72002/diff/3/ Changes: https://reviews.apache.org/r/72002/diff/2-3/ Testing --- `make check` Thanks, Benjamin Bannier
Re: Review Request 72002: Added `ExecutorInfo` field to tag agent-generated executors.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/72002/ --- (Updated Jan. 16, 2020, 2:12 p.m.) Review request for mesos, Andrei Sekretenko and Benjamin Mahler. Changes --- Reject tasks with added field set as suggested in https://reviews.apache.org/r/72003/#comment_container_307435 Bugs: MESOS-10084 https://issues.apache.org/jira/browse/MESOS-10084 Repository: mesos Description --- For command tasks the agent will generate an `ExecutorInfo` which was previously detected with a path-based heuristic. If the installation location of Mesos changes, e.g., in an upgrade this can lead to misdetections of generated executors. The flag added in this patch will be set and persisted in a follow-up patch which will allow for a more stable solution. Diffs (updated) - include/mesos/mesos.proto b0f590545df720df644d049e9d8f1e81bdbe814c include/mesos/v1/mesos.proto 53a7b9bc6190d826868a1633c11c9a0ecf9acf0a src/master/validation.cpp d628bb3443b32acac38e29a15a49be688b35b8b3 Diff: https://reviews.apache.org/r/72002/diff/2/ Changes: https://reviews.apache.org/r/72002/diff/1-2/ Testing --- `make check` Thanks, Benjamin Bannier
Re: Review Request 72003: Used persisted information to decide if an executor is agent-generated.
> On Jan. 15, 2020, 2:55 p.m., Andrei Sekretenko wrote: > > src/slave/slave.cpp > > Line 6583 (original), 6583 (patched) > > <https://reviews.apache.org/r/72003/diff/1/?file=2207139#file2207139line6583> > > > > If we are not validating that `ExecutorInfo` of a task does not have > > `generated_by_agent` set, maybe we can at least forcibly clear it? either > > here or in `Slave::runTask()`, and document (in the previous patch) that > > agent **ignores** this field when received from outside? > > Benjamin Bannier wrote: > That would be possible, but I am unsure it would be worth it. We > currently do perform mutations on user-provided executors, the documentation > of the added field clearly indicates that it should not be set nor be relied > on by frameworks, and even if it was set it would only lead to potentially > misbehaving tasks without wider effect. > > I feel we shouldn't do this, but please reopen if you believe it is > critical for some reason. Added rejection of framework-specified `generated_by_agent` in the previous patch. - Benjamin --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/72003/#review219270 ------- On Jan. 15, 2020, 4:12 p.m., Benjamin Bannier wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/72003/ > --- > > (Updated Jan. 15, 2020, 4:12 p.m.) > > > Review request for mesos, Andrei Sekretenko and Benjamin Mahler. > > > Bugs: MESOS-10084 > https://issues.apache.org/jira/browse/MESOS-10084 > > > Repository: mesos > > > Description > --- > > This patch modifies the agent code so it uses > `ExecutorInfo::generated_by_agent` instead of the previous heuristic to > detect agent-generated executors for command tasks. For that we add > recovery logic for upgrade scenarios to inject a correct value for > recovered executors, and set the appropriate value for newly created > executors. > > > Diffs > - > > src/slave/slave.hpp 3d191dcbdd58e851ad5e86fbb01a890407409c64 > src/slave/slave.cpp 0005971717f5b90da368b45caad8e209ada95fa5 > > > Diff: https://reviews.apache.org/r/72003/diff/2/ > > > Testing > --- > > `make check` > > > Thanks, > > Benjamin Bannier > >
Re: Review Request 72003: Used persisted information to decide if an executor is agent-generated.
> On Jan. 15, 2020, 2:55 p.m., Andrei Sekretenko wrote: > > src/slave/slave.cpp > > Line 6583 (original), 6583 (patched) > > <https://reviews.apache.org/r/72003/diff/1/?file=2207139#file2207139line6583> > > > > If we are not validating that `ExecutorInfo` of a task does not have > > `generated_by_agent` set, maybe we can at least forcibly clear it? either > > here or in `Slave::runTask()`, and document (in the previous patch) that > > agent **ignores** this field when received from outside? That would be possible, but I am unsure it would be worth it. We currently do perform mutations on user-provided executors, the documentation of the added field clearly indicates that it should not be set nor be relied on by frameworks, and even if it was set it would only lead to potentially misbehaving tasks without wider effect. I feel we shouldn't do this, but please reopen if you believe it is critical for some reason. - Benjamin --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/72003/#review219270 ------- On Jan. 15, 2020, 4:12 p.m., Benjamin Bannier wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/72003/ > --- > > (Updated Jan. 15, 2020, 4:12 p.m.) > > > Review request for mesos, Andrei Sekretenko and Benjamin Mahler. > > > Bugs: MESOS-10084 > https://issues.apache.org/jira/browse/MESOS-10084 > > > Repository: mesos > > > Description > --- > > This patch modifies the agent code so it uses > `ExecutorInfo::generated_by_agent` instead of the previous heuristic to > detect agent-generated executors for command tasks. For that we add > recovery logic for upgrade scenarios to inject a correct value for > recovered executors, and set the appropriate value for newly created > executors. > > > Diffs > - > > src/slave/slave.hpp 3d191dcbdd58e851ad5e86fbb01a890407409c64 > src/slave/slave.cpp 0005971717f5b90da368b45caad8e209ada95fa5 > > > Diff: https://reviews.apache.org/r/72003/diff/2/ > > > Testing > --- > > `make check` > > > Thanks, > > Benjamin Bannier > >
Re: Review Request 72003: Used persisted information to decide if an executor is agent-generated.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/72003/ --- (Updated Jan. 15, 2020, 4:12 p.m.) Review request for mesos, Andrei Sekretenko and Benjamin Mahler. Bugs: MESOS-10084 https://issues.apache.org/jira/browse/MESOS-10084 Repository: mesos Description --- This patch modifies the agent code so it uses `ExecutorInfo::generated_by_agent` instead of the previous heuristic to detect agent-generated executors for command tasks. For that we add recovery logic for upgrade scenarios to inject a correct value for recovered executors, and set the appropriate value for newly created executors. Diffs (updated) - src/slave/slave.hpp 3d191dcbdd58e851ad5e86fbb01a890407409c64 src/slave/slave.cpp 0005971717f5b90da368b45caad8e209ada95fa5 Diff: https://reviews.apache.org/r/72003/diff/2/ Changes: https://reviews.apache.org/r/72003/diff/1-2/ Testing --- `make check` Thanks, Benjamin Bannier
Re: Review Request 72003: Used persisted information to decide if an executor is agent-generated.
> On Jan. 15, 2020, 2:55 p.m., Andrei Sekretenko wrote: > > src/slave/slave.cpp > > Lines 6593 (patched) > > <https://reviews.apache.org/r/72003/diff/1/?file=2207139#file2207139line6593> > > > > The logic "executor is generated by agent -> master receives no > > updates" is not covered by any of the local tests; if we are not > > introducing the test right now, consider adding TODO (and, probably, a > > separate ticket). > > > > Personally, I'm more or less OK with `upgradeExecutorInfo(...)` not > > covered: the necessary setup is difficult to create, that code is hacky > > anyway, and we want to phase it out. Added a `TODO` at the usage site in `Slave::doReliableRegistration`. > On Jan. 15, 2020, 2:55 p.m., Andrei Sekretenko wrote: > > src/slave/slave.cpp > > Line 10601 (original), 10626 (patched) > > <https://reviews.apache.org/r/72003/diff/1/?file=2207139#file2207139line10626> > > > > Maybe add/move CHECK_NOTNULL to this line, so that it does not rely on > > the fact that `slave` is **declared** before `info`? I added another `CHECK_NOTNULL` here. - Benjamin --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/72003/#review219270 --- On Jan. 15, 2020, 12:25 p.m., Benjamin Bannier wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/72003/ > --- > > (Updated Jan. 15, 2020, 12:25 p.m.) > > > Review request for mesos, Andrei Sekretenko and Benjamin Mahler. > > > Bugs: MESOS-10084 > https://issues.apache.org/jira/browse/MESOS-10084 > > > Repository: mesos > > > Description > --- > > This patch modifies the agent code so it uses > `ExecutorInfo::generated_by_agent` instead of the previous heuristic to > detect agent-generated executors for command tasks. For that we add > recovery logic for upgrade scenarios to inject a correct value for > recovered executors, and set the appropriate value for newly created > executors. > > > Diffs > - > > src/slave/slave.hpp 3d191dcbdd58e851ad5e86fbb01a890407409c64 > src/slave/slave.cpp 0005971717f5b90da368b45caad8e209ada95fa5 > > > Diff: https://reviews.apache.org/r/72003/diff/1/ > > > Testing > --- > > `make check` > > > Thanks, > > Benjamin Bannier > >
Review Request 72003: Used persisted information to decide if an executor is agent-generated.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/72003/ --- Review request for mesos, Andrei Sekretenko and Benjamin Mahler. Bugs: MESOS-10084 https://issues.apache.org/jira/browse/MESOS-10084 Repository: mesos Description --- This patch modifies the agent code so it uses `ExecutorInfo::generated_by_agent` instead of the previous heuristic to detect agent-generated executors for command tasks. For that we add recovery logic for upgrade scenarios to inject a correct value for recovered executors, and set the appropriate value for newly created executors. Diffs - src/slave/slave.hpp 3d191dcbdd58e851ad5e86fbb01a890407409c64 src/slave/slave.cpp 0005971717f5b90da368b45caad8e209ada95fa5 Diff: https://reviews.apache.org/r/72003/diff/1/ Testing --- `make check` Thanks, Benjamin Bannier
Review Request 72002: Added `ExecutorInfo` field to tag agent-generated executors.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/72002/ --- Review request for mesos, Andrei Sekretenko and Benjamin Mahler. Bugs: MESOS-10084 https://issues.apache.org/jira/browse/MESOS-10084 Repository: mesos Description --- For command tasks the agent will generate an `ExecutorInfo` which was previously detected with a path-based heuristic. If the installation location of Mesos changes, e.g., in an upgrade this can lead to misdetections of generated executors. The flag added in this patch will be set and persisted in a follow-up patch which will allow for a more stable solution. Diffs - include/mesos/mesos.proto b0f590545df720df644d049e9d8f1e81bdbe814c include/mesos/v1/mesos.proto 53a7b9bc6190d826868a1633c11c9a0ecf9acf0a Diff: https://reviews.apache.org/r/72002/diff/1/ Testing --- `make check` Thanks, Benjamin Bannier
Review Request 71996: Fixed issues flagged by cpplint.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/71996/ --- Review request for mesos, Andrei Sekretenko and Benjamin Mahler. Repository: mesos Description --- Fixed issues flagged by cpplint. Diffs - src/master/authorization.hpp f28b29f36650ea3a0727f4f812951d457b522de9 src/master/authorization.cpp ea101751b83b7219f5921fe4f463c8bc8577cebd src/master/master.cpp 23f747c493d1ab613cf56611b91932f0525fb33c src/tests/containerizer/io_switchboard_tests.cpp 1b347eb5e86d310e2404be30947f74774f1b0ace Diff: https://reviews.apache.org/r/71996/diff/1/ Testing --- `make check` Thanks, Benjamin Bannier
Review Request 71987: Disabled `DefaultExecutorTest.DomainSockets` on non-Linux platforms.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/71987/ --- Review request for mesos and Benno Evers. Repository: mesos Description --- While the interfaces are available on all platforms, currently domain sockets for executor communication are only available on Linux. This patch ifdef's the code away on non-Linux platforms; this is consistent with how we disable tests on non-Linux platforms elsewhere where we never use a test filter. Diffs - src/tests/default_executor_tests.cpp 6c71b3c2231d3da77d2fb793cb2bbc38e02cef24 Diff: https://reviews.apache.org/r/71987/diff/1/ Testing --- `make check` on macos Thanks, Benjamin Bannier
Review Request 71986: Removed redundant calls to `c_str` flagged by mesos-tidy.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/71986/ --- Review request for mesos and Benno Evers. Repository: mesos Description --- These calls were flagged by the upstream `readability-redundant-string-cstr` check. Diffs - src/linux/systemd.cpp d9cb5662cbf8e4d3531518af8bb633d9bb12d038 Diff: https://reviews.apache.org/r/71986/diff/1/ Testing --- confirmed that `clang-tidy` does not report anything from these lines anymore Thanks, Benjamin Bannier
Review Request 71985: Properly initialized dummy variable.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/71985/ --- Review request for mesos and Benno Evers. Repository: mesos Description --- Since `Try` does not have a default constructor we need to provide a dummy value. Diffs - src/slave/main.cpp 9e40743a9d379d5b1ff9f97372826d4296167ce3 Diff: https://reviews.apache.org/r/71985/diff/1/ Testing --- `make check` on macos Thanks, Benjamin Bannier
Re: Review Request 71977: Added systemd support to domain socket agent flag.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/71977/#review219224 --- Fix it, then Ship it! src/slave/main.cpp Lines 50 (patched) <https://reviews.apache.org/r/71977/#comment307382> Outdated `todo`? src/slave/main.cpp Lines 653 (patched) <https://reviews.apache.org/r/71977/#comment307383> This can be simplified to `socketFds->size() == 1`. - Benjamin Bannier On Jan. 10, 2020, 2:48 a.m., Benno Evers wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/71977/ > --- > > (Updated Jan. 10, 2020, 2:48 a.m.) > > > Review request for mesos and Benjamin Bannier. > > > Repository: mesos > > > Description > --- > > Added the ability to specify a unix domain socket > as `systemd:` for the `--domain_socket_location` > agent flag. > > This will instruct the agent to expect the domain socket > being passed by systemd with the specified name. > > > Diffs > - > > src/slave/flags.cpp 50b09cf3b0cf89568a45afd3fe89fa16c6a79222 > src/slave/main.cpp fd58637cd680291e6794bcdb0655603bb97744c7 > > > Diff: https://reviews.apache.org/r/71977/diff/1/ > > > Testing > --- > > > Thanks, > > Benno Evers > >
Re: Review Request 71816: Added domain socket-related flags to Mesos agent.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/71816/#review219223 --- src/slave/flags.hpp Lines 185-186 (patched) <https://reviews.apache.org/r/71816/#comment307381> Let's also document these flags in `docs/configuration/agent.md`. - Benjamin Bannier On Jan. 10, 2020, 2:43 a.m., Benno Evers wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/71816/ > --- > > (Updated Jan. 10, 2020, 2:43 a.m.) > > > Review request for mesos and Benjamin Bannier. > > > Bugs: MESOS-10035 > https://issues.apache.org/jira/browse/MESOS-10035 > > > Repository: mesos > > > Description > --- > > Added two new flags 'http_executor_domain_sockets' and > 'domain_socket_location' to the agent, along with minimal > logic to pass the relevant values on to executors. > > They are used to control whether the agent should > provide a domain socket to HTTP executors and the > location of the socket on the host filesystem, respectively. > > > Diffs > - > > src/slave/constants.hpp 721afe13ff590122878a8ead62c66f89e8549e91 > src/slave/flags.hpp 3c5ffca62454c340656cfc89c53a198757815794 > src/slave/flags.cpp 50b09cf3b0cf89568a45afd3fe89fa16c6a79222 > src/slave/main.cpp fd58637cd680291e6794bcdb0655603bb97744c7 > src/slave/slave.cpp 3839a120446339fea8aa857f431a2dba28ed4002 > > > Diff: https://reviews.apache.org/r/71816/diff/3/ > > > Testing > --- > > > Thanks, > > Benno Evers > >
Re: Review Request 71976: Added support for systemd socket activation API.
---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/71976/#review219222
---
Fix it, then Ship it!
src/linux/systemd.cpp
Lines 346 (patched)
<https://reviews.apache.org/r/71976/#comment307370>
Can you declare and initilize in one go?
```
int flags = ...
```
src/linux/systemd.cpp
Lines 351-355 (patched)
<https://reviews.apache.org/r/71976/#comment307371>
Let's also declare and init in one go. Using a ternary operator shouldn't
be too bad.
src/linux/systemd.cpp
Lines 384 (patched)
<https://reviews.apache.org/r/71976/#comment307376>
It might be helpful to rephrase the `Error` so we can show `*listenPidEnv`
in quotes.
src/linux/systemd.cpp
Lines 385 (patched)
<https://reviews.apache.org/r/71976/#comment307373>
No period at the end of this `Error`.
src/linux/systemd.cpp
Lines 402 (patched)
<https://reviews.apache.org/r/71976/#comment307377>
It might be helpful to rephrase the `Error` so we can show `*listenPidEnv`
in quotes.
src/linux/systemd.cpp
Lines 403 (patched)
<https://reviews.apache.org/r/71976/#comment307374>
No period at the end of this `Error`.
src/linux/systemd.cpp
Lines 408 (patched)
<https://reviews.apache.org/r/71976/#comment307375>
Remove period.
src/linux/systemd.cpp
Lines 414 (patched)
<https://reviews.apache.org/r/71976/#comment307378>
Can we add context here which fd we couldn't set cloexec for?
```
return Error("Could not set CLOEXEC for filedescriptor ...: + "
cloexec.error());
```
src/linux/systemd.cpp
Lines 419 (patched)
<https://reviews.apache.org/r/71976/#comment307372>
Let's add spaces around `+` and a newline before `return`.
src/linux/systemd.cpp
Lines 458 (patched)
<https://reviews.apache.org/r/71976/#comment307380>
Can you document somewhere very prominently that this function is not safe
to call from multiple threads since it mutates env?
src/linux/systemd.cpp
Lines 460-462 (patched)
<https://reviews.apache.org/r/71976/#comment307379>
Let's use `os::unsetenv` for "consistency".
- Benjamin Bannier
On Jan. 10, 2020, 2:48 a.m., Benno Evers wrote:
>
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/71976/
> -------
>
> (Updated Jan. 10, 2020, 2:48 a.m.)
>
>
> Review request for mesos, Benjamin Bannier and Benjamin Mahler.
>
>
> Repository: mesos
>
>
> Description
> ---
>
> Added support for the systemd socket activation api,
> that allows systemd to pass listening file descriptors
> to a given service.
>
>
> Diffs
> -
>
> src/linux/systemd.hpp f760e2c02683f0169ba5d3b0a3da53f66a7f3d91
> src/linux/systemd.cpp 8126b31a542b62ce51cc6e0f6372986bffcc948b
>
>
> Diff: https://reviews.apache.org/r/71976/diff/1/
>
>
> Testing
> ---
>
>
> Thanks,
>
> Benno Evers
>
>
Re: Review Request 71833: Created unix domain socket on agent startup.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/71833/#review219213 --- Fix it, then Ship it! src/common/domain_sockets.hpp Lines 31 (patched) <https://reviews.apache.org/r/71833/#comment307325> Can you get rid of the `using` decl and use the FQN instead? src/common/domain_sockets.hpp Lines 39 (patched) <https://reviews.apache.org/r/71833/#comment307326> Can you get rid of this variable and instead directly return `*socket`? src/common/domain_sockets.hpp Lines 80-81 (patched) <https://reviews.apache.org/r/71833/#comment307327> Are we not returning an `Error` here on purpose? Without context that would seem preferable. If we do indeed want to allow this to fail we should add a comment explaining the reasoning. src/slave/main.cpp Lines 59-61 (original), 59-62 (patched) <https://reviews.apache.org/r/71833/#comment307328> Let's keep these sorted. src/slave/main.cpp Lines 111 (patched) <https://reviews.apache.org/r/71833/#comment307329> This looks unused. src/slave/main.cpp Line 352 (original), 358 (patched) <https://reviews.apache.org/r/71833/#comment307330> Reuse a generic constant instead of magic `108`. src/slave/main.cpp Lines 629 (patched) <https://reviews.apache.org/r/71833/#comment307331> Can you init this with `None`? src/slave/main.cpp Lines 633 (patched) <https://reviews.apache.org/r/71833/#comment307332> You can use `CHECK_SOME` here. src/tests/cluster.cpp Lines 124 (patched) <https://reviews.apache.org/r/71833/#comment307333> This looks unused (and sorted incorrectly). src/tests/cluster.cpp Lines 614 (patched) <https://reviews.apache.org/r/71833/#comment307335> Init with `None`. src/tests/cluster.cpp Lines 618 (patched) <https://reviews.apache.org/r/71833/#comment307334> `CHECK_SOME` - Benjamin Bannier On Jan. 10, 2020, 2:46 a.m., Benno Evers wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/71833/ > --- > > (Updated Jan. 10, 2020, 2:46 a.m.) > > > Review request for mesos and Benjamin Bannier. > > > Bugs: MESOS-10036 > https://issues.apache.org/jira/browse/MESOS-10036 > > > Repository: mesos > > > Description > --- > > Added agent code to check whether domain socket support is > enabled, and if so to create or open the socket at > MESOS_DOMAIN_SOCKET_LOCATION. > > > Diffs > - > > src/Makefile.am 47ad1bd27e155f0193aafa956df0bd43baafd348 > src/common/domain_sockets.hpp PRE-CREATION > src/local/local.cpp 8ff43618f06463b4ba86b64b25e5de692e406448 > src/slave/main.cpp fd58637cd680291e6794bcdb0655603bb97744c7 > src/slave/slave.hpp 77b5bc0082c6bb73fbd48a2ebe812629921645cb > src/slave/slave.cpp 3839a120446339fea8aa857f431a2dba28ed4002 > src/tests/cluster.cpp f7bc882a644ec65710ada3d15507e1d4c5ba06f7 > src/tests/mock_slave.cpp 71be957884ea88258ef37e60649e3947e89b12d0 > > > Diff: https://reviews.apache.org/r/71833/diff/5/ > > > Testing > --- > > > Thanks, > > Benno Evers > >
Re: Review Request 71816: Added domain socket-related flags to Mesos agent.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/71816/#review219212 --- Fix it, then Ship it! Forgot to attach these comments to my latest FixIt/ShipIt. src/slave/flags.cpp Lines 991 (patched) <https://reviews.apache.org/r/71816/#comment307323> Can you put the e.g., single quotes? src/slave/flags.cpp Lines 995 (patched) <https://reviews.apache.org/r/71816/#comment307322> Use the constant introduced elsewhere here as well. src/slave/slave.cpp Lines 11095 (patched) <https://reviews.apache.org/r/71816/#comment307324> Ditto. - Benjamin Bannier On Jan. 10, 2020, 2:43 a.m., Benno Evers wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/71816/ > --- > > (Updated Jan. 10, 2020, 2:43 a.m.) > > > Review request for mesos and Benjamin Bannier. > > > Bugs: MESOS-10035 > https://issues.apache.org/jira/browse/MESOS-10035 > > > Repository: mesos > > > Description > --- > > Added two new flags 'http_executor_domain_sockets' and > 'domain_socket_location' to the agent, along with minimal > logic to pass the relevant values on to executors. > > They are used to control whether the agent should > provide a domain socket to HTTP executors and the > location of the socket on the host filesystem, respectively. > > > Diffs > - > > src/slave/constants.hpp 721afe13ff590122878a8ead62c66f89e8549e91 > src/slave/flags.hpp 3c5ffca62454c340656cfc89c53a198757815794 > src/slave/flags.cpp 50b09cf3b0cf89568a45afd3fe89fa16c6a79222 > src/slave/main.cpp fd58637cd680291e6794bcdb0655603bb97744c7 > src/slave/slave.cpp 3839a120446339fea8aa857f431a2dba28ed4002 > > > Diff: https://reviews.apache.org/r/71816/diff/3/ > > > Testing > --- > > > Thanks, > > Benno Evers > >
Re: Review Request 71816: Added domain socket-related flags to Mesos agent.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/71816/#review219210 --- Fix it, then Ship it! src/slave/main.cpp Lines 348 (patched) <https://reviews.apache.org/r/71816/#comment307319> `domain_socket_location` is an `Option` and this does not compile. src/slave/main.cpp Lines 352 (patched) <https://reviews.apache.org/r/71816/#comment307320> Ditto. src/slave/main.cpp Lines 354 (patched) <https://reviews.apache.org/r/71816/#comment307321> Ditto. - Benjamin Bannier On Jan. 10, 2020, 2:43 a.m., Benno Evers wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/71816/ > --- > > (Updated Jan. 10, 2020, 2:43 a.m.) > > > Review request for mesos and Benjamin Bannier. > > > Bugs: MESOS-10035 > https://issues.apache.org/jira/browse/MESOS-10035 > > > Repository: mesos > > > Description > --- > > Added two new flags 'http_executor_domain_sockets' and > 'domain_socket_location' to the agent, along with minimal > logic to pass the relevant values on to executors. > > They are used to control whether the agent should > provide a domain socket to HTTP executors and the > location of the socket on the host filesystem, respectively. > > > Diffs > - > > src/slave/constants.hpp 721afe13ff590122878a8ead62c66f89e8549e91 > src/slave/flags.hpp 3c5ffca62454c340656cfc89c53a198757815794 > src/slave/flags.cpp 50b09cf3b0cf89568a45afd3fe89fa16c6a79222 > src/slave/main.cpp fd58637cd680291e6794bcdb0655603bb97744c7 > src/slave/slave.cpp 3839a120446339fea8aa857f431a2dba28ed4002 > > > Diff: https://reviews.apache.org/r/71816/diff/3/ > > > Testing > --- > > > Thanks, > > Benno Evers > >
Re: Review Request 71815: Made the default executors connect via domain sockets if available.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/71815/#review219209 --- Fix it, then Ship it! src/executor/executor.cpp Lines 243 (patched) <https://reviews.apache.org/r/71815/#comment307317> Can you introduce a constant for this `108` and the one below? We should add documentation where this value comes from to that constant. src/tests/command_executor_tests.cpp Lines 511-512 (patched) <https://reviews.apache.org/r/71815/#comment307318> These flags are only present and active later in the chain. If you have time you could reorder the patches so we get a clean bisect, but I don't think that this is critical. - Benjamin Bannier On Jan. 10, 2020, 2:42 a.m., Benno Evers wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/71815/ > --- > > (Updated Jan. 10, 2020, 2:42 a.m.) > > > Review request for mesos and Benjamin Bannier. > > > Bugs: MESOS-10039 > https://issues.apache.org/jira/browse/MESOS-10039 > > > Repository: mesos > > > Description > --- > > Added support for the `MESOS_DOMAIN_SOCKET` environment variable > to the default executor. If this variable is present, the executor > will use the specified domain socket to connect to the agent, > as opposed to the IP address encoded in the agent PID. > > > Diffs > - > > src/executor/executor.cpp dfa58206b65a6827ad4b958879c19a093a672e80 > src/launcher/default_executor.cpp 521494a2770192e39b5159624f057e01f2690c8a > src/tests/command_executor_tests.cpp > c7f7711a7b085bdacd0f16b16a6c85cc47e2045c > src/tests/default_executor_tests.cpp > 49c4e3b37bde848dc7a4fd02a1458c650a84a16f > > > Diff: https://reviews.apache.org/r/71815/diff/4/ > > > Testing > --- > > Ran the new test. > > > Thanks, > > Benno Evers > >
Re: Review Request 71814: Added support for new 'http+unix' URL scheme in libprocess.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/71814/#review219208 --- Ship it! Ship It! - Benjamin Bannier On Dec. 3, 2019, 7:27 p.m., Benno Evers wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/71814/ > --- > > (Updated Dec. 3, 2019, 7:27 p.m.) > > > Review request for mesos, Benjamin Bannier and Benjamin Mahler. > > > Bugs: MESOS-10039 > https://issues.apache.org/jira/browse/MESOS-10039 > > > Repository: mesos > > > Description > --- > > Added support for a new 'http+unix' URL scheme, to denote > resources accessible by HTTP connections over unix domain > sockets. > > The encoding looks like > > http+unix:/// > > optionally followed by query and fragment sections. > > > Diffs > - > > 3rdparty/libprocess/include/process/http.hpp > 0013850f07110d9b9dbdb9b0c8ac7001a82420d2 > 3rdparty/libprocess/src/http.cpp b487ce214128193a3443f4e62a5af24205cbd399 > > > Diff: https://reviews.apache.org/r/71814/diff/3/ > > > Testing > --- > > > Thanks, > > Benno Evers > >
Re: Review Request 71947: Handled embedded null bytes in abstract domain socket names.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/71947/#review219207 --- Ship it! Ship It! - Benjamin Bannier On Jan. 10, 2020, 2:37 a.m., Benno Evers wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/71947/ > --- > > (Updated Jan. 10, 2020, 2:37 a.m.) > > > Review request for mesos, Benjamin Bannier and Benjamin Mahler. > > > Repository: mesos > > > Description > --- > > Address handling code for unix domain sockets assumed that > strlen() could be used to compute the name of a unix domain > socket, but that fails for unnamed sockets or in the case > where an abstract domain socket contains embedded null bytes. > > This patch adds a new `length` parameter to correctly handle > these special cases. > > > Diffs > - > > 3rdparty/libprocess/include/process/address.hpp > 749498056b52b916dfaf6c85f83ecc05e0d5406c > 3rdparty/libprocess/include/process/network.hpp > 8f48a4a78557309a9b1b00d7defb45eed454b077 > > > Diff: https://reviews.apache.org/r/71947/diff/2/ > > > Testing > --- > > Ran existing unit tests and verified that the newly added `CHECK()` doesn't > trigger. > > > Thanks, > > Benno Evers > >
Re: Review Request 71947: Handled embedded null bytes in abstract domain socket names.
> On Jan. 6, 2020, 1:59 p.m., Benjamin Bannier wrote: > > 3rdparty/libprocess/include/process/address.hpp > > Line 232 (original), 247 (patched) > > <https://reviews.apache.org/r/71947/diff/1/?file=2191675#file2191675line247> > > > > For unnamed sockets we would now return an empty string here which > > seems leaky. What do you think about changing the return type of > > `Address::path` to e.g., `Option` instead and returning a `None` in > > that case? > > Benno Evers wrote: > I've thought about it and an empty string actually uniquely identifies > unnamed sockets. Pathname sockets always must have at least one character, > and the difference between an empty string and an abstract domain socket > whose name is a single null byte would be that the former has size 0, and the > latter has size 1. > > So I think as long as we document this correspondence, this should be > fine. SG, dropping. - Benjamin --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/71947/#review219127 --- On Jan. 10, 2020, 2:37 a.m., Benno Evers wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/71947/ > ------- > > (Updated Jan. 10, 2020, 2:37 a.m.) > > > Review request for mesos, Benjamin Bannier and Benjamin Mahler. > > > Repository: mesos > > > Description > --- > > Address handling code for unix domain sockets assumed that > strlen() could be used to compute the name of a unix domain > socket, but that fails for unnamed sockets or in the case > where an abstract domain socket contains embedded null bytes. > > This patch adds a new `length` parameter to correctly handle > these special cases. > > > Diffs > - > > 3rdparty/libprocess/include/process/address.hpp > 749498056b52b916dfaf6c85f83ecc05e0d5406c > 3rdparty/libprocess/include/process/network.hpp > 8f48a4a78557309a9b1b00d7defb45eed454b077 > > > Diff: https://reviews.apache.org/r/71947/diff/2/ > > > Testing > --- > > Ran existing unit tests and verified that the newly added `CHECK()` doesn't > trigger. > > > Thanks, > > Benno Evers > >
Re: Review Request 71971: Added special case to stout's handling of C++ attributes.
> On Jan. 8, 2020, 2:28 p.m., Benjamin Bannier wrote: > > 3rdparty/stout/include/stout/attributes.hpp > > Lines 52 (patched) > > <https://reviews.apache.org/r/71971/diff/1/?file=2199431#file2199431line53> > > > > This makes `STOUT_DEPRECATED` a noop on all current clang versions. We > > could add another `TODO` here to figure out what until which release clang > > emitted the warning (I did not see it with clang-10.0.0). > > Benno Evers wrote: > I think clangs behaviour here might be arguably correct, after re-reading > the timeline C++11 only came with the `noreturn` and `carries_dependency` > attributes, and `deprecated` was indeed only accepted in C++14. > > I think the correct way to handle this might be to introduce another case > like this: > > #elif defined(__clang__) && STOUT_HAS_CPP_ATTRIBUTE(deprecated) > # define STOUT_DEPRECATED __attribute__((deprecated)) > #endif > > What do you think? I think this is the better approach if it works. - Benjamin --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/71971/#review219174 --- On Jan. 8, 2020, 1:15 p.m., Benno Evers wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/71971/ > ------- > > (Updated Jan. 8, 2020, 1:15 p.m.) > > > Review request for mesos and Benjamin Bannier. > > > Repository: mesos > > > Description > --- > > Added a special case for the `[[deprecated]]` attribute to > handle configurations where clang claims to support this > attribute but also emits a warning that it should not be used. > > > Diffs > - > > 3rdparty/stout/include/stout/attributes.hpp > 54d438dca56d5346c58eb28eaeda7916a3a62076 > > > Diff: https://reviews.apache.org/r/71971/diff/1/ > > > Testing > --- > > Internal unit test run in progress. > > > Thanks, > > Benno Evers > >
Re: Review Request 71971: Added special case to stout's handling of C++ attributes.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/71971/#review219174 --- Fix it, then Ship it! 3rdparty/stout/include/stout/attributes.hpp Lines 47-48 (patched) <https://reviews.apache.org/r/71971/#comment307292> nit: can you put the period at the end of the sentence, after the closing paren? 3rdparty/stout/include/stout/attributes.hpp Lines 52 (patched) <https://reviews.apache.org/r/71971/#comment307293> This makes `STOUT_DEPRECATED` a noop on all current clang versions. We could add another `TODO` here to figure out what until which release clang emitted the warning (I did not see it with clang-10.0.0). - Benjamin Bannier On Jan. 8, 2020, 1:15 p.m., Benno Evers wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/71971/ > --- > > (Updated Jan. 8, 2020, 1:15 p.m.) > > > Review request for mesos and Benjamin Bannier. > > > Repository: mesos > > > Description > --- > > Added a special case for the `[[deprecated]]` attribute to > handle configurations where clang claims to support this > attribute but also emits a warning that it should not be used. > > > Diffs > - > > 3rdparty/stout/include/stout/attributes.hpp > 54d438dca56d5346c58eb28eaeda7916a3a62076 > > > Diff: https://reviews.apache.org/r/71971/diff/1/ > > > Testing > --- > > Internal unit test run in progress. > > > Thanks, > > Benno Evers > >
Re: Review Request 71961: Added deprecated absolute() function for backwards compatibility.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/71961/#review219142 --- Ship it! Ship It! - Benjamin Bannier On Jan. 7, 2020, 1:24 p.m., Benno Evers wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/71961/ > --- > > (Updated Jan. 7, 2020, 1:24 p.m.) > > > Review request for mesos, Benjamin Bannier and Benjamin Mahler. > > > Repository: mesos > > > Description > --- > > Added deprecated absolute() function for backwards compatibility. > > > Diffs > - > > 3rdparty/stout/include/stout/path.hpp > ba1f665ce94b9636d88a7ecce8643c56758f7b5c > > > Diff: https://reviews.apache.org/r/71961/diff/1/ > > > Testing > --- > > > Thanks, > > Benno Evers > >
Re: Review Request 71960: Added support for deprecated attribute to stout.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/71960/#review219141 --- Ship it! Ship It! - Benjamin Bannier On Jan. 7, 2020, 1:24 p.m., Benno Evers wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/71960/ > --- > > (Updated Jan. 7, 2020, 1:24 p.m.) > > > Review request for mesos, Benjamin Bannier and Benjamin Mahler. > > > Repository: mesos > > > Description > --- > > Added support for the [[deprecated]] attribute to stout. > > > Diffs > - > > 3rdparty/stout/include/stout/attributes.hpp > a02ee79e39e90c2fd7f2e9b43949606559ce9ccb > > > Diff: https://reviews.apache.org/r/71960/diff/1/ > > > Testing > --- > > > Thanks, > > Benno Evers > >
Re: Review Request 71538: Allowed disabling use of NVML headers.
> On Jan. 6, 2020, 2:19 p.m., Benno Evers wrote: > > configure.ac > > Lines 271 (patched) > > <https://reviews.apache.org/r/71538/diff/1/?file=2166425#file2166425line271> > > > > Why not just `--enable-nvml`/`--disable-nvml`? Good idea! - Benjamin --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/71538/#review219132 --- On Jan. 6, 2020, 3:43 p.m., Benjamin Bannier wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/71538/ > --- > > (Updated Jan. 6, 2020, 3:43 p.m.) > > > Review request for mesos, Gilbert Song and Kevin Klues. > > > Bugs: MESOS-9978 > https://issues.apache.org/jira/browse/MESOS-9978 > > > Repository: mesos > > > Description > --- > > NVML is distributed under a license which is probably not "free" which > can e.g., be aan obstacle when including Mesos using symbols from it in > certain distributions. > > This patch adds a configure flag to disable use of the NVML headers > completely. Note that with this flag disabled it is impossible to use > GPU isolation with the Mesos containerizer. > > > Diffs > - > > 3rdparty/CMakeLists.txt 23ef7c1e5c6fe1555b6f2458f2a858471783ae2a > 3rdparty/Makefile.am 0de005dad9775f9f7e1a77a242e3b345219c7ac3 > cmake/CompilationConfigure.cmake 089df917d0667b338624459bbcdac433598be304 > configure.ac 2bf3a34f8e43e22783aff1852094b6232e5e0608 > docs/configuration/autotools.md 577e79465ebf0d3c3a9b3626ccac15f1fe0aed8f > docs/configuration/cmake.md 554c3bf458bc85609225a19ad4843029772ab51c > src/CMakeLists.txt ef9382dc77d37fed344b7267119f3251acd3088a > src/Makefile.am 111c156c8a7abc5ece04779e8ac8879a30c22dbf > src/slave/containerizer/mesos/containerizer.cpp > dd940162b1e44dbdab1773894b2b33a260fdc8da > src/slave/containerizer/mesos/isolators/gpu/nvml.hpp > eb5c0b008abc651d826c399f4538ad8916469b86 > src/slave/containerizer/mesos/isolators/gpu/nvml.cpp > 48a5bf6720e5c9a9ceec24740d47c14f0ad097b7 > src/tests/environment.cpp c3596fb94020d2433f15e630516b1320875a9fa3 > > > Diff: https://reviews.apache.org/r/71538/diff/2/ > > > Testing > --- > > * tested toggle both ways in cmake and autotools build > * wasn't able to test test filter since I don't seem to have access to a dev > machine with GPUs > > > Thanks, > > Benjamin Bannier > >
Re: Review Request 71538: Allowed disabling use of NVML headers.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/71538/ --- (Updated Jan. 6, 2020, 3:43 p.m.) Review request for mesos, Gilbert Song and Kevin Klues. Changes --- Rename flags, fix issue with cmake-3.16 Bugs: MESOS-9978 https://issues.apache.org/jira/browse/MESOS-9978 Repository: mesos Description --- NVML is distributed under a license which is probably not "free" which can e.g., be aan obstacle when including Mesos using symbols from it in certain distributions. This patch adds a configure flag to disable use of the NVML headers completely. Note that with this flag disabled it is impossible to use GPU isolation with the Mesos containerizer. Diffs (updated) - 3rdparty/CMakeLists.txt 23ef7c1e5c6fe1555b6f2458f2a858471783ae2a 3rdparty/Makefile.am 0de005dad9775f9f7e1a77a242e3b345219c7ac3 cmake/CompilationConfigure.cmake 089df917d0667b338624459bbcdac433598be304 configure.ac 2bf3a34f8e43e22783aff1852094b6232e5e0608 docs/configuration/autotools.md 577e79465ebf0d3c3a9b3626ccac15f1fe0aed8f docs/configuration/cmake.md 554c3bf458bc85609225a19ad4843029772ab51c src/CMakeLists.txt ef9382dc77d37fed344b7267119f3251acd3088a src/Makefile.am 111c156c8a7abc5ece04779e8ac8879a30c22dbf src/slave/containerizer/mesos/containerizer.cpp dd940162b1e44dbdab1773894b2b33a260fdc8da src/slave/containerizer/mesos/isolators/gpu/nvml.hpp eb5c0b008abc651d826c399f4538ad8916469b86 src/slave/containerizer/mesos/isolators/gpu/nvml.cpp 48a5bf6720e5c9a9ceec24740d47c14f0ad097b7 src/tests/environment.cpp c3596fb94020d2433f15e630516b1320875a9fa3 Diff: https://reviews.apache.org/r/71538/diff/2/ Changes: https://reviews.apache.org/r/71538/diff/1-2/ Testing --- * tested toggle both ways in cmake and autotools build * wasn't able to test test filter since I don't seem to have access to a dev machine with GPUs Thanks, Benjamin Bannier
Review Request 71717: Made sure all targets are build for cmake `tests` target.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/71717/ --- Review request for mesos, Benno Evers and Till Toenshoff. Repository: mesos Description --- This target not only contains dependencies for the `test` target (which actually exercises the tests), but alls all build dependencies so we can make sure the targets still build. Diffs - src/tests/CMakeLists.txt 0a3c176d1bafc5445840146a1dd19a0825cab04d Diff: https://reviews.apache.org/r/71717/diff/1/ Testing --- * `make tests` * `make check` Thanks, Benjamin Bannier
Re: Review Request 71947: Handled embedded null bytes in abstract domain socket names.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/71947/#review219127 --- 3rdparty/libprocess/include/process/address.hpp Lines 224 (patched) <https://reviews.apache.org/r/71947/#comment307224> Let's include `` for `offsetof`. 3rdparty/libprocess/include/process/address.hpp Line 226 (original), 227-228 (patched) <https://reviews.apache.org/r/71947/#comment307226> This is not very clear to me. Can we give a very explicit example on how to obtain `_length`, or alternatively compute it ourself as suggested below? 3rdparty/libprocess/include/process/address.hpp Lines 238 (patched) <https://reviews.apache.org/r/71947/#comment307229> As discussed offline, let's add a `CHECK` here that `sun_path` is not empty. 3rdparty/libprocess/include/process/address.hpp Line 232 (original), 247 (patched) <https://reviews.apache.org/r/71947/#comment307227> For unnamed sockets we would now return an empty string here which seems leaky. What do you think about changing the return type of `Address::path` to e.g., `Option` instead and returning a `None` in that case? 3rdparty/libprocess/include/process/address.hpp Lines 275 (patched) <https://reviews.apache.org/r/71947/#comment307228> nit: Missing trailing dot. 3rdparty/libprocess/include/process/address.hpp Lines 318 (patched) <https://reviews.apache.org/r/71947/#comment307230> Does it make sense to pass an `Option` defaulted to `None` to make it clearer that `length` is only interesting in certain cases? In any case we need to do add documentation on the semantics of the parameters. - Benjamin Bannier On Jan. 3, 2020, 2:38 a.m., Benno Evers wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/71947/ > --- > > (Updated Jan. 3, 2020, 2:38 a.m.) > > > Review request for mesos, Benjamin Bannier and Benjamin Mahler. > > > Repository: mesos > > > Description > --- > > Address handling code for unix domain sockets assumed that > strlen() could be used to compute the name of a unix domain > socket, but that fails for unnamed sockets or in the case > where an abstract domain socket contains embedded null bytes. > > This patch adds a new `length` parameter to correctly handle > these special cases. > > > Diffs > - > > 3rdparty/libprocess/include/process/address.hpp > 749498056b52b916dfaf6c85f83ecc05e0d5406c > 3rdparty/libprocess/include/process/network.hpp > 8f48a4a78557309a9b1b00d7defb45eed454b077 > > > Diff: https://reviews.apache.org/r/71947/diff/1/ > > > Testing > --- > > Ran existing unit tests and verified that the newly added `CHECK()` doesn't > trigger. > > > Thanks, > > Benno Evers > >
Review Request 71934: Bumped site's rack to rack-1.16.12.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/71934/ --- Review request for mesos, Benno Evers and Vinod Kone. Repository: mesos Description --- This addresses CVE-2019-16782 which should not affect us. Diffs - site/Gemfile.lock 87d825c4e4056c33e0702b3c429a48b01cc1b035 Diff: https://reviews.apache.org/r/71934/diff/1/ Testing --- Confirmed that the website still builds. Thanks, Benjamin Bannier
Review Request 71916: Removed outdated check assuming no reservations with disks.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/71916/ --- Review request for mesos and Benno Evers. Repository: mesos Description --- Since `disk` resources can be re-reserved we need to allow for them to appear in `RESERVE` operations. The validity of the operation and the resources appearing in them are already check elsewhere and the check removed here was intended more as a sanity check/avenue to create a better error message. With re-reservations it is now incorrect. Diffs - src/master/validation.cpp c5fbbdd55e4a217c756e2ece3f8fd6ed5e88048f Diff: https://reviews.apache.org/r/71916/diff/1/ Testing --- * `make check` * tested re-reserving a `disk` resource in a test cluster with authorization enabled Thanks, Benjamin Bannier
Re: Review Request 71882: Added a stout function to compute relative paths.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/71882/ --- (Updated Dec. 9, 2019, 5:48 p.m.) Review request for mesos and Benno Evers. Bugs: MESOS-10062 https://issues.apache.org/jira/browse/MESOS-10062 Repository: mesos Description --- Added a stout function to compute relative paths. Diffs (updated) - 3rdparty/stout/include/stout/path.hpp ba1f665ce94b9636d88a7ecce8643c56758f7b5c 3rdparty/stout/tests/path_tests.cpp 19dd910a534040468aeb48f15ebdf56dff32bc15 Diff: https://reviews.apache.org/r/71882/diff/2/ Changes: https://reviews.apache.org/r/71882/diff/1-2/ Testing --- `make check` Thanks, Benjamin Bannier
Re: Review Request 71882: Added a stout function to compute relative paths.
> On Dec. 9, 2019, 1:09 p.m., Benno Evers wrote: > > 3rdparty/stout/include/stout/path.hpp > > Lines 21 (patched) > > <https://reviews.apache.org/r/71882/diff/1/?file=2182174#file2182174line21> > > > > This doesn't seem to be necessary anymore? This is required for e.g., `CHECK_EQ` (now). > On Dec. 9, 2019, 1:09 p.m., Benno Evers wrote: > > 3rdparty/stout/include/stout/path.hpp > > Lines 551 (patched) > > <https://reviews.apache.org/r/71882/diff/1/?file=2182174#file2182174line551> > > > > I found the description below easier to parse: > > > > > Relative paths can only be computed between paths which are either > > both absolute or both relative. Fixed. > On Dec. 9, 2019, 1:09 p.m., Benno Evers wrote: > > 3rdparty/stout/include/stout/path.hpp > > Lines 590 (patched) > > <https://reviews.apache.org/r/71882/diff/1/?file=2182174#file2182174line590> > > > > How about > > > > the range of `base` > > > > the `base` range > > > > the range `[base.begin(), base.end())` > > > > to avoid the awkward backtick-single-quote? Went with your first suggestion. - Benjamin --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/71882/#review218976 --- On Dec. 9, 2019, 5:48 p.m., Benjamin Bannier wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/71882/ > --- > > (Updated Dec. 9, 2019, 5:48 p.m.) > > > Review request for mesos and Benno Evers. > > > Bugs: MESOS-10062 > https://issues.apache.org/jira/browse/MESOS-10062 > > > Repository: mesos > > > Description > --- > > Added a stout function to compute relative paths. > > > Diffs > - > > 3rdparty/stout/include/stout/path.hpp > ba1f665ce94b9636d88a7ecce8643c56758f7b5c > 3rdparty/stout/tests/path_tests.cpp > 19dd910a534040468aeb48f15ebdf56dff32bc15 > > > Diff: https://reviews.apache.org/r/71882/diff/2/ > > > Testing > --- > > `make check` > > > Thanks, > > Benjamin Bannier > >
Re: Review Request 71878: Added iteration support to stout's Path.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/71878/ --- (Updated Dec. 9, 2019, 5:48 p.m.) Review request for mesos and Benno Evers. Changes --- Correct changeset Bugs: MESOS-10062 https://issues.apache.org/jira/browse/MESOS-10062 Repository: mesos Description --- Added iteration support to stout's Path. Diffs (updated) - 3rdparty/stout/include/stout/path.hpp ba1f665ce94b9636d88a7ecce8643c56758f7b5c 3rdparty/stout/tests/path_tests.cpp 19dd910a534040468aeb48f15ebdf56dff32bc15 Diff: https://reviews.apache.org/r/71878/diff/3/ Changes: https://reviews.apache.org/r/71878/diff/2-3/ Testing --- `make check` Thanks, Benjamin Bannier
Re: Review Request 71878: Added iteration support to stout's Path.
> On Dec. 9, 2019, 3:34 p.m., Benno Evers wrote:
> > 3rdparty/stout/include/stout/path.hpp
> > Lines 388 (patched)
> > <https://reviews.apache.org/r/71878/diff/1/?file=2182150#file2182150line388>
> >
> > This feels a bit more "common" to me:
> >
> > struct PathComponentIterator {
> > // ..
> > };
> >
> > typedef const PathComponentIterator const_iterator;
> >
> > What do you think?
I'd vote to not do this as we do not need it.
> On Dec. 9, 2019, 3:34 p.m., Benno Evers wrote:
> > 3rdparty/stout/include/stout/path.hpp
> > Lines 389 (patched)
> > <https://reviews.apache.org/r/71878/diff/1/?file=2182150#file2182150line389>
> >
> > Using `std::iterator` is going to be deprecated in C++17, apparently
> > the preferred alternative is to just create typedefs for `value_type`, etc.
> > in the iterator class directly.
I'll do that since it also documents more what is happening in a more obvious
way.
> On Dec. 9, 2019, 3:34 p.m., Benno Evers wrote:
> > 3rdparty/stout/include/stout/path.hpp
> > Lines 437 (patched)
> > <https://reviews.apache.org/r/71878/diff/1/?file=2182150#file2182150line437>
> >
> > I think this needs to be a loop, consider e.g. `/home/foobar`
That would be path normalization which I didn't want to do in this class as it
has a number of other complications. For a path like `/home/foo//bar` we would
like to generate elements `{"home", "foo", "", "bar"}`. I'll clean up remnants
of my attempts in this class here and in tests.
> On Dec. 9, 2019, 3:34 p.m., Benno Evers wrote:
> > 3rdparty/stout/include/stout/path.hpp
> > Lines 454 (patched)
> > <https://reviews.apache.org/r/71878/diff/1/?file=2182150#file2182150line454>
> >
> > Should we also check `path == other.path`? Or maybe even `CHECK(path ==
> > other.path)`?
I'll end the hard `CHECK` once I have gotten rid of the end sentinel.
> On Dec. 9, 2019, 3:34 p.m., Benno Evers wrote:
> > 3rdparty/stout/include/stout/path.hpp
> > Lines 469 (patched)
> > <https://reviews.apache.org/r/71878/diff/1/?file=2182150#file2182150line469>
> >
> > I don't completely understand why we need a sentinel, isn't
> > `path->end()` suitable for everything we need to do?
I removed as per our offline discussion.
- Benjamin
---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/71878/#review218978
---
On Dec. 9, 2019, 5:38 p.m., Benjamin Bannier wrote:
>
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/71878/
> ---
>
> (Updated Dec. 9, 2019, 5:38 p.m.)
>
>
> Review request for mesos and Benno Evers.
>
>
> Bugs: MESOS-10062
> https://issues.apache.org/jira/browse/MESOS-10062
>
>
> Repository: mesos
>
>
> Description
> ---
>
> Added iteration support to stout's Path.
>
>
> Diffs
> -
>
> 3rdparty/stout/include/stout/path.hpp
> ba1f665ce94b9636d88a7ecce8643c56758f7b5c
> 3rdparty/stout/tests/path_tests.cpp
> 19dd910a534040468aeb48f15ebdf56dff32bc15
>
>
> Diff: https://reviews.apache.org/r/71878/diff/2/
>
>
> Testing
> ---
>
> `make check`
>
>
> Thanks,
>
> Benjamin Bannier
>
>
Re: Review Request 71878: Added iteration support to stout's Path.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/71878/ --- (Updated Dec. 9, 2019, 5:38 p.m.) Review request for mesos and Benno Evers. Bugs: MESOS-10062 https://issues.apache.org/jira/browse/MESOS-10062 Repository: mesos Description --- Added iteration support to stout's Path. Diffs (updated) - 3rdparty/stout/include/stout/path.hpp ba1f665ce94b9636d88a7ecce8643c56758f7b5c 3rdparty/stout/tests/path_tests.cpp 19dd910a534040468aeb48f15ebdf56dff32bc15 Diff: https://reviews.apache.org/r/71878/diff/2/ Changes: https://reviews.apache.org/r/71878/diff/1-2/ Testing --- `make check` Thanks, Benjamin Bannier
Re: Review Request 71852: Used CMake's automatic parallelization in mesos-tidy setup.
> On Dec. 6, 2019, 12:51 p.m., Benno Evers wrote: > > support/mesos-tidy/entrypoint.sh > > Line 38 (original), 38 (patched) > > <https://reviews.apache.org/r/71852/diff/1/?file=2181662#file2181662line38> > > > > From > > > > https://cmake.org/cmake/help/v3.12/release/3.12.html#command-line > > > > it sounds like the `--parallel` option still wants a parameter to set > > the number of cores? If that is not required, maybe you could add a link to > > the cmake documentation in the commit message? The release notes look incorrect to me. The documentation for this flag is here, https://cmake.org/cmake/help/latest/manual/cmake.1.html#index-0-envvar:CMAKE_BUILD_PARALLEL_LEVEL. While the documentation states when no explicit number is given the default parallelism level of the choosen build tool would be used, this still built fast for me (even though we build with `make` which by default does not parallelize). I added an explicit setting to make this less surprising ;) - Benjamin --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/71852/#review218955 ------- On Dec. 6, 2019, 5:23 p.m., Benjamin Bannier wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/71852/ > --- > > (Updated Dec. 6, 2019, 5:23 p.m.) > > > Review request for mesos and Benno Evers. > > > Repository: mesos > > > Description > --- > > Since the `--parallel` flag for `cmake build` is only supported in more > recent CMake versions we bump the installed version to the latest > release. > > > Diffs > - > > support/mesos-tidy/Dockerfile 7d17a82c023c614016e771c305e3a114731eb832 > support/mesos-tidy/entrypoint.sh 7d2225d8f75998ee13acf0bd57c9483dfc7acd98 > > > Diff: https://reviews.apache.org/r/71852/diff/2/ > > > Testing > --- > > Built a local image and tested it with a modified `support/mesos-tidy.sh`. > > > Thanks, > > Benjamin Bannier > >
Re: Review Request 71852: Used CMake's automatic parallelization in mesos-tidy setup.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/71852/ --- (Updated Dec. 6, 2019, 5:23 p.m.) Review request for mesos and Benno Evers. Repository: mesos Description --- Since the `--parallel` flag for `cmake build` is only supported in more recent CMake versions we bump the installed version to the latest release. Diffs (updated) - support/mesos-tidy/Dockerfile 7d17a82c023c614016e771c305e3a114731eb832 support/mesos-tidy/entrypoint.sh 7d2225d8f75998ee13acf0bd57c9483dfc7acd98 Diff: https://reviews.apache.org/r/71852/diff/2/ Changes: https://reviews.apache.org/r/71852/diff/1-2/ Testing --- Built a local image and tested it with a modified `support/mesos-tidy.sh`. Thanks, Benjamin Bannier
Re: Review Request 71833: Created unix domain socket on agent startup.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/71833/#review218939 --- src/slave/main.cpp Lines 614 (patched) <https://reviews.apache.org/r/71833/#comment306915> Same comments as in `tests/cluster.cpp` below. src/tests/cluster.cpp Lines 613-614 (patched) <https://reviews.apache.org/r/71833/#comment306918> Either implement this `TODO` before landing or explicitly reference the code you added above. src/tests/cluster.cpp Lines 630 (patched) <https://reviews.apache.org/r/71833/#comment306917> There is another, currently unhandled branch where `domain_socket_location` exists, but is not a socket. Either we handle that case as well, or just check if _something_ exists where we'd expect the socket and let this fail later. src/tests/cluster.cpp Lines 653-654 (patched) <https://reviews.apache.org/r/71833/#comment306914> This should be implemented when this patch lands, or be less terse. src/tests/cluster.cpp Lines 656 (patched) <https://reviews.apache.org/r/71833/#comment306913> Is this outdated? I think we want to log whether we created a new file or used an existing socket, and indicate whether we successfully did `bind` a socket at that path. - Benjamin Bannier On Dec. 3, 2019, 7:30 p.m., Benno Evers wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/71833/ > --- > > (Updated Dec. 3, 2019, 7:30 p.m.) > > > Review request for mesos and Benjamin Bannier. > > > Bugs: MESOS-10036 > https://issues.apache.org/jira/browse/MESOS-10036 > > > Repository: mesos > > > Description > --- > > Added agent code to check whether domain socket support is > enabled, and if so to create or open the socket at > MESOS_DOMAIN_SOCKET_LOCATION. > > > Diffs > - > > src/local/local.cpp 8ff43618f06463b4ba86b64b25e5de692e406448 > src/slave/main.cpp fd58637cd680291e6794bcdb0655603bb97744c7 > src/slave/slave.hpp 77b5bc0082c6bb73fbd48a2ebe812629921645cb > src/slave/slave.cpp 3839a120446339fea8aa857f431a2dba28ed4002 > src/tests/cluster.cpp f7bc882a644ec65710ada3d15507e1d4c5ba06f7 > src/tests/mock_slave.cpp 71be957884ea88258ef37e60649e3947e89b12d0 > > > Diff: https://reviews.apache.org/r/71833/diff/4/ > > > Testing > --- > > > Thanks, > > Benno Evers > >
Review Request 71878: Added iteration support to stout's Path.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/71878/ --- Review request for mesos and Benno Evers. Bugs: MESOS-10062 https://issues.apache.org/jira/browse/MESOS-10062 Repository: mesos Description --- Added iteration support to stout's Path. Diffs - 3rdparty/stout/include/stout/path.hpp ba1f665ce94b9636d88a7ecce8643c56758f7b5c 3rdparty/stout/tests/path_tests.cpp 19dd910a534040468aeb48f15ebdf56dff32bc15 Diff: https://reviews.apache.org/r/71878/diff/1/ Testing --- `make check` Thanks, Benjamin Bannier
Review Request 71879: Renamed stout's path-related absolute functions to is_absolute.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/71879/ --- Review request for mesos and Benno Evers. Bugs: MESOS-10062 https://issues.apache.org/jira/browse/MESOS-10062 Repository: mesos Description --- Renamed stout's path-related absolute functions to is_absolute. Diffs - 3rdparty/stout/include/stout/internal/windows/longpath.hpp 499eef30a3d16ac1f6c2e3334ef773e91e987a45 3rdparty/stout/include/stout/os/posix/copyfile.hpp 631babc17fff978b4e2048e5041a9f8db9b282b2 3rdparty/stout/include/stout/os/windows/copyfile.hpp 34723bce9a151582de481d63a865509d4d29c02c 3rdparty/stout/include/stout/path.hpp ba1f665ce94b9636d88a7ecce8643c56758f7b5c 3rdparty/stout/tests/path_tests.cpp 19dd910a534040468aeb48f15ebdf56dff32bc15 Diff: https://reviews.apache.org/r/71879/diff/1/ Testing --- `make check` Thanks, Benjamin Bannier
Review Request 71880: Renamed stout's path-related absolute functions to is_absolute.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/71880/ --- Review request for mesos and Benno Evers. Bugs: MESOS-10062 https://issues.apache.org/jira/browse/MESOS-10062 Repository: mesos Description --- Renamed stout's path-related absolute functions to is_absolute. Diffs - src/docker/docker.cpp d13869db7c9ead178cb52eec5d0700931f987161 src/hdfs/hdfs.cpp 3947b69c965cfb7289f563202288ea6e4979d942 src/resource_provider/storage/uri_disk_profile_adaptor.hpp 027ceaac7cc3db100186aebf2c3ebfd1e06137d9 src/slave/container_loggers/logrotate.hpp c6dbd61a5083dbb5c52bfb514ea7979a7a51158b src/slave/containerizer/fetcher.cpp 3a5a74bf4e64eeaed161bb2e7875e185f2271aef src/slave/containerizer/mesos/isolators/docker/volume/isolator.cpp e4a19c471b3b5442e5fdc15b31b1799a6638a2d0 src/slave/containerizer/mesos/isolators/filesystem/linux.cpp ab192305b53c829fbf6cbf6367ce1686507f9c3d src/slave/containerizer/mesos/isolators/posix/disk.cpp 4d8047b1f5966428911d095c734a0155b1f5148a src/slave/containerizer/mesos/isolators/volume/host_path.cpp a20f7ac527a0ae68a03ec7095c09def5b34f5109 src/slave/containerizer/mesos/isolators/volume/image.cpp b1112529ac1abe01368fda501a675e7b3e6388ef src/slave/containerizer/mesos/isolators/volume/sandbox_path.cpp 45bd143bbc829c105dc1f1c8c88f0168aeaf53a0 src/slave/containerizer/mesos/isolators/volume/secret.cpp 91d701376069a65046e487a5070325e3bc61deb8 src/slave/containerizer/mesos/launch.cpp 1f8dc1ae2e3d750d56973f9efc1ccbd4b1e50eba src/slave/paths.cpp 5afcc7e104aa4232554d54e2f290aede4ebba3c1 src/tests/storage_local_resource_provider_tests.cpp d4a73c34afb034ac35a3e305e603c61a03b99b5d src/uri/fetchers/docker.cpp 1aa3def8362269e94c7f0bb05f9aa10049bf4af4 Diff: https://reviews.apache.org/r/71880/diff/1/ Testing --- `make check` Thanks, Benjamin Bannier
Review Request 71881: Allowed specifying path separator in a `path::join` overload.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/71881/ --- Review request for mesos and Benno Evers. Bugs: MESOS-10062 https://issues.apache.org/jira/browse/MESOS-10062 Repository: mesos Description --- Allowed specifying path separator in a `path::join` overload. Diffs - 3rdparty/stout/include/stout/path.hpp ba1f665ce94b9636d88a7ecce8643c56758f7b5c Diff: https://reviews.apache.org/r/71881/diff/1/ Testing --- `make check` Thanks, Benjamin Bannier
Review Request 71882: Added a stout function to compute relative paths.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/71882/ --- Review request for mesos and Benno Evers. Bugs: MESOS-10062 https://issues.apache.org/jira/browse/MESOS-10062 Repository: mesos Description --- Added a stout function to compute relative paths. Diffs - 3rdparty/stout/include/stout/path.hpp ba1f665ce94b9636d88a7ecce8643c56758f7b5c 3rdparty/stout/tests/path_tests.cpp 19dd910a534040468aeb48f15ebdf56dff32bc15 Diff: https://reviews.apache.org/r/71882/diff/1/ Testing --- `make check` Thanks, Benjamin Bannier
Re: Review Request 71866: Added new chmod() function to stout.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/71866/#review218937 --- Fix it, then Ship it! 3rdparty/stout/include/stout/os/chmod.hpp Lines 18-26 (patched) <https://reviews.apache.org/r/71866/#comment306912> We need to add a test for `os::chown` (disabled on Windows for now). Since stout is a header-only library even a trivial test is better than none. 3rdparty/stout/include/stout/os/posix/chmod.hpp Lines 29 (patched) <https://reviews.apache.org/r/71866/#comment306911> Should we also include `sys/types.h` for `mode_t`? 3rdparty/stout/include/stout/os/posix/chmod.hpp Lines 34 (patched) <https://reviews.apache.org/r/71866/#comment306910> Let's return a `ErrnoError(errno)` here. Do we need to include `cstdio`/`stdio.h` for `::strerror`? - Benjamin Bannier On Dec. 3, 2019, 7:32 p.m., Benno Evers wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/71866/ > --- > > (Updated Dec. 3, 2019, 7:32 p.m.) > > > Review request for mesos, Benjamin Bannier and Benjamin Mahler. > > > Repository: mesos > > > Description > --- > > Added new functions `os::chmod()` and `os::posix::chmod()` that > are wrapping the POSIX `::chmod()` function. > > > Diffs > - > > 3rdparty/stout/include/stout/os/chmod.hpp PRE-CREATION > 3rdparty/stout/include/stout/os/posix/chmod.hpp PRE-CREATION > 3rdparty/stout/include/stout/os/windows/chmod.hpp PRE-CREATION > > > Diff: https://reviews.apache.org/r/71866/diff/1/ > > > Testing > --- > > > Thanks, > > Benno Evers > >
Review Request 71854: Handled `/api/v1` and /api/v1/executor` over agent executor socket.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/71854/ --- Review request for mesos and Benno Evers. Bugs: MESOS-10038 https://issues.apache.org/jira/browse/MESOS-10038 Repository: mesos Description --- This patch wires up the agent executor socket to handle calls for `/api/v1` and `/api/v1/executor` so executors can use domain sockets to communicate with the agent; executors use the latter to subscribe with the agent and the former to e.g., launch containers. Note that with this patch we now expose the full operator API over the agent's domain socket. Diffs - src/slave/slave.hpp 77b5bc0082c6bb73fbd48a2ebe812629921645cb src/slave/slave.cpp 3839a120446339fea8aa857f431a2dba28ed4002 Diff: https://reviews.apache.org/r/71854/diff/1/ Testing --- In both an SSL and non-SSL setup `make check` and also with tested with `LinuxFilesystemIsolatorMesosTest.ROOT_DomainSocketVariable` added earlier in this chain. The Mesos test harness with authenticate and authorize in SSL builds and permit all requests in non-SSL builds. Thanks, Benjamin Bannier
Review Request 71853: Constructed `HttpEvent` from owning instead of raw pointers.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/71853/ --- Review request for mesos and Benno Evers. Repository: mesos Description --- Constructed `HttpEvent` from owning instead of raw pointers. Diffs - 3rdparty/libprocess/include/process/event.hpp 6d7f8127cfe19e77ee35bcea7d881a4e5180a64e 3rdparty/libprocess/src/process.cpp 1de6da2d77f75fa228fbd20d4e0f46e7dcf91719 Diff: https://reviews.apache.org/r/71853/diff/1/ Testing --- `make check` Thanks, Benjamin Bannier
Review Request 71852: Used CMake's automatic parallelization in mesos-tidy setup.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/71852/ --- Review request for mesos and Benno Evers. Repository: mesos Description --- Since the `--parallel` flag for `cmake build` is only supported in more recent CMake versions we bump the installed version to the latest release. Diffs - support/mesos-tidy/Dockerfile 7d17a82c023c614016e771c305e3a114731eb832 support/mesos-tidy/entrypoint.sh 7d2225d8f75998ee13acf0bd57c9483dfc7acd98 Diff: https://reviews.apache.org/r/71852/diff/1/ Testing --- Built a local image and tested it with a modified `support/mesos-tidy.sh`. Thanks, Benjamin Bannier
Re: Review Request 71837: Dont reject the word 'WIP' in commit titles.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/71837/#review218824 --- Ship it! LGTM. - Benjamin Bannier On Nov. 27, 2019, 4:18 p.m., Benno Evers wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/71837/ > --- > > (Updated Nov. 27, 2019, 4:18 p.m.) > > > Review request for mesos and Benjamin Bannier. > > > Repository: mesos > > > Description > --- > > Change gitlint rules to not reject the word 'WIP' in commit > titles anymore, since we want to encourage placing this marker > as visible as possible for WIP commits, to prevent accidental > merging of a commit that was not considered ready by the > author. > > > Diffs > - > > support/gitlint cac9253323f5a9a99876a0e3bd6dd4acbc19d9c1 > > > Diff: https://reviews.apache.org/r/71837/diff/1/ > > > Testing > --- > > Created a commit containing the word `WIP` in the title. > > > Thanks, > > Benno Evers > >
Re: Review Request 71832: Added 'issocket()' function to stout.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/71832/#review218821 --- Ship it! Can you add a trivial patch for this (`issocket` for socket returns `true`, for a non-socket returns `false`). - Benjamin Bannier On Nov. 27, 2019, 12:43 p.m., Benno Evers wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/71832/ > --- > > (Updated Nov. 27, 2019, 12:43 p.m.) > > > Review request for mesos, Benjamin Bannier and Benjamin Mahler. > > > Bugs: MESOS-10036 > https://issues.apache.org/jira/browse/MESOS-10036 > > > Repository: mesos > > > Description > --- > > Added a new helper 'issocket()' that tells whether a given > file is a socket or not. > > > Diffs > - > > 3rdparty/stout/include/stout/os/posix/stat.hpp > 74e0c934baa29ec936e8011221a3da29dd67a465 > > > Diff: https://reviews.apache.org/r/71832/diff/1/ > > > Testing > --- > > Manual testing by using it in the subsequent patch. > > > Thanks, > > Benno Evers > >
Re: Review Request 71741: Updated operator API documention to use rereservation format.
> On Nov. 20, 2019, 3:41 p.m., Benno Evers wrote: > > docs/operator-http-api.md > > Line 1870 (original), 1872 (patched) > > <https://reviews.apache.org/r/71741/diff/2/?file=2174609#file2174609line1872> > > > > Maybe ask a native speaker for a second opinion, but "takes" sounds a > > bit weird to me. Maybe "accepts parameters"? > > > > Also, I would propose adding a short summary of what the call *does* > > with these parameters, instead of launching straight into backwards > > compatibility exceptions, something like > > > > ``` > > [...] and the target resources, and updates the reservations of > > `source` to be equal to the reservations of `target`. > > ``` `takes` was used by a couple of native speakers in this doc and looks fine to me. Thanks for the suggestion on describing effects, updated the doc. > On Nov. 20, 2019, 3:41 p.m., Benno Evers wrote: > > docs/operator-http-api.md > > Lines 1956 (patched) > > <https://reviews.apache.org/r/71741/diff/2/?file=2174609#file2174609line1956> > > > > Mentioning "multiple levels" here feels a bit like an implementation > > detail; from a users perspective we simply allow changing one role to > > another. (also "levels" are not present in this example anyways.) Good point. Dropping this whole sentence as we now already already above call out the requirement for `source` and `resources` to be identical apart from reservations. > On Nov. 20, 2019, 3:41 p.m., Benno Evers wrote: > > docs/operator-http-api.md > > Lines 1957 (patched) > > <https://reviews.apache.org/r/71741/diff/2/?file=2174609#file2174609line1957> > > > > s/fall/for/ Sentence was removed. - Benjamin ------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/71741/#review218721 --- On Nov. 22, 2019, 12:55 p.m., Benjamin Bannier wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/71741/ > --- > > (Updated Nov. 22, 2019, 12:55 p.m.) > > > Review request for mesos and Benno Evers. > > > Bugs: MESOS-9993 > https://issues.apache.org/jira/browse/MESOS-9993 > > > Repository: mesos > > > Description > --- > > Updated operator API documention to use rereservation format. > > > Diffs > - > > docs/operator-http-api.md 6cc367dabf3bec507e8d49af54d9aad9ac17471c > > > Diff: https://reviews.apache.org/r/71741/diff/3/ > > > Testing > --- > > Previewed in generated site > > > Thanks, > > Benjamin Bannier > >
Re: Review Request 71741: Updated operator API documention to use rereservation format.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/71741/ --- (Updated Nov. 22, 2019, 12:55 p.m.) Review request for mesos and Benno Evers. Bugs: MESOS-9993 https://issues.apache.org/jira/browse/MESOS-9993 Repository: mesos Description --- Updated operator API documention to use rereservation format. Diffs (updated) - docs/operator-http-api.md 6cc367dabf3bec507e8d49af54d9aad9ac17471c Diff: https://reviews.apache.org/r/71741/diff/3/ Changes: https://reviews.apache.org/r/71741/diff/2-3/ Testing --- Previewed in generated site Thanks, Benjamin Bannier
Re: Review Request 71787: Added end-to-end test for reservation update with persistent volume.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/71787/#review218758 --- Ship it! Ship It! - Benjamin Bannier On Nov. 20, 2019, 3:49 p.m., Benno Evers wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/71787/ > --- > > (Updated Nov. 20, 2019, 3:49 p.m.) > > > Review request for mesos and Benjamin Bannier. > > > Bugs: MESOS-9992 > https://issues.apache.org/jira/browse/MESOS-9992 > > > Repository: mesos > > > Description > --- > > Added a new test that verifies that the reservation for a > persistent disk volume can be updated after it has been > created. > > > Diffs > - > > src/tests/persistent_volume_endpoints_tests.cpp > bddc9467c5b9fe6cdcbd84f1b110356a43b59ba0 > > > Diff: https://reviews.apache.org/r/71787/diff/3/ > > > Testing > --- > > `./src/mesos-tests --gtest_filter="*ReservationUpdate*"` > > > Thanks, > > Benno Evers > >
Review Request 71782: Moved check on subscriber presences into subscribers.send.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/71782/ --- Review request for mesos, Andrei Sekretenko and Benjamin Mahler. Repository: mesos Description --- Moved check on subscriber presences into subscribers.send. Diffs - src/master/master.cpp 14b90a5e276df055bb8a570331f27cab200c9869 Diff: https://reviews.apache.org/r/71782/diff/1/ Testing --- `make check` Thanks, Benjamin Bannier
Review Request 71780: Deferred sending of initial heartbeat to Heartbeater.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/71780/ --- Review request for mesos, Andrei Sekretenko and Benjamin Mahler. Repository: mesos Description --- Deferred sending of initial heartbeat to Heartbeater. Diffs - src/master/http.cpp 1778664dddf19f9ab6d6c09ec35d64674ae488df src/master/master.hpp 8a140650a016c8afbfb39729eba2b5e78ea81c5f Diff: https://reviews.apache.org/r/71780/diff/1/ Testing --- `make check` Thanks, Benjamin Bannier
Review Request 71781: Used a potential use after free bug.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/71781/ --- Review request for mesos, Andrei Sekretenko and Benjamin Mahler. Repository: mesos Description --- Moving a framework to `frameworks.completed` transfers ownership of the Framework to that container and e.g., using the original framework pointer after that could fail (the framework could e.g., be already be rotated out). This patch fixes the master to not use the potentially cleaned up framework pointer anymore. Diffs - src/master/master.cpp 14b90a5e276df055bb8a570331f27cab200c9869 Diff: https://reviews.apache.org/r/71781/diff/1/ Testing --- `make check` Thanks, Benjamin Bannier
Re: Review Request 71739: Used post-reservation refinement format in some operator API docs.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/71739/ --- (Updated Nov. 20, 2019, 3:16 p.m.) Review request for mesos and Benno Evers. Bugs: MESOS-9993 https://issues.apache.org/jira/browse/MESOS-9993 Repository: mesos Description --- Used post-reservation refinement format in some operator API docs. Diffs (updated) - docs/operator-http-api.md 6cc367dabf3bec507e8d49af54d9aad9ac17471c Diff: https://reviews.apache.org/r/71739/diff/2/ Changes: https://reviews.apache.org/r/71739/diff/1-2/ Testing --- Previewed in generated site. Thanks, Benjamin Bannier
Re: Review Request 71741: Updated operator API documention to use rereservation format.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/71741/ --- (Updated Nov. 20, 2019, 3:17 p.m.) Review request for mesos and Benno Evers. Changes --- Expand documentation and examples Bugs: MESOS-9993 https://issues.apache.org/jira/browse/MESOS-9993 Repository: mesos Description --- Updated operator API documention to use rereservation format. Diffs (updated) - docs/operator-http-api.md 6cc367dabf3bec507e8d49af54d9aad9ac17471c Diff: https://reviews.apache.org/r/71741/diff/2/ Changes: https://reviews.apache.org/r/71741/diff/1-2/ Testing --- Previewed in generated site Thanks, Benjamin Bannier
Re: Review Request 71787: Added end-to-end test for reservation update with persistent volume.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/71787/#review218720 --- src/tests/persistent_volume_endpoints_tests.cpp Lines 347 (patched) <https://reviews.apache.org/r/71787/#comment306583> We are overloading this test, but could call out here that we also test that a persistent volume should not end up unreserved. src/tests/persistent_volume_endpoints_tests.cpp Lines 399 (patched) <https://reviews.apache.org/r/71787/#comment306580> Let's add a comment here that we are now re-reserving a persistent volume from `foo` to `bar`. src/tests/persistent_volume_endpoints_tests.cpp Lines 433 (patched) <https://reviews.apache.org/r/71787/#comment306581> nit: we could get rid of this empty line src/tests/persistent_volume_endpoints_tests.cpp Lines 456-457 (patched) <https://reviews.apache.org/r/71787/#comment306582> Let's get rid of one of these. - Benjamin Bannier On Nov. 20, 2019, 2:07 p.m., Benno Evers wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/71787/ > --- > > (Updated Nov. 20, 2019, 2:07 p.m.) > > > Review request for mesos and Benjamin Bannier. > > > Bugs: MESOS-9992 > https://issues.apache.org/jira/browse/MESOS-9992 > > > Repository: mesos > > > Description > --- > > Added a new test that verifies that the reservation for a > persistent disk volume can be updated after it has been > created. > > > Diffs > - > > src/tests/persistent_volume_endpoints_tests.cpp > bddc9467c5b9fe6cdcbd84f1b110356a43b59ba0 > > > Diff: https://reviews.apache.org/r/71787/diff/2/ > > > Testing > --- > > `./src/mesos-tests --gtest_filter="*ReservationUpdate*"` > > > Thanks, > > Benno Evers > >
Re: Review Request 71785: Removed check when validating reservations.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/71785/#review218718 --- Ship it! Like discussed offline, let's add a test as part of https://reviews.apache.org/r/71787/ which validates that a persistent volume cannot be unreserved through `RESERVE_RESOURCES` calls. - Benjamin Bannier On Nov. 19, 2019, 5:52 p.m., Benno Evers wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/71785/ > --- > > (Updated Nov. 19, 2019, 5:52 p.m.) > > > Review request for mesos and Benjamin Bannier. > > > Repository: mesos > > > Description > --- > > Removed a validation that was checking that `RESERVE` operations > did not contain disk resources, based on the theory that disk > resources must already be reserved. > > This assumption does not hold up in the presence of reservation > updates, so it is removed. > > > Diffs > - > > src/master/validation.cpp c5fbbdd55e4a217c756e2ece3f8fd6ed5e88048f > > > Diff: https://reviews.apache.org/r/71785/diff/1/ > > > Testing > --- > > > Thanks, > > Benno Evers > >
Re: Review Request 71741: Updated operator API documention to use rereservation format.
> On Nov. 18, 2019, 5:41 p.m., Benno Evers wrote: > > I'm not sure that this example does much to explain the semantics of the > > `RESERVE_RESOURCES` call to unsuspecting readers: The example you modify > > will behave exactly the same whether the `source` field is present or not. > > > > At the very least, `source` should be formally introduced in the > > description of the `RESERVE_RESOURCES` call at the beginning of the > > section. Ideally, we'd have a first example without `source` (i.e. the one > > we had before) and then a second example using `source` to do an actual > > reservation update. (In your commit message you use the word > > `rereservation`, do we use that anywhere else?) I updated the documentation and examples. I went with two examples (covering both "simple reservation" and re-reservation). I did however in both cases specify `source` as IMO it is the cleaner API. - Benjamin --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/71741/#review218664 ------- On Nov. 8, 2019, 2:13 p.m., Benjamin Bannier wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/71741/ > --- > > (Updated Nov. 8, 2019, 2:13 p.m.) > > > Review request for mesos and Benno Evers. > > > Bugs: MESOS-9993 > https://issues.apache.org/jira/browse/MESOS-9993 > > > Repository: mesos > > > Description > --- > > Updated operator API documention to use rereservation format. > > > Diffs > - > > docs/operator-http-api.md 6cc367dabf3bec507e8d49af54d9aad9ac17471c > > > Diff: https://reviews.apache.org/r/71741/diff/1/ > > > Testing > --- > > Previewed in generated site > > > Thanks, > > Benjamin Bannier > >
Review Request 71769: Gracefully handled duplicated volumes from non-conforming CSI plugins.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/71769/ --- Review request for mesos and Chun-Hung Hsiao. Bugs: MESOS-9956 https://issues.apache.org/jira/browse/MESOS-9956 Repository: mesos Description --- If the SLRP uses a plugin that does not conform to the CSI spec and reports duplicated volumes, the duplicate would be removed. This is a backport of `43b86da531a889b1c4b1d7ca6acb2eb924ea01e1`. We are not backporting test changes as the original patch relies on periodic plugin polling. Diffs - src/resource_provider/storage/provider.cpp 6d632606f411d3ca99d3573a57c9f68b02ba8072 Diff: https://reviews.apache.org/r/71769/diff/1/ Testing --- `sudo make check` results in only known failures Thanks, Benjamin Bannier
Review Request 71741: Updated operator API documention to use rereservation format.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/71741/ --- Review request for mesos and Benno Evers. Bugs: MESOS-9993 https://issues.apache.org/jira/browse/MESOS-9993 Repository: mesos Description --- Updated operator API documention to use rereservation format. Diffs - docs/operator-http-api.md 6cc367dabf3bec507e8d49af54d9aad9ac17471c Diff: https://reviews.apache.org/r/71741/diff/1/ Testing --- Previewed in generated site Thanks, Benjamin Bannier
Review Request 71739: Used post-reservation refinement format in some operator API docs.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/71739/ --- Review request for mesos and Benno Evers. Bugs: MESOS-9993 https://issues.apache.org/jira/browse/MESOS-9993 Repository: mesos Description --- Used post-reservation refinement format in some operator API docs. Diffs - docs/operator-http-api.md 6cc367dabf3bec507e8d49af54d9aad9ac17471c Diff: https://reviews.apache.org/r/71739/diff/1/ Testing --- Previewed in generated site. Thanks, Benjamin Bannier
Re: Review Request 71729: Added authorization handling for reservations with `source`.
> On Nov. 6, 2019, 5:11 p.m., Benno Evers wrote: > > src/master/master.cpp > > Lines 3810 (patched) > > <https://reviews.apache.org/r/71729/diff/1/?file=2171587#file2171587line3810> > > > > It doesn't seem ideal to have recursively nested calls to > > `collectauthorizations()`, even if the logic is sound it seems hard to > > reason about. > > > > Would it be possible to eliminate the branching by setting `source` to > > be `resources.popReservation()` if `source` is empty? > > Benjamin Bannier wrote: > > Would it be possible to eliminate the branching by setting source to be > resources.popReservation() if source is empty? > > This would only work if we know that all resources passed to `RESERVE` > are indeed reserved. Unfortunately that is not the case in the current > implementation (e.g., `cpus(A):1;mem:256` would reserve only `cpus`). We need > to keep support for that behavior as it is part of the APII. > > In the patch I put up we go from the narrower extended API (e.g., all > resources passed to `RESERVE` must have identical reservations) to the wider > existing API so we are good. Going from wider to narrower doesn't work, > though. > > What I could do for the sake of readibility would be to introduce a > dedicated function for the legacy behavior to avoid the self-recursion. I am > not sure that would help (and might it even make harder to follow the code). > > WDYT? > > Benno Evers wrote: > Intuitively introducing a dedicated function sounds cleaner to me, but > the self-recursion should be fine as well if there's no easy way to avoid it. > Maybe it would be good to add some of the reasoning above to the comment, > though. I added context to the comment. - Benjamin --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/71729/#review218545 --- On Nov. 8, 2019, 1:48 p.m., Benjamin Bannier wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/71729/ > --- > > (Updated Nov. 8, 2019, 1:48 p.m.) > > > Review request for mesos and Benno Evers. > > > Bugs: MESOS-9991 > https://issues.apache.org/jira/browse/MESOS-9991 > > > Repository: mesos > > > Description > --- > > This patch adds authorization handling for `RESERVE` operations > containing `source` fields. In order to stay backwards-compatible we add > a dedicated authorization branch for such operations which under the > hood translates each removed reservation to an `UNRESERVE` operation and > every added reservation as a `RESERVE` operation where we fall back to > existing authorization code for authorization. > > > Diffs > - > > src/master/master.cpp 2fdd6f7ddbb488d785c6f875c8b0c46c5f881d9d > src/tests/master_authorization_tests.cpp > 06471aa7779d399f4474ed40db3fbcc60b8298b2 > > > Diff: https://reviews.apache.org/r/71729/diff/3/ > > > Testing > --- > > `make check` > > > Thanks, > > Benjamin Bannier > >
Re: Review Request 71729: Added authorization handling for reservations with `source`.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/71729/ --- (Updated Nov. 8, 2019, 1:48 p.m.) Review request for mesos and Benno Evers. Bugs: MESOS-9991 https://issues.apache.org/jira/browse/MESOS-9991 Repository: mesos Description --- This patch adds authorization handling for `RESERVE` operations containing `source` fields. In order to stay backwards-compatible we add a dedicated authorization branch for such operations which under the hood translates each removed reservation to an `UNRESERVE` operation and every added reservation as a `RESERVE` operation where we fall back to existing authorization code for authorization. Diffs (updated) - src/master/master.cpp 2fdd6f7ddbb488d785c6f875c8b0c46c5f881d9d src/tests/master_authorization_tests.cpp 06471aa7779d399f4474ed40db3fbcc60b8298b2 Diff: https://reviews.apache.org/r/71729/diff/3/ Changes: https://reviews.apache.org/r/71729/diff/2-3/ Testing --- `make check` Thanks, Benjamin Bannier
Re: Review Request 71735: Added 'nodiscard' attribute to some Resources member functions.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/71735/#review218567 --- Ship it! Thanks for this. As for the formatting, personally I'd prefer whatever formatting our `clang-format` does, if only to remove the human element from this as much as possible. - Benjamin Bannier On Nov. 6, 2019, 3:35 p.m., Benno Evers wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/71735/ > --- > > (Updated Nov. 6, 2019, 3:35 p.m.) > > > Review request for mesos, Benjamin Bannier and Greg Mann. > > > Repository: mesos > > > Description > --- > > Added the `[[nodiscard]]` attribute to `Resources::pushReservation()` > and `Resources::popReservation()`, in order to prevent mistakes by > authors incorrectly assuming that these functions would modify the > resources objects in place. > > > Diffs > - > > include/mesos/resources.hpp b8aef28e08f85c87bb78f25a64b0d7318f2727cc > > > Diff: https://reviews.apache.org/r/71735/diff/1/ > > > Testing > --- > > > Thanks, > > Benno Evers > >
Re: Review Request 71722: Improved error reporting in 'Resources::pushReservation()'.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/71722/#review218566 --- Ship it! Thanks for this, I remember having patched that in for development repeatedly. - Benjamin Bannier On Nov. 5, 2019, 6:05 p.m., Benno Evers wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/71722/ > --- > > (Updated Nov. 5, 2019, 6:05 p.m.) > > > Review request for mesos and Benjamin Bannier. > > > Repository: mesos > > > Description > --- > > Make sure to print the actual error message after an assertion > failure in `Resources::pushReservations()`. > > > Diffs > - > > src/common/resources.cpp bfa9f3ea7e8c3e2dc9b4c4f7c86ad29b0de81f24 > > > Diff: https://reviews.apache.org/r/71722/diff/1/ > > > Testing > --- > > > Thanks, > > Benno Evers > >
Re: Review Request 71729: Added authorization handling for reservations with `source`.
> On Nov. 6, 2019, 5:11 p.m., Benno Evers wrote: > > src/master/master.cpp > > Lines 3810 (patched) > > <https://reviews.apache.org/r/71729/diff/1/?file=2171587#file2171587line3810> > > > > It doesn't seem ideal to have recursively nested calls to > > `collectauthorizations()`, even if the logic is sound it seems hard to > > reason about. > > > > Would it be possible to eliminate the branching by setting `source` to > > be `resources.popReservation()` if `source` is empty? > Would it be possible to eliminate the branching by setting source to be > resources.popReservation() if source is empty? This would only work if we know that all resources passed to `RESERVE` are indeed reserved. Unfortunately that is not the case in the current implementation (e.g., `cpus(A):1;mem:256` would reserve only `cpus`). We need to keep support for that behavior as it is part of the APII. In the patch I put up we go from the narrower extended API (e.g., all resources passed to `RESERVE` must have identical reservations) to the wider existing API so we are good. Going from wider to narrower doesn't work, though. What I could do for the sake of readibility would be to introduce a dedicated function for the legacy behavior to avoid the self-recursion. I am not sure that would help (and might it even make harder to follow the code). WDYT? > On Nov. 6, 2019, 5:11 p.m., Benno Evers wrote: > > src/master/master.cpp > > Lines 3820 (patched) > > <https://reviews.apache.org/r/71729/diff/1/?file=2171587#file2171587line3820> > > > > Shouldn't the first `Unreserve` operation contain the original `source`? Of course. > On Nov. 6, 2019, 5:11 p.m., Benno Evers wrote: > > src/master/master.cpp > > Lines 3828 (patched) > > <https://reviews.apache.org/r/71729/diff/1/?file=2171587#file2171587line3828> > > > > Is this the same as `reserve.resources().reservations()`? Good point, this is indeed the same as `reserve.resources(0).reservations`. Using that seems to be a better idea than going strictly with symmetric approaches for `targetReservations` and `ancestorReservations` as it can help avoid a number of temporaries. > On Nov. 6, 2019, 5:11 p.m., Benno Evers wrote: > > src/master/master.cpp > > Lines 3834 (patched) > > <https://reviews.apache.org/r/71729/diff/1/?file=2171587#file2171587line3834> > > > > That look more like debug-statements rather than `INFO`-level logging? Indeed, even explicitly marked up as such with my magic string, yet still missed. - Benjamin ------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/71729/#review218545 --- On Nov. 7, 2019, noon, Benjamin Bannier wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/71729/ > --- > > (Updated Nov. 7, 2019, noon) > > > Review request for mesos and Benno Evers. > > > Bugs: MESOS-9991 > https://issues.apache.org/jira/browse/MESOS-9991 > > > Repository: mesos > > > Description > --- > > This patch adds authorization handling for `RESERVE` operations > containing `source` fields. In order to stay backwards-compatible we add > a dedicated authorization branch for such operations which under the > hood translates each removed reservation to an `UNRESERVE` operation and > every added reservation as a `RESERVE` operation where we fall back to > existing authorization code for authorization. > > > Diffs > ----- > > src/master/master.cpp e7609f361b58f9b1f0d2d5eb6037f98edcb41a56 > src/tests/master_authorization_tests.cpp > 06471aa7779d399f4474ed40db3fbcc60b8298b2 > > > Diff: https://reviews.apache.org/r/71729/diff/2/ > > > Testing > --- > > `make check` > > > Thanks, > > Benjamin Bannier > >
Re: Review Request 71729: Added authorization handling for reservations with `source`.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/71729/ --- (Updated Nov. 7, 2019, noon) Review request for mesos and Benno Evers. Bugs: MESOS-9991 https://issues.apache.org/jira/browse/MESOS-9991 Repository: mesos Description --- This patch adds authorization handling for `RESERVE` operations containing `source` fields. In order to stay backwards-compatible we add a dedicated authorization branch for such operations which under the hood translates each removed reservation to an `UNRESERVE` operation and every added reservation as a `RESERVE` operation where we fall back to existing authorization code for authorization. Diffs (updated) - src/master/master.cpp e7609f361b58f9b1f0d2d5eb6037f98edcb41a56 src/tests/master_authorization_tests.cpp 06471aa7779d399f4474ed40db3fbcc60b8298b2 Diff: https://reviews.apache.org/r/71729/diff/2/ Changes: https://reviews.apache.org/r/71729/diff/1-2/ Testing --- `make check` Thanks, Benjamin Bannier
Re: Review Request 71734: Revamped attribute handling in stout.
> On Nov. 6, 2019, 5:03 p.m., Benjamin Bannier wrote: > > 3rdparty/stout/include/stout/exit.hpp > > Line 65 (original), 65 (patched) > > <https://reviews.apache.org/r/71734/diff/2/?file=2171707#file2171707line65> > > > > nit: single line? > > Benno Evers wrote: > I thought about it, but right now (i.e. after this patch is applied) all > code is inconsistent in using the > > ``` > attributes > void foo(args); > ``` > style of declaration, which looks pretty good to me. This is mostly a matter of personal preference, but at least for such simple cases which do not come with "local consistency" precedence I'd pick whatever our `clang-format` fork would do, if only to make this less of a hassle in the future (the attribute on a separate line also has a non-C++ C vibe for me personally). So I guess the only argument I could bring would be: easier to work with since tooling-enabled. - Benjamin --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/71734/#review218544 --- On Nov. 6, 2019, 4:56 p.m., Benno Evers wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/71734/ > ------- > > (Updated Nov. 6, 2019, 4:56 p.m.) > > > Review request for mesos, Benjamin Bannier, Benjamin Mahler, and Greg Mann. > > > Repository: mesos > > > Description > --- > > This makes several changes to the attribute compatibility layer > provided by stout: > > * Add a new `STOUT_HAS_CPP_ATTRIBUTE` macro to test compiler > support for a given attribute. > * Renamed the non-namespaced `NORETURN` macro to `STOUT_NORETURN`. > * Preferred the use of the standardized `[[noreturn]]` syntax > if supported by the compiler. > * Fixed previous usages of `NORETURN` in the stout codebase. > * Added support for the `[[nodiscard]]` attribute. > > > Diffs > - > > 3rdparty/stout/include/stout/abort.hpp > 43ed5ce2830c493e4c801cc81f8dde0922c99a8d > 3rdparty/stout/include/stout/attributes.hpp > aa377db82e1dbdb8727b1128780e2409accc8ae9 > 3rdparty/stout/include/stout/exit.hpp > 34585a005063b17d0c7754c8e8c13f0641383bc4 > 3rdparty/stout/include/stout/unimplemented.hpp > ab6caa8fa9645bca66a3efcdc6d337f3fb0481d7 > 3rdparty/stout/include/stout/unreachable.hpp > d4b3bb0582eb9e64e6f150735d1e9f2956edbca6 > > > Diff: https://reviews.apache.org/r/71734/diff/2/ > > > Testing > --- > > > Thanks, > > Benno Evers > >
Re: Review Request 71734: Revamped attribute handling in stout.
> On Nov. 6, 2019, 5:03 p.m., Benjamin Bannier wrote: > > 3rdparty/stout/include/stout/attributes.hpp > > Lines 17 (patched) > > <https://reviews.apache.org/r/71734/diff/2/?file=2171706#file2171706line17> > > > > Did you want to export this macro? I am not sure it would be useful > > elsewhere since any use of it should probably be in this file. Maybe let's > > just `#undef` it once we are done here. > > Benno Evers wrote: > I wasn't sure, but it seems like a macro that could be generally useful > if some code wants to test for the presence of an attribute, and it's already > in the correct header for attribute-related convenience methods. (and it's > namespaced, so there's no need to worry about namespace pollution) > > What do you think? Makes sense to me. - Benjamin --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/71734/#review218544 --- On Nov. 6, 2019, 4:56 p.m., Benno Evers wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/71734/ > ------- > > (Updated Nov. 6, 2019, 4:56 p.m.) > > > Review request for mesos, Benjamin Bannier, Benjamin Mahler, and Greg Mann. > > > Repository: mesos > > > Description > --- > > This makes several changes to the attribute compatibility layer > provided by stout: > > * Add a new `STOUT_HAS_CPP_ATTRIBUTE` macro to test compiler > support for a given attribute. > * Renamed the non-namespaced `NORETURN` macro to `STOUT_NORETURN`. > * Preferred the use of the standardized `[[noreturn]]` syntax > if supported by the compiler. > * Fixed previous usages of `NORETURN` in the stout codebase. > * Added support for the `[[nodiscard]]` attribute. > > > Diffs > - > > 3rdparty/stout/include/stout/abort.hpp > 43ed5ce2830c493e4c801cc81f8dde0922c99a8d > 3rdparty/stout/include/stout/attributes.hpp > aa377db82e1dbdb8727b1128780e2409accc8ae9 > 3rdparty/stout/include/stout/exit.hpp > 34585a005063b17d0c7754c8e8c13f0641383bc4 > 3rdparty/stout/include/stout/unimplemented.hpp > ab6caa8fa9645bca66a3efcdc6d337f3fb0481d7 > 3rdparty/stout/include/stout/unreachable.hpp > d4b3bb0582eb9e64e6f150735d1e9f2956edbca6 > > > Diff: https://reviews.apache.org/r/71734/diff/2/ > > > Testing > --- > > > Thanks, > > Benno Evers > >
Re: Review Request 71734: Revamped attribute handling in stout.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/71734/#review218544 --- Fix it, then Ship it! 3rdparty/stout/include/stout/attributes.hpp Lines 17 (patched) <https://reviews.apache.org/r/71734/#comment306282> Did you want to export this macro? I am not sure it would be useful elsewhere since any use of it should probably be in this file. Maybe let's just `#undef` it once we are done here. 3rdparty/stout/include/stout/attributes.hpp Line 17 (original), 22 (patched) <https://reviews.apache.org/r/71734/#comment306278> Since you are add it you could add a comment here that we _always_ want to specify `noreturn` since it might enable additional optimizations (`nodiscard` OTOH likely has mostly effects in the frontend). 3rdparty/stout/include/stout/exit.hpp Line 65 (original), 65 (patched) <https://reviews.apache.org/r/71734/#comment306281> nit: single line? 3rdparty/stout/include/stout/unreachable.hpp Line 25 (original), 25 (patched) <https://reviews.apache.org/r/71734/#comment306283> nit: single line? - Benjamin Bannier On Nov. 6, 2019, 4:56 p.m., Benno Evers wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/71734/ > --- > > (Updated Nov. 6, 2019, 4:56 p.m.) > > > Review request for mesos, Benjamin Bannier, Benjamin Mahler, and Greg Mann. > > > Repository: mesos > > > Description > --- > > This makes several changes to the attribute compatibility layer > provided by stout: > > * Add a new `STOUT_HAS_CPP_ATTRIBUTE` macro to test compiler > support for a given attribute. > * Renamed the non-namespaced `NORETURN` macro to `STOUT_NORETURN`. > * Preferred the use of the standardized `[[noreturn]]` syntax > if supported by the compiler. > * Fixed previous usages of `NORETURN` in the stout codebase. > * Added support for the `[[nodiscard]]` attribute. > > > Diffs > - > > 3rdparty/stout/include/stout/abort.hpp > 43ed5ce2830c493e4c801cc81f8dde0922c99a8d > 3rdparty/stout/include/stout/attributes.hpp > aa377db82e1dbdb8727b1128780e2409accc8ae9 > 3rdparty/stout/include/stout/exit.hpp > 34585a005063b17d0c7754c8e8c13f0641383bc4 > 3rdparty/stout/include/stout/unimplemented.hpp > ab6caa8fa9645bca66a3efcdc6d337f3fb0481d7 > 3rdparty/stout/include/stout/unreachable.hpp > d4b3bb0582eb9e64e6f150735d1e9f2956edbca6 > > > Diff: https://reviews.apache.org/r/71734/diff/2/ > > > Testing > --- > > > Thanks, > > Benno Evers > >
Re: Review Request 71695: Updated 'Master::Http::_reserve' to pass along new 'source' field.
> On Nov. 6, 2019, 2:35 a.m., Greg Mann wrote: > > src/master/http.cpp > > Lines 1988 (patched) > > <https://reviews.apache.org/r/71695/diff/2/?file=2171533#file2171533line1994> > > > > Should this be an Option? Since it isn't, it seems like we will set the > > `source` field to a default-constructed value in `_reserve()` when the > > operator does not specify that parameter. Would it be better to avoid > > setting the `source` field at all in the offer operation if it isn't found > > in the query parameters? > > Benno Evers wrote: > As far as I know, there's no difference in the protobuf wire format > between an unset and a zero-length repeated field. So as soon as we have > constructed the `Reserve` protobuf inside `_reserve()`, we have no way to > distinguish these cases anyways. > > So the only question seems to be how early we want to forget about > whether or not `source` was specified, and I opted to do it here for symmetry > with how we pass `resources`. I agree with Benno here. Additionally, we are merely unpacking the protobuf data here, and want to make decisions based on empty/non-empty state elsewhere (which for `repeated` translates to set/unset). I cannot see what additional wrapping in `Option` would add. - Benjamin --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/71695/#review218519 --- On Nov. 5, 2019, 2:25 p.m., Benno Evers wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/71695/ > ------- > > (Updated Nov. 5, 2019, 2:25 p.m.) > > > Review request for mesos and Benjamin Bannier. > > > Bugs: MESOS-9987 and MESOS-9989 > https://issues.apache.org/jira/browse/MESOS-9987 > https://issues.apache.org/jira/browse/MESOS-9989 > > > Repository: mesos > > > Description > --- > > Updated 'Master::Http::_reserve()' to correctly set the new `source` > field in the `Offer::Operation` created from operator API input. > > > Diffs > - > > src/master/http.cpp 60765c9b9d6903f6ed94fa8c614055698caad0da > src/master/master.hpp dc45028d2ecfb61bf9ea82d90d2393af648a6023 > > > Diff: https://reviews.apache.org/r/71695/diff/2/ > > > Testing > --- > > > Thanks, > > Benno Evers > >
Re: Review Request 71695: Updated 'Master::Http::_reserve' to pass along new 'source' field.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/71695/#review218541 --- Ship it! Ship It! - Benjamin Bannier On Nov. 5, 2019, 2:25 p.m., Benno Evers wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/71695/ > --- > > (Updated Nov. 5, 2019, 2:25 p.m.) > > > Review request for mesos and Benjamin Bannier. > > > Bugs: MESOS-9987 and MESOS-9989 > https://issues.apache.org/jira/browse/MESOS-9987 > https://issues.apache.org/jira/browse/MESOS-9989 > > > Repository: mesos > > > Description > --- > > Updated 'Master::Http::_reserve()' to correctly set the new `source` > field in the `Offer::Operation` created from operator API input. > > > Diffs > - > > src/master/http.cpp 60765c9b9d6903f6ed94fa8c614055698caad0da > src/master/master.hpp dc45028d2ecfb61bf9ea82d90d2393af648a6023 > > > Diff: https://reviews.apache.org/r/71695/diff/2/ > > > Testing > --- > > > Thanks, > > Benno Evers > >
Re: Review Request 71725: Added end-to-end test for operator API reservation updates.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/71725/#review218537 --- Fix it, then Ship it! Nice approach to make the "meat" of the test more obvious through helpers. src/tests/api_tests.cpp Lines 1491 (patched) <https://reviews.apache.org/r/71725/#comment306271> Stray spaces src/tests/api_tests.cpp Lines 1519 (patched) <https://reviews.apache.org/r/71725/#comment306273> Nit: to more match the usage we could define this before `verifyReservation`. src/tests/api_tests.cpp Lines 1521 (patched) <https://reviews.apache.org/r/71725/#comment306274> We could name this `resources` for symmetry. src/tests/api_tests.cpp Lines 1553 (patched) <https://reviews.apache.org/r/71725/#comment306272> Line too long - Benjamin Bannier On Nov. 6, 2019, 12:09 p.m., Benno Evers wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/71725/ > --- > > (Updated Nov. 6, 2019, 12:09 p.m.) > > > Review request for mesos and Benjamin Bannier. > > > Repository: mesos > > > Description > --- > > Added a new test to verify that reservations can be updated > using the operator API. > > > Diffs > - > > src/tests/api_tests.cpp bd207eaebc8fc14de16f7af633d943b315328e8a > > > Diff: https://reviews.apache.org/r/71725/diff/1/ > > > Testing > --- > > > Thanks, > > Benno Evers > >
Review Request 71732: Fixed an incorrect resource mutation.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/71732/ --- Review request for mesos, Benno Evers and Greg Mann. Repository: mesos Description --- `Resources::pushReservation` does not mutate the `Resources` it is invoked on; instead it returns its result which should then be used. This issue was identified by making `Resources::pushReservation` `nodiscard`. Diffs - src/tests/scheduler_tests.cpp e0ed02900330c678bbf5c609c1f45d05147851ed Diff: https://reviews.apache.org/r/71732/diff/1/ Testing --- `make check` Thanks, Benjamin Bannier
Re: Review Request 71719: Updated 'getResourceConversion()' for reservation updates.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/71719/#review218536 --- Ship it! Ship It! - Benjamin Bannier On Nov. 6, 2019, 12:06 p.m., Benno Evers wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/71719/ > --- > > (Updated Nov. 6, 2019, 12:06 p.m.) > > > Review request for mesos. > > > Repository: mesos > > > Description > --- > > Updated the `getResourcesConversion()` function to correctly > handle the `source` field in `RESERVE` operations. > > > Diffs > - > > src/common/resources_utils.cpp 5e78997fb9673faec37f20566d06328fe347b7e1 > > > Diff: https://reviews.apache.org/r/71719/diff/1/ > > > Testing > --- > > `make check` > > > Thanks, > > Benno Evers > >
Re: Review Request 71700: Updated offer operation resource validation for reservation updates.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/71700/#review218535 --- Ship it! Patch looks great! - Benjamin Bannier On Oct. 29, 2019, 7:13 p.m., Benno Evers wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/71700/ > --- > > (Updated Oct. 29, 2019, 7:13 p.m.) > > > Review request for mesos and Benjamin Bannier. > > > Repository: mesos > > > Description > --- > > Updated `validateAndUpgradeResources()` to also validate the `source` > field in a reservation. > > > Diffs > - > > src/common/resources_utils.cpp cbdad4b045ad957b2ab1fd09a6b9fb12bb4fb523 > > > Diff: https://reviews.apache.org/r/71700/diff/1/ > > > Testing > --- > > > Thanks, > > Benno Evers > >
Re: Review Request 71696: Updated validation of 'Reserve' call.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/71696/#review218534 --- Ship it! Ship It! - Benjamin Bannier On Nov. 5, 2019, 2:25 p.m., Benno Evers wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/71696/ > --- > > (Updated Nov. 5, 2019, 2:25 p.m.) > > > Review request for mesos and Benjamin Bannier. > > > Bugs: MESOS-9985 > https://issues.apache.org/jira/browse/MESOS-9985 > > > Repository: mesos > > > Description > --- > > Updated `master::validation::operation::validate(Reserve&)` to > account for the new `source` field in the reserve call, verifying > that `source` and `resources` have a common reservation ancestor. > > > Diffs > - > > src/master/validation.cpp a7ecefb8a1e186901301419feca75600d8de001b > > > Diff: https://reviews.apache.org/r/71696/diff/2/ > > > Testing > --- > > > Thanks, > > Benno Evers > >
Re: Review Request 71690: Added function to compute a common reservation ancestor.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/71690/#review218533 --- src/common/resources.cpp Line 612 (original) <https://reviews.apache.org/r/71690/#comment306269> Let's keep this line. src/common/resources.cpp Lines 1339 (patched) <https://reviews.apache.org/r/71690/#comment306270> This function has an implicit precondition on `r1` and `r2` referring to identical resource quantities. Let's at least add this to the declarations's documentation. - Benjamin Bannier On Nov. 5, 2019, 6:12 p.m., Benno Evers wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/71690/ > --- > > (Updated Nov. 5, 2019, 6:12 p.m.) > > > Review request for mesos and Benjamin Bannier. > > > Bugs: MESOS-9984 > https://issues.apache.org/jira/browse/MESOS-9984 > > > Repository: mesos > > > Description > --- > > Added a new function `Resources::getReservationAncestor()` to compute > a common reservation ancestor between two `Resources`. > > > Diffs > - > > include/mesos/resources.hpp b8aef28e08f85c87bb78f25a64b0d7318f2727cc > src/common/resources.cpp bfa9f3ea7e8c3e2dc9b4c4f7c86ad29b0de81f24 > src/tests/resources_tests.cpp b5854656b7e9ce7af9e1d8ecad708066512d814f > > > Diff: https://reviews.apache.org/r/71690/diff/3/ > > > Testing > --- > > > Thanks, > > Benno Evers > >
Re: Review Request 71688: Reject operator API calls that include reservation updates.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/71688/#review218532 --- Ship it! Ship It! - Benjamin Bannier On Nov. 1, 2019, 5:01 p.m., Benno Evers wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/71688/ > --- > > (Updated Nov. 1, 2019, 5:01 p.m.) > > > Review request for mesos and Benjamin Bannier. > > > Repository: mesos > > > Description > --- > > Reject operator API calls that include reservation updates. > > > Diffs > - > > src/master/http.cpp 60765c9b9d6903f6ed94fa8c614055698caad0da > > > Diff: https://reviews.apache.org/r/71688/diff/2/ > > > Testing > --- > > > Thanks, > > Benno Evers > >
Re: Review Request 71687: Rejected scheduler calls that include reservation updates.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/71687/#review218530 --- src/master/master.cpp Lines 4489 (patched) <https://reviews.apache.org/r/71687/#comment306268> `error.isNone()` - Benjamin Bannier On Nov. 1, 2019, 4:59 p.m., Benno Evers wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/71687/ > --- > > (Updated Nov. 1, 2019, 4:59 p.m.) > > > Review request for mesos and Benjamin Bannier. > > > Bugs: MESOS-9983 > https://issues.apache.org/jira/browse/MESOS-9983 > > > Repository: mesos > > > Description > --- > > Until reservation updates are implemented, ensure > that calls making use of them are properly rejected. > > > Diffs > - > > src/master/master.cpp 2fdd6f7ddbb488d785c6f875c8b0c46c5f881d9d > > > Diff: https://reviews.apache.org/r/71687/diff/2/ > > > Testing > --- > > > Thanks, > > Benno Evers > >
