Re: Review Request 56757: Added the SecretGenerator module interface.

[Greg Mann]

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




include/mesos/authentication/secret_generator.hpp
Lines 29-32 (patched)


Could you elaborate a bit here? The secret is meant to contain a token (for 
example, an Authorization header) that can be used to authenticate, such that 
an authenticator will return the principal that was passed to the `generate` 
method of this module.


- Greg Mann


On Feb. 28, 2017, 1:37 p.m., Jan Schlicht wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/56757/
> ---
> 
> (Updated Feb. 28, 2017, 1:37 p.m.)
> 
> 
> Review request for mesos, Alexander Rojas and Greg Mann.
> 
> 
> Bugs: MESOS-6997
> https://issues.apache.org/jira/browse/MESOS-6997
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This interface will be used by agents to create credentials for the
> default executor.
> 
> 
> Diffs
> -
> 
>   include/mesos/authentication/secret_generator.hpp PRE-CREATION 
>   include/mesos/module/secret_generator.hpp PRE-CREATION 
>   src/Makefile.am 76d58a0bbd77fe59ff8a81ba7a68c84dc7169750 
> 
> 
> Diff: https://reviews.apache.org/r/56757/diff/2/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Jan Schlicht
> 
>

Review Request 57364: Fixed flaky test FaultToleranceTest.FrameworkReregister.

[Jay Guo]

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

Review request for mesos and Neil Conway.


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


Repository: mesos


Description
---

Fixed flaky test FaultToleranceTest.FrameworkReregister.


Diffs
-

  src/tests/fault_tolerance_tests.cpp b13a7e2527189931b733fb4f188b1463fe1f919a 


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


Testing
---

./bin/mesos-tests.sh --gtest_filter="FaultToleranceTest.FrameworkReregister" 
--gtest_break_on_failure --gtest_repeat=-1


Thanks,

Jay Guo

Re: Review Request 57166: Updated role validation for hierarchical roles.

[Michael Park]

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


Fix it, then Ship it!





src/common/roles.cpp
Line 71 (original), 71-79 (patched)


Newlines in between.



src/common/roles.cpp
Lines 72 (patched)


For these error messages, how about we say something like this?

```cpp
"A role cannot start with a slash, given: '" + role + "'"
```

Here and below.



src/common/roles.cpp
Lines 82 (patched)


`s/pathElements/components/`



src/common/roles.cpp
Line 78 (original), 90 (patched)


`s/element/component/`


- Michael Park


On March 2, 2017, 10:12 a.m., Neil Conway wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/57166/
> ---
> 
> (Updated March 2, 2017, 10:12 a.m.)
> 
> 
> Review request for mesos, Benjamin Bannier and Michael Park.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Role names can now contain forward slashes. Each component in a role
> name must now be validated separately.
> 
> 
> Diffs
> -
> 
>   src/common/roles.cpp 31774a9b8f99f5efeed35b1c3e3486e05ca00f6a 
>   src/tests/role_tests.cpp 77f3d46a544a51ba71476e2f0735bb32758dd9e1 
> 
> 
> Diff: https://reviews.apache.org/r/57166/diff/2/
> 
> 
> Testing
> ---
> 
> `make check`
> 
> 
> Thanks,
> 
> Neil Conway
> 
>

Re: Review Request 57059: Updated default executor tests.

[Mesos Reviewbot]

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



Patch looks great!

Reviews applied: [57058, 57059]

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

- Mesos Reviewbot


On March 7, 2017, 1:17 a.m., Vinod Kone wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/57059/
> ---
> 
> (Updated March 7, 2017, 1:17 a.m.)
> 
> 
> Review request for mesos, Anand Mazumdar and Gilbert Song.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Reorganized so that objects are defined closer to their usage.
> 
> 
> Diffs
> -
> 
>   src/tests/default_executor_tests.cpp 
> e4d43c8ad447577a9c5c7951207596bda1070856 
> 
> 
> Diff: https://reviews.apache.org/r/57059/diff/2/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Vinod Kone
> 
>

Review Request 57360: Replace `.get().` in favor of `->` in tests.

[Jay Guo]

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

Review request for mesos and Benjamin Mahler.


Repository: mesos


Description
---

Instead of using `get()` to fetch data, we are inclined to use
`->` operator to direct access its methods.


Diffs
-

  src/tests/api_tests.cpp 607392ff6b714a9e812c2802f4d1465e8f71ad09 
  src/tests/authentication_tests.cpp adbe03043dd75a0ebb0dadb2c0b218c77b58e900 
  src/tests/cluster.cpp 04b70831ec05f715074cd93426c3645572d866ca 
  src/tests/command_executor_tests.cpp de735c62ae4be268b3e37d636f43f120f879a624 
  src/tests/container_logger_tests.cpp 54e5b29ce0668449027bde6185e37dc8b636d8c7 
  src/tests/containerizer/appc_spec_tests.cpp 
840dbde1a16c6a201a65257ff207309f53f57772 
  src/tests/containerizer/cgroups_isolator_tests.cpp 
971ea47ec5d2aed5e38eecf995b70ae4b0fa14a1 
  src/tests/containerizer/cgroups_tests.cpp 
76fabce4530ccc0a1d685cd48d932ced5a64bc58 
  src/tests/containerizer/cni_isolator_tests.cpp 
cb893d3ef005a9cc60c40768fa669b27c4863020 
  src/tests/containerizer/cpu_isolator_tests.cpp 
f117826d9366820fd9ef5a113bb4b3d39deeca4b 
  src/tests/containerizer/docker_containerizer_tests.cpp 
648453328d2f0792f7b5ba89b0351a11f6630be5 
  src/tests/containerizer/docker_spec_tests.cpp 
82bddd228df3db95a00eb277ff9c380039b70b1e 
  src/tests/containerizer/docker_tests.cpp 
452858d508e668fe62826de5558ea332cd4279d5 
  src/tests/containerizer/docker_volume_isolator_tests.cpp 
4040d36d1dbf4968f62fa593024936858f28b4df 
  src/tests/containerizer/fs_tests.cpp f726fb06ac3e74f7f0d6d0310ac14eef60ae4fd9 
  src/tests/containerizer/io_switchboard_tests.cpp 
9031815f5711cf940315ab3d8538aa099b356847 
  src/tests/containerizer/linux_filesystem_isolator_tests.cpp 
51017a3a809ed0a72ea5c986a578344fff9b3276 
  src/tests/containerizer/memory_isolator_tests.cpp 
388e62a9d10fc806f60d84002c10d9a072071228 
  src/tests/containerizer/memory_pressure_tests.cpp 
ff6c2b630bf67a679060bb8afeebc85f5c23a2c5 
  src/tests/containerizer/mesos_containerizer_tests.cpp 
683807f38778095f31d37f5598d037ab87abb66b 
  src/tests/containerizer/nested_mesos_containerizer_tests.cpp 
ea01fe55a28d17105157004d8cf0976202a49b7c 
  src/tests/containerizer/ns_tests.cpp d6be1076a393f7959cd6ac225591c8b3aa66721d 
  src/tests/containerizer/port_mapping_tests.cpp 
3e848165ace8346ead39224a46f8b85a5c4ff495 
  src/tests/containerizer/provisioner_appc_tests.cpp 
eb6509bc56214623fe9a37cbe5227768897f102c 
  src/tests/containerizer/provisioner_backend_tests.cpp 
ab7805f62767c6146588e3181954518d65076b40 
  src/tests/containerizer/provisioner_docker_tests.cpp 
af24b37acb265b5d301e4a5a236ac58c6f25a4a5 
  src/tests/containerizer/routing_tests.cpp 
e6b7eadce3eeab8f7bcb15bc1e55dff78f2a4f51 
  src/tests/containerizer/xfs_quota_tests.cpp 
0fbaddd68af55c51c106962377be20afa599fb97 
  src/tests/cram_md5_authentication_tests.cpp 
cd8607136f963b036f6563dc0947ee0d435f5c68 
  src/tests/credentials_tests.cpp 809f8c744b7e77c9aa42eaf8fe2d9d0ac9a52b4a 
  src/tests/default_executor_tests.cpp e4d43c8ad447577a9c5c7951207596bda1070856 
  src/tests/disk_quota_tests.cpp d2dfc94c5f9875fc9ce05b1c86049f8e06730029 
  src/tests/dynamic_weights_tests.cpp 5571ab7b57059e4781671f45c6e59e7c3b0c42c9 
  src/tests/environment.cpp 4c6c8f6656b62e5d8fbd8d4e8f2d7903f71e884c 
  src/tests/exception_tests.cpp 316c329c0e6cc0aaf93c6c3ca3ab4355d7a7f2ea 
  src/tests/executor_http_api_tests.cpp 
892f8f669392708968ba96e5ed22eac99ae64e18 
  src/tests/fault_tolerance_tests.cpp b13a7e2527189931b733fb4f188b1463fe1f919a 
  src/tests/fetcher_cache_tests.cpp 85246ef610fac958c54b1655361a9b9e82c23200 
  src/tests/fetcher_tests.cpp c4854b9fe4c520efc8ea06cdb42c79c333257eb1 
  src/tests/files_tests.cpp d492adf71ecb22c433f0eba4d974e99f610b5dd3 
  src/tests/gc_tests.cpp 6b6437ca0f0fa93c389e12cf2e9b23273cf23631 
  src/tests/group_tests.cpp 193a158c4d07591584fc64b6baa41f2e90618732 
  src/tests/health_check_tests.cpp 56e90747f2c943daee675738428d8ddeeafde36d 
  src/tests/hook_tests.cpp 95dcfb6655709ea2e177b32ec522ceceeaff90a1 
  src/tests/http_authentication_tests.cpp 
d5fabf0058755502f19eb6385bd99a0d45419508 
  src/tests/http_fault_tolerance_tests.cpp 
aca2466aa42c6434004eba4a43577f961e223c6a 
  src/tests/ldd_tests.cpp 2352399c30674ecaf9535ce4289efb743a2a9e8f 
  src/tests/log_tests.cpp 72182d39aa4e9d092b829316cf7214dbe7b7e98e 
  src/tests/master_allocator_tests.cpp 7b0b786f1c6c53616fd7ae1f7f765752d94a4f83 
  src/tests/master_authorization_tests.cpp 
94107d00a9f4eeac5c68b90bc010e9f78cfb3457 
  src/tests/master_contender_detector_tests.cpp 
2849471471ad44bfc2724ccc072db4533ea4f2a7 
  src/tests/master_maintenance_tests.cpp 
237dc03b0d954f63acc82e3d80a77fae1cad5ae4 
  src/tests/master_slave_reconciliation_tests.cpp 
1c7a3d686e2f924ad14c75fcab2ccafaab6d7b81 
  src/tests/master_tests.cpp b9dfe9eb7d759550673896980cffc3909f4c0105 
  

Re: Review Request 57269: Added a test to ensure allocation roles are exposed in master API.

[Jay Guo]

> On March 7, 2017, 6:38 a.m., Benjamin Mahler wrote:
> > src/tests/master_tests.cpp
> > Lines 4292-4293 (patched)
> > 
> >
> > Are you planning to test the agent endpoint as well?

I modified existing test to include this 
https://github.com/apache/mesos/blob/master/src/tests/slave_tests.cpp#L1645-L1663
 do you think that's no sufficient?


> On March 7, 2017, 6:38 a.m., Benjamin Mahler wrote:
> > src/tests/master_tests.cpp
> > Lines 4356 (patched)
> > 
> >
> > Can you do a sweep for using the -> operator in this test?

Fixed in a separate patch: https://reviews.apache.org/r/57360/


- Jay


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


On March 7, 2017, 11:35 a.m., Jay Guo wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/57269/
> ---
> 
> (Updated March 7, 2017, 11:35 a.m.)
> 
> 
> Review request for mesos and Benjamin Mahler.
> 
> 
> Bugs: MESOS-7158
> https://issues.apache.org/jira/browse/MESOS-7158
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This test ensures that allocation roles of tasks and executors are
> exposed via `/state` endpoint of master v0 API.
> 
> 
> Diffs
> -
> 
>   src/tests/master_tests.cpp b9dfe9eb7d759550673896980cffc3909f4c0105 
> 
> 
> Diff: https://reviews.apache.org/r/57269/diff/2/
> 
> 
> Testing
> ---
> 
> Added a test `MasterTest.StateEndpointAllocationRole`
> 
> make check
> 
> 
> Thanks,
> 
> Jay Guo
> 
>

