Re: Review Request 70686: Added a unit test for master operation authorization.

[Chun-Hung Hsiao]

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

(Updated May 22, 2019, 4:09 a.m.)


Review request for mesos, Benjamin Bannier, Gastón Kleiman, and Greg Mann.


Changes
---

Addressed Benjamin's comments.


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


Repository: mesos


Description
---

This test verifies that allowing or denying an action will only result
in a success or failure on specific operations but not other operations
in an accept call. This is a regression test for MESOS-9474 and
MESOS-9480.


Diffs (updated)
-

  src/slave/slave.hpp c740bf7c4af1699dd71a31f9169d29c1b4adb40d 
  src/tests/master_authorization_tests.cpp 
f65b6210a8189ec5e9089bce845a46d325253058 
  src/tests/mesos.hpp e70e0e1bbebef41557f9bb1f26c379357fc74e2a 
  src/tests/mock_slave.hpp 326a450eb2ae9912a19c6e0220e80f74d2953add 
  src/tests/mock_slave.cpp 1122c2a8b56b9d4e5deea4e892d81061f5e39a8d 


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

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


Testing
---

make check

Ran the unit test under stress for 500 iterations.


Thanks,

Chun-Hung Hsiao

Re: Review Request 70686: Added a unit test for master operation authorization.

[Chun-Hung Hsiao]

> On May 21, 2019, 12:07 p.m., Benjamin Bannier wrote:
> > src/tests/master_authorization_tests.cpp
> > Lines 3317-3320 (patched)
> > 
> >
> > If you get rid of the `vector` `evolve` function in r/70683/ we'd 
> > create a temporary `RepeatedPtrField` and convert when creating the accept 
> > call.
> > 
> > 
> > RepeatedPtrField operations_ =
> >   evolve(
> >   {operations.begin(), operations.end()});
> >   
> > mesos.send(v1::createCallAccept(
> > subscribed->framework_id(),
> > offers->offers(0),
> > {operations_.begin(), operations_.end()}));
> > 

Instead of doing this, the operations are now created with v1 proto messages, 
since operation feeback is only supported in v1. As a result, we no longer need 
the vector evolve helper.


- Chun-Hung


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


On May 20, 2019, 10:37 p.m., Chun-Hung Hsiao wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/70686/
> ---
> 
> (Updated May 20, 2019, 10:37 p.m.)
> 
> 
> Review request for mesos, Benjamin Bannier, Gastón Kleiman, and Greg Mann.
> 
> 
> Bugs: MESOS-9485
> https://issues.apache.org/jira/browse/MESOS-9485
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This test verifies that allowing or denying an action will only result
> in a success or failure on specific operations but not other operations
> in an accept call. This is a regression test for MESOS-9474 and
> MESOS-9480.
> 
> 
> Diffs
> -
> 
>   src/slave/slave.hpp c740bf7c4af1699dd71a31f9169d29c1b4adb40d 
>   src/tests/master_authorization_tests.cpp 
> f65b6210a8189ec5e9089bce845a46d325253058 
>   src/tests/mesos.hpp e70e0e1bbebef41557f9bb1f26c379357fc74e2a 
>   src/tests/mock_slave.hpp 326a450eb2ae9912a19c6e0220e80f74d2953add 
>   src/tests/mock_slave.cpp 1122c2a8b56b9d4e5deea4e892d81061f5e39a8d 
> 
> 
> Diff: https://reviews.apache.org/r/70686/diff/1/
> 
> 
> Testing
> ---
> 
> make check
> 
> Ran the unit test under stress for 500 iterations.
> 
> 
> Thanks,
> 
> Chun-Hung Hsiao
> 
>

Re: Review Request 70683: Added helpers for evolving operation-related proto messages.

[Chun-Hung Hsiao]

> On May 21, 2019, 12:07 p.m., Benjamin Bannier wrote:
> > src/internal/evolve.hpp
> > Lines 106-114 (original), 122-133 (patched)
> > 
> >
> > This helper doesn't seem to fit well, and we'd only use this helper in 
> > a single spot later on in the chain.
> > 
> > I'd suggest to remove it here and just manually adjust in the later 
> > patch.

Removed all evolve helpers and added a devolve helper instead. In the test 
introduced in the later patch, all operations are generated with v1 proto 
messages, since operation feedback is only supported in v1.


- Chun-Hung


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


On May 20, 2019, 10:30 p.m., Chun-Hung Hsiao wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/70683/
> ---
> 
> (Updated May 20, 2019, 10:30 p.m.)
> 
> 
> Review request for mesos, Benjamin Bannier and Greg Mann.
> 
> 
> Bugs: MESOS-9485
> https://issues.apache.org/jira/browse/MESOS-9485
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added helpers for evolving operation-related proto messages.
> 
> 
> Diffs
> -
> 
>   src/internal/evolve.hpp 1044d9df75b6fc1f60d3704be9cb5751e6d4321d 
>   src/internal/evolve.cpp 19c155967bf090fb2ec39211805ff1385787ab59 
> 
> 
> Diff: https://reviews.apache.org/r/70683/diff/1/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Chun-Hung Hsiao
> 
>

Re: Review Request 70683: Added a helper to devolve offer operations.

[Chun-Hung Hsiao]

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

(Updated May 22, 2019, 4:06 a.m.)


Review request for mesos, Benjamin Bannier and Greg Mann.


Changes
---

Addressed Benjamin's comment.


Summary (updated)
-

Added a helper to devolve offer operations.


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


Repository: mesos


Description (updated)
---

Added a helper to devolve offer operations.


Diffs (updated)
-

  src/internal/devolve.hpp a1f8d8d333c5304b4d3795800485fdbfc14d4455 
  src/internal/devolve.cpp e23ed3ccf6c05f5b8ea91788788469250c70248c 


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

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


Testing
---

make check


Thanks,

Chun-Hung Hsiao

Re: Review Request 70622: Added a unit test to verify if SLRP allows changes in volume context.

[Chun-Hung Hsiao]

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

(Updated May 22, 2019, 3:15 a.m.)


Review request for mesos and Benjamin Bannier.


Changes
---

Addressed Benjamin's comments.


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


Repository: mesos


Description
---

Added a unit test to verify if SLRP allows changes in volume context.


Diffs (updated)
-

  src/examples/test_csi_plugin.cpp 03f782ead136a79c4c5b099496072933f6737598 
  src/tests/storage_local_resource_provider_tests.cpp 
ecd7aeef1cb3d1a5b4b3419dfd912d41a8c6 


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

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


Testing
---

make check


Thanks,

Chun-Hung Hsiao

Re: Review Request 70622: Added a unit test to verify if SLRP allows changes in volume context.

[Mesos Reviewbot]

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



Bad review!

Reviews applied: [70622, 70621, 70620]

Error:
2019-05-22 03:06:00 URL:https://reviews.apache.org/r/70622/diff/raw/ 
[13556/13556] -> "70622.patch" [1]
error: patch failed: src/examples/test_csi_plugin.cpp:217
error: src/examples/test_csi_plugin.cpp: patch does not apply

- Mesos Reviewbot


