Re: Review Request 67830: Moved `executor::Call` validation to common validation library.

[Mesos Reviewbot]

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



Patch looks great!

Reviews applied: [67830]

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

- Mesos Reviewbot


On July 5, 2018, 2:17 p.m., James Peach wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/67830/
> ---
> 
> (Updated July 5, 2018, 2:17 p.m.)
> 
> 
> Review request for mesos and Alexander Rukletsov.
> 
> 
> Bugs: MESOS-9051
> https://issues.apache.org/jira/browse/MESOS-9051
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> `executor::Call` validation is used by both the agent and the
> executor driver, so move it to the common validation library.
> 
> 
> Diffs
> -
> 
>   src/common/validation.hpp fa7f4a7164fda0f6af98c60a27e944146a231ee7 
>   src/common/validation.cpp f89b0ca886ef910d01cc0dba6e10505dc5e6ff6f 
>   src/executor/executor.cpp ab67caefd82f46eacd33221270b2c408ef70cd17 
>   src/slave/http.cpp aa9cdc5f58f73323958a6872e2721c83317f354c 
>   src/slave/validation.hpp 3a278e43f0e98c1ed6dcdec60e71c131973ccb43 
>   src/slave/validation.cpp 3e333a7b308ec9c42541036f52b48b4ad74f5b88 
> 
> 
> Diff: https://reviews.apache.org/r/67830/diff/1/
> 
> 
> Testing
> ---
> 
> make check (Fedora 28)
> 
> 
> Thanks,
> 
> James Peach
> 
>

Re: Review Request 67671: Prevented resource providers from changing their name or type.

[Chun-Hung Hsiao]

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




src/resource_provider/manager.cpp
Lines 104 (patched)


Let's do a `CHECK(resourceProviderInfo.has_id())` here. Otherwise this 
function may create an invalid 
`mesos::resource_provider::registry::ResourceProvider` since `id` is required.


- Chun-Hung Hsiao


On June 20, 2018, 12:35 p.m., Benjamin Bannier wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/67671/
> ---
> 
> (Updated June 20, 2018, 12:35 p.m.)
> 
> 
> Review request for mesos, Chun-Hung Hsiao and Jan Schlicht.
> 
> 
> Bugs: MESOS-8838
> https://issues.apache.org/jira/browse/MESOS-8838
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Since the agent uses e.g., a resource provider's name or type to
> construct paths to persist resource provider state, changes to this
> information on resource provider resubscription are not supported.
> 
> This patch persists a resource provider's name and type in the
> resource provider registry and rejects a resource provider
> resubscription if incompatible changes are detected. Since we did not
> persist this information previous to mesos-1.7.0 we cannot and do not
> perform validation against resource provider registry information
> stored with earlier versions of Mesos.
> 
> 
> Diffs
> -
> 
>   src/resource_provider/manager.cpp 6400e707ee834c73d4bf18c1f8d8344e8cf714cc 
>   src/resource_provider/registrar.hpp 
> ded56e1c24a1f8b8db8dc70151682a7deb9e6544 
>   src/resource_provider/registrar.cpp 
> a855a2b61fdbfc96a12a133c702ecaf7af666d8b 
>   src/resource_provider/registry.hpp 4c6c4d40686e3af8bbc7affbe3fa673479cc2fbf 
>   src/resource_provider/registry.proto 
> 491263ef679cd4cea59338b002c6845d890f8eae 
>   src/tests/resource_provider_manager_tests.cpp 
> 58bdbf0e88f59b5cb0cad42e38b24029fc0f2b41 
> 
> 
> Diff: https://reviews.apache.org/r/67671/diff/1/
> 
> 
> Testing
> ---
> 
> `sudo make check`
> 
> 
> Thanks,
> 
> Benjamin Bannier
> 
>

Re: Review Request 67671: Prevented resource providers from changing their name or type.

[Chun-Hung Hsiao]

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


Fix it, then Ship it!





src/resource_provider/registry.hpp
Lines 21 (patched)


This is not used.

I guess you probably has used this originally but changed the 
implementation to use explicit comparison, which seems better as the code 
doesn't rely on the semantics of the message differencer.



src/resource_provider/registry.hpp
Lines 38-50 (patched)


We could write the code in a style similar to `src/common/type_utils.cpp` 
is:
```
return left.id() == right.id() &&
  (!left.has_type() || !right.has_type() || left.type() == right.type()) &&
  (!left.has_name() || !right.has_name() || left.name() == right.name()));
```

But whether this is more readable or not is subjective so not opening an 
issue here.



src/resource_provider/registry.hpp
Lines 66 (patched)


Would `google::protobuf::util::MessageToJsonString` be better here?



src/resource_provider/registry.proto
Lines 32-33 (patched)


Let's swap the order:
```
  optional string type = 2;
  optional string name = 3;
```


- Chun-Hung Hsiao


On June 20, 2018, 12:35 p.m., Benjamin Bannier wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/67671/
> ---
> 
> (Updated June 20, 2018, 12:35 p.m.)
> 
> 
> Review request for mesos, Chun-Hung Hsiao and Jan Schlicht.
> 
> 
> Bugs: MESOS-8838
> https://issues.apache.org/jira/browse/MESOS-8838
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Since the agent uses e.g., a resource provider's name or type to
> construct paths to persist resource provider state, changes to this
> information on resource provider resubscription are not supported.
> 
> This patch persists a resource provider's name and type in the
> resource provider registry and rejects a resource provider
> resubscription if incompatible changes are detected. Since we did not
> persist this information previous to mesos-1.7.0 we cannot and do not
> perform validation against resource provider registry information
> stored with earlier versions of Mesos.
> 
> 
> Diffs
> -
> 
>   src/resource_provider/manager.cpp 6400e707ee834c73d4bf18c1f8d8344e8cf714cc 
>   src/resource_provider/registrar.hpp 
> ded56e1c24a1f8b8db8dc70151682a7deb9e6544 
>   src/resource_provider/registrar.cpp 
> a855a2b61fdbfc96a12a133c702ecaf7af666d8b 
>   src/resource_provider/registry.hpp 4c6c4d40686e3af8bbc7affbe3fa673479cc2fbf 
>   src/resource_provider/registry.proto 
> 491263ef679cd4cea59338b002c6845d890f8eae 
>   src/tests/resource_provider_manager_tests.cpp 
> 58bdbf0e88f59b5cb0cad42e38b24029fc0f2b41 
> 
> 
> Diff: https://reviews.apache.org/r/67671/diff/1/
> 
> 
> Testing
> ---
> 
> `sudo make check`
> 
> 
> Thanks,
> 
> Benjamin Bannier
> 
>

Re: Review Request 67821: Added override specifiers to virtual methods of several isolators.

[Mesos Reviewbot Windows]

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



FAIL: Some of the unit tests failed. Please check the relevant logs.

Reviews applied: `['67820', '67821']`

Failed command: `Start-MesosCITesting`

All the build artifacts available at: 
http://dcos-win.westus.cloudapp.azure.com/mesos-build/review/67821

Relevant logs:

- 
[mesos-tests-stdout.log](http://dcos-win.westus.cloudapp.azure.com/mesos-build/review/67821/logs/mesos-tests-stdout.log):

```
[ RUN  ] DockerContainerizerTest.ROOT_DOCKER_DestroyWhilePulling
[   OK ] DockerContainerizerTest.ROOT_DOCKER_DestroyWhilePulling (799 ms)
[ RUN  ] DockerContainerizerTest.ROOT_DOCKER_DestroyUnknownContainer
[   OK ] DockerContainerizerTest.ROOT_DOCKER_DestroyUnknownContainer (601 
ms)
[ RUN  ] DockerContainerizerTest.ROOT_DOCKER_ExecutorCleanupWhenLaunchFailed
[   OK ] 
DockerContainerizerTest.ROOT_DOCKER_ExecutorCleanupWhenLaunchFailed (1330 ms)
[ RUN  ] DockerContainerizerTest.ROOT_DOCKER_FetchFailure
[   OK ] DockerContainerizerTest.ROOT_DOCKER_FetchFailure (799 ms)
[ RUN  ] DockerContainerizerTest.ROOT_DOCKER_DockerPullFailure
[   OK ] DockerContainerizerTest.ROOT_DOCKER_DockerPullFailure (801 ms)
[ RUN  ] DockerContainerizerTest.ROOT_DOCKER_DockerInspectDiscard
[   OK ] DockerContainerizerTest.ROOT_DOCKER_DockerInspectDiscard (1007 ms)
[ RUN  ] DockerContainerizerTest.ROOT_DOCKER_WaitUnknownContainer
[   OK ] DockerContainerizerTest.ROOT_DOCKER_WaitUnknownContainer (597 ms)
[ RUN  ] 
DockerContainerizerTest.ROOT_DOCKER_NoTransitionFromKillingToRunning
[   OK ] 
DockerContainerizerTest.ROOT_DOCKER_NoTransitionFromKillingToRunning (5259 ms)
[ RUN  ] DockerContainerizerTest.ROOT_DOCKER_DefaultDNS
[   OK ] DockerContainerizerTest.ROOT_DOCKER_DefaultDNS (4870 ms)
[--] 24 tests from DockerContainerizerTest (90444 ms total)

[--] 1 test from HungDockerTest
[ RUN  ] HungDockerTest.ROOT_DOCKER_InspectHungDuringPull

d:\dcos\mesos\mesos\src\tests\mock_docker.hpp(155): ERROR: this mock object 
(used in test HungDockerTest.ROOT_DOCKER_InspectHungDuringPull) should be 
deleted but never is. Its address is @002A42FAB960.
d:\dcos\mesos\mesos\src\tests\containerizer\docker_containerizer_tests.cpp(5187):
 ERROR: this mock object (used in test 
HungDockerTest.ROOT_DOCKER_InspectHungDuringPull) should be deleted but never 
is. Its address is @002A42FABBC0.
d:\dcos\mesos\mesos\3rdparty\libprocess\include\process\gmock.hpp(235): ERROR: 
this mock object (used in test 
HungDockerTest.ROOT_DOCKER_InspectHungDuringPull) should be deleted but never 
is. Its address is @01D306F6DDF8.
d:\dcos\mesos\mesos\src\tests\mock_registrar.cpp(54): ERROR: this mock object 
(used in test HungDockerTest.ROOT_DOCKER_InspectHungDuringPull) should be 
deleted but never is. Its address is @01D3076DB5C0.
d:\dcos\mesos\mesos\src\tests\mock_docker.cpp(48): ERROR: this mock object 
(used in test HungDockerTest.ROOT_DOCKER_InspectHungDuringPull) should be 
deleted but never is. Its address is @01D3077A0E90.
ERROR: 5 leaked mock objects found at program exit.
```

- 
[mesos-tests-stderr.log](http://dcos-win.westus.cloudapp.azure.com/mesos-build/review/67821/logs/mesos-tests-stderr.log):

```
I0705 22:35:05.461047  7208 authenticatee.cpp:299] Authentication success
I0705 22:35:05.461047  7636 master.cpp:9802] Successfully authenticated 
principal 'test-principal' at 
scheduler-cdf15ae7-06b3-4f09-84ff-0ecc7c1ac3be@192.10.1.6:53166
I0705 22:35:05.462044 15080 sched.cpp:501] Successfully authenticated with 
master master@192.10.1.6:53166
I0705 22:35:05.463049  7208 master.cpp:2927] Received SUBSCRIBE call for 
framework 'default' at 
scheduler-cdf15ae7-06b3-4f09-84ff-0ecc7c1ac3be@192.10.1.6:53166
I0705 22:35:05.463049  7208 master.cpp:2234] Authorizing framework principal 
'test-principal' to receive offers for roles '{ * }'
I0705 22:35:05.464046 14408 master.cpp:3008] Subscribing framework default with 
checkpointing disabled and capabilities [ MULTI_ROLE, RESERVATION_REFINEMENT ]
I0705 22:35:05.465046 14408 master.cpp:9993] Adding framework 
20d79648-37eb-48df-8c8d-cf834b343c8b- (default) at 
scheduler-cdf15ae7-06b3-4f09-84ff-0ecc7c1ac3be@192.10.1.6:53166 with roles {  } 
suppressed
I0705 22:35:05.467057  7976 sched.cpp:749] Framework registered with 
20d79648-37eb-48df-8c8d-cf834b343c8b-
I0705 22:35:05.467057 11904 hierarchical.cpp:299] Added framework 
20d79648-37eb-48df-8c8d-cf834b343c8b-
E0705 22:35:05.562525 15012 slave.cpp:7289] EXIT with status 1: Failed to 
perform recovery: Collect failed: Failed to run 'C:\Program Files 
(x86)\Microsoft Visual Studio\2017\Community\VC\Auxiliary\Build\test-docker.bat 
-H npipe:./pipe/docker_engine ps -a': exited with status 1; 
stderr=''C:\Program' is not 

Re: Review Request 67820: Fixed recover() virtual method signature in several isolators.

[Benjamin Mahler]

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


Ship it!




Ship It!

- Benjamin Mahler


On July 5, 2018, 9:31 p.m., Ilya Pronin wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/67820/
> ---
> 
> (Updated July 5, 2018, 9:31 p.m.)
> 
> 
> Review request for mesos, Benjamin Mahler and James Peach.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Commit a52e5288ea94ec7d84f82c6c01c1ce3c7a3902db replaced std::list with
> std::vector in several places including the Isolator interface. However
> XfsDiskIsolatorProcess, PortMappingIsolatorProcess and
> NetworkPortsIsolatorProcess were not updated and because of that their
> recover() method became "disabled".
> 
> 
> Diffs
> -
> 
>   src/slave/containerizer/mesos/isolators/network/port_mapping.hpp 
> 6ff3820de6af544c6ec92b0e76fd934715b87a96 
>   src/slave/containerizer/mesos/isolators/network/port_mapping.cpp 
> 3f51542ca27217c1063c19be481e8c47481f1e0c 
>   src/slave/containerizer/mesos/isolators/network/ports.hpp 
> 8d467ceceae21d30318195785dce635d054d0f8d 
>   src/slave/containerizer/mesos/isolators/network/ports.cpp 
> 82c14413cef0ac14ccfbc700f8f10a6c120c7fb0 
>   src/slave/containerizer/mesos/isolators/xfs/disk.hpp 
> 9a5ca8bd60c61d65beed611a02dd26ed6a0a594b 
>   src/slave/containerizer/mesos/isolators/xfs/disk.cpp 
> 362996b804be59dff631566d4c9db2d94233e0c9 
> 
> 
> Diff: https://reviews.apache.org/r/67820/diff/2/
> 
> 
> Testing
> ---
> 
> Built Mesos with `--enable-xfs-disk-isolator` and ran `sudo make check`. 
> `ROOT_XFS_QuotaTest.NoCheckpointRecovery` test, that was broken because of 
> the issue, passed.
> 
> 
> Thanks,
> 
> Ilya Pronin
> 
>

Re: Review Request 67820: Fixed recover() virtual method signature in several isolators.

[Benjamin Mahler]

> On July 5, 2018, 8:33 p.m., Benjamin Mahler wrote:
> > Also, is there a ticket? Did any releases ship with this broken? i.e. Do we 
> > need to backport this?
> 
> Ilya Pronin wrote:
> No, should I create one? AFAIK the commit mentioned in the description 
> was not included in any existing release. I checked branches `1.5.x` and 
> `1.6.x` and didn't find it there.

Oh ok, I'm ok without one then. I mainly wanted one for tracking any affected 
releases / backporting.


- Benjamin


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


On July 5, 2018, 9:31 p.m., Ilya Pronin wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/67820/
> ---
> 
> (Updated July 5, 2018, 9:31 p.m.)
> 
> 
> Review request for mesos, Benjamin Mahler and James Peach.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Commit a52e5288ea94ec7d84f82c6c01c1ce3c7a3902db replaced std::list with
> std::vector in several places including the Isolator interface. However
> XfsDiskIsolatorProcess, PortMappingIsolatorProcess and
> NetworkPortsIsolatorProcess were not updated and because of that their
> recover() method became "disabled".
> 
> 
> Diffs
> -
> 
>   src/slave/containerizer/mesos/isolators/network/port_mapping.hpp 
> 6ff3820de6af544c6ec92b0e76fd934715b87a96 
>   src/slave/containerizer/mesos/isolators/network/port_mapping.cpp 
> 3f51542ca27217c1063c19be481e8c47481f1e0c 
>   src/slave/containerizer/mesos/isolators/network/ports.hpp 
> 8d467ceceae21d30318195785dce635d054d0f8d 
>   src/slave/containerizer/mesos/isolators/network/ports.cpp 
> 82c14413cef0ac14ccfbc700f8f10a6c120c7fb0 
>   src/slave/containerizer/mesos/isolators/xfs/disk.hpp 
> 9a5ca8bd60c61d65beed611a02dd26ed6a0a594b 
>   src/slave/containerizer/mesos/isolators/xfs/disk.cpp 
> 362996b804be59dff631566d4c9db2d94233e0c9 
> 
> 
> Diff: https://reviews.apache.org/r/67820/diff/2/
> 
> 
> Testing
> ---
> 
> Built Mesos with `--enable-xfs-disk-isolator` and ran `sudo make check`. 
> `ROOT_XFS_QuotaTest.NoCheckpointRecovery` test, that was broken because of 
> the issue, passed.
> 
> 
> Thanks,
> 
> Ilya Pronin
> 
>

Re: Review Request 67827: Added a helper `stripIncapableResources` in the allocator.

[Benjamin Mahler]

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



Looks good, the second allocation loop is starting to look pretty succinct!

Just a few questions about the logic below


src/master/allocator/mesos/hierarchical.hpp
Lines 687 (patched)


s/ based on the ..././

Or this might be clearer?

// Returns the subset of resources that the framework
// is capable of being offered.



src/master/allocator/mesos/hierarchical.cpp
Lines 1768-1772 (original), 1768-1770 (patched)


Hm.. do we need the if condition? Can't this just be:

```
available -= offeredSharedResources.get(slaveId).getOrElse(Resources());
```



src/master/allocator/mesos/hierarchical.cpp
Line 1930 (original), 1912-1914 (patched)


Hm.. this looks like a no-op change?



src/master/allocator/mesos/hierarchical.cpp
Lines 1998-2003 (original), 1982-1984 (patched)


Ditto here



src/master/allocator/mesos/hierarchical.cpp
Line 2083 (original), 2044-2046 (patched)


Ditto here



src/master/allocator/mesos/hierarchical.cpp
Lines 2660-2685 (patched)


Should we re-write this now to just consistently use filter()?

```
Resources HierarchicalAllocatorProcess::stripIncapableResources(
const Resources& resources, // Can take a const ref now?
const protobuf::framework::Capabilities& frameworkCapabilities) const
{
  return resources.filter([&](const Resource& resource) {
if (!frameworkCapabilities.sharedResources &&
Resources::isShared(resource)) {
  return false;
}

if (!frameworkCapabilities.revocableResources &&
Resources::isRevocable(resource)) {
  return false;
}

// When reservation refinements are present, old frameworks without the
// RESERVATION_REFINEMENT capability won't be able to understand the
// new format. While it's possible to translate the refined reservations
// into the old format by "hiding" the intermediate reservations in the
// "stack", this leads to ambiguity when processing RESERVE / UNRESERVE
// operations. This is due to the loss of information when we drop the
// intermediate reservations. Therefore, for now we simply filter out
// resources with refined reservations if the framework does not have
// the capability.
if (!frameworkCapabilities.reservationRefinement &&
Resources::hasRefinedReservations(resource)) {
  return false;
}

return true;
  });
}
```

I think this approach avoids adding extra copyies as we add more cases?


- Benjamin Mahler


On July 4, 2018, 1:01 a.m., Meng Zhu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/67827/
> ---
> 
> (Updated July 4, 2018, 1:01 a.m.)
> 
> 
> Review request for mesos and Benjamin Mahler.
> 
> 
> Bugs: MESOS-8916
> https://issues.apache.org/jira/browse/MESOS-8916
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This helper removes any resources that the framework is not
> capable of receiving based on the given framework capability.
> 
> 
> Diffs
> -
> 
>   src/master/allocator/mesos/hierarchical.hpp 
> 0f6c0e96a105c64465d3f5db4ff663d8fdfe7e26 
>   src/master/allocator/mesos/hierarchical.cpp 
> 5a6cd3d2fc5bdbaaee2d05b9be9e83d4107c749b 
> 
> 
> Diff: https://reviews.apache.org/r/67827/diff/1/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Meng Zhu
> 
>

Re: Review Request 67693: Added filtering for `GET_OPERATIONS` calls.

[Chun-Hung Hsiao]

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




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


Do we want to:
1. Simply skip such operations, as suggested in the code here, or
2. Return an `InternalServerError` eventually, or
3. Just `CHECK` that this shouldn't happen?
Ditto below.

Please feel free to drop this if you think skipping is good enough.



src/tests/api_tests.cpp
Lines 131-134 (original), 131-134 (patched)


How about making the same change as you did in `AgentAPITest::post`?
```
Future post(
const process::PID& pid,
const v1::master::Call& call,
const ContentType& contentType,
const Credential& credential = DEFAULT_CREDENTIAL)
```



src/tests/api_tests.cpp
Lines 1015-1020 (patched)


Do we need this?



src/tests/api_tests.cpp
Lines 1149-1167 (patched)


If you change the `MasterAPITest::post` helper, we can use it here:
```
v1Response = post(master.get()->pid, v1Call, contentType, 
DEFAULT_CREDENTIAL_2)
  .then(...);
```



src/tests/api_tests.cpp
Lines 7102-7107 (patched)


Do we need this?



src/tests/api_tests.cpp
Lines 7225-7243 (patched)


We can use the `AgentAPITest::post` helper here:
```
v1Response = post(agent.get()->pid, v1Call, contentType, 
DEFAULT_CREDENTIAL_2);
```


- Chun-Hung Hsiao


On June 21, 2018, 3:53 p.m., Benjamin Bannier wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/67693/
> ---
> 
> (Updated June 21, 2018, 3:53 p.m.)
> 
> 
> Review request for mesos, Chun-Hung Hsiao and Jan Schlicht.
> 
> 
> Bugs: MESOS-8473
> https://issues.apache.org/jira/browse/MESOS-8473
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This patch adds filtering of operations reported for `GET_OPERATIONS`
> calls. A principal is allowed to view see all operations for which it
> is allowed to see the role of the underlying resources.
> 
> 
> Diffs
> -
> 
>   src/master/http.cpp 0492b979e4657a489ca3428e6f8022ef20cb05f5 
>   src/slave/http.cpp aa9cdc5f58f73323958a6872e2721c83317f354c 
>   src/tests/api_tests.cpp 4d6b5b3e938faed934e875e23e29c67fd50b9d6f 
> 
> 
> Diff: https://reviews.apache.org/r/67693/diff/3/
> 
> 
> Testing
> ---
> 
> `make check`
> 
> The added tests fails without the corresponding changes to the master and 
> agent code.
> 
> 
> Thanks,
> 
> Benjamin Bannier
> 
>

Re: Review Request 67826: Made `Slave::getAvailable()` return all shared resources.

[Benjamin Mahler]

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



Can you add Yan Xu to this review?

This looks interesting, can you clarify a few things in the description / code?

* Why is this being done? (E.g. this is more accurately representing what's 
available? It helps remove the one off logic for shared resources in the 
allocation loops?)
* Shouldn't the summary / code document that we only consider a single copy of 
shared resources to be available at a given time? Meaning it takes 1 round per 
additional reference of the shared resource that gets allocated?
* Does this patch preserved correctness in a standalone way? Or does it need 
the subsequent patch to preserve the shared resources behavior?
* Are there any implications for metrics or endpoint information? I'm not sure 
how we currently represent shared resources in allocated/available metrics as 
well as allocated/available state json/protobuf, but it would be interesting to 
know if this affects any of that.

- Benjamin Mahler


On July 4, 2018, 12:57 a.m., Meng Zhu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/67826/
> ---
> 
> (Updated July 4, 2018, 12:57 a.m.)
> 
> 
> Review request for mesos and Benjamin Mahler.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Currently, depending on already allocated resources,
> `HierarchicalAllocatorProcess::Slave::getAvailable()`
> may not contain all the shared resources. Since shared
> resources are always allocatable, we should include all
> shared resources in the agent available resources.
> 
> 
> Diffs
> -
> 
>   src/master/allocator/mesos/hierarchical.hpp 
> 0f6c0e96a105c64465d3f5db4ff663d8fdfe7e26 
> 
> 
> Diff: https://reviews.apache.org/r/67826/diff/1/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Meng Zhu
> 
>

Re: Review Request 67825: Added a resources utility `hasShared()`.

[Benjamin Mahler]

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




include/mesos/resources.hpp
Lines 448-449 (patched)


This implies adding a lot of other has functions:

hasUnreserved
hasPersistentVolumes
hasRevocable
hasNonRevocable
hasNonShared
etc

Can we just use `shared().empty()` in place of adding this for now?


- Benjamin Mahler


On July 4, 2018, 12:57 a.m., Meng Zhu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/67825/
> ---
> 
> (Updated July 4, 2018, 12:57 a.m.)
> 
> 
> Review request for mesos and Benjamin Mahler.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> See summary.
> 
> 
> Diffs
> -
> 
>   include/mesos/resources.hpp 175833c7d08443955d0aacd844c7df180d43e829 
>   include/mesos/v1/resources.hpp b607b68fd905c09356288e8bc16081a7fe15e6ba 
>   src/common/resources.cpp 253b8bcd720e38f485b5cd2f5b7666ac85e67d38 
>   src/v1/resources.cpp ab8fc3e738038b9b34d4902aed9f15a59b416217 
> 
> 
> Diff: https://reviews.apache.org/r/67825/diff/1/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Meng Zhu
> 
>

Re: Review Request 67820: Fixed recover() virtual method signature in several isolators.

[Ilya Pronin]

> On July 5, 2018, 1:33 p.m., Benjamin Mahler wrote:
> > Also, is there a ticket? Did any releases ship with this broken? i.e. Do we 
> > need to backport this?

No, should I create one? AFAIK the commit mentioned in the description was not 
included in any existing release. I checked branches `1.5.x` and `1.6.x` and 
didn't find it there.


- Ilya


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


On July 5, 2018, 2:31 p.m., Ilya Pronin wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/67820/
> ---
> 
> (Updated July 5, 2018, 2:31 p.m.)
> 
> 
> Review request for mesos, Benjamin Mahler and James Peach.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Commit a52e5288ea94ec7d84f82c6c01c1ce3c7a3902db replaced std::list with
> std::vector in several places including the Isolator interface. However
> XfsDiskIsolatorProcess, PortMappingIsolatorProcess and
> NetworkPortsIsolatorProcess were not updated and because of that their
> recover() method became "disabled".
> 
> 
> Diffs
> -
> 
>   src/slave/containerizer/mesos/isolators/network/port_mapping.hpp 
> 6ff3820de6af544c6ec92b0e76fd934715b87a96 
>   src/slave/containerizer/mesos/isolators/network/port_mapping.cpp 
> 3f51542ca27217c1063c19be481e8c47481f1e0c 
>   src/slave/containerizer/mesos/isolators/network/ports.hpp 
> 8d467ceceae21d30318195785dce635d054d0f8d 
>   src/slave/containerizer/mesos/isolators/network/ports.cpp 
> 82c14413cef0ac14ccfbc700f8f10a6c120c7fb0 
>   src/slave/containerizer/mesos/isolators/xfs/disk.hpp 
> 9a5ca8bd60c61d65beed611a02dd26ed6a0a594b 
>   src/slave/containerizer/mesos/isolators/xfs/disk.cpp 
> 362996b804be59dff631566d4c9db2d94233e0c9 
> 
> 
> Diff: https://reviews.apache.org/r/67820/diff/2/
> 
> 
> Testing
> ---
> 
> Built Mesos with `--enable-xfs-disk-isolator` and ran `sudo make check`. 
> `ROOT_XFS_QuotaTest.NoCheckpointRecovery` test, that was broken because of 
> the issue, passed.
> 
> 
> Thanks,
> 
> Ilya Pronin
> 
>

Re: Review Request 67777: Added a helper to match agent-framework capabilities in the allocator.

[Benjamin Mahler]

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


Fix it, then Ship it!





src/master/allocator/mesos/hierarchical.hpp
Lines 663-664 (patched)


Can you add a TODO to consider pulling this up into the framework struct 
and what's needed to make that happen?



src/master/allocator/mesos/hierarchical.hpp
Lines 665 (patched)


How about `isCapableForAgent` or `isCapableOnAgent` or 
`isCapableOfReceivingAgent`?



src/master/allocator/mesos/hierarchical.hpp
Lines 666 (patched)


Can you just take the Framework struct here? Feels a little odd that we 
take framework capabilities but not agent capabilities (I realize we can't take 
agent capabilities only, but seems like the caller doesn't need to know which 
bits of framework and slave structs we need?)


- Benjamin Mahler


On July 4, 2018, 12:19 a.m., Meng Zhu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/6/
> ---
> 
> (Updated July 4, 2018, 12:19 a.m.)
> 
> 
> Review request for mesos and Benjamin Mahler.
> 
> 
> Bugs: MESOS-8916
> https://issues.apache.org/jira/browse/MESOS-8916
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> `isFrameworkCapableReceivingAgent` checks if a framework
> is capable of receiving resources on the agent based on
> the framework capability.
> 
> 
> Diffs
> -
> 
>   src/master/allocator/mesos/hierarchical.hpp 
> 0f6c0e96a105c64465d3f5db4ff663d8fdfe7e26 
>   src/master/allocator/mesos/hierarchical.cpp 
> 5a6cd3d2fc5bdbaaee2d05b9be9e83d4107c749b 
> 
> 
> Diff: https://reviews.apache.org/r/6/diff/3/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Meng Zhu
> 
>

Re: Review Request 67820: Fixed recover() virtual method signature in several isolators.

[Ilya Pronin]

> On July 4, 2018, 3:42 a.m., James Peach wrote:
> > In the commit message, I'd prefer to refer to the abbreviated git SHA1 
> > instead of the review ID. I just think that makes the changes slightly 
> > easier to track back.
> > 
> > Can you please also update the `port_mapping` and `ports` isolators?

Fixed the commit message and expanded the patch to include `port_mapping` and 
`ports` isolators.


- Ilya


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


On July 5, 2018, 2:31 p.m., Ilya Pronin wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/67820/
> ---
> 
> (Updated July 5, 2018, 2:31 p.m.)
> 
> 
> Review request for mesos, Benjamin Mahler and James Peach.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Commit a52e5288ea94ec7d84f82c6c01c1ce3c7a3902db replaced std::list with
> std::vector in several places including the Isolator interface. However
> XfsDiskIsolatorProcess, PortMappingIsolatorProcess and
> NetworkPortsIsolatorProcess were not updated and because of that their
> recover() method became "disabled".
> 
> 
> Diffs
> -
> 
>   src/slave/containerizer/mesos/isolators/network/port_mapping.hpp 
> 6ff3820de6af544c6ec92b0e76fd934715b87a96 
>   src/slave/containerizer/mesos/isolators/network/port_mapping.cpp 
> 3f51542ca27217c1063c19be481e8c47481f1e0c 
>   src/slave/containerizer/mesos/isolators/network/ports.hpp 
> 8d467ceceae21d30318195785dce635d054d0f8d 
>   src/slave/containerizer/mesos/isolators/network/ports.cpp 
> 82c14413cef0ac14ccfbc700f8f10a6c120c7fb0 
>   src/slave/containerizer/mesos/isolators/xfs/disk.hpp 
> 9a5ca8bd60c61d65beed611a02dd26ed6a0a594b 
>   src/slave/containerizer/mesos/isolators/xfs/disk.cpp 
> 362996b804be59dff631566d4c9db2d94233e0c9 
> 
> 
> Diff: https://reviews.apache.org/r/67820/diff/2/
> 
> 
> Testing
> ---
> 
> Built Mesos with `--enable-xfs-disk-isolator` and ran `sudo make check`. 
> `ROOT_XFS_QuotaTest.NoCheckpointRecovery` test, that was broken because of 
> the issue, passed.
> 
> 
> Thanks,
> 
> Ilya Pronin
> 
>

Re: Review Request 67821: Added override specifiers to virtual methods of several isolators.

[Ilya Pronin]

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

(Updated July 5, 2018, 2:32 p.m.)


Review request for mesos, Benjamin Mahler and James Peach.


Changes
---

Expanded the change to include `network/port_mapping` and `network/ports` 
isolators.


Summary (updated)
-

Added override specifiers to virtual methods of several isolators.


Repository: mesos


Description (updated)
---

This is a follow-up change for disk/xfs, network/port_mapping and
network/ports isolators to avoid problems with virtual method signatures
in the future.


Diffs (updated)
-

  src/slave/containerizer/mesos/isolators/network/port_mapping.hpp 
6ff3820de6af544c6ec92b0e76fd934715b87a96 
  src/slave/containerizer/mesos/isolators/network/ports.hpp 
8d467ceceae21d30318195785dce635d054d0f8d 
  src/slave/containerizer/mesos/isolators/xfs/disk.hpp 
9a5ca8bd60c61d65beed611a02dd26ed6a0a594b 


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

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


Testing
---

Built Mesos with `--enable-xfs-disk-isolator` and ran `sudo make check`.


Thanks,

Ilya Pronin

Re: Review Request 67820: Fixed recover() virtual method signature in several isolators.

[Ilya Pronin]

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

(Updated July 5, 2018, 2:31 p.m.)


Review request for mesos, Benjamin Mahler and James Peach.


Changes
---

Expanded the change to include `network/port_mapping` and `network/ports` 
isolators.


Summary (updated)
-

Fixed recover() virtual method signature in several isolators.


Repository: mesos


Description (updated)
---

Commit a52e5288ea94ec7d84f82c6c01c1ce3c7a3902db replaced std::list with
std::vector in several places including the Isolator interface. However
XfsDiskIsolatorProcess, PortMappingIsolatorProcess and
NetworkPortsIsolatorProcess were not updated and because of that their
recover() method became "disabled".


Diffs (updated)
-

  src/slave/containerizer/mesos/isolators/network/port_mapping.hpp 
6ff3820de6af544c6ec92b0e76fd934715b87a96 
  src/slave/containerizer/mesos/isolators/network/port_mapping.cpp 
3f51542ca27217c1063c19be481e8c47481f1e0c 
  src/slave/containerizer/mesos/isolators/network/ports.hpp 
8d467ceceae21d30318195785dce635d054d0f8d 
  src/slave/containerizer/mesos/isolators/network/ports.cpp 
82c14413cef0ac14ccfbc700f8f10a6c120c7fb0 
  src/slave/containerizer/mesos/isolators/xfs/disk.hpp 
9a5ca8bd60c61d65beed611a02dd26ed6a0a594b 
  src/slave/containerizer/mesos/isolators/xfs/disk.cpp 
362996b804be59dff631566d4c9db2d94233e0c9 


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

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


Testing
---

Built Mesos with `--enable-xfs-disk-isolator` and ran `sudo make check`. 
`ROOT_XFS_QuotaTest.NoCheckpointRecovery` test, that was broken because of the 
issue, passed.


Thanks,

Ilya Pronin

Re: Review Request 67777: Added a helper to match agent-framework capabilities in the allocator.

[Benjamin Mahler]

> On July 2, 2018, 10:42 p.m., Benjamin Mahler wrote:
> > src/master/allocator/mesos/hierarchical.cpp
> > Line 1756 (original), 1755 (patched)
> > 
> >
> > How about a member function of the framework struct?
> > 
> > ```
> > if (!framework.isCapableOnAgent(slave)) {
> >   ...
> > }
> > ```
> 
> Meng Zhu wrote:
> That would need to move nested Slave class forward or outside. Currently, 
> all the helper functions for slave/framework are member functions of the 
> outer class. I was trying to be consistent. If you think it is necessary, I 
> can go ahead and overhaul the structure.
> 
> Meng Zhu wrote:
> I could forward declare Slave in the outer class. Yet it would still 
> depend on `filterGpuResources` of the outer class. Need to either cache a 
> reference or pass in the flag. Feel cleaner this way.

Which functions are you referring to? I only see isRemoteSlave and 
isWhitelisted that look like they should be members of Slave?

Yeah I think the ideal would be to pull these structs out of being nested so 
that the logic can be more clearly divided between them and the allocator, like 
we did in master.hpp for Slave and Framework structs. We can just put the TODOs 
in this patch and follow up separately for that, can you mark as fixed by 
adding TODOs?


- Benjamin


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


On July 4, 2018, 12:19 a.m., Meng Zhu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/6/
> ---
> 
> (Updated July 4, 2018, 12:19 a.m.)
> 
> 
> Review request for mesos and Benjamin Mahler.
> 
> 
> Bugs: MESOS-8916
> https://issues.apache.org/jira/browse/MESOS-8916
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> `isFrameworkCapableReceivingAgent` checks if a framework
> is capable of receiving resources on the agent based on
> the framework capability.
> 
> 
> Diffs
> -
> 
>   src/master/allocator/mesos/hierarchical.hpp 
> 0f6c0e96a105c64465d3f5db4ff663d8fdfe7e26 
>   src/master/allocator/mesos/hierarchical.cpp 
> 5a6cd3d2fc5bdbaaee2d05b9be9e83d4107c749b 
> 
> 
> Diff: https://reviews.apache.org/r/6/diff/3/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Meng Zhu
> 
>

Re: Review Request 67444: Persisted role consumed quota info in the allocator.

[Benjamin Mahler]

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



Looks like a great cleanup!

Persist tends to carry the connotation of writing something to durable storage. 
How about:

```
Made quota consumption tracking event-driven in the allocator.

The allocator needs to keep track of role consumed quota
to maintain quota headroom. Currently this info is
constructed on the fly prior to each allocation cycle.

This patch lets the allocator track quota consumption
across allocation cycles in an event-driven manner to improve
performance and reduce code complexity.
```


src/master/allocator/mesos/hierarchical.hpp
Lines 509-512 (patched)


```
  // To enforce quota, we keep track of how much quota is consumed by each 
role.
  // Quota consumption always includes the role's reservations (since they 
cannot
  // be allocated to other roles) as well as any allocated resources for 
the role.
  //
  // This is only tracked for roles with non-default quota.
```



src/master/allocator/mesos/hierarchical.hpp
Lines 526 (patched)


How about `consumedQuotaScalarQuantities`? We don't call it `rolesQuota` 
above for example.



src/master/allocator/mesos/hierarchical.cpp
Lines 1387-1390 (patched)


I don't think we need to repeat ourselves with the "in other words" part. I 
think we just need to explain why reservations are included in one place (in 
the header):

```
  // Track quota consumption.
  //
  // A role's quota consumption includes its allocation as well as any
  // unallocated reservations:
  //
  //   Consumed Quota = reservations + unreserved allocation
  //  = reservations + allocation - allocated reservations
```



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


It wasn't clear to me when reading this why it needs to be done on each 
agent, so we should probably clarify that?

```
// Lastly, subtract allocated reservations. This needs to be done on a 
per-agent basis because ...
```



src/master/allocator/mesos/hierarchical.cpp
Lines 1405-1406 (patched)


We probably want a TODO to optimize this by avoiding the copy by returning 
a const reference to the map?



src/master/allocator/mesos/hierarchical.cpp
Lines 1385-1390 (original), 1413-1418 (patched)


Just an unrelated observation, this note looks misleading, since the master 
does try to rescind offers to re-balance.



src/master/allocator/mesos/hierarchical.cpp
Lines 1439-1440 (patched)


It looks like it follows the same order as operations in setQuota, so I 
would expect this to be done after metrics.removeQuota. Any reason not to?

Actually, it seems like the metric should get added after quota tracking 
and removed before quota tracking is removed, since I would imagine the metrics 
look at the tracking information. It looks like we currently only expose 
'allocated' instead of 'consumed' for now, but we probably want to expose 
'consumed' soon, is there a ticket?



src/master/allocator/mesos/hierarchical.cpp
Lines 2613-2614 (original), 2576-2577 (patched)


Hm.. an aside, but I was puzzled about this function. Since it takes 
role->reservations it loses information for non-scalar resources.

Fortunately, this function only uses the scalar quantities anyway, but we 
should make the interface clearly take quantities to clarify the assumption.



src/master/allocator/mesos/hierarchical.cpp
Lines 2586-2588 (patched)


It's pretty hard to reason from here about whether this is correct. For 
example, how do we know that these quantities were not already tracked because 
they were allocated prior to becoming reserved? If that invariant doesn't hold 
we'll double count?



src/master/allocator/mesos/hierarchical.cpp
Lines 2607-2611 (patched)


Ditto here, it's hard to reason about why we can remove it here, what if 
the resources are unreserved but remain allocated?



src/master/allocator/mesos/hierarchical.cpp
Lines 2727-2728 (patched)


Hm.. there seem to be some invariants here in how the tracking functions 
are called but I can't quite figure them out. Is there a way to enforce them?

Let's say I have allocated 

Re: Review Request 67820: Fixed XfsDiskIsolatorProcess::recover() signature.

[Benjamin Mahler]

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



Also, is there a ticket? Did any releases ship with this broken? i.e. Do we 
need to backport this?

- Benjamin Mahler


On July 3, 2018, 9:31 p.m., Ilya Pronin wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/67820/
> ---
> 
> (Updated July 3, 2018, 9:31 p.m.)
> 
> 
> Review request for mesos, Benjamin Mahler and James Peach.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Change r/67312 replaced std::list with std::vector in several places
> including the Isolator interface. However XfsDiskIsolatorProcess was not
> updated and because of that its recover() method became "disabled".
> 
> 
> Diffs
> -
> 
>   src/slave/containerizer/mesos/isolators/xfs/disk.hpp 
> 9a5ca8bd60c61d65beed611a02dd26ed6a0a594b 
>   src/slave/containerizer/mesos/isolators/xfs/disk.cpp 
> 362996b804be59dff631566d4c9db2d94233e0c9 
> 
> 
> Diff: https://reviews.apache.org/r/67820/diff/1/
> 
> 
> Testing
> ---
> 
> Built Mesos with `--enable-xfs-disk-isolator` and ran `sudo make check`. 
> `ROOT_XFS_QuotaTest.NoCheckpointRecovery` test, that was broken because of 
> the issue, passed.
> 
> 
> Thanks,
> 
> Ilya Pronin
> 
>

Re: Review Request 67791: Prevented master from asking agents to shutdown on auth failures.

[Vinod Kone]

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


Ship it!




Ship It!

- Vinod Kone


On July 3, 2018, 11:57 p.m., Gastón Kleiman wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/67791/
> ---
> 
> (Updated July 3, 2018, 11:57 p.m.)
> 
> 
> Review request for mesos, Alexander Rojas, Greg Mann, and Jiang Yan Xu.
> 
> 
> Bugs: MESOS-8987
> https://issues.apache.org/jira/browse/MESOS-8987
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> The Mesos master sends a `ShutdownMessage` to an agent if there is an
> authentication or an authorization error during agent (re)registration.
> 
> Upon receipt of this message, the agent kills alls its tasks and commits
> suicide. This means that transient auth errors can lead to whole agents
> being killed along with it's tasks.
> 
> This patch prevents the master from sending a `ShutdownMessage` in these
> cases.
> 
> 
> Diffs
> -
> 
>   src/master/master.cpp ddc8df0ea82241be6c733237feef1553c7669eb2 
>   src/tests/authentication_tests.cpp bd46cbc6d565ea8f2f6956c0424a76ad58607017 
>   src/tests/master_authorization_tests.cpp 
> 80b9d49ba334b915461ff5d6df6c9f922d7593e3 
> 
> 
> Diff: https://reviews.apache.org/r/67791/diff/3/
> 
> 
> Testing
> ---
> 
> `bin/mesos-tests.sh` on GNU/Linux
> 
> 
> Thanks,
> 
> Gastón Kleiman
> 
>

Re: Review Request 67722: Fixed unproperly guarded future.

[Gastón Kleiman]

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




src/master/master.cpp
Lines 9794-9806 (original), 9794-9809 (patched)


I personally find the following easier to read than the nested ifs:

```
if (future.isReady() && future.isSome()) {
  LOG(INFO) << "Successfully authenticated principal '" << future->get() << 
"'"
<< " at " << pid;

  authenticated.put(pid, future->get());
} else if (future.isReady() && !future.isSome()) {
LOG(WARNING) << "Authentication of " << pid << " was unsuccessful:"
 << " Invalid credentials";
} else if (future.isFailed()) {
  LOG(WARNING) << "An error ocurred while attempting to authenticate " << 
pid
   << ": " << future.failure();
} else if (future.isDiscarded()) {
  LOG(INFO) << "Authentication of " << pid << " was discarded";
}
```

I'm curious about what other people think.



src/master/master.cpp
Lines 9801-9802 (patched)


Nit: we try to use leading spaces instead of trailing spaces, so this 
should be:

```
  LOG(INFO) << "Authentication of " << pid << " was unsuccessful:"
<< " Invalid credentials";
```



src/master/master.cpp
Lines 9807 (patched)


s/"was discarded"/" was discarded"/


- Gastón Kleiman


On July 3, 2018, 3:17 a.m., Alexander Rojas wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/67722/
> ---
> 
> (Updated July 3, 2018, 3:17 a.m.)
> 
> 
> Review request for mesos, Gastón Kleiman, Greg Mann, and Vinod Kone.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This patch fixes a bug where the code path could cause a crash because
> of calling `Fture::get()` on a future which is failed.
> 
> 
> Diffs
> -
> 
>   src/master/master.cpp ddc8df0ea82241be6c733237feef1553c7669eb2 
> 
> 
> Diff: https://reviews.apache.org/r/67722/diff/2/
> 
> 
> Testing
> ---
> 
> ```sh
> make check
> ```
> 
> 
> Thanks,
> 
> Alexander Rojas
> 
>

Re: Review Request 67722: Fixed unproperly guarded future.

[Gastón Kleiman]

> On July 3, 2018, 2:28 a.m., Alexander Rojas wrote:
> > src/master/master.cpp
> > Lines 9667-9672 (patched)
> > 
> >
> > Honestly, I'm not even sure there should be a warning there unless 
> > there is an error now that I think about it.
> > 
> > If a framework is refused authentication because of invalid 
> > credentials, that is just normal behavior and should be logged as info.

I would make the error a warning to be consistent with what we do when 
authorizing an agent: 
https://github.com/apache/mesos/blob/bf69856f241590b652485e012580dbbdb5e11674/src/master/master.cpp#L6791-L6794


- Gastón


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


On July 3, 2018, 3:17 a.m., Alexander Rojas wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/67722/
> ---
> 
> (Updated July 3, 2018, 3:17 a.m.)
> 
> 
> Review request for mesos, Gastón Kleiman, Greg Mann, and Vinod Kone.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This patch fixes a bug where the code path could cause a crash because
> of calling `Fture::get()` on a future which is failed.
> 
> 
> Diffs
> -
> 
>   src/master/master.cpp ddc8df0ea82241be6c733237feef1553c7669eb2 
> 
> 
> Diff: https://reviews.apache.org/r/67722/diff/2/
> 
> 
> Testing
> ---
> 
> ```sh
> make check
> ```
> 
> 
> Thanks,
> 
> Alexander Rojas
> 
>

Re: Review Request 67488: Updated CLI to Python 3.

[Mesos Reviewbot Windows]

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



FAIL: Some of the unit tests failed. Please check the relevant logs.

Reviews applied: `['67488']`

Failed command: `Start-MesosCITesting`

All the build artifacts available at: 
http://dcos-win.westus.cloudapp.azure.com/mesos-build/review/67488

Relevant logs:

- 
[mesos-tests-stdout.log](http://dcos-win.westus.cloudapp.azure.com/mesos-build/review/67488/logs/mesos-tests-stdout.log):

```
[ RUN  ] DockerContainerizerTest.ROOT_DOCKER_DestroyWhilePulling
[   OK ] DockerContainerizerTest.ROOT_DOCKER_DestroyWhilePulling (796 ms)
[ RUN  ] DockerContainerizerTest.ROOT_DOCKER_DestroyUnknownContainer
[   OK ] DockerContainerizerTest.ROOT_DOCKER_DestroyUnknownContainer (601 
ms)
[ RUN  ] DockerContainerizerTest.ROOT_DOCKER_ExecutorCleanupWhenLaunchFailed
[   OK ] 
DockerContainerizerTest.ROOT_DOCKER_ExecutorCleanupWhenLaunchFailed (1322 ms)
[ RUN  ] DockerContainerizerTest.ROOT_DOCKER_FetchFailure
[   OK ] DockerContainerizerTest.ROOT_DOCKER_FetchFailure (803 ms)
[ RUN  ] DockerContainerizerTest.ROOT_DOCKER_DockerPullFailure
[   OK ] DockerContainerizerTest.ROOT_DOCKER_DockerPullFailure (800 ms)
[ RUN  ] DockerContainerizerTest.ROOT_DOCKER_DockerInspectDiscard
[   OK ] DockerContainerizerTest.ROOT_DOCKER_DockerInspectDiscard (1002 ms)
[ RUN  ] DockerContainerizerTest.ROOT_DOCKER_WaitUnknownContainer
[   OK ] DockerContainerizerTest.ROOT_DOCKER_WaitUnknownContainer (601 ms)
[ RUN  ] 
DockerContainerizerTest.ROOT_DOCKER_NoTransitionFromKillingToRunning
[   OK ] 
DockerContainerizerTest.ROOT_DOCKER_NoTransitionFromKillingToRunning (5060 ms)
[ RUN  ] DockerContainerizerTest.ROOT_DOCKER_DefaultDNS
[   OK ] DockerContainerizerTest.ROOT_DOCKER_DefaultDNS (4858 ms)
[--] 24 tests from DockerContainerizerTest (89904 ms total)

[--] 1 test from HungDockerTest
[ RUN  ] HungDockerTest.ROOT_DOCKER_InspectHungDuringPull

d:\dcos\mesos\mesos\src\tests\mock_docker.hpp(155): ERROR: this mock object 
(used in test HungDockerTest.ROOT_DOCKER_InspectHungDuringPull) should be 
deleted but never is. Its address is @00901419B8A0.
d:\dcos\mesos\mesos\src\tests\containerizer\docker_containerizer_tests.cpp(5187):
 ERROR: this mock object (used in test 
HungDockerTest.ROOT_DOCKER_InspectHungDuringPull) should be deleted but never 
is. Its address is @00901419BB00.
d:\dcos\mesos\mesos\src\tests\mock_docker.cpp(48): ERROR: this mock object 
(used in test HungDockerTest.ROOT_DOCKER_InspectHungDuringPull) should be 
deleted but never is. Its address is @027B4512C090.
d:\dcos\mesos\mesos\src\tests\mock_registrar.cpp(54): ERROR: this mock object 
(used in test HungDockerTest.ROOT_DOCKER_InspectHungDuringPull) should be 
deleted but never is. Its address is @027B45B77BD0.
d:\dcos\mesos\mesos\3rdparty\libprocess\include\process\gmock.hpp(235): ERROR: 
this mock object (used in test 
HungDockerTest.ROOT_DOCKER_InspectHungDuringPull) should be deleted but never 
is. Its address is @027B46F7F738.
ERROR: 5 leaked mock objects found at program exit.
```

- 
[mesos-tests-stderr.log](http://dcos-win.westus.cloudapp.azure.com/mesos-build/review/67488/logs/mesos-tests-stderr.log):

```
I0705 14:24:38.341552 11648 authenticatee.cpp:299] Authentication success
I0705 14:24:38.342562 13080 master.cpp:9802] Successfully authenticated 
principal 'test-principal' at 
scheduler-b9beba28-fb2f-4472-b9c5-60adca8aa1be@192.10.1.6:51840
I0705 14:24:38.342562 10324 sched.cpp:501] Successfully authenticated with 
master master@192.10.1.6:51840
I0705 14:24:38.344561 13352 master.cpp:2927] Received SUBSCRIBE call for 
framework 'default' at 
scheduler-b9beba28-fb2f-4472-b9c5-60adca8aa1be@192.10.1.6:51840
I0705 14:24:38.344561 13352 master.cpp:2234] Authorizing framework principal 
'test-principal' to receive offers for roles '{ * }'
I0705 14:24:38.345543 13080 master.cpp:3008] Subscribing framework default with 
checkpointing disabled and capabilities [ MULTI_ROLE, RESERVATION_REFINEMENT ]
I0705 14:24:38.345543 13080 master.cpp:9993] Adding framework 
50acb31d-9a6e-4ab0-8b9a-6d21149a648b- (default) at 
scheduler-b9beba28-fb2f-4472-b9c5-60adca8aa1be@192.10.1.6:51840 with roles {  } 
suppressed
I0705 14:24:38.346565 13064 sched.cpp:749] Framework registered with 
50acb31d-9a6e-4ab0-8b9a-6d21149a648b-
I0705 14:24:38.347561  4344 hierarchical.cpp:299] Added framework 
50acb31d-9a6e-4ab0-8b9a-6d21149a648b-
E0705 14:24:38.441642 13812 slave.cpp:7289] EXIT with status 1: Failed to 
perform recovery: Collect failed: Failed to run 'C:\Program Files 
(x86)\Microsoft Visual Studio\2017\Community\VC\Auxiliary\Build\test-docker.bat 
-H npipe:./pipe/docker_engine ps -a': exited with status 1; 
stderr=''C:\Program' is not recognized as 