Re: Review Request 57269: Added a test to ensure allocation roles are exposed in master API.

[Jay Guo]

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

(Updated March 7, 2017, 11:35 a.m.)


Review request for mesos and Benjamin Mahler.


Changes
---

address bmahler's comments.


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


Repository: mesos


Description
---

This test ensures that allocation roles of tasks and executors are
exposed via `/state` endpoint of master v0 API.


Diffs (updated)
-

  src/tests/master_tests.cpp b9dfe9eb7d759550673896980cffc3909f4c0105 


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

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


Testing
---

Added a test `MasterTest.StateEndpointAllocationRole`

make check


Thanks,

Jay Guo

Re: Review Request 56733: Mount all supported subsystems in the containerizer tests.

[Mesos Reviewbot]

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



Patch looks great!

Reviews applied: [56730, 56731, 56732, 56733]

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

- Mesos Reviewbot


On March 6, 2017, 7:34 p.m., James Peach wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/56733/
> ---
> 
> (Updated March 6, 2017, 7:34 p.m.)
> 
> 
> Review request for mesos, Mesos Reviewbot and Jiang Yan Xu.
> 
> 
> Bugs: MESOS-7049
> https://issues.apache.org/jira/browse/MESOS-7049
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Rather than a hard-coded list of subsystems, just mount everything we
> find that is supported.
> 
> 
> Diffs
> -
> 
>   src/tests/mesos.cpp 6a96fa51dfc2a62063c3154b256bdac707b009bb 
> 
> 
> Diff: https://reviews.apache.org/r/56733/diff/2/
> 
> 
> Testing
> ---
> 
> sudo make check (Fedora 25)
> 
> 
> Thanks,
> 
> James Peach
> 
>

Review Request 57358: Fixed potential for circular dependencies in Future continuations.

[Joseph Wu]

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

Review request for mesos, Benjamin Mahler, Jie Yu, and Michael Park.


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


Repository: mesos


Description
---

This commit deals with a specific case where an object passes a
self-reference (shared_ptr) into a Future continuation and then
discards the original Future.

The behavior of `Future::discard` is that the `onDiscard` event
is only chained to the future immediately following the discarded
future.  i.e.

```
Future top;
top
  .onDiscard([]() { /* This gets executed. */ })

  .then([](const A&) { return Future(); })
  .onDiscard([]() { /* This also gets executed. */ })

  .then([](const B&) { return Future(); })
  .onDiscard([]() { /* But not this. */ });

top.discard();
```

When a Future is discarded, we only clear the `onDiscard` callbacks,
which means that all other continuations will continue to reside in
memory.  In the case of the `Socket` class, the Libevent SSL socket
implementation had several levels of continuations:

```
// Each item in this queue is backed by a Promise<>::Future.
Queue> accept_queue;

// LibeventSSLSocketImpl::accept.
accept_queue.get()
  .then(...)

// Socket::accept. This continuation holds a self-reference
// to the `shared_ptr`.
  .then([self](...) {...})
```

Because the second continuation is never discarded nor called, we
end up with a dangling pointer.  For the `Socket` class, this leads
to an FD leak.


Diffs
-

  3rdparty/libprocess/include/process/future.hpp 
ec7ef2251947f5f42a202af0d6cb0858338e7755 


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


Testing
---

cmake .. -DENABLE_LIBEVENT=1 -DENABLE_SSL=1

make libprocess-tests

3rdparty/libprocess/src/tests/libprocess-tests 
--gtest_filter="Scheme/HTTPTest.Endpoints/0" --gtest_repeat="`ulimit -n`"


Thanks,

Joseph Wu

Re: Review Request 56805: Simplified interface for setting weights in allocator.

[Adam B]

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


Ship it!




Ship It!

- Adam B


On Feb. 28, 2017, 12:24 p.m., Neil Conway wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/56805/
> ---
> 
> (Updated Feb. 28, 2017, 12:24 p.m.)
> 
> 
> Review request for mesos, Adam B and Yongqiao Wang.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Now that weights can change dynamically, passing a list of weights to
> the allocator at initialization-time is unnecessary and confusing (e.g.,
> the initial weights might be wrong, if a different set of weight values
> are recovered from the registry).
> 
> Instead, require that weights are communicated to the allocator via the
> existing `updateWeights` method.
> 
> 
> Diffs
> -
> 
>   include/mesos/allocator/allocator.hpp 
> 25ed5f36b186f2bd257dd0bdb366d0b21a795622 
>   src/master/allocator/mesos/allocator.hpp 
> 1defb59b686c9cd8d403a0ed6825219f62a7801d 
>   src/master/allocator/mesos/hierarchical.hpp 
> 0bb24be2761bde6d1baad69f5654029f3ceed553 
>   src/master/allocator/mesos/hierarchical.cpp 
> 696815795dc391b4ec7538892b1224b812482d34 
>   src/master/master.cpp ae36bf477851bf2fe11eb7913c580e3e0b9cbbe5 
>   src/tests/allocator.hpp b6b0022d581bd688900aaf5beb0af7ce6e0129a1 
>   src/tests/api_tests.cpp 607392ff6b714a9e812c2802f4d1465e8f71ad09 
>   src/tests/hierarchical_allocator_tests.cpp 
> cdf1f15b7802439b28405ca8f6634ce83e886630 
>   src/tests/master_allocator_tests.cpp 
> 7b0b786f1c6c53616fd7ae1f7f765752d94a4f83 
>   src/tests/master_quota_tests.cpp 91219d6693fdd119ed3b0bf734eaa55da9c58b0a 
>   src/tests/persistent_volume_endpoints_tests.cpp 
> 1cc6c9d01a3a473f5a44210ea725310ea5931ff6 
>   src/tests/reservation_endpoints_tests.cpp 
> 345f0457ec1fc00b7033d71227ff178c14e015bb 
>   src/tests/reservation_tests.cpp 5c0d01483efb4561b8c0016c3a2fa6ea5574196e 
>   src/tests/resource_offers_tests.cpp 
> 74dacf140e49e402a4ad02ce7751e7c7b2f78ee1 
>   src/tests/slave_recovery_tests.cpp b5b805868bed61bf482d71322fb1918a0d020d48 
> 
> 
> Diff: https://reviews.apache.org/r/56805/diff/2/
> 
> 
> Testing
> ---
> 
> `make check`
> 
> 
> Thanks,
> 
> Neil Conway
> 
>

Re: Review Request 56805: Simplified interface for setting weights in allocator.

[Michael Park]

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


Ship it!




Ship It!

- Michael Park


On Feb. 28, 2017, 12:24 p.m., Neil Conway wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/56805/
> ---
> 
> (Updated Feb. 28, 2017, 12:24 p.m.)
> 
> 
> Review request for mesos, Adam B and Yongqiao Wang.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Now that weights can change dynamically, passing a list of weights to
> the allocator at initialization-time is unnecessary and confusing (e.g.,
> the initial weights might be wrong, if a different set of weight values
> are recovered from the registry).
> 
> Instead, require that weights are communicated to the allocator via the
> existing `updateWeights` method.
> 
> 
> Diffs
> -
> 
>   include/mesos/allocator/allocator.hpp 
> 25ed5f36b186f2bd257dd0bdb366d0b21a795622 
>   src/master/allocator/mesos/allocator.hpp 
> 1defb59b686c9cd8d403a0ed6825219f62a7801d 
>   src/master/allocator/mesos/hierarchical.hpp 
> 0bb24be2761bde6d1baad69f5654029f3ceed553 
>   src/master/allocator/mesos/hierarchical.cpp 
> 696815795dc391b4ec7538892b1224b812482d34 
>   src/master/master.cpp ae36bf477851bf2fe11eb7913c580e3e0b9cbbe5 
>   src/tests/allocator.hpp b6b0022d581bd688900aaf5beb0af7ce6e0129a1 
>   src/tests/api_tests.cpp 607392ff6b714a9e812c2802f4d1465e8f71ad09 
>   src/tests/hierarchical_allocator_tests.cpp 
> cdf1f15b7802439b28405ca8f6634ce83e886630 
>   src/tests/master_allocator_tests.cpp 
> 7b0b786f1c6c53616fd7ae1f7f765752d94a4f83 
>   src/tests/master_quota_tests.cpp 91219d6693fdd119ed3b0bf734eaa55da9c58b0a 
>   src/tests/persistent_volume_endpoints_tests.cpp 
> 1cc6c9d01a3a473f5a44210ea725310ea5931ff6 
>   src/tests/reservation_endpoints_tests.cpp 
> 345f0457ec1fc00b7033d71227ff178c14e015bb 
>   src/tests/reservation_tests.cpp 5c0d01483efb4561b8c0016c3a2fa6ea5574196e 
>   src/tests/resource_offers_tests.cpp 
> 74dacf140e49e402a4ad02ce7751e7c7b2f78ee1 
>   src/tests/slave_recovery_tests.cpp b5b805868bed61bf482d71322fb1918a0d020d48 
> 
> 
> Diff: https://reviews.apache.org/r/56805/diff/2/
> 
> 
> Testing
> ---
> 
> `make check`
> 
> 
> Thanks,
> 
> Neil Conway
> 
>

Re: Review Request 57336: Updated `MultiRoleSchedulerUpgrade` to test framework updates.

[Michael Park]

> On March 6, 2017, 2:18 p.m., Benjamin Mahler wrote:
> > src/tests/upgrade_tests.cpp
> > Lines 519-541 (patched)
> > 
> >
> > Ditto here.

N/A


- Michael


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


On March 6, 2017, 5:33 p.m., Michael Park wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/57336/
> ---
> 
> (Updated March 6, 2017, 5:33 p.m.)
> 
> 
> Review request for mesos and Benjamin Mahler.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Updated `MultiRoleSchedulerUpgrade` to test framework updates.
> 
> 
> Diffs
> -
> 
>   src/tests/upgrade_tests.cpp 6cdd6d989df14af5f17b41af0bf631471feae00e 
> 
> 
> Diff: https://reviews.apache.org/r/57336/diff/3/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Michael Park
> 
>

Re: Review Request 57336: Updated `MultiRoleSchedulerUpgrade` to test framework updates.

[Michael Park]

> On March 6, 2017, 2:18 p.m., Benjamin Mahler wrote:
> > Looks good, wonder if we can just loop over the master and agent pids and 
> > use the same code to check the contents since they should be the same?

Yep. Thanks for the suggestion!


> On March 6, 2017, 2:18 p.m., Benjamin Mahler wrote:
> > src/tests/upgrade_tests.cpp
> > Lines 489-505 (patched)
> > 
> >
> > Have you tried using the contains member function to clean this up?
> > 
> > ```
> > // We check that the following is contained within
> > // the result:
> > //   {
> > // "frameworks":
> > // [
> > //   {
> > // "roles": ["role"]
> > //   }
> > // ]
> > //   }
> > ```
> > 
> > But since we can't do the following:
> > 
> > ```
> > JSON::Object expected = {
> >   {"frameworks", { {{"roles", {"foo"} }} }}
> > };
> > ```
> > 
> > It ends up being pretty tedious.

Introduced https://reviews.apache.org/r/57354/ to allow this.


> On March 6, 2017, 2:18 p.m., Benjamin Mahler wrote:
> > src/tests/upgrade_tests.cpp
> > Lines 489-505 (patched)
> > 
> >
> > Do you want to guard the .as calls with some .is ASSERTs?

No longer needed.


- Michael


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