On May 9, 2019, 6:20 p.m., Chun-Hung Hsiao wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/70622/
> ---
> 
> (Updated May 9, 2019, 6:20 p.m.)
> 
> 
> Review request for mesos and Benjamin Bannier.
> 
> 
> Bugs: MESOS-9395
> https://issues.apache.org/jira/browse/MESOS-9395
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added a unit test to verify if SLRP allows changes in volume context.
> 
> 
> Diffs
> -
> 
>   src/examples/test_csi_plugin.cpp 03f782ead136a79c4c5b099496072933f6737598 
>   src/tests/storage_local_resource_provider_tests.cpp 
> ecd7aeef1cb3d1a5b4b3419dfd912d41a8c6 
> 
> 
> Diff: https://reviews.apache.org/r/70622/diff/1/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Chun-Hung Hsiao
> 
>

Re: Review Request 70621: Used full paths as volume IDs for the test CSI plugin.

[Chun-Hung Hsiao]

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

(Updated May 22, 2019, 3:03 a.m.)


Review request for mesos and Benjamin Bannier.


Changes
---

Addressed Benjamin's comments.


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


Repository: mesos


Description
---

The full paths of simulated volumes are now in their ID instead of their
contextual information. This simplifies SLRP tests, and makes it cleaner
if we want to customize the contextual information in the future.


Diffs (updated)
-

  src/examples/test_csi_plugin.cpp 03f782ead136a79c4c5b099496072933f6737598 
  src/tests/storage_local_resource_provider_tests.cpp 
ecd7aeef1cb3d1a5b4b3419dfd912d41a8c6 


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

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


Testing
---

make check


Thanks,

Chun-Hung Hsiao

Re: Review Request 70620: Made SLRP allow changes in volume context.

[Chun-Hung Hsiao]

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

(Updated May 22, 2019, 2:58 a.m.)


Review request for mesos, Benjamin Bannier and James DeFelice.


Changes
---

Addressed Benjamin's comments.


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


Repository: mesos


Description
---

To make SLRP more robust against non-conforming CSI plugins that change
volume contexts, the `getExistVolumes` method returns a list of resource
conversions consisting of one for converting old volume contexts to new
volume contexts, and one to remove missing volumes and add new volumes.

To make the interfaces consistent, `getStoragePools` now also returns a
list of resource conversions consisting of one conversion.


Diffs (updated)
-

  include/mesos/mesos.proto dc6a87f14864cb4d46b9856f251c2946a475162d 
  include/mesos/v1/mesos.proto e8086e0f70cac73876a8ae31db3365b5059b5c44 
  src/resource_provider/storage/provider.cpp 
999fe95bb6f38f5a25068accd854b37788b24028 


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

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


Testing
---

sudo make check
more testing done later in chain


Thanks,

Chun-Hung Hsiao

Re: Review Request 70665: Moved the logic of sending 'Framework' updates into a separate method.

[Benjamin Mahler]

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


Ship it!




Ship It!

- Benjamin Mahler


On May 21, 2019, 1:40 p.m., Andrei Sekretenko wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/70665/
> ---
> 
> (Updated May 21, 2019, 1:40 p.m.)
> 
> 
> Review request for mesos and Benjamin Mahler.
> 
> 
> Bugs: MESOS-7258
> https://issues.apache.org/jira/browse/MESOS-7258
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This patch consolidates the scattered logic of sending Framework updates
> (namely, the FRAMEWORK_ADDED event to master V1 API subscribers and
> UpdateFrameworkMessage to agents).
> 
> NOTE: The UpdateFrameworkMessage is now also being sent in the case of
> non-`forced` subscription request from a framework which has already
> been registered. Previously it was not being sent in this case -
> this likely is a bug.
> 
> 
> Diffs
> -
> 
>   src/master/master.hpp bf7a6b4720ac2e7a072f145f6a27e96a1a9da41e 
>   src/master/master.cpp 24bd9572741c033c02d03a3a3a35d7d4ed8b60de 
> 
> 
> Diff: https://reviews.apache.org/r/70665/diff/2/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Andrei Sekretenko
> 
>

Re: Review Request 70621: Used full paths as volume IDs for the test CSI plugin.

[Chun-Hung Hsiao]

> On May 14, 2019, 11:37 a.m., Benjamin Bannier wrote:
> > src/examples/test_csi_plugin.cpp
> > Line 1253 (original), 1244 (patched)
> > 
> >
> > It would be great to have some testing that `getVolumePath -> 
> > parseVolumePath` roundtrips. Let's either add a simple dedicated test or, 
> > since this is test code, just add an assertion in e.g., `getVolumePath` 
> > that the created path would roundtrip.

Sure will do. However this function does not decode volume names so extra work 
needs to be done.


> On May 14, 2019, 11:37 a.m., Benjamin Bannier wrote:
> > src/examples/test_csi_plugin.cpp
> > Lines 1250 (patched)
> > 
> >
> > Can we do that "normalization" when we initialize `workDir` instead?

Doing that seems to increase the coupling between the initialization of 
`workDir`, which happens in the constructor, and the code here. Dropping.


> On May 14, 2019, 11:37 a.m., Benjamin Bannier wrote:
> > src/examples/test_csi_plugin.cpp
> > Line 1269 (original), 1269 (patched)
> > 
> >
> > We are leaving `id` default-initialized here.

No it's the context which got default-initialized. Let me make it explicit here.


- Chun-Hung


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


On May 10, 2019, 1:17 a.m., Chun-Hung Hsiao wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/70621/
> ---
> 
> (Updated May 10, 2019, 1:17 a.m.)
> 
> 
> Review request for mesos and Benjamin Bannier.
> 
> 
> Bugs: MESOS-9395
> https://issues.apache.org/jira/browse/MESOS-9395
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> The full paths of simulated volumes are now in their ID instead of their
> contextual information. This simplifies SLRP tests, and makes it cleaner
> if we want to customize the contextual information in the future.
> 
> 
> Diffs
> -
> 
>   src/examples/test_csi_plugin.cpp 03f782ead136a79c4c5b099496072933f6737598 
>   src/tests/storage_local_resource_provider_tests.cpp 
> ecd7aeef1cb3d1a5b4b3419dfd912d41a8c6 
> 
> 
> Diff: https://reviews.apache.org/r/70621/diff/1/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Chun-Hung Hsiao
> 
>

Re: Review Request 70678: Add containerizer support for masking paths.

[Jason Lai]

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


Fix it, then Ship it!




Good stuff! LGTM overall with some nits.


src/slave/containerizer/mesos/isolators/filesystem/linux.cpp
Lines 500 (patched)


Nit: `return false` first, so we won't need to nest the logic inside the 
previous `if` statement.



src/slave/containerizer/mesos/isolators/filesystem/linux.cpp
Lines 816 (patched)


Nit: I feel we should consider making the masked paths an instance variable 
of the isolator class and initializing it with `ROOTFS_MASKED_PATHS` instead, 
in the purpose of avoid hard coding.


- Jason Lai