Re: Review Request 67832: Changed docker subprocess calls to use exec form.

[Mesos Reviewbot Windows]

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



PASS: Mesos patch 67832 was successfully built and tested.

Reviews applied: `['67832']`

All the build artifacts available at: 
http://dcos-win.westus.cloudapp.azure.com/mesos-build/review/67832

- Mesos Reviewbot Windows


On July 5, 2018, 9:16 a.m., Akash Gupta wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/67832/
> ---
> 
> (Updated July 5, 2018, 9:16 a.m.)
> 
> 
> Review request for mesos, Andrew Schwartzmeyer and Joseph Wu.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> The docker executor used the command line subprocess form even it only
> ever called the docker executable. The command line form made the
> executor fail if the `--docker=` path contained spaces.
> 
> 
> Diffs
> -
> 
>   src/docker/docker.hpp 25d9ca662fa5d99b32c668a5fdfc75584132cc38 
>   src/docker/docker.cpp baac70f25ef3f944541341822aacb6a395853113 
>   src/tests/containerizer/docker_tests.cpp 
> 7097efc716ab0e1f34d5a1a35d8e0e173b113c91 
> 
> 
> Diff: https://reviews.apache.org/r/67832/diff/1/
> 
> 
> Testing
> ---
> 
> Tested by running `HungDockerTest` on a path with a space. Waiting for 
> Windows CI for verification.
> On Linux, ran mesos-tests.
> 
> 
> Thanks,
> 
> Akash Gupta
> 
>

