Re: Review Request 59525: Added filtering of `/slaves` endpoint and `GET_AGENTS` API call.

[Greg Mann]

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




src/master/http.cpp
Lines 402-403 (patched)


It's not clear to me why we would want to show the unapproved resources in 
a special field?



src/master/http.cpp
Line 2365 (original), 2380 (patched)


Nit: I think this indent is two spaces too large. I know it's within the 
parentheses of a function call, but since it's also within the lambda's `{}`, I 
think a two space indent is appropriate here?



src/master/http.cpp
Lines 2382 (patched)


Ditto regarding indent.



src/master/http.cpp
Lines 2385 (patched)


Nit: I think the ident here can be aligned with the `writer->field( ... ) 
{` line, since this curly brace is closing out the lambda.



src/master/http.cpp
Lines 2391-2395 (original), 2417-2421 (patched)


Ditto regarding indentation.



src/master/http.cpp
Lines 2485 (patched)


s/Master *master/Master* master/


- Greg Mann


On June 1, 2017, 2:58 p.m., Alexander Rojas wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/59525/
> ---
> 
> (Updated June 1, 2017, 2:58 p.m.)
> 
> 
> Review request for mesos, Adam B, Greg Mann, and Till Toenshoff.
> 
> 
> Bugs: MESOS-7416
> https://issues.apache.org/jira/browse/MESOS-7416
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Adds support of the `VIEW_ROLE` ACL to the results generated by
> the `/slaves` as well as the `GET_AGENTS` API v1 call. This means
> that calls to this endpoint (API call) will hide roles that the
> user making the request is not authorized to see.
> 
> 
> Diffs
> -
> 
>   src/master/http.cpp eb80830fa003ad8f58243d3dc4cec9e03285e96f 
>   src/master/master.hpp 89d0790fd5fea59e74276f462581fe0073594732 
> 
> 
> Diff: https://reviews.apache.org/r/59525/diff/2/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Alexander Rojas
> 
>

Re: Review Request 59101: Enabled authorization for v1 API call GET_MAINTENANCE_STATUS.

[Alexander Rojas]

> On June 2, 2017, 4:16 a.m., Greg Mann wrote:
> > src/master/http.cpp
> > Lines 4778 (patched)
> > 
> >
> > In the agent's HTTP handlers, when `ObjectApprover::approved` returns 
> > an error, the handler returns a `Failure`.
> > 
> > With this code as-is, if the authorizer is in a bad state and all 
> > `approved` calls return errors, the client would get a successful response 
> > with an empty body. Is that the behavior we want?
> > 
> > I would expect that in the case where all `approved` calls return 
> > errors, the handler would return a `Failure`. I'm not sure about the case 
> > where only some `approved` calls return errors.
> > 
> > Looking at the handler for '/containers' on the agent, we simply log a 
> > warning if authorization for one of the containers returns an error, so I 
> > think that endpoint may return a successful result when all `approved` 
> > calls return errors.
> > 
> > I think at the very least, we should log a warning when `approved` 
> > returns an error, and we should consider if there's a good way to return an 
> > unsuccessful response to the client when all authorizations fail.