On March 6, 2017, 5:33 p.m., Michael Park wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/57336/
> ---
> 
> (Updated March 6, 2017, 5:33 p.m.)
> 
> 
> Review request for mesos and Benjamin Mahler.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Updated `MultiRoleSchedulerUpgrade` to test framework updates.
> 
> 
> Diffs
> -
> 
>   src/tests/upgrade_tests.cpp 6cdd6d989df14af5f17b41af0bf631471feae00e 
> 
> 
> Diff: https://reviews.apache.org/r/57336/diff/3/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Michael Park
> 
>

Re: Review Request 57336: Updated `MultiRoleSchedulerUpgrade` to test framework updates.

[Michael Park]

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

(Updated March 6, 2017, 5:33 p.m.)


Review request for mesos and Benjamin Mahler.


Changes
---

Addressed bmahler's comments.


Repository: mesos


Description
---

Updated `MultiRoleSchedulerUpgrade` to test framework updates.


Diffs (updated)
-

  src/tests/upgrade_tests.cpp 6cdd6d989df14af5f17b41af0bf631471feae00e 


Diff: https://reviews.apache.org/r/57336/diff/3/

Changes: https://reviews.apache.org/r/57336/diff/2-3/


Testing
---


Thanks,

Michael Park

Review Request 57354: Added `initializer_list` constructors for `JSON::(Object|Array)`.

[Michael Park]

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

Review request for mesos and Benjamin Mahler.


Repository: mesos


Description
---

This improves test cleanliness, as it allows inline construction of
expected values with which to perform tests.

Refer to https://reviews.apache.org/r/57336 to see how this is used.


Diffs
-

  3rdparty/stout/include/stout/json.hpp 
b0fd667cdb004995553f961de24a168b09397948 


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


Testing
---


Thanks,

Michael Park

Re: Review Request 57059: Updated default executor tests.

[Vinod Kone]

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

(Updated March 7, 2017, 1:17 a.m.)


Review request for mesos, Anand Mazumdar and Gilbert Song.


Changes
---

addressed anand's comments.


Repository: mesos


Description
---

Reorganized so that objects are defined closer to their usage.


Diffs (updated)
-

  src/tests/default_executor_tests.cpp e4d43c8ad447577a9c5c7951207596bda1070856 


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

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


Testing
---

make check


Thanks,

Vinod Kone

Re: Review Request 57058: Updated default executor tests to exclusively use v1 protos.

[Vinod Kone]

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

(Updated March 7, 2017, 1:15 a.m.)


Review request for mesos, Anand Mazumdar and Gilbert Song.


Changes
---

anand's. NNFR.


Repository: mesos


Description
---

Now all the tests in this file use v1 protos.


Diffs (updated)
-

  src/tests/default_executor_tests.cpp e4d43c8ad447577a9c5c7951207596bda1070856 


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

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


Testing
---

make check


Thanks,

Vinod Kone

Re: Review Request 55887: Check task user before allowing a task to be launched on the agent.

[Jiang Yan Xu]

> On March 2, 2017, 1:26 a.m., Jiang Yan Xu wrote:
> > src/slave/slave.cpp
> > Lines 1830-1840 (original), 1818-1835 (patched)
> > 
> >
> > This results in a behavior change: if tasks are killed during 
> > unscheduling of directories, we now send TASK_DROPPED instead of 
> > TASK_KILLED. I think this is a better behavior because this is race anyways 
> > and the TASK_DROPPED here better reveals the error condition on the host.
> > 
> > Nevertheless we could check with the @anand and @vinod who last changed 
> > this.
> 
> Anindya Sinha wrote:
> I reinstated the original behavior. We can followup with @anand and 
> @vinod later.

So I checked with @anandm on slack: 
http://mesos.slackarchive.io/dev/-/1487591716.002096/1488847311.002217/1488846253002216/

He convinced me that it makes sense to consistently send `TASK_KILLED` in this 
case:

It takes multiple async steps to launch a task: 1) unscheudling GC, (2, new) 
authorize task, 3) launch executor and so far if a task kill lands on the agent 
in the other two cases it would result in a `TASK_KILLED`. We should probalby 
send `TASK_KILLED` for 2) as well.

Agreed?


- Jiang Yan


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


On March 3, 2017, 12:08 p.m., Anindya Sinha wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/55887/
> ---
> 
> (Updated March 3, 2017, 12:08 p.m.)
> 
> 
> Review request for mesos, Adam B, Alexander Rojas, and Jiang Yan Xu.
> 
> 
> Bugs: MESOS-6953
> https://issues.apache.org/jira/browse/MESOS-6953
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added support for action `run_tasks` on the agent's flag `acl`. Based on
> the ACL configured for `run_tasks`, a task to be launched on the agent
> can be (dis)allowed to launch on the agent.
> If a task or task group cannot be launched due to failed authorization,
> a `TASK_ERROR` Status Update shall be sent with a reason code of
> `REASON_TASK_UNAUTHORIZED` or `REASON_TASK_GROUP_UNAUTHORIZED` as
> applicable.
> Note that in case of a task group, all tasks fail if any of the tasks
> within the task group encounter the authorization error.
> 
> 
> Diffs
> -
> 
>   src/slave/slave.hpp 857338cc54b341873c338967223c6b0138e9dc3e 
>   src/slave/slave.cpp 775f43b449ec53aa51cc5d3de448ddc7c2059bff 
> 
> 
> Diff: https://reviews.apache.org/r/55887/diff/7/
> 
> 
> Testing
> ---
> 
> All tests passed.
> 
> 
> Thanks,
> 
> Anindya Sinha
> 
>

Re: Review Request 57165: Minor cleanup for quota validation code.

[Michael Park]

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


Ship it!




Ship It!

- Michael Park


On March 1, 2017, 10:23 a.m., Neil Conway wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/57165/
> ---
> 
> (Updated March 1, 2017, 10:23 a.m.)
> 
> 
> Review request for mesos, Benjamin Bannier and Michael Park.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Minor cleanup for quota validation code.
> 
> 
> Diffs
> -
> 
>   src/master/quota.cpp 847ec068f4f3fe94ddabc11ff583d7e76117d375 
> 
> 
> Diff: https://reviews.apache.org/r/57165/diff/3/
> 
> 
> Testing
> ---
> 
> `make check`
> 
> 
> Thanks,
> 
> Neil Conway
> 
>

Re: Review Request 57110: Updated the master to handle frameworks that changes its roles.

[Michael Park]

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

(Updated March 6, 2017, 4:07 p.m.)


Review request for mesos and Benjamin Mahler.


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


Repository: mesos


Description
---

Updated the master to handle frameworks that changes its roles.


Diffs (updated)
-

  src/master/master.hpp 47c5e61606fa7d971b763c97f2950799ead00e5f 
  src/master/master.cpp 3fa5438c74c852649179ed5a93202d6c050bd52a 


Diff: https://reviews.apache.org/r/57110/diff/10/

Changes: https://reviews.apache.org/r/57110/diff/9-10/


Testing
---


Thanks,

Michael Park

Re: Review Request 56753: Implemented the JWT authenticator.

[Greg Mann]

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



Almost there! Thanks Jan, code is looking great.


3rdparty/libprocess/include/process/authenticator.hpp
Lines 165-166 (original), 165-168 (patched)


Should there be two newlines here?



3rdparty/libprocess/include/process/authenticator.hpp
Lines 175 (patched)


s/RS256/HS256/



3rdparty/libprocess/include/process/authenticator.hpp
Lines 176 (patched)


s/successfull/successful/



3rdparty/libprocess/include/process/authenticator.hpp
Lines 177 (patched)


Instead of "the claims of 'principal' will be set to the claims represented 
by the tokens", what do you think about:

"the claims of the returned principal will be set to the claims within the 
token's payload"



3rdparty/libprocess/include/process/authenticator.hpp
Lines 184 (patched)


Can use `override` instead of `virtual` here.



3rdparty/libprocess/src/jwt_authenticator.cpp
Lines 43 (patched)


Eliminate `http` namespace here?



3rdparty/libprocess/src/jwt_authenticator.cpp
Lines 72 (patched)


Ah, my apologies for asking you to remove the `token.size() != 2` check; 
it's still possible that the size is _less_ than 2, so we should indeed check 
that to avoid a possible error when doing `token[1]`.



3rdparty/libprocess/src/tests/http_tests.cpp
Lines 2033 (patched)


Should we do something similar here to what you're doing below, and 
stringify an invalid JWT (invalid 'exp' or something else) to generate this?

I quite liked the "forged token" test we encountered recently (where a 
correct token was altered to have a slightly different payload, i.e. a 
different 'sub'), perhaps you could use that case here, or add another case for 
it?


- Greg Mann


On March 6, 2017, 2:55 p.m., Jan Schlicht wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/56753/
> ---
> 
> (Updated March 6, 2017, 2:55 p.m.)
> 
> 
> Review request for mesos, Alexander Rojas and Greg Mann.
> 
> 
> Bugs: MESOS-7001
> https://issues.apache.org/jira/browse/MESOS-7001
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This HTTP authenticator extracts a JWT from the requests' authorization
> header using the 'Bearer' schema and validates it against a secret using
> HMAC SHA256. The 'sub' claim of the JWT is the extracted principal, all
> other claims will be additional labels of the 'Principal'.
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/Makefile.am 75386184108214e67a58c328258ec204099d638c 
>   3rdparty/libprocess/include/process/authenticator.hpp 
> e5489c6cb4adc8a822e7dd4515542618c36136f9 
>   3rdparty/libprocess/src/authenticator.cpp 
> cfedb6f7674e0f6690e77a633cdd1bd494c7d2c7 
>   3rdparty/libprocess/src/jwt_authenticator.cpp PRE-CREATION 
>   3rdparty/libprocess/src/tests/http_tests.cpp 
> fb4da9aecff0370d97a15269c5d8fffb30e0478f 
> 
> 
> Diff: https://reviews.apache.org/r/56753/diff/4/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Jan Schlicht
> 
>

Re: Review Request 56667: Added support for JSON Web Tokens.

[Greg Mann]

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




3rdparty/libprocess/include/process/jwt.hpp
Lines 13-14 (patched)


s/JWT_HPP/PROCESS_JWT_HPP/



3rdparty/libprocess/include/process/jwt.hpp
Lines 26-27 (patched)


Could you provide a comment here explaining the motivation for adding the 
new error type?



3rdparty/libprocess/include/process/jwt.hpp
Lines 38-40 (patched)


Two newlines here?



3rdparty/libprocess/include/process/jwt.hpp
Lines 42 (patched)


Should we also provide a link to RFC-7515 here?



3rdparty/libprocess/include/process/jwt.hpp
Lines 124-125 (patched)


Fits on one line



3rdparty/libprocess/include/process/jwt.hpp
Lines 134 (patched)


s/JWT_HPP/PROCESS_JWT_HPP/



3rdparty/libprocess/src/jwt.cpp
Lines 16 (patched)


I think this include should occur first?



3rdparty/libprocess/src/jwt.cpp
Lines 42-43 (patched)


To make this text more consistent, perhaps should do "Expected 3 components 
in token"?



3rdparty/libprocess/src/jwt.cpp
Lines 169 (patched)


Do you think `parse_component` would be a better name for this?



3rdparty/libprocess/src/jwt.cpp
Lines 201-203 (patched)


It looks like this will silently ignore 'typ' headers which are not strings 
- should we return an error in that case?



3rdparty/libprocess/src/jwt.cpp
Lines 221 (patched)


To be consistent with the naming above, you could use the variable name 
`alg_string` here.



3rdparty/libprocess/src/jwt.cpp
Lines 288 (patched)


Should have a `break;` after this line.



3rdparty/libprocess/src/jwt.cpp
Lines 303 (patched)


Shouldn't we use `header.typ` here?



3rdparty/libprocess/src/tests/jwt_tests.cpp
Lines 38 (patched)


It might be more obvious what's going on, and more durable, if we use the 
`generate_hmac_sha256` helper here and below to make the key:
```
const string header = "NOT A VALID HEADER";
const string payload =
  base64::encode_url_safe("{\"exp\":99,\"sub\":\"foo\"}");
const string signature =
  base64::encode_url_safe(generate_hmac_sha256(strings::join(".", header, 
payload)));

const string token = strings::join(".", header, payload, signature);
```



