Re: Review Request 66050: Implemented grow and shrink of persistent volumes.

[Chun-Hung Hsiao]

> On March 27, 2018, 6:32 a.m., Zhitao Li wrote:
> > src/slave/slave.cpp
> > Lines 8053-8059 (patched)
> > 
> >
> > Sorry I'm not following this comment.
> > 
> > From what I can read, the sequence here is:
> > 
> > ```
> > Slave::addOperation();
> > calculate converted resources;
> > UpdateOperationStatusMessage update = generate OperationStatus;
> > Slave::updateOperation();
> >   - Slave::apply(...) // which performs checkpointing
> > send(master.get(), update); // actually sending to master
> > ```
> > so when we send the `UpdateOperationStatusMessage` to master, 
> > Slave::apply() should already be called.
> > 
> > Am I missing something?

Oops. Sorry I read the patch wrong. So you moved the checkpointing logic into 
`Slave::apply()`, which should be OK, just that we'll do extra checkpoints when 
processing operations related to RP resources. Probably still worth thinking if 
the code can be further simpilified with some refactoring.


- Chun-Hung


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


On March 27, 2018, 6:30 a.m., Zhitao Li wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/66050/
> ---
> 
> (Updated March 27, 2018, 6:30 a.m.)
> 
> 
> Review request for mesos, Chun-Hung Hsiao, Gaston Kleiman, and Greg Mann.
> 
> 
> Bugs: MESOS-4965
> https://issues.apache.org/jira/browse/MESOS-4965
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> The new offer operations are implemented as non-speculative operations,
> which means they cannot be chained with previous offer operations which
> depend on results of each other.
> 
> 
> Diffs
> -
> 
>   src/common/protobuf_utils.cpp 2acdc798200e82d14651b1edcc5f83e174477d53 
>   src/common/resources_utils.cpp 9be01c1abd48264e308960f35cc7c2ee8a367518 
>   src/master/master.cpp 20692c36c021fdae91de0b156f26fc56cf7c4f45 
>   src/master/validation.hpp 7c129ceb929596acbb64d37025e055661277e6bf 
>   src/master/validation.cpp 9254cd22e67d9aca53135701575fb0857ff6f7d6 
>   src/slave/slave.cpp e76daabad0d2d68aa42d1da809d4a23459eaaacb 
>   src/tests/mesos.hpp 01fd1c2bf21ef96dbab0769a1c1c92d7e3b60032 
>   src/tests/persistent_volume_tests.cpp 
> 924d8458e54e34a49c99593482b5908c5f7c7a48 
>   src/tests/reservation_tests.cpp 8d8e9c8390e65187269bd194bb322bbdff88e0bd 
> 
> 
> Diff: https://reviews.apache.org/r/66050/diff/6/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Zhitao Li
> 
>

Re: Review Request 66234: Fixed variable shadowing in the default executor.

[Gaston Kleiman]

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

(Updated March 27, 2018, 9:43 a.m.)


Review request for mesos and Alexander Rukletsov.


Repository: mesos


Description
---

Fixed variable shadowing in the default executor.


Diffs
-

  src/launcher/default_executor.cpp 906836f3b8e0af79d7c61f90fd8a95f193b26e84 


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


Testing
---

`sudo bin/mesos-tests.sh` on GNU/Linux


Thanks,

Gaston Kleiman

Re: Review Request 66219: Added helper functions to create grow and shrink volume in test.

[Zhitao Li]

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

(Updated March 26, 2018, 11:38 p.m.)


Review request for mesos, Chun-Hung Hsiao, Gaston Kleiman, and Greg Mann.


Changes
---

Review comments.


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


Repository: mesos


Description
---

Added helper functions to create grow and shrink volume in test.


Diffs (updated)
-

  src/tests/mesos.hpp 01fd1c2bf21ef96dbab0769a1c1c92d7e3b60032 


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

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


Testing
---


Thanks,

Zhitao Li

Re: Review Request 66050: Implemented grow and shrink of persistent volumes.

[Zhitao Li]

> On March 22, 2018, 3:43 p.m., Chun-Hung Hsiao wrote:
> > src/common/resources_utils.cpp
> > Lines 199-261 (patched)
> > 
> >
> > We should not speculatively carve out the conversion here. Let's wait 
> > for the `UpdateOperationStatusMessage` and get the converted resource 
> > instead.

Per discussion, we will reuse this function but the conversions carved out will 
not be applied in master.


- Zhitao


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


On March 26, 2018, 11:30 p.m., Zhitao Li wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/66050/
> ---
> 
> (Updated March 26, 2018, 11:30 p.m.)
> 
> 
> Review request for mesos, Chun-Hung Hsiao, Gaston Kleiman, and Greg Mann.
> 
> 
> Bugs: MESOS-4965
> https://issues.apache.org/jira/browse/MESOS-4965
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> The new offer operations are implemented as non-speculative operations,
> which means they cannot be chained with previous offer operations which
> depend on results of each other.
> 
> 
> Diffs
> -
> 
>   src/common/protobuf_utils.cpp 2acdc798200e82d14651b1edcc5f83e174477d53 
>   src/common/resources_utils.cpp 9be01c1abd48264e308960f35cc7c2ee8a367518 
>   src/master/master.cpp 20692c36c021fdae91de0b156f26fc56cf7c4f45 
>   src/master/validation.hpp 7c129ceb929596acbb64d37025e055661277e6bf 
>   src/master/validation.cpp 9254cd22e67d9aca53135701575fb0857ff6f7d6 
>   src/slave/slave.cpp e76daabad0d2d68aa42d1da809d4a23459eaaacb 
>   src/tests/mesos.hpp 01fd1c2bf21ef96dbab0769a1c1c92d7e3b60032 
>   src/tests/persistent_volume_tests.cpp 
> 924d8458e54e34a49c99593482b5908c5f7c7a48 
>   src/tests/reservation_tests.cpp 8d8e9c8390e65187269bd194bb322bbdff88e0bd 
> 
> 
> Diff: https://reviews.apache.org/r/66050/diff/6/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Zhitao Li
> 
>

Re: Review Request 66218: Ensured that agent does not delete volume upon grow or shrink.

[Zhitao Li]

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

(Updated March 26, 2018, 11:36 p.m.)


Review request for mesos, Chun-Hung Hsiao, Gaston Kleiman, and Greg Mann.


Changes
---

Review comments.


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


Repository: mesos


Description
---

Previously, `slave::syncCheckpointedResources` implementation will
delete a persistent volume using `Resources::contains` check, which
could cause a resized volume being deleted. The function was rewritten
to compare `set_difference` between old and new paths for all persistent
volumes and perform creation/deletion accordingly.


Diffs (updated)
-

  src/slave/slave.cpp e76daabad0d2d68aa42d1da809d4a23459eaaacb 


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

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


Testing
---


Thanks,

Zhitao Li

Re: Review Request 66050: Implemented grow and shrink of persistent volumes.

[Zhitao Li]

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




src/master/validation.cpp
Lines 2393 (patched)


Most of the containment checks are already checked in `Master::_accept` so 
we do not need to check again (except for `!isShared()`)



src/slave/slave.cpp
Lines 8053-8059 (patched)


Sorry I'm not following this comment.

From what I can read, the sequence here is:

```
Slave::addOperation();
calculate converted resources;
UpdateOperationStatusMessage update = generate OperationStatus;
Slave::updateOperation();
  - Slave::apply(...) // which performs checkpointing
send(master.get(), update); // actually sending to master
```
so when we send the `UpdateOperationStatusMessage` to master, 
Slave::apply() should already be called.

Am I missing something?


- Zhitao Li


On March 26, 2018, 11:30 p.m., Zhitao Li wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/66050/
> ---
> 
> (Updated March 26, 2018, 11:30 p.m.)
> 
> 
> Review request for mesos, Chun-Hung Hsiao, Gaston Kleiman, and Greg Mann.
> 
> 
> Bugs: MESOS-4965
> https://issues.apache.org/jira/browse/MESOS-4965
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> The new offer operations are implemented as non-speculative operations,
> which means they cannot be chained with previous offer operations which
> depend on results of each other.
> 
> 
> Diffs
> -
> 
>   src/common/protobuf_utils.cpp 2acdc798200e82d14651b1edcc5f83e174477d53 
>   src/common/resources_utils.cpp 9be01c1abd48264e308960f35cc7c2ee8a367518 
>   src/master/master.cpp 20692c36c021fdae91de0b156f26fc56cf7c4f45 
>   src/master/validation.hpp 7c129ceb929596acbb64d37025e055661277e6bf 
>   src/master/validation.cpp 9254cd22e67d9aca53135701575fb0857ff6f7d6 
>   src/slave/slave.cpp e76daabad0d2d68aa42d1da809d4a23459eaaacb 
>   src/tests/mesos.hpp 01fd1c2bf21ef96dbab0769a1c1c92d7e3b60032 
>   src/tests/persistent_volume_tests.cpp 
> 924d8458e54e34a49c99593482b5908c5f7c7a48 
>   src/tests/reservation_tests.cpp 8d8e9c8390e65187269bd194bb322bbdff88e0bd 
> 
> 
> Diff: https://reviews.apache.org/r/66050/diff/6/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Zhitao Li
> 
>

Re: Review Request 66050: Implemented grow and shrink of persistent volumes.

[Zhitao Li]