Re: Review Request 67832: Changed docker subprocess calls to use exec form.

[Mesos Reviewbot Windows]

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



PASS: Mesos patch 67832 was successfully built and tested.

Reviews applied: `['67832']`

All the build artifacts available at: 
http://dcos-win.westus.cloudapp.azure.com/mesos-build/review/67832

- Mesos Reviewbot Windows


On July 5, 2018, 9:16 a.m., Akash Gupta wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/67832/
> ---
> 
> (Updated July 5, 2018, 9:16 a.m.)
> 
> 
> Review request for mesos, Andrew Schwartzmeyer and Joseph Wu.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> The docker executor used the command line subprocess form even it only
> ever called the docker executable. The command line form made the
> executor fail if the `--docker=` path contained spaces.
> 
> 
> Diffs
> -
> 
>   src/docker/docker.hpp 25d9ca662fa5d99b32c668a5fdfc75584132cc38 
>   src/docker/docker.cpp baac70f25ef3f944541341822aacb6a395853113 
>   src/tests/containerizer/docker_tests.cpp 
> 7097efc716ab0e1f34d5a1a35d8e0e173b113c91 
> 
> 
> Diff: https://reviews.apache.org/r/67832/diff/1/
> 
> 
> Testing
> ---
> 
> Tested by running `HungDockerTest` on a path with a space. Waiting for 
> Windows CI for verification.
> On Linux, ran mesos-tests.
> 
> 
> Thanks,
> 
> Akash Gupta
> 
>