3rdparty/libprocess/src/tests/jwt_tests.cpp
Lines 249 (patched)


Use `generate_hmac_hs256` here?


- Greg Mann


On March 6, 2017, 2:53 p.m., Jan Schlicht wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/56667/
> ---
> 
> (Updated March 6, 2017, 2:53 p.m.)
> 
> 
> Review request for mesos, Alexander Rojas and Greg Mann.
> 
> 
> Bugs: MESOS-7001
> https://issues.apache.org/jira/browse/MESOS-7001
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> JSON Web Tokens can be used to create claim-based access tokens and is
> typically used for HTTP authentication.
> This implementation is intended for internal use, e.g. Mesos is supposed
> to only parse tokens that it also created. It doesn't fully comply with
> RFC 7519. Currently the only supported cryptographic algorithm is HMAC
> with SHA-256.
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/Makefile.am 75386184108214e67a58c328258ec204099d638c 
>   3rdparty/libprocess/include/process/jwt.hpp PRE-CREATION 
>   3rdparty/libprocess/src/jwt.cpp PRE-CREATION 
>   3rdparty/libprocess/src/tests/jwt_tests.cpp PRE-CREATION 
> 
> 
> Diff: https://reviews.apache.org/r/56667/diff/6/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Jan Schlicht
> 
>

Re: Review Request 55887: Check task user before allowing a task to be launched on the agent.

[Anindya Sinha]

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




src/slave/slave.hpp
Lines 340 (patched)


Yes.



src/slave/slave.cpp
Lines 1830-1840 (original), 1818-1835 (patched)


I added check for killed tasks before checking for unscheduling of 
directories which kind of brings in the original behavior.
We can followup if we need to change the task state in such case separately.


- Anindya Sinha


On March 3, 2017, 8:08 p.m., Anindya Sinha wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/55887/
> ---
> 
> (Updated March 3, 2017, 8:08 p.m.)
> 
> 
> Review request for mesos, Adam B, Alexander Rojas, and Jiang Yan Xu.
> 
> 
> Bugs: MESOS-6953
> https://issues.apache.org/jira/browse/MESOS-6953
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added support for action `run_tasks` on the agent's flag `acl`. Based on
> the ACL configured for `run_tasks`, a task to be launched on the agent
> can be (dis)allowed to launch on the agent.
> If a task or task group cannot be launched due to failed authorization,
> a `TASK_ERROR` Status Update shall be sent with a reason code of
> `REASON_TASK_UNAUTHORIZED` or `REASON_TASK_GROUP_UNAUTHORIZED` as
> applicable.
> Note that in case of a task group, all tasks fail if any of the tasks
> within the task group encounter the authorization error.
> 
> 
> Diffs
> -
> 
>   src/slave/slave.hpp 857338cc54b341873c338967223c6b0138e9dc3e 
>   src/slave/slave.cpp 775f43b449ec53aa51cc5d3de448ddc7c2059bff 
> 
> 
> Diff: https://reviews.apache.org/r/55887/diff/7/
> 
> 
> Testing
> ---
> 
> All tests passed.
> 
> 
> Thanks,
> 
> Anindya Sinha
> 
>

Re: Review Request 56754: Implemented a JWT secret generator.

[Greg Mann]

> On March 3, 2017, 7:48 a.m., Greg Mann wrote:
> > src/authentication/executor/jwt_secret_generator.hpp
> > Lines 40 (patched)
> > 
> >
> > Use `override` instead of `virtual` here?
> 
> Jan Schlicht wrote:
> Do we use that for destructors as well? Seems uncommon in that context 
> and haven't seen any examples in our source base.

If the destructor is overriding a virtual destructor from the base class, then 
it's appropriate to use `override`. We don't make very wide use of the 
`override` keyword in the codebase currently (see MESOS-4871); for an example 
of a place where it was recently added, see the declaration of the 
`LibeventSSLSocketImpl` class in libprocess.


- Greg


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


On March 6, 2017, 2:57 p.m., Jan Schlicht wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/56754/
> ---
> 
> (Updated March 6, 2017, 2:57 p.m.)
> 
> 
> Review request for mesos, Alexander Rojas and Greg Mann.
> 
> 
> Bugs: MESOS-7000
> https://issues.apache.org/jira/browse/MESOS-7000
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This can be used to create a 'Secret' from a 'Principal'.
> The resulting secret will provide a JWT.
> 
> 
> Diffs
> -
> 
>   src/Makefile.am bb917c5d1b36f5106a74914fa3a864038a95e8bb 
>   src/authentication/executor/jwt_secret_generator.hpp PRE-CREATION 
>   src/authentication/executor/jwt_secret_generator.cpp PRE-CREATION 
>   src/tests/secret_generator_tests.cpp PRE-CREATION 
> 
> 
> Diff: https://reviews.apache.org/r/56754/diff/5/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Jan Schlicht
> 
>

Re: Review Request 57269: Added a test to ensure allocation roles are exposed in master API.

[Benjamin Mahler]

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



Looks good, just a few minor things and we can get this committed.


src/tests/master_tests.cpp
Lines 4292-4293 (patched)


Are you planning to test the agent endpoint as well?



src/tests/master_tests.cpp
Lines 4356 (patched)


Can you do a sweep for using the -> operator in this test?



src/tests/master_tests.cpp
Lines 4376-4384 (patched)


Do you want to add some `is()` asserts here?


- Benjamin Mahler


On March 3, 2017, 8:30 a.m., Jay Guo wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/57269/
> ---
> 
> (Updated March 3, 2017, 8:30 a.m.)
> 
> 
> Review request for mesos and Benjamin Mahler.
> 
> 
> Bugs: MESOS-7158
> https://issues.apache.org/jira/browse/MESOS-7158
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This test ensures that allocation roles of tasks and executors are
> exposed via `/state` endpoint of master v0 API.
> 
> 
> Diffs
> -
> 
>   src/tests/master_tests.cpp b9dfe9eb7d759550673896980cffc3909f4c0105 
> 
> 
> Diff: https://reviews.apache.org/r/57269/diff/1/
> 
> 
> Testing
> ---
> 
> Added a test `MasterTest.StateEndpointAllocationRole`
> 
> make check
> 
> 
> Thanks,
> 
> Jay Guo
> 
>

Re: Review Request 57186: Added unit test for persistent volume using default executor.

[Gilbert Song]

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

(Updated March 6, 2017, 2:25 p.m.)


Review request for mesos, Anand Mazumdar, Avinash sridharan, Jie Yu, Joris Van 
Remoortere, and Vinod Kone.


Repository: mesos


Description
---

This unit test verifies that the task group launched in the default
executor can access the persistent volume on the executor level by
using 'volume/sandbox_path' isolator.

The test is parameterized as the following three cases:
1. posix launcher + volume/sandbox_path (symlink).
2. linux launcher + volume/sandbox_path (symlink).
3. linux launcher + filesystem/linux + volume/sandbox_path (bind mount).


Diffs (updated)
-

  src/tests/default_executor_tests.cpp eaf63946735b28b21bfb7e16aca5924a5a82 


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

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


Testing
---

make check


Thanks,

Gilbert Song

Re: Review Request 57336: Updated `MultiRoleSchedulerUpgrade` to test framework updates.

[Benjamin Mahler]

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


Fix it, then Ship it!




Looks good, wonder if we can just loop over the master and agent pids and use 
the same code to check the contents since they should be the same?


src/tests/upgrade_tests.cpp
Lines 475-543 (patched)


Hm.. have you considered having checking both the agent and master endpoint 
result with the same code?

E.g.

```
// Check that the master and agent endpoints reflect the
// update to use the `roles` field.
foreach (const UPID& upid, { master.get()->pid, agent.get()->pid }) {
  // ...
}
```



src/tests/upgrade_tests.cpp
Lines 489-505 (patched)


Have you tried using the contains member function to clean this up?

```
// We check that the following is contained within
// the result:
//   {
// "frameworks":
// [
//   {
// "roles": ["role"]
//   }
// ]
//   }
```

But since we can't do the following:

```
JSON::Object expected = {
  {"frameworks", { {{"roles", {"foo"} }} }}
};
```

It ends up being pretty tedious.



src/tests/upgrade_tests.cpp
Lines 489-505 (patched)


Do you want to guard the .as calls with some .is ASSERTs?



src/tests/upgrade_tests.cpp
Lines 508-509 (patched)


You might want a Clock::settle here to ensure it's processed should the 
handling of updateFrameworkMessage become asynchronous in the agent.



src/tests/upgrade_tests.cpp
Lines 519-541 (patched)


Ditto here.


- Benjamin Mahler


On March 6, 2017, 1:19 p.m., Michael Park wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/57336/
> ---
> 
> (Updated March 6, 2017, 1:19 p.m.)
> 
> 
> Review request for mesos and Benjamin Mahler.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Updated `MultiRoleSchedulerUpgrade` to test framework updates.
> 
> 
> Diffs
> -
> 
>   src/tests/upgrade_tests.cpp 6cdd6d989df14af5f17b41af0bf631471feae00e 
> 
> 
> Diff: https://reviews.apache.org/r/57336/diff/2/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Michael Park
> 
>

Re: Review Request 54449: Check quotas are enabled in the XFS disk isolator.

[Mesos Reviewbot]

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



Patch looks great!

Reviews applied: [54449]

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

- Mesos Reviewbot


On March 6, 2017, 11:10 a.m., James Peach wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/54449/
> ---
> 
> (Updated March 6, 2017, 11:10 a.m.)
> 
> 
> Review request for mesos, Jie Yu and Jiang Yan Xu.
> 
> 
> Bugs: MESOS-6732
> https://issues.apache.org/jira/browse/MESOS-6732
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> The XFS disk isolator checks that the filesystem is XFS, but doesn't
> check whether project quotas are actually enabled. This means that
> an invalid configuration will start but will always fail when tasks
> are launched.
> 
> Add a check to test whether project quotas are enabled on the work
> directory and fail hard if they are not.
> 
> 
> Diffs
> -
> 
>   configure.ac 1e47babefa9ebd7e6fa3c23e8cb0e88bee16c671 
>   src/slave/containerizer/mesos/isolators/xfs/disk.cpp 
> dd4df86bf90bfa9cbf4664d89274cf3c64c2e374 
>   src/slave/containerizer/mesos/isolators/xfs/utils.hpp 
> 7602fe3b6ab069db643397418732e773d0417f8a 
>   src/slave/containerizer/mesos/isolators/xfs/utils.cpp 
> b9d8e7dc999ba3064bee7105eff0f9553d825df8 
>   src/tests/containerizer/xfs_quota_tests.cpp 
> 0fbaddd68af55c51c106962377be20afa599fb97 
> 
> 
> Diff: https://reviews.apache.org/r/54449/diff/4/
> 
> 
> Testing
> ---
> 
> Make check on Fedora 25. Manual test on F25 with mesos-execute.
> 
> 
> Thanks,
> 
> James Peach
> 
>

Re: Review Request 57335: Migrated `MultiRoleSchedulerUpgrade` from `MasterTest` to `UpgradeTest`.

[Benjamin Mahler]

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


Ship it!




Ship It!

- Benjamin Mahler


On March 6, 2017, 1:19 p.m., Michael Park wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/57335/
> ---
> 
> (Updated March 6, 2017, 1:19 p.m.)
> 
> 
> Review request for mesos and Benjamin Mahler.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Migrated `MultiRoleSchedulerUpgrade` from `MasterTest` to `UpgradeTest`.
> 
> 
> Diffs
> -
> 
>   src/tests/master_tests.cpp b9dfe9eb7d759550673896980cffc3909f4c0105 
>   src/tests/upgrade_tests.cpp 6cdd6d989df14af5f17b41af0bf631471feae00e 
> 
> 
> Diff: https://reviews.apache.org/r/57335/diff/2/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Michael Park
> 
>

Re: Review Request 57164: Cleaned up header includes.

[Michael Park]

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


Ship it!




Ship It!

- Michael Park