On May 20, 2019, 1:41 a.m., James Peach wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/70678/
> ---
> 
> (Updated May 20, 2019, 1:41 a.m.)
> 
> 
> Review request for mesos, Gilbert Song, Jason Lai, and Jie Yu.
> 
> 
> Bugs: MESOS-9771
> https://issues.apache.org/jira/browse/MESOS-9771
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Add support to the `filesystem/linux` isolator for masking container
> paths. Add a set of standard default paths to be masked, as derived
> from commonly used container runtimes. These paths either expose
> information about other system processes, or capabilities that
> should not be exposed to untrusted containers.
> 
> We don't mask if the container is privileged, which is defined
> as sharing the host's PID namespace. For nested containers, we
> verify that the PID namespace is shared from the host all the way
> up the tree.
> 
> 
> Diffs
> -
> 
>   include/mesos/slave/containerizer.proto 
> 48ffa2e6bd1a03f3dc68a3a78d883855f14bf10c 
>   src/slave/containerizer/mesos/isolators/filesystem/linux.cpp 
> 7b50258ef5480c1ea3f0016aace3b838395becfd 
>   src/slave/containerizer/mesos/launch.cpp 
> 88b97a572916defbe65692036be77395053eb8e8 
>   src/tests/containerizer/linux_filesystem_isolator_tests.cpp 
> 60e9ae5970a0a45314d0b3569556bef36d350d2b 
>   src/tests/containerizer/rootfs.cpp 48eb0108cf26729a0528528a1102247410cf80fe 
> 
> 
> Diff: https://reviews.apache.org/r/70678/diff/3/
> 
> 
> Testing
> ---
> 
> sudo make check (Fedora 30)
> 
> 
> Thanks,
> 
> James Peach
> 
>

Re: Review Request 70696: Renamed sets of role names from 'roles' into 'roleNames'.

[Mesos Reviewbot]

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



Patch looks great!

Reviews applied: [70693, 70694, 70695, 70696]

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

- Mesos Reviewbot


On May 21, 2019, 5:35 p.m., Andrei Sekretenko wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/70696/
> ---
> 
> (Updated May 21, 2019, 5:35 p.m.)
> 
> 
> Review request for mesos, Benjamin Mahler and Meng Zhu.
> 
> 
> Bugs: MESOS-9701
> https://issues.apache.org/jira/browse/MESOS-9701
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This patch renames the sets of strings named `roles` into `roleNames`
> throughout the hierarchial allocator code. This is done to keep
> consistency with the renaming of `string role` introduced in the
> preceding patch and to avoid confusing readers by the name `roles`
> after `struct Role` is introduced.
> 
> 
> Diffs
> -
> 
>   src/master/allocator/mesos/hierarchical.hpp 
> c2058baca5159da4cdcab77afd5de3c0d5ae6c48 
>   src/master/allocator/mesos/hierarchical.cpp 
> 6df9a3b4ef470e57862815a862494cdb5e26d3e3 
> 
> 
> Diff: https://reviews.apache.org/r/70696/diff/1/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Andrei Sekretenko
> 
>

Re: Review Request 70628: Return 409 if `UPDATE_RESOURCE_PROVIDER_CONFIG` names a missing config.

[Benjamin Bannier]

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


Ship it!




Ship It!

- Benjamin Bannier


On May 11, 2019, 1:07 a.m., Chun-Hung Hsiao wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/70628/
> ---
> 
> (Updated May 11, 2019, 1:07 a.m.)
> 
> 
> Review request for mesos, Benjamin Bannier, Greg Mann, and James DeFelice.
> 
> 
> Bugs: MESOS-9779
> https://issues.apache.org/jira/browse/MESOS-9779
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Since 404 is returned if the API endpoint route is not set yet, this
> error code becomes ambiguous and makes clients hard to programmatically
> handle it. Therefore, the error code for specifying a missing config
> in this API call is changed to 409 Conflict.
> 
> 
> Diffs
> -
> 
>   include/mesos/agent/agent.proto ff408a4a1f144237e7425961aa3d11cba163e7c0 
>   include/mesos/v1/agent/agent.proto 19d6c424365f8fd6e6d4a5bb7a61fbe5a66f031d 
>   src/slave/http.cpp 2c4e792d16ad4fd3303760a9db3cba4269152e7d 
>   src/tests/agent_resource_provider_config_api_tests.cpp 
> 7185ac00b5137c7ab208b623e36d03ffd43aab6e 
> 
> 
> Diff: https://reviews.apache.org/r/70628/diff/1/
> 
> 
> Testing
> ---
> 
> sudo make check
> 
> 
> Thanks,
> 
> Chun-Hung Hsiao
> 
>

Review Request 70694: Renamed `suppressedRoles` in the allocator implementation.

[Andrei Sekretenko]

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

Review request for mesos, Benjamin Mahler and Meng Zhu.


Repository: mesos


Description
---

This patch renames `suppressedRoles` into `suppressedRoleNames`
to keep consistency with the preceding rename of `string rolea` and
to avoid confusing readers with the name `suppressedRoles` after the
`struct Role` containing role-specific data is introduced.


Diffs
-

  src/master/allocator/mesos/hierarchical.hpp 
c2058baca5159da4cdcab77afd5de3c0d5ae6c48 
  src/master/allocator/mesos/hierarchical.cpp 
6df9a3b4ef470e57862815a862494cdb5e26d3e3 


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


Testing
---

make check


Thanks,

Andrei Sekretenko

Review Request 70695: Renamed `*Roles` into `*RoleNames` in the allocator code.

[Andrei Sekretenko]

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

Review request for mesos, Benjamin Mahler and Meng Zhu.


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


Repository: mesos


Description
---

This patch renames variables like `set& newRoles`
to keep consistency with renaming of `string role` into
`string RoleName` and to avoid confusing readers after `struct Role`
containing role-specific data is introduced.


Diffs
-

  src/master/allocator/mesos/allocator.hpp 
2d83f382eed9d9dca218adf84bc03883f7486349 
  src/master/allocator/mesos/hierarchical.cpp 
6df9a3b4ef470e57862815a862494cdb5e26d3e3 


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


Testing
---

make check


Thanks,

Andrei Sekretenko

Review Request 70696: Renamed sets of role names from 'roles' into 'roleNames'.

[Andrei Sekretenko]

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

Review request for mesos, Benjamin Mahler and Meng Zhu.


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


Repository: mesos


Description
---

This patch renames the sets of strings named `roles` into `roleNames`
throughout the hierarchial allocator code. This is done to keep
consistency with the renaming of `string role` introduced in the
preceding patch and to avoid confusing readers by the name `roles`
after `struct Role` is introduced.


Diffs
-

  src/master/allocator/mesos/hierarchical.hpp 
c2058baca5159da4cdcab77afd5de3c0d5ae6c48 
  src/master/allocator/mesos/hierarchical.cpp 
6df9a3b4ef470e57862815a862494cdb5e26d3e3 


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


Testing
---

make check


Thanks,

Andrei Sekretenko

Review Request 70693: Renamed `role` into `roleName` throughout HierarchicalAllocatorProcess.

[Andrei Sekretenko]

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

Review request for mesos, Benjamin Mahler and Meng Zhu.


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


Repository: mesos


