Re: Review Request 70248: Adapted the unversioned `VolumeCapability`.

2019-03-21 Thread Mesos Reviewbot Windows 

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



FAIL: Failed to apply the dependent review: 70222.

Failed command: `python.exe .\support\apply-reviews.py -n -r 70222`

All the build artifacts available at: 
http://dcos-win.westus2.cloudapp.azure.com/artifacts/mesos-reviewbot-testing/2993/mesos-review-70248

Relevant logs:

- 
[apply-review-70222.log](http://dcos-win.westus2.cloudapp.azure.com/artifacts/mesos-reviewbot-testing/2993/mesos-review-70248/logs/apply-review-70222.log):

```
error: patch failed: src/csi/v0_volume_manager.cpp:423
error: src/csi/v0_volume_manager.cpp: patch does not apply
error: patch failed: src/csi/v0_volume_manager_process.hpp:118
error: src/csi/v0_volume_manager_process.hpp: patch does not apply
error: patch failed: src/resource_provider/storage/provider.cpp:74
error: src/resource_provider/storage/provider.cpp: patch does not apply
error: patch failed: src/resource_provider/storage/provider_process.hpp:52
error: src/resource_provider/storage/provider_process.hpp: patch does not apply
```

- Mesos Reviewbot Windows


On March 20, 2019, 9:40 p.m., Chun-Hung Hsiao wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/70248/
> ---
> 
> (Updated March 20, 2019, 9:40 p.m.)
> 
> 
> Review request for mesos, Benjamin Bannier, James DeFelice, and Jan Schlicht.
> 
> 
> Bugs: MESOS-9625
> https://issues.apache.org/jira/browse/MESOS-9625
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This patch makes the disk profile adaptor module API and the CSI volume
> state protobuf to use the unversioned `VolumeCapability`, so the profile
> adaptors and the volume state checkpoints can be upgraded to support CSI
> v1 in a backward compatible way.
> 
> The `DiskProfileMapping` protobuf used by `UriDiskProfileAdaptor`,
> however, still uses CSI v0 `VolumeCapability` for now, and devolve it
> to the unversioned protobuf during profile translation.
> 
> 
> Diffs
> -
> 
>   include/mesos/resource_provider/storage/disk_profile_adaptor.hpp 
> 739585b84245a85649d36ddde3a6086a5cf309cc 
>   src/csi/state.proto b5ccf165255864072a7f7276ea50035d14d571f4 
>   src/csi/v0_volume_manager.hpp PRE-CREATION 
>   src/csi/v0_volume_manager.cpp PRE-CREATION 
>   src/csi/v0_volume_manager_process.hpp PRE-CREATION 
>   src/csi/volume_manager.hpp PRE-CREATION 
>   src/resource_provider/state.proto eb56a9129f522eee4295fa2fd2fe0beced4c31a2 
>   src/resource_provider/storage/disk_profile_adaptor.cpp 
> b4551a681addcf70f3fe93c7e5ffbb4a6434c50a 
>   src/resource_provider/storage/uri_disk_profile_adaptor.hpp 
> 7e610d30110f22cd6ee0745979ef8ced3a07765e 
>   src/resource_provider/storage/uri_disk_profile_adaptor.cpp 
> cb574be2a4b4e443248b2001f822d739e5bbe7b9 
>   src/tests/disk_profile_adaptor_tests.cpp 
> 0ccbc79d7ffb82a68b7ed5aeab930bcd8e6e770e 
> 
> 
> Diff: https://reviews.apache.org/r/70248/diff/2/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Chun-Hung Hsiao
> 
>

Re: Review Request 70217: Cleanup volume creation, validation and deletion for SLRP.

2019-03-21 Thread Chun-Hung Hsiao 

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

(Updated March 22, 2019, 6:19 a.m.)


Review request for mesos, Benjamin Bannier, Jie Yu, and Jan Schlicht.


Changes
---

Restructured the refactoring.


Summary (updated)
-

Cleanup volume creation, validation and deletion for SLRP.


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


Repository: mesos


Description (updated)
---

This patch introduces methods for volume creation, validation and
deletion that conform to `VolumeManager`'s public interface in SLRP, and
cleans up SLRP based on these functions. They will be moved out from
SLRP to v0 `VolumeManager` later.

Specifically, volume deletion now supports deleting untracked volumes.


Diffs (updated)
-

  src/resource_provider/storage/provider.cpp 
fea623c292158deb1b4b4b9ab1ac208031471519 
  src/resource_provider/storage/provider_process.hpp 
a5536b3d735e01eb1c4dc52d0602d973155f3c93 


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

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


Testing
---

make check


Thanks,

Chun-Hung Hsiao

Re: Review Request 70215: Cleanup volume attaching and publishing for SLRP.

2019-03-21 Thread Chun-Hung Hsiao 

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

(Updated March 22, 2019, 6:18 a.m.)


Review request for mesos, Benjamin Bannier, Jie Yu, and Jan Schlicht.


Changes
---

Addressed Benjamin's comments and restructured the refactoring.


Summary (updated)
-

Cleanup volume attaching and publishing for SLRP.


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


Repository: mesos


Description (updated)
---

This patch introduces methods for volume attaching and publishing that
conform to `VolumeManager`'s public interface in SLRP, and cleans up
SLRP based on these functions. They will be moved out from SLRP to v0
`VolumeManager` later.

Volume attaching and publishing are implemented with internal helpers,
each individually deals with one steady and two transient states, and
makes a proper CSI call to achieve its goal state. If the given volume
is not in the designed state, it would recursively call other helpers
to transition the volume to the designed state first.

These helper functions are serialized through the volume's own sequence
to avoid racing operations on the same volume.


Diffs (updated)
-

  src/resource_provider/storage/provider.cpp 
fea623c292158deb1b4b4b9ab1ac208031471519 
  src/resource_provider/storage/provider_process.hpp 
a5536b3d735e01eb1c4dc52d0602d973155f3c93 


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

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


Testing
---

make check


Thanks,

Chun-Hung Hsiao

Re: Review Request 70248: Adapted the unversioned `VolumeCapability`.

2019-03-21 Thread Mesos Reviewbot Windows 

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



FAIL: Failed to apply the dependent review: 70217.

Failed command: `python.exe .\support\apply-reviews.py -n -r 70217`

All the build artifacts available at: 
http://dcos-win.westus2.cloudapp.azure.com/artifacts/mesos-reviewbot-testing/2991/mesos-review-70248

Relevant logs:

- 
[apply-review-70217.log](http://dcos-win.westus2.cloudapp.azure.com/artifacts/mesos-reviewbot-testing/2991/mesos-review-70248/logs/apply-review-70217.log):

```
error: patch failed: src/csi/v0_volume_manager.cpp:32
error: src/csi/v0_volume_manager.cpp: patch does not apply
error: patch failed: src/csi/v0_volume_manager_process.hpp:134
error: src/csi/v0_volume_manager_process.hpp: patch does not apply
```

- Mesos Reviewbot Windows


On March 20, 2019, 9:40 p.m., Chun-Hung Hsiao wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/70248/
> ---
> 
> (Updated March 20, 2019, 9:40 p.m.)
> 
> 
> Review request for mesos, Benjamin Bannier, James DeFelice, and Jan Schlicht.
> 
> 
> Bugs: MESOS-9625
> https://issues.apache.org/jira/browse/MESOS-9625
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This patch makes the disk profile adaptor module API and the CSI volume
> state protobuf to use the unversioned `VolumeCapability`, so the profile
> adaptors and the volume state checkpoints can be upgraded to support CSI
> v1 in a backward compatible way.
> 
> The `DiskProfileMapping` protobuf used by `UriDiskProfileAdaptor`,
> however, still uses CSI v0 `VolumeCapability` for now, and devolve it
> to the unversioned protobuf during profile translation.
> 
> 
> Diffs
> -
> 
>   include/mesos/resource_provider/storage/disk_profile_adaptor.hpp 
> 739585b84245a85649d36ddde3a6086a5cf309cc 
>   src/csi/state.proto b5ccf165255864072a7f7276ea50035d14d571f4 
>   src/csi/v0_volume_manager.hpp PRE-CREATION 
>   src/csi/v0_volume_manager.cpp PRE-CREATION 
>   src/csi/v0_volume_manager_process.hpp PRE-CREATION 
>   src/csi/volume_manager.hpp PRE-CREATION 
>   src/resource_provider/state.proto eb56a9129f522eee4295fa2fd2fe0beced4c31a2 
>   src/resource_provider/storage/disk_profile_adaptor.cpp 
> b4551a681addcf70f3fe93c7e5ffbb4a6434c50a 
>   src/resource_provider/storage/uri_disk_profile_adaptor.hpp 
> 7e610d30110f22cd6ee0745979ef8ced3a07765e 
>   src/resource_provider/storage/uri_disk_profile_adaptor.cpp 
> cb574be2a4b4e443248b2001f822d739e5bbe7b9 
>   src/tests/disk_profile_adaptor_tests.cpp 
> 0ccbc79d7ffb82a68b7ed5aeab930bcd8e6e770e 
> 
> 
> Diff: https://reviews.apache.org/r/70248/diff/2/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Chun-Hung Hsiao
> 
>

Re: Review Request 70216: Cleanup the recovery logic for refactoring SLRP.

2019-03-21 Thread Chun-Hung Hsiao 

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

(Updated March 22, 2019, 6:14 a.m.)


Review request for mesos, Benjamin Bannier, Jie Yu, and Jan Schlicht.


Changes
---

Addressed Benjamin's comments and restructure the refactoring. Now it's done in 
SLRP first.


Summary (updated)
-

Cleanup the recovery logic for refactoring SLRP.


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


Repository: mesos


Description (updated)
---

In addition to perform volume state recovery, the `recoverVolumes`
function also recovers service manager and preparing services now. The
whole logic will be moved out from SLRP to v0 `VolumeManager` later.

During volume state recovery, we no longer recover all volumes to steady
states, since transient states are properly handled in SLRP. To simplify
the recovery logic, a `publishVolume` method that conforms to the volume
manager's `publishVolume` is introduced.


Diffs (updated)
-

  src/resource_provider/storage/provider.cpp 
fea623c292158deb1b4b4b9ab1ac208031471519 
  src/resource_provider/storage/provider_process.hpp 
a5536b3d735e01eb1c4dc52d0602d973155f3c93 


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

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


Testing
---

make check


Thanks,

Chun-Hung Hsiao

Review Request 70267: Added more arithmetic operations in class `ResourceQuantities`.

2019-03-21 Thread Meng Zhu 

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

Review request for mesos and Benjamin Mahler.


Repository: mesos


Description
---

Added more arithmetic operations in class `ResourceQuantities`.


Diffs
-

  src/common/resource_quantities.hpp e85ff4f3c9201a5409f292b0a43b99c73341a3d0 
  src/common/resource_quantities.cpp d9042e9c0fa5e90e619470830ca430a31b2ce5bc 


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


Testing
---

make check


Thanks,

Meng Zhu

Review Request 70268: Removed `limits` from `QuotaInfo` and `QuotaRequest` protobuf.

2019-03-21 Thread Meng Zhu 

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

Review request for mesos and Benjamin Mahler.


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


Repository: mesos


Description
---

`Limits` was added as an experimental field and the
related calls were not implementation complete.
`QuotaConfig` has been added where limits can be
specified. This patch cleans up the experimental
field and related validation and tests.


Diffs
-

  include/mesos/quota/quota.proto 34d4b697a78464f6cbc0ec2c3c6e117690dc2917 
  include/mesos/v1/quota/quota.proto 8a6a64a5e6d7b7986f33cf3ef85609dbce1ff363 
  src/master/quota.cpp 62b788f504256b2e81070c714428f8dd26d604b5 
  src/tests/master_quota_tests.cpp 3e7a94cad94b64faf01e73f880281d24098fadbd 


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


Testing
---

make check


Thanks,

Meng Zhu

Review Request 70266: Removed a lambda to simplify `__allocate()`.

2019-03-21 Thread Meng Zhu 

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

Review request for mesos and Benjamin Mahler.


Repository: mesos


Description
---

Removed a lambda to simplify `__allocate()`.


Diffs
-

  src/master/allocator/mesos/hierarchical.cpp 
c9cf704f046c0f08131ac3f2fee54f4b649eabff 


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


Testing
---

make check


Thanks,

Meng Zhu

Re: Review Request 70214: Added skeleton code for v0 `VolumeManager`.

2019-03-21 Thread Chun-Hung Hsiao 

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

(Updated March 22, 2019, 6:12 a.m.)


Review request for mesos, Benjamin Bannier, Jie Yu, and Jan Schlicht.


Changes
---

Addressed Benjamin's comments.


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


Repository: mesos


Description
---

Added skeleton code for v0 `VolumeManager`.


Diffs (updated)
-

  src/CMakeLists.txt 3397c3b1d4e8a7900b2e5f870679cc7aa30b4be2 
  src/Makefile.am d451d7cabe3bf5d4f5039cfac5de1b03ef891d07 
  src/csi/v0_volume_manager.hpp PRE-CREATION 
  src/csi/v0_volume_manager.cpp PRE-CREATION 
  src/csi/v0_volume_manager_process.hpp PRE-CREATION 
  src/csi/volume_manager.cpp PRE-CREATION 


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

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


Testing
---

make check


Thanks,

Chun-Hung Hsiao

Re: Review Request 70243: Improved operation reconciliation for unsubscribed resource providers.

2019-03-21 Thread Gastón Kleiman 

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




include/mesos/resource_provider/resource_provider.proto
Lines 101-104 (patched)


I think we should document somewhere (either protos or in the 
reconciliation handlers, or maybe in both places?) the difference in semantics, 
i.e., exactly which cases are handled differently depending on who initiated 
the reconciliation request.



src/master/master.cpp
Lines 9684-9693 (patched)


Nit: I think the following version would be more concise:

```
ReconcileOperationsMessage::Operation* forwardedOperation = 
reconciliationMessage.add_operations();
forwardedOperation->mutable_operation_id()
  ->CopyFrom(operation.operation_id());
if (resourceProviderId.isSome()) {
  forwardedOperation->mutable_resource_provider_id()
->CopyFrom(resourceProviderId.get());
}
```



src/slave/slave.cpp
Lines 4524-4525 (patched)


Nit:

```
  } else if (operationId.isSome() &&
operationIds.contains(operationId.get())) {
```

But I like Joseph's suggestion better =).


- Gastón Kleiman


On March 21, 2019, 3:26 p.m., Greg Mann wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/70243/
> ---
> 
> (Updated March 21, 2019, 3:26 p.m.)
> 
> 
> Review request for mesos, Benjamin Bannier, Chun-Hung Hsiao, Gastón Kleiman, 
> and Joseph Wu.
> 
> 
> Bugs: MESOS-9318
> https://issues.apache.org/jira/browse/MESOS-9318
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This patch updates the operation reconciliation pipeline to
> forward some framework-initiated reconciliation requests to
> the agent. In cases where an explicit reconciliation request
> specifies an operation which is not recognized on a resource
> provider which is also not recognized, the master will
> forward the request to the agent so that the resource
> provider manager can satisfy the request based on whether or
> not the resource provider has been seen before.
> 
> 
> Diffs
> -
> 
>   include/mesos/resource_provider/resource_provider.proto 
> 5ea9e2009209b1609619874ebd63cb1e2e698434 
>   src/master/master.cpp 9c4a9e83da94535873d72c902835f229c4f96320 
>   src/messages/messages.proto 633dddbaa874550f7f0d9513c608ed75b18059a8 
>   src/resource_provider/manager.cpp 7d3338ea7fbf330a25416f848db7742ad1bea52f 
>   src/slave/slave.cpp 36424f89a8c1f183febabcc9582975dd21213c25 
> 
> 
> Diff: https://reviews.apache.org/r/70243/diff/2/
> 
> 
> Testing
> ---
> 
> Testing details at the end of this chain.
> 
> 
> Thanks,
> 
> Greg Mann
> 
>

Re: Review Request 70244: Added new operation reconciliation tests.

2019-03-21 Thread Mesos Reviewbot 

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



Patch looks great!

Reviews applied: [70264, 70242, 70243, 70244]

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

- Mesos Reviewbot


On March 20, 2019, 12:47 p.m., Greg Mann wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/70244/
> ---
> 
> (Updated March 20, 2019, 12:47 p.m.)
> 
> 
> Review request for mesos, Benjamin Bannier, Chun-Hung Hsiao, Gastón Kleiman, 
> and Joseph Wu.
> 
> 
> Bugs: MESOS-9318
> https://issues.apache.org/jira/browse/MESOS-9318
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This patch adds two new operation reconciliation tests,
> `OperationOnUnsubscribedProvider` and
> `FrameworkReconciliationRaceWithUpdateSlaveMessage`, to
> probe scenarios relevant to MESOS-9648 and MESOS-9318.
> 
> 
> Diffs
> -
> 
>   src/tests/operation_reconciliation_tests.cpp 
> 6a815ad694e2a608ce324715c920833f825793a0 
> 
> 
> Diff: https://reviews.apache.org/r/70244/diff/2/
> 
> 
> Testing
> ---
> 
> `make check`
> `bin/mesos-tests.sh 
> --gtest_filter="*FrameworkReconciliationRaceWithUpdateSlaveMessage*" 
> --gtest_repeat=-1 --gtest_break_on_failure`
> `bin/mesos-tests.sh --gtest_filter="*OperationOnUnsubscribedProvider*" 
> --gtest_repeat=-1 --gtest_break_on_failure`
> 
> 
> Thanks,
> 
> Greg Mann
> 
>

Re: Review Request 70244: Added new operation reconciliation tests.

2019-03-21 Thread Joseph Wu 

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



+1 on the first test.  Some questions on the second one.


src/tests/operation_reconciliation_tests.cpp
Lines 1576-1578 (patched)


Extra newline.  Although I think this is a copy-pasta error in this file or 
in similar tests.



src/tests/operation_reconciliation_tests.cpp
Lines 1706-1707 (patched)


I'm not seeing a clear race in the test body.

The `UpdateSlaveMessage` right before the operation reconciliation is 
properly waited for before the reconciliation starts.  So the master would 
already know about the agent, and the agent's RP.



src/tests/operation_reconciliation_tests.cpp
Lines 1889-1903 (patched)


Is it possible to intercept the message forwarded between master and agent? 
 There doesn't seem to be strong evidence that the master actually asked the 
agent for the latest status.


- Joseph Wu


On March 20, 2019, 12:47 p.m., Greg Mann wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/70244/
> ---
> 
> (Updated March 20, 2019, 12:47 p.m.)
> 
> 
> Review request for mesos, Benjamin Bannier, Chun-Hung Hsiao, Gastón Kleiman, 
> and Joseph Wu.
> 
> 
> Bugs: MESOS-9318
> https://issues.apache.org/jira/browse/MESOS-9318
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This patch adds two new operation reconciliation tests,
> `OperationOnUnsubscribedProvider` and
> `FrameworkReconciliationRaceWithUpdateSlaveMessage`, to
> probe scenarios relevant to MESOS-9648 and MESOS-9318.
> 
> 
> Diffs
> -
> 
>   src/tests/operation_reconciliation_tests.cpp 
> 6a815ad694e2a608ce324715c920833f825793a0 
> 
> 
> Diff: https://reviews.apache.org/r/70244/diff/2/
> 
> 
> Testing
> ---
> 
> `make check`
> `bin/mesos-tests.sh 
> --gtest_filter="*FrameworkReconciliationRaceWithUpdateSlaveMessage*" 
> --gtest_repeat=-1 --gtest_break_on_failure`
> `bin/mesos-tests.sh --gtest_filter="*OperationOnUnsubscribedProvider*" 
> --gtest_repeat=-1 --gtest_break_on_failure`
> 
> 
> Thanks,
> 
> Greg Mann
> 
>

Re: Review Request 70244: Added new operation reconciliation tests.

2019-03-21 Thread Mesos Reviewbot Windows 

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



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

Reviews applied: `['70264', '70242', '70243', '70244']`

Failed command: `Start-MesosCITesting`

All the build artifacts available at: 
http://dcos-win.westus2.cloudapp.azure.com/artifacts/mesos-reviewbot-testing/2990/mesos-review-70244

Relevant logs:

- 
[mesos-tests.log](http://dcos-win.westus2.cloudapp.azure.com/artifacts/mesos-reviewbot-testing/2990/mesos-review-70244/logs/mesos-tests.log):

```
I0321 23:25:52.451818 23624 master.cpp:1295] Agent 
a9c136cd-584b-49f5-9574-f1954599c2fc-S0 at slave(502)@192.10.1.6:59694 
(windows-02.chtsmhjxogyevckjfayqqcnjda.xx.internal.cloudapp.net) disconnected
I0321 23:25:52.452865 23624 master.cpp:3330] Disconnecting agent 
a9c136cd-584b-49f5-9574-f1954599c2fc-S0 at slave(502)@192.10.1.6:59694 
(windows-02.chtsmhjxogyevckjfayqqcnjda.xx.internal.cloudapp.net)
I0321 23:25:52.452865 23624 master.cpp:3349] Deactivating agent 
a9c136cd-584b-49f5-9574-f1954599c2fc-S0 at slave(502)@192.10.1.6:59694 
(windows-02.chtsmhjxogyevckjfayqqcnjda.xx.internal.cloudapp.net)
I0321 23:25:52.452865 23084 hierarchical.cpp:391] Removed framework 
a9c136cd-584b-49f5-9574-f1954599c2fc-
I0321 23:25:52.452865 23084 hierarchical.cpp:828] Agent 
a9c136cd-584b-49f5-9574-f1954599c2fc-S0 deactivated
I0321 23:25:52.453860 23624 containerizer.cpp:2576] Destroying container 
5230119f-ea8d-4a8e-90b7-c5e3b74c727e in RUNNING stat[   OK ] 
IsolationFlag/MemoryIsolatorTest.ROOT_MemUsage/0 (690 ms)
[--] 1 test from IsolationFlag/MemoryIsolatorTest (707 ms total)

[--] Global test environment tear-down
[==] 1128 tests from 107 test cases ran. (540082 ms total)
[  PASSED  ] 1126 tests.
[  FAILED  ] 2 tests, listed below:
[  FAILED  ] 
ContentType/OperationReconciliationTest.FrameworkReconciliationRaceWithUpdateSlaveMessage/0,
 where GetParam() = application/x-protobuf
[  FAILED  ] 
ContentType/OperationReconciliationTest.FrameworkReconciliationRaceWithUpdateSlaveMessage/1,
 where GetParam() = application/json

 2 FAILED TESTS
  YOU HAVE 231 DISABLED TESTS

e
I0321 23:25:52.453860 23624 containerizer.cpp:3278] Transitioning the state of 
container 5230119f-ea8d-4a8e-90b7-c5e3b74c727e from RUNNING to DESTROYING
I0321 23:25:52.454818 23624 launcher.cpp:161] Asked to destroy container 
5230119f-ea8d-4a8e-90b7-c5e3b74c727e
W0321 23:25:52.455790 20672 process.cpp:1423] Failed to recv on socket 
WindowsFD::Type::SOCKET=8168 to peer '192.10.1.6:65519': IO failed with error 
code: The specified network name is no longer available.

W0321 23:25:52.455790 20672 process.cpp:838] Failed to recv on socket 
WindowsFD::Type::SOCKET=7940 to peer '192.10.1.6:65520': IO failed with error 
code: The specified network name is no longer available.

I0321 23:25:52.542614 17448 containerizer.cpp:3117] Container 
5230119f-ea8d-4a8e-90b7-c5e3b74c727e has exited
I0321 23:25:52.572590 26944 master.cpp:1135] Master terminating
I0321 23:25:52.574566 23624 hierarchical.cpp:679] Removed agent 
a9c136cd-584b-49f5-9574-f1954599c2fc-S0
I0321 23:25:52.983587 20672 process.cpp:927] Stopped the socket accept loop
```

- Mesos Reviewbot Windows


On March 20, 2019, 7:47 p.m., Greg Mann wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/70244/
> ---
> 
> (Updated March 20, 2019, 7:47 p.m.)
> 
> 
> Review request for mesos, Benjamin Bannier, Chun-Hung Hsiao, Gastón Kleiman, 
> and Joseph Wu.
> 
> 
> Bugs: MESOS-9318
> https://issues.apache.org/jira/browse/MESOS-9318
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This patch adds two new operation reconciliation tests,
> `OperationOnUnsubscribedProvider` and
> `FrameworkReconciliationRaceWithUpdateSlaveMessage`, to
> probe scenarios relevant to MESOS-9648 and MESOS-9318.
> 
> 
> Diffs
> -
> 
>   src/tests/operation_reconciliation_tests.cpp 
> 6a815ad694e2a608ce324715c920833f825793a0 
> 
> 
> Diff: https://reviews.apache.org/r/70244/diff/2/
> 
> 
> Testing
> ---
> 
> `make check`
> `bin/mesos-tests.sh 
> --gtest_filter="*FrameworkReconciliationRaceWithUpdateSlaveMessage*" 
> --gtest_repeat=-1 --gtest_break_on_failure`
> `bin/mesos-tests.sh --gtest_filter="*OperationOnUnsubscribedProvider*" 
> --gtest_repeat=-1 --gtest_break_on_failure`
> 
> 
> Thanks,
> 
> Greg Mann
> 
>

Re: Review Request 70243: Improved operation reconciliation for unsubscribed resource providers.

2019-03-21 Thread Joseph Wu 

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




src/master/master.cpp
Lines 9677-9678 (patched)


I feel like we usually try to avoid constructing entries via `operator[]`, 
but here it is intentional.

You can consider adding a note, so no one else thinks it is a mistake.



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


This statement looks out of place compared to the rest of the function, so 
a note might help: 
```
// Defer sending the operation status update until after this loop,
// which coagulates any reconciliation operations into combined
// messages for the agent (`forwardedReconciliations`).
```



src/resource_provider/manager.cpp
Lines 626-628 (patched)


Is the `SlaveID` field here meant to be blank?  I thought only external 
resource providers could omit this field.



src/slave/slave.cpp
Lines 4517-4527 (patched)


You could probably do without the ternary:
```
Option operationUuid;
if (operation.has_operation_uuid()) {
  operationUuid = operation.operation_uuid();
} else if (
operation.has_operation_id() && 
operationIds.contains(operation.operation_id())) {
  operationUuid = operationIds.at(operation.operation_id());
}
```


- Joseph Wu


On March 21, 2019, 3:26 p.m., Greg Mann wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/70243/
> ---
> 
> (Updated March 21, 2019, 3:26 p.m.)
> 
> 
> Review request for mesos, Benjamin Bannier, Chun-Hung Hsiao, Gastón Kleiman, 
> and Joseph Wu.
> 
> 
> Bugs: MESOS-9318
> https://issues.apache.org/jira/browse/MESOS-9318
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This patch updates the operation reconciliation pipeline to
> forward some framework-initiated reconciliation requests to
> the agent. In cases where an explicit reconciliation request
> specifies an operation which is not recognized on a resource
> provider which is also not recognized, the master will
> forward the request to the agent so that the resource
> provider manager can satisfy the request based on whether or
> not the resource provider has been seen before.
> 
> 
> Diffs
> -
> 
>   include/mesos/resource_provider/resource_provider.proto 
> 5ea9e2009209b1609619874ebd63cb1e2e698434 
>   src/master/master.cpp 9c4a9e83da94535873d72c902835f229c4f96320 
>   src/messages/messages.proto 633dddbaa874550f7f0d9513c608ed75b18059a8 
>   src/resource_provider/manager.cpp 7d3338ea7fbf330a25416f848db7742ad1bea52f 
>   src/slave/slave.cpp 36424f89a8c1f183febabcc9582975dd21213c25 
> 
> 
> Diff: https://reviews.apache.org/r/70243/diff/2/
> 
> 
> Testing
> ---
> 
> Testing details at the end of this chain.
> 
> 
> Thanks,
> 
> Greg Mann
> 
>

Re: Review Request 70243: Improved operation reconciliation for unsubscribed resource providers.

2019-03-21 Thread Greg Mann 

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

(Updated March 21, 2019, 10:26 p.m.)


Review request for mesos, Benjamin Bannier, Chun-Hung Hsiao, Gastón Kleiman, 
and Joseph Wu.


Changes
---

Updated the `operationIds` map operation to accommodate changes in the 
preceding patch.


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


Repository: mesos


Description
---

This patch updates the operation reconciliation pipeline to
forward some framework-initiated reconciliation requests to
the agent. In cases where an explicit reconciliation request
specifies an operation which is not recognized on a resource
provider which is also not recognized, the master will
forward the request to the agent so that the resource
provider manager can satisfy the request based on whether or
not the resource provider has been seen before.


Diffs (updated)
-

  include/mesos/resource_provider/resource_provider.proto 
5ea9e2009209b1609619874ebd63cb1e2e698434 
  src/master/master.cpp 9c4a9e83da94535873d72c902835f229c4f96320 
  src/messages/messages.proto 633dddbaa874550f7f0d9513c608ed75b18059a8 
  src/resource_provider/manager.cpp 7d3338ea7fbf330a25416f848db7742ad1bea52f 
  src/slave/slave.cpp 36424f89a8c1f183febabcc9582975dd21213c25 


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

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


Testing
---

Testing details at the end of this chain.


Thanks,

Greg Mann

Review Request 70264: Reordered hash definitions in the type utils.

2019-03-21 Thread Greg Mann 

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

Review request for mesos, Gastón Kleiman and Joseph Wu.


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


Repository: mesos


Description
---

This patch reorders the hash template specializations so
that they are in alphabetical order, with the specialization
for a `std::pair` moved to the end so that hashes of
combinations of types can be grouped together after the
definitions for the individual types.


Diffs
-

  include/mesos/type_utils.hpp acf5ba8e4db85bcb5e9370ed23d7d16260905e01 


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


Testing
---

Testing details at the end of this chain.


Thanks,

Greg Mann

Re: Review Request 70242: Tracked operation IDs in the agent.

2019-03-21 Thread Greg Mann 

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

(Updated March 21, 2019, 10:23 p.m.)


Review request for mesos, Benjamin Bannier, Gastón Kleiman, and Joseph Wu.


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


Repository: mesos


Description
---

This patch updates the agent to track operation IDs in a
hashmap. This map will be used to satisfy some reconciliation
requests which are forwarded from the master, when the agent
must be able to find the internal operation UUID for an
operation which the master did not recognize.


Diffs (updated)
-

  include/mesos/type_utils.hpp acf5ba8e4db85bcb5e9370ed23d7d16260905e01 
  src/slave/slave.hpp 271593460b0aaeb8dfa0b1659e4365919e0c32d5 
  src/slave/slave.cpp 36424f89a8c1f183febabcc9582975dd21213c25 


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

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


Testing
---

Testing details at the end of this chain.


Thanks,

Greg Mann

Re: Review Request 70242: Tracked operation IDs in the agent.

2019-03-21 Thread Greg Mann 


> On March 21, 2019, 8:27 p.m., Joseph Wu wrote:
> > src/slave/slave.hpp
> > Lines 875-878 (patched)
> > 
> >
> > Aren't these IDs only unique within a Framework's scope?
> > 
> > Two frameworks could presumably launch an operation with the same ID, 
> > on the same agent.  
> > 
> > Looks like the master stores a similar mapping in the Framework struct, 
> > so there is no possibility of overwrite there.

Whoops :D Thanks Joseph!!! I'll try changing this to a 
`hashmap, UUID>`, with a note to put the 
map in the framework struct once MESOS-8582 is resolved.


- Greg


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


On March 20, 2019, 2:21 a.m., Greg Mann wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/70242/
> ---
> 
> (Updated March 20, 2019, 2:21 a.m.)
> 
> 
> Review request for mesos, Benjamin Bannier, Gastón Kleiman, and Joseph Wu.
> 
> 
> Bugs: MESOS-9318
> https://issues.apache.org/jira/browse/MESOS-9318
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This patch updates the agent to track operation IDs in a
> hashmap. This map will be used to satisfy some reconciliation
> requests which are forwarded from the master, when the agent
> must be able to find the internal operation UUID for an
> operation which the master did not recognize.
> 
> 
> Diffs
> -
> 
>   src/slave/slave.hpp 271593460b0aaeb8dfa0b1659e4365919e0c32d5 
>   src/slave/slave.cpp 36424f89a8c1f183febabcc9582975dd21213c25 
> 
> 
> Diff: https://reviews.apache.org/r/70242/diff/1/
> 
> 
> Testing
> ---
> 
> Testing details at the end of this chain.
> 
> 
> Thanks,
> 
> Greg Mann
> 
>

Re: Review Request 70242: Tracked operation IDs in the agent.

2019-03-21 Thread Joseph Wu 

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




src/slave/slave.hpp
Lines 875-878 (patched)


Aren't these IDs only unique within a Framework's scope?

Two frameworks could presumably launch an operation with the same ID, on 
the same agent.  

Looks like the master stores a similar mapping in the Framework struct, so 
there is no possibility of overwrite there.


- Joseph Wu


On March 19, 2019, 7:21 p.m., Greg Mann wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/70242/
> ---
> 
> (Updated March 19, 2019, 7:21 p.m.)
> 
> 
> Review request for mesos, Benjamin Bannier, Gastón Kleiman, and Joseph Wu.
> 
> 
> Bugs: MESOS-9318
> https://issues.apache.org/jira/browse/MESOS-9318
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This patch updates the agent to track operation IDs in a
> hashmap. This map will be used to satisfy some reconciliation
> requests which are forwarded from the master, when the agent
> must be able to find the internal operation UUID for an
> operation which the master did not recognize.
> 
> 
> Diffs
> -
> 
>   src/slave/slave.hpp 271593460b0aaeb8dfa0b1659e4365919e0c32d5 
>   src/slave/slave.cpp 36424f89a8c1f183febabcc9582975dd21213c25 
> 
> 
> Diff: https://reviews.apache.org/r/70242/diff/1/
> 
> 
> Testing
> ---
> 
> Testing details at the end of this chain.
> 
> 
> Thanks,
> 
> Greg Mann
> 
>

Re: Review Request 70262: Added a new 'iterable_queue' template to stout.

2019-03-21 Thread Joseph Wu 

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



So besides changing the callback signature from this:
```
  void received(queue events)
  {
while (!events.empty()) {
  Event event = events.front();
  events.pop();
  
  ...
}
  }
```

To something like this:
```
  void received(iterable_queue events)
  {
foreach (const Event event&, events) {
  ...
}
  }
```

What are the benefits of allowing iteration?  Are there events we expect to 
receive in batches?  Or some benefit to looking forward?

- Joseph Wu


On March 21, 2019, 3:57 a.m., Benno Evers wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/70262/
> ---
> 
> (Updated March 21, 2019, 3:57 a.m.)
> 
> 
> Review request for mesos, Benjamin Bannier, Benjamin Mahler, and Joseph Wu.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This template exposes the iterator interface to a `std::queue`s
> underlying container.
> 
> The motivation for this is so it can be used in the `received()`
> callback of Mesos framework using the v1 API, to allow iterating
> over all received events.
> 
> 
> Diffs
> -
> 
>   3rdparty/stout/include/stout/iterable_queue.hpp PRE-CREATION 
> 
> 
> Diff: https://reviews.apache.org/r/70262/diff/1/
> 
> 
> Testing
> ---
> 
> Will add a unit test if we have consensus to move forward with adding this.
> 
> 
> Thanks,
> 
> Benno Evers
> 
>

Re: Review Request 70216: Implemented the recovery logic for v0 `VolumeManager`.

2019-03-21 Thread Chun-Hung Hsiao 


> On March 20, 2019, 3:13 p.m., Benjamin Bannier wrote:
> > src/csi/v0_volume_manager_process.hpp
> > Lines 176 (patched)
> > 
> >
> > Can or should this be `const`?

Although this can, it has a side effect that's not directly related to the data 
members. Given this, and given that it's an actor in a non-copyable object, I'd 
prefer not adding the `const` qualifier to give an illusion that it's 
side-effect free. WDYT?


- Chun-Hung


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


On March 15, 2019, 5:17 a.m., Chun-Hung Hsiao wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/70216/
> ---
> 
> (Updated March 15, 2019, 5:17 a.m.)
> 
> 
> Review request for mesos, Benjamin Bannier, Jie Yu, and Jan Schlicht.
> 
> 
> Bugs: MESOS-9622
> https://issues.apache.org/jira/browse/MESOS-9622
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> The recovery logic is similar to SLRP's `recoverVolumes` method. But
> `VolumeManager` no longer recovers all volumes to their steady states
> since this is not necessary. Together with the new `publishVolume`
> design, the recovery logic is now simpler.
> 
> 
> Diffs
> -
> 
>   src/csi/v0_volume_manager.cpp PRE-CREATION 
>   src/csi/v0_volume_manager_process.hpp PRE-CREATION 
> 
> 
> Diff: https://reviews.apache.org/r/70216/diff/2/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Chun-Hung Hsiao
> 
>

Re: Review Request 70262: Added a new 'iterable_queue' template to stout.

2019-03-21 Thread Mesos Reviewbot 

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



Patch looks great!

Reviews applied: [70262]

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

- Mesos Reviewbot


On March 21, 2019, 3:57 a.m., Benno Evers wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/70262/
> ---
> 
> (Updated March 21, 2019, 3:57 a.m.)
> 
> 
> Review request for mesos, Benjamin Bannier, Benjamin Mahler, and Joseph Wu.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This template exposes the iterator interface to a `std::queue`s
> underlying container.
> 
> The motivation for this is so it can be used in the `received()`
> callback of Mesos framework using the v1 API, to allow iterating
> over all received events.
> 
> 
> Diffs
> -
> 
>   3rdparty/stout/include/stout/iterable_queue.hpp PRE-CREATION 
> 
> 
> Diff: https://reviews.apache.org/r/70262/diff/1/
> 
> 
> Testing
> ---
> 
> Will add a unit test if we have consensus to move forward with adding this.
> 
> 
> Thanks,
> 
> Benno Evers
> 
>

Re: Review Request 70262: Added a new 'iterable_queue' template to stout.

2019-03-21 Thread Mesos Reviewbot Windows 

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



PASS: Mesos patch 70262 was successfully built and tested.

Reviews applied: `['70262']`

All the build artifacts available at: 
http://dcos-win.westus2.cloudapp.azure.com/artifacts/mesos-reviewbot-testing/2989/mesos-review-70262

- Mesos Reviewbot Windows


On March 21, 2019, 10:57 a.m., Benno Evers wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/70262/
> ---
> 
> (Updated March 21, 2019, 10:57 a.m.)
> 
> 
> Review request for mesos, Benjamin Bannier, Benjamin Mahler, and Joseph Wu.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This template exposes the iterator interface to a `std::queue`s
> underlying container.
> 
> The motivation for this is so it can be used in the `received()`
> callback of Mesos framework using the v1 API, to allow iterating
> over all received events.
> 
> 
> Diffs
> -
> 
>   3rdparty/stout/include/stout/iterable_queue.hpp PRE-CREATION 
> 
> 
> Diff: https://reviews.apache.org/r/70262/diff/1/
> 
> 
> Testing
> ---
> 
> Will add a unit test if we have consensus to move forward with adding this.
> 
> 
> Thanks,
> 
> Benno Evers
> 
>

Review Request 70262: Added a new 'iterable_queue' template to stout.

2019-03-21 Thread Benno Evers 

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

Review request for mesos, Benjamin Bannier, Benjamin Mahler, and Joseph Wu.


Repository: mesos


Description
---

This template exposes the iterator interface to a `std::queue`s
underlying container.

The motivation for this is so it can be used in the `received()`
callback of Mesos framework using the v1 API, to allow iterating
over all received events.


Diffs
-

  3rdparty/stout/include/stout/iterable_queue.hpp PRE-CREATION 


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


Testing
---

Will add a unit test if we have consensus to move forward with adding this.


Thanks,

Benno Evers

Re: Review Request 70216: Implemented the recovery logic for v0 `VolumeManager`.

2019-03-21 Thread Chun-Hung Hsiao 


> On March 20, 2019, 3:13 p.m., Benjamin Bannier wrote:
> > src/csi/v0_volume_manager.cpp
> > Lines 440 (patched)
> > 
> >
> > Should we have used an `Option` for this member?

I'm relying on the fact that the default capabilities are all off, and only 
turned on after recovery finishes, to avoid code like
```
if (pluginCapabilities.isSome() && pluginCapabilities->controllerService) {
  ...
}
```
That said, if this is more readable, I'm willing to change this and other 
capabilities variables to `Option`. WDYT?


> On March 20, 2019, 3:13 p.m., Benjamin Bannier wrote:
> > src/csi/v0_volume_manager.cpp
> > Lines 507 (patched)
> > 
> >
> > `Option`?

It's already an `Option`. Dropping.


- Chun-Hung


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


On March 15, 2019, 5:17 a.m., Chun-Hung Hsiao wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/70216/
> ---
> 
> (Updated March 15, 2019, 5:17 a.m.)
> 
> 
> Review request for mesos, Benjamin Bannier, Jie Yu, and Jan Schlicht.
> 
> 
> Bugs: MESOS-9622
> https://issues.apache.org/jira/browse/MESOS-9622
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> The recovery logic is similar to SLRP's `recoverVolumes` method. But
> `VolumeManager` no longer recovers all volumes to their steady states
> since this is not necessary. Together with the new `publishVolume`
> design, the recovery logic is now simpler.
> 
> 
> Diffs
> -
> 
>   src/csi/v0_volume_manager.cpp PRE-CREATION 
>   src/csi/v0_volume_manager_process.hpp PRE-CREATION 
> 
> 
> Diff: https://reviews.apache.org/r/70216/diff/2/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Chun-Hung Hsiao
> 
>