Review Request 67832: Changed docker subprocess calls to use exec form.

[Akash Gupta]

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

Review request for mesos, Andrew Schwartzmeyer and Joseph Wu.


Repository: mesos


Description
---

The docker executor used the command line subprocess form even it only
ever called the docker executable. The command line form made the
executor fail if the `--docker=` path contained spaces.


Diffs
-

  src/docker/docker.hpp 25d9ca662fa5d99b32c668a5fdfc75584132cc38 
  src/docker/docker.cpp baac70f25ef3f944541341822aacb6a395853113 
  src/tests/containerizer/docker_tests.cpp 
7097efc716ab0e1f34d5a1a35d8e0e173b113c91 


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


Testing
---

Tested by running `HungDockerTest` on a path with a space. Waiting for Windows 
CI for verification.
On Linux, ran mesos-tests.


Thanks,

Akash Gupta

Re: Review Request 67830: Moved `executor::Call` validation to common validation library.

[Mesos Reviewbot Windows]

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



FAIL: Some of the unit tests failed. Please check the relevant logs.

Reviews applied: `['67830']`

Failed command: `Start-MesosCITesting`

All the build artifacts available at: 
http://dcos-win.westus.cloudapp.azure.com/mesos-build/review/67830

Relevant logs:

- 
[mesos-tests-stdout.log](http://dcos-win.westus.cloudapp.azure.com/mesos-build/review/67830/logs/mesos-tests-stdout.log):

```
[ RUN  ] DockerContainerizerTest.ROOT_DOCKER_DestroyWhilePulling
[   OK ] DockerContainerizerTest.ROOT_DOCKER_DestroyWhilePulling (797 ms)
[ RUN  ] DockerContainerizerTest.ROOT_DOCKER_DestroyUnknownContainer
[   OK ] DockerContainerizerTest.ROOT_DOCKER_DestroyUnknownContainer (602 
ms)
[ RUN  ] DockerContainerizerTest.ROOT_DOCKER_ExecutorCleanupWhenLaunchFailed
[   OK ] 
DockerContainerizerTest.ROOT_DOCKER_ExecutorCleanupWhenLaunchFailed (1224 ms)
[ RUN  ] DockerContainerizerTest.ROOT_DOCKER_FetchFailure
[   OK ] DockerContainerizerTest.ROOT_DOCKER_FetchFailure (801 ms)
[ RUN  ] DockerContainerizerTest.ROOT_DOCKER_DockerPullFailure
[   OK ] DockerContainerizerTest.ROOT_DOCKER_DockerPullFailure (801 ms)
[ RUN  ] DockerContainerizerTest.ROOT_DOCKER_DockerInspectDiscard
[   OK ] DockerContainerizerTest.ROOT_DOCKER_DockerInspectDiscard (904 ms)
[ RUN  ] DockerContainerizerTest.ROOT_DOCKER_WaitUnknownContainer
[   OK ] DockerContainerizerTest.ROOT_DOCKER_WaitUnknownContainer (600 ms)
[ RUN  ] 
DockerContainerizerTest.ROOT_DOCKER_NoTransitionFromKillingToRunning
[   OK ] 
DockerContainerizerTest.ROOT_DOCKER_NoTransitionFromKillingToRunning (5160 ms)
[ RUN  ] DockerContainerizerTest.ROOT_DOCKER_DefaultDNS
[   OK ] DockerContainerizerTest.ROOT_DOCKER_DefaultDNS (4950 ms)
[--] 24 tests from DockerContainerizerTest (91671 ms total)

[--] 1 test from HungDockerTest
[ RUN  ] HungDockerTest.ROOT_DOCKER_InspectHungDuringPull

d:\dcos\mesos\mesos\src\tests\mock_docker.hpp(155): ERROR: this mock object 
(used in test HungDockerTest.ROOT_DOCKER_InspectHungDuringPull) should be 
deleted but never is. Its address is @00061F0FBDC0.
d:\dcos\mesos\mesos\src\tests\containerizer\docker_containerizer_tests.cpp(5187):
 ERROR: this mock object (used in test 
HungDockerTest.ROOT_DOCKER_InspectHungDuringPull) should be deleted but never 
is. Its address is @00061F0FC020.
d:\dcos\mesos\mesos\src\tests\mock_registrar.cpp(54): ERROR: this mock object 
(used in test HungDockerTest.ROOT_DOCKER_InspectHungDuringPull) should be 
deleted but never is. Its address is @01C3EEC7E070.
d:\dcos\mesos\mesos\3rdparty\libprocess\include\process\gmock.hpp(235): ERROR: 
this mock object (used in test 
HungDockerTest.ROOT_DOCKER_InspectHungDuringPull) should be deleted but never 
is. Its address is @01C3EF385088.
d:\dcos\mesos\mesos\src\tests\mock_docker.cpp(48): ERROR: this mock object 
(used in test HungDockerTest.ROOT_DOCKER_InspectHungDuringPull) should be 
deleted but never is. Its address is @01C3F047C4D0.
ERROR: 5 leaked mock objects found at program exit.
```

- 
[mesos-tests-stderr.log](http://dcos-win.westus.cloudapp.azure.com/mesos-build/review/67830/logs/mesos-tests-stderr.log):

```
I0705 07:14:33.270709 10704 authenticatee.cpp:299] Authentication success
I0705 07:14:33.270709 10940 master.cpp:9802] Successfully authenticated 
principal 'test-principal' at 
scheduler-95410873-6ffa-4c2f-81e9-3bad7ac58ad9@192.10.1.6:64891
I0705 07:14:33.270709 13468 sched.cpp:501] Successfully authenticated with 
master master@192.10.1.6:64891
I0705 07:14:33.271716 12772 master.cpp:2927] Received SUBSCRIBE call for 
framework 'default' at 
scheduler-95410873-6ffa-4c2f-81e9-3bad7ac58ad9@192.10.1.6:64891
I0705 07:14:33.272716 12772 master.cpp:2234] Authorizing framework principal 
'test-principal' to receive offers for roles '{ * }'
I0705 07:14:33.272716 12936 master.cpp:3008] Subscribing framework default with 
checkpointing disabled and capabilities [ MULTI_ROLE, RESERVATION_REFINEMENT ]
I0705 07:14:33.273715 12936 master.cpp:9993] Adding framework 
0355c3cb-80d6-47f7-a0f1-bb48f80bf740- (default) at 
scheduler-95410873-6ffa-4c2f-81e9-3bad7ac58ad9@192.10.1.6:64891 with roles {  } 
suppressed
I0705 07:14:33.274713 10940 sched.cpp:749] Framework registered with 
0355c3cb-80d6-47f7-a0f1-bb48f80bf740-
I0705 07:14:33.275730 13144 hierarchical.cpp:299] Added framework 
0355c3cb-80d6-47f7-a0f1-bb48f80bf740-
E0705 07:14:33.370712 10704 slave.cpp:7289] EXIT with status 1: Failed to 
perform recovery: Collect failed: Failed to run 'C:\Program Files 
(x86)\Microsoft Visual Studio\2017\Community\VC\Auxiliary\Build\test-docker.bat 
-H npipe:./pipe/docker_engine ps -a': exited with status 1; 
stderr=''C:\Program' is not recognized as 

Review Request 67830: Moved `executor::Call` validation to common validation library.

[James Peach]

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

Review request for mesos and Alexander Rukletsov.


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


Repository: mesos


Description
---

`executor::Call` validation is used by both the agent and the
executor driver, so move it to the common validation library.


Diffs
-

  src/common/validation.hpp fa7f4a7164fda0f6af98c60a27e944146a231ee7 
  src/common/validation.cpp f89b0ca886ef910d01cc0dba6e10505dc5e6ff6f 
  src/executor/executor.cpp ab67caefd82f46eacd33221270b2c408ef70cd17 
  src/slave/http.cpp aa9cdc5f58f73323958a6872e2721c83317f354c 
  src/slave/validation.hpp 3a278e43f0e98c1ed6dcdec60e71c131973ccb43 
  src/slave/validation.cpp 3e333a7b308ec9c42541036f52b48b4ad74f5b88 


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


Testing
---

make check (Fedora 28)


Thanks,

James Peach