Description
---

This patch renames variables like `string& role` into `string& roleName`
to avoid confusing readers with the name `role` after `struct Role`
containing role-specific data is introduced.


Diffs
-

  src/master/allocator/mesos/hierarchical.hpp 
c2058baca5159da4cdcab77afd5de3c0d5ae6c48 
  src/master/allocator/mesos/hierarchical.cpp 
6df9a3b4ef470e57862815a862494cdb5e26d3e3 


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


Testing
---

make check


Thanks,

Andrei Sekretenko

Re: Review Request 70689: Improved log messages for SSL configuration.

[Mesos Reviewbot]

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



Bad review!

Reviews applied: [70689]

Error:
2019-05-21 16:42:34 URL:https://reviews.apache.org/r/70689/diff/raw/ 
[1223/1223] -> "70689.patch" [1]
error: patch failed: 3rdparty/libprocess/src/openssl.cpp:515
error: 3rdparty/libprocess/src/openssl.cpp: patch does not apply

- Mesos Reviewbot


On May 21, 2019, 2:33 p.m., Benno Evers wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/70689/
> ---
> 
> (Updated May 21, 2019, 2:33 p.m.)
> 
> 
> Review request for mesos and Jan-Philip Gehrcke.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Improved log messages for SSL configuration.
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/src/openssl.cpp 
> 789bef61b4bb38fa83ab2a016fd780ec97a8e6b7 
> 
> 
> Diff: https://reviews.apache.org/r/70689/diff/1/
> 
> 
> Testing
> ---
> 
> No.
> 
> 
> Thanks,
> 
> Benno Evers
> 
>

Re: Review Request 70534: Added tests for the V1 UPDATE_FRAMEWORK call.

[Mesos Reviewbot]

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



Patch looks great!

Reviews applied: [70532, 70663, 70664, 70665, 70668, 70669, 70670, 70533, 
70671, 70534]

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

- Mesos Reviewbot


On May 21, 2019, 2:10 p.m., Andrei Sekretenko wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/70534/
> ---
> 
> (Updated May 21, 2019, 2:10 p.m.)
> 
> 
> Review request for mesos and Benjamin Mahler.
> 
> 
> Bugs: MESOS-7258
> https://issues.apache.org/jira/browse/MESOS-7258
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added tests for the V1 UPDATE_FRAMEWORK call.
> 
> 
> Diffs
> -
> 
>   src/Makefile.am 5f97523fbe2d80733fbdcc7706f2761f5a071f9f 
>   src/tests/CMakeLists.txt e6b1d8a097246f0921467d7f778034d6754fcff6 
>   src/tests/master/update_framework_tests.cpp PRE-CREATION 
> 
> 
> Diff: https://reviews.apache.org/r/70534/diff/7/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Andrei Sekretenko
> 
>

Review Request 70691: Added more comments regarding `message QuotaConfig`.

[Meng Zhu]

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

Review request for mesos, Alexander Rukletsov and Benjamin Mahler.


Repository: mesos


Description
---

Added more comments regarding `message QuotaConfig`.


Diffs
-

  include/mesos/v1/quota/quota.proto b109ca2a9c7c4b8f1444cc654298c9e580d0ca42 


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


Testing
---

N/A


Thanks,

Meng Zhu

Review Request 70690: Added `repeated QuotaConfig` to `QuotaStatus`.

[Meng Zhu]

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

Review request for mesos and Benjamin Mahler.


Repository: mesos


Description
---

Also marked the `infos` field as deprecated.

`QuotaStatus` is returned by `GET_QUOTA` and `GET /quota`.
As we introduce quota limits, a new mesage `QuotaConfig`
is introduced to describe the quota configuration. For
backwards compatibility, we will fill in both fields
until `QuotaInfo` is removed (in Mesos 2.0).


Diffs
-

  include/mesos/quota/quota.proto 682c0cc40529839ad90c5f412532269861584b71 
  include/mesos/v1/quota/quota.proto b109ca2a9c7c4b8f1444cc654298c9e580d0ca42 


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


Testing
---

make check


Thanks,

Meng Zhu

Re: Review Request 70689: Improved log messages for SSL configuration.

[Jan-Philip Gehrcke via Review Board]

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


Ship it!




Ship It!

- Jan-Philip Gehrcke


On May 21, 2019, 2:33 p.m., Benno Evers wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/70689/
> ---
> 
> (Updated May 21, 2019, 2:33 p.m.)
> 
> 
> Review request for mesos and Jan-Philip Gehrcke.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Improved log messages for SSL configuration.
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/src/openssl.cpp 
> 789bef61b4bb38fa83ab2a016fd780ec97a8e6b7 
> 
> 
> Diff: https://reviews.apache.org/r/70689/diff/1/
> 
> 
> Testing
> ---
> 
> No.
> 
> 
> Thanks,
> 
> Benno Evers
> 
>

Review Request 70689: Improved log messages for SSL configuration.

[Benno Evers]

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

Review request for mesos and Jan-Philip Gehrcke.


Repository: mesos


Description
---

Improved log messages for SSL configuration.


Diffs
-

  3rdparty/libprocess/src/openssl.cpp 789bef61b4bb38fa83ab2a016fd780ec97a8e6b7 


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


Testing
---

No.


Thanks,

Benno Evers

Re: Review Request 70534: Added tests for the V1 UPDATE_FRAMEWORK call.

[Andrei Sekretenko]

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

(Updated May 21, 2019, 2:10 p.m.)


Review request for mesos and Benjamin Mahler.


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


Repository: mesos


Description
---

Added tests for the V1 UPDATE_FRAMEWORK call.


Diffs (updated)
-

  src/Makefile.am 5f97523fbe2d80733fbdcc7706f2761f5a071f9f 
  src/tests/CMakeLists.txt e6b1d8a097246f0921467d7f778034d6754fcff6 
  src/tests/master/update_framework_tests.cpp PRE-CREATION 


Diff: https://reviews.apache.org/r/70534/diff/7/

Changes: https://reviews.apache.org/r/70534/diff/6-7/


Testing
---


Thanks,

Andrei Sekretenko

Re: Review Request 70534: Added tests for the V1 UPDATE_FRAMEWORK call.

[Andrei Sekretenko]

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

(Updated May 21, 2019, 1:56 p.m.)


Review request for mesos and Benjamin Mahler.


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


Repository: mesos


Description
---

Added tests for the V1 UPDATE_FRAMEWORK call.


Diffs (updated)
-

  src/Makefile.am 5f97523fbe2d80733fbdcc7706f2761f5a071f9f 
  src/tests/CMakeLists.txt e6b1d8a097246f0921467d7f778034d6754fcff6 
  src/tests/master/update_framework_tests.cpp PRE-CREATION 


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

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


Testing
---


Thanks,

Andrei Sekretenko

Re: Review Request 70671: Added a class for setting expectations on master V1 API events in tests.

[Andrei Sekretenko]

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

(Updated May 21, 2019, 1:54 p.m.)


Review request for mesos and Benjamin Mahler.


Repository: mesos


Description
---

This patch introduces a class with mock methods for subscribing to the
events of master's V1 streaming API and setting expectations for them.