On Feb. 28, 2017, 12:18 p.m., Neil Conway wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/57164/
> ---
> 
> (Updated Feb. 28, 2017, 12:18 p.m.)
> 
> 
> Review request for mesos and Michael Park.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Cleaned up header includes.
> 
> 
> Diffs
> -
> 
>   include/mesos/roles.hpp 1e6954447b821c2cf0dfa11373480161a0da5ae6 
>   src/common/roles.cpp 31774a9b8f99f5efeed35b1c3e3486e05ca00f6a 
>   src/tests/reconciliation_tests.cpp 3573d558ee656ee3e10a25c3b3ed1c36f9fe6a07 
>   src/tests/slave_recovery_tests.cpp b5b805868bed61bf482d71322fb1918a0d020d48 
>   src/tests/sorter_tests.cpp c93d236b13256f4022a811d019990ef81521aa77 
> 
> 
> Diff: https://reviews.apache.org/r/57164/diff/1/
> 
> 
> Testing
> ---
> 
> `make check`
> 
> 
> Thanks,
> 
> Neil Conway
> 
>

Re: Review Request 57163: Cleaned up sorter test cases.

[Michael Park]

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


Ship it!




Ship It!

- Michael Park


On Feb. 28, 2017, 12:17 p.m., Neil Conway wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/57163/
> ---
> 
> (Updated Feb. 28, 2017, 12:17 p.m.)
> 
> 
> Review request for mesos.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Cleaned up sorter test cases.
> 
> 
> Diffs
> -
> 
>   src/tests/sorter_tests.cpp c93d236b13256f4022a811d019990ef81521aa77 
> 
> 
> Diff: https://reviews.apache.org/r/57163/diff/1/
> 
> 
> Testing
> ---
> 
> `make check`
> 
> 
> Thanks,
> 
> Neil Conway
> 
>

Re: Review Request 57162: Minor cleanup for slave tests.

[Michael Park]

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


Ship it!




Ship It!

- Michael Park


On Feb. 28, 2017, 12:14 p.m., Neil Conway wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/57162/
> ---
> 
> (Updated Feb. 28, 2017, 12:14 p.m.)
> 
> 
> Review request for mesos and Michael Park.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Minor cleanup for slave tests.
> 
> 
> Diffs
> -
> 
>   src/tests/slave_tests.cpp 3731c7607d5e49f3000c4863b1999851fac45705 
> 
> 
> Diff: https://reviews.apache.org/r/57162/diff/1/
> 
> 
> Testing
> ---
> 
> `make check`
> 
> 
> Thanks,
> 
> Neil Conway
> 
>

Review Request 57156: Minor cleanup for slave tests.

[Neil Conway]

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

Review request for mesos.


Repository: mesos


Description
---

Minor cleanup for slave tests.


Testing
---


Thanks,

Neil Conway

Re: Review Request 55323: Used process::loop for framework scheduler heartbeater.

[Vinod Kone]

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


Ship it!




Ship It!

- Vinod Kone


On Jan. 8, 2017, 7:50 a.m., Benjamin Hindman wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/55323/
> ---
> 
> (Updated Jan. 8, 2017, 7:50 a.m.)
> 
> 
> Review request for mesos, Jie Yu and Vinod Kone.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Used process::loop for framework scheduler heartbeater.
> 
> 
> Diffs
> -
> 
>   src/master/master.hpp 57fc6e6f2995078df80f0aa52707727db802ede0 
> 
> 
> Diff: https://reviews.apache.org/r/55323/diff/1/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Benjamin Hindman
> 
>

Re: Review Request 51624: Implemented 'GetAgent' call in v1 agent API.

[Vinod Kone]

> On Jan. 31, 2017, 11:45 p.m., Vinod Kone wrote:
> > Ship It!
> 
> Vinod Kone wrote:
> Can you commit this @haosdent?
> 
> haosdent huang wrote:
> Sure, let me commit.

ping!


- Vinod


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


On Oct. 8, 2016, 12:37 p.m., haosdent huang wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/51624/
> ---
> 
> (Updated Oct. 8, 2016, 12:37 p.m.)
> 
> 
> Review request for mesos, Anand Mazumdar, Vinod Kone, and Zhitao Li.
> 
> 
> Bugs: MESOS-6123
> https://issues.apache.org/jira/browse/MESOS-6123
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Implemented 'GetAgent' call in v1 agent API.
> 
> 
> Diffs
> -
> 
>   include/mesos/agent/agent.proto 5b91677487f8e23301e67af14c2115c6b4a533ac 
>   include/mesos/v1/agent/agent.proto 8145669073553dc8aa56cfe2c0a0b756d70fee0e 
>   src/slave/http.cpp 67463105d7fa38b2158a64f6994e3dd353a9fcc7 
>   src/slave/slave.hpp 13c76d1eaea2b49519948c9116e5db5caf9407ea 
>   src/slave/validation.cpp a9f318214182a87783a985cd8495a0ec00c95378 
>   src/tests/api_tests.cpp 26f99f7c337fbbc5278d1b30d3d5c8f659ddf5ca 
> 
> 
> Diff: https://reviews.apache.org/r/51624/diff/5/
> 
> 
> Testing
> ---
> 
> Added a new test case `AgentAPITest.GetAgent`.
> 
> 
> Thanks,
> 
> haosdent huang
> 
>

Re: Review Request 56666: Added a HMAC SHA256 generator.

[Greg Mann]

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


Fix it, then Ship it!





3rdparty/libprocess/include/process/ssl/utilities.hpp
Lines 115 (patched)


s/hase/hash/


- Greg Mann


On March 3, 2017, 2:02 p.m., Jan Schlicht wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/5/
> ---
> 
> (Updated March 3, 2017, 2:02 p.m.)
> 
> 
> Review request for mesos, Alexander Rojas and Greg Mann.
> 
> 
> Bugs: MESOS-7001
> https://issues.apache.org/jira/browse/MESOS-7001
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> HMAC SHA256 can be used to create or verify message signatures.
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/include/process/ssl/utilities.hpp 
> c2f64a91a505675d568ddf5aa081125e4e32fe17 
>   3rdparty/libprocess/src/ssl/utilities.cpp 
> 8aec613312eee3dd11d9df8c3828a5185407e073 
> 
> 
> Diff: https://reviews.apache.org/r/5/diff/4/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Jan Schlicht
> 
>

Re: Review Request 57110: Updated the master to handle frameworks that changes its roles.

[Benjamin Mahler]

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


Fix it, then Ship it!




New function names look good.


src/master/master.hpp
Lines 2246 (patched)


Can we take a const ref here to avoid the copy?



src/master/master.hpp
Lines 2413-2414 (patched)


Ditto here.


- Benjamin Mahler


On March 6, 2017, 11:37 a.m., Michael Park wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/57110/
> ---
> 
> (Updated March 6, 2017, 11:37 a.m.)
> 
> 
> Review request for mesos and Benjamin Mahler.
> 
> 
> Bugs: MESOS-6627
> https://issues.apache.org/jira/browse/MESOS-6627
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Updated the master to handle frameworks that changes its roles.
> 
> 
> Diffs
> -
> 
>   src/master/master.hpp 81320e0da3a2cd99d8dea1fbea9276296ef9e515 
>   src/master/master.cpp a15c6d8fb1f99d117eed8e9453a643ec80319a52 
> 
> 
> Diff: https://reviews.apache.org/r/57110/diff/9/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Michael Park
> 
>

Re: Review Request 56733: Mount all supported subsystems in the containerizer tests.

[James Peach]

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

(Updated March 6, 2017, 7:34 p.m.)


Review request for mesos, Mesos Reviewbot and Jiang Yan Xu.


Changes
---

Addressed review feedback.


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


Repository: mesos


Description
---

Rather than a hard-coded list of subsystems, just mount everything we
find that is supported.


Diffs (updated)
-

  src/tests/mesos.cpp 6a96fa51dfc2a62063c3154b256bdac707b009bb 


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

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


Testing
---

sudo make check (Fedora 25)


Thanks,

James Peach

Re: Review Request 56730: Kill stray tasks when tearing down test cgroups.

[James Peach]

> On March 3, 2017, 12:40 a.m., Jiang Yan Xu wrote:
> > Did you run into issues without this patch? cgroups::destroy() *should* 
> > kill all tasks in it.

On my reading of `cgroups::destroy`, processes would only be killed when 
destroying the `freezer` cgroup, since that is the only one with the 
`freezer.state` control that would trigger the `internal::Destroyer` to run.

