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

[Mesos Reviewbot]

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



Patch looks great!

Reviews applied: [70245, 70168, 70213, 70214, 70216, 70215, 70217, 70284, 
70222, 70285, 70223, 70225, 70247, 70258, 70248]

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 22, 2019, 11:39 p.m., Chun-Hung Hsiao wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/70248/
> ---
> 
> (Updated March 22, 2019, 11:39 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/3/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Chun-Hung Hsiao
> 
>

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

[Mesos Reviewbot Windows]

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



PASS: Mesos patch 70244 was successfully built and tested.

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

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

- Mesos Reviewbot Windows


On March 23, 2019, 2:35 a.m., Greg Mann wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/70244/
> ---
> 
> (Updated March 23, 2019, 2:35 a.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 
> 8bd1dc02a74c4e6c1b97b25e73098c0b75f2d38e 
> 
> 
> Diff: https://reviews.apache.org/r/70244/diff/3/
> 
> 
> 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.

[Mesos Reviewbot]

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



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 23, 2019, 2:35 a.m., Greg Mann wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/70244/
> ---
> 
> (Updated March 23, 2019, 2:35 a.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 
> 8bd1dc02a74c4e6c1b97b25e73098c0b75f2d38e 
> 
> 
> Diff: https://reviews.apache.org/r/70244/diff/3/
> 
> 
> 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.

[Greg Mann]

> On March 21, 2019, 11:39 p.m., Joseph Wu wrote:
> > 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.

I had to change the test to restart the master to clear out the operation 
state, and now I drop the UpdateSlaveMessage so that the master doesn't know 
about the RP.


> On March 21, 2019, 11:39 p.m., Joseph Wu wrote:
> > 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.

I added a FUTURE_PROTOBUF call to look for the ReconcileOperationsMessage.


- Greg


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


On March 23, 2019, 2:35 a.m., Greg Mann wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/70244/
> ---
> 
> (Updated March 23, 2019, 2:35 a.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 
> 8bd1dc02a74c4e6c1b97b25e73098c0b75f2d38e 
> 
> 
> Diff: https://reviews.apache.org/r/70244/diff/3/
> 
> 
> 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.

[Greg Mann]

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

(Updated March 23, 2019, 2:35 a.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 (updated)
-

  src/tests/operation_reconciliation_tests.cpp 
8bd1dc02a74c4e6c1b97b25e73098c0b75f2d38e 


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

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


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.

[Greg Mann]

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

(Updated March 23, 2019, 2:25 a.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 (updated)
-

  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/4/

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


Testing
---

Testing details at the end of this chain.


Thanks,

Greg Mann

Re: Review Request 70283: Improved handling of resources consumed by orphan operations.

[Mesos Reviewbot]

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



Patch looks great!

Reviews applied: [70283]

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 22, 2019, 8:37 p.m., Gastón Kleiman wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/70283/
> ---
> 
> (Updated March 22, 2019, 8:37 p.m.)
> 
> 
> Review request for mesos, Benjamin Bannier, Greg Mann, Joseph Wu, and Meng 
> Zhu.
> 
> 
> Bugs: MESOS-9635
> https://issues.apache.org/jira/browse/MESOS-9635
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This patch makes the master's `UpdateSlaveMessage` handler include
> resources consumed by orphan operations when calling
> `allocator->addResourceProvider()`.
> 
> The change prevents some races that lead to the master reoffering the
> resources consumed by the operations and makes the
> `OperationReconciliationTest.AgentPendingOperationAfterMasterFailover`
> test stable.
> 
> 
> Diffs
> -
> 
>   src/master/master.cpp 9c4a9e83da94535873d72c902835f229c4f96320 
> 
> 
> Diff: https://reviews.apache.org/r/70283/diff/1/
> 
> 
> Testing
> ---
> 
> `OperationReconciliationTest.AgentPendingOperationAfterMasterFailover` passed 
> over 5000 iterations under stress. Other tests still pass on GNU/Linux.
> 
> 
> Thanks,
> 
> Gastón Kleiman
> 
>

Re: Review Request 70284: Cleanup volume and storage pool listing.

[Mesos Reviewbot Windows]

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



PASS: Mesos patch 70284 was successfully built and tested.

Reviews applied: `['70245', '70168', '70213', '70214', '70216', '70215', 
'70217', '70284']`

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

- Mesos Reviewbot Windows


On March 22, 2019, 11:34 p.m., Chun-Hung Hsiao wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/70284/
> ---
> 
> (Updated March 22, 2019, 11:34 p.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
> ---
> 
> This patch modified SLRP's volume and storage pool listing methods to
> conform to `VolumeManager`s public interface. They will be moved out
> from SLRP to v0 `VolumeManager` later.
> 
> 
> Diffs
> -
> 
>   src/resource_provider/storage/provider.cpp 
> fea623c292158deb1b4b4b9ab1ac208031471519 
>   src/resource_provider/storage/provider_process.hpp 
> a5536b3d735e01eb1c4dc52d0602d973155f3c93 
> 
> 
> Diff: https://reviews.apache.org/r/70284/diff/1/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Chun-Hung Hsiao
> 
>

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

[Mesos Reviewbot Windows]

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



PASS: Mesos patch 70248 was successfully built and tested.

Reviews applied: `['70245', '70168', '70213', '70214', '70216', '70215', 
'70217', '70284', '70222', '70285', '70223', '70225', '70247', '70258', 
'70248']`

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

- Mesos Reviewbot Windows


On March 22, 2019, 11:39 p.m., Chun-Hung Hsiao wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/70248/
> ---
> 
> (Updated March 22, 2019, 11:39 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/3/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Chun-Hung Hsiao
> 
>

Re: Review Request 70283: Improved handling of resources consumed by orphan operations.

[Meng Zhu]

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



As pointed out by Greg earlier, this patch violates the comment here: 
https://github.com/apache/mesos/blob/4580834471fb3bc0b95e2b96e04a63d34faef724/src/master/allocator/mesos/hierarchical.cpp#L769-L770
While I think the current code seems to work with this patch (except the 
comment mentioned above), I think we should check with @Bbannier. In 
particular, the `TODO` here 
https://github.com/apache/mesos/blob/4580834471fb3bc0b95e2b96e04a63d34faef724/src/master/master.cpp#L8330-L8334:

```
// TODO(bbannier): Consider introducing ways of making sure an agent
// always knows the `FrameworkInfo` of operations triggered on its
// resources, e.g., by adding an explicit `FrameworkInfo` to
// operations like is already done for `RunTaskMessage`, see
// MESOS-8582.

```
Preferably, it will be great if we can fix the above TODO and ensure 
frameworkInfo is always available.


src/master/master.cpp
Line 8384 (original), 8371 (patched)


can we separate out irrelevant changes?


- Meng Zhu


On March 22, 2019, 1:37 p.m., Gastón Kleiman wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/70283/
> ---
> 
> (Updated March 22, 2019, 1:37 p.m.)
> 
> 
> Review request for mesos, Benjamin Bannier, Greg Mann, Joseph Wu, and Meng 
> Zhu.
> 
> 
> Bugs: MESOS-9635
> https://issues.apache.org/jira/browse/MESOS-9635
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This patch makes the master's `UpdateSlaveMessage` handler include
> resources consumed by orphan operations when calling
> `allocator->addResourceProvider()`.
> 
> The change prevents some races that lead to the master reoffering the
> resources consumed by the operations and makes the
> `OperationReconciliationTest.AgentPendingOperationAfterMasterFailover`
> test stable.
> 
> 
> Diffs
> -
> 
>   src/master/master.cpp 9c4a9e83da94535873d72c902835f229c4f96320 
> 
> 
> Diff: https://reviews.apache.org/r/70283/diff/1/
> 
> 
> Testing
> ---
> 
> `OperationReconciliationTest.AgentPendingOperationAfterMasterFailover` passed 
> over 5000 iterations under stress. Other tests still pass on GNU/Linux.
> 
> 
> Thanks,
> 
> Gastón Kleiman
> 
>

Re: Review Request 70222: Refactored SLRP to use v0 `VolumeManager`.

[Chun-Hung Hsiao]

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

(Updated March 22, 2019, 11:41 p.m.)


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


Changes
---

Restructured the refactoring.


Summary (updated)
-

Refactored SLRP to use v0 `VolumeManager`.


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


Repository: mesos


Description (updated)
---

This patch moves volume management code from SLRP to the v0
`VolumeManager`, and make SLRP uses the `VolumeManager` interface
polymorphically.

However, since SLRP now no longer keeps track of CSI volume states, it
will not be able to verify that a persistent volume is published before
being destroyed (although this should be guaranteed by volume manager
recovery).


Diffs (updated)
-

  src/csi/v0_volume_manager.cpp PRE-CREATION 
  src/csi/v0_volume_manager_process.hpp PRE-CREATION 
  src/resource_provider/storage/provider.cpp 
fea623c292158deb1b4b4b9ab1ac208031471519 
  src/resource_provider/storage/provider_process.hpp 
a5536b3d735e01eb1c4dc52d0602d973155f3c93 
  src/tests/storage_local_resource_provider_tests.cpp 
7945384867f26fa15dc734a235ae509d5d6d350f 


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

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


Testing
---

sudo make check


Thanks,

Chun-Hung Hsiao

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

[Chun-Hung Hsiao]

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

(Updated March 22, 2019, 11:39 p.m.)


Review request for mesos, Benjamin Bannier, James DeFelice, and Jan Schlicht.


Changes
---

Rebased.


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 (updated)
-

  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/3/

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


Testing
---

make check


Thanks,

Chun-Hung Hsiao

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

[Mesos Reviewbot Windows]

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



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/3000/mesos-review-70248

Relevant logs:

- 
[apply-review-70222.log](http://dcos-win.westus2.cloudapp.azure.com/artifacts/mesos-reviewbot-testing/3000/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 70248: Adapted the unversioned `VolumeCapability`.

[Mesos Reviewbot]

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



Bad review!

Reviews applied: [70248, 70258, 70247, 70225, 70223, 70222, 70217, 70215, 
70216, 70214, 70213, 70168, 70245]

Error:
2019-03-22 23:37:41 URL:https://reviews.apache.org/r/70222/diff/raw/ 
[80925/80925] -> "70222.patch" [1]
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


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 70215: Cleanup volume attaching and publishing for SLRP.

[Chun-Hung Hsiao]

> On March 19, 2019, 5:06 p.m., Benjamin Bannier wrote:
> > src/csi/v0_volume_manager.cpp
> > Lines 146 (patched)
> > 
> >
> > `s/sequentialized/serialized/` here and below?
> > 
> > _attaching_ to be consistent with other methods?

Fixed in r/70285.


> On March 19, 2019, 5:06 p.m., Benjamin Bannier wrote:
> > src/csi/v0_volume_manager.cpp
> > Lines 159 (patched)
> > 
> >
> > _detaching_?

Fixed in r/70285.


> On March 19, 2019, 5:06 p.m., Benjamin Bannier wrote:
> > src/csi/v0_volume_manager.cpp
> > Lines 185 (patched)
> > 
> >
> > _unpublishing_?

Fixed in r/70285.


> On March 19, 2019, 5:06 p.m., Benjamin Bannier wrote:
> > src/csi/v0_volume_manager.cpp
> > Lines 192 (patched)
> > 
> >
> > Let's use uppercase initials for even for non-type template parameters, 
> > `s/rpc/Rpc/g`.
> > 
> > Here and below.

Fixed in r/70222.


> On March 19, 2019, 5:06 p.m., Benjamin Bannier wrote:
> > src/csi/v0_volume_manager.cpp
> > Lines 202 (patched)
> > 
> >
> > Break line before `.then`.

Fixed in r/70222.


- Chun-Hung


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


On March 22, 2019, 6:18 a.m., Chun-Hung Hsiao wrote:
> 
> ---
> 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.
> 
> 
> Bugs: MESOS-9622
> https://issues.apache.org/jira/browse/MESOS-9622
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> 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
> -
> 
>   src/resource_provider/storage/provider.cpp 
> fea623c292158deb1b4b4b9ab1ac208031471519 
>   src/resource_provider/storage/provider_process.hpp 
> a5536b3d735e01eb1c4dc52d0602d973155f3c93 
> 
> 
> Diff: https://reviews.apache.org/r/70215/diff/4/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Chun-Hung Hsiao
> 
>

Review Request 70285: Implemented the remaining methods of v0 `VolumeManager`.

[Chun-Hung Hsiao]

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

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
---

This patch completes the v0 `VolumeManager` by properly implemented the
`attachVolume`, `detachVolume` and `unpublishVolume` functions that are
not used by SLRP.


Diffs
-

  src/csi/v0_volume_manager.cpp PRE-CREATION 


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


Testing
---

make check


Thanks,

Chun-Hung Hsiao

Review Request 70284: Cleanup volume and storage pool listing.

[Chun-Hung Hsiao]

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

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
---

This patch modified SLRP's volume and storage pool listing methods to
conform to `VolumeManager`s public interface. They will be moved out
from SLRP to v0 `VolumeManager` later.


Diffs
-

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


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


Testing
---

make check


Thanks,

Chun-Hung Hsiao

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

[Chun-Hung Hsiao]

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

(Updated March 22, 2019, 11:33 p.m.)


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


Changes
---

Fixed some bad logging/error messages.


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


Repository: mesos


Description
---

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/5/

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


Testing
---

make check


Thanks,

Chun-Hung Hsiao

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

[Chun-Hung Hsiao]

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

(Updated March 22, 2019, 11:31 p.m.)


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


Changes
---

Inlined an internal recovery helper so the recovery logic is in one place.


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


Repository: mesos


Description
---

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/4/

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


Testing
---

make check


Thanks,

Chun-Hung Hsiao

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

[Mesos Reviewbot Windows]

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



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/2999/mesos-review-70244

Relevant logs:

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

```
I0322 23:20:34.496538 27168 master.cpp:1295] Agent 
30cc-b7c6-4b39-b6f2-f167aaa2bc17-S0 at slave(502)@192.10.1.6:61024 
(windows-02.chtsmhjxogyevckjfayqqcnjda.xx.internal.cloudapp.net) disconnected
I0322 23:20:34.496538 27168 master.cpp:3330] Disconnecting agent 
30cc-b7c6-4b39-b6f2-f167aaa2bc17-S0 at slave(502)@192.10.1.6:61024 
(windows-02.chtsmhjxogyevckjfayqqcnjda.xx.internal.cloudapp.net)
I0322 23:20:34.496538 27168 master.cpp:3349] Deactivating agent 
30cc-b7c6-4b39-b6f2-f167aaa2bc17-S0 at slave(502)@192.10.1.6:61024 
(windows-02.chtsmhjxogyevckjfayqqcnjda.xx.internal.cloudapp.net)
I0322 23:20:34.497529 16232 hierarchical.cpp:391] Removed framework 
30cc-b7c6-4b39-b6f2-f167aaa2bc17-
I0322 23:20:34.497529 25384 containerizer.cpp:2576] Destroying container 
7b1c1833-f800-4db2-9c09-e6c86d2ba521 in RUNNING state
I0322 23:20:34.497529 16232 hierarchical.cpp:828] Agent 
30cc-b7c6-4b39-b6f2-f167aaa2bc17-S0 deactivated
I0322 23:20:34.497529 25384 containerizer.cpp:3278] Transitioning the state of 
container 7b1c1833-f800-4db2-9c09-e6c86d2ba521 from RUNNING to DESTROYING
I0322 23:20:34.498554 25384 launcher.cpp:161] Asked to destroy container 
7b1c1833-f800-4db2-9c09-e6c86d2ba521
W0322 23:20:34.499524 24904 process.cpp:1423] Failed to recv on socket 
WindowsFD::Type::SOCKET=9948 to peer '192.10.1.6:50461': IO failed with error 
code: The specified network name is no longer available.

W0322 23:20:34.499524 24904 process.cpp:838] Failed to recv on socket 
WindowsFD::Type::SOCKET=9312 to peer '192.10.1.6:50462': IO failed with error 
code: The specified network name is no longer available.

I0322 23:20:34.569519 28152 containerizer.cpp:3117] Container 
7b1c1833-f800-4db2-9c09-e6c86d2ba521 has exited
I0322 23:20:34.599589 25384 master.cpp:113[   OK ] 
IsolationFlag/MemoryIsolatorTest.ROOT_MemUsage/0 (687 ms)
[--] 1 test from IsolationFlag/MemoryIsolatorTest (704 ms total)

[--] Global test environment tear-down
[==] 1128 tests from 107 test cases ran. (551266 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

5] Master terminating
I0322 23:20:34.601016 28376 hierarchical.cpp:679] Removed agent 
30cc-b7c6-4b39-b6f2-f167aaa2bc17-S0
I0322 23:20:35.032517 24904 process.cpp:927] Stopped the socket accept loop
```

- Mesos Reviewbot Windows


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.

[Mesos Reviewbot]

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



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, 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 70283: Improved handling of resources consumed by orphan operations.

[Mesos Reviewbot Windows]

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



PASS: Mesos patch 70283 was successfully built and tested.

Reviews applied: `['70283']`

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

- Mesos Reviewbot Windows


On March 22, 2019, 8:37 p.m., Gastón Kleiman wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/70283/
> ---
> 
> (Updated March 22, 2019, 8:37 p.m.)
> 
> 
> Review request for mesos, Benjamin Bannier, Greg Mann, Joseph Wu, and Meng 
> Zhu.
> 
> 
> Bugs: MESOS-9635
> https://issues.apache.org/jira/browse/MESOS-9635
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This patch makes the master's `UpdateSlaveMessage` handler include
> resources consumed by orphan operations when calling
> `allocator->addResourceProvider()`.
> 
> The change prevents some races that lead to the master reoffering the
> resources consumed by the operations and makes the
> `OperationReconciliationTest.AgentPendingOperationAfterMasterFailover`
> test stable.
> 
> 
> Diffs
> -
> 
>   src/master/master.cpp 9c4a9e83da94535873d72c902835f229c4f96320 
> 
> 
> Diff: https://reviews.apache.org/r/70283/diff/1/
> 
> 
> Testing
> ---
> 
> `OperationReconciliationTest.AgentPendingOperationAfterMasterFailover` passed 
> over 5000 iterations under stress. Other tests still pass on GNU/Linux.
> 
> 
> Thanks,
> 
> Gastón Kleiman
> 
>

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

[Greg Mann]

> On March 22, 2019, 1:24 a.m., Gastón Kleiman wrote:
> > 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.

Ah, I realized that this proto change is actually not needed since I'm not 
forwarding anything to the RP. I did, however, update the comment in the 
`framework_id` field that I added to the `ReconcileOperationsMessage`, and 
updated the comments in the agent handler for that message.


> On March 22, 2019, 1:24 a.m., Gastón Kleiman wrote:
> > src/slave/slave.cpp
> > Lines 4524-4525 (patched)
> > 
> >
> > Nit:
> > 
> > ```
> >   } else if (operationId.isSome() &&
> > 
> > operationIds.contains(operationId.get())) {
> > ```
> > 
> > But I like Joseph's suggestion better =).

Good point. Had to tweak a bit now that this map is indexed by (FrameworkID, 
OperationID).


- Greg


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


On March 22, 2019, 9:09 p.m., Greg Mann wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/70243/
> ---
> 
> (Updated March 22, 2019, 9:09 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
> -
> 
>   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/3/
> 
> 
> Testing
> ---
> 
> Testing details at the end of this chain.
> 
> 
> Thanks,
> 
> Greg Mann
> 
>

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

[Greg Mann]

> On March 21, 2019, 11:18 p.m., Joseph Wu wrote:
> > 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.

Yea the RP manager doesn't have the agent ID so the agent injects it before 
forwarding to the master (that code is already in place: 
https://github.com/apache/mesos/blob/4580834471fb3bc0b95e2b96e04a63d34faef724/src/slave/slave.cpp#L8250-L8254).
 I've added a comment to that effect.


> On March 21, 2019, 11:18 p.m., Joseph Wu wrote:
> > 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());
> > }
> > ```

Good point. Had to tweak a bit now that this map is indexed by (FrameworkID, 
OperationID).


- Greg


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


On March 22, 2019, 9:09 p.m., Greg Mann wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/70243/
> ---
> 
> (Updated March 22, 2019, 9:09 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
> -
> 
>   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/3/
> 
> 
> Testing
> ---
> 
> Testing details at the end of this chain.
> 
> 
> Thanks,
> 
> Greg Mann
> 
>

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

[Greg Mann]

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

(Updated March 22, 2019, 9:09 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 (updated)
-

  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/3/

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


Testing
---

Testing details at the end of this chain.


Thanks,

Greg Mann

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

[Mesos Reviewbot]

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



Bad review!

Reviews applied: [70248, 70258, 70247, 70225, 70223, 70222, 70217, 70215, 
70216, 70214, 70213, 70168, 70245]

Error:
2019-03-22 20:44:20 URL:https://reviews.apache.org/r/70222/diff/raw/ 
[80925/80925] -> "70222.patch" [1]
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


On March 20, 2019, 2: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, 2: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
> 
>

Review Request 70283: Improved handling of resources consumed by orphan operations.

[Gastón Kleiman]

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

Review request for mesos, Benjamin Bannier, Greg Mann, Joseph Wu, and Meng Zhu.


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


Repository: mesos


Description
---

This patch makes the master's `UpdateSlaveMessage` handler include
resources consumed by orphan operations when calling
`allocator->addResourceProvider()`.

The change prevents some races that lead to the master reoffering the
resources consumed by the operations and makes the
`OperationReconciliationTest.AgentPendingOperationAfterMasterFailover`
test stable.


Diffs
-

  src/master/master.cpp 9c4a9e83da94535873d72c902835f229c4f96320 


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


Testing
---

`OperationReconciliationTest.AgentPendingOperationAfterMasterFailover` passed 
over 5000 iterations under stress. Other tests still pass on GNU/Linux.


Thanks,

Gastón Kleiman

Re: Review Request 70282: Added new example framework for operation feedback.

[Mesos Reviewbot]

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



Patch looks great!

Reviews applied: [70281, 70282]

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 22, 2019, 4:57 p.m., Benno Evers wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/70282/
> ---
> 
> (Updated March 22, 2019, 4:57 p.m.)
> 
> 
> Review request for mesos, Gastón Kleiman, Greg Mann, and Joseph Wu.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This adds a new example framework showcasing a possible
> implementation of the newly added operation feedback API.
> 
> 
> Diffs
> -
> 
>   src/Makefile.am bcafe48b2105575371464a29783bc6f3f1c2cf8d 
>   src/examples/CMakeLists.txt f47753980a0bf8d23757df0e7be859db34ae1822 
>   src/examples/operation_feedback_framework.cpp PRE-CREATION 
>   src/tests/CMakeLists.txt ab7f6c2027d937038ee70145705b699c4fb8f05c 
> 
> 
> Diff: https://reviews.apache.org/r/70282/diff/1/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Benno Evers
> 
>

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

[Chun-Hung Hsiao]

> On March 19, 2019, 2:57 p.m., Benjamin Bannier wrote:
> > src/csi/v0_volume_manager.hpp
> > Lines 100 (patched)
> > 
> >
> > Does it make sense to move this into the process? It would lead to 
> > simpler dispatch in the wrapper around the process, and probably be more in 
> > line with the way we usually lay this out.
> 
> Chun-Hung Hsiao wrote:
> Yes I agree doing what you suggested is more consistent with the existing 
> codebase.
> 
> The reason I do this is because we can save one dispatch. If we move it 
> into the actor, it would be like:
> ```
> Future VolumeManagerProcess::attachVolume(...)
> {
>   return recovered
> .then(defer(self(), [=] { ... }));
> }
> ```
> Since `recovered` might carry a future that is satisfied in another 
> actor. And we'll have:
> ```
> Future VolumeManager::attachVolume(...)
> {
>   return dispatch(process.get(), &VolumeProcessManager::attachVolume, 
> ...);
> }
> ```
> See, we'll always get two dispatches in a single `attachVolume` call.
> 
> There are two ways to solve this:
> 1. Keep `recovered` in the actor, but do the following:
> ```
> // Ensure that the following future is satisfied in this actor.
> recovered = recover().then(defer(self(), [] { return Nothing(); }));
> ```
> So we could remove a `defer` in `attachVolume`. But the last `then` looks 
> a bit unnatural.
> 
> 2. Do what I did in this patch.
> 
> 3. Don't care about the two dispatches, since this is less likely to 
> introduce much overhead.
> 
> WDYT?
> 
> Benjamin Bannier wrote:
> TBH, I don't really understand what (1) accomplishes as it seems that to 
> be able to drop the `defer` we'd need to assert that we are already recovered.
> 
> I'd be in favor of (3) as it would just appear simpler and I do not 
> expect a lot of contention on the actor which makes optimizing this less 
> necessary, but I'll leave this up to you.

For (1), let's say we have the following code instead:
```
Future VolumeManagerProcess::attachVolume(...)
{
  return recovered.then([=] { return _attachVolume(...); });
}
```
if `recovered` is already completed, the continuation will be invoked 
synchronously; if not, it will be invoked in the same actor where `recovered` 
will be set.
That said, I will never want to write code like this ;) Just listing what else 
can be done theoratically.

A small benefit of (3) is that we don't need to introduce one more layer of 
indentation in the actor method itself, and `FUTURE_DISPATCH` on the actor 
function would be a bit more meaningful.


> On March 19, 2019, 2:57 p.m., Benjamin Bannier wrote:
> > src/csi/volume_manager.cpp
> > Line 43 (original), 49 (patched)
> > 
> >
> > Same question as on previous review: how do you plan to do 
> > version-dependent dispatch here?
> 
> Chun-Hung Hsiao wrote:
> Something like:
> ```
> // TODO(chhsiao): Do a move-capture with `Owned` once we get C++14.
> auto serviceManager = make_shared(new Owned(new 
> ServiceManager(...)));
> return (*serviceManager)->getCsiVersion()
>   .then([=](const string& version) -> Future> {
> if (version == v1::VERSION) {
>   return new v1::VolumeManager(..., std::move(*serviceManager));
> } else if (version == v0::VERSION) {
>   return new v0::VolumeManager(..., std::move(*serviceManager));
> }
> 
> UNREACHABLE();
>   });
> ```
> 
> Benjamin Bannier wrote:
> Looks good.
> 
> Let's use a `shared_ptr` instead of an `Owned` so this code is 
> semantically correct.

Hmm I believe it is already semantically correct but it seems that `auto` + 
`make_shared` is really not that readable ;) Maybe I should go with
```
std::shared_ptr> serviceManager(new 
Owned(new ServiceManager(...)));
```
or
```
std::shared_ptr> serviceManager = std::make_shared(new 
Owned(new ServiceManager(...;

```
But both seem a bit less idomatic. 

The type of `serviceManager` is `std::shared_ptr>`.
The problem is that `std::shared_ptr` has no method to relinquish the ownership 
without destroying the object.

I can go with `std::shared_ptr` instead and change all 
interfaces taking this argument to `std::shared_ptr` as well. But that means in 
the future we'd like to do the changes again to switch back to eithor `Owned` 
or `std::unique_ptr`. WDYT?


- Chun-Hung


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


On March 22, 2019, 6:12 a.m., Chun-Hung Hsiao wrote:
> 
> ---
> This is an automatically generated e-mail. 

Re: Review Request 70169: Refactored SLRP to use `ServiceManager`.

[Mesos Reviewbot Windows]

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



FAIL: Failed to apply the current review.

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

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

Relevant logs:

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

```
error: patch failed: src/resource_provider/storage/provider.cpp:41
error: src/resource_provider/storage/provider.cpp: patch does not apply
error: patch failed: src/resource_provider/storage/provider_process.hpp:17
error: src/resource_provider/storage/provider_process.hpp: patch does not apply
```

- Mesos Reviewbot Windows


On March 20, 2019, 6:03 a.m., Chun-Hung Hsiao wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/70169/
> ---
> 
> (Updated March 20, 2019, 6:03 a.m.)
> 
> 
> Review request for mesos, Benjamin Bannier, Jie Yu, and Jan Schlicht.
> 
> 
> Bugs: MESOS-9632
> https://issues.apache.org/jira/browse/MESOS-9632
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Refactored SLRP to use `ServiceManager`.
> 
> 
> Diffs
> -
> 
>   src/resource_provider/storage/provider.cpp 
> fea623c292158deb1b4b4b9ab1ac208031471519 
>   src/resource_provider/storage/provider_process.hpp 
> a5536b3d735e01eb1c4dc52d0602d973155f3c93 
> 
> 
> Diff: https://reviews.apache.org/r/70169/diff/3/
> 
> 
> Testing
> ---
> 
> sudo make check
> 
> 
> Thanks,
> 
> Chun-Hung Hsiao
> 
>

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

[Mesos Reviewbot Windows]

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



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/2997/mesos-review-70248

Relevant logs:

- 
[apply-review-70222.log](http://dcos-win.westus2.cloudapp.azure.com/artifacts/mesos-reviewbot-testing/2997/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 70282: Added new example framework for operation feedback.

[Mesos Reviewbot Windows]

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



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

Reviews applied: `['70281', '70282']`

Failed command: `Start-MesosCITesting`

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

Relevant logs:

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

```
W0322 18:41:34.561565 27736 slave.cpp:3932] Ignoring shutdown framework 
8c014286-17b2-40d2-bd53-75459bc9a259- because it is terminating
I0322 18:41:34.564551 19516 master.cpp:1295] Agent 
8c014286-17b2-40d2-bd53-75459bc9a259-S0 at slave(496)@192.10.1.6:55730 
(windows-02.chtsmhjxogyevckjfayqqcnjda.xx.internal.cloudapp.net) disconnected
I0322 18:41:34.564551 19516 master.cpp:3330] Disconnecting agent 
8c014286-17b2-40d2-bd53-75459bc9a259-S0 at slave(496)@192.10.1.6:55730 
(windows-02.chtsmhjxogyevckjfayqqcnjda.xx.internal.cloudapp.net)
I0322 18:41:34.564551 19516 master.cpp:3349] Deactivating agent 
8c014286-17b2-40d2-bd53-75459bc9a259-S0 at slave(496)@192.10.1.6:55730 
(windows-02.chtsmhjxogyevckjfayqqcnjda.xx.internal.cloudapp.net)
I0322 18:41:34.564551 30388 hierarchical.cpp:391] Removed framework 
8c014286-17b2-40d2-bd53-75459bc9a259-
I0322 18:41:34.565562 30388 hierarchical.cpp:828] Agent 
8c014286-17b2-40d2-bd53-75459bc9a259-S0 deactivated
I0322 18:41:34.566567 19516 containerizer.cpp:2576] Destroying container 
6fe13f05-451c-4b4e-8d48-041309340bfb in RUNNING state
I0322 18:41:34.566567 19516 containerizer.cpp:3278] Transitioning the state of 
container 6fe13f05-451c-4b4e-8d48-041309340bfb from RUNNING to DESTROYING
I0322 18:41:34.567555 19516 launcher.cpp:161] Asked to destroy container 
6fe13f05-451c-4b4e-8d48-041309340bfb
W0322 18:41:34.568560 30064 process.cpp:1423] Failed to recv on socket 
WindowsFD::Type::SOCKET=3600 to peer '192.10.1.6:58047': IO failed with error 
code: The specified network name is no longer available.

W0322 18:41:34.568560 30064 process.cpp:838] Failed to recv on socket 
WindowsFD::Type::SOCKET=4304 to peer '192.10.1.6:58048': IO failed with error 
code: The specified network name is no longer available.

I0322 18[   OK ] IsolationFlag/MemoryIsolatorTest.ROOT_MemUsage/0 (589 ms)
[--] 1 test from IsolationFlag/MemoryIsolatorTest (607 ms total)

[--] Global test environment tear-down
[==] 1124 tests from 107 test cases ran. (507237 ms total)
[  PASSED  ] 1123 tests.
[  FAILED  ] 1 test, listed below:
[  FAILED  ] DockerFetcherPluginTest.INTERNET_CURL_FetchBlob

 1 FAILED TEST
  YOU HAVE 231 DISABLED TESTS

:41:34.573561 30388 containerizer.cpp:3117] Container 
6fe13f05-451c-4b4e-8d48-041309340bfb has exited
I0322 18:41:34.602557 30680 master.cpp:1135] Master terminating
I0322 18:41:34.603572 12112 hierarchical.cpp:679] Removed agent 
8c014286-17b2-40d2-bd53-75459bc9a259-S0
I0322 18:41:35.005559 30064 process.cpp:927] Stopped the socket accept loop
```

- Mesos Reviewbot Windows


On March 22, 2019, 4:57 p.m., Benno Evers wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/70282/
> ---
> 
> (Updated March 22, 2019, 4:57 p.m.)
> 
> 
> Review request for mesos, Gastón Kleiman, Greg Mann, and Joseph Wu.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This adds a new example framework showcasing a possible
> implementation of the newly added operation feedback API.
> 
> 
> Diffs
> -
> 
>   src/Makefile.am bcafe48b2105575371464a29783bc6f3f1c2cf8d 
>   src/examples/CMakeLists.txt f47753980a0bf8d23757df0e7be859db34ae1822 
>   src/examples/operation_feedback_framework.cpp PRE-CREATION 
>   src/tests/CMakeLists.txt ab7f6c2027d937038ee70145705b699c4fb8f05c 
> 
> 
> Diff: https://reviews.apache.org/r/70282/diff/1/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Benno Evers
> 
>

Re: Review Request 70168: Refactored SLRP with `ServiceManager` to manage container lifecycles.

[Chun-Hung Hsiao]

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

(Updated March 22, 2019, 6:36 p.m.)


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


Changes
---

Squashed r/70169 into this patch.


Summary (updated)
-

Refactored SLRP with `ServiceManager` to manage container lifecycles.


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


Repository: mesos


Description (updated)
---

Container management is moved out from SLRP to `ServiceManager`. It is
agnostic to CSI versions, so can be used to manage plugin containers for
both CSI v0 and v1 plugins.

This patch squashes the changes from r/70169.


Diffs (updated)
-

  src/CMakeLists.txt 3397c3b1d4e8a7900b2e5f870679cc7aa30b4be2 
  src/Makefile.am d451d7cabe3bf5d4f5039cfac5de1b03ef891d07 
  src/csi/service_manager.hpp PRE-CREATION 
  src/csi/service_manager.cpp PRE-CREATION 
  src/resource_provider/storage/provider.cpp 
fea623c292158deb1b4b4b9ab1ac208031471519 
  src/resource_provider/storage/provider_process.hpp 
a5536b3d735e01eb1c4dc52d0602d973155f3c93 


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

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


Testing
---

Testing done later in chain.


Thanks,

Chun-Hung Hsiao

Re: Review Request 70279: Updated quota overcommit message to include total quota and capacity.

[Mesos Reviewbot]

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



Patch looks great!

Reviews applied: [70272, 70273, 70274, 70275, 70276, 70277, 70278, 70279]

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 22, 2019, 3:54 p.m., Benjamin Mahler wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/70279/
> ---
> 
> (Updated March 22, 2019, 3:54 p.m.)
> 
> 
> Review request for mesos, Benno Evers and Meng Zhu.
> 
> 
> Bugs: MESOS-9292
> https://issues.apache.org/jira/browse/MESOS-9292
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Per MESOS-9292, users would find it helpful for the quota overcommit
> error message to include more information about the cluster capactiy
> vs total quota guarantees.
> 
> 
> Diffs
> -
> 
>   src/master/quota_handler.cpp 2dcfdcaf836f21d7d39b5ef5c36de0db25ca7517 
>   src/tests/master_quota_tests.cpp 3e7a94cad94b64faf01e73f880281d24098fadbd 
> 
> 
> Diff: https://reviews.apache.org/r/70279/diff/1/
> 
> 
> Testing
> ---
> 
> Updated a test.
> 
> 
> Thanks,
> 
> Benjamin Mahler
> 
>

Re: Review Request 70279: Updated quota overcommit message to include total quota and capacity.

[Mesos Reviewbot Windows]

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



PASS: Mesos patch 70279 was successfully built and tested.

Reviews applied: `['70272', '70273', '70274', '70275', '70276', '70277', 
'70278', '70279']`

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

- Mesos Reviewbot Windows


On March 22, 2019, 3:54 p.m., Benjamin Mahler wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/70279/
> ---
> 
> (Updated March 22, 2019, 3:54 p.m.)
> 
> 
> Review request for mesos, Benno Evers and Meng Zhu.
> 
> 
> Bugs: MESOS-9292
> https://issues.apache.org/jira/browse/MESOS-9292
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Per MESOS-9292, users would find it helpful for the quota overcommit
> error message to include more information about the cluster capactiy
> vs total quota guarantees.
> 
> 
> Diffs
> -
> 
>   src/master/quota_handler.cpp 2dcfdcaf836f21d7d39b5ef5c36de0db25ca7517 
>   src/tests/master_quota_tests.cpp 3e7a94cad94b64faf01e73f880281d24098fadbd 
> 
> 
> Diff: https://reviews.apache.org/r/70279/diff/1/
> 
> 
> Testing
> ---
> 
> Updated a test.
> 
> 
> Thanks,
> 
> Benjamin Mahler
> 
>

Review Request 70281: Added stream operator overload for OperationStatus messages.

[Benno Evers]

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

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


Repository: mesos


Description
---

This commit adds a new overload

ostream& operator<<(ostream&, const OperationStatus&);

to make the printing of OperationStatus messages
more convenient.


Diffs
-

  include/mesos/v1/mesos.hpp dc328bf45d8b69dcf096785b98031513b7ea9099 
  src/v1/mesos.cpp 704ad7697689d9cb21d6ed2675ac6c0044ce18c8 


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


Testing
---


Thanks,

Benno Evers

Review Request 70282: Added new example framework for operation feedback.

[Benno Evers]

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

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


Repository: mesos


Description
---

This adds a new example framework showcasing a possible
implementation of the newly added operation feedback API.


Diffs
-

  src/Makefile.am bcafe48b2105575371464a29783bc6f3f1c2cf8d 
  src/examples/CMakeLists.txt f47753980a0bf8d23757df0e7be859db34ae1822 
  src/examples/operation_feedback_framework.cpp PRE-CREATION 
  src/tests/CMakeLists.txt ab7f6c2027d937038ee70145705b699c4fb8f05c 


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


Testing
---


Thanks,

Benno Evers

[GitHub] [mesos] asekretenko opened a new pull request #327: Updated glog to 0.3.5 and then to 0.4.0, added microseconds to LogSink::send().

[GitBox]

asekretenko opened a new pull request #327: Updated glog to 0.3.5 and then to 
0.4.0, added microseconds to LogSink::send().
URL: https://github.com/apache/mesos/pull/327
 
 
   Also replaced the `os.path.exists()`-based logic in `ext_modules.py.in` with 
logic based on automake substitutes.


This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org


With regards,
Apache Git Services

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

[Benjamin Mahler]

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


Ship it!




Ship It!

- Benjamin Mahler


On March 22, 2019, 6:13 a.m., Meng Zhu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/70267/
> ---
> 
> (Updated March 22, 2019, 6:13 a.m.)
> 
> 
> 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
> 
>

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

[Benjamin Mahler]

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


Ship it!




Ship It!

- Benjamin Mahler


On March 22, 2019, 6:13 a.m., Meng Zhu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/70268/
> ---
> 
> (Updated March 22, 2019, 6:13 a.m.)
> 
> 
> 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
> 
>

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

[Benjamin Mahler]

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


Fix it, then Ship it!





src/master/allocator/mesos/hierarchical.cpp
Line 1725 (original), 1711-1712 (patched)


Do we need to say tracked by the quota role sorter? The code shows this. Is 
there something important about it coming from the quota role sorter here that 
the reader needs to know? (e.g. since this is a quota role it must be tracked 
in the quota role sorter? won't it be also tracked in the regular role sorter 
though?)


- Benjamin Mahler


On March 22, 2019, 6:13 a.m., Meng Zhu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/70266/
> ---
> 
> (Updated March 22, 2019, 6:13 a.m.)
> 
> 
> 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
> 
>

Review Request 70272: Eliminate a copy of the master's quota during quota validation.

[Benjamin Mahler]

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

Review request for mesos and Meng Zhu.


Repository: mesos


Description
---

Rather than copying the quota map, QuotaTree also supports insertion
of each entry. This allows us to insert each entry while ensuring
that the mutated role gets inserted with the new value.


Diffs
-

  src/master/quota_handler.cpp 2dcfdcaf836f21d7d39b5ef5c36de0db25ca7517 


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


Testing
---

make check


Thanks,

Benjamin Mahler

Review Request 70279: Updated quota overcommit message to include total quota and capacity.

[Benjamin Mahler]

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

Review request for mesos, Benno Evers and Meng Zhu.


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


Repository: mesos


Description
---

Per MESOS-9292, users would find it helpful for the quota overcommit
error message to include more information about the cluster capactiy
vs total quota guarantees.


Diffs
-

  src/master/quota_handler.cpp 2dcfdcaf836f21d7d39b5ef5c36de0db25ca7517 
  src/tests/master_quota_tests.cpp 3e7a94cad94b64faf01e73f880281d24098fadbd 


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


Testing
---

Updated a test.


Thanks,

Benjamin Mahler

Review Request 70274: Updated quota capacity check to not exclude disconnected agents.

[Benjamin Mahler]

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

Review request for mesos and Meng Zhu.


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


Repository: mesos


Description
---

The exclusion of disconnected agents produces a rather unstable
calculated capacity in the face of upgrades, transient network
issues, etc. To provide a more stable calculated capacity, this
includes registered agents, even when they are disconnected.

This also adds a TODO to include recovered agents so that the
calculated capacity after a failover is the same as it was prior
to the failover.


Diffs
-

  src/master/quota_handler.cpp 2dcfdcaf836f21d7d39b5ef5c36de0db25ca7517 


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


Testing
---

Added a test in the next patch.


Thanks,

Benjamin Mahler

Review Request 70278: Added an << operator for ResourceQuantities.

[Benjamin Mahler]

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

Review request for mesos and Meng Zhu.


Repository: mesos


Description
---

This follows the same output format as Resources.


Diffs
-

  src/common/resource_quantities.hpp e85ff4f3c9201a5409f292b0a43b99c73341a3d0 
  src/common/resource_quantities.cpp d9042e9c0fa5e90e619470830ca430a31b2ce5bc 
  src/tests/resource_quantities_tests.cpp 
441b48950e9adde92758cbbd09814c69bf6e6ca3 


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


Testing
---

Added a test.


Thanks,

Benjamin Mahler

Review Request 70275: Added a test ensuring quota capacity check includes disconnected agents.

[Benjamin Mahler]

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

Review request for mesos and Meng Zhu.


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


Repository: mesos


Description
---

See https://reviews.apache.org/r/70274 for the change that
prompted this test.


Diffs
-

  src/tests/master_quota_tests.cpp 3e7a94cad94b64faf01e73f880281d24098fadbd 


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


Testing
---

make check


Thanks,

Benjamin Mahler

Review Request 70276: Included RP resources / static reservations in quota capacity check.

[Benjamin Mahler]

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

Review request for mesos and Meng Zhu.


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


Repository: mesos


Description
---

Static reservations and resource provider resources both count towards
quota consumption and should therefore be included in the quota
capacity check.

Note also that this renames the function to be an "overcommit" check
rather than making any suggestions of being a "satisfiability" check.
The latter is far more complex to implement properly, and the current
quota API design includes a simple overcommit check rather than any
attempt as satisfiability checking.


Diffs
-

  src/master/master.hpp 77be494d1ca2d3cd79ab033e41a5ff66c0225f79 
  src/master/quota_handler.cpp 2dcfdcaf836f21d7d39b5ef5c36de0db25ca7517 


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


Testing
---

Added a test in the subsequent patch.


Thanks,

Benjamin Mahler

Review Request 70277: Added a test to ensure quota capacity check includes reservations.

[Benjamin Mahler]

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

Review request for mesos and Meng Zhu.


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


Repository: mesos


Description
---

This is done by having a single agent with static reservations,
and requesting quota for the same amount as the reservations.
Note that the quota is not satisfiable despite passing the quota
capacity check.


Diffs
-

  src/tests/master_quota_tests.cpp 3e7a94cad94b64faf01e73f880281d24098fadbd 


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


Testing
---

make check


Thanks,

Benjamin Mahler

Review Request 70273: Refactored the quota capcity heuristic check to be a static function.

[Benjamin Mahler]

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

Review request for mesos and Meng Zhu.


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


Repository: mesos


Description
---

This allows the capacity heuristic code to be moved up and unit tested.


Diffs
-

  src/master/master.hpp 77be494d1ca2d3cd79ab033e41a5ff66c0225f79 
  src/master/quota_handler.cpp 2dcfdcaf836f21d7d39b5ef5c36de0db25ca7517 


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


Testing
---

make check


Thanks,

Benjamin Mahler

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

[Benjamin Bannier]

> On March 19, 2019, 3:57 p.m., Benjamin Bannier wrote:
> > src/csi/v0_volume_manager.hpp
> > Lines 100 (patched)
> > 
> >
> > Does it make sense to move this into the process? It would lead to 
> > simpler dispatch in the wrapper around the process, and probably be more in 
> > line with the way we usually lay this out.
> 
> Chun-Hung Hsiao wrote:
> Yes I agree doing what you suggested is more consistent with the existing 
> codebase.
> 
> The reason I do this is because we can save one dispatch. If we move it 
> into the actor, it would be like:
> ```
> Future VolumeManagerProcess::attachVolume(...)
> {
>   return recovered
> .then(defer(self(), [=] { ... }));
> }
> ```
> Since `recovered` might carry a future that is satisfied in another 
> actor. And we'll have:
> ```
> Future VolumeManager::attachVolume(...)
> {
>   return dispatch(process.get(), &VolumeProcessManager::attachVolume, 
> ...);
> }
> ```
> See, we'll always get two dispatches in a single `attachVolume` call.
> 
> There are two ways to solve this:
> 1. Keep `recovered` in the actor, but do the following:
> ```
> // Ensure that the following future is satisfied in this actor.
> recovered = recover().then(defer(self(), [] { return Nothing(); }));
> ```
> So we could remove a `defer` in `attachVolume`. But the last `then` looks 
> a bit unnatural.
> 
> 2. Do what I did in this patch.
> 
> 3. Don't care about the two dispatches, since this is less likely to 
> introduce much overhead.
> 
> WDYT?

TBH, I don't really understand what (1) accomplishes as it seems that to be 
able to drop the `defer` we'd need to assert that we are already recovered.

I'd be in favor of (3) as it would just appear simpler and I do not expect a 
lot of contention on the actor which makes optimizing this less necessary, but 
I'll leave this up to you.


> On March 19, 2019, 3:57 p.m., Benjamin Bannier wrote:
> > src/csi/volume_manager.cpp
> > Line 43 (original), 49 (patched)
> > 
> >
> > Same question as on previous review: how do you plan to do 
> > version-dependent dispatch here?
> 
> Chun-Hung Hsiao wrote:
> Something like:
> ```
> // TODO(chhsiao): Do a move-capture with `Owned` once we get C++14.
> auto serviceManager = make_shared(new Owned(new 
> ServiceManager(...)));
> return (*serviceManager)->getCsiVersion()
>   .then([=](const string& version) -> Future> {
> if (version == v1::VERSION) {
>   return new v1::VolumeManager(..., std::move(*serviceManager));
> } else if (version == v0::VERSION) {
>   return new v0::VolumeManager(..., std::move(*serviceManager));
> }
> 
> UNREACHABLE();
>   });
> ```

Looks good.

Let's use a `shared_ptr` instead of an `Owned` so this code is semantically 
correct.


- Benjamin


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


On March 22, 2019, 7:12 a.m., Chun-Hung Hsiao wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/70214/
> ---
> 
> (Updated March 22, 2019, 7:12 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
> ---
> 
> Added skeleton code for v0 `VolumeManager`.
> 
> 
> Diffs
> -
> 
>   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/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Chun-Hung Hsiao
> 
>

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

[Mesos Reviewbot]

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



Bad review!

Reviews applied: [70248, 70258, 70247, 70225, 70223, 70222, 70217, 70215, 
70216, 70214, 70213, 70169, 70168, 70245]

Error:
2019-03-22 09:48:23 URL:https://reviews.apache.org/r/70222/diff/raw/ 
[80925/80925] -> "70222.patch" [1]
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


On March 21, 2019, 3:10 a.m., Chun-Hung Hsiao wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/70248/
> ---
> 
> (Updated March 21, 2019, 3:10 a.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 70268: Removed `limits` from `QuotaInfo` and `QuotaRequest` protobuf.

[Mesos Reviewbot]

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



Patch looks great!

Reviews applied: [70266, 70267, 70268]

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 22, 2019, 6:13 a.m., Meng Zhu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/70268/
> ---
> 
> (Updated March 22, 2019, 6:13 a.m.)
> 
> 
> 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
> 
>

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

[Mesos Reviewbot Windows]

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



PASS: Mesos patch 70268 was successfully built and tested.

Reviews applied: `['70266', '70267', '70268']`

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

- Mesos Reviewbot Windows


On March 22, 2019, 6:13 a.m., Meng Zhu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/70268/
> ---
> 
> (Updated March 22, 2019, 6:13 a.m.)
> 
> 
> 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
> 
>