Diffs (updated)
-

  src/Makefile.am 5f97523fbe2d80733fbdcc7706f2761f5a071f9f 
  src/tests/CMakeLists.txt e6b1d8a097246f0921467d7f778034d6754fcff6 
  src/tests/mock_master_api_subscriber.hpp PRE-CREATION 
  src/tests/mock_master_api_subscriber.cpp PRE-CREATION 


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

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


Testing
---


Thanks,

Andrei Sekretenko

Re: Review Request 70533: Implemented the UPDATE_FRAMEWORK call in the V1 API.

[Andrei Sekretenko]

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

(Updated May 21, 2019, 1:54 p.m.)


Review request for mesos and Benjamin Mahler.


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


Repository: mesos


Description
---

This patch is based on the previous implementation attempt:
https://reviews.apache.org/r/66229/


Diffs (updated)
-

  src/master/http.cpp 1618794cdb9f44656e697de2abbf6c7e0b6fa50d 
  src/master/master.hpp bf7a6b4720ac2e7a072f145f6a27e96a1a9da41e 
  src/master/master.cpp 24bd9572741c033c02d03a3a3a35d7d4ed8b60de 
  src/master/validation.cpp 39f5bfdb39fd6944a3e893b1587e3c2e26818d27 


Diff: https://reviews.apache.org/r/70533/diff/7/

Changes: https://reviews.apache.org/r/70533/diff/6-7/


Testing
---


Thanks,

Andrei Sekretenko

Re: Review Request 70670: Simplified the `Framework::update()` method.

[Andrei Sekretenko]

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

(Updated May 21, 2019, 1:53 p.m.)


Review request for mesos and Benjamin Mahler.


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


Repository: mesos


Description
---

Now that ignoring some fields on re-subscription is performed outside
of this method, we can replace the fragile filed-by-field copying
of `FrameworkInfo` on updates with copying it as a whole.


Diffs (updated)
-

  src/master/framework.cpp 05f5514c589b2dba08afe77281e5fbc4e29f232b 


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

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


Testing
---


Thanks,

Andrei Sekretenko

Re: Review Request 70669: Made it possible to validate against `user` and `checkpoint` updates.

[Andrei Sekretenko]

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

(Updated May 21, 2019, 1:49 p.m.)


Review request for mesos and Benjamin Mahler.


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


Repository: mesos


Description
---

This patch adds `user` and `checkpoint` fields of `FrameworkInfo`
into the `validateUpdate()` function for the purpose of using this
function to implement the UPDATE_FRAMEWORK call. To preserve the legacy
behaviour of re-subscription which silently ignores these fields,
the re-subscription path now explicitly sets them to their old values.


Diffs (updated)
-

  src/master/master.cpp 24bd9572741c033c02d03a3a3a35d7d4ed8b60de 
  src/master/validation.hpp 97c24064b127b1625a58fbc8ac189f18599b0495 
  src/master/validation.cpp 39f5bfdb39fd6944a3e893b1587e3c2e26818d27 
  src/tests/master_validation_tests.cpp 
f102906a4d4fd54328e91fef33eb1b34aa6ce646 


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

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


Testing
---


Thanks,

Andrei Sekretenko

Re: Review Request 70668: Fixed the race between validating and applying FrameworkInfo updates.

[Andrei Sekretenko]

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

(Updated May 21, 2019, 1:47 p.m.)


Review request for mesos and Benjamin Mahler.


Bugs: MESOS-7258 and MESOS-9763
https://issues.apache.org/jira/browse/MESOS-7258
https://issues.apache.org/jira/browse/MESOS-9763


Repository: mesos


Description
---

This patch moves the FrameworkInfo update validation into the
continuation of the SUBSCRIBE calls (after authorization). This fixes
the race between two concurrent SUBSCRIBE calls attempting to change
principal (see MESOS-9763) and prevents the future changes of the update
validation from exacerbating this race.


Diffs (updated)
-

  src/master/master.cpp 24bd9572741c033c02d03a3a3a35d7d4ed8b60de 


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

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


Testing
---


Thanks,

Andrei Sekretenko

Re: Review Request 70665: Moved the logic of sending 'Framework' updates into a separate method.

[Andrei Sekretenko]

> On May 20, 2019, 6:19 p.m., Benjamin Mahler wrote:
> > src/master/master.hpp
> > Lines 667 (patched)
> > 
> >
> > Why did this need to be passed separately? Is there some reason 
> > `framework->pid` can't be used?

I checked, looks like `framework->pid` is updated correctly on all the 
re-subscription paths. So there is no need to pass this separately. Switched to 
`framework->pid`.


> On May 20, 2019, 6:19 p.m., Benjamin Mahler wrote:
> > src/master/master.cpp
> > Lines 2832-2835 (original), 2839-2842 (patched)
> > 
> >
> > This looks like a bug, the pid needs to be set here in the non http api 
> > case.

Certainly a bug.

Looks like we are not testing this anywhere.
There seems to be exactly one test which has checking UpdateFrameworkMessage 
among its purposes:
https://github.com/apache/mesos/blob/eecb82c77117998af0c67a53c64e9b1e975acfa4/src/tests/fault_tolerance_tests.cpp#L1432
I think I'll have to add a check for PID there.


- Andrei


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


On May 21, 2019, 1:40 p.m., Andrei Sekretenko wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/70665/
> ---
> 
> (Updated May 21, 2019, 1:40 p.m.)
> 
> 
> Review request for mesos and Benjamin Mahler.
> 
> 
> Bugs: MESOS-7258
> https://issues.apache.org/jira/browse/MESOS-7258
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This patch consolidates the scattered logic of sending Framework updates
> (namely, the FRAMEWORK_ADDED event to master V1 API subscribers and
> UpdateFrameworkMessage to agents).
> 
> NOTE: The UpdateFrameworkMessage is now also being sent in the case of
> non-`forced` subscription request from a framework which has already
> been registered. Previously it was not being sent in this case -
> this likely is a bug.
> 
> 
> Diffs
> -
> 
>   src/master/master.hpp bf7a6b4720ac2e7a072f145f6a27e96a1a9da41e 
>   src/master/master.cpp 24bd9572741c033c02d03a3a3a35d7d4ed8b60de 
> 
> 
> Diff: https://reviews.apache.org/r/70665/diff/2/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Andrei Sekretenko
> 
>

Re: Review Request 70665: Moved the logic of sending 'Framework' updates into a separate method.

[Andrei Sekretenko]

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

(Updated May 21, 2019, 1:40 p.m.)


Review request for mesos and Benjamin Mahler.


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


Repository: mesos


Description
---

This patch consolidates the scattered logic of sending Framework updates
(namely, the FRAMEWORK_ADDED event to master V1 API subscribers and
UpdateFrameworkMessage to agents).

NOTE: The UpdateFrameworkMessage is now also being sent in the case of
non-`forced` subscription request from a framework which has already
been registered. Previously it was not being sent in this case -
this likely is a bug.


Diffs (updated)
-

  src/master/master.hpp bf7a6b4720ac2e7a072f145f6a27e96a1a9da41e 
  src/master/master.cpp 24bd9572741c033c02d03a3a3a35d7d4ed8b60de 


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

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