> On March 22, 2018, 3:43 p.m., Chun-Hung Hsiao wrote:
> > src/master/validation.cpp
> > Lines 2354 (patched)
> > 
> >
> > Let's do the following in order.
> > 
> > First, general validations:
> > - `resource::validate(grow.volume())` (The check is not included in 
> > `validatePersistentVolume`)
> > - `resource::validate(grow.addition())`
> > - `!isPersistentVolume(grow.addition())`
> > 
> > Second, reject RP resources:
> > ``` 
> > if (Resources::hasResourceProvider(grow.volume())) {
> >   return Error("Growing a volume from a resource provider is not 
> > supported");
> > }
> > ```
> > 
> > Third, validations specific to agent default resources:
> > - Validations for `grow.volume()`:
> >   1) `resource::validatePersistentVolume(grow.volume())`
> >   2) `!Resources::isShared(grow.volume())
> >   3) `checkpointedResources.contains(unallocated(grow.volume()))`
> >   4) `!unallocated(usedResources).contains(unallocated(grow.volume()))`
> > 
> > - Validations for `grow.addition()`:
> >   1) `!Resources::hasResourceProvider(grow.addition())`
> >   2) `checkpointedResources.contains(unallocated(grow.addition()))`
> >   3) `!unallocated(usedResources).contains(unallocated(grow.addition())`
> >  
> > Alternatively, the containment checks for `checkpointedResources` and 
> > `usedResources` could be put together if that makes the code easier to 
> > read. Also if `checkpointedResources` or `usedResources` contains RP 
> > resources, we cloud move the checks to the first section. (I think that's 
> > not the case for `checkpointedResources` but need double check.)
> > 
> > I'm not sure if we need to check if `pendingTasks` contains the 
> > resources to be consumed. Wouldn't this be validated later when validating 
> > `LAUNCH` or `LAUNCH_GROUP`?
> > 
> > Finally, for agent default resources, we coulde exercise `addable()` to 
> > validate if the two resources are compatible:
> > ```
> > Resources stripped = grow.volume();
> > if (stripped.disk.has_source()) {
> >   stripped.mutable_disk()->clear_persistence();
> >   stripped.mutable_disk()->clear_volume();
> > }
> > 
> > if ((stripped + grow.addition()).size() != 1) {
> >   return Error("Incompatible resources in the 'volume' and 'addition' 
> > field");
> > }
> > ```

I like the `addable()` idea. I would even propose that we also create helper 
function to capture the logic of clear persistent volume info from a disk 
resource.

```
Try backToDisk = grow.volume().clearPersistentVolume();

...
```

notes: also add: `!isEmpty(addition), !isPersistentVolume(addition)`


- Zhitao


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


On March 26, 2018, 11:30 p.m., Zhitao Li wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/66050/
> ---
> 
> (Updated March 26, 2018, 11:30 p.m.)
> 
> 
> Review request for mesos, Chun-Hung Hsiao, Gaston Kleiman, and Greg Mann.
> 
> 
> Bugs: MESOS-4965
> https://issues.apache.org/jira/browse/MESOS-4965
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> The new offer operations are implemented as non-speculative operations,
> which means they cannot be chained with previous offer operations which
> depend on results of each other.
> 
> 
> Diffs
> -
> 
>   src/common/protobuf_utils.cpp 2acdc798200e82d14651b1edcc5f83e174477d53 
>   src/common/resources_utils.cpp 9be01c1abd48264e308960f35cc7c2ee8a367518 
>   src/master/master.cpp 20692c36c021fdae91de0b156f26fc56cf7c4f45 
>   src/master/validation.hpp 7c129ceb929596acbb64d37025e055661277e6bf 
>   src/master/validation.cpp 9254cd22e67d9aca53135701575fb0857ff6f7d6 
>   src/slave/slave.cpp e76daabad0d2d68aa42d1da809d4a23459eaaacb 
>   src/tests/mesos.hpp 01fd1c2bf21ef96dbab0769a1c1c92d7e3b60032 
>   src/tests/persistent_volume_tests.cpp 
> 924d8458e54e34a49c99593482b5908c5f7c7a48 
>   src/tests/reservation_tests.cpp 8d8e9c8390e65187269bd194bb322bbdff88e0bd 
> 
> 
> Diff: https://reviews.apache.org/r/66050/diff/6/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Zhitao Li
> 
>

Re: Review Request 66314: Fix 3rdparty build commands for FreeBSD.

[Mesos Reviewbot]

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



Patch looks great!

Reviews applied: [66314]

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 March 27, 2018, 7:10 p.m., David Forsythe wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/66314/
> ---
> 
> (Updated March 27, 2018, 7:10 p.m.)
> 
> 
> Review request for mesos and Benjamin Bannier.
> 
> 
> Bugs: MESOS-4176
> https://issues.apache.org/jira/browse/MESOS-4176
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Fix 3rdparty build commands for FreeBSD.
> 
> 
> Diffs
> -
> 
>   3rdparty/CMakeLists.txt 2b63b58f7d6a88c9986b746283dcfa79b7bcb270 
>   cmake/CompilationConfigure.cmake 64cc56ee4208afe05df0f28af5890157e4c7d82c 
> 
> 
> Diff: https://reviews.apache.org/r/66314/diff/1/
> 
> 
> Testing
> ---
> 
> cmake --build on FreeBSD
> 
> 
> Thanks,
> 
> David Forsythe
> 
>

Re: Review Request 66143: Refactored agent task launch for better composition [2/2].

[Meng Zhu]

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

(Updated March 27, 2018, 6:07 p.m.)


Review request for mesos, Chun-Hung Hsiao and Greg Mann.


Bugs: MESOS-8617 and MESOS-8624
https://issues.apache.org/jira/browse/MESOS-8617
https://issues.apache.org/jira/browse/MESOS-8624


Repository: mesos


Description
---

This helps to encapsulate a task launch into a single
future which will come in handy when enforcing the task
launch order.

Affected tests are also updated.


Diffs (updated)
-

  src/slave/slave.hpp 37f0361251524e63d02d251e8a03901812b8affb 
  src/slave/slave.cpp e76daabad0d2d68aa42d1da809d4a23459eaaacb 
  src/tests/mock_slave.hpp 8ec2b65a75f774d970ef6507df8b0c02fda5f2fd 
  src/tests/mock_slave.cpp 8ec55b6aa84dd8b1c3743cbcdc246311585d0f63 
  src/tests/slave_tests.cpp 0d7d9726ee39e4c588ea5eb8a54a73fbe1249353 


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

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


Testing
---

make check


Thanks,

Meng Zhu

Re: Review Request 66318: Made agent resource provider config API calls idempotent.

[Mesos Reviewbot]

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



Patch looks great!

Reviews applied: [66318]

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 March 27, 2018, 4:27 p.m., Chun-Hung Hsiao wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/66318/
> ---
> 
> (Updated March 27, 2018, 4:27 p.m.)
> 
> 
> Review request for mesos, Benjamin Bannier, James DeFelice, and Jie Yu.
> 
> 
> Bugs: MESOS-8742
> https://issues.apache.org/jira/browse/MESOS-8742
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This patch adds descriptions to declare the following agent API calls
> idempotent:
>   - `ADD_RESOURCE_PROVIDER_CONFIG`
>   - `UPDATE_RESOURCE_PROVIDER_CONFIG`
>   - `REMOVE_RESOURCE_PROVIDER_CONFIG`
> 
> 
> Diffs
> -
> 
>   include/mesos/agent/agent.proto adaccb3509fdfc1e7cac9482e93a91e83bc5625d 
>   include/mesos/v1/agent/agent.proto 71352a79fe7d28d633a4badceafe18086c1e5ab7 
> 
> 
> Diff: https://reviews.apache.org/r/66318/diff/1/
> 
> 
> Testing
> ---
> 
> N/A
> 
> The actual implementation will be in the next patch.
> 
> 
> Thanks,
> 
> Chun-Hung Hsiao
> 
>

Re: Review Request 66051: Implemented operator API to grow and shrink persistent volume.

[Greg Mann]

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




src/master/master.cpp
Lines 11881-11887 (original), 11885-11890 (patched)


Currently, it looks like `Slave::usedResources` is the same as the 
"allocated resources" from that agent.

When a GROW or SHRINK operation is initiated by an operator, its consumed 
resource is in a unique state in the master while that operation is pending: 
the volume is not allocated, but it is "used" in some sense.

One place where we use `Slave::usedResources` is when validating DESTROY 
operations: 
https://github.com/apache/mesos/blob/21e4b45b388d0b272236b1c58313569f8a1d1fc8/src/master/master.cpp#L4841

This means if the master receives a DESTROY call for a persistent volume 
while there is a pending operator-initiated GROW_VOLUME call for that same 
volume (i.e. the operation was forwarded to the agent for processing but the 
master hasn't heard back yet), we would accept the DESTROY operation and 
forward it to the agent. Is this what we want?

This brings something else to mind: would it be possible for the allocator 
to send out an offer containing the consumed resource of a GROW_VOLUME or 
SHRINK_VOLUME operation, while the operation is pending on the agent? This 
seems bad, but I believe it's possible. I think this may be the first time we 
have a set of resources which we don't want the allocator to offer for a period 
of time, but which are not currently allocated to some framework/role. I wonder 
if we need some new tools in the allocator to handle this?


- Greg Mann


On March 27, 2018, 6:37 a.m., Zhitao Li wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/66051/
> ---
> 
> (Updated March 27, 2018, 6:37 a.m.)
> 
> 
> Review request for mesos, Chun-Hung Hsiao, Gaston Kleiman, and Greg Mann.
> 
> 
> Bugs: MESOS-4965
> https://issues.apache.org/jira/browse/MESOS-4965
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Implemented operator API to grow and shrink persistent volume.
> 
> 
> Diffs
> -
> 
>   src/master/http.cpp 34c9023906eca94965acc994f20e888c1f47b962 
>   src/master/master.hpp 0d9620dd0c232dc1df83477e838eeb7313bf8828 
>   src/master/master.cpp 20692c36c021fdae91de0b156f26fc56cf7c4f45 
>   src/master/validation.cpp 9254cd22e67d9aca53135701575fb0857ff6f7d6 
> 
> 
> Diff: https://reviews.apache.org/r/66051/diff/7/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Zhitao Li
> 
>

Re: Review Request 66318: Made agent resource provider config API calls idempotent.

[Chun-Hung Hsiao]

> On March 28, 2018, 2:36 a.m., James DeFelice wrote:
> > include/mesos/agent/agent.proto
> > Line 327 (original), 331 (patched)
> > 
> >
> > "Note that only..."
> > 
> > I don't think this statement is true. I have e2e tests that add a 
> > provider and then update it. The provider DID NOT exist at agent startup.

You are right. I described the statement in a wrong way. It should be:
"Note that if a config file is placed into the resource provider config 
directory out-of-band after the starts up, it will not be checked against this 
call."


> On March 28, 2018, 2:36 a.m., James DeFelice wrote:
> > include/mesos/agent/agent.proto
> > Line 332 (original), 338 (patched)
> > 
> >
> > Maybe return 409 instead of 404 here, if the caller tries to update a 
> > config that does not exist? Because, you know, 404s can be misleading given 
> > the convo we had earlier...

Yeah I was aware of this issue and discussed with Jie but we still thought 404 
is semantically more correct here. Will probably raise a general question 
related to this on the next API WG.


- Chun-Hung


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


On March 27, 2018, 11:27 p.m., Chun-Hung Hsiao wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/66318/
> ---
> 
> (Updated March 27, 2018, 11:27 p.m.)
> 
> 
> Review request for mesos, Benjamin Bannier, James DeFelice, and Jie Yu.
> 
> 
> Bugs: MESOS-8742
> https://issues.apache.org/jira/browse/MESOS-8742
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This patch adds descriptions to declare the following agent API calls
> idempotent:
>   - `ADD_RESOURCE_PROVIDER_CONFIG`
>   - `UPDATE_RESOURCE_PROVIDER_CONFIG`
>   - `REMOVE_RESOURCE_PROVIDER_CONFIG`
> 
> 
> Diffs
> -
> 
>   include/mesos/agent/agent.proto adaccb3509fdfc1e7cac9482e93a91e83bc5625d 
>   include/mesos/v1/agent/agent.proto 71352a79fe7d28d633a4badceafe18086c1e5ab7 
> 
> 
> Diff: https://reviews.apache.org/r/66318/diff/1/
> 
> 
> Testing
> ---
> 
> N/A
> 
> The actual implementation will be in the next patch.
> 
> 
> Thanks,
> 
> Chun-Hung Hsiao
> 
>

Re: Review Request 66326: Added tests for agent resource provider API idempotency.

[Mesos Reviewbot Windows]

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



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

Reviews applied: `['66318', '66325', '66326']`

Failed command: `Start-MesosCITesting`

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

Relevant logs:

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

```
[   OK ] Endpoint/SlaveEndpointTest.NoAuthorizer/2 (108 ms)
[--] 9 tests from Endpoint/SlaveEndpointTest (1034 ms total)

[--] 2 tests from ContainerizerType/DefaultContainerDNSFlagTest
[ RUN  ] ContainerizerType/DefaultContainerDNSFlagTest.ValidateFlag/0
[   OK ] ContainerizerType/DefaultContainerDNSFlagTest.ValidateFlag/0 (32 
ms)
[ RUN  ] ContainerizerType/DefaultContainerDNSFlagTest.ValidateFlag/1
[   OK ] ContainerizerType/DefaultContainerDNSFlagTest.ValidateFlag/1 (36 
ms)
[--] 2 tests from ContainerizerType/DefaultContainerDNSFlagTest (69 ms 
total)

[--] 1 test from IsolationFlag/CpuIsolatorTest
[ RUN  ] IsolationFlag/CpuIsolatorTest.ROOT_UserCpuUsage/0
[   OK ] IsolationFlag/CpuIsolatorTest.ROOT_UserCpuUsage/0 (744 ms)
[--] 1 test from IsolationFlag/CpuIsolatorTest (767 ms total)

[--] 1 test from IsolationFlag/MemoryIsolatorTest
[ RUN  ] IsolationFlag/MemoryIsolatorTest.ROOT_MemUsage/0
[   OK ] IsolationFlag/MemoryIsolatorTest.ROOT_MemUsage/0 (731 ms)
[--] 1 test from IsolationFlag/MemoryIsolatorTest (753 ms total)

[--] Global test environment tear-down
[==] 949 tests from 94 test cases ran. (429420 ms total)
[  PASSED  ] 948 tests.
[  FAILED  ] 1 test, listed below:
[  FAILED  ] CommandExecutorCheckTest.CommandCheckTimeout

 1 FAILED TEST
  YOU HAVE 214 DISABLED TESTS

```

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

```
I0328 04:08:42.594311  3856 exec.cpp:236] Executor registered on agent 
adad2926-091b-46da-bc2b-d9283256f266-S0
I0328 04:08:42.598310  8172 executor.cpp:176] Received SUBSCRIBED event
I0328 04:08:42.602340  8172 executor.cpp:180] Subscribed executor on 
winbldsrv-02.zq4gs31qjdiunm1ryi1452nvnh.dx.internal.cloudapp.net
I0328 04:08:42.603307  8172 executor.cpp:176] Received LAUNCH event
I0328 04:08:42.607303  8172 executor.cpp:648] Starting task 
2d20625e-45c9-4512-900d-7f83a2aa494f
I0328 04:08:42.688307  8172 executor.cpp:483] Running 
'D:\DCOS\mesos\src\mesos-containerizer.exe launch '
I0328 04:08:42.722344  8172 executor.cpp:661] Forked command at 4312
I0328 04:08:42.750303   exec.cpp:445] Executor asked to shutdown
I0328 04:08:42.750303  5736 executor.cpp:176] Received SHUTDOWN event
I0328 04:08:42.750303  5736 executor.cpp:758] Shutting down
I0328 04:08:42.750303  5736 executor.cpp:868] Sending SIGTERM to process tree 
at pid 4al.cpp:405] Deactivated framework 
adad2926-091b-46da-bc2b-d9283256f266-
I0328 04:08:42.748447  8028 slave.cpp:3873] Shutting down framework 
adad2926-091b-46da-bc2b-d9283256f266-
I0328 04:08:42.748447  4984 master.cpp:10446] Updating the state of task 
2d20625e-45c9-4512-900d-7f83a2aa494f of framework 
adad2926-091b-46da-bc2b-d9283256f266- (latest state: TASK_KILLED, status 
update state: TASK_KILLED)
I0328 04:08:42.748447  8028 slave.cpp:6566] Shutting down executor 
'2d20625e-45c9-4512-900d-7f83a2aa494f' of framework 
adad2926-091b-46da-bc2b-d9283256f266- at executor(1)@10.3.1.5:62338
I0328 04:08:42.749302  8028 slave.cpp:919] Agent terminating
W0328 04:08:42.749302  8028 slave.cpp:3869] Ignoring shutdown framework 
adad2926-091b-46da-bc2b-d9283256f266- because it is terminating
I0328 04:08:42.751302  4984 master.cpp:10545] Removing task 
2d20625e-45c9-4512-900d-7f83a2aa494f with resources cpus(allocated: *):4; 
mem(allocated: *):2048; disk(allocated: *):1024; ports(allocated: 
*):[31000-32000] of framework adad2926-091b-46da-bc2b-d9283256f266- on 
agent adad2926-091b-46da-bc2b-d9283256f266-S0 at slave(418)@10.3.1.5:62317 
(winbldsrv-02.zq4gs31qjdiunm1ryi1452nvnh.dx.internal.cloudapp.net)
I0328 04:08:42.753310  4988 containerizer.cpp:2338] Destroying container 
72d1f9a3-036a-4461-9735-090203ffc7ae in RUNNING state
I0328 04:08:42.753310  4988 containerizer.cpp:2952] Transitioning the state of 
container 72d1f9a3-036a-4461-9735-090203ffc7ae from RUNNING to DESTROYING
I0328 04:08:42.754333  4984 master.cpp:1295] Agent 
adad2926-091b-46da-bc2b-d9283256f266-S0 at slave(418)@10.3.1.5:62317 
(winbldsrv-02.zq4gs31qjdiunm1ryi1452nvnh.dx.internal.cloudapp.net) disconnected
I0328 04:08:42.754333  4984 master.cpp:3283] Disconnecting agent 
adad2926-091b-46da-bc2b-d9283256f266-S0 at slave(418)@10.3.1.5:62317 