I'm in for returning a warning, however all the filtering endpoints assume 
`false` in the case of error. Examples: 
[`approveViewExecutorInfo`](https://github.com/apache/mesos/blob/master/src/common/http.cpp#L830),
 
[`approveViewFrameworkInfo`](https://github.com/apache/mesos/blob/master/src/common/http.cpp#L812),
 
[`approveViewTaskInfo`](https://github.com/apache/mesos/blob/master/src/common/http.cpp#L850),
 
[`approveViewTask`](https://github.com/apache/mesos/blob/master/src/common/http.cpp#L869),
 
[`approveViewFlags`](https://github.com/apache/mesos/blob/master/src/common/http.cpp#L888),
 
[`approveViewRole`](https://github.com/apache/mesos/blob/master/src/common/http.cpp#L945)


- Alexander


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


On June 1, 2017, 4:57 p.m., Alexander Rojas wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/59101/
> ---
> 
> (Updated June 1, 2017, 4:57 p.m.)
> 
> 
> Review request for mesos, Adam B, Greg Mann, and Till Toenshoff.
> 
> 
> Bugs: MESOS-7415
> https://issues.apache.org/jira/browse/MESOS-7415
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Enables the use of authorization for the `GET_MAINTENANCE_STATUS`
> v1 API call, using the ACL `GetMaintenanceStatus` and the action
> of the same name as the API call.
> 
> It also updates the ApiTests to take into account the authorization
> case.
> 
> 
> Diffs
> -
> 
>   src/master/http.cpp 7060b8fa53e0682681c50e051908ffbbf50fb7da 
>   src/master/master.hpp 89d0790fd5fea59e74276f462581fe0073594732 
>   src/tests/api_tests.cpp faf3242f9c86d866c4bb5e457fcfe47c1063cc09 
> 
> 
> Diff: https://reviews.apache.org/r/59101/diff/4/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Alexander Rojas
> 
>

Re: Review Request 58951: Made reservation endpoints tests independent from allocator.

[Alexander Rukletsov]

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




src/tests/reservation_endpoints_tests.cpp
Line 374 (original), 349 (patched)


Pass `masterFlags`?



src/tests/reservation_endpoints_tests.cpp
Line 442 (original), 413 (patched)


Do you want to update the comment?



src/tests/reservation_endpoints_tests.cpp
Lines 419 (patched)


Is this change related to removal of `TestAllocator` or is it a general 
improvement? If it is the latter, then please do it in a separate patch.



src/tests/reservation_endpoints_tests.cpp
Lines 460-474 (original), 431-443 (patched)


Do we need to check both `StatusUpdateMessage` and delivery of status 
update to the scheduler?

Instead, does it maybe make sense to additionaly check the task state of 
the task status update delivered to the scheduler?



src/tests/reservation_endpoints_tests.cpp
Lines 479-482 (original), 448-450 (patched)


Could you please explain why you are doing this change?



src/tests/reservation_endpoints_tests.cpp
Lines 570-571 (original), 530-532 (patched)


Ditto. Separate patch?



src/tests/reservation_endpoints_tests.cpp
Line 609 (original), 565 (patched)


Update the comment as well?



src/tests/reservation_endpoints_tests.cpp
Lines 628-639 (original), 583-590 (patched)


Let's capture the status update message and check the state is 
`TASK_KILLED`.



src/tests/reservation_endpoints_tests.cpp
Lines 640-641 (original)


This looks like a valuable check. Do we have another test that ensures a 
similar thing?



src/tests/reservation_endpoints_tests.cpp
Lines 646-648 (original), 595-597 (patched)


Same question here: why this change in this patch?



src/tests/reservation_endpoints_tests.cpp
Lines 1403 (patched)


You don't necessarily need this here. Actually it might even seem 
misleading because people may think it is doing something.


- Alexander Rukletsov


On May 30, 2017, 10:22 a.m., Benjamin Bannier wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/58951/
> ---
> 
> (Updated May 30, 2017, 10:22 a.m.)
> 
> 
> Review request for mesos, Alexander Rukletsov and Benjamin Mahler.
> 
> 
> Bugs: MESOS-7388
> https://issues.apache.org/jira/browse/MESOS-7388
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> The tests in the case often require an agent ID. To obtain the ID they
> were using a mock allocator to grab agent ID, but not other operations
> with the allocator were performed.
> 
> Instead we now just capture the SlaveRegisteredMessage in order to
> learn an agent's ID.
> 
> 
> Diffs
> -
> 
>   src/tests/reservation_endpoints_tests.cpp 
> 8c195eb7d788fbca8722d66d81c77d399702160a 
> 
> 
> Diff: https://reviews.apache.org/r/58951/diff/2/
> 
> 
> Testing
> ---
> 
> `make check`, also in repetition.
> 
> While this patch is part of this series since later patches depend on it, it 
> could be commited independently.
> 
> 
> Thanks,
> 
> Benjamin Bannier
> 
>

Re: Review Request 58950: Made persistent volume endpoints tests independent from allocator.

[Alexander Rukletsov]

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




src/tests/persistent_volume_endpoints_tests.cpp
Lines 827 (patched)


Let's remove this.



src/tests/persistent_volume_endpoints_tests.cpp
Line 1067 (original), 1032 (patched)


s/befor/before



src/tests/persistent_volume_endpoints_tests.cpp
Lines 1034 (patched)


No need for this.



src/tests/persistent_volume_endpoints_tests.cpp
Line 1286 (original), 1249 (patched)


s/befor/before/



src/tests/persistent_volume_endpoints_tests.cpp
Lines 1251 (patched)


Let's not call it explicitly.



src/tests/persistent_volume_endpoints_tests.cpp
Lines 1848-1849 (original), 1791-1793 (patched)


I believe `slaveId1` is more common in our code base. Feel free to change 
it, preferably in a separate patch : )


- Alexander Rukletsov


On May 30, 2017, 10:22 a.m., Benjamin Bannier wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/58950/
> ---
> 
> (Updated May 30, 2017, 10:22 a.m.)
> 
> 
> Review request for mesos, Alexander Rukletsov and Benjamin Mahler.
> 
> 
> Bugs: MESOS-7388
> https://issues.apache.org/jira/browse/MESOS-7388
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> The tests in the case often require an agent ID. To obtain the ID they
> were using a mock allocator to grab agent ID, but not other operations
> with the allocator were performed.
> 
> Instead we now just capture the SlaveRegisteredMessage in order to
> learn an agent's ID.
> 
> 
> Diffs
> -
> 
>   src/tests/persistent_volume_endpoints_tests.cpp 
> 21136b29d724959527b889f52611baee0668 
> 
> 
> Diff: https://reviews.apache.org/r/58950/diff/2/
> 
> 
> Testing
> ---
> 
> `make check`, also in repetition.
> 
> While this patch is part of this series since later patches depend on it, it 
> could be commited independently.
> 
> 
> Thanks,
> 
> Benjamin Bannier
> 
>

Re: Review Request 59100: Enabled authorization for v1 API call GET_MAINTENANCE_SCHEDULE.

[Alexander Rojas]

> On June 2, 2017, 3:43 a.m., Greg Mann wrote:
> > src/master/http.cpp
> > Lines 4222-4240 (patched)
> > 
> >
> > Simply copying into a new `Schedule` message is inefficient, but 
> > probably OK for now, as it's more readable and the output of this endpoint 
> > may not become too large.
> > 
> > I'm mostly just leaving this comment for posterity, to note that we may 
> > want to use the more sophisiticated JSON writer-based filtering in the 
> > future if performance becomes an issue, as we do in the `/state` handler.

I thought the same when I built it, but then I saw other related functons, like 
`getMaintenanceStatus` do something similar.


- Alexander


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


On June 1, 2017, 4:57 p.m., Alexander Rojas wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/59100/
> ---
> 
> (Updated June 1, 2017, 4:57 p.m.)
> 
> 
> Review request for mesos, Adam B, Greg Mann, and Till Toenshoff.
> 
> 
> Bugs: MESOS-7415
> https://issues.apache.org/jira/browse/MESOS-7415
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Enables the use of authorization for the `GET_MAINTENANCE_SCHEDULE`
> v1 API call, using the ACL `GetMaintenanceSchedule` and the action
> of the same name as the API call.
> 
> It also updates the ApiTests to take into account the authorization
> case.
> 
> 
> Diffs
> -
> 
>   src/master/http.cpp 7060b8fa53e0682681c50e051908ffbbf50fb7da 
>   src/master/master.hpp 89d0790fd5fea59e74276f462581fe0073594732 
>   src/tests/api_tests.cpp faf3242f9c86d866c4bb5e457fcfe47c1063cc09 
> 
> 
> Diff: https://reviews.apache.org/r/59100/diff/4/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Alexander Rojas
> 
>

Re: Review Request 59605: Use glog for logging in executors.

[Benjamin Bannier]

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


Ship it!




LGTM! It would be great if you could mention MESOS-7586 in the commit message 
as documentation of the pattern you followed.


src/docker/executor.cpp
Line 814 (original), 814 (patched)


Note that in e.g., `src/slave/main.cpp` parsing errors from either the 
flags or env vars are reported with glog (via `EXIT(EXIT_FAILURE)`).

The pattern here does make sense to me (report setup messages from `main` 
vio stdout/stderr; from functions via glog), and it would be great if you could 
mention MESOS-7586 in the commit message as documentation of the pattern.

Here and elsewhere.


- Benjamin Bannier


On May 29, 2017, 4:59 p.m., Andrei Budnik wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/59605/
> ---
> 
> (Updated May 29, 2017, 4:59 p.m.)
> 
> 
> Review request for mesos, Alexander Rukletsov, Benjamin Bannier, Benjamin 
> Mahler, and Till Toenshoff.
> 
> 
> Bugs: MESOS-6961
> https://issues.apache.org/jira/browse/MESOS-6961
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Use glog for logging in executors.
> 
> 
> Diffs
> -
> 
>   src/docker/executor.cpp 464a74859e0f477ec2e5525a5fc726ea420a00a2 
>   src/launcher/default_executor.cpp 1a60e3bb2b0e8312b349cddedb4cd55716c1b574 
>   src/launcher/executor.cpp 9ac3c3d84c0ec47954c5c72228ad0b8795ff3eec 
> 
> 
> Diff: https://reviews.apache.org/r/59605/diff/1/
> 
> 
> Testing
> ---
> 
> 1. make check (mac os x 10.12, fedora 25)
> 2. jenkins: 
> https://jenkins.mesosphere.com/service/jenkins/job/mesos/job/Mesos_CI-build/1087/
> 
> 3. I've modified NoTaskKillingCapability from command_executor_tests.cpp to 
> launch command "sleep 0.1" (as well as "sleep 0"),
> then to wait for a task completion. The test was launched in an endless loop:
> 
> GLOG_v=1 ./bin/mesos-tests.sh  --gtest_repeat=-1 --gtest_break_on_failure 
> --gtest_filter="HTTPCommandExecutor/CommandExecutorTest.NoTaskKillingCapability/0"
>  --verbose
> 
> I've run this test multiple times for a while both on mac os and linux 
> (gru1.hw.ca1.mesosphere.com).
> Race condition problem described in a jira ticket wasn't reproduced.
> 
> 
> Thanks,
> 
> Andrei Budnik
> 
>

Re: Review Request 59605: Used glog for logging in executors.

[Andrei Budnik]

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

(Updated June 2, 2017, 11:53 a.m.)


Review request for mesos, Alexander Rukletsov, Benjamin Bannier, Benjamin 
Mahler, and Till Toenshoff.


Summary (updated)
-

Used glog for logging in executors.


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


Repository: mesos


Description (updated)
---

The rule for choosing between glog and cout is described by MESOS-7586.


Diffs (updated)
-

  src/docker/executor.cpp 464a74859e0f477ec2e5525a5fc726ea420a00a2 
  src/launcher/default_executor.cpp 1a60e3bb2b0e8312b349cddedb4cd55716c1b574 
  src/launcher/executor.cpp 9ac3c3d84c0ec47954c5c72228ad0b8795ff3eec 


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

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


Testing
---

1. make check (mac os x 10.12, fedora 25)
2. jenkins: 
https://jenkins.mesosphere.com/service/jenkins/job/mesos/job/Mesos_CI-build/1087/

3. I've modified NoTaskKillingCapability from command_executor_tests.cpp to 
launch command "sleep 0.1" (as well as "sleep 0"),
then to wait for a task completion. The test was launched in an endless loop:

GLOG_v=1 ./bin/mesos-tests.sh  --gtest_repeat=-1 --gtest_break_on_failure 
--gtest_filter="HTTPCommandExecutor/CommandExecutorTest.NoTaskKillingCapability/0"
 --verbose

I've run this test multiple times for a while both on mac os and linux 
(gru1.hw.ca1.mesosphere.com).
Race condition problem described in a jira ticket wasn't reproduced.


Thanks,

Andrei Budnik

Re: Review Request 59600: Added `--resource_providers` flag to the agent.

[Jan Schlicht]

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




docs/configuration.md
Lines 1853 (patched)


The example JSON isn't a `ResourceProviderInfo`. Would a 
`ResourceProviderInfo` even make sense here? I.e. how does this provide enough 
information for an agent to decide which standalone container (containing the 
actual resource provider) to launch? Also, `ResourceProviderInfo` contains a 
`resources` field which IMO shouldn't be set here. Instead the resources should 
be reported by the resource provider when subscribing with a master.


- Jan Schlicht


On May 26, 2017, 3:12 a.m., Jie Yu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/59600/
> ---
> 
> (Updated May 26, 2017, 3:12 a.m.)
> 
> 
> Review request for mesos, Benjamin Bannier, Chun-Hung Hsiao, Joseph Wu, and 
> Jan Schlicht.
> 
> 
> Bugs: MESOS-7571
> https://issues.apache.org/jira/browse/MESOS-7571
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added `--resource_providers` flag to the agent.
> 
> 
> Diffs
> -
> 
>   docs/configuration.md 59e1bbe7502c7f55fb3fb3167ccdccc1294e6db7 
>   include/mesos/type_utils.hpp c7872c0cd7f19d923e8be1f33a775d5eb17a0cf7 
>   src/common/parse.hpp 64eabf89f81a6708cfac89081e0d9f9586721693 
>   src/common/type_utils.cpp 5f8e72b97d6766f9729079f6c6c013bc117cedb9 
>   src/slave/flags.hpp b66995630f89dfb95a6d0cf66efc5d7590e90cbc 
>   src/slave/flags.cpp 0c8276e425a6a7d22ee68edc6cc25b331635ec44 
> 
> 
> Diff: https://reviews.apache.org/r/59600/diff/1/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Jie Yu
> 
>

Re: Review Request 59596: Renamed the ifdef guard for v1 resource provider header.

[Jan Schlicht]

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


Ship it!




Ship It!

- Jan Schlicht


On May 26, 2017, 3:01 a.m., Jie Yu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/59596/
> ---
> 
> (Updated May 26, 2017, 3:01 a.m.)
> 
> 
> Review request for mesos, Benjamin Bannier, Chun-Hung Hsiao, Joseph Wu, and 
> Jan Schlicht.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Renamed the ifdef guard for v1 resource provider header.
> 
> 
> Diffs
> -
> 
>   include/mesos/v1/resource_provider/resource_provider.hpp 
> 2b8c8afab852621fb49b132813d512d0c96bc68c 
> 
> 
> Diff: https://reviews.apache.org/r/59596/diff/1/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Jie Yu
> 
>

Re: Review Request 58955: Adds authorization for the master's v1 API call SET_LOGGING_LEVEL.

[Alexander Rojas]

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

(Updated June 2, 2017, 2:25 p.m.)


Review request for mesos, Adam B, Greg Mann, and Till Toenshoff.


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


Repository: mesos


Description (updated)
---

Enables authorization in the v1 API call `SET_LOGGING_LEVEL` following
the existing implementation for the agent call with the same name.
Likewise it reuses the authorizer action `SET_LOG_LEVEL`.


Diffs (updated)
-

  src/master/http.cpp 1dcfe6ef00b0e3984deb79a511e665f638661323 
  src/tests/api_tests.cpp 91b3473452b8e65cab9f2e873837d64a0edf4b54 


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

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


Testing
---

make check


Thanks,

Alexander Rojas

Re: Review Request 58964: Added authorization support for operator endpoints.

[Alexander Rojas]

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

(Updated June 2, 2017, 2:32 p.m.)


Review request for mesos, Adam B, Greg Mann, and Till Toenshoff.


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


Repository: mesos


Description (updated)
---

Adds the actions `UPDATE_MAINTENANCE_SCHEDULE`,
`GET_MAINTENANCE_SCHEDULE`, `START_MAINTENANCE`, `STOP_MAINTENANCE`
and `GET_MAINTENANCE_STATUS` to the authorizer API as well as the
necesary code to handle these new actions.

While the interface `mesos::Authorizer` takes an object with type
`MachineID` to perform authorization; the default implementation of
the interface `mesos::LocalAuthorizer` ignores the object choosing the
semantics of allow maintenance on all nodes or none. This was done to
extend the capacities of custom authorizers which may have special
rules for authorization.


Diffs (updated)
-

  docs/authorization.md d94f0f9d142e66118b89ecac28b9a4b21e88b6c8 
  include/mesos/authorizer/acls.proto 36b3ac231d8be9d40aad08940e870a69b0b72aa8 
  include/mesos/authorizer/authorizer.hpp 
4a7376fb6ca2be0a513ad54f56eea3cf8cdd024d 
  include/mesos/authorizer/authorizer.proto 
c9184d151befa4cea9bdebb36a315c760e6424b2 
  src/authorizer/local/authorizer.cpp 1f2a9902e88705cf412af834c127c3afe6d3bef4 
  src/tests/authorization_tests.cpp 6d85a5452d50390f96e0b11c2bac5c96722f6eac 


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

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


Testing
---

make check


Thanks,

Alexander Rojas

Re: Review Request 59099: Enabled authorization for v1 API call UPDATE_MAINTENANCE_SCHEDULE.

[Alexander Rojas]

> On June 2, 2017, 1:36 a.m., Greg Mann wrote:
> > src/tests/api_tests.cpp
> > Lines 1205-1218 (patched)
> > 
> >
> > I understand that you were just following an existing pattern here, but 
> > it does seem pretty inconsistent with the rest of our test code. I would 
> > prefer Till's suggestion as well.
> > 
> > Perhaps you could create a ticket to update all the tests here which 
> > use this `.then` continuation approach, to make them more consistent with 
> > the rest of the codebase, and add a TODO in this file with a link to the 
> > ticket? Could go in a separate review.

Will cleanup in a follow up review.


- Alexander


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


On June 1, 2017, 4:57 p.m., Alexander Rojas wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/59099/
> ---
> 
> (Updated June 1, 2017, 4:57 p.m.)
> 
> 
> Review request for mesos, Adam B, Greg Mann, and Till Toenshoff.
> 
> 
> Bugs: MESOS-7415
> https://issues.apache.org/jira/browse/MESOS-7415
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Enables the use of authorization for the `UPDATE_MAINTENANCE_SCHEDULE`
> v1 API call, using the ACL `UpdateMaintenanceSchedule` and the action
> of the same name as the API call.
> 
> It also updates the ApiTests to take into account the authorization
> case.
> 
> 
> Diffs
> -
> 
>   src/master/http.cpp 1dcfe6ef00b0e3984deb79a511e665f638661323 
>   src/master/master.hpp 89d0790fd5fea59e74276f462581fe0073594732 
>   src/tests/api_tests.cpp 91b3473452b8e65cab9f2e873837d64a0edf4b54 
> 
> 
> Diff: https://reviews.apache.org/r/59099/diff/5/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Alexander Rojas
> 
>

Re: Review Request 59605: Used glog for logging in executors.

[Mesos Reviewbot Windows]

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



Patch looks great!

Reviews applied: [59605]

Passed command: support\windows-build.bat

- Mesos Reviewbot Windows


On June 2, 2017, 1:53 p.m., Andrei Budnik wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/59605/
> ---
> 
> (Updated June 2, 2017, 1:53 p.m.)
> 
> 
> Review request for mesos, Alexander Rukletsov, Benjamin Bannier, Benjamin 
> Mahler, and Till Toenshoff.
> 
> 
> Bugs: MESOS-6961
> https://issues.apache.org/jira/browse/MESOS-6961
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> The rule for choosing between glog and cout is described by MESOS-7586.
> 
> 
> Diffs
> -
> 
>   src/docker/executor.cpp 464a74859e0f477ec2e5525a5fc726ea420a00a2 
>   src/launcher/default_executor.cpp 1a60e3bb2b0e8312b349cddedb4cd55716c1b574 
>   src/launcher/executor.cpp 9ac3c3d84c0ec47954c5c72228ad0b8795ff3eec 
> 
> 
> Diff: https://reviews.apache.org/r/59605/diff/2/
> 
> 
> Testing
> ---
> 
> 1. make check (mac os x 10.12, fedora 25)
> 2. jenkins: 
> https://jenkins.mesosphere.com/service/jenkins/job/mesos/job/Mesos_CI-build/1087/
> 
> 3. I've modified NoTaskKillingCapability from command_executor_tests.cpp to 
> launch command "sleep 0.1" (as well as "sleep 0"),
> then to wait for a task completion. The test was launched in an endless loop:
> 
> GLOG_v=1 ./bin/mesos-tests.sh  --gtest_repeat=-1 --gtest_break_on_failure 
> --gtest_filter="HTTPCommandExecutor/CommandExecutorTest.NoTaskKillingCapability/0"
>  --verbose
> 
> I've run this test multiple times for a while both on mac os and linux 
> (gru1.hw.ca1.mesosphere.com).
> Race condition problem described in a jira ticket wasn't reproduced.
> 
> 
> Thanks,
> 
> Andrei Budnik
> 
>

Re: Review Request 59099: Enabled authorization for v1 API call UPDATE_MAINTENANCE_SCHEDULE.

[Alexander Rojas]

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

(Updated June 2, 2017, 2:36 p.m.)


Review request for mesos, Adam B, Greg Mann, and Till Toenshoff.


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


Repository: mesos


Description (updated)
---

Enables the use of authorization for the \`UPDATE_MAINTENANCE_SCHEDULE\`
v1 API call, using the ACL \`UpdateMaintenanceSchedule\` and the action
of the same name as the API call.

It also updates the ApiTests to take into account the authorization
case.


Diffs (updated)
-

  src/master/http.cpp 1dcfe6ef00b0e3984deb79a511e665f638661323 
  src/master/master.hpp 89d0790fd5fea59e74276f462581fe0073594732 
  src/tests/api_tests.cpp 91b3473452b8e65cab9f2e873837d64a0edf4b54 


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

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


Testing
---

make check


Thanks,

Alexander Rojas

Re: Review Request 59100: Enabled authorization for v1 API call GET_MAINTENANCE_SCHEDULE.

[Alexander Rojas]

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

(Updated June 2, 2017, 2:39 p.m.)


Review request for mesos, Adam B, Greg Mann, and Till Toenshoff.


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


Repository: mesos


Description
---

Enables the use of authorization for the `GET_MAINTENANCE_SCHEDULE`
v1 API call, using the ACL `GetMaintenanceSchedule` and the action
of the same name as the API call.

It also updates the ApiTests to take into account the authorization
case.


Diffs (updated)
-

  src/master/http.cpp 1dcfe6ef00b0e3984deb79a511e665f638661323 
  src/master/master.hpp 89d0790fd5fea59e74276f462581fe0073594732 
  src/tests/api_tests.cpp 91b3473452b8e65cab9f2e873837d64a0edf4b54 


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

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


Testing
---

make check


Thanks,

Alexander Rojas

Re: Review Request 59598: Added registration logic to storage local resource provider.

[Jan Schlicht]

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




src/resource_provider/storage/provider.cpp
Lines 108 (patched)


The `SUBSCRIBED` call will also provide a `ResourceProviderID` assigned by 
the master. This should be handled here.



src/resource_provider/storage/provider.cpp
Lines 131 (patched)


Can you add a `TODO` here, so that it's clear that this function isn't 
intented to be empty and will be implemented later?


- Jan Schlicht


On May 26, 2017, 3:09 a.m., Jie Yu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/59598/
> ---
> 
> (Updated May 26, 2017, 3:09 a.m.)
> 
> 
> Review request for mesos, Benjamin Bannier, Chun-Hung Hsiao, Joseph Wu, and 
> Jan Schlicht.
> 
> 
> Bugs: MESOS-7570
> https://issues.apache.org/jira/browse/MESOS-7570
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added registration logic to storage local resource provider.
> 
> 
> Diffs
> -
> 
>   src/resource_provider/storage/provider.cpp PRE-CREATION 
> 
> 
> Diff: https://reviews.apache.org/r/59598/diff/1/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Jie Yu
> 
>

Re: Review Request 59600: Added `--resource_providers` flag to the agent.

[Jan Schlicht]

> On June 2, 2017, 2:15 p.m., Jan Schlicht wrote:
> > docs/configuration.md
> > Lines 1853 (patched)
> > 
> >
> > The example JSON isn't a `ResourceProviderInfo`. Would a 
> > `ResourceProviderInfo` even make sense here? I.e. how does this provide 
> > enough information for an agent to decide which standalone container 
> > (containing the actual resource provider) to launch? Also, 
> > `ResourceProviderInfo` contains a `resources` field which IMO shouldn't be 
> > set here. Instead the resources should be reported by the resource provider 
> > when subscribing with a master.

Sorry, looked into this before looking into #59599. The example is indeed a 
`ResourceProviderInfo` JSON. While I think that it makes sense to have `name` 
and `type` fields in `ResourceProviderInfo`, IMO the JSON to describe a 
resource provider as part of the `--resource_providers` flag shouldn't be a 
`ResourceProviderInfo`. The other fields don't make sense in that context (see 
above). How about we introduce a `ResourceProviderDescription` that's having 
the `name` and `type` fields and use that here?


- Jan


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


On May 26, 2017, 3:12 a.m., Jie Yu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/59600/
> ---
> 
> (Updated May 26, 2017, 3:12 a.m.)
> 
> 
> Review request for mesos, Benjamin Bannier, Chun-Hung Hsiao, Joseph Wu, and 
> Jan Schlicht.
> 
> 
> Bugs: MESOS-7571
> https://issues.apache.org/jira/browse/MESOS-7571
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added `--resource_providers` flag to the agent.
> 
> 
> Diffs
> -
> 
>   docs/configuration.md 59e1bbe7502c7f55fb3fb3167ccdccc1294e6db7 
>   include/mesos/type_utils.hpp c7872c0cd7f19d923e8be1f33a775d5eb17a0cf7 
>   src/common/parse.hpp 64eabf89f81a6708cfac89081e0d9f9586721693 
>   src/common/type_utils.cpp 5f8e72b97d6766f9729079f6c6c013bc117cedb9 
>   src/slave/flags.hpp b66995630f89dfb95a6d0cf66efc5d7590e90cbc 
>   src/slave/flags.cpp 0c8276e425a6a7d22ee68edc6cc25b331635ec44 
> 
> 
> Diff: https://reviews.apache.org/r/59600/diff/1/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Jie Yu
> 
>

Review Request 59746: Separated discarded and failed cases for container launch.

[Alexander Rukletsov]

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

Review request for mesos, Ian Downes, Jie Yu, Joseph Wu, and Jan Schlicht.


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


Repository: mesos


Description
---

Discarded future returned from the containerizer->launch() does not
necessarily mean that the container launch has failed. For example,
a framework may stop while its task are being started.


Diffs
-

  include/mesos/mesos.proto 5f80170fcd3c05add8b6e9e3107cff062818c1dc 
  include/mesos/v1/mesos.proto 4b528751006f709f841e44f48c9f5c2dc035b402 
  src/slave/slave.cpp 0c7e5f4ef905b3897d341c3147a208fc7a8a12e0 


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


Testing
---

make check on several Linux distros.


Thanks,

Alexander Rukletsov

Re: Review Request 59746: Separated discarded and failed cases for container launch.

[Armand Grillet]

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


Ship it!




Nice addition, LGTM.

- Armand Grillet


On June 2, 2017, 1:10 p.m., Alexander Rukletsov wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/59746/
> ---
> 
> (Updated June 2, 2017, 1:10 p.m.)
> 
> 
> Review request for mesos, Ian Downes, Jie Yu, Joseph Wu, and Jan Schlicht.
> 
> 
> Bugs: MESOS-7601
> https://issues.apache.org/jira/browse/MESOS-7601
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Discarded future returned from the containerizer->launch() does not
> necessarily mean that the container launch has failed. For example,
> a framework may stop while its task are being started.
> 
> 
> Diffs
> -
> 
>   include/mesos/mesos.proto 5f80170fcd3c05add8b6e9e3107cff062818c1dc 
>   include/mesos/v1/mesos.proto 4b528751006f709f841e44f48c9f5c2dc035b402 
>   src/slave/slave.cpp 0c7e5f4ef905b3897d341c3147a208fc7a8a12e0 
> 
> 
> Diff: https://reviews.apache.org/r/59746/diff/1/
> 
> 
> Testing
> ---
> 
> make check on several Linux distros.
> 
> 
> Thanks,
> 
> Alexander Rukletsov
> 
>

Re: Review Request 59605: Used glog for logging in executors.

[Andrei Budnik]

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

(Updated June 2, 2017, 1:35 p.m.)


Review request for mesos, Alexander Rukletsov, Benjamin Bannier, Benjamin 
Mahler, and Till Toenshoff.


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


Repository: mesos


Description
---

The rule for choosing between glog and cout is described by MESOS-7586.


Diffs
-

  src/docker/executor.cpp 464a74859e0f477ec2e5525a5fc726ea420a00a2 
  src/launcher/default_executor.cpp 1a60e3bb2b0e8312b349cddedb4cd55716c1b574 
  src/launcher/executor.cpp 9ac3c3d84c0ec47954c5c72228ad0b8795ff3eec 


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


Testing (updated)
---

1. make check (mac os x 10.12, fedora 25)
2. jenkins: 
https://jenkins.mesosphere.com/service/jenkins/job/mesos/job/Mesos_CI-build/1087/

3. I've temporarily modified NoTaskKillingCapability from 
command_executor_tests.cpp to launch command "sleep 0.1" (as well as "sleep 0"),
then to wait for a task completion. The test was launched in an endless loop:

GLOG_v=1 ./bin/mesos-tests.sh  --gtest_repeat=-1 --gtest_break_on_failure 
--gtest_filter="HTTPCommandExecutor/CommandExecutorTest.NoTaskKillingCapability/0"
 --verbose

I've run this test multiple times for a while both on mac os and Fedora linux. 
Race condition problem described in a jira ticket wasn't reproduced.


Thanks,

Andrei Budnik

Re: Review Request 59746: Separated discarded and failed cases for container launch.

[Jan Schlicht]

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




src/slave/slave.cpp
Line 5147 (original), 5147 (patched)


Should we also cover the `PENDING` state of a future? `!future.isReady()` 
could also mean that it's in `PENDING` state. The old code (wrongly) logged a 
"future discarded" but covered that case.


- Jan Schlicht


On June 2, 2017, 3:10 p.m., Alexander Rukletsov wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/59746/
> ---
> 
> (Updated June 2, 2017, 3:10 p.m.)
> 
> 
> Review request for mesos, Ian Downes, Jie Yu, Joseph Wu, and Jan Schlicht.
> 
> 
> Bugs: MESOS-7601
> https://issues.apache.org/jira/browse/MESOS-7601
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Discarded future returned from the containerizer->launch() does not
> necessarily mean that the container launch has failed. For example,
> a framework may stop while its task are being started.
> 
> 
> Diffs
> -
> 
>   include/mesos/mesos.proto 5f80170fcd3c05add8b6e9e3107cff062818c1dc 
>   include/mesos/v1/mesos.proto 4b528751006f709f841e44f48c9f5c2dc035b402 
>   src/slave/slave.cpp 0c7e5f4ef905b3897d341c3147a208fc7a8a12e0 
> 
> 
> Diff: https://reviews.apache.org/r/59746/diff/1/
> 
> 
> Testing
> ---
> 
> make check on several Linux distros.
> 
> 
> Thanks,
> 
> Alexander Rukletsov
> 
>

Re: Review Request 59708: Enhanced 'gpu/nvidia' isolator to handle OCI image.

[Mesos Reviewbot]

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



Bad patch!

Reviews applied: [59708, 59707, 59706, 56157, 56156, 56155, 56154, 56153, 
56152, 56151, 56150, 56149, 56148, 55333, 56147, 55332, 54639, 59474, 59473, 
59472, 52382, 54638, 52379, 55331, 59518, 59471, 58261, 55140, 55139, 52349]

Failed command: python support/apply-reviews.py -n -r 56154

Error:
2017-06-02 13:51:50 URL:https://reviews.apache.org/r/56154/diff/raw/ 
[4372/4372] -> "56154.patch" [1]
error: patch failed: 
src/slave/containerizer/mesos/provisioner/oci/prefix_puller.cpp:36
error: src/slave/containerizer/mesos/provisioner/oci/prefix_puller.cpp: patch 
does not apply

Full log: https://builds.apache.org/job/Mesos-Reviewbot/18247/console

- Mesos Reviewbot


On June 1, 2017, 8:58 a.m., Qian Zhang wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/59708/
> ---
> 
> (Updated June 1, 2017, 8:58 a.m.)
> 
> 
> Review request for mesos, Gilbert Song, Jie Yu, and Kevin Klues.
> 
> 
> Bugs: MESOS-7598
> https://issues.apache.org/jira/browse/MESOS-7598
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Enhanced 'gpu/nvidia' isolator to handle OCI image.
> 
> 
> Diffs
> -
> 
>   src/slave/containerizer/mesos/isolators/gpu/isolator.cpp 
> 25636b50314cb4ea53d1931c97c87c65ecb658a8 
>   src/slave/containerizer/mesos/isolators/gpu/volume.hpp 
> e71fe95234ff10c72cfaa4ad39591f70a531c383 
>   src/slave/containerizer/mesos/isolators/gpu/volume.cpp 
> 536a3c711faf3c165bebaaa3c92717737d67b6f3 
>   src/tests/containerizer/nvidia_gpu_isolator_tests.cpp 
> 9a78ae65c1cd414b5093b54ff51724e31e31c9d3 
> 
> 
> Diff: https://reviews.apache.org/r/59708/diff/1/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Qian Zhang
> 
>

Re: Review Request 59746: Separated discarded and failed cases for container launch.

[Mesos Reviewbot Windows]

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



Patch looks great!

Reviews applied: [59746]

Passed command: support\windows-build.bat

- Mesos Reviewbot Windows


On June 2, 2017, 1:10 p.m., Alexander Rukletsov wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/59746/
> ---
> 
> (Updated June 2, 2017, 1:10 p.m.)
> 
> 
> Review request for mesos, Ian Downes, Jie Yu, Joseph Wu, and Jan Schlicht.
> 
> 
> Bugs: MESOS-7601
> https://issues.apache.org/jira/browse/MESOS-7601
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Discarded future returned from the containerizer->launch() does not
> necessarily mean that the container launch has failed. For example,
> a framework may stop while its task are being started.
> 
> 
> Diffs
> -
> 
>   include/mesos/mesos.proto 5f80170fcd3c05add8b6e9e3107cff062818c1dc 
>   include/mesos/v1/mesos.proto 4b528751006f709f841e44f48c9f5c2dc035b402 
>   src/slave/slave.cpp 0c7e5f4ef905b3897d341c3147a208fc7a8a12e0 
> 
> 
> Diff: https://reviews.apache.org/r/59746/diff/1/
> 
> 
> Testing
> ---
> 
> make check on several Linux distros.
> 
> 
> Thanks,
> 
> Alexander Rukletsov
> 
>

Re: Review Request 59605: Used glog for logging in executors.

[Alexander Rukletsov]

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




src/launcher/executor.cpp
Lines 1245-1246 (original), 1247-1249 (patched)


`EXIT(EXIT_FAILURE)` according to 
https://issues.apache.org/jira/browse/MESOS-7586 ?


- Alexander Rukletsov


On June 2, 2017, 1:35 p.m., Andrei Budnik wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/59605/
> ---
> 
> (Updated June 2, 2017, 1:35 p.m.)
> 
> 
> Review request for mesos, Alexander Rukletsov, Benjamin Bannier, Benjamin 
> Mahler, and Till Toenshoff.
> 
> 
> Bugs: MESOS-6961
> https://issues.apache.org/jira/browse/MESOS-6961
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> The rule for choosing between glog and cout is described by MESOS-7586.
> 
> 
> Diffs
> -
> 
>   src/docker/executor.cpp 464a74859e0f477ec2e5525a5fc726ea420a00a2 
>   src/launcher/default_executor.cpp 1a60e3bb2b0e8312b349cddedb4cd55716c1b574 
>   src/launcher/executor.cpp 9ac3c3d84c0ec47954c5c72228ad0b8795ff3eec 
> 
> 
> Diff: https://reviews.apache.org/r/59605/diff/2/
> 
> 
> Testing
> ---
> 
> 1. make check (mac os x 10.12, fedora 25)
> 2. jenkins: 
> https://jenkins.mesosphere.com/service/jenkins/job/mesos/job/Mesos_CI-build/1087/
> 
> 3. I've temporarily modified NoTaskKillingCapability from 
> command_executor_tests.cpp to launch command "sleep 0.1" (as well as "sleep 
> 0"),
> then to wait for a task completion. The test was launched in an endless loop:
> 
> GLOG_v=1 ./bin/mesos-tests.sh  --gtest_repeat=-1 --gtest_break_on_failure 
> --gtest_filter="HTTPCommandExecutor/CommandExecutorTest.NoTaskKillingCapability/0"
>  --verbose
> 
> I've run this test multiple times for a while both on mac os and Fedora 
> linux. Race condition problem described in a jira ticket wasn't reproduced.
> 
> 
> Thanks,
> 
> Andrei Budnik
> 
>

Re: Review Request 59605: Used glog for logging in executors.

[Alexander Rukletsov]

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




src/docker/executor.cpp
Lines 747-750 (original), 747-750 (patched)


We should probably move this higher up before initializing logging, as we 
do in other executors.



src/docker/executor.cpp
Lines 752-770 (original), 752-770 (patched)


This looks like validation of flags, which is probably should be done 
before we initialize logging for consistency.



src/docker/executor.cpp
Lines 772-800 (original), 772-800 (patched)


I'd say this can use `EXIT(EXIT_FAILURE) << `



src/docker/executor.cpp
Lines 814-816 (original), 814-816 (patched)


Looks like we use `EXIT(EXIT_FAILURE)` in similar cases in other executors.



src/launcher/default_executor.cpp
Line 1365 (original), 1367-1369 (patched)


Not yours, but could you also print warnings as we do it for example for 
command executor? In a separate patch, please.
```
  // Log any flag warnings (after logging is initialized).
  foreach (const flags::Warning& warning, load->warnings) {
LOG(WARNING) << warning.message;
  }
```


- Alexander Rukletsov


On June 2, 2017, 1:35 p.m., Andrei Budnik wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/59605/
> ---
> 
> (Updated June 2, 2017, 1:35 p.m.)
> 
> 
> Review request for mesos, Alexander Rukletsov, Benjamin Bannier, Benjamin 
> Mahler, and Till Toenshoff.
> 
> 
> Bugs: MESOS-6961
> https://issues.apache.org/jira/browse/MESOS-6961
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> The rule for choosing between glog and cout is described by MESOS-7586.
> 
> 
> Diffs
> -
> 
>   src/docker/executor.cpp 464a74859e0f477ec2e5525a5fc726ea420a00a2 
>   src/launcher/default_executor.cpp 1a60e3bb2b0e8312b349cddedb4cd55716c1b574 
>   src/launcher/executor.cpp 9ac3c3d84c0ec47954c5c72228ad0b8795ff3eec 
> 
> 
> Diff: https://reviews.apache.org/r/59605/diff/2/
> 
> 
> Testing
> ---
> 
> 1. make check (mac os x 10.12, fedora 25)
> 2. jenkins: 
> https://jenkins.mesosphere.com/service/jenkins/job/mesos/job/Mesos_CI-build/1087/
> 
> 3. I've temporarily modified NoTaskKillingCapability from 
> command_executor_tests.cpp to launch command "sleep 0.1" (as well as "sleep 
> 0"),
> then to wait for a task completion. The test was launched in an endless loop:
> 
> GLOG_v=1 ./bin/mesos-tests.sh  --gtest_repeat=-1 --gtest_break_on_failure 
> --gtest_filter="HTTPCommandExecutor/CommandExecutorTest.NoTaskKillingCapability/0"
>  --verbose
> 
> I've run this test multiple times for a while both on mac os and Fedora 
> linux. Race condition problem described in a jira ticket wasn't reproduced.
> 
> 
> Thanks,
> 
> Andrei Budnik
> 
>

Re: Review Request 59746: Separated discarded and failed cases for container launch.

[Alexander Rukletsov]

> On June 2, 2017, 1:39 p.m., Jan Schlicht wrote:
> > src/slave/slave.cpp
> > Line 5147 (original), 5147 (patched)
> > 
> >
> > Should we also cover the `PENDING` state of a future? 
> > `!future.isReady()` could also mean that it's in `PENDING` state. The old 
> > code (wrongly) logged a "future discarded" but covered that case.

I don't think so. IIUC, this continuation is only called when a future, on 
which this continuation is chained, has entered a terminal state. However, it 
might make sense to `CHECK` that the future is not pending, since we consider 
this an internal invariant.


- Alexander


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


On June 2, 2017, 1:10 p.m., Alexander Rukletsov wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/59746/
> ---
> 
> (Updated June 2, 2017, 1:10 p.m.)
> 
> 
> Review request for mesos, Ian Downes, Jie Yu, Joseph Wu, and Jan Schlicht.
> 
> 
> Bugs: MESOS-7601
> https://issues.apache.org/jira/browse/MESOS-7601
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Discarded future returned from the containerizer->launch() does not
> necessarily mean that the container launch has failed. For example,
> a framework may stop while its task are being started.
> 
> 
> Diffs
> -
> 
>   include/mesos/mesos.proto 5f80170fcd3c05add8b6e9e3107cff062818c1dc 
>   include/mesos/v1/mesos.proto 4b528751006f709f841e44f48c9f5c2dc035b402 
>   src/slave/slave.cpp 0c7e5f4ef905b3897d341c3147a208fc7a8a12e0 
> 
> 
> Diff: https://reviews.apache.org/r/59746/diff/1/
> 
> 
> Testing
> ---
> 
> make check on several Linux distros.
> 
> 
> Thanks,
> 
> Alexander Rukletsov
> 
>

Re: Review Request 59641: Follow symlinks when resolving paths specified in the various master/agent endpoints.

[Aaron Wood via Review Board]

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

(Updated June 2, 2017, 2:28 p.m.)


Review request for mesos, Andrew Schwartzmeyer, James Peach, Vinod Kone, and 
Zhitao Li.


Changes
---

Added check to make sure the attached latest symlink is cleaned up.


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


Repository: mesos


Description
---

The main benefit of following symlinks in endpoints such as `/files` is that 
frameworks will be able to construct a path to the sandbox much easier. This 
will assist framework developers in making features that need to provide a path 
when hitting various operator API endpoints. Currently, making use of a path 
ending in `runs/latest` throws a 404.

One such application could be a scheduler providing the ability for users to 
work with their task's sandbox directly without going to the Mesos UI, API 
endpoints, or the actual system themselves.


Diffs (updated)
-

  src/slave/slave.cpp 14de72fa4 
  src/tests/gc_tests.cpp b8fa6a4b9 


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

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


Testing
---

`mkdir build && cd build && cmake .. && make -j2 && make check -j2`

Checked the original behavior: 
```
curl -i 
localhost:5051/files/browse?path=/tmp/slave/slaves/d3566e51-4d13-4ed1-b66b-d538d3d7ef28-S0/frameworks/d3566e51-4d13-4ed1-b66b-d538d3d7ef28-0001/executors/testapp-924b5c1524d84676a6a71665e2054d31/runs/cbec99c9-da2d-4aaa-bab7-4cee9a4df031
HTTP/1.1 200 OK
Date: Tue, 30 May 2017 17:43:08 GMT
Content-Length: 644
Content-Type: application/json

[{"gid":"root","mode":"-rw-r--r--","mtime":1496161813.0,"nlink":1,"path":"\/tmp\/slave\/slaves\/d3566e51-4d13-4ed1-b66b-d538d3d7ef28-S0\/frameworks\/d3566e51-4d13-4ed1-b66b-d538d3d7ef28-0001\/executors\/testapp-924b5c1524d84676a6a71665e2054d31\/runs\/cbec99c9-da2d-4aaa-bab7-4cee9a4df031\/stderr","size":255,"uid":"root"},{"gid":"root","mode":"-rw-r--r--","mtime":1496161812.0,"nlink":1,"path":"\/tmp\/slave\/slaves\/d3566e51-4d13-4ed1-b66b-d538d3d7ef28-S0\/frameworks\/d3566e51-4d13-4ed1-b66b-d538d3d7ef28-0001\/executors\/testapp-924b5c1524d84676a6a71665e2054d31\/runs\/cbec99c9-da2d-4aaa-bab7-4cee9a4df031\/stdout","size":1142,"uid":"root"}]
```

Checked the new behavior (this would return a 404 before this patch):
```
curl -i 
localhost:5051/files/browse?path=/tmp/slave/slaves/d3566e51-4d13-4ed1-b66b-d538d3d7ef28-S0/frameworks/d3566e51-4d13-4eors/testapp-924b5c1524d84676a6a71665e2054d31/runs/latest
HTTP/1.1 200 OK
Date: Tue, 30 May 2017 17:43:13 GMT
Content-Length: 584
Content-Type: application/json

[{"gid":"root","mode":"-rw-r--r--","mtime":1496161813.0,"nlink":1,"path":"\/tmp\/slave\/slaves\/d3566e51-4d13-4ed1-b66b-d538d3d7ef28-S0\/frameworks\/d3566e51-4d13-4ed1-b66b-d538d3d7ef28-0001\/executors\/testapp-924b5c1524d84676a6a71665e2054d31\/runs\/latest\/stderr","size":255,"uid":"root"},{"gid":"root","mode":"-rw-r--r--","mtime":1496161812.0,"nlink":1,"path":"\/tmp\/slave\/slaves\/d3566e51-4d13-4ed1-b66b-d538d3d7ef28-S0\/frameworks\/d3566e51-4d13-4ed1-b66b-d538d3d7ef28-0001\/executors\/testapp-924b5c1524d84676a6a71665e2054d31\/runs\/latest\/stdout","size":1142,"uid":"root"}]
```


Thanks,

Aaron Wood

Re: Review Request 59746: Separated discarded and failed cases for container launch.

[Jan Schlicht]

> On June 2, 2017, 3:39 p.m., Jan Schlicht wrote:
> > src/slave/slave.cpp
> > Line 5147 (original), 5147 (patched)
> > 
> >
> > Should we also cover the `PENDING` state of a future? 
> > `!future.isReady()` could also mean that it's in `PENDING` state. The old 
> > code (wrongly) logged a "future discarded" but covered that case.
> 
> Alexander Rukletsov wrote:
> I don't think so. IIUC, this continuation is only called when a future, 
> on which this continuation is chained, has entered a terminal state. However, 
> it might make sense to `CHECK` that the future is not pending, since we 
> consider this an internal invariant.

Agreed, a `CHECK` makes sense here as we wouldn't expect a `PENDING` state at 
this point.


- Jan


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


On June 2, 2017, 3:10 p.m., Alexander Rukletsov wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/59746/
> ---
> 
> (Updated June 2, 2017, 3:10 p.m.)
> 
> 
> Review request for mesos, Ian Downes, Jie Yu, Joseph Wu, and Jan Schlicht.
> 
> 
> Bugs: MESOS-7601
> https://issues.apache.org/jira/browse/MESOS-7601
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Discarded future returned from the containerizer->launch() does not
> necessarily mean that the container launch has failed. For example,
> a framework may stop while its task are being started.
> 
> 
> Diffs
> -
> 
>   include/mesos/mesos.proto 5f80170fcd3c05add8b6e9e3107cff062818c1dc 
>   include/mesos/v1/mesos.proto 4b528751006f709f841e44f48c9f5c2dc035b402 
>   src/slave/slave.cpp 0c7e5f4ef905b3897d341c3147a208fc7a8a12e0 
> 
> 
> Diff: https://reviews.apache.org/r/59746/diff/1/
> 
> 
> Testing
> ---
> 
> make check on several Linux distros.
> 
> 
> Thanks,
> 
> Alexander Rukletsov
> 
>

Review Request 59750: Added mailing list guidance for working groups.

[Jie Yu]

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

Review request for mesos, Benjamin Mahler and Vinod Kone.


Repository: mesos


Description
---

Added mailing list guidance for working groups.


Diffs
-

  docs/working-groups.md b6a0b358a340658849e25646f5174e4c299734ff 


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


Testing
---

n/a


Thanks,

Jie Yu

Review Request 59749: Removed out-dated working groups.

[Jie Yu]

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

Review request for mesos, Benjamin Mahler and Vinod Kone.


Repository: mesos


Description
---

Removed out-dated working groups.


Diffs
-

  docs/working-groups.md b6a0b358a340658849e25646f5174e4c299734ff 


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


Testing
---

n/a


Thanks,

Jie Yu

Review Request 59751: Added a table for the active working groups.

[Jie Yu]

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

Review request for mesos, Benjamin Mahler and Vinod Kone.


Repository: mesos


Description
---

Added a table for the active working groups.


Diffs
-

  docs/working-groups.md b6a0b358a340658849e25646f5174e4c299734ff 


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


Testing
---

n/a


Thanks,

Jie Yu

Re: Review Request 59605: Used glog for logging in executors.

[Andrei Budnik]

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

(Updated June 2, 2017, 4:18 p.m.)


Review request for mesos, Alexander Rukletsov, Benjamin Bannier, Benjamin 
Mahler, and Till Toenshoff.


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


Repository: mesos


Description
---

The rule for choosing between glog and cout is described by MESOS-7586.


Diffs (updated)
-

  src/docker/executor.cpp 464a74859e0f477ec2e5525a5fc726ea420a00a2 
  src/launcher/default_executor.cpp 1a60e3bb2b0e8312b349cddedb4cd55716c1b574 
  src/launcher/executor.cpp 9ac3c3d84c0ec47954c5c72228ad0b8795ff3eec 


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

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


Testing
---

1. make check (mac os x 10.12, fedora 25)
2. jenkins: 
https://jenkins.mesosphere.com/service/jenkins/job/mesos/job/Mesos_CI-build/1087/

3. I've temporarily modified NoTaskKillingCapability from 
command_executor_tests.cpp to launch command "sleep 0.1" (as well as "sleep 0"),
then to wait for a task completion. The test was launched in an endless loop:

GLOG_v=1 ./bin/mesos-tests.sh  --gtest_repeat=-1 --gtest_break_on_failure 
--gtest_filter="HTTPCommandExecutor/CommandExecutorTest.NoTaskKillingCapability/0"
 --verbose

I've run this test multiple times for a while both on mac os and Fedora linux. 
Race condition problem described in a jira ticket wasn't reproduced.


Thanks,

Andrei Budnik

Re: Review Request 57935: Recalculate shares only when total scalar quantities have changed.

[Neil Conway]

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




src/master/allocator/sorter/drf/sorter.hpp
Lines 346 (patched)


I think `totalChanged` would be a more idiomatic variable name.



src/master/allocator/sorter/drf/sorter.hpp
Lines 347 (patched)


Why is `flatten` necessary here? We are already comparing two scalar 
quantities, so is the flatten required?



src/master/allocator/sorter/drf/sorter.hpp
Lines 358 (patched)


I'd move the definition/calculation of `isTotalChanged` down here, to where 
it is used.



src/master/allocator/sorter/drf/sorter.cpp
Line 309 (original), 304 (patched)






src/master/allocator/sorter/drf/sorter.cpp
Lines 307 (patched)






src/master/allocator/sorter/drf/sorter.cpp
Lines 307 (patched)






src/master/allocator/sorter/drf/sorter.cpp
Lines 307 (patched)


The return value of `update` depends only on the parameters `oldAllocation` 
and `newAllocation`, right? In which case, the way this is written seems 
confusing to me (e.g., it implies it would be possible for `dirty` to remain 
false when we start at a leaf node, but then for `dirty` to be true for an 
ancestor node).

An improvement would be to `CHECK` that `update()` returns the same value 
for all the nodes in the path from leaf -> root.

We could alternatively check whether the total allocation has changed 
outside `update` and only update `dirty` once. We could conceivably compute 
`oldAllocationQuantity` and `newAllocationQuantity` in `allocated` and pass 
them into `update` (for efficiency), but that is a bit ugly.

Let me know what you think.


- Neil Conway


On May 11, 2017, 9:02 p.m., Anindya Sinha wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/57935/
> ---
> 
> (Updated May 11, 2017, 9:02 p.m.)
> 
> 
> Review request for mesos and Neil Conway.
> 
> 
> Bugs: MESOS-7138
> https://issues.apache.org/jira/browse/MESOS-7138
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> In `DRFSorter::update`, we should set the `dirty` flag only when the
> total scalar quantities have changed in any of the cleints in the
> hierarchy, and not always.
> 
> 
> Diffs
> -
> 
>   src/master/allocator/sorter/drf/sorter.hpp 
> fee58d6d1f08163e2a06a4a20c891fe535c3dcff 
>   src/master/allocator/sorter/drf/sorter.cpp 
> 26b77f578f3235a8792c72d4575d607cdb2c7de7 
> 
> 
> Diff: https://reviews.apache.org/r/57935/diff/3/
> 
> 
> Testing
> ---
> 
> All tests passed.
> 
> 
> Thanks,
> 
> Anindya Sinha
> 
>

Re: Review Request 59605: Used glog for logging in executors.

[Andrei Budnik]

> On June 2, 2017, 2:16 p.m., Alexander Rukletsov wrote:
> > src/docker/executor.cpp
> > Lines 747-750 (original), 747-750 (patched)
> > 
> >
> > We should probably move this higher up before initializing logging, as 
> > we do in other executors.

Fixed.


> On June 2, 2017, 2:16 p.m., Alexander Rukletsov wrote:
> > src/docker/executor.cpp
> > Lines 752-770 (original), 752-770 (patched)
> > 
> >
> > This looks like validation of flags, which is probably should be done 
> > before we initialize logging for consistency.

Fixed: used EXIT(EXIT_FAILURE) according to 
https://issues.apache.org/jira/browse/MESOS-7586


> On June 2, 2017, 2:16 p.m., Alexander Rukletsov wrote:
> > src/docker/executor.cpp
> > Lines 772-800 (original), 772-800 (patched)
> > 
> >
> > I'd say this can use `EXIT(EXIT_FAILURE) << `

Fixed.


> On June 2, 2017, 2:16 p.m., Alexander Rukletsov wrote:
> > src/docker/executor.cpp
> > Lines 814-816 (original), 814-816 (patched)
> > 
> >
> > Looks like we use `EXIT(EXIT_FAILURE)` in similar cases in other 
> > executors.

Fixed.


- Andrei


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


On June 2, 2017, 4:18 p.m., Andrei Budnik wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/59605/
> ---
> 
> (Updated June 2, 2017, 4:18 p.m.)
> 
> 
> Review request for mesos, Alexander Rukletsov, Benjamin Bannier, Benjamin 
> Mahler, and Till Toenshoff.
> 
> 
> Bugs: MESOS-6961
> https://issues.apache.org/jira/browse/MESOS-6961
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> The rule for choosing between glog and cout is described by MESOS-7586.
> 
> 
> Diffs
> -
> 
>   src/docker/executor.cpp 464a74859e0f477ec2e5525a5fc726ea420a00a2 
>   src/launcher/default_executor.cpp 1a60e3bb2b0e8312b349cddedb4cd55716c1b574 
>   src/launcher/executor.cpp 9ac3c3d84c0ec47954c5c72228ad0b8795ff3eec 
> 
> 
> Diff: https://reviews.apache.org/r/59605/diff/3/
> 
> 
> Testing
> ---
> 
> 1. make check (mac os x 10.12, fedora 25)
> 2. jenkins: 
> https://jenkins.mesosphere.com/service/jenkins/job/mesos/job/Mesos_CI-build/1087/
> 
> 3. I've temporarily modified NoTaskKillingCapability from 
> command_executor_tests.cpp to launch command "sleep 0.1" (as well as "sleep 
> 0"),
> then to wait for a task completion. The test was launched in an endless loop:
> 
> GLOG_v=1 ./bin/mesos-tests.sh  --gtest_repeat=-1 --gtest_break_on_failure 
> --gtest_filter="HTTPCommandExecutor/CommandExecutorTest.NoTaskKillingCapability/0"
>  --verbose
> 
> I've run this test multiple times for a while both on mac os and Fedora 
> linux. Race condition problem described in a jira ticket wasn't reproduced.
> 
> 
> Thanks,
> 
> Andrei Budnik
> 
>

Re: Review Request 59746: Separated discarded and failed cases for container launch.

[Jie Yu]

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




src/slave/slave.cpp
Line 5147 (original), 5147 (patched)


Can you explain to me in what scenario, the `future` will be in DISCARDED 
state? who discard the promise associated with this future?


- Jie Yu


On June 2, 2017, 1:10 p.m., Alexander Rukletsov wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/59746/
> ---
> 
> (Updated June 2, 2017, 1:10 p.m.)
> 
> 
> Review request for mesos, Ian Downes, Jie Yu, Joseph Wu, and Jan Schlicht.
> 
> 
> Bugs: MESOS-7601
> https://issues.apache.org/jira/browse/MESOS-7601
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Discarded future returned from the containerizer->launch() does not
> necessarily mean that the container launch has failed. For example,
> a framework may stop while its task are being started.
> 
> 
> Diffs
> -
> 
>   include/mesos/mesos.proto 5f80170fcd3c05add8b6e9e3107cff062818c1dc 
>   include/mesos/v1/mesos.proto 4b528751006f709f841e44f48c9f5c2dc035b402 
>   src/slave/slave.cpp 0c7e5f4ef905b3897d341c3147a208fc7a8a12e0 
> 
> 
> Diff: https://reviews.apache.org/r/59746/diff/1/
> 
> 
> Testing
> ---
> 
> make check on several Linux distros.
> 
> 
> Thanks,
> 
> Alexander Rukletsov
> 
>

Re: Review Request 59749: Removed out-dated working groups.

[Mesos Reviewbot Windows]

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



Patch looks great!

Reviews applied: [59749]

Passed command: support\windows-build.bat

- Mesos Reviewbot Windows


On June 2, 2017, 4:13 p.m., Jie Yu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/59749/
> ---
> 
> (Updated June 2, 2017, 4:13 p.m.)
> 
> 
> Review request for mesos, Benjamin Mahler and Vinod Kone.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Removed out-dated working groups.
> 
> 
> Diffs
> -
> 
>   docs/working-groups.md b6a0b358a340658849e25646f5174e4c299734ff 
> 
> 
> Diff: https://reviews.apache.org/r/59749/diff/1/
> 
> 
> Testing
> ---
> 
> n/a
> 
> 
> Thanks,
> 
> Jie Yu
> 
>

Re: Review Request 57935: Recalculate shares only when total scalar quantities have changed.

[Anindya Sinha]

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

(Updated June 2, 2017, 4:59 p.m.)


Review request for mesos and Neil Conway.


Changes
---

Addressed review comments.


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


Repository: mesos


Description (updated)
---

In `DRFSorter::update`, we should set the `dirty` flag only when the
total scalar quantities have changed in any of the clients in the
hierarchy, and not always.


Diffs (updated)
-

  src/master/allocator/sorter/drf/sorter.hpp 
77e52dec735d276389643f7f356cd763b2f785e9 
  src/master/allocator/sorter/drf/sorter.cpp 
ecc5586737b6b447c5a1cf1a37037832bcbacd69 


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

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


Testing
---

All tests passed.


Thanks,

Anindya Sinha

Re: Review Request 57935: Recalculate shares only when total scalar quantities have changed.

[Anindya Sinha]

> On June 2, 2017, 4:26 p.m., Neil Conway wrote:
> > src/master/allocator/sorter/drf/sorter.hpp
> > Lines 347 (patched)
> > 
> >
> > Why is `flatten` necessary here? We are already comparing two scalar 
> > quantities, so is the flatten required?

In the case of a `RESERVE` or `UNRESERVE` operation, the `newAllocation` and 
`oldAllocation` varies in distribution of resources across roles.

eg. For `RESERVE`, oldAllocation = `disk(*):100`, and newAllocation = 
`disk(*):90; disk(role1):10`.

So, `createStrippedScalarQuantities()` on these `Resources` shall drop the 
`ReservationInfo` but the roles will remain intact.
Without `flatten()`, `oldAllocationQuantity` and `newAllocationQuantity` will 
not be the same (due to different roles) and hence `dirty` shall be set. But I 
do not think we need to recalculate shares since the total resources have not 
changed (only the distribution has changed in terms of roles). That is the 
reason for the comparison having the `flatten()`.

Looking at when `dirty` is true: We update `totals` (which is hashmap 
`` in `update()`. And when `calculateShare()` is 
called, we calculate share based on totals in the Sorter and individual Node. 
So I think we should be good to have the `flatten()` in this comparison.

Let me know if this does not sound ok.


> On June 2, 2017, 4:26 p.m., Neil Conway wrote:
> > src/master/allocator/sorter/drf/sorter.cpp
> > Lines 307 (patched)
> > 
> >
> >

Dropping this as there is no comment here.


> On June 2, 2017, 4:26 p.m., Neil Conway wrote:
> > src/master/allocator/sorter/drf/sorter.cpp
> > Lines 307 (patched)
> > 
> >
> >

Dropping this as there is no comment here.


> On June 2, 2017, 4:26 p.m., Neil Conway wrote:
> > src/master/allocator/sorter/drf/sorter.cpp
> > Lines 307 (patched)
> > 
> >
> > The return value of `update` depends only on the parameters 
> > `oldAllocation` and `newAllocation`, right? In which case, the way this is 
> > written seems confusing to me (e.g., it implies it would be possible for 
> > `dirty` to remain false when we start at a leaf node, but then for `dirty` 
> > to be true for an ancestor node).
> > 
> > An improvement would be to `CHECK` that `update()` returns the same 
> > value for all the nodes in the path from leaf -> root.
> > 
> > We could alternatively check whether the total allocation has changed 
> > outside `update` and only update `dirty` once. We could conceivably compute 
> > `oldAllocationQuantity` and `newAllocationQuantity` in `allocated` and pass 
> > them into `update` (for efficiency), but that is a bit ugly.
> > 
> > Let me know what you think.

SGTM. I modified based on the 1st recommendation. I do a `CHECK` to ensure that 
`update()` returns the same value for all nodes in the path from leaf -> root.


- Anindya


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


On June 2, 2017, 4:59 p.m., Anindya Sinha wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/57935/
> ---
> 
> (Updated June 2, 2017, 4:59 p.m.)
> 
> 
> Review request for mesos and Neil Conway.
> 
> 
> Bugs: MESOS-7138
> https://issues.apache.org/jira/browse/MESOS-7138
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> In `DRFSorter::update`, we should set the `dirty` flag only when the
> total scalar quantities have changed in any of the clients in the
> hierarchy, and not always.
> 
> 
> Diffs
> -
> 
>   src/master/allocator/sorter/drf/sorter.hpp 
> 77e52dec735d276389643f7f356cd763b2f785e9 
>   src/master/allocator/sorter/drf/sorter.cpp 
> ecc5586737b6b447c5a1cf1a37037832bcbacd69 
> 
> 
> Diff: https://reviews.apache.org/r/57935/diff/4/
> 
> 
> Testing
> ---
> 
> All tests passed.
> 
> 
> Thanks,
> 
> Anindya Sinha
> 
>

Re: Review Request 59731: Added subcomponent maintainers for the mesos containerizer.

[Jie Yu]

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


Ship it!




Ship It!

- Jie Yu


On June 1, 2017, 10:02 p.m., Benjamin Mahler wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/59731/
> ---
> 
> (Updated June 1, 2017, 10:02 p.m.)
> 
> 
> Review request for mesos, Avinash sridharan, Benjamin Hindman, Gilbert Song, 
> haosdent huang, Jie Yu, James Peach, Kevin Klues, Qian Zhang, and Jiang Yan 
> Xu.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Since the maintenance of the mesos containerizer is done on a
> subcomponent basis, this documents who has been maintaining
> the various subcomponents.
> 
> 
> Diffs
> -
> 
>   docs/committers.md cb4e5d3c06a4346cd890739d1f630b90a85b9e8a 
> 
> 
> Diff: https://reviews.apache.org/r/59731/diff/1/
> 
> 
> Testing
> ---
> 
> N/A
> 
> 
> Thanks,
> 
> Benjamin Mahler
> 
>

Re: Review Request 59749: Removed out-dated working groups.

[Benjamin Mahler]

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


Ship it!




Ship It!

- Benjamin Mahler


On June 2, 2017, 4:13 p.m., Jie Yu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/59749/
> ---
> 
> (Updated June 2, 2017, 4:13 p.m.)
> 
> 
> Review request for mesos, Benjamin Mahler and Vinod Kone.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Removed out-dated working groups.
> 
> 
> Diffs
> -
> 
>   docs/working-groups.md b6a0b358a340658849e25646f5174e4c299734ff 
> 
> 
> Diff: https://reviews.apache.org/r/59749/diff/1/
> 
> 
> Testing
> ---
> 
> n/a
> 
> 
> Thanks,
> 
> Jie Yu
> 
>

Re: Review Request 59750: Added mailing list guidance for working groups.

[Benjamin Mahler]

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


Fix it, then Ship it!





docs/working-groups.md
Line 6 (original), 6 (patched)


Are you planning to define what a working group is? Seems we should have 
some clarity on that, since for example, there are lots of slack channels 
organized by topic, or components in JIRA, etc. Would be good this as part of 
this chain.



docs/working-groups.md
Lines 10 (patched)


Is the link helpful? Maybe just "the d...@mesos.apache.org list"



docs/working-groups.md
Lines 11 (patched)


s/list/lists/


- Benjamin Mahler


On June 2, 2017, 4:14 p.m., Jie Yu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/59750/
> ---
> 
> (Updated June 2, 2017, 4:14 p.m.)
> 
> 
> Review request for mesos, Benjamin Mahler and Vinod Kone.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added mailing list guidance for working groups.
> 
> 
> Diffs
> -
> 
>   docs/working-groups.md b6a0b358a340658849e25646f5174e4c299734ff 
> 
> 
> Diff: https://reviews.apache.org/r/59750/diff/1/
> 
> 
> Testing
> ---
> 
> n/a
> 
> 
> Thanks,
> 
> Jie Yu
> 
>

Re: Review Request 59751: Added a table for the active working groups.

[Benjamin Mahler]

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


Ship it!




Would it be good to define what an "active" working group is?

- Benjamin Mahler


On June 2, 2017, 4:14 p.m., Jie Yu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/59751/
> ---
> 
> (Updated June 2, 2017, 4:14 p.m.)
> 
> 
> Review request for mesos, Benjamin Mahler and Vinod Kone.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added a table for the active working groups.
> 
> 
> Diffs
> -
> 
>   docs/working-groups.md b6a0b358a340658849e25646f5174e4c299734ff 
> 
> 
> Diff: https://reviews.apache.org/r/59751/diff/1/
> 
> 
> Testing
> ---
> 
> n/a
> 
> 
> Thanks,
> 
> Jie Yu
> 
>

Re: Review Request 59751: Added a table for the active working groups.

[Mesos Reviewbot Windows]

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



Patch looks great!

Reviews applied: [59749, 59750, 59751]

Passed command: support\windows-build.bat

- Mesos Reviewbot Windows


On June 2, 2017, 4:14 p.m., Jie Yu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/59751/
> ---
> 
> (Updated June 2, 2017, 4:14 p.m.)
> 
> 
> Review request for mesos, Benjamin Mahler and Vinod Kone.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added a table for the active working groups.
> 
> 
> Diffs
> -
> 
>   docs/working-groups.md b6a0b358a340658849e25646f5174e4c299734ff 
> 
> 
> Diff: https://reviews.apache.org/r/59751/diff/1/
> 
> 
> Testing
> ---
> 
> n/a
> 
> 
> Thanks,
> 
> Jie Yu
> 
>

Re: Review Request 57817: Offers not sent for suppressed roles as indicated in `SUBSCRIBE`.

[Anindya Sinha]

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

(Updated June 2, 2017, 5:49 p.m.)


Review request for mesos, James Peach, Vinod Kone, and Jiang Yan Xu.


Changes
---

Addressed comments.


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


Repository: mesos


Description
---

If the `SUBSCRIBE` indicates a subset of roles to be suppressed during
framework (re)registration, the allocator does not offer resources for
those roles to such frameworks. Note that this functionality is added
for `v1::SUBSCRIBE` only (and not for scheduler driver API).

In addition, the master validates the suppressed roles to be a subset
of all the roles being (re)registered by the framework.


Diffs (updated)
-

  include/mesos/allocator/allocator.hpp 
b31fbcfc18cbbe1e6b5fb5ccbc234f790326a5b4 
  src/master/allocator/mesos/allocator.hpp 
b2dcb566c49a1bbb1d955ca46e1c8eef91e62733 
  src/master/allocator/mesos/hierarchical.hpp 
5e7c3068061012c51d4b9220dedf476408016a12 
  src/master/allocator/mesos/hierarchical.cpp 
8ebdbc6a2b98feab7ce4d7f07b15d8fb92992270 
  src/master/master.hpp 89d0790fd5fea59e74276f462581fe0073594732 
  src/master/master.cpp c66907bd55cb2eb549ec89f048d41376df556eb9 
  src/tests/allocator.hpp 0b9fdeaccc65bb6988103ac156e0584cf5df17fe 
  src/tests/hierarchical_allocator_tests.cpp 
eb2b647d3247a85e8b9b82e5589232c74ad8570f 
  src/tests/master_allocator_tests.cpp 0082045ccaebe015725c5e178aa89dac32b1c50c 
  src/tests/persistent_volume_endpoints_tests.cpp 
9b820b8f4e0d0e2c62b5c9b420b790cdfb59dd0c 
  src/tests/reservation_tests.cpp 34628eed671aee7ed69497d282c8043d65620c14 
  src/tests/resource_offers_tests.cpp 7ad037e5ec1d63cc8c2b8b0ad092e3f5f6f402c8 
  src/tests/slave_recovery_tests.cpp 86cf971aa8aea012a74740a4dcbaf5ed2ad9c866 


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

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


Testing
---

All tests passed.


Thanks,

Anindya Sinha

Re: Review Request 57817: Offers not sent for suppressed roles as indicated in `SUBSCRIBE`.

[Anindya Sinha]

> On June 1, 2017, 5:41 p.m., Benjamin Mahler wrote:
> > src/common/protobuf_utils.hpp
> > Lines 359-361 (patched)
> > 
> >
> > This looks to be just a generic conversion from repeated ptr field to 
> > set, not something role specific?
> > 
> > This means we should either inline the set construction (using the 
> > iterator approach), or expose a generic conversion (would we want to return 
> > an Error if there are duplicates?)

Agreed. Removed the function and calling it inline instead.


> On June 1, 2017, 5:41 p.m., Benjamin Mahler wrote:
> > src/master/allocator/mesos/hierarchical.cpp
> > Lines 391-395 (patched)
> > 
> >
> > Is this stale? Seems to refer to "deactivated" roles whereas the 
> > variables are "suppressed" roles

Since `deactivate()` is idempotent (as in next comment), there is no need to 
add the `if (!framework.suppressedRoles.count(role))` condition, and hence I do 
not think the comment is needed here.


> On June 1, 2017, 5:41 p.m., Benjamin Mahler wrote:
> > src/master/allocator/mesos/hierarchical.cpp
> > Lines 449-450 (original), 479-481 (patched)
> > 
> >
> > Not your bug, but it seems wrong to be activating roles here if the 
> > framework was deactivated. Would be good to sync with mpark on this 
> > original change. It seems to me that if the framework is deactivated via 
> > deactivateFramework, we wouldn't want to be activating things here in 
> > updateFrameowrk.

Reached out to @mpark. I kept it as-is till he has a chance to look into it.


> On June 1, 2017, 5:41 p.m., Benjamin Mahler wrote:
> > src/master/allocator/mesos/hierarchical.cpp
> > Lines 485 (patched)
> > 
> >
> > Seems there is a bug here? What about existing roles (not covered by 
> > "removed roles" or "added roles" loops above) that have now become 
> > suppressed? For these, we want to deactivate them.

Yes, that is correct.
So, the roles that need to be deactivated are the roles which have been 
removed, as well as the roles which have moved from non-suppressed mode to 
suppressed mode. Similarly, the roles that need to be activated are the roles 
that have been added, as well as the roles which have moved from suppressed to 
non-suppressed mode.


- Anindya


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


On June 1, 2017, 12:38 a.m., Anindya Sinha wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/57817/
> ---
> 
> (Updated June 1, 2017, 12:38 a.m.)
> 
> 
> Review request for mesos, James Peach, Vinod Kone, and Jiang Yan Xu.
> 
> 
> Bugs: MESOS-7015
> https://issues.apache.org/jira/browse/MESOS-7015
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> If the `SUBSCRIBE` indicates a subset of roles to be suppressed during
> framework (re)registration, the allocator does not offer resources for
> those roles to such frameworks. Note that this functionality is added
> for `v1::SUBSCRIBE` only (and not for scheduler driver API).
> 
> In addition, the master validates the suppressed roles to be a subset
> of all the roles being (re)registered by the framework.
> 
> 
> Diffs
> -
> 
>   include/mesos/allocator/allocator.hpp 
> b31fbcfc18cbbe1e6b5fb5ccbc234f790326a5b4 
>   src/common/protobuf_utils.hpp be2325f05b81b847fa592eff65175cbc99764fd6 
>   src/common/protobuf_utils.cpp 3fcaf786b29a00f003c10b0f1614a2c7eddc725d 
>   src/master/allocator/mesos/allocator.hpp 
> b2dcb566c49a1bbb1d955ca46e1c8eef91e62733 
>   src/master/allocator/mesos/hierarchical.hpp 
> 5e7c3068061012c51d4b9220dedf476408016a12 
>   src/master/allocator/mesos/hierarchical.cpp 
> 6679d47ea53bbcd68d375654edf6e85890e5772a 
>   src/master/master.hpp 89d0790fd5fea59e74276f462581fe0073594732 
>   src/master/master.cpp c66907bd55cb2eb549ec89f048d41376df556eb9 
>   src/tests/allocator.hpp 0b9fdeaccc65bb6988103ac156e0584cf5df17fe 
>   src/tests/hierarchical_allocator_tests.cpp 
> eb2b647d3247a85e8b9b82e5589232c74ad8570f 
>   src/tests/master_allocator_tests.cpp 
> 0082045ccaebe015725c5e178aa89dac32b1c50c 
>   src/tests/persistent_volume_endpoints_tests.cpp 
> 9b820b8f4e0d0e2c62b5c9b420b790cdfb59dd0c 
>   src/tests/reservation_tests.cpp 34628eed671aee7ed69497d282c8043d65620c14 
>   src/tests/resource_offers_tests.cpp 
> 7ad037e5ec1d63cc8c2b8b0ad092e3f5f6f402c8 
>   src/tests/slave_recovery_tests.cpp e140f4d902a43b8fb2390541a357a217896b6228 
> 
> 
> Diff: https:/

Re: Review Request 57818: Added unit tests to verify offers are suppressed based on registration.

[Anindya Sinha]

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

(Updated June 2, 2017, 5:50 p.m.)


Review request for mesos, James Peach, Vinod Kone, and Jiang Yan Xu.


Changes
---

Rebased.


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


Repository: mesos


Description
---

Added unit tests to verify offers are suppressed based on registration.


Diffs (updated)
-

  src/tests/scheduler_tests.cpp d23a393a8123b7e1a0d613dec225daabe3694169 


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

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


Testing
---

All tests passed.


Thanks,

Anindya Sinha

Re: Review Request 59605: Used glog for logging in executors.

[Mesos Reviewbot Windows]

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



Patch looks great!

Reviews applied: [59605]

Passed command: support\windows-build.bat

- Mesos Reviewbot Windows


On June 2, 2017, 4:18 p.m., Andrei Budnik wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/59605/
> ---
> 
> (Updated June 2, 2017, 4:18 p.m.)
> 
> 
> Review request for mesos, Alexander Rukletsov, Benjamin Bannier, Benjamin 
> Mahler, and Till Toenshoff.
> 
> 
> Bugs: MESOS-6961
> https://issues.apache.org/jira/browse/MESOS-6961
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> The rule for choosing between glog and cout is described by MESOS-7586.
> 
> 
> Diffs
> -
> 
>   src/docker/executor.cpp 464a74859e0f477ec2e5525a5fc726ea420a00a2 
>   src/launcher/default_executor.cpp 1a60e3bb2b0e8312b349cddedb4cd55716c1b574 
>   src/launcher/executor.cpp 9ac3c3d84c0ec47954c5c72228ad0b8795ff3eec 
> 
> 
> Diff: https://reviews.apache.org/r/59605/diff/3/
> 
> 
> Testing
> ---
> 
> 1. make check (mac os x 10.12, fedora 25)
> 2. jenkins: 
> https://jenkins.mesosphere.com/service/jenkins/job/mesos/job/Mesos_CI-build/1087/
> 
> 3. I've temporarily modified NoTaskKillingCapability from 
> command_executor_tests.cpp to launch command "sleep 0.1" (as well as "sleep 
> 0"),
> then to wait for a task completion. The test was launched in an endless loop:
> 
> GLOG_v=1 ./bin/mesos-tests.sh  --gtest_repeat=-1 --gtest_break_on_failure 
> --gtest_filter="HTTPCommandExecutor/CommandExecutorTest.NoTaskKillingCapability/0"
>  --verbose
> 
> I've run this test multiple times for a while both on mac os and Fedora 
> linux. Race condition problem described in a jira ticket wasn't reproduced.
> 
> 
> Thanks,
> 
> Andrei Budnik
> 
>

Re: Review Request 59750: Added mailing list guidance for working groups.

[Vinod Kone]

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


Ship it!




LGTM. Modue

- Vinod Kone


On June 2, 2017, 4:14 p.m., Jie Yu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/59750/
> ---
> 
> (Updated June 2, 2017, 4:14 p.m.)
> 
> 
> Review request for mesos, Benjamin Mahler and Vinod Kone.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added mailing list guidance for working groups.
> 
> 
> Diffs
> -
> 
>   docs/working-groups.md b6a0b358a340658849e25646f5174e4c299734ff 
> 
> 
> Diff: https://reviews.apache.org/r/59750/diff/1/
> 
> 
> Testing
> ---
> 
> n/a
> 
> 
> Thanks,
> 
> Jie Yu
> 
>

Re: Review Request 59750: Added mailing list guidance for working groups.

[Vinod Kone]

> On June 2, 2017, 5:27 p.m., Benjamin Mahler wrote:
> > docs/working-groups.md
> > Line 6 (original), 6 (patched)
> > 
> >
> > Are you planning to define what a working group is? Seems we should 
> > have some clarity on that, since for example, there are lots of slack 
> > channels organized by topic, or components in JIRA, etc. Would be good this 
> > as part of this chain.

I agree. How about the following

"
A working group is a group of people who are interested in actively 
participating in specific area of the project. Below are some of the 
expectations of a working group

--> Should have atleast one lead who is responsible for organizing and driving 
the work group meetings.

--> Should have regular (online) meetings that are open to public, recorded and 
archived.

--> Should maintain meeting notes that are shared with the dev list regularly.

--> Should have a corresponding slack channel (also publicly archived) where 
synchronous discussions take place.
"


- Vinod


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


On June 2, 2017, 4:14 p.m., Jie Yu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/59750/
> ---
> 
> (Updated June 2, 2017, 4:14 p.m.)
> 
> 
> Review request for mesos, Benjamin Mahler and Vinod Kone.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added mailing list guidance for working groups.
> 
> 
> Diffs
> -
> 
>   docs/working-groups.md b6a0b358a340658849e25646f5174e4c299734ff 
> 
> 
> Diff: https://reviews.apache.org/r/59750/diff/1/
> 
> 
> Testing
> ---
> 
> n/a
> 
> 
> Thanks,
> 
> Jie Yu
> 
>

Re: Review Request 59746: Separated discarded and failed cases for container launch.

[Gastón Kleiman]

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




src/slave/slave.cpp
Lines 5162 (patched)


I don't think that including "future discarded" in the message is useful 
for framework developers.

It is an implementation detail and they shouldn't have to dig in the code 
to find where it comes from.


- Gastón Kleiman


On June 2, 2017, 1:10 p.m., Alexander Rukletsov wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/59746/
> ---
> 
> (Updated June 2, 2017, 1:10 p.m.)
> 
> 
> Review request for mesos, Ian Downes, Jie Yu, Joseph Wu, and Jan Schlicht.
> 
> 
> Bugs: MESOS-7601
> https://issues.apache.org/jira/browse/MESOS-7601
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Discarded future returned from the containerizer->launch() does not
> necessarily mean that the container launch has failed. For example,
> a framework may stop while its task are being started.
> 
> 
> Diffs
> -
> 
>   include/mesos/mesos.proto 5f80170fcd3c05add8b6e9e3107cff062818c1dc 
>   include/mesos/v1/mesos.proto 4b528751006f709f841e44f48c9f5c2dc035b402 
>   src/slave/slave.cpp 0c7e5f4ef905b3897d341c3147a208fc7a8a12e0 
> 
> 
> Diff: https://reviews.apache.org/r/59746/diff/1/
> 
> 
> Testing
> ---
> 
> make check on several Linux distros.
> 
> 
> Thanks,
> 
> Alexander Rukletsov
> 
>

Re: Review Request 59746: Separated discarded and failed cases for container launch.

[Gastón Kleiman]

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




src/slave/slave.cpp
Lines 5152 (patched)


We might want to have a metric called `aborted_container_launches` or 
something like that.

If a framework is expected not to stop until no more tasks are running, 
operators could monitor this metric and be alerted if there's a bug.


- Gastón Kleiman


On June 2, 2017, 1:10 p.m., Alexander Rukletsov wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/59746/
> ---
> 
> (Updated June 2, 2017, 1:10 p.m.)
> 
> 
> Review request for mesos, Ian Downes, Jie Yu, Joseph Wu, and Jan Schlicht.
> 
> 
> Bugs: MESOS-7601
> https://issues.apache.org/jira/browse/MESOS-7601
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Discarded future returned from the containerizer->launch() does not
> necessarily mean that the container launch has failed. For example,
> a framework may stop while its task are being started.
> 
> 
> Diffs
> -
> 
>   include/mesos/mesos.proto 5f80170fcd3c05add8b6e9e3107cff062818c1dc 
>   include/mesos/v1/mesos.proto 4b528751006f709f841e44f48c9f5c2dc035b402 
>   src/slave/slave.cpp 0c7e5f4ef905b3897d341c3147a208fc7a8a12e0 
> 
> 
> Diff: https://reviews.apache.org/r/59746/diff/1/
> 
> 
> Testing
> ---
> 
> make check on several Linux distros.
> 
> 
> Thanks,
> 
> Alexander Rukletsov
> 
>

Re: Review Request 57935: Recalculate shares only when total scalar quantities have changed.

[Mesos Reviewbot Windows]

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



Patch looks great!

Reviews applied: [57935]

Passed command: support\windows-build.bat

- Mesos Reviewbot Windows


On June 2, 2017, 4:59 p.m., Anindya Sinha wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/57935/
> ---
> 
> (Updated June 2, 2017, 4:59 p.m.)
> 
> 
> Review request for mesos and Neil Conway.
> 
> 
> Bugs: MESOS-7138
> https://issues.apache.org/jira/browse/MESOS-7138
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> In `DRFSorter::update`, we should set the `dirty` flag only when the
> total scalar quantities have changed in any of the clients in the
> hierarchy, and not always.
> 
> 
> Diffs
> -
> 
>   src/master/allocator/sorter/drf/sorter.hpp 
> 77e52dec735d276389643f7f356cd763b2f785e9 
>   src/master/allocator/sorter/drf/sorter.cpp 
> ecc5586737b6b447c5a1cf1a37037832bcbacd69 
> 
> 
> Diff: https://reviews.apache.org/r/57935/diff/4/
> 
> 
> Testing
> ---
> 
> All tests passed.
> 
> 
> Thanks,
> 
> Anindya Sinha
> 
>

Re: Review Request 57818: Added unit tests to verify offers are suppressed based on registration.

[Mesos Reviewbot Windows]

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



Patch looks great!

Reviews applied: [57815, 57817, 57818]

Passed command: support\windows-build.bat

- Mesos Reviewbot Windows


On June 2, 2017, 5:50 p.m., Anindya Sinha wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/57818/
> ---
> 
> (Updated June 2, 2017, 5:50 p.m.)
> 
> 
> Review request for mesos, James Peach, Vinod Kone, and Jiang Yan Xu.
> 
> 
> Bugs: MESOS-7015
> https://issues.apache.org/jira/browse/MESOS-7015
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added unit tests to verify offers are suppressed based on registration.
> 
> 
> Diffs
> -
> 
>   src/tests/scheduler_tests.cpp d23a393a8123b7e1a0d613dec225daabe3694169 
> 
> 
> Diff: https://reviews.apache.org/r/57818/diff/13/
> 
> 
> Testing
> ---
> 
> All tests passed.
> 
> 
> Thanks,
> 
> Anindya Sinha
> 
>

Re: Review Request 57817: Offers not sent for suppressed roles as indicated in `SUBSCRIBE`.

[Vinod Kone]

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




src/master/allocator/mesos/hierarchical.cpp
Lines 430-431 (patched)


what about roles that are suppressed in old state and new state?



src/master/allocator/mesos/hierarchical.cpp
Lines 463 (patched)


what about new roles that are also suppressed?


- Vinod Kone


On June 2, 2017, 5:49 p.m., Anindya Sinha wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/57817/
> ---
> 
> (Updated June 2, 2017, 5:49 p.m.)
> 
> 
> Review request for mesos, James Peach, Vinod Kone, and Jiang Yan Xu.
> 
> 
> Bugs: MESOS-7015
> https://issues.apache.org/jira/browse/MESOS-7015
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> If the `SUBSCRIBE` indicates a subset of roles to be suppressed during
> framework (re)registration, the allocator does not offer resources for
> those roles to such frameworks. Note that this functionality is added
> for `v1::SUBSCRIBE` only (and not for scheduler driver API).
> 
> In addition, the master validates the suppressed roles to be a subset
> of all the roles being (re)registered by the framework.
> 
> 
> Diffs
> -
> 
>   include/mesos/allocator/allocator.hpp 
> b31fbcfc18cbbe1e6b5fb5ccbc234f790326a5b4 
>   src/master/allocator/mesos/allocator.hpp 
> b2dcb566c49a1bbb1d955ca46e1c8eef91e62733 
>   src/master/allocator/mesos/hierarchical.hpp 
> 5e7c3068061012c51d4b9220dedf476408016a12 
>   src/master/allocator/mesos/hierarchical.cpp 
> 8ebdbc6a2b98feab7ce4d7f07b15d8fb92992270 
>   src/master/master.hpp 89d0790fd5fea59e74276f462581fe0073594732 
>   src/master/master.cpp c66907bd55cb2eb549ec89f048d41376df556eb9 
>   src/tests/allocator.hpp 0b9fdeaccc65bb6988103ac156e0584cf5df17fe 
>   src/tests/hierarchical_allocator_tests.cpp 
> eb2b647d3247a85e8b9b82e5589232c74ad8570f 
>   src/tests/master_allocator_tests.cpp 
> 0082045ccaebe015725c5e178aa89dac32b1c50c 
>   src/tests/persistent_volume_endpoints_tests.cpp 
> 9b820b8f4e0d0e2c62b5c9b420b790cdfb59dd0c 
>   src/tests/reservation_tests.cpp 34628eed671aee7ed69497d282c8043d65620c14 
>   src/tests/resource_offers_tests.cpp 
> 7ad037e5ec1d63cc8c2b8b0ad092e3f5f6f402c8 
>   src/tests/slave_recovery_tests.cpp 86cf971aa8aea012a74740a4dcbaf5ed2ad9c866 
> 
> 
> Diff: https://reviews.apache.org/r/57817/diff/13/
> 
> 
> Testing
> ---
> 
> All tests passed.
> 
> 
> Thanks,
> 
> Anindya Sinha
> 
>

Re: Review Request 58508: Introduced method `Resources::inheritable` to aggregate reservations.

[Michael Park]

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


Fix it, then Ship it!





include/mesos/resources.hpp
Lines 292 (patched)


We also need a v1 version of these, right?


- Michael Park


On April 20, 2017, 9:18 a.m., Jay Guo wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/58508/
> ---
> 
> (Updated April 20, 2017, 9:18 a.m.)
> 
> 
> Review request for mesos and Michael Park.
> 
> 
> Bugs: MESOS-7149
> https://issues.apache.org/jira/browse/MESOS-7149
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> A reservation is inheritable by 'role' iff:
>  - it is reserved to ancestor of 'role' in hierarchy, OR
>  - it is reserved to 'role' itself.
> 
> This is a helper function for reservation delegate, where reserved
> resources can be 'delegated' downwards in the hierarchy.
> 
> 
> Diffs
> -
> 
>   include/mesos/resources.hpp 74583e3c784f22b45cce81f07c5a69b339e1684d 
>   src/common/resources.cpp 77bac0c4390da905016a942991b14a79877f8455 
>   src/tests/resources_tests.cpp 4cf320d802a749f1419ac5b9f63b6c73b0c974be 
> 
> 
> Diff: https://reviews.apache.org/r/58508/diff/2/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Jay Guo
> 
>

Re: Review Request 58509: Enabled allocator to handle hierarchical reservation.

[Michael Park]

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


Ship it!




Ship It!

- Michael Park


On April 18, 2017, 8:44 a.m., Jay Guo wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/58509/
> ---
> 
> (Updated April 18, 2017, 8:44 a.m.)
> 
> 
> Review request for mesos and Michael Park.
> 
> 
> Bugs: MESOS-7149
> https://issues.apache.org/jira/browse/MESOS-7149
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Other than looking at reservation made for a role, now allocator also
> aggregates reservations of all its ancestor in the hierarchy.
> 
> 
> Diffs
> -
> 
>   src/master/allocator/mesos/hierarchical.cpp 
> 051f749dd5921a322ca930a042c31814616d38f9 
> 
> 
> Diff: https://reviews.apache.org/r/58509/diff/1/
> 
> 
> Testing
> ---
> 
> see next patch in chain
> 
> 
> Thanks,
> 
> Jay Guo
> 
>

Re: Review Request 58510: Added two tests for hierarchical reservation.

[Michael Park]

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


Ship it!




Ship It!

- Michael Park


On April 18, 2017, 8:45 a.m., Jay Guo wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/58510/
> ---
> 
> (Updated April 18, 2017, 8:45 a.m.)
> 
> 
> Review request for mesos and Michael Park.
> 
> 
> Bugs: MESOS-7149
> https://issues.apache.org/jira/browse/MESOS-7149
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added two tests for hierarchical reservation.
> 
> 
> Diffs
> -
> 
>   src/tests/hierarchical_allocator_tests.cpp 
> 33e7b455f8664858eb4f03727b076a10c80cd6e0 
>   src/tests/reservation_tests.cpp 4504831d77c1bfcf5f2ddf6d28cd45dea2c421ad 
> 
> 
> Diff: https://reviews.apache.org/r/58510/diff/1/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Jay Guo
> 
>

Re: Review Request 58510: Added two tests for hierarchical reservation.

[Mesos Reviewbot Windows]

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



Bad review!

Reviews applied: [58510, 58509, 58508, 57516, 58584, 57254, 58112, 58110, 
57564, 57528, 57527, 57788, 57167, 57166, 56805, 57165, 57164]

Error:
No reviewers specified. Please find a reviewer by asking on JIRA or the mailing 
list.

- Mesos Reviewbot Windows


On April 18, 2017, 3:45 p.m., Jay Guo wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/58510/
> ---
> 
> (Updated April 18, 2017, 3:45 p.m.)
> 
> 
> Review request for mesos and Michael Park.
> 
> 
> Bugs: MESOS-7149
> https://issues.apache.org/jira/browse/MESOS-7149
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added two tests for hierarchical reservation.
> 
> 
> Diffs
> -
> 
>   src/tests/hierarchical_allocator_tests.cpp 
> 33e7b455f8664858eb4f03727b076a10c80cd6e0 
>   src/tests/reservation_tests.cpp 4504831d77c1bfcf5f2ddf6d28cd45dea2c421ad 
> 
> 
> Diff: https://reviews.apache.org/r/58510/diff/1/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Jay Guo
> 
>

Review Request 59759: Added protobuf definitions for fault domains.

[Neil Conway]

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

Review request for mesos and Vinod Kone.


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


Repository: mesos


Description
---

This introduces a first-class notion of a "domain", which is a set of
hosts that have similar characteristics. Mesos will initially only
support "fault domains", which identify groups of hosts with similar
failure characteristics.


Diffs
-

  include/mesos/mesos.proto 5f80170fcd3c05add8b6e9e3107cff062818c1dc 
  include/mesos/type_utils.hpp c7872c0cd7f19d923e8be1f33a775d5eb17a0cf7 
  include/mesos/v1/mesos.hpp c2e6ec5c4fa06d0179d6aaf3371ace4b6e6dbdaf 
  include/mesos/v1/mesos.proto 4b528751006f709f841e44f48c9f5c2dc035b402 
  src/common/type_utils.cpp 5f8e72b97d6766f9729079f6c6c013bc117cedb9 
  src/v1/mesos.cpp 3b9f9b43e748159ae753880bbe4a5975814073ab 


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


Testing
---

`make check`


Thanks,

Neil Conway

Review Request 59760: Added REGION_AWARE framework capability.

[Neil Conway]

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

Review request for mesos and Vinod Kone.


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


Repository: mesos


Description
---

Added REGION_AWARE framework capability.


Diffs
-

  include/mesos/mesos.proto 5f80170fcd3c05add8b6e9e3107cff062818c1dc 
  include/mesos/v1/mesos.proto 4b528751006f709f841e44f48c9f5c2dc035b402 
  src/common/protobuf_utils.hpp be2325f05b81b847fa592eff65175cbc99764fd6 
  src/tests/protobuf_utils_tests.cpp 6ec3a8e606e86406a0f1c9d5de4ed09d0811e49a 


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


Testing
---

`make check`


Thanks,

Neil Conway

Review Request 59762: Added domain to MasterInfo and SlaveInfo.

[Neil Conway]

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

Review request for mesos and Vinod Kone.


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


Repository: mesos


Description
---

This means that each master's domain is stored in ZooKeeper, along with
the rest of the MasterInfo protobuf message.

Each agent's domain is stored as part of its checkpointed resources.
Changing the agent's domain requires a full drain of the agent; this
behavior might be relaxed in the future.


Diffs
-

  include/mesos/mesos.proto 5f80170fcd3c05add8b6e9e3107cff062818c1dc 
  include/mesos/v1/mesos.proto 4b528751006f709f841e44f48c9f5c2dc035b402 
  src/common/http.hpp 93d6088e97c2384f9f6d26e010a501abf2deb43e 
  src/common/http.cpp 167dce2b9a2d3b68a1df5b4079f701482d34db28 
  src/common/type_utils.cpp 5f8e72b97d6766f9729079f6c6c013bc117cedb9 
  src/internal/evolve.hpp 9db5fe6155243576f186a8b974e81068505b9fcb 
  src/internal/evolve.cpp 93196f301e820b99572ee008b98a124ddafe9697 
  src/master/http.cpp 1dcfe6ef00b0e3984deb79a511e665f638661323 
  src/master/master.cpp c66907bd55cb2eb549ec89f048d41376df556eb9 
  src/slave/http.cpp 78b35865e465ff1e8e7e4950fdb60e3a48b916b6 
  src/slave/slave.cpp 0c7e5f4ef905b3897d341c3147a208fc7a8a12e0 
  src/tests/api_tests.cpp 91b3473452b8e65cab9f2e873837d64a0edf4b54 
  src/tests/master_tests.cpp 490d7ed4b275ebf5ff6956f7d40dbea3ce3b63e2 
  src/tests/mesos.hpp 48072a976cdbe6e655dc6f5f258abc6d737ed068 
  src/v1/mesos.cpp 3b9f9b43e748159ae753880bbe4a5975814073ab 


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


Testing
---

`make check`


Thanks,

Neil Conway

Review Request 59761: Added master and agent flags to specify domain.

[Neil Conway]

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

Review request for mesos and Vinod Kone.


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


Repository: mesos


Description
---

Added master and agent flags to specify domain.


Diffs
-

  docs/configuration.md ed510fa638878b71e7fcff4850152a8a8622127e 
  src/common/parse.hpp 64eabf89f81a6708cfac89081e0d9f9586721693 
  src/master/flags.hpp 8a9bd2dfa513f1de85dab2cc3cc1d78e80293837 
  src/master/flags.cpp ca5f02c728a9f39abbbee59fa169f95d20c69d3d 
  src/slave/flags.hpp 2f9d52e94c2c31e95208cd8b0640a5de2d2a61fd 
  src/slave/flags.cpp 93c8ffb5c822cf6c99071be7aca52a6b3d187619 


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


Testing
---

`make check`


Thanks,

Neil Conway

Review Request 59763: Caused master to abort when joining a mixed-region cluster.

[Neil Conway]

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

Review request for mesos and Vinod Kone.


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


Repository: mesos


Description
---

That is, if a standby master is configured to use region X but it learns
that the current master has region Y, the standby master will abort with
an error message. This enforces the requirement that all masters in a
single Mesos cluster are configured to use the same region (they can be
configured to use different zones in that region, however).

To allow graceful upgrades, we only abort the standby master if both the
standby master and the leading master have a configured domain; if
either master has the unset (default) domain, the standby master does
not abort.

NOTE: It would be nice to have unit tests to validate this behavior, but
the current unit test infrastructure does not support starting multiple
masters (MESOS-2976).


Diffs
-

  include/mesos/type_utils.hpp c7872c0cd7f19d923e8be1f33a775d5eb17a0cf7 
  include/mesos/v1/mesos.hpp c2e6ec5c4fa06d0179d6aaf3371ace4b6e6dbdaf 
  src/master/master.cpp c66907bd55cb2eb549ec89f048d41376df556eb9 


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


Testing
---

`make check`

Manual testing.


Thanks,

Neil Conway

Review Request 59764: Ignore registration attempts by agents with misconfigured domain.

[Neil Conway]

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

Review request for mesos and Vinod Kone.


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


Repository: mesos


Description
---

We expect the master's domain to be configured first, then the domain of
the agents to be configured. Therefore, if an agent with configured
domain attempts to register or re-register with a master that does not
have a configured domain, the registration attempt should be ignored.
This is important, because the master would have no way of determining
whether the agent is "remote" or not, which might result in placing
inappropriate workloads on it.


Diffs
-

  src/master/master.cpp c66907bd55cb2eb549ec89f048d41376df556eb9 
  src/tests/master_tests.cpp 490d7ed4b275ebf5ff6956f7d40dbea3ce3b63e2 


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


Testing
---

`make check`

Manual testing.


Thanks,

Neil Conway

Re: Review Request 59763: Caused master to abort when joining a mixed-region cluster.

[Mesos Reviewbot Windows]

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



Bad patch!

Reviews applied: [59763, 59762, 59761, 59760, 59759]

Failed command: python support/apply-reviews.py -n -r 59760

Error:
error: patch failed: include/mesos/mesos.proto:343
error: include/mesos/mesos.proto: patch does not apply
error: patch failed: include/mesos/v1/mesos.proto:343
error: include/mesos/v1/mesos.proto: patch does not apply

Full log: http://mesos-winbot.westus.cloudapp.azure.com/logs/console

- Mesos Reviewbot Windows


On June 2, 2017, 8:11 p.m., Neil Conway wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/59763/
> ---
> 
> (Updated June 2, 2017, 8:11 p.m.)
> 
> 
> Review request for mesos and Vinod Kone.
> 
> 
> Bugs: MESOS-7611
> https://issues.apache.org/jira/browse/MESOS-7611
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> That is, if a standby master is configured to use region X but it learns
> that the current master has region Y, the standby master will abort with
> an error message. This enforces the requirement that all masters in a
> single Mesos cluster are configured to use the same region (they can be
> configured to use different zones in that region, however).
> 
> To allow graceful upgrades, we only abort the standby master if both the
> standby master and the leading master have a configured domain; if
> either master has the unset (default) domain, the standby master does
> not abort.
> 
> NOTE: It would be nice to have unit tests to validate this behavior, but
> the current unit test infrastructure does not support starting multiple
> masters (MESOS-2976).
> 
> 
> Diffs
> -
> 
>   include/mesos/type_utils.hpp c7872c0cd7f19d923e8be1f33a775d5eb17a0cf7 
>   include/mesos/v1/mesos.hpp c2e6ec5c4fa06d0179d6aaf3371ace4b6e6dbdaf 
>   src/master/master.cpp c66907bd55cb2eb549ec89f048d41376df556eb9 
> 
> 
> Diff: https://reviews.apache.org/r/59763/diff/1/
> 
> 
> Testing
> ---
> 
> `make check`
> 
> Manual testing.
> 
> 
> Thanks,
> 
> Neil Conway
> 
>

Review Request 59765: Added DomainInfo to resource offers.

[Neil Conway]

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

Review request for mesos and Vinod Kone.


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


Repository: mesos


Description
---

Added DomainInfo to resource offers.


Diffs
-

  include/mesos/mesos.proto 5f80170fcd3c05add8b6e9e3107cff062818c1dc 
  include/mesos/v1/mesos.proto 4b528751006f709f841e44f48c9f5c2dc035b402 
  src/master/master.cpp c66907bd55cb2eb549ec89f048d41376df556eb9 


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


Testing
---


Thanks,

Neil Conway

Review Request 59766: Changed allocator to offer remote resources to region-aware frameworks.

[Neil Conway]

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

Review request for mesos and Vinod Kone.


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


Repository: mesos


Description
---

Changed allocator to offer remote resources to region-aware frameworks.


Diffs
-

  include/mesos/allocator/allocator.hpp 
b31fbcfc18cbbe1e6b5fb5ccbc234f790326a5b4 
  src/master/allocator/mesos/allocator.hpp 
b2dcb566c49a1bbb1d955ca46e1c8eef91e62733 
  src/master/allocator/mesos/hierarchical.hpp 
5e7c3068061012c51d4b9220dedf476408016a12 
  src/master/allocator/mesos/hierarchical.cpp 
8ebdbc6a2b98feab7ce4d7f07b15d8fb92992270 
  src/master/master.cpp c66907bd55cb2eb549ec89f048d41376df556eb9 
  src/tests/allocator.hpp 0b9fdeaccc65bb6988103ac156e0584cf5df17fe 
  src/tests/api_tests.cpp 91b3473452b8e65cab9f2e873837d64a0edf4b54 
  src/tests/master_allocator_tests.cpp 0082045ccaebe015725c5e178aa89dac32b1c50c 
  src/tests/master_quota_tests.cpp 6ca45706f47b7d75db519f98af64e6a52091eb84 
  src/tests/master_tests.cpp 490d7ed4b275ebf5ff6956f7d40dbea3ce3b63e2 
  src/tests/persistent_volume_endpoints_tests.cpp 
9b820b8f4e0d0e2c62b5c9b420b790cdfb59dd0c 
  src/tests/reservation_endpoints_tests.cpp 
505c5421e95378177a7a09f462e5625ffa75cd37 
  src/tests/reservation_tests.cpp 34628eed671aee7ed69497d282c8043d65620c14 
  src/tests/resource_offers_tests.cpp 7ad037e5ec1d63cc8c2b8b0ad092e3f5f6f402c8 
  src/tests/slave_recovery_tests.cpp 86cf971aa8aea012a74740a4dcbaf5ed2ad9c866 


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


Testing
---

`make check`.


Thanks,

Neil Conway

Re: Review Request 59766: Changed allocator to offer remote resources to region-aware frameworks.

[Mesos Reviewbot Windows]

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



Bad patch!

Reviews applied: [59766, 59765, 59764, 59763, 59762, 59761, 59760, 59759]

Failed command: python support/apply-reviews.py -n -r 59760

Error:
error: patch failed: include/mesos/mesos.proto:343
error: include/mesos/mesos.proto: patch does not apply
error: patch failed: include/mesos/v1/mesos.proto:343
error: include/mesos/v1/mesos.proto: patch does not apply

Full log: http://mesos-winbot.westus.cloudapp.azure.com/logs/console

- Mesos Reviewbot Windows


On June 2, 2017, 8:13 p.m., Neil Conway wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/59766/
> ---
> 
> (Updated June 2, 2017, 8:13 p.m.)
> 
> 
> Review request for mesos and Vinod Kone.
> 
> 
> Bugs: MESOS-7614
> https://issues.apache.org/jira/browse/MESOS-7614
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Changed allocator to offer remote resources to region-aware frameworks.
> 
> 
> Diffs
> -
> 
>   include/mesos/allocator/allocator.hpp 
> b31fbcfc18cbbe1e6b5fb5ccbc234f790326a5b4 
>   src/master/allocator/mesos/allocator.hpp 
> b2dcb566c49a1bbb1d955ca46e1c8eef91e62733 
>   src/master/allocator/mesos/hierarchical.hpp 
> 5e7c3068061012c51d4b9220dedf476408016a12 
>   src/master/allocator/mesos/hierarchical.cpp 
> 8ebdbc6a2b98feab7ce4d7f07b15d8fb92992270 
>   src/master/master.cpp c66907bd55cb2eb549ec89f048d41376df556eb9 
>   src/tests/allocator.hpp 0b9fdeaccc65bb6988103ac156e0584cf5df17fe 
>   src/tests/api_tests.cpp 91b3473452b8e65cab9f2e873837d64a0edf4b54 
>   src/tests/master_allocator_tests.cpp 
> 0082045ccaebe015725c5e178aa89dac32b1c50c 
>   src/tests/master_quota_tests.cpp 6ca45706f47b7d75db519f98af64e6a52091eb84 
>   src/tests/master_tests.cpp 490d7ed4b275ebf5ff6956f7d40dbea3ce3b63e2 
>   src/tests/persistent_volume_endpoints_tests.cpp 
> 9b820b8f4e0d0e2c62b5c9b420b790cdfb59dd0c 
>   src/tests/reservation_endpoints_tests.cpp 
> 505c5421e95378177a7a09f462e5625ffa75cd37 
>   src/tests/reservation_tests.cpp 34628eed671aee7ed69497d282c8043d65620c14 
>   src/tests/resource_offers_tests.cpp 
> 7ad037e5ec1d63cc8c2b8b0ad092e3f5f6f402c8 
>   src/tests/slave_recovery_tests.cpp 86cf971aa8aea012a74740a4dcbaf5ed2ad9c866 
> 
> 
> Diff: https://reviews.apache.org/r/59766/diff/1/
> 
> 
> Testing
> ---
> 
> `make check`.
> 
> 
> Thanks,
> 
> Neil Conway
> 
>

Re: Review Request 57817: Offers not sent for suppressed roles as indicated in `SUBSCRIBE`.

[Anindya Sinha]

> On June 2, 2017, 7:51 p.m., Vinod Kone wrote:
> > src/master/allocator/mesos/hierarchical.cpp
> > Lines 430-431 (patched)
> > 
> >
> > what about roles that are suppressed in old state and new state?

If a specific role was suppressed before, and is also supressed now, then that 
role is already deactivated. I do not think we need to deactivate them again. 
Similarly, if a role is non-suppressed before and after, we do not need to 
activate them since they are already activated. Or am I missing something?


> On June 2, 2017, 7:51 p.m., Vinod Kone wrote:
> > src/master/allocator/mesos/hierarchical.cpp
> > Lines 463 (patched)
> > 
> >
> > what about new roles that are also suppressed?

We do not activate new roles that are suppressed. We only activate roles if 
they are not suppressed. That is handled here:

```
if (!suppressedRoles.count(role)) {
  frameworkSorters.at(role)->activate(frameworkId.value());
}
```


- Anindya


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


On June 2, 2017, 5:49 p.m., Anindya Sinha wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/57817/
> ---
> 
> (Updated June 2, 2017, 5:49 p.m.)
> 
> 
> Review request for mesos, James Peach, Vinod Kone, and Jiang Yan Xu.
> 
> 
> Bugs: MESOS-7015
> https://issues.apache.org/jira/browse/MESOS-7015
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> If the `SUBSCRIBE` indicates a subset of roles to be suppressed during
> framework (re)registration, the allocator does not offer resources for
> those roles to such frameworks. Note that this functionality is added
> for `v1::SUBSCRIBE` only (and not for scheduler driver API).
> 
> In addition, the master validates the suppressed roles to be a subset
> of all the roles being (re)registered by the framework.
> 
> 
> Diffs
> -
> 
>   include/mesos/allocator/allocator.hpp 
> b31fbcfc18cbbe1e6b5fb5ccbc234f790326a5b4 
>   src/master/allocator/mesos/allocator.hpp 
> b2dcb566c49a1bbb1d955ca46e1c8eef91e62733 
>   src/master/allocator/mesos/hierarchical.hpp 
> 5e7c3068061012c51d4b9220dedf476408016a12 
>   src/master/allocator/mesos/hierarchical.cpp 
> 8ebdbc6a2b98feab7ce4d7f07b15d8fb92992270 
>   src/master/master.hpp 89d0790fd5fea59e74276f462581fe0073594732 
>   src/master/master.cpp c66907bd55cb2eb549ec89f048d41376df556eb9 
>   src/tests/allocator.hpp 0b9fdeaccc65bb6988103ac156e0584cf5df17fe 
>   src/tests/hierarchical_allocator_tests.cpp 
> eb2b647d3247a85e8b9b82e5589232c74ad8570f 
>   src/tests/master_allocator_tests.cpp 
> 0082045ccaebe015725c5e178aa89dac32b1c50c 
>   src/tests/persistent_volume_endpoints_tests.cpp 
> 9b820b8f4e0d0e2c62b5c9b420b790cdfb59dd0c 
>   src/tests/reservation_tests.cpp 34628eed671aee7ed69497d282c8043d65620c14 
>   src/tests/resource_offers_tests.cpp 
> 7ad037e5ec1d63cc8c2b8b0ad092e3f5f6f402c8 
>   src/tests/slave_recovery_tests.cpp 86cf971aa8aea012a74740a4dcbaf5ed2ad9c866 
> 
> 
> Diff: https://reviews.apache.org/r/57817/diff/13/
> 
> 
> Testing
> ---
> 
> All tests passed.
> 
> 
> Thanks,
> 
> Anindya Sinha
> 
>

Re: Review Request 57817: Offers not sent for suppressed roles as indicated in `SUBSCRIBE`.

[Anindya Sinha]

> On June 1, 2017, 5:41 p.m., Benjamin Mahler wrote:
> > src/master/allocator/mesos/hierarchical.cpp
> > Lines 449-450 (original), 479-481 (patched)
> > 
> >
> > Not your bug, but it seems wrong to be activating roles here if the 
> > framework was deactivated. Would be good to sync with mpark on this 
> > original change. It seems to me that if the framework is deactivated via 
> > deactivateFramework, we wouldn't want to be activating things here in 
> > updateFrameowrk.
> 
> Anindya Sinha wrote:
> Reached out to @mpark. I kept it as-is till he has a chance to look into 
> it.

So indeed, we need to activate roles if the framework is not deactivated. Can 
we push that fix in a separate JIRA/RR once this one is merged?


- Anindya


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


On June 2, 2017, 5:49 p.m., Anindya Sinha wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/57817/
> ---
> 
> (Updated June 2, 2017, 5:49 p.m.)
> 
> 
> Review request for mesos, James Peach, Vinod Kone, and Jiang Yan Xu.
> 
> 
> Bugs: MESOS-7015
> https://issues.apache.org/jira/browse/MESOS-7015
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> If the `SUBSCRIBE` indicates a subset of roles to be suppressed during
> framework (re)registration, the allocator does not offer resources for
> those roles to such frameworks. Note that this functionality is added
> for `v1::SUBSCRIBE` only (and not for scheduler driver API).
> 
> In addition, the master validates the suppressed roles to be a subset
> of all the roles being (re)registered by the framework.
> 
> 
> Diffs
> -
> 
>   include/mesos/allocator/allocator.hpp 
> b31fbcfc18cbbe1e6b5fb5ccbc234f790326a5b4 
>   src/master/allocator/mesos/allocator.hpp 
> b2dcb566c49a1bbb1d955ca46e1c8eef91e62733 
>   src/master/allocator/mesos/hierarchical.hpp 
> 5e7c3068061012c51d4b9220dedf476408016a12 
>   src/master/allocator/mesos/hierarchical.cpp 
> 8ebdbc6a2b98feab7ce4d7f07b15d8fb92992270 
>   src/master/master.hpp 89d0790fd5fea59e74276f462581fe0073594732 
>   src/master/master.cpp c66907bd55cb2eb549ec89f048d41376df556eb9 
>   src/tests/allocator.hpp 0b9fdeaccc65bb6988103ac156e0584cf5df17fe 
>   src/tests/hierarchical_allocator_tests.cpp 
> eb2b647d3247a85e8b9b82e5589232c74ad8570f 
>   src/tests/master_allocator_tests.cpp 
> 0082045ccaebe015725c5e178aa89dac32b1c50c 
>   src/tests/persistent_volume_endpoints_tests.cpp 
> 9b820b8f4e0d0e2c62b5c9b420b790cdfb59dd0c 
>   src/tests/reservation_tests.cpp 34628eed671aee7ed69497d282c8043d65620c14 
>   src/tests/resource_offers_tests.cpp 
> 7ad037e5ec1d63cc8c2b8b0ad092e3f5f6f402c8 
>   src/tests/slave_recovery_tests.cpp 86cf971aa8aea012a74740a4dcbaf5ed2ad9c866 
> 
> 
> Diff: https://reviews.apache.org/r/57817/diff/13/
> 
> 
> Testing
> ---
> 
> All tests passed.
> 
> 
> Thanks,
> 
> Anindya Sinha
> 
>

Re: Review Request 59641: Follow symlinks when resolving paths specified in the various master/agent endpoints.

[Aaron Wood via Review Board]

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

(Updated June 2, 2017, 8:59 p.m.)


Review request for mesos, Andrew Schwartzmeyer, James Peach, Vinod Kone, and 
Zhitao Li.


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


Repository: mesos


Description
---

The main benefit of following symlinks in endpoints such as `/files` is that 
frameworks will be able to construct a path to the sandbox much easier. This 
will assist framework developers in making features that need to provide a path 
when hitting various operator API endpoints. Currently, making use of a path 
ending in `runs/latest` throws a 404.

One such application could be a scheduler providing the ability for users to 
work with their task's sandbox directly without going to the Mesos UI, API 
endpoints, or the actual system themselves.


Diffs (updated)
-

  src/slave/slave.cpp 0c7e5f4ef 
  src/tests/gc_tests.cpp b8fa6a4b9 


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

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


Testing
---

`mkdir build && cd build && cmake .. && make -j2 && make check -j2`

Checked the original behavior: 
```
curl -i 
localhost:5051/files/browse?path=/tmp/slave/slaves/d3566e51-4d13-4ed1-b66b-d538d3d7ef28-S0/frameworks/d3566e51-4d13-4ed1-b66b-d538d3d7ef28-0001/executors/testapp-924b5c1524d84676a6a71665e2054d31/runs/cbec99c9-da2d-4aaa-bab7-4cee9a4df031
HTTP/1.1 200 OK
Date: Tue, 30 May 2017 17:43:08 GMT
Content-Length: 644
Content-Type: application/json

[{"gid":"root","mode":"-rw-r--r--","mtime":1496161813.0,"nlink":1,"path":"\/tmp\/slave\/slaves\/d3566e51-4d13-4ed1-b66b-d538d3d7ef28-S0\/frameworks\/d3566e51-4d13-4ed1-b66b-d538d3d7ef28-0001\/executors\/testapp-924b5c1524d84676a6a71665e2054d31\/runs\/cbec99c9-da2d-4aaa-bab7-4cee9a4df031\/stderr","size":255,"uid":"root"},{"gid":"root","mode":"-rw-r--r--","mtime":1496161812.0,"nlink":1,"path":"\/tmp\/slave\/slaves\/d3566e51-4d13-4ed1-b66b-d538d3d7ef28-S0\/frameworks\/d3566e51-4d13-4ed1-b66b-d538d3d7ef28-0001\/executors\/testapp-924b5c1524d84676a6a71665e2054d31\/runs\/cbec99c9-da2d-4aaa-bab7-4cee9a4df031\/stdout","size":1142,"uid":"root"}]
```

Checked the new behavior (this would return a 404 before this patch):
```
curl -i 
localhost:5051/files/browse?path=/tmp/slave/slaves/d3566e51-4d13-4ed1-b66b-d538d3d7ef28-S0/frameworks/d3566e51-4d13-4eors/testapp-924b5c1524d84676a6a71665e2054d31/runs/latest
HTTP/1.1 200 OK
Date: Tue, 30 May 2017 17:43:13 GMT
Content-Length: 584
Content-Type: application/json

[{"gid":"root","mode":"-rw-r--r--","mtime":1496161813.0,"nlink":1,"path":"\/tmp\/slave\/slaves\/d3566e51-4d13-4ed1-b66b-d538d3d7ef28-S0\/frameworks\/d3566e51-4d13-4ed1-b66b-d538d3d7ef28-0001\/executors\/testapp-924b5c1524d84676a6a71665e2054d31\/runs\/latest\/stderr","size":255,"uid":"root"},{"gid":"root","mode":"-rw-r--r--","mtime":1496161812.0,"nlink":1,"path":"\/tmp\/slave\/slaves\/d3566e51-4d13-4ed1-b66b-d538d3d7ef28-S0\/frameworks\/d3566e51-4d13-4ed1-b66b-d538d3d7ef28-0001\/executors\/testapp-924b5c1524d84676a6a71665e2054d31\/runs\/latest\/stdout","size":1142,"uid":"root"}]
```


Thanks,

Aaron Wood

Re: Review Request 57817: Offers not sent for suppressed roles as indicated in `SUBSCRIBE`.

[Anindya Sinha]

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

(Updated June 2, 2017, 9:29 p.m.)


Review request for mesos, James Peach, Vinod Kone, and Jiang Yan Xu.


Changes
---

Addressed a comment.


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


Repository: mesos


Description
---

If the `SUBSCRIBE` indicates a subset of roles to be suppressed during
framework (re)registration, the allocator does not offer resources for
those roles to such frameworks. Note that this functionality is added
for `v1::SUBSCRIBE` only (and not for scheduler driver API).

In addition, the master validates the suppressed roles to be a subset
of all the roles being (re)registered by the framework.


Diffs (updated)
-

  include/mesos/allocator/allocator.hpp 
b31fbcfc18cbbe1e6b5fb5ccbc234f790326a5b4 
  src/master/allocator/mesos/allocator.hpp 
b2dcb566c49a1bbb1d955ca46e1c8eef91e62733 
  src/master/allocator/mesos/hierarchical.hpp 
5e7c3068061012c51d4b9220dedf476408016a12 
  src/master/allocator/mesos/hierarchical.cpp 
8ebdbc6a2b98feab7ce4d7f07b15d8fb92992270 
  src/master/master.hpp 89d0790fd5fea59e74276f462581fe0073594732 
  src/master/master.cpp c66907bd55cb2eb549ec89f048d41376df556eb9 
  src/tests/allocator.hpp 0b9fdeaccc65bb6988103ac156e0584cf5df17fe 
  src/tests/hierarchical_allocator_tests.cpp 
eb2b647d3247a85e8b9b82e5589232c74ad8570f 
  src/tests/master_allocator_tests.cpp 0082045ccaebe015725c5e178aa89dac32b1c50c 
  src/tests/persistent_volume_endpoints_tests.cpp 
9b820b8f4e0d0e2c62b5c9b420b790cdfb59dd0c 
  src/tests/reservation_tests.cpp 34628eed671aee7ed69497d282c8043d65620c14 
  src/tests/resource_offers_tests.cpp 7ad037e5ec1d63cc8c2b8b0ad092e3f5f6f402c8 
  src/tests/slave_recovery_tests.cpp 86cf971aa8aea012a74740a4dcbaf5ed2ad9c866 


Diff: https://reviews.apache.org/r/57817/diff/14/

Changes: https://reviews.apache.org/r/57817/diff/13-14/


Testing
---

All tests passed.


Thanks,

Anindya Sinha

Re: Review Request 57817: Offers not sent for suppressed roles as indicated in `SUBSCRIBE`.

[Anindya Sinha]

> On June 2, 2017, 7:51 p.m., Vinod Kone wrote:
> > src/master/allocator/mesos/hierarchical.cpp
> > Lines 463 (patched)
> > 
> >
> > what about new roles that are also suppressed?
> 
> Anindya Sinha wrote:
> We do not activate new roles that are suppressed. We only activate roles 
> if they are not suppressed. That is handled here:
> 
> ```
> if (!suppressedRoles.count(role)) {
>   frameworkSorters.at(role)->activate(frameworkId.value());
> }
> ```

Consolidated this within `rolesToActivate`.


- Anindya


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


On June 2, 2017, 9:29 p.m., Anindya Sinha wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/57817/
> ---
> 
> (Updated June 2, 2017, 9:29 p.m.)
> 
> 
> Review request for mesos, James Peach, Vinod Kone, and Jiang Yan Xu.
> 
> 
> Bugs: MESOS-7015
> https://issues.apache.org/jira/browse/MESOS-7015
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> If the `SUBSCRIBE` indicates a subset of roles to be suppressed during
> framework (re)registration, the allocator does not offer resources for
> those roles to such frameworks. Note that this functionality is added
> for `v1::SUBSCRIBE` only (and not for scheduler driver API).
> 
> In addition, the master validates the suppressed roles to be a subset
> of all the roles being (re)registered by the framework.
> 
> 
> Diffs
> -
> 
>   include/mesos/allocator/allocator.hpp 
> b31fbcfc18cbbe1e6b5fb5ccbc234f790326a5b4 
>   src/master/allocator/mesos/allocator.hpp 
> b2dcb566c49a1bbb1d955ca46e1c8eef91e62733 
>   src/master/allocator/mesos/hierarchical.hpp 
> 5e7c3068061012c51d4b9220dedf476408016a12 
>   src/master/allocator/mesos/hierarchical.cpp 
> 8ebdbc6a2b98feab7ce4d7f07b15d8fb92992270 
>   src/master/master.hpp 89d0790fd5fea59e74276f462581fe0073594732 
>   src/master/master.cpp c66907bd55cb2eb549ec89f048d41376df556eb9 
>   src/tests/allocator.hpp 0b9fdeaccc65bb6988103ac156e0584cf5df17fe 
>   src/tests/hierarchical_allocator_tests.cpp 
> eb2b647d3247a85e8b9b82e5589232c74ad8570f 
>   src/tests/master_allocator_tests.cpp 
> 0082045ccaebe015725c5e178aa89dac32b1c50c 
>   src/tests/persistent_volume_endpoints_tests.cpp 
> 9b820b8f4e0d0e2c62b5c9b420b790cdfb59dd0c 
>   src/tests/reservation_tests.cpp 34628eed671aee7ed69497d282c8043d65620c14 
>   src/tests/resource_offers_tests.cpp 
> 7ad037e5ec1d63cc8c2b8b0ad092e3f5f6f402c8 
>   src/tests/slave_recovery_tests.cpp 86cf971aa8aea012a74740a4dcbaf5ed2ad9c866 
> 
> 
> Diff: https://reviews.apache.org/r/57817/diff/14/
> 
> 
> Testing
> ---
> 
> All tests passed.
> 
> 
> Thanks,
> 
> Anindya Sinha
> 
>

Re: Review Request 57817: Offers not sent for suppressed roles as indicated in `SUBSCRIBE`.

[Anindya Sinha]

- Anindya


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


On June 2, 2017, 9:29 p.m., Anindya Sinha wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/57817/
> ---
> 
> (Updated June 2, 2017, 9:29 p.m.)
> 
> 
> Review request for mesos, James Peach, Vinod Kone, and Jiang Yan Xu.
> 
> 
> Bugs: MESOS-7015
> https://issues.apache.org/jira/browse/MESOS-7015
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> If the `SUBSCRIBE` indicates a subset of roles to be suppressed during
> framework (re)registration, the allocator does not offer resources for
> those roles to such frameworks. Note that this functionality is added
> for `v1::SUBSCRIBE` only (and not for scheduler driver API).
> 
> In addition, the master validates the suppressed roles to be a subset
> of all the roles being (re)registered by the framework.
> 
> 
> Diffs
> -
> 
>   include/mesos/allocator/allocator.hpp 
> b31fbcfc18cbbe1e6b5fb5ccbc234f790326a5b4 
>   src/master/allocator/mesos/allocator.hpp 
> b2dcb566c49a1bbb1d955ca46e1c8eef91e62733 
>   src/master/allocator/mesos/hierarchical.hpp 
> 5e7c3068061012c51d4b9220dedf476408016a12 
>   src/master/allocator/mesos/hierarchical.cpp 
> 8ebdbc6a2b98feab7ce4d7f07b15d8fb92992270 
>   src/master/master.hpp 89d0790fd5fea59e74276f462581fe0073594732 
>   src/master/master.cpp c66907bd55cb2eb549ec89f048d41376df556eb9 
>   src/tests/allocator.hpp 0b9fdeaccc65bb6988103ac156e0584cf5df17fe 
>   src/tests/hierarchical_allocator_tests.cpp 
> eb2b647d3247a85e8b9b82e5589232c74ad8570f 
>   src/tests/master_allocator_tests.cpp 
> 0082045ccaebe015725c5e178aa89dac32b1c50c 
>   src/tests/persistent_volume_endpoints_tests.cpp 
> 9b820b8f4e0d0e2c62b5c9b420b790cdfb59dd0c 
>   src/tests/reservation_tests.cpp 34628eed671aee7ed69497d282c8043d65620c14 
>   src/tests/resource_offers_tests.cpp 
> 7ad037e5ec1d63cc8c2b8b0ad092e3f5f6f402c8 
>   src/tests/slave_recovery_tests.cpp 86cf971aa8aea012a74740a4dcbaf5ed2ad9c866 
> 
> 
> Diff: https://reviews.apache.org/r/57817/diff/14/
> 
> 
> Testing
> ---
> 
> All tests passed.
> 
> 
> Thanks,
> 
> Anindya Sinha
> 
>

Re: Review Request 57818: Added unit tests to verify offers are suppressed based on registration.

[Anindya Sinha]

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

(Updated June 2, 2017, 9:29 p.m.)


Review request for mesos, James Peach, Vinod Kone, and Jiang Yan Xu.


Changes
---

rebased.


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


Repository: mesos


Description
---

Added unit tests to verify offers are suppressed based on registration.


Diffs (updated)
-

  src/tests/scheduler_tests.cpp d23a393a8123b7e1a0d613dec225daabe3694169 


Diff: https://reviews.apache.org/r/57818/diff/14/

Changes: https://reviews.apache.org/r/57818/diff/13-14/


Testing
---

All tests passed.


Thanks,

Anindya Sinha

Re: Review Request 59641: Follow symlinks when resolving paths specified in the various master/agent endpoints.

[Mesos Reviewbot Windows]

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



Patch looks great!

Reviews applied: [59641]

Passed command: support\windows-build.bat

- Mesos Reviewbot Windows


On June 2, 2017, 8:59 p.m., Aaron Wood wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/59641/
> ---
> 
> (Updated June 2, 2017, 8:59 p.m.)
> 
> 
> Review request for mesos, Andrew Schwartzmeyer, James Peach, Vinod Kone, and 
> Zhitao Li.
> 
> 
> Bugs: MESOS-7572
> https://issues.apache.org/jira/browse/MESOS-7572
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> The main benefit of following symlinks in endpoints such as `/files` is that 
> frameworks will be able to construct a path to the sandbox much easier. This 
> will assist framework developers in making features that need to provide a 
> path when hitting various operator API endpoints. Currently, making use of a 
> path ending in `runs/latest` throws a 404.
> 
> One such application could be a scheduler providing the ability for users to 
> work with their task's sandbox directly without going to the Mesos UI, API 
> endpoints, or the actual system themselves.
> 
> 
> Diffs
> -
> 
>   src/slave/slave.cpp 0c7e5f4ef 
>   src/tests/gc_tests.cpp b8fa6a4b9 
> 
> 
> Diff: https://reviews.apache.org/r/59641/diff/6/
> 
> 
> Testing
> ---
> 
> `mkdir build && cd build && cmake .. && make -j2 && make check -j2`
> 
> Checked the original behavior: 
> ```
> curl -i 
> localhost:5051/files/browse?path=/tmp/slave/slaves/d3566e51-4d13-4ed1-b66b-d538d3d7ef28-S0/frameworks/d3566e51-4d13-4ed1-b66b-d538d3d7ef28-0001/executors/testapp-924b5c1524d84676a6a71665e2054d31/runs/cbec99c9-da2d-4aaa-bab7-4cee9a4df031
> HTTP/1.1 200 OK
> Date: Tue, 30 May 2017 17:43:08 GMT
> Content-Length: 644
> Content-Type: application/json
> 
> [{"gid":"root","mode":"-rw-r--r--","mtime":1496161813.0,"nlink":1,"path":"\/tmp\/slave\/slaves\/d3566e51-4d13-4ed1-b66b-d538d3d7ef28-S0\/frameworks\/d3566e51-4d13-4ed1-b66b-d538d3d7ef28-0001\/executors\/testapp-924b5c1524d84676a6a71665e2054d31\/runs\/cbec99c9-da2d-4aaa-bab7-4cee9a4df031\/stderr","size":255,"uid":"root"},{"gid":"root","mode":"-rw-r--r--","mtime":1496161812.0,"nlink":1,"path":"\/tmp\/slave\/slaves\/d3566e51-4d13-4ed1-b66b-d538d3d7ef28-S0\/frameworks\/d3566e51-4d13-4ed1-b66b-d538d3d7ef28-0001\/executors\/testapp-924b5c1524d84676a6a71665e2054d31\/runs\/cbec99c9-da2d-4aaa-bab7-4cee9a4df031\/stdout","size":1142,"uid":"root"}]
> ```
> 
> Checked the new behavior (this would return a 404 before this patch):
> ```
> curl -i 
> localhost:5051/files/browse?path=/tmp/slave/slaves/d3566e51-4d13-4ed1-b66b-d538d3d7ef28-S0/frameworks/d3566e51-4d13-4eors/testapp-924b5c1524d84676a6a71665e2054d31/runs/latest
> HTTP/1.1 200 OK
> Date: Tue, 30 May 2017 17:43:13 GMT
> Content-Length: 584
> Content-Type: application/json
> 
> [{"gid":"root","mode":"-rw-r--r--","mtime":1496161813.0,"nlink":1,"path":"\/tmp\/slave\/slaves\/d3566e51-4d13-4ed1-b66b-d538d3d7ef28-S0\/frameworks\/d3566e51-4d13-4ed1-b66b-d538d3d7ef28-0001\/executors\/testapp-924b5c1524d84676a6a71665e2054d31\/runs\/latest\/stderr","size":255,"uid":"root"},{"gid":"root","mode":"-rw-r--r--","mtime":1496161812.0,"nlink":1,"path":"\/tmp\/slave\/slaves\/d3566e51-4d13-4ed1-b66b-d538d3d7ef28-S0\/frameworks\/d3566e51-4d13-4ed1-b66b-d538d3d7ef28-0001\/executors\/testapp-924b5c1524d84676a6a71665e2054d31\/runs\/latest\/stdout","size":1142,"uid":"root"}]
> ```
> 
> 
> Thanks,
> 
> Aaron Wood
> 
>

Re: Review Request 57818: Added unit tests to verify offers are suppressed based on registration.

[Mesos Reviewbot Windows]

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



Patch looks great!

Reviews applied: [57815, 57817, 57818]

Passed command: support\windows-build.bat

- Mesos Reviewbot Windows


On June 2, 2017, 9:29 p.m., Anindya Sinha wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/57818/
> ---
> 
> (Updated June 2, 2017, 9:29 p.m.)
> 
> 
> Review request for mesos, James Peach, Vinod Kone, and Jiang Yan Xu.
> 
> 
> Bugs: MESOS-7015
> https://issues.apache.org/jira/browse/MESOS-7015
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added unit tests to verify offers are suppressed based on registration.
> 
> 
> Diffs
> -
> 
>   src/tests/scheduler_tests.cpp d23a393a8123b7e1a0d613dec225daabe3694169 
> 
> 
> Diff: https://reviews.apache.org/r/57818/diff/14/
> 
> 
> Testing
> ---
> 
> All tests passed.
> 
> 
> Thanks,
> 
> Anindya Sinha
> 
>

Re: Review Request 59749: Removed out-dated working groups.

[Gilbert Song]

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


Ship it!




Ship It!

- Gilbert Song


On June 2, 2017, 9:13 a.m., Jie Yu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/59749/
> ---
> 
> (Updated June 2, 2017, 9:13 a.m.)
> 
> 
> Review request for mesos, Benjamin Mahler and Vinod Kone.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Removed out-dated working groups.
> 
> 
> Diffs
> -
> 
>   docs/working-groups.md b6a0b358a340658849e25646f5174e4c299734ff 
> 
> 
> Diff: https://reviews.apache.org/r/59749/diff/1/
> 
> 
> Testing
> ---
> 
> n/a
> 
> 
> Thanks,
> 
> Jie Yu
> 
>

Re: Review Request 59100: Enabled authorization for v1 API call GET_MAINTENANCE_SCHEDULE.

[Greg Mann]

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




src/tests/api_tests.cpp
Lines 1249 (patched)


What do you think about using just `schedule` instead of `respSchedule` 
here?


- Greg Mann


On June 2, 2017, 12:39 p.m., Alexander Rojas wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/59100/
> ---
> 
> (Updated June 2, 2017, 12:39 p.m.)
> 
> 
> Review request for mesos, Adam B, Greg Mann, and Till Toenshoff.
> 
> 
> Bugs: MESOS-7415
> https://issues.apache.org/jira/browse/MESOS-7415
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Enables the use of authorization for the `GET_MAINTENANCE_SCHEDULE`
> v1 API call, using the ACL `GetMaintenanceSchedule` and the action
> of the same name as the API call.
> 
> It also updates the ApiTests to take into account the authorization
> case.
> 
> 
> Diffs
> -
> 
>   src/master/http.cpp 1dcfe6ef00b0e3984deb79a511e665f638661323 
>   src/master/master.hpp 89d0790fd5fea59e74276f462581fe0073594732 
>   src/tests/api_tests.cpp 91b3473452b8e65cab9f2e873837d64a0edf4b54 
> 
> 
> Diff: https://reviews.apache.org/r/59100/diff/5/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Alexander Rojas
> 
>