Testing
---


Thanks,

Andrei Sekretenko

Re: Review Request 70685: Removed the `TaskStatusUpdateIsTerminalState` matcher.

[Benjamin Bannier]

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


Ship it!




Ship It!

- Benjamin Bannier


On May 21, 2019, 12:34 a.m., Chun-Hung Hsiao wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/70685/
> ---
> 
> (Updated May 21, 2019, 12:34 a.m.)
> 
> 
> Review request for mesos, Benjamin Bannier and Gilbert Song.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> The `TaskStatusUpdateIsTerminalState` test matcher does not handle both
> v0 and v1 status updates. Since introducing such a matcher in the
> `v1::scheduler` namespace is inconsistent and it is not hard to use the
> built-in `Truly` matcher to implement the same functionality, this
> matcher is removed for now.
> 
> 
> Diffs
> -
> 
>   src/tests/containerizer/nvidia_gpu_isolator_tests.cpp 
> 7f5bd8cd4c07b724fe2c49068bf327e5afe9ec41 
>   src/tests/mesos.hpp e70e0e1bbebef41557f9bb1f26c379357fc74e2a 
> 
> 
> Diff: https://reviews.apache.org/r/70685/diff/1/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Chun-Hung Hsiao
> 
>

Re: Review Request 70620: Made SLRP allow changes in volume context.

[Benjamin Bannier]

> On May 14, 2019, 12:04 p.m., Benjamin Bannier wrote:
> > src/resource_provider/storage/provider.cpp
> > Line 923 (original), 930 (patched)
> > 
> >
> > Let's create a dedicated error message.
> 
> Chun-Hung Hsiao wrote:
> Do you mean printing out an error or making it a hard failure? In theory 
> this should never fail and that's why I make it a hard assertion here.

I meant printing out some additional context around a possible error. Thinking 
about it again it might not be worth the hassle though. Dropping.


> On May 14, 2019, 12:04 p.m., Benjamin Bannier wrote:
> > src/resource_provider/storage/provider.cpp
> > Lines 1067 (patched)
> > 
> >
> > I don't think we need optional semantics here, I'd suggest to just work 
> > with an empty `Labels` here.
> > 
> > If you want you can convert an empty `Labels` to a `None` when invoking 
> > `createRawDiskResource` below. I think even that function might not need 
> > optional semantics for labels though.
> 
> Chun-Hung Hsiao wrote:
> This is for maintaining the following proto2 <-> proto3 semantic, since 
> proto3 doesn't have an OPTIONAL semantic, instead any field w/ the default 
> value will not go onto the wire:
> 
> Resource.DiskInfo.Source.has_metadata() <=> !VolumeInfo.context.empty()
> 
> Since in practice there's no difference, we can change this to set up 
> `metadata`to an empty `Labels` even if the `context` map is empty, as long as 
> we handle backward compatibility carefully. But if we don't do this, it seems 
> more reasonable to me to pass in a `None` here than passing a `Labels` here 
> and doing a special check in another function.

Makes sense, dropping.


- Benjamin


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


On May 10, 2019, 3:14 a.m., Chun-Hung Hsiao wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/70620/
> ---
> 
> (Updated May 10, 2019, 3:14 a.m.)
> 
> 
> Review request for mesos, Benjamin Bannier and James DeFelice.
> 
> 
> Bugs: MESOS-9395
> https://issues.apache.org/jira/browse/MESOS-9395
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> To make SLRP more robust against non-conforming CSI plugins that change
> volume contexts, the `getExistVolumes` method returns a list of resource
> conversions consisting of one for converting old volume contexts to new
> volume contexts, and one to remove missing volumes and add new volumes.
> 
> To make the interfaces consistent, `getStoragePools` now also returns a
> list of resource conversions consisting of one conversion.
> 
> 
> Diffs
> -
> 
>   include/mesos/mesos.proto dc6a87f14864cb4d46b9856f251c2946a475162d 
>   include/mesos/v1/mesos.proto e8086e0f70cac73876a8ae31db3365b5059b5c44 
>   src/resource_provider/storage/provider.cpp 
> 999fe95bb6f38f5a25068accd854b37788b24028 
> 
> 
> Diff: https://reviews.apache.org/r/70620/diff/1/
> 
> 
> Testing
> ---
> 
> sudo make check
> more testing done later in chain
> 
> 
> Thanks,
> 
> Chun-Hung Hsiao
> 
>

Re: Review Request 70686: Added a unit test for master operation authorization.

[Benjamin Bannier]

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


Fix it, then Ship it!




This is a very exhaustive test, but slightly hard to follow.


src/tests/master_authorization_tests.cpp
Lines 3317-3320 (patched)


If you get rid of the `vector` `evolve` function in r/70683/ we'd create a 
temporary `RepeatedPtrField` and convert when creating the accept call.


RepeatedPtrField operations_ =
  evolve(
  {operations.begin(), operations.end()});
  
mesos.send(v1::createCallAccept(
subscribed->framework_id(),
offers->offers(0),
{operations_.begin(), operations_.end()}));




src/tests/mesos.hpp
Line 1818 (original), 1860 (patched)


Unintentional edit?


- Benjamin Bannier


On May 21, 2019, 12:37 a.m., Chun-Hung Hsiao wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/70686/
> ---
> 
> (Updated May 21, 2019, 12:37 a.m.)
> 
> 
> Review request for mesos, Benjamin Bannier, Gastón Kleiman, and Greg Mann.
> 
> 
> Bugs: MESOS-9485
> https://issues.apache.org/jira/browse/MESOS-9485
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This test verifies that allowing or denying an action will only result
> in a success or failure on specific operations but not other operations
> in an accept call. This is a regression test for MESOS-9474 and
> MESOS-9480.
> 
> 
> Diffs
> -
> 
>   src/slave/slave.hpp c740bf7c4af1699dd71a31f9169d29c1b4adb40d 
>   src/tests/master_authorization_tests.cpp 
> f65b6210a8189ec5e9089bce845a46d325253058 
>   src/tests/mesos.hpp e70e0e1bbebef41557f9bb1f26c379357fc74e2a 
>   src/tests/mock_slave.hpp 326a450eb2ae9912a19c6e0220e80f74d2953add 
>   src/tests/mock_slave.cpp 1122c2a8b56b9d4e5deea4e892d81061f5e39a8d 
> 
> 
> Diff: https://reviews.apache.org/r/70686/diff/1/
> 
> 
> Testing
> ---
> 
> make check
> 
> Ran the unit test under stress for 500 iterations.
> 
> 
> Thanks,
> 
> Chun-Hung Hsiao
> 
>

Re: Review Request 70683: Added helpers for evolving operation-related proto messages.

[Benjamin Bannier]

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


Fix it, then Ship it!





src/internal/evolve.hpp
Lines 106-114 (original), 122-133 (patched)


This helper doesn't seem to fit well, and we'd only use this helper in a 
single spot later on in the chain.

I'd suggest to remove it here and just manually adjust in the later patch.


- Benjamin Bannier