Review Request 66327: WIP: Added the `LIST_RESOURCE_PROVIDER_CONFIGS` agent API call.

[Chun-Hung Hsiao]

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

Review request for mesos, Benjamin Bannier, James DeFelice, and Jie Yu.


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


Repository: mesos


Description
---

`LIST_RESOURCE_PROVIDER_CONFIGS` returns a list of all valid resource
provider configs from the resource provider config directory, except
for the ones that are placed out-of-band.


Diffs
-

  include/mesos/agent/agent.proto adaccb3509fdfc1e7cac9482e93a91e83bc5625d 
  include/mesos/v1/agent/agent.proto 71352a79fe7d28d633a4badceafe18086c1e5ab7 


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


Testing
---


Thanks,

Chun-Hung Hsiao

Re: Review Request 66327: WIP: Added the `LIST_RESOURCE_PROVIDER_CONFIGS` agent API call.

[Mesos Reviewbot Windows]

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



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

Reviews applied: `['66318', '66325', '66326', '66327']`

Failed command: `Start-MesosCITesting`

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

Relevant logs:

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

```
 
d:\dcos\mesos\3rdparty\zookeeper-3.4.8\src\zookeeper-3.4.8\src\c\src\zookeeper.c(3500):
 warning C4996: 'inet_ntoa': Use inet_ntop() or InetNtop() instead or define 
_WINSOCK_DEPRECATED_NO_WARNINGS to disable deprecated API warnings 
[D:\DCOS\mesos\3rdparty\zookeeper-3.4.8\src\zookeeper-3.4.8-build\zookeeper.vcxproj]
 [D:\DCOS\mesos\3rdparty\zookeeper-3.4.8.vcxproj]
 
d:\dcos\mesos\3rdparty\zookeeper-3.4.8\src\zookeeper-3.4.8\src\c\src\zookeeper.c(3501):
 warning C4996: 'sprintf': This function or variable may be unsafe. Consider 
using sprintf_s instead. To disable deprecation, use _CRT_SECURE_NO_WARNINGS. 
See online help for details. 
[D:\DCOS\mesos\3rdparty\zookeeper-3.4.8\src\zookeeper-3.4.8-build\zookeeper.vcxproj]
 [D:\DCOS\mesos\3rdparty\zookeeper-3.4.8.vcxproj]
 
d:\dcos\mesos\3rdparty\zookeeper-3.4.8\src\zookeeper-3.4.8\src\c\src\zookeeper.c(3479):
 warning C4101: 'addrstr': unreferenced local variable 
[D:\DCOS\mesos\3rdparty\zookeeper-3.4.8\src\zookeeper-3.4.8-build\zookeeper.vcxproj]
 [D:\DCOS\mesos\3rdparty\zookeeper-3.4.8.vcxproj]
 
d:\dcos\mesos\3rdparty\zookeeper-3.4.8\src\zookeeper-3.4.8\src\c\src\recordio.c(170):
 warning C4267: '=': conversion from 'size_t' to 'int32_t', possible loss of 
data 
[D:\DCOS\mesos\3rdparty\zookeeper-3.4.8\src\zookeeper-3.4.8-build\zookeeper.vcxproj]
 [D:\DCOS\mesos\3rdparty\zookeeper-3.4.8.vcxproj]
 
d:\dcos\mesos\3rdparty\zookeeper-3.4.8\src\zookeeper-3.4.8\src\c\src\mt_adaptor.c(496):
 warning C4244: '=': conversion from 'time_t' to 'int32_t', possible loss of 
data 
[D:\DCOS\mesos\3rdparty\zookeeper-3.4.8\src\zookeeper-3.4.8-build\zookeeper.vcxproj]
 [D:\DCOS\mesos\3rdparty\zookeeper-3.4.8.vcxproj]
 
d:\dcos\mesos\3rdparty\zookeeper-3.4.8\src\zookeeper-3.4.8\src\c\src\winport.c(256):
 warning C4090: 'function': different 'const' qualifiers 
[D:\DCOS\mesos\3rdparty\zookeeper-3.4.8\src\zookeeper-3.4.8-build\zookeeper.vcxproj]
 [D:\DCOS\mesos\3rdparty\zookeeper-3.4.8.vcxproj]
 
d:\dcos\mesos\3rdparty\zookeeper-3.4.8\src\zookeeper-3.4.8\src\c\src\winport.c(166):
 warning C4716: 'pthread_cond_broadcast': must return a value 
[D:\DCOS\mesos\3rdparty\zookeeper-3.4.8\src\zookeeper-3.4.8-build\zookeeper.vcxproj]
 [D:\DCOS\mesos\3rdparty\zookeeper-3.4.8.vcxproj]
 
d:\dcos\mesos\3rdparty\zookeeper-3.4.8\src\zookeeper-3.4.8\src\c\src\winport.c(205):
 warning C4716: 'pthread_cond_wait': must return a value 
[D:\DCOS\mesos\3rdparty\zookeeper-3.4.8\src\zookeeper-3.4.8-build\zookeeper.vcxproj]
 [D:\DCOS\mesos\3rdparty\zookeeper-3.4.8.vcxproj]
 
d:\dcos\mesos\3rdparty\zookeeper-3.4.8\src\zookeeper-3.4.8\src\c\src\cli.c(124):
 warning C4996: 'fopen': This function or variable may be unsafe. Consider 
using fopen_s instead. To disable deprecation, use _CRT_SECURE_NO_WARNINGS. See 
online help for details. 
[D:\DCOS\mesos\3rdparty\zookeeper-3.4.8\src\zookeeper-3.4.8-build\cli.vcxproj] 
[D:\DCOS\mesos\3rdparty\zookeeper-3.4.8.vcxproj]
 
d:\dcos\mesos\3rdparty\zookeeper-3.4.8\src\zookeeper-3.4.8\src\c\src\cli.c(128):
 warning C4267: 'initializing': conversion from 'size_t' to 'int', possible 
loss of data 
[D:\DCOS\mesos\3rdparty\zookeeper-3.4.8\src\zookeeper-3.4.8-build\cli.vcxproj] 
[D:\DCOS\mesos\3rdparty\zookeeper-3.4.8.vcxproj]
 
d:\dcos\mesos\3rdparty\zookeeper-3.4.8\src\zookeeper-3.4.8\src\c\src\cli.c(279):
 warning C4267: 'function': conversion from 'size_t' to 'int', possible loss of 
data 
[D:\DCOS\mesos\3rdparty\zookeeper-3.4.8\src\zookeeper-3.4.8-build\cli.vcxproj] 
[D:\DCOS\mesos\3rdparty\zookeeper-3.4.8.vcxproj]
 
d:\dcos\mesos\3rdparty\zookeeper-3.4.8\src\zookeeper-3.4.8\src\c\src\cli.c(301):
 warning C4267: 'initializing': conversion from 'size_t' to 'int', possible 
loss of data 
[D:\DCOS\mesos\3rdparty\zookeeper-3.4.8\src\zookeeper-3.4.8-build\cli.vcxproj] 
[D:\DCOS\mesos\3rdparty\zookeeper-3.4.8.vcxproj]
 
d:\dcos\mesos\3rdparty\zookeeper-3.4.8\src\zookeeper-3.4.8\src\c\src\cli.c(368):
 warning C4267: 'function': conversion from 'size_t' to 'int', possible loss of 
data 
[D:\DCOS\mesos\3rdparty\zookeeper-3.4.8\src\zookeeper-3.4.8-build\cli.vcxproj] 
[D:\DCOS\mesos\3rdparty\zookeeper-3.4.8.vcxproj]
 
d:\dcos\mesos\3rdparty\zookeeper-3.4.8\src\zookeeper-3.4.8\src\c\src\cli.c(372):
 warning 

Re: Review Request 66322: Fixed a potential race in `Sequence`.

[Mesos Reviewbot]

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



Patch looks great!

Reviews applied: [66118, 66119, 66120, 65679, 66126, 66143, 66322]

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 March 28, 2018, 1:15 a.m., Meng Zhu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/66322/
> ---
> 
> (Updated March 28, 2018, 1:15 a.m.)
> 
> 
> Review request for mesos, Chun-Hung Hsiao, Greg Mann, and Jie Yu.
> 
> 
> Bugs: MESOS-8741
> https://issues.apache.org/jira/browse/MESOS-8741
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Adding item to sequence is realized by dispatching
> `add()` to the sequence actor. However, this could
> race with the sequence actor termination.
> 
> This patch fixes this by enqueueing the terminate
> message at the end of the message queue.
> 
> Also removed the clock settle in the test `DiscardAll`.
> As the processing of the messages are now guaranteed
> to happen before the actor termination.
> 
> Also added comments to clarify the onDiscard propagation.
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/include/process/sequence.hpp 
> b4d7593221bf225a9e53e3b7f94e45624400078a 
>   3rdparty/libprocess/src/tests/sequence_tests.cpp 
> 43911b6c7a4220a4b8ea1baca191035355817e7b 
> 
> 
> Diff: https://reviews.apache.org/r/66322/diff/1/
> 
> 
> Testing
> ---
> 
> make check
> 
> After modifying test `DiscardAll`, it fails with the old implementation. 
> After modifying the `inject` flag, it passes.
> 
> 
> Thanks,
> 
> Meng Zhu
> 
>

Review Request 66318: Made agent resource provider config API calls idempotent.

[Chun-Hung Hsiao]

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

Review request for mesos, Benjamin Bannier, James DeFelice, and Jie Yu.


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


Repository: mesos


Description
---

This patch adds descriptions to declare the following agent API calls
idempotent:
  - `ADD_RESOURCE_PROVIDER_CONFIG`
  - `UPDATE_RESOURCE_PROVIDER_CONFIG`
  - `REMOVE_RESOURCE_PROVIDER_CONFIG`


Diffs
-

  include/mesos/agent/agent.proto adaccb3509fdfc1e7cac9482e93a91e83bc5625d 
  include/mesos/v1/agent/agent.proto 71352a79fe7d28d633a4badceafe18086c1e5ab7 


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


Testing
---

N/A

The actual implementation will be in the next patch.


Thanks,

Chun-Hung Hsiao

Re: Review Request 66318: Made agent resource provider config API calls idempotent.

[James DeFelice]

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




include/mesos/agent/agent.proto
Line 321 (original), 325 (patched)


nit: there are two spaces between "field" and "will"



include/mesos/agent/agent.proto
Line 327 (original), 331 (patched)


"Note that only..."

I don't think this statement is true. I have e2e tests that add a provider 
and then update it. The provider DID NOT exist at agent startup.



include/mesos/agent/agent.proto
Line 332 (original), 338 (patched)


Maybe return 409 instead of 404 here, if the caller tries to update a 
config that does not exist? Because, you know, 404s can be misleading given the 
convo we had earlier...



include/mesos/agent/agent.proto
Lines 351 (patched)


"Note that only..."

again, I have e2e tests that prove otherwise


- James DeFelice


On March 27, 2018, 11:27 p.m., Chun-Hung Hsiao wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/66318/
> ---
> 
> (Updated March 27, 2018, 11:27 p.m.)
> 
> 
> Review request for mesos, Benjamin Bannier, James DeFelice, and Jie Yu.
> 
> 
> Bugs: MESOS-8742
> https://issues.apache.org/jira/browse/MESOS-8742
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This patch adds descriptions to declare the following agent API calls
> idempotent:
>   - `ADD_RESOURCE_PROVIDER_CONFIG`
>   - `UPDATE_RESOURCE_PROVIDER_CONFIG`
>   - `REMOVE_RESOURCE_PROVIDER_CONFIG`
> 
> 
> Diffs
> -
> 
>   include/mesos/agent/agent.proto adaccb3509fdfc1e7cac9482e93a91e83bc5625d 
>   include/mesos/v1/agent/agent.proto 71352a79fe7d28d633a4badceafe18086c1e5ab7 
> 
> 
> Diff: https://reviews.apache.org/r/66318/diff/1/
> 
> 
> Testing
> ---
> 
> N/A
> 
> The actual implementation will be in the next patch.
> 
> 
> Thanks,
> 
> Chun-Hung Hsiao
> 
>

Re: Review Request 66218: Ensured that agent does not delete volume upon grow or shrink.

[Zhitao Li]

> On March 27, 2018, 12:14 p.m., Chun-Hung Hsiao wrote:
> > src/slave/slave.cpp
> > Lines 4169-4184 (patched)
> > 
> >
> > This is not a `set` operation but a `hashset` operation, so the 
> > comments are incorrect. Could you experiment on adding this operaton in 
> > `stout/hashset.hpp` and see if it works?

Actually this is a bug as `set_diffrence` requirs iteration to have order while 
this does not.

Let me fix it differently in hashset.hpp and list as a precendent. I will also 
check persistent volume content in test patch.


- Zhitao


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


On March 26, 2018, 11:36 p.m., Zhitao Li wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/66218/
> ---
> 
> (Updated March 26, 2018, 11:36 p.m.)
> 
> 
> Review request for mesos, Chun-Hung Hsiao, Gaston Kleiman, and Greg Mann.
> 
> 
> Bugs: MESOS-4965
> https://issues.apache.org/jira/browse/MESOS-4965
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Previously, `slave::syncCheckpointedResources` implementation will
> delete a persistent volume using `Resources::contains` check, which
> could cause a resized volume being deleted. The function was rewritten
> to compare `set_difference` between old and new paths for all persistent
> volumes and perform creation/deletion accordingly.
> 
> 
> Diffs
> -
> 
>   src/slave/slave.cpp e76daabad0d2d68aa42d1da809d4a23459eaaacb 
> 
> 
> Diff: https://reviews.apache.org/r/66218/diff/3/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Zhitao Li
> 
>

Re: Review Request 66051: Implemented operator API to grow and shrink persistent volume.

[Greg Mann]

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




src/master/http.cpp
Lines 1525-1526 (patched)


Not indented far enough.



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


s/have/have the/



src/master/http.cpp
Lines 1544-1546 (patched)


Nit: you can probably remove this newline?



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


s/volume and addition/`volume` and `addition`/



src/master/http.cpp
Lines 1558-1561 (patched)


Not indented far enough.



src/master/http.cpp
Lines 1595-1596 (patched)


Not indented far enough.



src/master/http.cpp
Lines 1614-1616 (patched)


Nit: I would probably remove this newline.



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


Suggestion: "The `volume` field contains the resources required for this 
operation."



src/master/master.cpp
Lines 10798-10837 (original), 10794-10839 (patched)


As discussed in chat, we need to update the agent's resources in the 
allocator for operator-initiated operations.



src/master/validation.cpp
Lines 198-201 (patched)


Let's provide more specific feedback for these cases. Perhaps:

```
  if (!call.grow_volume().has_agent_id()) {
return Error("Expecting 'agent_id' to be present; only agent 
default resources are supported right now");
  }
  
  if (call.grow_volume().has_resource_provider_id()) {
return Error("Expecting 'resource_provider_id' to be unset; only 
agent default resources are supported right now");
  }
```


- Greg Mann


On March 27, 2018, 6:37 a.m., Zhitao Li wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/66051/
> ---
> 
> (Updated March 27, 2018, 6:37 a.m.)
> 
> 
> Review request for mesos, Chun-Hung Hsiao, Gaston Kleiman, and Greg Mann.
> 
> 
> Bugs: MESOS-4965
> https://issues.apache.org/jira/browse/MESOS-4965
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Implemented operator API to grow and shrink persistent volume.
> 
> 
> Diffs
> -
> 
>   src/master/http.cpp 34c9023906eca94965acc994f20e888c1f47b962 
>   src/master/master.hpp 0d9620dd0c232dc1df83477e838eeb7313bf8828 
>   src/master/master.cpp 20692c36c021fdae91de0b156f26fc56cf7c4f45 
>   src/master/validation.cpp 9254cd22e67d9aca53135701575fb0857ff6f7d6 
> 
> 
> Diff: https://reviews.apache.org/r/66051/diff/7/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Zhitao Li
> 
>

Re: Review Request 66126: Refactored agent task launch for better composition [1/2].

[Meng Zhu]

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

(Updated March 27, 2018, 6:05 p.m.)


Review request for mesos, Chun-Hung Hsiao and Greg Mann.


Bugs: MESOS-8617 and MESOS-8624
https://issues.apache.org/jira/browse/MESOS-8617
https://issues.apache.org/jira/browse/MESOS-8624


Repository: mesos


Description
---

This helps to encapsulate a task launch into a single
future which will come in handy when enforcing the task
launch order.

This patch also consolidated the error handling code
in the task launch path.

Affected tests are also updated.


Diffs (updated)
-

  src/slave/slave.hpp 37f0361251524e63d02d251e8a03901812b8affb 
  src/slave/slave.cpp e76daabad0d2d68aa42d1da809d4a23459eaaacb 
  src/tests/mock_slave.hpp 8ec2b65a75f774d970ef6507df8b0c02fda5f2fd 
  src/tests/mock_slave.cpp 8ec55b6aa84dd8b1c3743cbcdc246311585d0f63 
  src/tests/slave_tests.cpp 0d7d9726ee39e4c588ea5eb8a54a73fbe1249353 


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

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


Testing
---

make check


Thanks,

Meng Zhu

Re: Review Request 66051: Implemented operator API to grow and shrink persistent volume.

[Chun-Hung Hsiao]

> On March 28, 2018, 1:58 a.m., Greg Mann wrote:
> > src/master/master.cpp
> > Lines 11881-11887 (original), 11885-11890 (patched)
> > 
> >
> > Currently, it looks like `Slave::usedResources` is the same as the 
> > "allocated resources" from that agent.
> > 
> > When a GROW or SHRINK operation is initiated by an operator, its 
> > consumed resource is in a unique state in the master while that operation 
> > is pending: the volume is not allocated, but it is "used" in some sense.
> > 
> > One place where we use `Slave::usedResources` is when validating 
> > DESTROY operations: 
> > https://github.com/apache/mesos/blob/21e4b45b388d0b272236b1c58313569f8a1d1fc8/src/master/master.cpp#L4841
> > 
> > This means if the master receives a DESTROY call for a persistent 
> > volume while there is a pending operator-initiated GROW_VOLUME call for 
> > that same volume (i.e. the operation was forwarded to the agent for 
> > processing but the master hasn't heard back yet), we would accept the 
> > DESTROY operation and forward it to the agent. Is this what we want?
> > 
> > This brings something else to mind: would it be possible for the 
> > allocator to send out an offer containing the consumed resource of a 
> > GROW_VOLUME or SHRINK_VOLUME operation, while the operation is pending on 
> > the agent? This seems bad, but I believe it's possible. I think this may be 
> > the first time we have a set of resources which we don't want the allocator 
> > to offer for a period of time, but which are not currently allocated to 
> > some framework/role. I wonder if we need some new tools in the allocator to 
> > handle this?

Yeah this sounds bad. Would it help if we remove the consumed resources from 
the allocator and put it back after receiving the terminal status update?


- Chun-Hung


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


On March 27, 2018, 6:37 a.m., Zhitao Li wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/66051/
> ---
> 
> (Updated March 27, 2018, 6:37 a.m.)
> 
> 
> Review request for mesos, Chun-Hung Hsiao, Gaston Kleiman, and Greg Mann.
> 
> 
> Bugs: MESOS-4965
> https://issues.apache.org/jira/browse/MESOS-4965
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Implemented operator API to grow and shrink persistent volume.
> 
> 
> Diffs
> -
> 
>   src/master/http.cpp 34c9023906eca94965acc994f20e888c1f47b962 
>   src/master/master.hpp 0d9620dd0c232dc1df83477e838eeb7313bf8828 
>   src/master/master.cpp 20692c36c021fdae91de0b156f26fc56cf7c4f45 
>   src/master/validation.cpp 9254cd22e67d9aca53135701575fb0857ff6f7d6 
> 
> 
> Diff: https://reviews.apache.org/r/66051/diff/7/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Zhitao Li
> 
>

Re: Review Request 66322: Fixed a potential race in `Sequence`.

[Mesos Reviewbot Windows]

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



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

Reviews applied: `['66118', '66119', '66120', '65679', '66126', '66143', 
'66322']`

Failed command: `Start-MesosCITesting`

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

Relevant logs:

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

```
[   OK ] Endpoint/SlaveEndpointTest.NoAuthorizer/2 (106 ms)
[--] 9 tests from Endpoint/SlaveEndpointTest (996 ms total)

[--] 2 tests from ContainerizerType/DefaultContainerDNSFlagTest
[ RUN  ] ContainerizerType/DefaultContainerDNSFlagTest.ValidateFlag/0
[   OK ] ContainerizerType/DefaultContainerDNSFlagTest.ValidateFlag/0 (36 
ms)
[ RUN  ] ContainerizerType/DefaultContainerDNSFlagTest.ValidateFlag/1
[   OK ] ContainerizerType/DefaultContainerDNSFlagTest.ValidateFlag/1 (36 
ms)
[--] 2 tests from ContainerizerType/DefaultContainerDNSFlagTest (73 ms 
total)

[--] 1 test from IsolationFlag/CpuIsolatorTest
[ RUN  ] IsolationFlag/CpuIsolatorTest.ROOT_UserCpuUsage/0
[   OK ] IsolationFlag/CpuIsolatorTest.ROOT_UserCpuUsage/0 (811 ms)
[--] 1 test from IsolationFlag/CpuIsolatorTest (834 ms total)

[--] 1 test from IsolationFlag/MemoryIsolatorTest
[ RUN  ] IsolationFlag/MemoryIsolatorTest.ROOT_MemUsage/0
[   OK ] IsolationFlag/MemoryIsolatorTest.ROOT_MemUsage/0 (836 ms)
[--] 1 test from IsolationFlag/MemoryIsolatorTest (857 ms total)

[--] Global test environment tear-down
[==] 949 tests from 94 test cases ran. (437776 ms total)
[  PASSED  ] 948 tests.
[  FAILED  ] 1 test, listed below:
[  FAILED  ] CommandExecutorCheckTest.CommandCheckTimeout

 1 FAILED TEST
  YOU HAVE 214 DISABLED TESTS

```

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

```
I0328 02:31:26.019050  9324 master.cpp:10446] Updating the state of task 
cd024448-e06c-408e-af5d-1e6910914d24 of framework 
721e27b7-323c-4565-8ae8-e1dcbcbc75d1- (latest state: TASK_KILLED, status 
update state: TASK_KILLED)
I0328 02:31:26.019050  7904 slave.cpp:3892] Shutting down framework 
721e27b7-323c-4565-8ae8-e1dcbcbc75d1-
I0328 02:31:26.019050  7904 slave.cpp:6585] Shutting down executor 
'cd024448-e06c-408e-af5d-1e6910914d24' of framework 
721e27b7-323c-4565-8ae8-e1dcbcbc75d1- at executor(1)@10.3.1.5:59976
I032I0328 02:31:25.831063  4468 exec.cpp:162] Version: 1.6.0
I0328 02:31:25.857064 10932 exec.cpp:236] Executor registered on agent 
721e27b7-323c-4565-8ae8-e1dcbcbc75d1-S0
I0328 02:31:25.862030  6444 executor.cpp:176] Received SUBSCRIBED event
I0328 02:31:25.866063  6444 executor.cpp:180] Subscribed executor on 
winbldsrv-02.zq4gs31qjdiunm1ryi1452nvnh.dx.internal.cloudapp.net
I0328 02:31:25.866063  6444 executor.cpp:176] Received LAUNCH event
I0328 02:31:25.871065  6444 executor.cpp:648] Starting task 
cd024448-e06c-408e-af5d-1e6910914d24
I0328 02:31:25.954066  6444 executor.cpp:483] Running 
'D:\DCOS\mesos\src\mesos-containerizer.exe launch '
I0328 02:31:25.988071  6444 executor.cpp:661] Forked command at 6332
I0328 02:31:26.021034  9448 exec.cpp:445] Executor asked to shutdown
I0328 02:31:26.021034  7732 executor.cpp:176] Received SHUTDOWN event
I0328 02:31:26.021034  7732 executor.cpp:758] Shutting down
I0328 02:31:26.021034  7732 executor.cpp:868] Sending SIGTERM to process tree 
at pid 68 02:31:26.020408  1852 slave.cpp:919] Agent terminating
W0328 02:31:26.021034  1852 slave.cpp:3888] Ignoring shutdown framework 
721e27b7-323c-4565-8ae8-e1dcbcbc75d1- because it is terminating
I0328 02:31:26.022033  9324 master.cpp:10545] Removing task 
cd024448-e06c-408e-af5d-1e6910914d24 with resources cpus(allocated: *):4; 
mem(allocated: *):2048; disk(allocated: *):1024; ports(allocated: 
*):[31000-32000] of framework 721e27b7-323c-4565-8ae8-e1dcbcbc75d1- on 
agent 721e27b7-323c-4565-8ae8-e1dcbcbc75d1-S0 at slave(418)@10.3.1.5:59955 
(winbldsrv-02.zq4gs31qjdiunm1ryi1452nvnh.dx.internal.cloudapp.net)
I0328 02:31:26.024032  3996 containerizer.cpp:2338] Destroying container 
33b9544f-1e45-45bc-ad0a-eacb95b1824e in RUNNING state
I0328 02:31:26.024032  3996 containerizer.cpp:2952] Transitioning the state of 
container 33b9544f-1e45-45bc-ad0a-eacb95b1824e from RUNNING to DESTROYING
I0328 02:31:26.024032  9324 master.cpp:1295] Agent 
721e27b7-323c-4565-8ae8-e1dcbcbc75d1-S0 at slave(418)@10.3.1.5:59955 
(winbldsrv-02.zq4gs31qjdiunm1ryi1452nvnh.dx.internal.cloudapp.net) disconnected
I0328 02:31:26.025032  9324 master.cpp:3283] Disconnecting agent 
721e27b7-323c-4565-8ae8-e1dcbcbc75d1-S0 at 