The linked bug [MESO-7049](https://issues.apache.org/jira/browse/MESOS-7049) 
shows test output where the cgroup teardown fails.


- James


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


On Feb. 15, 2017, 11:26 p.m., James Peach wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/56730/
> ---
> 
> (Updated Feb. 15, 2017, 11:26 p.m.)
> 
> 
> Review request for mesos, Mesos Reviewbot and Jiang Yan Xu.
> 
> 
> Bugs: MESOS-7049
> https://issues.apache.org/jira/browse/MESOS-7049
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> If a test case fails, it may leave stray tasks in the cgroup which keeps
> us from tearing it down when the test completes. Kill any stray tasks
> before destroying the cgroup.
> 
> 
> Diffs
> -
> 
>   src/tests/containerizer/cgroups_tests.cpp 
> 76fabce4530ccc0a1d685cd48d932ced5a64bc58 
>   src/tests/mesos.cpp 6a96fa51dfc2a62063c3154b256bdac707b009bb 
> 
> 
> Diff: https://reviews.apache.org/r/56730/diff/1/
> 
> 
> Testing
> ---
> 
> sudo make check (Fedora 25)
> 
> 
> Thanks,
> 
> James Peach
> 
>

Re: Review Request 57186: Added unit test for persistent volume using default executor.

[Vinod Kone]

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


Fix it, then Ship it!





src/tests/default_executor_tests.cpp
Lines 1522 (patched)


// Ignore subsequent offers.



src/tests/default_executor_tests.cpp
Lines 1585-1587 (patched)


This should be moved up to #1569 to ensure the new update is not received 
before the expectation is set up.


- Vinod Kone


On March 1, 2017, 8:15 a.m., Gilbert Song wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/57186/
> ---
> 
> (Updated March 1, 2017, 8:15 a.m.)
> 
> 
> Review request for mesos, Anand Mazumdar, Avinash sridharan, Jie Yu, Joris 
> Van Remoortere, and Vinod Kone.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This unit test verifies that the task group launched in the default
> executor can access the persistent volume on the executor level by
> using 'volume/sandbox_path' isolator.
> 
> The test is parameterized as the following three cases:
> 1. posix launcher + volume/sandbox_path (symlink).
> 2. linux launcher + volume/sandbox_path (symlink).
> 3. linux launcher + filesystem/linux + volume/sandbox_path (bind mount).
> 
> 
> Diffs
> -
> 
>   src/tests/default_executor_tests.cpp 
> eaf63946735b28b21bfb7e16aca5924a5a82 
> 
> 
> Diff: https://reviews.apache.org/r/57186/diff/1/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Gilbert Song
> 
>

Re: Review Request 57329: Renamed `activeRoles` to `roles` in the master.

[Benjamin Mahler]

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


Ship it!




Ship It!

- Benjamin Mahler


On March 6, 2017, 11:45 a.m., Michael Park wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/57329/
> ---
> 
> (Updated March 6, 2017, 11:45 a.m.)
> 
> 
> Review request for mesos and Benjamin Mahler.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> The notion of "active" currently refers to an entity's state in regards
> to the flow of resources. For example, a framework is said to be
> "active" if it is receiving resources, and an agent is said to be
> "active" if it is sending resources. `activeRoles` however does not
> follow this notion of "active", since it simply consists of roles with
> at least one __registered__ framework, rather than __active__.
> 
> Instead, it is a list of roles that the master is keeping track of.
> We therefore opt to drop the "active" portion of this list of roles.
> 
> 
> Diffs
> -
> 
>   src/master/http.cpp 37b96c07f17d3e5ba96f6eade69d86ba9e039b3d 
>   src/master/master.hpp 81320e0da3a2cd99d8dea1fbea9276296ef9e515 
>   src/master/master.cpp a15c6d8fb1f99d117eed8e9453a643ec80319a52 
>   src/master/quota_handler.cpp 3ad28e4a9363a877d0610b529a6c17fb30ece37a 
>   src/master/weights_handler.cpp d8047f2354322ec18b008afe44f507069b36a71f 
> 
> 
> Diff: https://reviews.apache.org/r/57329/diff/2/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Michael Park
> 
>

Re: Review Request 57185: Support 'v1::createCallAccept()' helper with multi operations.

[Vinod Kone]

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


Ship it!




Ship It!

- Vinod Kone


On March 1, 2017, 8:15 a.m., Gilbert Song wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/57185/
> ---
> 
> (Updated March 1, 2017, 8:15 a.m.)
> 
> 
> Review request for mesos, Anand Mazumdar, Avinash sridharan, Jie Yu, Joris 
> Van Remoortere, and Vinod Kone.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Multiple offer operations should be supported for 'Accept' call.
> 
> 
> Diffs
> -
> 
>   src/tests/containerizer/runtime_isolator_tests.cpp 
> 1c1f7befc9e7ad38c8259f87a21656b003971b46 
>   src/tests/default_executor_tests.cpp 
> eaf63946735b28b21bfb7e16aca5924a5a82 
>   src/tests/mesos.hpp b450a04dfbf3bbeeb6b605fb78097dca390cbdbe 
> 
> 
> Diff: https://reviews.apache.org/r/57185/diff/1/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Gilbert Song
> 
>

Re: Review Request 57184: Fixed bugs in tests/mesos.hpp helpers.

[Vinod Kone]

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


Ship it!




Ship It!

- Vinod Kone


On March 1, 2017, 8:15 a.m., Gilbert Song wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/57184/
> ---
> 
> (Updated March 1, 2017, 8:15 a.m.)
> 
> 
> Review request for mesos, Anand Mazumdar, Avinash sridharan, Jie Yu, Joris 
> Van Remoortere, and Vinod Kone.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Segfault may occur on two function with v1 scenario:
> 1. createPersistentVolume().
> 2. createDiskResource().
> These are due to the use of common 'Resources::parse()'. This patch
> fixes this issue by replacing with 'TResources::parse()'.
> 
> 
> Diffs
> -
> 
>   src/tests/mesos.hpp b450a04dfbf3bbeeb6b605fb78097dca390cbdbe 
> 
> 
> Diff: https://reviews.apache.org/r/57184/diff/1/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Gilbert Song
> 
>

Re: Review Request 57330: Renamed `source` to `newInfo` for `Framework::updateFrameworkInfo`.

[Benjamin Mahler]

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


Ship it!




Ship It!

- Benjamin Mahler


On March 6, 2017, 11:36 a.m., Michael Park wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/57330/
> ---
> 
> (Updated March 6, 2017, 11:36 a.m.)
> 
> 
> Review request for mesos and Benjamin Mahler.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> See summary.
> 
> 
> Diffs
> -
> 
>   src/master/master.hpp 81320e0da3a2cd99d8dea1fbea9276296ef9e515 
> 
> 
> Diff: https://reviews.apache.org/r/57330/diff/3/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Michael Park
> 
>

Re: Review Request 57331: Used `at` rather than `operator[]` in a few instances.

[Benjamin Mahler]

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


Ship it!




Ship It!

- Benjamin Mahler


On March 6, 2017, 10:57 a.m., Michael Park wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/57331/
> ---
> 
> (Updated March 6, 2017, 10:57 a.m.)
> 
> 
> Review request for mesos and Benjamin Mahler.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> See summary.
> 
> 
> Diffs
> -
> 
>   src/master/http.cpp 37b96c07f17d3e5ba96f6eade69d86ba9e039b3d 
>   src/master/quota_handler.cpp 3ad28e4a9363a877d0610b529a6c17fb30ece37a 
> 
> 
> Diff: https://reviews.apache.org/r/57331/diff/1/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Michael Park
> 
>

Re: Review Request 56813: Updated master handlers to use the 'Principal' type.

[Greg Mann]

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

(Updated March 6, 2017, 7:11 p.m.)


Review request for mesos, Adam B, Alexander Rojas, Jan Schlicht, Till 
Toenshoff, and Vinod Kone.


Changes
---

Removed a period


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


Repository: mesos


Description
---

This patch updates the HTTP endpoint handlers in the
master process to accept the `Principal` type instead
of an `Option& principal`.


Diffs (updated)
-

  src/master/http.cpp 37b96c07f17d3e5ba96f6eade69d86ba9e039b3d 
  src/master/master.hpp 81320e0da3a2cd99d8dea1fbea9276296ef9e515 
  src/master/master.cpp a15c6d8fb1f99d117eed8e9453a643ec80319a52 
  src/master/quota_handler.cpp 3ad28e4a9363a877d0610b529a6c17fb30ece37a 
  src/master/registrar.cpp d7134eea34102ab7b24d2f0131363bdd9005cfd3 
  src/master/weights_handler.cpp d8047f2354322ec18b008afe44f507069b36a71f 


Diff: https://reviews.apache.org/r/56813/diff/8/

Changes: https://reviews.apache.org/r/56813/diff/7-8/


Testing
---

Testing details can be found at the end of this review chain.


Thanks,

Greg Mann

Re: Review Request 54449: Check quotas are enabled in the XFS disk isolator.

[James Peach]

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

(Updated March 6, 2017, 7:10 p.m.)


Review request for mesos, Jie Yu and Jiang Yan Xu.


Changes
---

Rebase and address review feedback.


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


Repository: mesos


Description
---

The XFS disk isolator checks that the filesystem is XFS, but doesn't
check whether project quotas are actually enabled. This means that
an invalid configuration will start but will always fail when tasks
are launched.

Add a check to test whether project quotas are enabled on the work
directory and fail hard if they are not.


Diffs (updated)
-

  configure.ac 1e47babefa9ebd7e6fa3c23e8cb0e88bee16c671 
  src/slave/containerizer/mesos/isolators/xfs/disk.cpp 
dd4df86bf90bfa9cbf4664d89274cf3c64c2e374 
  src/slave/containerizer/mesos/isolators/xfs/utils.hpp 
7602fe3b6ab069db643397418732e773d0417f8a 
  src/slave/containerizer/mesos/isolators/xfs/utils.cpp 
b9d8e7dc999ba3064bee7105eff0f9553d825df8 
  src/tests/containerizer/xfs_quota_tests.cpp 
0fbaddd68af55c51c106962377be20afa599fb97 


Diff: https://reviews.apache.org/r/54449/diff/4/

Changes: https://reviews.apache.org/r/54449/diff/3-4/


Testing
---

Make check on Fedora 25. Manual test on F25 with mesos-execute.


Thanks,

James Peach

Re: Review Request 56754: Implemented a JWT secret generator.

[Mesos Reviewbot]

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



Patch looks great!

Reviews applied: [56623, 56617, 56618, 56901, 57054, 57153, 56619, 56812, 
56813, 56624, 57298, 56621, 56665, 5, 56667, 56753, 56757, 56754]

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

- Mesos Reviewbot


On March 6, 2017, 2:57 p.m., Jan Schlicht wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/56754/
> ---
> 
> (Updated March 6, 2017, 2:57 p.m.)
> 
> 
> Review request for mesos, Alexander Rojas and Greg Mann.
> 
> 
> Bugs: MESOS-7000
> https://issues.apache.org/jira/browse/MESOS-7000
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This can be used to create a 'Secret' from a 'Principal'.
> The resulting secret will provide a JWT.
> 
> 
> Diffs
> -
> 
>   src/Makefile.am bb917c5d1b36f5106a74914fa3a864038a95e8bb 
>   src/authentication/executor/jwt_secret_generator.hpp PRE-CREATION 
>   src/authentication/executor/jwt_secret_generator.cpp PRE-CREATION 
>   src/tests/secret_generator_tests.cpp PRE-CREATION 
> 
> 
> Diff: https://reviews.apache.org/r/56754/diff/5/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Jan Schlicht
> 
>

Re: Review Request 57166: Updated role validation for hierarchical roles.

[Neil Conway]

> On March 2, 2017, 1:21 a.m., Qian Zhang wrote:
> > src/common/roles.cpp
> > Line 71 (original), 71 (patched)
> > 
> >
> > Why can't role start with a slash? According to the design doc, it 
> > seems the role like `/eng/frontend` is valid.
> 
> Neil Conway wrote:
> Qian, this aspect of the hierarchical role design has changed since the 
> design doc was initially proposed. The design doc has been partially updated 
> -- I'll finish updating it shortly. The basic idea is that the syntax for 
> hierarchical roles will just be an extension to the current syntax -- i.e., 
> hierarchical role names will look like `eng/dev` and `eng/prod`, not 
> `/eng/dev`.
> 
> Qian Zhang wrote:
> Thanks Neil for the clarification. But with this new design, the role 
> name is more like a relative path rather than an absolute path, but I guess 
> what we need is an absolute one, so when framework tries to register a role 
> like `eng/dev`, we will always assume it is an absolute one (i.e., `eng` is 
> root role), right?

Role names do not begin with `/`, but they are effectively relative paths that 
are interpreted by starting from the root of the role tree.


- Neil


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


On March 2, 2017, 6:12 p.m., Neil Conway wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/57166/
> ---
> 
> (Updated March 2, 2017, 6:12 p.m.)
> 
> 
> Review request for mesos, Benjamin Bannier and Michael Park.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Role names can now contain forward slashes. Each component in a role
> name must now be validated separately.
> 
> 
> Diffs
> -
> 
>   src/common/roles.cpp 31774a9b8f99f5efeed35b1c3e3486e05ca00f6a 
>   src/tests/role_tests.cpp 77f3d46a544a51ba71476e2f0735bb32758dd9e1 
> 
> 
> Diff: https://reviews.apache.org/r/57166/diff/2/
> 
> 
> Testing
> ---
> 
> `make check`
> 
> 
> Thanks,
> 
> Neil Conway
> 
>

Re: Review Request 55901: Added support for command health checks to the default executor.

[Gastón Kleiman]

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

(Updated March 6, 2017, 4:23 p.m.)


Review request for mesos, Alexander Rukletsov, Anand Mazumdar, haosdent huang, 
and Vinod Kone.


Changes
---

Rebased + removed env inheritance.


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


Repository: mesos


Description
---

Added support for command health checks to the default executor.


Diffs (updated)
-

  src/checks/health_checker.hpp f1f2834b3429fb00cc49c179fa9a3de328f597b5 
  src/checks/health_checker.cpp 6c97369fd9a567ba16dd92085bf142d43f71eaf1 
  src/launcher/default_executor.cpp 0ed436faa68e984d0be4e5186138f738bc7f1b52 
  src/tests/health_check_tests.cpp 56e90747f2c943daee675738428d8ddeeafde36d 


Diff: https://reviews.apache.org/r/55901/diff/13/

Changes: https://reviews.apache.org/r/55901/diff/12-13/


Testing
---

Introduced a new test: `HealthCheckTest.DefaultExecutorCmdHealthCheck`. It 
passes on Linux, but not on macOS, because of MESOS-7050.


Thanks,

Gastón Kleiman

Re: Review Request 57336: Updated `MultiRoleSchedulerUpgrade` to test framework updates.

[Mesos Reviewbot]

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



Patch looks great!

Reviews applied: [57319, 57329, 57330, 57331, 57110, 57111, 57112, 57335, 57336]

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

- Mesos Reviewbot


On March 6, 2017, 1:19 p.m., Michael Park wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/57336/
> ---
> 
> (Updated March 6, 2017, 1:19 p.m.)
> 
> 
> Review request for mesos and Benjamin Mahler.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Updated `MultiRoleSchedulerUpgrade` to test framework updates.
> 
> 
> Diffs
> -
> 
>   src/tests/upgrade_tests.cpp 6cdd6d989df14af5f17b41af0bf631471feae00e 
> 
> 
> Diff: https://reviews.apache.org/r/57336/diff/2/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Michael Park
> 
>

Review Request 57340: Remove adjustment code within Resources::apply.

[Jay Guo]

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

Review request for mesos and Benjamin Mahler.


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


Repository: mesos


Description
---

Remove adjustment code within Resources::apply.


Diffs
-

  src/common/protobuf_utils.hpp 09e468c77f0cdd931302d1bdcc192370b6ce3340 
  src/common/protobuf_utils.cpp 34c14e8ebd7b575627704c7edebcbb0458eeb3b1 
  src/common/resources.cpp 388e3ef3eabea0dd8d1300e56d493b92b70c75e3 
  src/master/allocator/mesos/hierarchical.cpp 
dcafc79421fec179f274aff05da516e64447c924 
  src/master/master.cpp a15c6d8fb1f99d117eed8e9453a643ec80319a52 
  src/tests/resources_tests.cpp 2bdce3c496108a66308ab6c8484dd171cc6c019f 
  src/v1/resources.cpp e47c4d49e69b915d37e40a91fef69f75dff52463 


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


Testing
---

WIP


Thanks,

Jay Guo

Re: Review Request 56754: Implemented a JWT secret generator.

[Jan Schlicht]

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

(Updated March 6, 2017, 3:57 p.m.)


Review request for mesos, Alexander Rojas and Greg Mann.


Changes
---

Address issues and use `JWTError`.


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


Repository: mesos


Description (updated)
---

This can be used to create a 'Secret' from a 'Principal'.
The resulting secret will provide a JWT.


Diffs (updated)
-

  src/Makefile.am bb917c5d1b36f5106a74914fa3a864038a95e8bb 
  src/authentication/executor/jwt_secret_generator.hpp PRE-CREATION 
  src/authentication/executor/jwt_secret_generator.cpp PRE-CREATION 
  src/tests/secret_generator_tests.cpp PRE-CREATION 


Diff: https://reviews.apache.org/r/56754/diff/5/

Changes: https://reviews.apache.org/r/56754/diff/4-5/


Testing
---

make check


Thanks,

Jan Schlicht

Re: Review Request 56753: Implemented the JWT authenticator.

[Jan Schlicht]

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

(Updated March 6, 2017, 3:55 p.m.)


Review request for mesos, Alexander Rojas and Greg Mann.


Changes
---

Address issues and use `JWTError`.


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


Repository: mesos


Description (updated)
---

This HTTP authenticator extracts a JWT from the requests' authorization
header using the 'Bearer' schema and validates it against a secret using
HMAC SHA256. The 'sub' claim of the JWT is the extracted principal, all
other claims will be additional labels of the 'Principal'.


Diffs (updated)
-

  3rdparty/libprocess/Makefile.am 75386184108214e67a58c328258ec204099d638c 
  3rdparty/libprocess/include/process/authenticator.hpp 
e5489c6cb4adc8a822e7dd4515542618c36136f9 
  3rdparty/libprocess/src/authenticator.cpp 
cfedb6f7674e0f6690e77a633cdd1bd494c7d2c7 
  3rdparty/libprocess/src/jwt_authenticator.cpp PRE-CREATION 
  3rdparty/libprocess/src/tests/http_tests.cpp 
fb4da9aecff0370d97a15269c5d8fffb30e0478f 


Diff: https://reviews.apache.org/r/56753/diff/4/

Changes: https://reviews.apache.org/r/56753/diff/3-4/


Testing
---

make check


Thanks,

Jan Schlicht

Re: Review Request 56667: Added support for JSON Web Tokens.

[Jan Schlicht]

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

(Updated March 6, 2017, 3:53 p.m.)


Review request for mesos, Alexander Rojas and Greg Mann.


Changes
---

Added a `JWTError` class to distinguish between JWT error types.


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


Repository: mesos


Description
---

JSON Web Tokens can be used to create claim-based access tokens and is
typically used for HTTP authentication.
This implementation is intended for internal use, e.g. Mesos is supposed
to only parse tokens that it also created. It doesn't fully comply with
RFC 7519. Currently the only supported cryptographic algorithm is HMAC
with SHA-256.


Diffs (updated)
-

  3rdparty/libprocess/Makefile.am 75386184108214e67a58c328258ec204099d638c 
  3rdparty/libprocess/include/process/jwt.hpp PRE-CREATION 
  3rdparty/libprocess/src/jwt.cpp PRE-CREATION 
  3rdparty/libprocess/src/tests/jwt_tests.cpp PRE-CREATION 


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

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


Testing
---

make check


Thanks,

Jan Schlicht

Re: Review Request 56754: Implemented a JWT secret generator.

[Jan Schlicht]

> On March 3, 2017, 8:48 a.m., Greg Mann wrote:
> > src/authentication/executor/jwt_secret_generator.hpp
> > Lines 40 (patched)
> > 
> >
> > Use `override` instead of `virtual` here?

Do we use that for destructors as well? Seems uncommon in that context and 
haven't seen any examples in our source base.


- Jan


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


On Feb. 28, 2017, 2:39 p.m., Jan Schlicht wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/56754/
> ---
> 
> (Updated Feb. 28, 2017, 2:39 p.m.)
> 
> 
> Review request for mesos, Alexander Rojas and Greg Mann.
> 
> 
> Bugs: MESOS-7000
> https://issues.apache.org/jira/browse/MESOS-7000
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This can be used to create a 'Secret' from an 'AuthenticationContext'.
> The resulting secret will provide a JWT.
> 
> 
> Diffs
> -
> 
>   src/Makefile.am 76d58a0bbd77fe59ff8a81ba7a68c84dc7169750 
>   src/authentication/executor/jwt_secret_generator.hpp PRE-CREATION 
>   src/authentication/executor/jwt_secret_generator.cpp PRE-CREATION 
>   src/tests/secret_generator_tests.cpp PRE-CREATION 
> 
> 
> Diff: https://reviews.apache.org/r/56754/diff/4/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Jan Schlicht
> 
>

Re: Review Request 56753: Implemented the JWT authenticator.

[Jan Schlicht]

> On March 1, 2017, 8:41 a.m., Greg Mann wrote:
> > 3rdparty/libprocess/include/process/authenticator.hpp
> > Lines 186-189 (patched)
> > 
> >
> > `override` keyword implies `virtual`, so the `virtual` here can be 
> > removed

Thanks, will remove it here, as well as in `BasicAuthenticator` above.


- Jan


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


On Feb. 28, 2017, 2:36 p.m., Jan Schlicht wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/56753/
> ---
> 
> (Updated Feb. 28, 2017, 2:36 p.m.)
> 
> 
> Review request for mesos, Alexander Rojas and Greg Mann.
> 
> 
> Bugs: MESOS-7001
> https://issues.apache.org/jira/browse/MESOS-7001
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This HTTP authenticator extracts a JWT from the requests' authorization
> header using the 'Bearer' schema and validates it against a secret using
> HMAC SHA256. The 'sub' claim of the JWT is the extracted principal, all
> other claims will be additional labels of the 'AuthenticationContext'.
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/Makefile.am 75386184108214e67a58c328258ec204099d638c 
>   3rdparty/libprocess/include/process/authenticator.hpp 
> e5489c6cb4adc8a822e7dd4515542618c36136f9 
>   3rdparty/libprocess/src/authenticator.cpp 
> cfedb6f7674e0f6690e77a633cdd1bd494c7d2c7 
>   3rdparty/libprocess/src/jwt_authenticator.cpp PRE-CREATION 
>   3rdparty/libprocess/src/tests/http_tests.cpp 
> fb4da9aecff0370d97a15269c5d8fffb30e0478f 
> 
> 
> Diff: https://reviews.apache.org/r/56753/diff/3/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Jan Schlicht
> 
>

Re: Review Request 56753: Implemented the JWT authenticator.

[Jan Schlicht]

> On March 1, 2017, 8:41 a.m., Greg Mann wrote:
> > 3rdparty/libprocess/src/tests/http_tests.cpp
> > Lines 2067-2068 (patched)
> > 
> >
> > Ditto as above - can we consolidate to one line?
> 
> Greg Mann wrote:
> Unless it's possible to generate the token using the constructed `JWT`, 
> per my previous comment?

Yes, that was the intention here, looks like I've forgotten about it. Will fix 
that.


- Jan


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


On Feb. 28, 2017, 2:36 p.m., Jan Schlicht wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/56753/
> ---
> 
> (Updated Feb. 28, 2017, 2:36 p.m.)
> 
> 
> Review request for mesos, Alexander Rojas and Greg Mann.
> 
> 
> Bugs: MESOS-7001
> https://issues.apache.org/jira/browse/MESOS-7001
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This HTTP authenticator extracts a JWT from the requests' authorization
> header using the 'Bearer' schema and validates it against a secret using
> HMAC SHA256. The 'sub' claim of the JWT is the extracted principal, all
> other claims will be additional labels of the 'AuthenticationContext'.
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/Makefile.am 75386184108214e67a58c328258ec204099d638c 
>   3rdparty/libprocess/include/process/authenticator.hpp 
> e5489c6cb4adc8a822e7dd4515542618c36136f9 
>   3rdparty/libprocess/src/authenticator.cpp 
> cfedb6f7674e0f6690e77a633cdd1bd494c7d2c7 
>   3rdparty/libprocess/src/jwt_authenticator.cpp PRE-CREATION 
>   3rdparty/libprocess/src/tests/http_tests.cpp 
> fb4da9aecff0370d97a15269c5d8fffb30e0478f 
> 
> 
> Diff: https://reviews.apache.org/r/56753/diff/3/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Jan Schlicht
> 
>

Re: Review Request 57112: Updated existing test cases to allow for frameworks to change its roles.

[Mesos Reviewbot]

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



Patch looks great!

Reviews applied: [57319, 57329, 57330, 57331, 57110, 57111, 57112]

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

- Mesos Reviewbot


On March 6, 2017, 12:01 p.m., Michael Park wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/57112/
> ---
> 
> (Updated March 6, 2017, 12:01 p.m.)
> 
> 
> Review request for mesos and Benjamin Mahler.
> 
> 
> Bugs: MESOS-6627
> https://issues.apache.org/jira/browse/MESOS-6627
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> See summary.
> 
> 
> Diffs
> -
> 
>   src/tests/master_validation_tests.cpp 
> 5a84c8d03cacd150ac61213a7c2d8613fd1780e7 
> 
> 
> Diff: https://reviews.apache.org/r/57112/diff/2/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Michael Park
> 
>

Re: Review Request 57336: Updated `MultiRoleSchedulerUpgrade` to test framework updates.

[Michael Park]

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

(Updated March 6, 2017, 5:19 a.m.)


Review request for mesos and Benjamin Mahler.


Repository: mesos


Description (updated)
---

Updated `MultiRoleSchedulerUpgrade` to test framework updates.


Diffs (updated)
-

  src/tests/upgrade_tests.cpp 6cdd6d989df14af5f17b41af0bf631471feae00e 


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

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


Testing
---


Thanks,

Michael Park

Re: Review Request 57335: Migrated `MultiRoleSchedulerUpgrade` from `MasterTest` to `UpgradeTest`.

[Michael Park]

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

(Updated March 6, 2017, 5:19 a.m.)


Review request for mesos and Benjamin Mahler.


Repository: mesos


Description (updated)
---

Migrated `MultiRoleSchedulerUpgrade` from `MasterTest` to `UpgradeTest`.


Diffs (updated)
-

  src/tests/master_tests.cpp b9dfe9eb7d759550673896980cffc3909f4c0105 
  src/tests/upgrade_tests.cpp 6cdd6d989df14af5f17b41af0bf631471feae00e 


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

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


Testing
---


Thanks,

Michael Park

Review Request 57336: Updated `MultiRoleSchedulerUpgrade` to test framework updates.

[Michael Park]

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

Review request for mesos and Benjamin Mahler.


Repository: mesos


Description
---

See summary.


Diffs
-

  src/tests/upgrade_tests.cpp 6cdd6d989df14af5f17b41af0bf631471feae00e 


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


Testing
---


Thanks,

Michael Park

Review Request 57335: Migrated `MultiRoleSchedulerUpgrade` from `MasterTest` to `UpgradeTest`.

[Michael Park]

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

Review request for mesos and Benjamin Mahler.


Repository: mesos


Description
---

See summary.


Diffs
-

  src/tests/master_tests.cpp b9dfe9eb7d759550673896980cffc3909f4c0105 
  src/tests/upgrade_tests.cpp 6cdd6d989df14af5f17b41af0bf631471feae00e 


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


Testing
---


Thanks,

Michael Park

Re: Review Request 57112: Updated existing test cases to allow for frameworks to change its roles.

[Michael Park]

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

(Updated March 6, 2017, 4:01 a.m.)


Review request for mesos and Benjamin Mahler.


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


Repository: mesos


Description (updated)
---

See summary.


Diffs
-

  src/tests/master_validation_tests.cpp 
5a84c8d03cacd150ac61213a7c2d8613fd1780e7 


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


Testing
---


Thanks,

Michael Park

Re: Review Request 57112: Updated existing test cases to allow for frameworks to change its roles.

[Michael Park]

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

(Updated March 6, 2017, 3:59 a.m.)


Review request for mesos and Benjamin Mahler.


Changes
---

Addressed bmahler's comments.


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


Repository: mesos


Description (updated)
---

Updated existing test cases to allow for frameworks to change its roles.


Diffs (updated)
-

  src/tests/master_validation_tests.cpp 
5a84c8d03cacd150ac61213a7c2d8613fd1780e7 


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

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


Testing
---


Thanks,

Michael Park

Re: Review Request 57111: Updated the allocator to handle frameworks that change its roles.

[Michael Park]

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

(Updated March 6, 2017, 3:52 a.m.)


Review request for mesos and Benjamin Mahler.


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


Repository: mesos


Description (updated)
---

See summary.


Diffs (updated)
-

  src/master/allocator/mesos/hierarchical.hpp 
0bb24be2761bde6d1baad69f5654029f3ceed553 
  src/master/allocator/mesos/hierarchical.cpp 
dcafc79421fec179f274aff05da516e64447c924 


Diff: https://reviews.apache.org/r/57111/diff/5/

Changes: https://reviews.apache.org/r/57111/diff/4-5/


Testing
---


Thanks,

Michael Park

Re: Review Request 57329: Renamed `activeRoles` to `roles` in the master.

[Michael Park]

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

(Updated March 6, 2017, 3:45 a.m.)


Review request for mesos and Benjamin Mahler.


Repository: mesos


Description (updated)
---

The notion of "active" currently refers to an entity's state in regards
to the flow of resources. For example, a framework is said to be
"active" if it is receiving resources, and an agent is said to be
"active" if it is sending resources. `activeRoles` however does not
follow this notion of "active", since it simply consists of roles with
at least one __registered__ framework, rather than __active__.

Instead, it is a list of roles that the master is keeping track of.
We therefore opt to drop the "active" portion of this list of roles.


Diffs (updated)
-

  src/master/http.cpp 37b96c07f17d3e5ba96f6eade69d86ba9e039b3d 
  src/master/master.hpp 81320e0da3a2cd99d8dea1fbea9276296ef9e515 
  src/master/master.cpp a15c6d8fb1f99d117eed8e9453a643ec80319a52 
  src/master/quota_handler.cpp 3ad28e4a9363a877d0610b529a6c17fb30ece37a 
  src/master/weights_handler.cpp d8047f2354322ec18b008afe44f507069b36a71f 


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

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


Testing
---


Thanks,

Michael Park

Re: Review Request 57110: Updated the master to handle frameworks that changes its roles.

[Michael Park]

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

(Updated March 6, 2017, 3:37 a.m.)


Review request for mesos and Benjamin Mahler.


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


Repository: mesos


Description (updated)
---

Updated the master to handle frameworks that changes its roles.


Diffs (updated)
-

  src/master/master.hpp 81320e0da3a2cd99d8dea1fbea9276296ef9e515 
  src/master/master.cpp a15c6d8fb1f99d117eed8e9453a643ec80319a52 


Diff: https://reviews.apache.org/r/57110/diff/9/

Changes: https://reviews.apache.org/r/57110/diff/8-9/


Testing
---


Thanks,

Michael Park

Re: Review Request 57330: Renamed `source` to `newInfo` for `Framework::updateFrameworkInfo`.

[Michael Park]

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

(Updated March 6, 2017, 3:36 a.m.)


Review request for mesos and Benjamin Mahler.


Repository: mesos


Description
---

See summary.


Diffs (updated)
-

  src/master/master.hpp 81320e0da3a2cd99d8dea1fbea9276296ef9e515 


Diff: https://reviews.apache.org/r/57330/diff/3/

Changes: https://reviews.apache.org/r/57330/diff/2-3/


Testing
---


Thanks,

Michael Park

Re: Review Request 57110: Updated the master to handle frameworks that changes its roles.

[Michael Park]

> On March 5, 2017, 12:37 p.m., Benjamin Mahler wrote:
> > src/master/master.hpp
> > Lines 1785-1786 (original), 1789-1793 (patched)
> > 
> >
> > It looks like we should have pulled out this rename to into a separate 
> > patch to have this be a bit cleaner, oh well.

https://reviews.apache.org/r/57329/


> On March 5, 2017, 12:37 p.m., Benjamin Mahler wrote:
> > src/master/master.hpp
> > Line 2423 (original), 2467 (patched)
> > 
> >
> > Do you want to pull this 'source' to 'newInfo' rename patch out to 
> > unclutter this diff?

https://reviews.apache.org/r/57330/


- Michael


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


On March 6, 2017, 2:57 a.m., Michael Park wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/57110/
> ---
> 
> (Updated March 6, 2017, 2:57 a.m.)
> 
> 
> Review request for mesos and Benjamin Mahler.
> 
> 
> Bugs: MESOS-6627
> https://issues.apache.org/jira/browse/MESOS-6627
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> See summary.
> 
> 
> Diffs
> -
> 
>   src/master/master.hpp 81320e0da3a2cd99d8dea1fbea9276296ef9e515 
>   src/master/master.cpp a15c6d8fb1f99d117eed8e9453a643ec80319a52 
> 
> 
> Diff: https://reviews.apache.org/r/57110/diff/8/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Michael Park
> 
>

Re: Review Request 57110: Updated the master to handle frameworks that changes its roles.

[Michael Park]

> On March 5, 2017, 12:37 p.m., Benjamin Mahler wrote:
> > src/master/master.cpp
> > Lines 401-412 (patched)
> > 
> >
> > Since this is effectively `stopTrackingAllocationToRole`, should we be 
> > CHECKing the invariants? i.e. that there is no more allocation and that 
> > we're not subscribed to the role

I've added `CHECK(roles.count(role) > 0);` to `trackUnderRole`, and
added the following to `untrackUnderRole`:
```cpp
  // NOTE: Ideally we would also `CHECK` that we're not currently subscribed
  // to the role. We don't do this currently because this function is used in
  // `Master::removeFramework` where we're still subscribed to `roles`.

  auto allocatedToRole = [](const Resource& resource) {
return resource.allocation_info().role() == role;
  };

  CHECK(totalUsedResources.filter(allocatedToRole).empty());
  CHECK(totalOfferedResources.filter(allocatedToRole).empty());
```


> On March 5, 2017, 12:37 p.m., Benjamin Mahler wrote:
> > src/master/quota_handler.cpp
> > Line 135 (original), 135 (patched)
> > 
> >
> > Not yours, but do you want commit a change to use .at here?

https://reviews.apache.org/r/57331/


- Michael


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


On March 6, 2017, 2:57 a.m., Michael Park wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/57110/
> ---
> 
> (Updated March 6, 2017, 2:57 a.m.)
> 
> 
> Review request for mesos and Benjamin Mahler.
> 
> 
> Bugs: MESOS-6627
> https://issues.apache.org/jira/browse/MESOS-6627
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> See summary.
> 
> 
> Diffs
> -
> 
>   src/master/master.hpp 81320e0da3a2cd99d8dea1fbea9276296ef9e515 
>   src/master/master.cpp a15c6d8fb1f99d117eed8e9453a643ec80319a52 
> 
> 
> Diff: https://reviews.apache.org/r/57110/diff/8/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Michael Park
> 
>

Re: Review Request 57110: Updated the master to handle frameworks that changes its roles.

[Michael Park]

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

(Updated March 6, 2017, 2:57 a.m.)


Review request for mesos and Benjamin Mahler.


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


Repository: mesos


Description (updated)
---

See summary.


Diffs (updated)
-

  src/master/master.hpp 81320e0da3a2cd99d8dea1fbea9276296ef9e515 
  src/master/master.cpp a15c6d8fb1f99d117eed8e9453a643ec80319a52 


Diff: https://reviews.apache.org/r/57110/diff/8/

Changes: https://reviews.apache.org/r/57110/diff/7-8/


Testing
---


Thanks,

Michael Park

Re: Review Request 57331: Used `at` rather than `operator[]` in a few instances.

[Michael Park]

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

(Updated March 6, 2017, 2:57 a.m.)


Review request for mesos and Benjamin Mahler.


Summary (updated)
-

Used `at` rather than `operator[]` in a few instances.


Repository: mesos


Description
---

See summary.


Diffs
-

  src/master/http.cpp 37b96c07f17d3e5ba96f6eade69d86ba9e039b3d 
  src/master/quota_handler.cpp 3ad28e4a9363a877d0610b529a6c17fb30ece37a 


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


Testing
---


Thanks,

Michael Park

Review Request 57331: Used `at` in a few instances.

[Michael Park]

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

Review request for mesos and Benjamin Mahler.


Repository: mesos


Description
---

See summary.


Diffs
-

  src/master/http.cpp 37b96c07f17d3e5ba96f6eade69d86ba9e039b3d 
  src/master/quota_handler.cpp 3ad28e4a9363a877d0610b529a6c17fb30ece37a 


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


Testing
---


Thanks,

Michael Park

Re: Review Request 57330: Renamed `source` to `newInfo` for `Framework::updateFrameworkInfo`.

[Michael Park]

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

(Updated March 6, 2017, 2:56 a.m.)


Review request for mesos and Benjamin Mahler.


Repository: mesos


Description (updated)
---

See summary.


Diffs
-

  src/master/master.hpp 81320e0da3a2cd99d8dea1fbea9276296ef9e515 


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


Testing
---


Thanks,

Michael Park

Re: Review Request 57330: Renamed `source` to `newInfo` for `Framework::updateFrameworkInfo`.

[Michael Park]

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

(Updated March 6, 2017, 2:55 a.m.)


Review request for mesos and Benjamin Mahler.


Repository: mesos


Description (updated)
---

Renamed `source` to `newInfo` for `Framework::updateFrameworkInfo`.


Diffs (updated)
-

  src/master/master.hpp 81320e0da3a2cd99d8dea1fbea9276296ef9e515 


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

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


Testing
---


Thanks,

Michael Park

Review Request 57329: Renamed `activeRoles` to `roles` in the master.

[Michael Park]

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

Review request for mesos and Benjamin Mahler.


Repository: mesos


Description
---

See summary.


Diffs
-

  src/master/http.cpp 37b96c07f17d3e5ba96f6eade69d86ba9e039b3d 
  src/master/master.hpp 81320e0da3a2cd99d8dea1fbea9276296ef9e515 
  src/master/master.cpp a15c6d8fb1f99d117eed8e9453a643ec80319a52 
  src/master/quota_handler.cpp 3ad28e4a9363a877d0610b529a6c17fb30ece37a 
  src/master/weights_handler.cpp d8047f2354322ec18b008afe44f507069b36a71f 


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


Testing
---


Thanks,

Michael Park

Review Request 57330: Renamed `source` to `newInfo` for `Framework::updateFrameworkInfo`.

[Michael Park]

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

Review request for mesos and Benjamin Mahler.


Repository: mesos


Description
---

See summary.


Diffs
-

  src/master/master.hpp 81320e0da3a2cd99d8dea1fbea9276296ef9e515 


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


Testing
---


Thanks,

Michael Park