On May 21, 2019, 12:30 a.m., Chun-Hung Hsiao wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/70683/
> ---
> 
> (Updated May 21, 2019, 12:30 a.m.)
> 
> 
> Review request for mesos, Benjamin Bannier and Greg Mann.
> 
> 
> Bugs: MESOS-9485
> https://issues.apache.org/jira/browse/MESOS-9485
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added helpers for evolving operation-related proto messages.
> 
> 
> Diffs
> -
> 
>   src/internal/evolve.hpp 1044d9df75b6fc1f60d3704be9cb5751e6d4321d 
>   src/internal/evolve.cpp 19c155967bf090fb2ec39211805ff1385787ab59 
> 
> 
> Diff: https://reviews.apache.org/r/70683/diff/1/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Chun-Hung Hsiao
> 
>

Re: Review Request 70684: Changed the `*TaskIdEq` test matchers to take a `TaskID`.

[Benjamin Bannier]

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


Ship it!




- Benjamin Bannier


On May 21, 2019, 12:32 a.m., Chun-Hung Hsiao wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/70684/
> ---
> 
> (Updated May 21, 2019, 12:32 a.m.)
> 
> 
> Review request for mesos, Benjamin Bannier and Greg Mann.
> 
> 
> Bugs: MESOS-9485
> https://issues.apache.org/jira/browse/MESOS-9485
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> The `TaskStatusTaskIdEq` and `TaskStatusUpdateTaskIdEq` matchers
> previously take a `TaskInfo`, which is not very intuitive and
> inconsistent with other matchers such as `OptionTaskHasTaskID`. By
> making these matchers take a `TaskID` we also reduce coupling of this
> matcher and the structure of `TaskInfo`.
> 
> 
> Diffs
> -
> 
>   src/tests/api_tests.cpp bc19d7e9661f091aebf3072967548c7c3196239d 
>   src/tests/containerizer/nvidia_gpu_isolator_tests.cpp 
> 7f5bd8cd4c07b724fe2c49068bf327e5afe9ec41 
>   src/tests/default_executor_tests.cpp 
> 93d7a1cfa1a9df838426c8257b6158e3553b5037 
>   src/tests/gc_tests.cpp a583f1d79a9dd9def24ccfeb9e49ea0392173420 
>   src/tests/master_tests.cpp 964d935771a99efaee63187affe46b551146f310 
>   src/tests/mesos.hpp e70e0e1bbebef41557f9bb1f26c379357fc74e2a 
>   src/tests/partition_tests.cpp 8148aff6bf1d9637d486b549d7f8c7124566d86c 
>   src/tests/slave_tests.cpp 50882a5aa1b20c234f3cec8366f346fa9bfd4f83 
> 
> 
> Diff: https://reviews.apache.org/r/70684/diff/1/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Chun-Hung Hsiao
> 
>

Re: Review Request 70618: Store framework sorters inside RoleInfos.

[Meng Zhu]

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



After some thought, I think we should not do the refactoring at the moment. I 
think a good next step is to deprecate the "sorter" such that each `roleInfo` 
does not need to have a sorter instance. The fact that when constructing a 
`RoleInfo` we need to take in global args `options` and add all slave resources 
is unfortunate. This also prevents RoleInfo from having a default constructor 
which prevents us from using the map insertion interface.

But there are some good points in this patch (e.g. comments fix), lets pull 
those out if possible.

- Meng Zhu


On May 20, 2019, 9:25 a.m., Andrei Sekretenko wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/70618/
> ---
> 
> (Updated May 20, 2019, 9:25 a.m.)
> 
> 
> Review request for mesos, Benjamin Mahler and Meng Zhu.
> 
> 
> Bugs: MESOS-9701
> https://issues.apache.org/jira/browse/MESOS-9701
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This patch replaces a hashmap> used for tracking
> framework sorters with a unique_ptr inside of the RoleInfo.
> This simplifies the framework tracking logic and slightly improves
> performance.
> 
> 
> Diffs
> -
> 
>   src/master/allocator/mesos/hierarchical.hpp 
> c2058baca5159da4cdcab77afd5de3c0d5ae6c48 
>   src/master/allocator/mesos/hierarchical.cpp 
> 6df9a3b4ef470e57862815a862494cdb5e26d3e3 
> 
> 
> Diff: https://reviews.apache.org/r/70618/diff/4/
> 
> 
> Testing
> ---
> 
> make check
> 
> Benchmarking: 5 runs of 
> BENCHMARK_HierarchicalAllocator_WithQuotaParam.LargeAndSmallQuota/5 with the 
> optimized build.
> 
> BEFORE (master):
> 
> Added 3000 agents in 57.444301ms
> Added 3000 frameworks in 15.100499419secs
> Benchmark setup: 3000 agents, 3000 roles, 3000 frameworks, with random sorter
> Made 3856 allocations in 13.359926851secs
> Made 0 allocation in 12.147765014secs
> 
> Added 3000 agents in 58.651887ms
> Added 3000 frameworks in 14.485925735secs
> Benchmark setup: 3000 agents, 3000 roles, 3000 frameworks, with random sorter
> Made 3856 allocations in 13.6526783secs
> Made 0 allocation in 12.138439924secs
> 
> Added 3000 agents in 59.028581ms
> Added 3000 frameworks in 14.72945866secs
> Benchmark setup: 3000 agents, 3000 roles, 3000 frameworks, with random sorter
> Made 3856 allocations in 12.739135078secs
> Made 0 allocation in 12.673302496secs
> 
> Added 3000 agents in 59.050577ms
> Added 3000 frameworks in 14.78273576secs
> Benchmark setup: 3000 agents, 3000 roles, 3000 frameworks, with random sorter
> Made 3856 allocations in 13.20400492secs
> Made 0 allocation in 13.289808943secs
> 
> Added 3000 agents in 58.629888ms
> Added 3000 frameworks in 14.714786337secs
> Benchmark setup: 3000 agents, 3000 roles, 3000 frameworks, with random sorter
> Made 3856 allocations in 13.721094744secs
> Made 0 allocation in 12.688377237secs
> 
> ---
> AFTER (master + previous patch https://reviews.apache.org/r/70591/ + this 
> patch):
> 
> Added 3000 agents in 58.04155ms
> Added 3000 frameworks in 13.694651977secs
> Benchmark setup: 3000 agents, 3000 roles, 3000 frameworks, with random sorter
> Made 3856 allocations in 12.417541213secs
> Made 0 allocation in 12.130755905secs
> 
> Added 3000 agents in 55.246813ms
> Added 3000 frameworks in 13.460479936secs
> Benchmark setup: 3000 agents, 3000 roles, 3000 frameworks, with random sorter
> Made 3856 allocations in 12.393765514secs
> Made 0 allocation in 12.196981426secs
> 
> Added 3000 agents in 58.013477ms
> Added 3000 frameworks in 13.69361015secs
> Benchmark setup: 3000 agents, 3000 roles, 3000 frameworks, with random sorter
> Made 3856 allocations in 12.245916159secs
> Made 0 allocation in 11.699553888secs
> 
> Added 3000 agents in 57.163681ms
> Added 3000 frameworks in 13.738218369secs
> Benchmark setup: 3000 agents, 3000 roles, 3000 frameworks, with random sorter
> Made 3856 allocations in 12.809206431secs
> Made 0 allocation in 12.400140164secs
> 
> Added 3000 agents in 57.942087ms
> Added 3000 frameworks in 13.836390727secs
> Benchmark setup: 3000 agents, 3000 roles, 3000 frameworks, with random sorter
> Made 3856 allocations in 12.75595625secs
> Made 0 allocation in 11.939263998secs
> 
> 
> Thanks,
> 
> Andrei Sekretenko
> 
>