Re: Review Request 66318: Made agent resource provider config API calls idempotent.

[Chun-Hung Hsiao]

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

(Updated March 28, 2018, 3:22 a.m.)


Review request for mesos, Benjamin Bannier, James DeFelice, and Jie Yu.


Changes
---

Addressed James' comments.


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


Repository: mesos


Description
---

This patch adds descriptions to declare the following agent API calls
idempotent:
  - `ADD_RESOURCE_PROVIDER_CONFIG`
  - `UPDATE_RESOURCE_PROVIDER_CONFIG`
  - `REMOVE_RESOURCE_PROVIDER_CONFIG`


Diffs (updated)
-

  include/mesos/agent/agent.proto adaccb3509fdfc1e7cac9482e93a91e83bc5625d 
  include/mesos/v1/agent/agent.proto 71352a79fe7d28d633a4badceafe18086c1e5ab7 


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

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


Testing
---

N/A

The actual implementation will be in the next patch.


Thanks,

Chun-Hung Hsiao

Review Request 66322: Fixed a potential race in `Sequence`.

[Meng Zhu]

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

Review request for mesos, Chun-Hung Hsiao, Greg Mann, and Jie Yu.


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


Repository: mesos


Description
---

Adding item to sequence is realized by dispatching
`add()` to the sequence actor. However, this could
race with the sequence actor termination.

This patch fixes this by enqueueing the terminate
message at the end of the message queue.

Also removed the clock settle in the test `DiscardAll`.
As the processing of the messages are now guaranteed
to happen before the actor termination.

Also added comments to clarify the onDiscard propagation.


Diffs
-

  3rdparty/libprocess/include/process/sequence.hpp 
b4d7593221bf225a9e53e3b7f94e45624400078a 
  3rdparty/libprocess/src/tests/sequence_tests.cpp 
43911b6c7a4220a4b8ea1baca191035355817e7b 


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


Testing
---

make check

After modifying test `DiscardAll`, it fails with the old implementation. After 
modifying the `inject` flag, it passes.


Thanks,

Meng Zhu

Review Request 66326: Added tests for agent resource provider API idempotency.

[Chun-Hung Hsiao]

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

Review request for mesos, Benjamin Bannier, James DeFelice, and Jie Yu.


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


Repository: mesos


Description
---

This patche adds tests to verify that adding, updating and removing the
same resource provider config will return 200 OK without triggering any
resource provider launch/termination.


Diffs
-

  src/tests/agent_resource_provider_config_api_tests.cpp 
53b8cb8f9463dec5fa684e64c61a3ad5b6c88104 


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


Testing
---

sudo make check


Thanks,

Chun-Hung Hsiao

Review Request 66325: Implemented idempotency for agent resource provider config API calls.

[Chun-Hung Hsiao]

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

Review request for mesos, Benjamin Bannier, James DeFelice, and Jie Yu.


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


Repository: mesos


Description
---

`ADD_RESOURCE_PROVIDER_CONFIG`, `UPDATE_RESOURCE_PROVIDER_CONFIG` and
`REMOVE_RESOURCE_PROVIDER_CONFIG` now return 200 for identical
repetivite calls.


Diffs
-

  src/resource_provider/daemon.cpp d0a8186da80a1bffbb71f524e639bc7d75ef6243 
  src/slave/http.cpp 65081c95c888a8236aafdfc627a7aa4e9a72b65d 


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


Testing
---

make

NOTE: Test `*/AgentResourceProviderConfigApiTest.RemoveNotFound` won't pass 
since the semantics of `REMOVE_RESOURCE_PROVIDER_CONFIG` is changed.
Tests are added in the next patch.


Thanks,

Chun-Hung Hsiao

Re: Review Request 66318: Made agent resource provider config API calls idempotent.

[Mesos Reviewbot Windows]

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



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

Reviews applied: `['66318']`

Failed command: `Start-MesosCITesting`

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

Relevant logs:

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

```
[   OK ] Endpoint/SlaveEndpointTest.NoAuthorizer/2 (106 ms)
[--] 9 tests from Endpoint/SlaveEndpointTest (1034 ms total)

[--] 2 tests from ContainerizerType/DefaultContainerDNSFlagTest
[ RUN  ] ContainerizerType/DefaultContainerDNSFlagTest.ValidateFlag/0
[   OK ] ContainerizerType/DefaultContainerDNSFlagTest.ValidateFlag/0 (31 
ms)
[ RUN  ] ContainerizerType/DefaultContainerDNSFlagTest.ValidateFlag/1
[   OK ] ContainerizerType/DefaultContainerDNSFlagTest.ValidateFlag/1 (49 
ms)
[--] 2 tests from ContainerizerType/DefaultContainerDNSFlagTest (83 ms 
total)

[--] 1 test from IsolationFlag/CpuIsolatorTest
[ RUN  ] IsolationFlag/CpuIsolatorTest.ROOT_UserCpuUsage/0
[   OK ] IsolationFlag/CpuIsolatorTest.ROOT_UserCpuUsage/0 (744 ms)
[--] 1 test from IsolationFlag/CpuIsolatorTest (771 ms total)

[--] 1 test from IsolationFlag/MemoryIsolatorTest
[ RUN  ] IsolationFlag/MemoryIsolatorTest.ROOT_MemUsage/0
[   OK ] IsolationFlag/MemoryIsolatorTest.ROOT_MemUsage/0 (730 ms)
[--] 1 test from IsolationFlag/MemoryIsolatorTest (755 ms total)

[--] Global test environment tear-down
[==] 949 tests from 94 test cases ran. (424151 ms total)
[  PASSED  ] 948 tests.
[  FAILED  ] 1 test, listed below:
[  FAILED  ] CommandExecutorCheckTest.CommandCheckTimeout

 1 FAILED TEST
  YOU HAVE 214 DISABLED TESTS

```

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