Re: Review Request 70679: Fixed comment to reflect the existence of sorters other than DRF.

[Meng Zhu]

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


Ship it!





src/master/allocator/mesos/hierarchical.cpp
Line 1862 (original), 1862 (patched)


I find the newline here hurts readability a bit, I will remove it when I 
make the push.


- Meng Zhu


On May 20, 2019, 9:26 a.m., Andrei Sekretenko wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/70679/
> ---
> 
> (Updated May 20, 2019, 9:26 a.m.)
> 
> 
> Review request for mesos and Meng Zhu.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Fixed comment to reflect the existence of sorters other than DRF.
> 
> 
> Diffs
> -
> 
>   src/master/allocator/mesos/hierarchical.cpp 
> 6df9a3b4ef470e57862815a862494cdb5e26d3e3 
> 
> 
> Diff: https://reviews.apache.org/r/70679/diff/1/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Andrei Sekretenko
> 
>

Re: Review Request 70591: Added `struct RoleInfo` to track role reservations and framework IDs.

[Meng Zhu]

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


Ship it!




- Meng Zhu


On May 20, 2019, 9:26 a.m., Andrei Sekretenko wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/70591/
> ---
> 
> (Updated May 20, 2019, 9:26 a.m.)
> 
> 
> Review request for mesos, Benjamin Mahler and Meng Zhu.
> 
> 
> Bugs: MESOS-9701
> https://issues.apache.org/jira/browse/MESOS-9701
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This patch introduces struct RoleInfo which contains the framework IDs
> and reservatons tracked under a role. Two separate hashmaps used for
> this purpose previously are replaced by a single hashmap, thus the need
> to keep the two consistent with each other is removed. This simplifies
> the role tracking logic in the allocator. The patch introduces minimal
> to no performance impact.
> 
> 
> Diffs
> -
> 
>   src/master/allocator/mesos/hierarchical.hpp 
> c2058baca5159da4cdcab77afd5de3c0d5ae6c48 
>   src/master/allocator/mesos/hierarchical.cpp 
> 6df9a3b4ef470e57862815a862494cdb5e26d3e3 
> 
> 
> Diff: https://reviews.apache.org/r/70591/diff/5/
> 
> 
> Testing
> ---
> 
> make check
> 
> Benchmarking: 5 runs of 
> `BENCHMARK_HierarchicalAllocator_WithQuotaParam.LargeAndSmallQuota/5` with 
> the optimized build.
> 
> Performance impact in this benchmark seems to be negligible.
> 
> BEFORE:
> Added 3000 agents in 51.553929ms
> Added 3000 frameworks in 15.174748344secs
> Benchmark setup: 3000 agents, 3000 roles, 3000 frameworks, with random sorter
> Made 3856 allocations in 12.400805171secs
> Made 0 allocation in 12.5850238secs
> 
> Added 3000 agents in 55.739336ms
> Added 3000 frameworks in 14.730404769secs
> Benchmark setup: 3000 agents, 3000 roles, 3000 frameworks, with random sorter
> Made 3856 allocations in 12.563439682secs
> Made 0 allocation in 13.063555055secs
> 
> Added 3000 agents in 54.414733ms
> Added 3000 frameworks in 15.10136842secs
> Benchmark setup: 3000 agents, 3000 roles, 3000 frameworks, with random sorter
> Made 3856 allocations in 13.501664915secs
> Made 0 allocation in 12.89034382secs
> 
> Added 3000 agents in 52.58252ms
> Added 3000 frameworks in 14.048350298secs
> Benchmark setup: 3000 agents, 3000 roles, 3000 frameworks, with random sorter
> Made 3856 allocations in 12.299952145secs
> Made 0 allocation in 11.888248811secs
> 
> Added 3000 agents in 52.821439ms
> Added 3000 frameworks in 15.344450583secs
> Benchmark setup: 3000 agents, 3000 roles, 3000 frameworks, with random sorter
> Made 3856 allocations in 12.63425secs
> Made 0 allocation in 12.427171541secs
> 
> 
> AFTER:
> 
> Added 3000 agents in 69.716648ms
> Added 3000 frameworks in 15.249001979secs
> Benchmark setup: 3000 agents, 3000 roles, 3000 frameworks, with random sorter
> Made 3856 allocations in 12.860494226secs
> Made 0 allocation in 12.228866329secs
> 
> Added 3000 agents in 52.639388ms
> Added 3000 frameworks in 15.207895482secs
> Benchmark setup: 3000 agents, 3000 roles, 3000 frameworks, with random sorter
> Made 3856 allocations in 12.504777266secs
> Made 0 allocation in 12.70388062secs
> 
> Added 3000 agents in 56.865794ms
> Added 3000 frameworks in 15.284003915secs
> Benchmark setup: 3000 agents, 3000 roles, 3000 frameworks, with random sorter
> Made 3856 allocations in 13.86859815secs
> Made 0 allocation in 12.538958231secs
> 
> Added 3000 agents in 56.028013ms
> Added 3000 frameworks in 13.892577869secs
> Benchmark setup: 3000 agents, 3000 roles, 3000 frameworks, with random sorter
> Made 3856 allocations in 12.341724418secs
> Made 0 allocation in 12.23022189secs
> 
> Added 3000 agents in 52.368219ms
> Added 3000 frameworks in 13.978581104secs
> Benchmark setup: 3000 agents, 3000 roles, 3000 frameworks, with random sorter
> Made 3856 allocations in 12.701682501secs
> Made 0 allocation in 12.141360313secs
> 
> 
> Thanks,
> 
> Andrei Sekretenko
> 
>

Re: Review Request 70626: Refactored `untrackReservations()` method.

[Meng Zhu]

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


Ship it!




Ship It!

- Meng Zhu


On May 20, 2019, 9:27 a.m., Andrei Sekretenko wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/70626/
> ---
> 
> (Updated May 20, 2019, 9:27 a.m.)
> 
> 
> Review request for mesos and Meng Zhu.
> 
> 
> Bugs: MESOS-9701
> https://issues.apache.org/jira/browse/MESOS-9701
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This patch gets rid of insertion into the beginning of the vector
> and also of shadowing HierarchicalAllocatorProcess::roles by a local.
> 
> 
> Diffs
> -
> 
>   src/master/allocator/mesos/hierarchical.cpp 
> 6df9a3b4ef470e57862815a862494cdb5e26d3e3 
> 
> 
> Diff: https://reviews.apache.org/r/70626/diff/3/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Andrei Sekretenko
> 
>