```
I0328 00:52:22.522769 10496 master.cpp:10446] Updating the state of task 
38ba006b-fb82-47a8-8792-0fba6f99ae4a of framework 
2982a78e-810d-4839-8d2e-ee618792c50a- (latest state: TASK_KILLED, status 
update state: TASK_KILLED)
I0328 00:52:22.522769 12672 slave.cpp:3873] Shutting down framework 
2982a78e-810d-4839-8d2e-ee618792c50a-
I0328 00:52:22.522769 12672 slave.cpp:6566] Shutting down executor 
'38ba006b-fb82-47a8-8792-0fba6f99ae4a' of framework 
2982a78e-810d-4839-8d2e-ee618792c50a- at executor(1)@10.3.1.8:53140
I0328 00:52:22.523767 14360 slave.cpp:919] Agent terminating
W0328 00:52:22.523767 14360 slave.cpp:3869] Ignoring shutdown framework 
2982a78e-810d-4839-8d2e-ee618792c50a-I0328 00:52:22.365787  2380 exec.cpp:162] 
Version: 1.6.0
I0328 00:52:22.390769  8796 exec.cpp:236] Executor registered on agent 
2982a78e-810d-4839-8d2e-ee618792c50a-S0
I0328 00:52:22.394785  5828 executor.cpp:176] Received SUBSCRIBED event
I0328 00:52:22.400787  5828 executor.cpp:180] Subscribed executor on 
winbldsrv-01.zq4gs31qjdiunm1ryi1452nvnh.dx.internal.cloudapp.net
I0328 00:52:22.400787  5828 executor.cpp:176] Received LAUNCH event
I0328 00:52:22.404789  5828 executor.cpp:648] Starting task 
38ba006b-fb82-47a8-8792-0fba6f99ae4a
I0328 00:52:22.484781  5828 executor.cpp:483] Running 
'D:\DCOS\mesos\src\mesos-containerizer.exe launch '
I0328 00:52:22.496755  5828 executor.cpp:661] Forked command at 120
I0328 00:52:22.523767 13756 exec.cpp:445] Executor asked to shutdown
I0328 00:52:22.524767 14148 executor.cpp:176] Received SHUTDOWN event
I0328 00:52:22.524767 14148 executor.cpp:758] Shutting down
I0328 00:52:22.524767 14148 executor.cpp:868] Sending SIGTERM to process tree 
at pid 12 because it is terminating
I0328 00:52:22.524767 10496 master.cpp:10545] Removing task 
38ba006b-fb82-47a8-8792-0fba6f99ae4a with resources cpus(allocated: *):4; 
mem(allocated: *):2048; disk(allocated: *):1024; ports(allocated: 
*):[31000-32000] of framework 2982a78e-810d-4839-8d2e-ee618792c50a- on 
agent 2982a78e-810d-4839-8d2e-ee618792c50a-S0 at slave(418)@10.3.1.8:53119 
(winbldsrv-01.zq4gs31qjdiunm1ryi1452nvnh.dx.internal.cloudapp.net)
I0328 00:52:22.527761 10496 master.cpp:1295] Agent 
2982a78e-810d-4839-8d2e-ee618792c50a-S0 at slave(418)@10.3.1.8:53119 
(winbldsrv-01.zq4gs31qjdiunm1ryi1452nvnh.dx.internal.cloudapp.net) disconnected
I0328 00:52:22.527761 10496 master.cpp:3283] Disconnecting agent 
2982a78e-810d-4839-8d2e-ee618792c50a-S0 at slave(418)@10.3.1.8:53119 
(winbldsrv-01.zq4gs31qjdiunm1ryi1452nvnh.dx.internal.cloudapp.net)
I0328 00:52:22.527761 10496 master.cpp:3302] Deactivating agent 
2982a78e-810d-4839-8d2e-ee618792c50a-S0 at slave(418)@10.3.1.8:53119 
(winbldsrv-01.zq4gs31qjdiunm1ryi1452nvnh.dx.internal.cloudapp.net)
I0328 00:52:22.527761  5560 containerizer.cpp:2338] 

Review Request 66308: Delayed construction of the agent's resource provider manager.

[Benjamin Bannier]

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

Review request for mesos, Jie Yu and Jan Schlicht.


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


Repository: mesos


Description
---

By delaying the construction of the agent's resource provider manager
we prepare for a following patch introducing a dependency on the
resource provider manager on the agent's ID.

Depending on whether the agent was able to recover an agent ID from
its log or still needs to obtain on in a first registration with the
master, we can only construct the resource provider manager after
different points in the initialization of the agent. To capture the
common code we introduce a helper function performing the necessary
steps.


Diffs
-

  src/slave/slave.hpp 37f0361251524e63d02d251e8a03901812b8affb 
  src/slave/slave.cpp e76daabad0d2d68aa42d1da809d4a23459eaaacb 
  src/tests/resource_provider_manager_tests.cpp 
d947bd037190e6b7ea7b2277b5fbe47816878de4 


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


Testing
---

`make check`


Thanks,

Benjamin Bannier

Review Request 66311: Implement recovery of resource provider manager.

[Benjamin Bannier]

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

Review request for mesos, Jie Yu and Jan Schlicht.


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


Repository: mesos


Description
---

This patch adjusts the control flow of the resource provider manager
so that we can in the future make use of persisted resource provider
information.


Diffs
-

  src/resource_provider/manager.cpp 2d3ffcdd01671f81729fe538348f2ca4e555eb4f 
  src/resource_provider/registrar.hpp 39f45b0a2a7c35bfe72a9305f5248409e4a3ef45 
  src/resource_provider/registrar.cpp 9eb49f1327a0b598c5464a29a09ee286d7018f09 
  src/tests/resource_provider_manager_tests.cpp 
d947bd037190e6b7ea7b2277b5fbe47816878de4 


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


Testing
---

`make check`


Thanks,

Benjamin Bannier

Review Request 66307: Added implicit conversion from nullptr to Owned and Shared.

[Benjamin Bannier]

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

Review request for mesos, Alexander Rukletsov, Jie Yu, and Jan Schlicht.


Repository: mesos


Description
---

To communicate absence of a reference we usually use `nullptr` inside
smart pointers. This patch adds smart pointer constructors from
`nullptr_t` which mirror conversion present in `std::unique_ptr` and
`std::shared_ptr` simplifying such use cases.


Diffs
-

  3rdparty/libprocess/include/process/owned.hpp 
cc149e78165d42f1243168c34fc64fd121c6c83f 
  3rdparty/libprocess/include/process/shared.hpp 
6eb69439d21897398157b2d400a00c49ea720e68 


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


Testing
---

`make check`


Thanks,

Benjamin Bannier

Review Request 66310: Passed on registrar when constructing resource provider manager.

[Benjamin Bannier]

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

Review request for mesos, Jie Yu and Jan Schlicht.


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


Repository: mesos


Description
---

In order to support recovering resource provider manager information
in the future, we need to adjust the construction of the manager to be
able to consume a registrar.

This patch lays the groundwork by adjusting interfaces and their
usage; we will make use of the passed on information in a following
patch.


Diffs
-

  src/resource_provider/manager.hpp bc017fa3998b780cec82718cabe2a8b470a66db2 
  src/resource_provider/manager.cpp 2d3ffcdd01671f81729fe538348f2ca4e555eb4f 
  src/slave/slave.cpp e76daabad0d2d68aa42d1da809d4a23459eaaacb 
  src/tests/resource_provider_manager_tests.cpp 
d947bd037190e6b7ea7b2277b5fbe47816878de4 


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


Testing
---

`make check`


Thanks,

Benjamin Bannier

Review Request 66309: Externalize creation of resource provider manager backing storage.

[Benjamin Bannier]

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

Review request for mesos, Jie Yu and Jan Schlicht.


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


Repository: mesos


Description
---

This patch changes the way the storage backing an agent's resource
provider registrar is created: while before we created it implicitly
when constructing the registrar, we now consume storage passed on
construction.

Being able to explicitly inject the used storage simplifies testing.


Diffs
-

  src/resource_provider/registrar.hpp 39f45b0a2a7c35bfe72a9305f5248409e4a3ef45 
  src/resource_provider/registrar.cpp 9eb49f1327a0b598c5464a29a09ee286d7018f09 
  src/tests/resource_provider_manager_tests.cpp 
d947bd037190e6b7ea7b2277b5fbe47816878de4 


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


Testing
---

`make check`


Thanks,

Benjamin Bannier

Re: Review Request 66052: Added new operator API to grow and shrink persistent volume.

[Greg Mann]

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




include/mesos/master/master.proto
Lines 187 (patched)


To keep with the existing pattern, in this file you should use 'slave_id'. 
You can use 'agent_id' in the V1 proto.

The comment above should be changed to reference 'slave_id' as well.



include/mesos/master/master.proto
Lines 194 (patched)


s/size/the size/



include/mesos/master/master.proto
Lines 196 (patched)


s/disk/disks/

Here and above.


- Greg Mann


On March 26, 2018, 4:54 p.m., Zhitao Li wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/66052/
> ---
> 
> (Updated March 26, 2018, 4:54 p.m.)
> 
> 
> Review request for mesos, Chun-Hung Hsiao, Gaston Kleiman, and Greg Mann.
> 
> 
> Bugs: MESOS-4965
> https://issues.apache.org/jira/browse/MESOS-4965
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> The same API could be used in the future to grow or shrink CSI volumes,
> but currently only persistent volumes are supported.
> 
> 
> Diffs
> -
> 
>   include/mesos/master/master.proto aa63904a33290a3beda162bbc9f44b56ab04a1e7 
>   include/mesos/v1/master/master.proto 
> ddb28f96b2a3a439bb9a829995a9a3015f65ba43 
> 
> 
> Diff: https://reviews.apache.org/r/66052/diff/4/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Zhitao Li
> 
>

Re: Review Request 66001: MESOS-6575: Add soft limit and kill to disk/xfs.

[James Peach]

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




src/slave/containerizer/mesos/isolators/xfs/disk.cpp
Lines 30 (patched)


Looke like we don't need this include?



src/slave/containerizer/mesos/isolators/xfs/disk.cpp
Lines 42 (patched)


I don't think you need these. You'll probably get rid of the explicit 
`ControlFlow` usage, and you only use `Continue` once.



src/slave/containerizer/mesos/isolators/xfs/disk.cpp
Lines 171 (patched)


I think that this formulation makes the logic easier to read:

```
xfs::QuotaPolicy quotaPolicy = xfs::QuotaPolicy::ACCOUNTING;

if (flags.enforce_container_disk_quota) {
  quotaPolicy = flags.xfs_kill_containers
? xfs::QuotaPolicy::ENFORCING_ACTIVE
: xfs::QuotaPolicy::ENFORCING_PASSIVE;
}
```



src/slave/containerizer/mesos/isolators/xfs/disk.cpp
Line 188 (original)


Keep this newline. See the [new line 
style](https://github.com/apache/mesos/blob/master/docs/c%2B%2B-style-guide.md#empty-lines)



src/slave/containerizer/mesos/isolators/xfs/disk.cpp
Lines 319 (patched)


Double newline here.



src/slave/containerizer/mesos/isolators/xfs/disk.cpp
Line 306 (original), 325 (patched)


Keep the existing failure. The continerizer shouldn't be asking for a 
container we don't have.



src/slave/containerizer/mesos/isolators/xfs/disk.cpp
Lines 333 (patched)


This starts a loop for each container. You want to do what 
`network/ports.cpp` does and start a single loop once in 
`XfsDiskIsolatorProcess::initialize`.



src/slave/containerizer/mesos/isolators/xfs/disk.cpp
Lines 341 (patched)


This should be simplified down to:
```
process::loop(
  self(),
  [=]() {
return process::after(watchInterval);
  },
  [=]() {
check();
return Continue();
  }
```

Running the loop against `self()` causes it to dispatch the (serialized) 
lambdas against the XFS process. This means that we can safely capture the 
`this` pointer and use it to access member variables.

After refactoring, you might find that you want to just move the code from 
`::check()` into the body of the loop.



src/slave/containerizer/mesos/isolators/xfs/disk.cpp
Lines 346 (patched)


All we need to do in this function is
```
return infos[containerId]->limitation.future();
```

The background check loop will satisfy the future if necessary.



src/slave/containerizer/mesos/isolators/xfs/disk.cpp
Lines 361 (patched)


Just keep the newlines the way they were.



src/slave/containerizer/mesos/isolators/xfs/disk.cpp
Lines 368 (patched)


Move this into the case statement block.



src/slave/containerizer/mesos/isolators/xfs/disk.cpp
Lines 400 (patched)


Here's how this should be formatted:
```
  Bytes hardLimit = needed.get();

  // The purpose behind adding to the hard limit is so that the soft 
limit
  // can be exceeded thereby allowing for us to check if the limit has 
been
  // reached without allowing the process to allocate too much beyond 
the
  // desired limit.
  if (quotaPolicy == xfs::QuotaPolicy::ENFORCING_ACTIVE) {
hardLimit += Megabytes(10);
  }

  Try status = xfs::setProjectQuota(
  info->directory, info->projectId, hardLimit, needed.get());

```

Note the 4-space indent after the dangling `(`.



src/slave/containerizer/mesos/isolators/xfs/disk.cpp
Line 356 (original), 409 (patched)


Let's format this as:
"Set quota ... to " << softLimit << "/" << hardLimit;



src/slave/containerizer/mesos/isolators/xfs/disk.cpp
Line 362 (original), 414 (patched)


`needed` is still the allocated quota so you don't need to use `hardLimit` 
here.



src/slave/containerizer/mesos/isolators/xfs/disk.cpp
Lines 420 (patched)


It's unfortunately verbose, but we should do this:

```
  CHECK_NE(
  static_cast(xfs::QuotaPolicy::ACCOUNTING),
  

Re: Review Request 66049: Added offer operation to grow and shrink persistent volumes.

[Greg Mann]

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




include/mesos/mesos.proto
Lines 1970 (patched)


s/size/the size/


- Greg Mann


On March 26, 2018, 4:53 p.m., Zhitao Li wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/66049/
> ---
> 
> (Updated March 26, 2018, 4:53 p.m.)
> 
> 
> Review request for mesos, Chun-Hung Hsiao, Gaston Kleiman, and Greg Mann.
> 
> 
> Bugs: MESOS-4965
> https://issues.apache.org/jira/browse/MESOS-4965
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added offer operation to grow and shrink persistent volumes.
> 
> 
> Diffs
> -
> 
>   include/mesos/mesos.proto 676f0b090cad7ebf59eb32556f17ff8b5f247907 
>   include/mesos/v1/mesos.proto 10d506517c9f098374ab0c8f4dcfda42e1896c95 
> 
> 
> Diff: https://reviews.apache.org/r/66049/diff/4/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Zhitao Li
> 
>

Review Request 66313: Fixed an oversubscription test for agent registration backoff.

[Benjamin Bannier]

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

Review request for mesos, Alexander Rukletsov and Till Toenshoff.


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


Repository: mesos


Description
---

In the `OversubscriptionTest.ForwardUpdateSlaveMessage` test we
observe a single `UpdateSlaveMessage` to make sure the agent has fully
recovered. This message is sent from the resource provider-capable
agent to communicate its (empty) set of resource providers after
registration.

Since message was sent with a running clock, it is possible that the
agent encounters a timeout of its registration backoff timers. The
agent would then register agent, triggering another similar message
which is not expected in the test.

This patch adjusts the test to always run with paused clock
eliminating this particular scenario.


Diffs
-

  src/tests/oversubscription_tests.cpp 47c51e3d035eb5143d00efb466675eb02236b52e 


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


Testing
---

`make check`

Before this patch this test would fail after a handful of iterations; with this 
patch I was able to execute this test >110,000 times without issues.


Thanks,

Benjamin Bannier

Re: Review Request 66173: Add test for new `disk/xfs` kill functionality.

[James Peach]

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



Running the test, I get this failure:
```
../../src/tests/containerizer/xfs_quota_tests.cpp:533: Failure
Failed to wait 15secs for killedStatus
```

Which indicates that the task doesn't get killed.

We should also add the following tests:

1. Verify that `flags.xfs_kill_containers` doesn't kill containers that don't 
violate there limits. You can do this by snsuring that you can kill the 
container after guaranteeing that at least one disk space usage check has 
elapsed (this is an argument for keeping the isolator `check` functio, since 
you can expect it to be called).
2. Add soft limit checks to QuotaGetSet.
3. Add soft limit checks to QuotaLimit.


src/tests/containerizer/xfs_quota_tests.cpp
Line 147 (original), 147 (patched)


Since we are now pausing the clock, we need to make sure that it is resumed 
so that the `losetup` exec works correctly:

```
// Make sure we resume the clock so that we can wait on the
// `losetup` process.
if (Clock::paused()) {
  Clock::resume();
}
```



src/tests/containerizer/xfs_quota_tests.cpp
Lines 457 (patched)


Double newline above here.



src/tests/containerizer/xfs_quota_tests.cpp
Lines 460 (patched)


Need to update this comment for accuracy.



src/tests/containerizer/xfs_quota_tests.cpp
Lines 469 (patched)


Newline before and after this to make it stand out.



src/tests/containerizer/xfs_quota_tests.cpp
Lines 498 (patched)


We can just sleep:
```
"dd if=/dev/zero of=file bs=1048576 count=2 && sleep 10"
```

and update the comment.



src/tests/containerizer/xfs_quota_tests.cpp
Lines 517 (patched)


So that you don't have to wait for the full check interval, you can advance 
the clock:
```
// Create TaskInfo ...

Clock::pause();

// Expect TASK_RUNNING ...

Clock::advance(flags.container_disk_watch_interval);
Clock::settle();
Clock::resume();

// Expect more stuff ...

```



src/tests/containerizer/xfs_quota_tests.cpp
Lines 524 (patched)


Here we should check that the resource in the limitation is what we expect:

```
 EXPECT_EQ(TaskStatus::SOURCE_SLAVE, killedStatus->source());
 EXPECT_EQ(
 TaskStatus::REASON_CONTAINER_LIMITATION_DISK, killedStatus->reason());
 
 ASSERT_TRUE(killedStatus->has_limitation()) << 
JSON::protobuf(killedStatus.get());
 
 Resources limit = Resources(killedStatus->limitation().resources());
 
 // Expect that we were limited on a single disk resource that represents
 // the amount of disk that the task consumed.
 EXPECT_EQ(1u, limit.size());
 EXPECT_SOME_EQ(Megabytes(2), limit.disk());
 
```



src/tests/containerizer/xfs_quota_tests.cpp
Lines 528 (patched)


Double newline after here.


- James Peach


On March 22, 2018, 2:11 p.m., Harold Dost wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/66173/
> ---
> 
> (Updated March 22, 2018, 2:11 p.m.)
> 
> 
> Review request for mesos and James Peach.
> 
> 
> Bugs: MESOS-6575
> https://issues.apache.org/jira/browse/MESOS-6575
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> MESOS-6575
> 
> 
> Diffs
> -
> 
>   src/tests/containerizer/xfs_quota_tests.cpp 
> 64c3e1c3f0bc435897626cb0a13bc19c7cb1a4fe 
> 
> 
> Diff: https://reviews.apache.org/r/66173/diff/2/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Harold Dost
> 
>

Review Request 66314: Fix 3rdparty build commands for FreeBSD.

[David Forsythe]

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

Review request for mesos.


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


Repository: mesos


Description
---

Fix 3rdparty build commands for FreeBSD.


Diffs
-

  3rdparty/CMakeLists.txt 2b63b58f7d6a88c9986b746283dcfa79b7bcb270 
  cmake/CompilationConfigure.cmake 64cc56ee4208afe05df0f28af5890157e4c7d82c 


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


Testing
---

cmake --build on FreeBSD


Thanks,

David Forsythe

Re: Review Request 66050: Implemented grow and shrink of persistent volumes.

[Greg Mann]

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




src/common/resources_utils.cpp
Lines 203-205 (patched)


This is already done in the validation code - do we need to do it again 
here? Ditto below.



src/common/resources_utils.cpp
Lines 207-208 (patched)


Suggestion:

"To grow a persistent volume, we consume the original volume and the 
additional resource and convert into a single volume with the new size."



src/common/resources_utils.cpp
Lines 223 (patched)


s/consume/consume the/



src/common/resources_utils.cpp
Lines 232-237 (patched)


Could you provide a comment explaining the two cases here?



src/common/resources_utils.cpp
Lines 243 (patched)


s/shrinked/shrunk/



src/common/resources_utils.cpp
Lines 696-698 (patched)


No period at the end of error messages. Here and below.



src/master/master.cpp
Lines 4925-4928 (patched)


Not indented far enough.



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


s/have/have the/

Here and below.



src/master/master.cpp
Lines 4945-4946 (patched)


Indented too far.



src/master/master.cpp
Lines 4949-4953 (patched)


Could you update the indentation on these `drop()` calls to be consistent? 
I'm fine with either
```
drop(
framework,
etc...
```
or
```
drop(framework,
 etc...
```



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


s/Operation/operation/

Here and below.



src/master/master.cpp
Lines 4973-4974 (patched)


Fits on one line.



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


s/GROW_VOLUME/SHRINK_VOLUME/



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


s/with subtract/subtracting/



src/master/validation.cpp
Lines 2341 (patched)


"Addition in grow_volume must be greater than zero"



src/master/validation.cpp
Lines 2345-2346 (patched)


Fits on one line.



src/master/validation.cpp
Lines 2355-2356 (patched)


Fits on one line.



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


Is this error message accurate? Looks like it should be something like "Not 
a valid resource: "



src/master/validation.cpp
Lines 2387 (patched)


"The 'subtract' field in 'shrink_volume' must be greater than zero"



src/master/validation.cpp
Lines 2392 (patched)


"The 'subtract' field in 'shrink_volume' must be smaller than the 
persistent volume's size"



src/master/validation.cpp
Lines 2395 (patched)


Is this test necessary, given the above check for `shrink.volume().scalar() 
<= shrink.subtract()`?



src/master/validation.cpp
Lines 2411-2412 (patched)


Fits on one line.


- Greg Mann


On March 27, 2018, 6:30 a.m., Zhitao Li wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/66050/
> ---
> 
> (Updated March 27, 2018, 6:30 a.m.)
> 
> 
> Review request for mesos, Chun-Hung Hsiao, Gaston Kleiman, and Greg Mann.
> 
> 
> Bugs: MESOS-4965
> https://issues.apache.org/jira/browse/MESOS-4965
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> The new offer operations are implemented as non-speculative operations,
> which means they cannot be chained with previous offer operations which
> depend on results of each other.
> 
> 
> Diffs
> -
> 
>   src/common/protobuf_utils.cpp 2acdc798200e82d14651b1edcc5f83e174477d53 
>   src/common/resources_utils.cpp 9be01c1abd48264e308960f35cc7c2ee8a367518 
>   src/master/master.cpp 20692c36c021fdae91de0b156f26fc56cf7c4f45 
>   src/master/validation.hpp 7c129ceb929596acbb64d37025e055661277e6bf 
>   

Re: Review Request 66050: Implemented grow and shrink of persistent volumes.

[Chun-Hung Hsiao]

> On March 27, 2018, 7:14 p.m., Greg Mann wrote:
> > src/common/resources_utils.cpp
> > Lines 203-205 (patched)
> > 
> >
> > This is already done in the validation code - do we need to do it again 
> > here? Ditto below.

IMO from the perspective of designing a utility function, it seems appropriate 
to me to return an error here, and we do a CHECK at the call site. It is also 
consistent with other operations such as `LAUNCH`.


> On March 27, 2018, 7:14 p.m., Greg Mann wrote:
> > src/common/resources_utils.cpp
> > Lines 243 (patched)
> > 
> >
> > s/shrinked/shrunk/

Oops this is on me lol.


- Chun-Hung


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


On March 27, 2018, 6:30 a.m., Zhitao Li wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/66050/
> ---
> 
> (Updated March 27, 2018, 6:30 a.m.)
> 
> 
> Review request for mesos, Chun-Hung Hsiao, Gaston Kleiman, and Greg Mann.
> 
> 
> Bugs: MESOS-4965
> https://issues.apache.org/jira/browse/MESOS-4965
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> The new offer operations are implemented as non-speculative operations,
> which means they cannot be chained with previous offer operations which
> depend on results of each other.
> 
> 
> Diffs
> -
> 
>   src/common/protobuf_utils.cpp 2acdc798200e82d14651b1edcc5f83e174477d53 
>   src/common/resources_utils.cpp 9be01c1abd48264e308960f35cc7c2ee8a367518 
>   src/master/master.cpp 20692c36c021fdae91de0b156f26fc56cf7c4f45 
>   src/master/validation.hpp 7c129ceb929596acbb64d37025e055661277e6bf 
>   src/master/validation.cpp 9254cd22e67d9aca53135701575fb0857ff6f7d6 
>   src/slave/slave.cpp e76daabad0d2d68aa42d1da809d4a23459eaaacb 
>   src/tests/mesos.hpp 01fd1c2bf21ef96dbab0769a1c1c92d7e3b60032 
>   src/tests/persistent_volume_tests.cpp 
> 924d8458e54e34a49c99593482b5908c5f7c7a48 
>   src/tests/reservation_tests.cpp 8d8e9c8390e65187269bd194bb322bbdff88e0bd 
> 
> 
> Diff: https://reviews.apache.org/r/66050/diff/6/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Zhitao Li
> 
>

Re: Review Request 66313: Fixed an oversubscription test for agent registration backoff.

[Mesos Reviewbot]

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



Patch looks great!

Reviews applied: [66313]

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 March 27, 2018, 11:32 a.m., Benjamin Bannier wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/66313/
> ---
> 
> (Updated March 27, 2018, 11:32 a.m.)
> 
> 
> Review request for mesos, Alexander Rukletsov and Till Toenshoff.
> 
> 
> Bugs: MESOS-8733
> https://issues.apache.org/jira/browse/MESOS-8733
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> In the `OversubscriptionTest.ForwardUpdateSlaveMessage` test we
> observe a single `UpdateSlaveMessage` to make sure the agent has fully
> recovered. This message is sent from the resource provider-capable
> agent to communicate its (empty) set of resource providers after
> registration.
> 
> Since message was sent with a running clock, it is possible that the
> agent encounters a timeout of its registration backoff timers. The
> agent would then register agent, triggering another similar message
> which is not expected in the test.
> 
> This patch adjusts the test to always run with paused clock
> eliminating this particular scenario.
> 
> 
> Diffs
> -
> 
>   src/tests/oversubscription_tests.cpp 
> 47c51e3d035eb5143d00efb466675eb02236b52e 
> 
> 
> Diff: https://reviews.apache.org/r/66313/diff/1/
> 
> 
> Testing
> ---
> 
> `make check`
> 
> Before this patch this test would fail after a handful of iterations; with 
> this patch I was able to execute this test >110,000 times without issues.
> 
> 
> Thanks,
> 
> Benjamin Bannier
> 
>

Re: Review Request 66292: Validated that all tasks in the same group have same max_duration.

[James Peach]

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



What's the rationale for this? A common use of task groups is to specify 
sidecars for a primary task container, in which case it is most natural to have 
the `max_duration` only on the task container.

- James Peach


On March 26, 2018, 9:49 p.m., Zhitao Li wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/66292/
> ---
> 
> (Updated March 26, 2018, 9:49 p.m.)
> 
> 
> Review request for mesos, Jason Lai and James Peach.
> 
> 
> Bugs: MESOS-8725
> https://issues.apache.org/jira/browse/MESOS-8725
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Validated that all tasks in the same group have same max_duration.
> 
> 
> Diffs
> -
> 
>   src/master/validation.cpp 9254cd22e67d9aca53135701575fb0857ff6f7d6 
> 
> 
> Diff: https://reviews.apache.org/r/66292/diff/1/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Zhitao Li
> 
>

Re: Review Request 66307: Added implicit conversion from nullptr to Owned and Shared.

[Alexander Rukletsov]

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


Ship it!




Ship It!

- Alexander Rukletsov


On March 27, 2018, 3:34 p.m., Benjamin Bannier wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/66307/
> ---
> 
> (Updated March 27, 2018, 3:34 p.m.)
> 
> 
> Review request for mesos, Alexander Rukletsov, Jie Yu, and Jan Schlicht.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> To communicate absence of a reference we usually use `nullptr` inside
> smart pointers. This patch adds smart pointer constructors from
> `nullptr_t` which mirror conversion present in `std::unique_ptr` and
> `std::shared_ptr` simplifying such use cases.
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/include/process/owned.hpp 
> cc149e78165d42f1243168c34fc64fd121c6c83f 
>   3rdparty/libprocess/include/process/shared.hpp 
> 6eb69439d21897398157b2d400a00c49ea720e68 
> 
> 
> Diff: https://reviews.apache.org/r/66307/diff/1/
> 
> 
> Testing
> ---
> 
> `make check`
> 
> 
> Thanks,
> 
> Benjamin Bannier
> 
>

Re: Review Request 66314: Fix 3rdparty build commands for FreeBSD.

[Mesos Reviewbot Windows]

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



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

Reviews applied: `['66314']`

Failed command: `Start-MesosCITesting`

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

Relevant logs:

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

```
[   OK ] Endpoint/SlaveEndpointTest.NoAuthorizer/2 (101 ms)
[--] 9 tests from Endpoint/SlaveEndpointTest (994 ms total)

[--] 2 tests from ContainerizerType/DefaultContainerDNSFlagTest
[ RUN  ] ContainerizerType/DefaultContainerDNSFlagTest.ValidateFlag/0
[   OK ] ContainerizerType/DefaultContainerDNSFlagTest.ValidateFlag/0 (32 
ms)
[ RUN  ] ContainerizerType/DefaultContainerDNSFlagTest.ValidateFlag/1
[   OK ] ContainerizerType/DefaultContainerDNSFlagTest.ValidateFlag/1 (36 
ms)
[--] 2 tests from ContainerizerType/DefaultContainerDNSFlagTest (70 ms 
total)

[--] 1 test from IsolationFlag/CpuIsolatorTest
[ RUN  ] IsolationFlag/CpuIsolatorTest.ROOT_UserCpuUsage/0
[   OK ] IsolationFlag/CpuIsolatorTest.ROOT_UserCpuUsage/0 (813 ms)
[--] 1 test from IsolationFlag/CpuIsolatorTest (841 ms total)

[--] 1 test from IsolationFlag/MemoryIsolatorTest
[ RUN  ] IsolationFlag/MemoryIsolatorTest.ROOT_MemUsage/0
[   OK ] IsolationFlag/MemoryIsolatorTest.ROOT_MemUsage/0 (725 ms)
[--] 1 test from IsolationFlag/MemoryIsolatorTest (752 ms total)

[--] Global test environment tear-down
[==] 949 tests from 94 test cases ran. (417969 ms total)
[  PASSED  ] 948 tests.
[  FAILED  ] 1 test, listed below:
[  FAILED  ] CommandExecutorCheckTest.CommandCheckTimeout

 1 FAILED TEST
  YOU HAVE 214 DISABLED TESTS

```

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

```
I0327 21:51:35.280153 11208 master.cpp:10446] Updating the state of task 
730f23b7-015e-4282-9c84-44c1626d6675 of framework 
368c5872-5c09-420e-8442-a9c9bd2b6ae5- (latest state: TASK_KILLED, status 
update state: TASK_KILLED)
I0327 21:51:35.281184 14524 slave.cpp:3873] Shutting down framework 
368c5872-5c09-420e-8442-a9c9bd2b6ae5-
I0327 21:51:35.281184 14524 slave.cpp:6566] Shutting down executor 
'730f23b7-015e-4282-9c84-44c1626d6675' of framework 
368c5872-5c09-420e-8442-a9c9bd2b6ae5- at executor(1)@10.3.1.8:63953
I0327 21:51:35.28218932 slave.cpp:919] Agent terminating
W0327 21:51:35.28218932 slave.cpp:3869] Ignoring shutdown framework 
368c5872-5c09-420e-8442-a9c9bd2b6ae5- because it is teI0327 21:51:35.103152 
13644 exec.cpp:162] Version: 1.6.0
I0327 21:51:35.135192 15492 exec.cpp:236] Executor registered on agent 
368c5872-5c09-420e-8442-a9c9bd2b6ae5-S0
I0327 21:51:35.139149  6088 executor.cpp:176] Received SUBSCRIBED event
I0327 21:51:35.144176  6088 executor.cpp:180] Subscribed executor on 
winbldsrv-01.zq4gs31qjdiunm1ryi1452nvnh.dx.internal.cloudapp.net
I0327 21:51:35.144176  6088 executor.cpp:176] Received LAUNCH event
I0327 21:51:35.150198  6088 executor.cpp:648] Starting task 
730f23b7-015e-4282-9c84-44c1626d6675
I0327 21:51:35.239214  6088 executor.cpp:483] Running 
'D:\DCOS\mesos\src\mesos-containerizer.exe launch '
I0327 21:51:35.252205  6088 executor.cpp:661] Forked command at 5816
I0327 21:51:35.283177 10168 exec.cpp:445] Executor asked to shutdown
I0327 21:51:35.283177 15792 executor.cpp:176] Received SHUTDOWN event
I0327 21:51:35.283177 15792 executor.cpp:758] Shutting down
I0327 21:51:35.283177 15792 executor.cpp:868] Sending SIGTERM to process tree 
at pid 5rminating
I0327 21:51:35.283177 11208 master.cpp:10545] Removing task 
730f23b7-015e-4282-9c84-44c1626d6675 with resources cpus(allocated: *):4; 
mem(allocated: *):2048; disk(allocated: *):1024; ports(allocated: 
*):[31000-32000] of framework 368c5872-5c09-420e-8442-a9c9bd2b6ae5- on 
agent 368c5872-5c09-420e-8442-a9c9bd2b6ae5-S0 at slave(418)@10.3.1.8:63932 
(winbldsrv-01.zq4gs31qjdiunm1ryi1452nvnh.dx.internal.cloudapp.net)
I0327 21:51:35.286193 11208 master.cpp:1295] Agent 
368c5872-5c09-420e-8442-a9c9bd2b6ae5-S0 at slave(418)@10.3.1.8:63932 
(winbldsrv-01.zq4gs31qjdiunm1ryi1452nvnh.dx.internal.cloudapp.net) disconnected
I0327 21:51:35.286193 11208 master.cpp:3283] Disconnecting agent 
368c5872-5c09-420e-8442-a9c9bd2b6ae5-S0 at slave(418)@10.3.1.8:63932 
(winbldsrv-01.zq4gs31qjdiunm1ryi1452nvnh.dx.internal.cloudapp.net)
I0327 21:51:35.286193 11208 master.cpp:3302] Deactivating agent 
368c5872-5c09-420e-8442-a9c9bd2b6ae5-S0 at slave(418)@10.3.1.8:63932 
(winbldsrv-01.zq4gs31qjdiunm1ryi1452nvnh.dx.internal.cloudapp.net)
I0327 21:51:35.286193 13976 containerizer.cpp:2338]