Re: Review Request 69905: Added a SLRP unit test for failed persistent volume cleanup.

[Mesos Reviewbot Windows]

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



PASS: Mesos patch 69905 was successfully built and tested.

Reviews applied: `['69858', '69866', '69892', '69893', '69894', '69895', 
'69896', '69897', '69898', '69904', '69905']`

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

- Mesos Reviewbot Windows


On Feb. 6, 2019, 1:38 p.m., Chun-Hung Hsiao wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/69905/
> ---
> 
> (Updated Feb. 6, 2019, 1:38 p.m.)
> 
> 
> Review request for mesos and Benjamin Bannier.
> 
> 
> Bugs: MESOS-9544
> https://issues.apache.org/jira/browse/MESOS-9544
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Test `ROOT_CreateDestroyPersistentMountVolumeFailed` verifies that if
> SLRP fails to clean up a persistent volume, `DESTROY` would fail, the
> persistent volume would remain, and depended operations will be dropped.
> 
> 
> Diffs
> -
> 
>   src/tests/storage_local_resource_provider_tests.cpp 
> e8ed20f818ed7f1a3ce15758ea3c366520443377 
> 
> 
> Diff: https://reviews.apache.org/r/69905/diff/3/
> 
> 
> Testing
> ---
> 
> sudo make check
> 
> 
> Thanks,
> 
> Chun-Hung Hsiao
> 
>

Re: Review Request 69902: Fixed incorrect skipping in the allocation loops.

[Meng Zhu]

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


Fix it, then Ship it!




Thanks for fixing this! Can you add a test?


src/master/allocator/mesos/hierarchical.cpp
Lines 1799-1803 (patched)


I guess the reason we are not addressing this now is due to the overhead of 
subtraction and resource copies. If so, we probably want to spit that out. And, 
to me, the overhead looks like a legit reason for not doing the check here, so 
maybe just a note instead of the todo? ditto below.



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


why not put `.allocatableTo(role)` like below?
ditto blew in the second stage



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


Not in this patch, but I think we can save quite a few map lookups by doing 
something like:

foreach agent {
  agentOfferedShared = offeredSharedResources.get(slaveId)
  for each role {
for each framework {
  ...
  available -= agentOfferedShared...
  
  agentOfferedShared += toAllocate.shared()
}
  }
  offeredSharedResources[slaveId] = agentOfferedShared
}

ditto below in the second stage


- Meng Zhu


On Feb. 5, 2019, 2:30 p.m., Benjamin Mahler wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/69902/
> ---
> 
> (Updated Feb. 5, 2019, 2:30 p.m.)
> 
> 
> Review request for mesos, Benjamin Bannier and Meng Zhu.
> 
> 
> Bugs: MESOS-9554
> https://issues.apache.org/jira/browse/MESOS-9554
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> The allocation loop was breaking out of the framework loop based on
> resources filtered by the framework's capabilities. This can lead
> to incorrect breaking in the case that a framework is incapable of
> receiving resources whereas others are capable of receiving them.
> 
> 
> Diffs
> -
> 
>   src/master/allocator/mesos/hierarchical.cpp 
> bb9a9c95979f36c0564af5b3babb1c43077a363b 
> 
> 
> Diff: https://reviews.apache.org/r/69902/diff/1/
> 
> 
> Testing
> ---
> 
> make check
> 
> Ran benchmarks:
> 
> Before:
> ```
> [ RUN  ] 
> SlaveAndFrameworkCount/HierarchicalAllocator_BENCHMARK_Test.DeclineOffers/22
> Using 1 agents and 3000 frameworks
> Added 3000 frameworks in 161.434141ms
> Added 1 agents in 3.334691038secs
> round 0 allocate() took 2.770901571secs to make 1 offers after filtering 
> 1 offers
> round 1 allocate() took 2.470565355secs to make 1 offers after filtering 
> 2 offers
> round 2 allocate() took 2.594315228secs to make 1 offers after filtering 
> 3 offers
> round 3 allocate() took 2.389658909secs to make 1 offers after filtering 
> 4 offers
> round 4 allocate() took 2.580871077secs to make 1 offers after filtering 
> 5 offers
> round 5 allocate() took 2.522765768secs to make 1 offers after filtering 
> 6 offers
> round 6 allocate() took 2.513849225secs to make 1 offers after filtering 
> 7 offers
> round 7 allocate() took 2.500960884secs to make 1 offers after filtering 
> 8 offers
> round 8 allocate() took 2.52565301secs to make 1 offers after filtering 
> 9 offers
> round 9 allocate() took 2.445142852secs to make 1 offers after filtering 
> 10 offers
> round 10 allocate() took 2.450911751secs to make 1 offers after filtering 
> 11 offers
> ```
> 
> After:
> ```
> [ RUN  ] 
> SlaveAndFrameworkCount/HierarchicalAllocator_BENCHMARK_Test.DeclineOffers/22
> Using 1 agents and 3000 frameworks
> Added 3000 frameworks in 168.484316ms
> Added 1 agents in 3.371731069secs
> round 0 allocate() took 2.836207029secs to make 1 offers after filtering 
> 1 offers
> round 1 allocate() took 2.383960243secs to make 1 offers after filtering 
> 2 offers
> round 2 allocate() took 2.340208926secs to make 1 offers after filtering 
> 3 offers
> round 3 allocate() took 2.396341093secs to make 1 offers after filtering 
> 4 offers
> round 4 allocate() took 2.39952541secs to make 1 offers after filtering 
> 5 offers
> round 5 allocate() took 2.401885613secs to make 1 offers after filtering 
> 6 offers
> round 6 allocate() took 2.338538188secs to make 1 offers after filtering 
> 7 offers
> round 7 allocate() took 2.301651652secs to make 1 offers after filtering 
> 8 offers
> round 8 allocate() took 2.416522086secs to make 1 offers after filtering 
> 9 offers
> round 9 allocate() took 

Review Request 69905: Added a SLRP unit test for failed persistent volume cleanup.

[Chun-Hung Hsiao]

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

Review request for mesos and Benjamin Bannier.


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


Repository: mesos


Description
---

Test `ROOT_CreateDestroyPersistentMountVolumeFailed` verifies that if
SLRP fails to clean up a persistent volume, `DESTROY` would fail, the
persistent volume would remain, and depended operations will be dropped.


Diffs
-

  src/tests/storage_local_resource_provider_tests.cpp 
e8ed20f818ed7f1a3ce15758ea3c366520443377 


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


Testing
---

sudo make check


Thanks,

Chun-Hung Hsiao

Review Request 69904: Extracted common offer matching functions from SLRP tests.

[Chun-Hung Hsiao]

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

Review request for mesos and Benjamin Bannier.


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


Repository: mesos


Description
---

This patch extracts lambda functions `isStoragePool`, `isMountDisk` and
`isPreprovisionedVolume` from all tests and makes them common utitily
functions in the test fixture.


Diffs
-

  src/tests/storage_local_resource_provider_tests.cpp 
e8ed20f818ed7f1a3ce15758ea3c366520443377 


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


Testing
---

make check


Thanks,

Chun-Hung Hsiao

Re: Review Request 69893: Clean up persistent volumes on SLRP disks.

[Chun-Hung Hsiao]

> On Feb. 5, 2019, 5:55 p.m., Benjamin Bannier wrote:
> > src/resource_provider/storage/provider.cpp
> > Lines 3137 (patched)
> > 
> >
> > This is some pretty strong coupling to `master::validate` checking 
> > `resource::validatePersistentVolume`. That's probably okay, but would could 
> > also just re-validate here instead.

This is similar to what we do between the master and the agent: the master 
validates and the agent checks.


> On Feb. 5, 2019, 5:55 p.m., Benjamin Bannier wrote:
> > src/resource_provider/storage/provider.cpp
> > Lines 3125-3126 (original), 3138-3139 (patched)
> > 
> >
> > Do we actually validate this somewhere?

https://github.com/apache/mesos/blob/ae7e7330d7fc7bbe2ce923025025577e087f8348/src/master/validation.cpp#L2570-L2573


> On Feb. 5, 2019, 5:55 p.m., Benjamin Bannier wrote:
> > src/resource_provider/storage/provider.cpp
> > Lines 3230 (patched)
> > 
> >
> > Is this validated somewhere?

https://github.com/apache/mesos/blob/ae7e7330d7fc7bbe2ce923025025577e087f8348/src/master/validation.cpp#L2570-L2573
 combined with L3208--L3213 in this patch.


> On Feb. 5, 2019, 5:55 p.m., Benjamin Bannier wrote:
> > src/resource_provider/storage/provider.cpp
> > Lines 3234 (patched)
> > 
> >
> > Is this validated?

All MOUNT disk resources managed by SLRP will have an entry in `volumes`, and 
the master only allows an operation to be applied if its consumed resources are 
contained in the resource pool, hence this is validated implicitly by the 
master.


> On Feb. 5, 2019, 5:55 p.m., Benjamin Bannier wrote:
> > src/resource_provider/storage/provider.cpp
> > Lines 3244 (patched)
> > 
> >
> > Does this break seemless upgrades? Probably okay, but still something 
> > worth documenting in the upgrades guide.

This depends on what we'll do for 
https://reviews.apache.org/r/69892/#comment298376. In the current proposal 
where there are separated boot IDs, this won't introduce any upgrade issue 
since it will be empty anyway. During SLRP recovery, it would treat an existing 
published volume as `VOL_READY` but not `PUBLISHED` and will call 
`NodePublishVolume` again. But this is okay since `NodePublishVolume` is 
idempotent.


- Chun-Hung


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


On Feb. 5, 2019, 7:42 a.m., Chun-Hung Hsiao wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/69893/
> ---
> 
> (Updated Feb. 5, 2019, 7:42 a.m.)
> 
> 
> Review request for mesos, Benjamin Bannier, James DeFelice, and Jie Yu.
> 
> 
> Bugs: MESOS-9544
> https://issues.apache.org/jira/browse/MESOS-9544
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This patch limits SLRP to only support persistent volumes on MOUNT
> disks, and makes it clean up data in persistent volumes when processing
> `DESTROY` operations.
> 
> 
> Diffs
> -
> 
>   src/resource_provider/storage/provider.cpp 
> d6e20a549ede189c757ae3ae922ab7cb86d2be2c 
>   src/resource_provider/storage/provider_process.hpp PRE-CREATION 
> 
> 
> Diff: https://reviews.apache.org/r/69893/diff/1/
> 
> 
> Testing
> ---
> 
> `make check`
> 
> More testing done later in chain.
> 
> 
> Thanks,
> 
> Chun-Hung Hsiao
> 
>

Re: Review Request 69894: Disallowed `DESTROY_DISK` on persistent volumes.

[Chun-Hung Hsiao]

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

(Updated Feb. 6, 2019, 4:59 a.m.)


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


Changes
---

Addressed Benjamin's comment.


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


Repository: mesos


Description
---

`DESTROY_DISK` would bypass persistent volume cleanup and directly ask
the CSI plugin to delete the backed volume. Since the CSI spec does not
require the plugin to do data cleanup, to avoid data leakage, we require
that if there is persistent volume on the CSI volume, it should be
destroyed first.


Diffs (updated)
-

  src/master/validation.cpp a71edeb7827b534e27ad3e3398abe555bf36e741 
  src/tests/master_validation_tests.cpp 
c00e8bb315c28bdf438da2089dd81f5e348982e5 


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

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


Testing
---

make check


Thanks,

Chun-Hung Hsiao

Re: Review Request 69894: Disallowed `DESTROY_DISK` on persistent volumes.

[Chun-Hung Hsiao]

> On Feb. 5, 2019, 5:15 p.m., Benjamin Bannier wrote:
> > src/master/validation.cpp
> > Lines 2577-2579 (patched)
> > 
> >
> > Maybe make this more in line with what we already have for `UNRESERVE` 
> > of persistent volumes?
> > ```
> > A disk resource containing a persistent volume {source} cannot be 
> > destroyed directly. Please destroy the persistent volume first then destroy 
> > the disk resource.
> > ```
> > Also, spaces either at start or end of line, not mixed. At the start 
> > seems popular.

The line breaking is done by clang-format ;) But I'll follow your suggestion.


- Chun-Hung


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


On Feb. 5, 2019, 7:43 a.m., Chun-Hung Hsiao wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/69894/
> ---
> 
> (Updated Feb. 5, 2019, 7:43 a.m.)
> 
> 
> Review request for mesos, Benjamin Bannier, James DeFelice, and Jie Yu.
> 
> 
> Bugs: MESOS-9544
> https://issues.apache.org/jira/browse/MESOS-9544
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> `DESTROY_DISK` would bypass persistent volume cleanup and directly ask
> the CSI plugin to delete the backed volume. Since the CSI spec does not
> require the plugin to do data cleanup, to avoid data leakage, we require
> that if there is persistent volume on the CSI volume, it should be
> destroyed first.
> 
> 
> Diffs
> -
> 
>   src/master/validation.cpp a71edeb7827b534e27ad3e3398abe555bf36e741 
>   src/tests/master_validation_tests.cpp 
> c00e8bb315c28bdf438da2089dd81f5e348982e5 
> 
> 
> Diff: https://reviews.apache.org/r/69894/diff/1/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Chun-Hung Hsiao
> 
>

Re: Review Request 69892: Made SLRP recover node-published volumes after reboot.

[Chun-Hung Hsiao]

> On Feb. 5, 2019, 5:41 p.m., Benjamin Bannier wrote:
> > src/csi/state.proto
> > Lines 62-67 (original), 62-77 (patched)
> > 
> >
> > Any reason we cannot use a single field containing the `bootId` of the 
> > last transition? A single field would cut down on the number of possible 
> > message permutations, and also allow simpler handling (branching a changed 
> > `boot_id`, triggering `state`-dependent handling). We could set such a 
> > `boot_id` whenever there is a state transition.
> 
> Chun-Hung Hsiao wrote:
> Consider the following scenario:
> `CREATED` -> `NODE_READY` -> `VOL_READY` -> `PUBLISHED` -> reboot -> 
> `VOL_READY` -> reboot
> If we share the same `boot_id` for all transitions, we won't be able to 
> tell that this volume has been published before.
> If we dedicate `boot_id` to `PUBLISHED`, we won't be able to know that 
> there has been a reboot after the last `VOL_READY` so we need to call 
> `NodeStageVolume` again.
> 
> Chun-Hung Hsiao wrote:
> After an offline discussion, we decided to simplify the state machine, 
> and SLRP will try to bring a volume to `PUBLISHED` during recovery as long as 
> it has ever been in `VOL_READY` before. This would mean that a 
> misconfiguration that makes a plugin succeed on `NodeStageVolume` but fail on 
> `NodePublishVolume` will make the SLRP unable destroy the persisten volume, 
> even if no data have ever been written to it.
> 
> James DeFelice wrote:
> How would an operator recover from such a sitution?

The operator needs to be somehow notified, then investigate the problem, then 
restart the SLRP (through a config update or agent restart).

It seems to me that the scenario where the plugin is able to do 
`NodeStageVolume` but not `NodePublishVolume` should be rare.

However, it is more likely that the plugin is not 
`STAGE_UNSTAGE_VOLUME`-capable and misconfigured in a way that it cannot do 
`NodePublishVolume`.
In this case SLRP would use a no-op to transition a volume from `NODE_READY` to 
`VOL_READY`, and set up the boot ID. However, because it cannot use the plugin 
to bring the volume to `PUBLISHED`, it won't be able to destroy the PV or 
recover.

The originaly proposal, where we keep track of the boot IDs of 
`NodeStageVolume` and `NodePublishVolume` separatedly, is more resilient to bad 
plugins: PV destroy and SLRP recovery is blocked only when necessary. WDYT?


- Chun-Hung


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


On Feb. 5, 2019, 7:40 a.m., Chun-Hung Hsiao wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/69892/
> ---
> 
> (Updated Feb. 5, 2019, 7:40 a.m.)
> 
> 
> Review request for mesos, Benjamin Bannier, James DeFelice, and Jie Yu.
> 
> 
> Bugs: MESOS-9544
> https://issues.apache.org/jira/browse/MESOS-9544
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> If a CSI volume has been node-published before a reboot, SLRP will now
> try to bring it back to node-published again. This is important to
> perform synchronous persistent volume cleanup for `DESTROY`.
> 
> To achieve this, in addition to keeping track of the boot ID when a CSI
> volume is node-staged in `VolumeState.vol_ready_boot_id` (formerly
> `VolumeState.boot_id`), SLRP now also keeps track of the boot ID when
> the volume is node-published. This helps SLRP to better determine if a
> volume has been published before reboot.
> 
> 
> Diffs
> -
> 
>   src/csi/state.proto 264a5657dd37605a6f3bdadd0e8d18ba9673191a 
>   src/resource_provider/storage/provider.cpp 
> d6e20a549ede189c757ae3ae922ab7cb86d2be2c 
>   src/tests/storage_local_resource_provider_tests.cpp 
> e8ed20f818ed7f1a3ce15758ea3c366520443377 
> 
> 
> Diff: https://reviews.apache.org/r/69892/diff/1/
> 
> 
> Testing
> ---
> 
> `make check`
> 
> Testing for publish failures will be done later in chain.
> 
> 
> Thanks,
> 
> Chun-Hung Hsiao
> 
>

Re: Review Request 69900: Reduced unnecessary agent lookups in the allocation loops.

[Meng Zhu]

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


Ship it!




Ship It!

- Meng Zhu


On Feb. 5, 2019, 2:30 p.m., Benjamin Mahler wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/69900/
> ---
> 
> (Updated Feb. 5, 2019, 2:30 p.m.)
> 
> 
> Review request for mesos, Benjamin Bannier and Meng Zhu.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Reduced unnecessary agent lookups in the allocation loops.
> 
> 
> Diffs
> -
> 
>   src/master/allocator/mesos/hierarchical.cpp 
> bb9a9c95979f36c0564af5b3babb1c43077a363b 
> 
> 
> Diff: https://reviews.apache.org/r/69900/diff/1/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Benjamin Mahler
> 
>

Re: Review Request 69869: [WIP] Added test for tearing down frameworks while creating disks.

[Mesos Reviewbot Windows]

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



PASS: Mesos patch 69869 was successfully built and tested.

Reviews applied: `['69872', '69869']`

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

- Mesos Reviewbot Windows


On Jan. 31, 2019, 1:38 a.m., Joseph Wu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/69869/
> ---
> 
> (Updated Jan. 31, 2019, 1:38 a.m.)
> 
> 
> Review request for mesos, Benno Evers, Gastón Kleiman, and Greg Mann.
> 
> 
> Bugs: MESOS-9542
> https://issues.apache.org/jira/browse/MESOS-9542
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> The CREATE_DISK and DESTROY_DISK operations are "non-speculative"
> operations, which means the master must wait for the operations to
> complete successfully before the master can update its resources.
> Because the master must wait to update the results of non-speculative
> operations, it is possible for the framework making the
> CREATE/DESTROY_DISK to be torn down before the operation completes.
> 
> This commit adds a test to make sure the master can gracefully handle
> such a case.
> 
> 
> Diffs
> -
> 
>   src/tests/storage_local_resource_provider_tests.cpp 
> fb001aa8d32d1a0a03014a35772fe10b65ce8d9a 
> 
> 
> Diff: https://reviews.apache.org/r/69869/diff/2/
> 
> 
> Testing
> ---
> 
> The test currently fails with the exact message as MESOS-9542, which is the 
> intended behavior right now (we are discussing the fix).
> 
> src/mesos-tests 
> --gtest_filter="StorageLocalResourceProviderTest.FrameworkTeardownBeforeTerminalOperationStatusUpdate"
>  --verbose
> 
> 
> Thanks,
> 
> Joseph Wu
> 
>

Re: Review Request 69898: Updated `CreateDestroyDisk*` SLRP tests to test pipelined operations.

[Mesos Reviewbot]

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



Patch looks great!

Reviews applied: [69858, 69866, 69892, 69893, 69894, 69895, 69896, 69897, 69898]

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 Feb. 5, 2019, 8:06 a.m., Chun-Hung Hsiao wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/69898/
> ---
> 
> (Updated Feb. 5, 2019, 8:06 a.m.)
> 
> 
> Review request for mesos and Benjamin Bannier.
> 
> 
> Bugs: MESOS-9544
> https://issues.apache.org/jira/browse/MESOS-9544
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This patch extends the code coverage of the `CreateDestroyDisk` and
> `CreateDestroyDiskRecovery` tests by testing pipelined `RESERVE`,
> `CREATE`, `DESTROY` and `UNRESERVE` operations along with `CREATE_DISK`
> and `DESTROY_DISK`. It also renames `CreateDestroyDiskRecovery` to
> `CreateDestroyDiskWithRecovery` for consistency.
> 
> 
> Diffs
> -
> 
>   src/tests/storage_local_resource_provider_tests.cpp 
> e8ed20f818ed7f1a3ce15758ea3c366520443377 
> 
> 
> Diff: https://reviews.apache.org/r/69898/diff/1/
> 
> 
> Testing
> ---
> 
> `make check`
> 
> Run in repetition
> 
> 
> Thanks,
> 
> Chun-Hung Hsiao
> 
>

Re: Review Request 69892: Made SLRP recover node-published volumes after reboot.

[James DeFelice]

> On Feb. 5, 2019, 5:41 p.m., Benjamin Bannier wrote:
> > src/csi/state.proto
> > Lines 62-67 (original), 62-77 (patched)
> > 
> >
> > Any reason we cannot use a single field containing the `bootId` of the 
> > last transition? A single field would cut down on the number of possible 
> > message permutations, and also allow simpler handling (branching a changed 
> > `boot_id`, triggering `state`-dependent handling). We could set such a 
> > `boot_id` whenever there is a state transition.
> 
> Chun-Hung Hsiao wrote:
> Consider the following scenario:
> `CREATED` -> `NODE_READY` -> `VOL_READY` -> `PUBLISHED` -> reboot -> 
> `VOL_READY` -> reboot
> If we share the same `boot_id` for all transitions, we won't be able to 
> tell that this volume has been published before.
> If we dedicate `boot_id` to `PUBLISHED`, we won't be able to know that 
> there has been a reboot after the last `VOL_READY` so we need to call 
> `NodeStageVolume` again.
> 
> Chun-Hung Hsiao wrote:
> After an offline discussion, we decided to simplify the state machine, 
> and SLRP will try to bring a volume to `PUBLISHED` during recovery as long as 
> it has ever been in `VOL_READY` before. This would mean that a 
> misconfiguration that makes a plugin succeed on `NodeStageVolume` but fail on 
> `NodePublishVolume` will make the SLRP unable destroy the persisten volume, 
> even if no data have ever been written to it.

How would an operator recover from such a sitution?


- James


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


On Feb. 5, 2019, 7:40 a.m., Chun-Hung Hsiao wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/69892/
> ---
> 
> (Updated Feb. 5, 2019, 7:40 a.m.)
> 
> 
> Review request for mesos, Benjamin Bannier, James DeFelice, and Jie Yu.
> 
> 
> Bugs: MESOS-9544
> https://issues.apache.org/jira/browse/MESOS-9544
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> If a CSI volume has been node-published before a reboot, SLRP will now
> try to bring it back to node-published again. This is important to
> perform synchronous persistent volume cleanup for `DESTROY`.
> 
> To achieve this, in addition to keeping track of the boot ID when a CSI
> volume is node-staged in `VolumeState.vol_ready_boot_id` (formerly
> `VolumeState.boot_id`), SLRP now also keeps track of the boot ID when
> the volume is node-published. This helps SLRP to better determine if a
> volume has been published before reboot.
> 
> 
> Diffs
> -
> 
>   src/csi/state.proto 264a5657dd37605a6f3bdadd0e8d18ba9673191a 
>   src/resource_provider/storage/provider.cpp 
> d6e20a549ede189c757ae3ae922ab7cb86d2be2c 
>   src/tests/storage_local_resource_provider_tests.cpp 
> e8ed20f818ed7f1a3ce15758ea3c366520443377 
> 
> 
> Diff: https://reviews.apache.org/r/69892/diff/1/
> 
> 
> Testing
> ---
> 
> `make check`
> 
> Testing for publish failures will be done later in chain.
> 
> 
> Thanks,
> 
> Chun-Hung Hsiao
> 
>

Re: Review Request 69858: Persisted intentionally dropped operations in SLRP.

[Chun-Hung Hsiao]

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

(Updated Feb. 6, 2019, 12:04 a.m.)


Review request for mesos, Benjamin Bannier, Gastón Kleiman, and Greg Mann.


Changes
---

Addressed Benjamin's comments and fixed a bug.


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


Repository: mesos


Description
---

If an operation is dropped intentionally (e.g., because of a resource
version mismatch), the operation should be persisted so no conflicting
status update would be generated for operation reconciliation.


Diffs (updated)
-

  src/resource_provider/storage/provider.cpp 
d6e20a549ede189c757ae3ae922ab7cb86d2be2c 
  src/resource_provider/storage/provider_process.hpp PRE-CREATION 


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

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


Testing
---

`make check`

Additional test MESOS-9537 is added later in chain.


Thanks,

Chun-Hung Hsiao

Re: Review Request 69892: Made SLRP recover node-published volumes after reboot.

[Chun-Hung Hsiao]

> On Feb. 5, 2019, 5:41 p.m., Benjamin Bannier wrote:
> > src/csi/state.proto
> > Lines 62-67 (original), 62-77 (patched)
> > 
> >
> > Any reason we cannot use a single field containing the `bootId` of the 
> > last transition? A single field would cut down on the number of possible 
> > message permutations, and also allow simpler handling (branching a changed 
> > `boot_id`, triggering `state`-dependent handling). We could set such a 
> > `boot_id` whenever there is a state transition.
> 
> Chun-Hung Hsiao wrote:
> Consider the following scenario:
> `CREATED` -> `NODE_READY` -> `VOL_READY` -> `PUBLISHED` -> reboot -> 
> `VOL_READY` -> reboot
> If we share the same `boot_id` for all transitions, we won't be able to 
> tell that this volume has been published before.
> If we dedicate `boot_id` to `PUBLISHED`, we won't be able to know that 
> there has been a reboot after the last `VOL_READY` so we need to call 
> `NodeStageVolume` again.

After an offline discussion, we decided to simplify the state machine, and SLRP 
will try to bring a volume to `PUBLISHED` during recovery as long as it has 
ever been in `VOL_READY` before. This would mean that a misconfiguration that 
makes a plugin succeed on `NodeStageVolume` but fail on `NodePublishVolume` 
will make the SLRP unable destroy the persisten volume, even if no data have 
ever been written to it.


- Chun-Hung


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


On Feb. 5, 2019, 7:40 a.m., Chun-Hung Hsiao wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/69892/
> ---
> 
> (Updated Feb. 5, 2019, 7:40 a.m.)
> 
> 
> Review request for mesos, Benjamin Bannier, James DeFelice, and Jie Yu.
> 
> 
> Bugs: MESOS-9544
> https://issues.apache.org/jira/browse/MESOS-9544
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> If a CSI volume has been node-published before a reboot, SLRP will now
> try to bring it back to node-published again. This is important to
> perform synchronous persistent volume cleanup for `DESTROY`.
> 
> To achieve this, in addition to keeping track of the boot ID when a CSI
> volume is node-staged in `VolumeState.vol_ready_boot_id` (formerly
> `VolumeState.boot_id`), SLRP now also keeps track of the boot ID when
> the volume is node-published. This helps SLRP to better determine if a
> volume has been published before reboot.
> 
> 
> Diffs
> -
> 
>   src/csi/state.proto 264a5657dd37605a6f3bdadd0e8d18ba9673191a 
>   src/resource_provider/storage/provider.cpp 
> d6e20a549ede189c757ae3ae922ab7cb86d2be2c 
>   src/tests/storage_local_resource_provider_tests.cpp 
> e8ed20f818ed7f1a3ce15758ea3c366520443377 
> 
> 
> Diff: https://reviews.apache.org/r/69892/diff/1/
> 
> 
> Testing
> ---
> 
> `make check`
> 
> Testing for publish failures will be done later in chain.
> 
> 
> Thanks,
> 
> Chun-Hung Hsiao
> 
>

Re: Review Request 69902: Fixed incorrect skipping in the allocation loops.

[Mesos Reviewbot Windows]

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



PASS: Mesos patch 69902 was successfully built and tested.

Reviews applied: `['69900', '69902']`

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

- Mesos Reviewbot Windows


On Feb. 5, 2019, 10:30 p.m., Benjamin Mahler wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/69902/
> ---
> 
> (Updated Feb. 5, 2019, 10:30 p.m.)
> 
> 
> Review request for mesos, Benjamin Bannier and Meng Zhu.
> 
> 
> Bugs: MESOS-9554
> https://issues.apache.org/jira/browse/MESOS-9554
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> The allocation loop was breaking out of the framework loop based on
> resources filtered by the framework's capabilities. This can lead
> to incorrect breaking in the case that a framework is incapable of
> receiving resources whereas others are capable of receiving them.
> 
> 
> Diffs
> -
> 
>   src/master/allocator/mesos/hierarchical.cpp 
> bb9a9c95979f36c0564af5b3babb1c43077a363b 
> 
> 
> Diff: https://reviews.apache.org/r/69902/diff/1/
> 
> 
> Testing
> ---
> 
> make check
> 
> Ran benchmarks:
> 
> Before:
> ```
> [ RUN  ] 
> SlaveAndFrameworkCount/HierarchicalAllocator_BENCHMARK_Test.DeclineOffers/22
> Using 1 agents and 3000 frameworks
> Added 3000 frameworks in 161.434141ms
> Added 1 agents in 3.334691038secs
> round 0 allocate() took 2.770901571secs to make 1 offers after filtering 
> 1 offers
> round 1 allocate() took 2.470565355secs to make 1 offers after filtering 
> 2 offers
> round 2 allocate() took 2.594315228secs to make 1 offers after filtering 
> 3 offers
> round 3 allocate() took 2.389658909secs to make 1 offers after filtering 
> 4 offers
> round 4 allocate() took 2.580871077secs to make 1 offers after filtering 
> 5 offers
> round 5 allocate() took 2.522765768secs to make 1 offers after filtering 
> 6 offers
> round 6 allocate() took 2.513849225secs to make 1 offers after filtering 
> 7 offers
> round 7 allocate() took 2.500960884secs to make 1 offers after filtering 
> 8 offers
> round 8 allocate() took 2.52565301secs to make 1 offers after filtering 
> 9 offers
> round 9 allocate() took 2.445142852secs to make 1 offers after filtering 
> 10 offers
> round 10 allocate() took 2.450911751secs to make 1 offers after filtering 
> 11 offers
> ```
> 
> After:
> ```
> [ RUN  ] 
> SlaveAndFrameworkCount/HierarchicalAllocator_BENCHMARK_Test.DeclineOffers/22
> Using 1 agents and 3000 frameworks
> Added 3000 frameworks in 168.484316ms
> Added 1 agents in 3.371731069secs
> round 0 allocate() took 2.836207029secs to make 1 offers after filtering 
> 1 offers
> round 1 allocate() took 2.383960243secs to make 1 offers after filtering 
> 2 offers
> round 2 allocate() took 2.340208926secs to make 1 offers after filtering 
> 3 offers
> round 3 allocate() took 2.396341093secs to make 1 offers after filtering 
> 4 offers
> round 4 allocate() took 2.39952541secs to make 1 offers after filtering 
> 5 offers
> round 5 allocate() took 2.401885613secs to make 1 offers after filtering 
> 6 offers
> round 6 allocate() took 2.338538188secs to make 1 offers after filtering 
> 7 offers
> round 7 allocate() took 2.301651652secs to make 1 offers after filtering 
> 8 offers
> round 8 allocate() took 2.416522086secs to make 1 offers after filtering 
> 9 offers
> round 9 allocate() took 2.68406696secs to make 1 offers after filtering 
> 10 offers
> round 10 allocate() took 2.764043651secs to make 1 offers after filtering 
> 11 offers
> ```
> 
> 
> Thanks,
> 
> Benjamin Mahler
> 
>

Re: Review Request 69866: Updated SLRP test `ProfileDisappeared` to request operation feedback.

[Chun-Hung Hsiao]

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

(Updated Feb. 6, 2019, 12:05 a.m.)


Review request for mesos, Benjamin Bannier, Gastón Kleiman, and Greg Mann.


Changes
---

Addressed Benjamin's comments.


Summary (updated)
-

Updated SLRP test `ProfileDisappeared` to request operation feedback.


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


Repository: mesos


Description (updated)
---

This patch updates `StorageLocalResourceProviderTest.ProfileDisappeared`
to use the v1 scheduler API to request operation feedback, so MESOS-9537
would be triggered when an outstanding `UPDATE_STATE` call from the
resource provider races with an offer operation.


Diffs (updated)
-

  src/tests/mesos.hpp c2a5e5531de7498241e537ef1699e1a5e669b550 
  src/tests/storage_local_resource_provider_tests.cpp 
e8ed20f818ed7f1a3ce15758ea3c366520443377 


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

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


Testing
---

`make check`

Ran the test ~1000 times.

This test will fail without r/69858:
```
E0130 22:48:02.790385 24040 provider.cpp:2903] Failed to update status of 
operation (uuid: 2bf6813c-bde9-4c0e-8831-a779f5dc50ac): Mismatched framework ID 
for operation status update OPERATION_DROPPED (Status UUID: 
8a625c40-96e8-46e2-8050-69843032969a) for operation UUID 
2bf6813c-bde9-4c0e-8831-a779f5dc50ac on agent 
bfa6e505-54b7-4bb8-a8aa-99fb12291fb1-S0 (expected 
bfa6e505-54b7-4bb8-a8aa-99fb12291fb1- got no framework ID)
I0130 22:48:02.791590 24042 manager.cpp:163] Terminating resource provider 
765391c2-9485-4614-a6e5-45e67ef7a92c
...
../../src/tests/storage_local_resource_provider_tests.cpp:1387: Failure
  Expected: v1::OPERATION_FINISHED
  Which is: OPERATION_FINISHED
To be equal to: update->status().state()
  Which is: OPERATION_ERROR
../../src/tests/storage_local_resource_provider_tests.cpp:1393: Failure
Failed to wait 15secs for offers
...
../../src/tests/storage_local_resource_provider_tests.cpp:1361: Failure
Actual function call count doesn't match EXPECT_CALL(*scheduler, offers(_, 
v1::scheduler::OffersHaveAnyResource( std::bind(isStoragePool, lambda::_1, 
"test2"...
 Expected: to be called once
   Actual: never called - unsatisfied and active
```


Thanks,

Chun-Hung Hsiao

Re: Review Request 69858: Persisted intentionally dropped operations in SLRP.

[Chun-Hung Hsiao]

> On Feb. 5, 2019, 12:08 p.m., Benjamin Bannier wrote:
> > src/resource_provider/storage/provider.cpp
> > Lines 2889 (patched)
> > 
> >
> > Why is this always true? Probably a good idea to document this here if 
> > we make this assertion. I'd suggest to not make this assertion and instead 
> > simplify the control flow, see below.

Hmm good point. If we support operator API in the future than this assertion 
won't hold.


> On Feb. 5, 2019, 12:08 p.m., Benjamin Bannier wrote:
> > src/resource_provider/storage/provider.cpp
> > Lines 2888-2892 (original), 2898-2910 (patched)
> > 
> >
> > What do you think about constructing a single update outside of any 
> > condition (like was done previously), and then only have branching 
> > depending on whether we want to use the status update manager or send 
> > unreliably?
> > 
> > I think this would read much better if we'd:
> > * gather all required input to create the status update in variables,
> > * construct the status update, and
> > * a single `if/else` for the sending.
> > * update metrics in a single place

I come up with a slightly different refactor. Please give feedback if it's 
still not readable :) Leaving this open for now.


> On Feb. 5, 2019, 12:08 p.m., Benjamin Bannier wrote:
> > src/resource_provider/storage/provider.cpp
> > Lines 2923 (patched)
> > 
> >
> > We could introduce a variable for this.

Resolved through a different refactor. Closing.


- Chun-Hung


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


On Jan. 30, 2019, 10:35 p.m., Chun-Hung Hsiao wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/69858/
> ---
> 
> (Updated Jan. 30, 2019, 10:35 p.m.)
> 
> 
> Review request for mesos, Benjamin Bannier, Gastón Kleiman, and Greg Mann.
> 
> 
> Bugs: MESOS-9537
> https://issues.apache.org/jira/browse/MESOS-9537
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> If an operation is dropped intentionally (e.g., because of a resource
> version mismatch), the operation should be persisted so no conflicting
> status update would be generated for operation reconciliation.
> 
> 
> Diffs
> -
> 
>   src/resource_provider/storage/provider.cpp 
> d6e20a549ede189c757ae3ae922ab7cb86d2be2c 
>   src/resource_provider/storage/provider_process.hpp PRE-CREATION 
> 
> 
> Diff: https://reviews.apache.org/r/69858/diff/3/
> 
> 
> Testing
> ---
> 
> `make check`
> 
> Additional test MESOS-9537 is added later in chain.
> 
> 
> Thanks,
> 
> Chun-Hung Hsiao
> 
>

Re: Review Request 69866: Updated SLRP test `ProfileDisappeared` to request operation feedbacks.

[Chun-Hung Hsiao]

> On Feb. 5, 2019, 12:07 p.m., Benjamin Bannier wrote:
> > src/tests/storage_local_resource_provider_tests.cpp
> > Lines 1342-1343 (original), 1391-1392 (patched)
> > 
> >
> > This seems weird as `UpdateSlaveMessages` should only be triggered for 
> > speculation failures and `DESTROY_DISK` should never be speculated.
> > 
> > Don't we see the update due to the profile changes (which triggered a 
> > storage pool reconciliation)?

Hmm I'm skipping some important details. It should be that `DESTROY_DISK` on a 
disappeared profile triggers a reconciliation, which in turns triggers a an 
update. However this is too implementation-specific and might change, so let me 
make the comment more generic.


- Chun-Hung


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


On Feb. 5, 2019, 7:38 a.m., Chun-Hung Hsiao wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/69866/
> ---
> 
> (Updated Feb. 5, 2019, 7:38 a.m.)
> 
> 
> Review request for mesos, Benjamin Bannier, Gastón Kleiman, and Greg Mann.
> 
> 
> Bugs: MESOS-9537
> https://issues.apache.org/jira/browse/MESOS-9537
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This patch updates `StorageLocalResourceProviderTest.ProfileDisappeared`
> to use the v1 scheduler API to request operation feedbacks, so
> MESOS-9537 would be triggered when an outstanding `UPDATE_STATE` call
> from the resource provider races with an offer operation.
> 
> 
> Diffs
> -
> 
>   src/tests/mesos.hpp c2a5e5531de7498241e537ef1699e1a5e669b550 
>   src/tests/storage_local_resource_provider_tests.cpp 
> e8ed20f818ed7f1a3ce15758ea3c366520443377 
> 
> 
> Diff: https://reviews.apache.org/r/69866/diff/3/
> 
> 
> Testing
> ---
> 
> `make check`
> 
> Ran the test ~1000 times.
> 
> This test will fail without r/69858:
> ```
> E0130 22:48:02.790385 24040 provider.cpp:2903] Failed to update status of 
> operation (uuid: 2bf6813c-bde9-4c0e-8831-a779f5dc50ac): Mismatched framework 
> ID for operation status update OPERATION_DROPPED (Status UUID: 
> 8a625c40-96e8-46e2-8050-69843032969a) for operation UUID 
> 2bf6813c-bde9-4c0e-8831-a779f5dc50ac on agent 
> bfa6e505-54b7-4bb8-a8aa-99fb12291fb1-S0 (expected 
> bfa6e505-54b7-4bb8-a8aa-99fb12291fb1- got no framework ID)
> I0130 22:48:02.791590 24042 manager.cpp:163] Terminating resource provider 
> 765391c2-9485-4614-a6e5-45e67ef7a92c
> ...
> ../../src/tests/storage_local_resource_provider_tests.cpp:1387: Failure
>   Expected: v1::OPERATION_FINISHED
>   Which is: OPERATION_FINISHED
> To be equal to: update->status().state()
>   Which is: OPERATION_ERROR
> ../../src/tests/storage_local_resource_provider_tests.cpp:1393: Failure
> Failed to wait 15secs for offers
> ...
> ../../src/tests/storage_local_resource_provider_tests.cpp:1361: Failure
> Actual function call count doesn't match EXPECT_CALL(*scheduler, offers(_, 
> v1::scheduler::OffersHaveAnyResource( std::bind(isStoragePool, lambda::_1, 
> "test2"...
>  Expected: to be called once
>Actual: never called - unsatisfied and active
> ```
> 
> 
> Thanks,
> 
> Chun-Hung Hsiao
> 
>

Review Request 69900: Reduced unnecessary agent lookups in the allocation loops.

[Benjamin Mahler]

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

Review request for mesos, Benjamin Bannier and Meng Zhu.


Repository: mesos


Description
---

Reduced unnecessary agent lookups in the allocation loops.


Diffs
-

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


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


Testing
---

make check


Thanks,

Benjamin Mahler

Re: Review Request 69821: Enforced minimal allocatable resources in the hierarchical allocator.

[Benjamin Mahler]

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



Much appreciated for having split out the changes! This is nice and small now :)

I had a hard time figuring out what the logic should be if MESOS-9554 isn't 
addressed by this patch, so I ended up pulling out a fix:

https://reviews.apache.org/r/69902/ (note the 1 small dependency patch in front 
of it)

We can base this patch on this fixed version of the code and it should be 
pretty clear. If we can land the fix quickly then we could just commit it and 
rebase this patch.


src/master/allocator/mesos/hierarchical.cpp
Lines 1985-1996 (original), 1985-1988 (patched)


It's hard to review this logic without fixing MESOS-9554, so I've put up a 
patch:

https://reviews.apache.org/r/69902/

Note that there's a small dependency pulled out in front of it to make it a 
smaller change. We can make this review depend on it, or probably easier, land 
that patch and then rebase this against master.


- Benjamin Mahler


On Feb. 5, 2019, 4:38 p.m., Benjamin Bannier wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/69821/
> ---
> 
> (Updated Feb. 5, 2019, 4:38 p.m.)
> 
> 
> Review request for mesos, Benjamin Mahler and Meng Zhu.
> 
> 
> Bugs: MESOS-9523
> https://issues.apache.org/jira/browse/MESOS-9523
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This patch modifies the hierarchical allocator to take
> framework-specified minimal allocatable resources into account.
> 
> While previously the allocator was inspecting the minimal allocatable
> resources specified in its global options, it can now also inspects
> framework-specific resource requirements. With that frameworks can e.g.,
> configure resource requirements above the default minimal allocatable
> resource, or opt into receiving resources considered too small to be
> allocatable by the allocator in its default behavior.
> 
> For that we change the hierarchical allocator's `allocatable` function
> to be framework and role-specific. As that does in some places not allow
> us to abort iterating over candidate resource consumers like before an
> additional check on whether any resources are left in an allocation
> cycle is added as a break condition.
> 
> 
> Diffs
> -
> 
>   src/master/allocator/mesos/hierarchical.hpp 
> 1420c2638786d85f7b04379e5d79e59990c3e6cf 
>   src/master/allocator/mesos/hierarchical.cpp 
> bb9a9c95979f36c0564af5b3babb1c43077a363b 
> 
> 
> Diff: https://reviews.apache.org/r/69821/diff/8/
> 
> 
> Testing
> ---
> 
> `make check`
> 
> 
> File Attachments
> 
> 
> Ratio new/old timings
>   
> https://reviews.apache.org/media/uploaded/files/2019/01/31/d76189de-8882-4aff-956b-090dab729358__new_over_old.png
> 
> 
> Thanks,
> 
> Benjamin Bannier
> 
>

Review Request 69902: Fixed incorrect skipping in the allocation loops.

[Benjamin Mahler]

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

Review request for mesos, Benjamin Bannier and Meng Zhu.


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


Repository: mesos


Description
---

The allocation loop was breaking out of the framework loop based on
resources filtered by the framework's capabilities. This can lead
to incorrect breaking in the case that a framework is incapable of
receiving resources whereas others are capable of receiving them.


Diffs
-

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


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


Testing
---

make check

Ran benchmarks:

Before:
```
[ RUN  ] 
SlaveAndFrameworkCount/HierarchicalAllocator_BENCHMARK_Test.DeclineOffers/22
Using 1 agents and 3000 frameworks
Added 3000 frameworks in 161.434141ms
Added 1 agents in 3.334691038secs
round 0 allocate() took 2.770901571secs to make 1 offers after filtering 
1 offers
round 1 allocate() took 2.470565355secs to make 1 offers after filtering 
2 offers
round 2 allocate() took 2.594315228secs to make 1 offers after filtering 
3 offers
round 3 allocate() took 2.389658909secs to make 1 offers after filtering 
4 offers
round 4 allocate() took 2.580871077secs to make 1 offers after filtering 
5 offers
round 5 allocate() took 2.522765768secs to make 1 offers after filtering 
6 offers
round 6 allocate() took 2.513849225secs to make 1 offers after filtering 
7 offers
round 7 allocate() took 2.500960884secs to make 1 offers after filtering 
8 offers
round 8 allocate() took 2.52565301secs to make 1 offers after filtering 
9 offers
round 9 allocate() took 2.445142852secs to make 1 offers after filtering 
10 offers
round 10 allocate() took 2.450911751secs to make 1 offers after filtering 
11 offers
```

After:
```
[ RUN  ] 
SlaveAndFrameworkCount/HierarchicalAllocator_BENCHMARK_Test.DeclineOffers/22
Using 1 agents and 3000 frameworks
Added 3000 frameworks in 168.484316ms
Added 1 agents in 3.371731069secs
round 0 allocate() took 2.836207029secs to make 1 offers after filtering 
1 offers
round 1 allocate() took 2.383960243secs to make 1 offers after filtering 
2 offers
round 2 allocate() took 2.340208926secs to make 1 offers after filtering 
3 offers
round 3 allocate() took 2.396341093secs to make 1 offers after filtering 
4 offers
round 4 allocate() took 2.39952541secs to make 1 offers after filtering 
5 offers
round 5 allocate() took 2.401885613secs to make 1 offers after filtering 
6 offers
round 6 allocate() took 2.338538188secs to make 1 offers after filtering 
7 offers
round 7 allocate() took 2.301651652secs to make 1 offers after filtering 
8 offers
round 8 allocate() took 2.416522086secs to make 1 offers after filtering 
9 offers
round 9 allocate() took 2.68406696secs to make 1 offers after filtering 
10 offers
round 10 allocate() took 2.764043651secs to make 1 offers after filtering 
11 offers
```


Thanks,

Benjamin Mahler

Re: Review Request 69892: Made SLRP recover node-published volumes after reboot.

[Chun-Hung Hsiao]

> On Feb. 5, 2019, 5:41 p.m., Benjamin Bannier wrote:
> > src/csi/state.proto
> > Lines 62-67 (original), 62-77 (patched)
> > 
> >
> > Any reason we cannot use a single field containing the `bootId` of the 
> > last transition? A single field would cut down on the number of possible 
> > message permutations, and also allow simpler handling (branching a changed 
> > `boot_id`, triggering `state`-dependent handling). We could set such a 
> > `boot_id` whenever there is a state transition.

Consider the following scenario:
`CREATED` -> `NODE_READY` -> `VOL_READY` -> `PUBLISHED` -> reboot -> 
`VOL_READY` -> reboot
If we share the same `boot_id` for all transitions, we won't be able to tell 
that this volume has been published before.
If we dedicate `boot_id` to `PUBLISHED`, we won't be able to know that there 
has been a reboot after the last `VOL_READY` so we need to call 
`NodeStageVolume` again.


> On Feb. 5, 2019, 5:41 p.m., Benjamin Bannier wrote:
> > src/resource_provider/storage/provider.cpp
> > Line 817 (original), 827 (patched)
> > 
> >
> > Is us not using a sequence anymore related to what is being done here?
> > 
> > Here and below.

No. The sequence is still used. Note that the `recovered` future is wrapped by 
a `recoverVolume` lambda, and we put the lambda in the sequence so all steps 
will be executed as a whole without being interleved with other calls. Dropping.


- Chun-Hung


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


On Feb. 5, 2019, 7:40 a.m., Chun-Hung Hsiao wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/69892/
> ---
> 
> (Updated Feb. 5, 2019, 7:40 a.m.)
> 
> 
> Review request for mesos, Benjamin Bannier, James DeFelice, and Jie Yu.
> 
> 
> Bugs: MESOS-9544
> https://issues.apache.org/jira/browse/MESOS-9544
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> If a CSI volume has been node-published before a reboot, SLRP will now
> try to bring it back to node-published again. This is important to
> perform synchronous persistent volume cleanup for `DESTROY`.
> 
> To achieve this, in addition to keeping track of the boot ID when a CSI
> volume is node-staged in `VolumeState.vol_ready_boot_id` (formerly
> `VolumeState.boot_id`), SLRP now also keeps track of the boot ID when
> the volume is node-published. This helps SLRP to better determine if a
> volume has been published before reboot.
> 
> 
> Diffs
> -
> 
>   src/csi/state.proto 264a5657dd37605a6f3bdadd0e8d18ba9673191a 
>   src/resource_provider/storage/provider.cpp 
> d6e20a549ede189c757ae3ae922ab7cb86d2be2c 
>   src/tests/storage_local_resource_provider_tests.cpp 
> e8ed20f818ed7f1a3ce15758ea3c366520443377 
> 
> 
> Diff: https://reviews.apache.org/r/69892/diff/1/
> 
> 
> Testing
> ---
> 
> `make check`
> 
> Testing for publish failures will be done later in chain.
> 
> 
> Thanks,
> 
> Chun-Hung Hsiao
> 
>

Re: Review Request 69871: Added more documentation for operation feedback.

[Mesos Reviewbot Windows]

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



PASS: Mesos patch 69871 was successfully built and tested.

Reviews applied: `['69871']`

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

- Mesos Reviewbot Windows


On Feb. 5, 2019, 4:33 p.m., Greg Mann wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/69871/
> ---
> 
> (Updated Feb. 5, 2019, 4:33 p.m.)
> 
> 
> Review request for mesos, Benjamin Bannier, Gastón Kleiman, and James 
> DeFelice.
> 
> 
> Bugs: MESOS-9477
> https://issues.apache.org/jira/browse/MESOS-9477
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added more documentation for operation feedback.
> 
> 
> Diffs
> -
> 
>   docs/high-availability-framework-guide.md 
> 1bf58e166c76af9ef9d7182d9d4e413bc02e253c 
>   docs/reconciliation.md bfb117c11969f97127ef2c55321191232814a1c2 
>   docs/scheduler-http-api.md 84f6187943b60d4472a4b7e0c667678ab055e482 
> 
> 
> Diff: https://reviews.apache.org/r/69871/diff/3/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Greg Mann
> 
>

Re: Review Request 69680: Have master acknowledge operation updates of completed frameworks.

[Mesos Reviewbot Windows]

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



PASS: Mesos patch 69680 was successfully built and tested.

Reviews applied: `['69854', '69680']`

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

- Mesos Reviewbot Windows


On Feb. 5, 2019, 5:02 p.m., Benjamin Bannier wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/69680/
> ---
> 
> (Updated Feb. 5, 2019, 5:02 p.m.)
> 
> 
> Review request for mesos, Gastón Kleiman and Greg Mann.
> 
> 
> Bugs: MESOS-9434
> https://issues.apache.org/jira/browse/MESOS-9434
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> After a framework was removed and has unacknowledged operations status
> updates, it was impossible to remove terminal operations as nobody could
> acknowledge them.
> 
> In this patch we make the master acknowledge operation status updates
> for frameworks it knows are removed so that e.g., terminal operations
> can be removed. Since masters do not persist completed frameworks this
> is not reliable (e.g., an agent was partitioned for a long time and
> still tracks a completed framework's `FrameworkInfo`, and comes back
> only after the master knowing about the framework's completion has
> failed over). We merely extend the existing master behavior (e.g., send
> `ShutdownFrameworkMessage` to all currently registered agents) to
> operations.
> 
> 
> Diffs
> -
> 
>   src/master/master.cpp f74b7c280569e1c24e0940463bb28bd795d429d5 
>   src/tests/master_tests.cpp acc6096239e4992bdca084d0d644ab4a2385 
> 
> 
> Diff: https://reviews.apache.org/r/69680/diff/3/
> 
> 
> Testing
> ---
> 
> * `make check`
> * tested on a number of configurations in internal CI
> * ran added test in repetition, both with and without additional stress
> 
> 
> Thanks,
> 
> Benjamin Bannier
> 
>

Re: Review Request 69889: Stored static framework offer filters in allocator framework class.

[Benjamin Mahler]

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



Whoops, missed that this needs to be updated in updateFramework.


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


Actually, just realized when reading the subsequent patch (and thinking 
through what it would mean for the allocatable function to be a member function 
of the `Framework` struct) that this field needs to be updated when 
`HierarchicalAllocatorProcess::updateFramework(...)` is called.

Can we also have a test ensuring updateFramework correctly updates it?


- Benjamin Mahler


On Feb. 4, 2019, 10:05 p.m., Benjamin Bannier wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/69889/
> ---
> 
> (Updated Feb. 4, 2019, 10:05 p.m.)
> 
> 
> Review request for mesos, Benjamin Mahler and Meng Zhu.
> 
> 
> Bugs: MESOS-9523
> https://issues.apache.org/jira/browse/MESOS-9523
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This commit unpacks static offer filters configured in `FrameworkInfo`
> into a dedicated field in the allocator-specific `Framework` class which
> uses `ResourceQuantities`.
> 
> We will update the allocator to make use of this information in a
> subsequent patch.
> 
> 
> Diffs
> -
> 
>   src/master/allocator/mesos/hierarchical.hpp 
> 1420c2638786d85f7b04379e5d79e59990c3e6cf 
>   src/master/allocator/mesos/hierarchical.cpp 
> bb9a9c95979f36c0564af5b3babb1c43077a363b 
> 
> 
> Diff: https://reviews.apache.org/r/69889/diff/1/
> 
> 
> Testing
> ---
> 
> `make check`
> 
> 
> Thanks,
> 
> Benjamin Bannier
> 
>

Re: Review Request 69890: Added test for per-framework, per-role minimal allocatable resources.

[Benjamin Mahler]

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



Looks good! Just some minor comments below


src/tests/hierarchical_allocator_tests.cpp
Lines 2287 (patched)


any reason not to stick to using "minimum" for these? ditto below

is "finite" needed here?



src/tests/hierarchical_allocator_tests.cpp
Lines 2287-2288 (patched)


suggestion:

"resources, it correctly overrides the global minimum. We check this by 
ensuring it gets an offer when the global minimum is not satisfied but the 
framework's role specific mininium is"



src/tests/hierarchical_allocator_tests.cpp
Lines 2296 (patched)


Can we pass a master::Flags object into initialize that has an explicit min 
allocatable resources flag set? (It seems it already is set up to take this 
flag)

Right now this test seems to be making a pretty strong assumption that 
MIN_CPUS is what's being used?

Ditto for the other tests below



src/tests/hierarchical_allocator_tests.cpp
Lines 2345-2346 (patched)


seems like this should describe that "a single empty quantity" is used?



src/tests/hierarchical_allocator_tests.cpp
Lines 2352-2353 (patched)


Do we actually need these two lines? It seems like having a "set" but empty 
`min_allocatable_resources` should be enough? (i.e. without having to add a 
single array item within its `quantities`)



src/tests/hierarchical_allocator_tests.cpp
Lines 2357 (patched)


Ditto earlier comment, it's probably not helpful for  the reader to see a 
mixture of "minimal" and "minimum" used?



src/tests/hierarchical_allocator_tests.cpp
Lines 2378-2381 (patched)


This seems like the "EmptyMinAllocatable" case and the one above seems like 
the "EmptyQuantityMinAllocatable" case?


- Benjamin Mahler


On Feb. 4, 2019, 10:05 p.m., Benjamin Bannier wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/69890/
> ---
> 
> (Updated Feb. 4, 2019, 10:05 p.m.)
> 
> 
> Review request for mesos, Benjamin Mahler and Meng Zhu.
> 
> 
> Bugs: MESOS-9523
> https://issues.apache.org/jira/browse/MESOS-9523
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This patch adds tests for the behavior of per-framework, per-role
> minimal allocatable resources. We validate the behavior of framework
> minimal allocatable resources below the globally configured limits and
> the behavior of empty filters or minimal resource quantities set up for
> a framework.
> 
> 
> Diffs
> -
> 
>   src/tests/hierarchical_allocator_tests.cpp 
> cc88afbad1b4e6bf707cb13b50c964aa01f9a3ee 
> 
> 
> Diff: https://reviews.apache.org/r/69890/diff/1/
> 
> 
> Testing
> ---
> 
> `make check`
> 
> 
> Thanks,
> 
> Benjamin Bannier
> 
>

Re: Review Request 69889: Stored static framework offer filters in allocator framework class.

[Benjamin Mahler]

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


Fix it, then Ship it!





src/master/allocator/mesos/hierarchical.hpp
Line 480 (original), 482-483 (patched)


Actually on second look, it seems odd to store both the entire options and 
the sub-option?

Why not just access via the `options` struct? As it stands it seems prone 
to be accessed both ways by accident (which becomes a problem if we ever do 
mutation).


- Benjamin Mahler


On Feb. 4, 2019, 10:05 p.m., Benjamin Bannier wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/69889/
> ---
> 
> (Updated Feb. 4, 2019, 10:05 p.m.)
> 
> 
> Review request for mesos, Benjamin Mahler and Meng Zhu.
> 
> 
> Bugs: MESOS-9523
> https://issues.apache.org/jira/browse/MESOS-9523
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This commit unpacks static offer filters configured in `FrameworkInfo`
> into a dedicated field in the allocator-specific `Framework` class which
> uses `ResourceQuantities`.
> 
> We will update the allocator to make use of this information in a
> subsequent patch.
> 
> 
> Diffs
> -
> 
>   src/master/allocator/mesos/hierarchical.hpp 
> 1420c2638786d85f7b04379e5d79e59990c3e6cf 
>   src/master/allocator/mesos/hierarchical.cpp 
> bb9a9c95979f36c0564af5b3babb1c43077a363b 
> 
> 
> Diff: https://reviews.apache.org/r/69889/diff/1/
> 
> 
> Testing
> ---
> 
> `make check`
> 
> 
> Thanks,
> 
> Benjamin Bannier
> 
>

Re: Review Request 69889: Stored static framework offer filters in allocator framework class.

[Benjamin Mahler]

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


Ship it!




Ship It!

- Benjamin Mahler


On Feb. 4, 2019, 10:05 p.m., Benjamin Bannier wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/69889/
> ---
> 
> (Updated Feb. 4, 2019, 10:05 p.m.)
> 
> 
> Review request for mesos, Benjamin Mahler and Meng Zhu.
> 
> 
> Bugs: MESOS-9523
> https://issues.apache.org/jira/browse/MESOS-9523
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This commit unpacks static offer filters configured in `FrameworkInfo`
> into a dedicated field in the allocator-specific `Framework` class which
> uses `ResourceQuantities`.
> 
> We will update the allocator to make use of this information in a
> subsequent patch.
> 
> 
> Diffs
> -
> 
>   src/master/allocator/mesos/hierarchical.hpp 
> 1420c2638786d85f7b04379e5d79e59990c3e6cf 
>   src/master/allocator/mesos/hierarchical.cpp 
> bb9a9c95979f36c0564af5b3babb1c43077a363b 
> 
> 
> Diff: https://reviews.apache.org/r/69889/diff/1/
> 
> 
> Testing
> ---
> 
> `make check`
> 
> 
> Thanks,
> 
> Benjamin Bannier
> 
>

Re: Review Request 69876: Removed operations from master state when an agent is downgraded.

[Gastón Kleiman]

> On Feb. 4, 2019, 4:28 p.m., Gastón Kleiman wrote:
> > src/tests/master_tests.cpp
> > Lines 9419 (patched)
> > 
> >
> > We should consider making the agent not recover the operation status 
> > update manager if it isn't started with the `AGENT_OPERATION_FEEDBACK` 
> > capability.

If we don't, we should not drop this message and make sure that the framework 
can acknowledge the update, so that the agent stops resending it.


- Gastón


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


On Jan. 31, 2019, 3:08 p.m., Greg Mann wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/69876/
> ---
> 
> (Updated Jan. 31, 2019, 3:08 p.m.)
> 
> 
> Review request for mesos, Benjamin Bannier, Chun-Hung Hsiao, and Gastón 
> Kleiman.
> 
> 
> Bugs: MESOS-9535
> https://issues.apache.org/jira/browse/MESOS-9535
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> When an agent is downgraded from one with the AGENT_OPERATION_FEEDBACK
> capability to one without this capability, the master needs to remove
> terminal-but-unACKed operations from its state which operate on agent
> default resources, since the downgraded agent will not resend status
> updates for these operations.
> 
> 
> Diffs
> -
> 
>   src/master/master.cpp f74b7c280569e1c24e0940463bb28bd795d429d5 
>   src/tests/master_tests.cpp acc6096239e4992bdca084d0d644ab4a2385 
> 
> 
> Diff: https://reviews.apache.org/r/69876/diff/1/
> 
> 
> Testing
> ---
> 
> `make check`
> `bin/mesos-tests.sh --gtest_filter="*CleanupOperationsAfterAgentDowngrade*" 
> --gtest_repeat=-1 --gtest_break_on_failure`
> 
> 
> Thanks,
> 
> Greg Mann
> 
>

Re: Review Request 69891: Sent operation updates to schedulers when agents are removed.

[Mesos Reviewbot Windows]

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



PASS: Mesos patch 69891 was successfully built and tested.

Reviews applied: `['69876', '69880', '69891']`

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

- Mesos Reviewbot Windows


On Feb. 5, 2019, 4:24 p.m., Greg Mann wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/69891/
> ---
> 
> (Updated Feb. 5, 2019, 4:24 p.m.)
> 
> 
> Review request for mesos, Gastón Kleiman and Joseph Wu.
> 
> 
> Bugs: MESOS-9541
> https://issues.apache.org/jira/browse/MESOS-9541
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This patch makes the master send operation updates when
> an agent is removed and a framework has requested
> feedback for a pending operation on that agent.
> 
> 
> Diffs
> -
> 
>   src/master/master.cpp f74b7c280569e1c24e0940463bb28bd795d429d5 
>   src/tests/master_tests.cpp acc6096239e4992bdca084d0d644ab4a2385 
> 
> 
> Diff: https://reviews.apache.org/r/69891/diff/1/
> 
> 
> Testing
> ---
> 
> `make check`
> `bin/mesos-tests.sh --gtest_filter="*OperationUpdatesAfterAgentShutdown*" 
> --gtest_repeat=-1 --gtest_break_on_failure`
> 
> 
> Thanks,
> 
> Greg Mann
> 
>

Re: Review Request 69893: Clean up persistent volumes on SLRP disks.

[Benjamin Bannier]

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




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


This is some pretty strong coupling to `master::validate` checking 
`resource::validatePersistentVolume`. That's probably okay, but would could 
also just re-validate here instead.



src/resource_provider/storage/provider.cpp
Lines 3125-3126 (original), 3138-3139 (patched)


Do we actually validate this somewhere?



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


Is this validated somewhere?



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


Is this validated?



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


Does this break seemless upgrades? Probably okay, but still something worth 
documenting in the upgrades guide.


- Benjamin Bannier


On Feb. 5, 2019, 8:42 a.m., Chun-Hung Hsiao wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/69893/
> ---
> 
> (Updated Feb. 5, 2019, 8:42 a.m.)
> 
> 
> Review request for mesos, Benjamin Bannier, James DeFelice, and Jie Yu.
> 
> 
> Bugs: MESOS-9544
> https://issues.apache.org/jira/browse/MESOS-9544
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This patch limits SLRP to only support persistent volumes on MOUNT
> disks, and makes it clean up data in persistent volumes when processing
> `DESTROY` operations.
> 
> 
> Diffs
> -
> 
>   src/resource_provider/storage/provider.cpp 
> d6e20a549ede189c757ae3ae922ab7cb86d2be2c 
>   src/resource_provider/storage/provider_process.hpp PRE-CREATION 
> 
> 
> Diff: https://reviews.apache.org/r/69893/diff/1/
> 
> 
> Testing
> ---
> 
> `make check`
> 
> More testing done later in chain.
> 
> 
> Thanks,
> 
> Chun-Hung Hsiao
> 
>

Re: Review Request 69892: Made SLRP recover node-published volumes after reboot.

[Benjamin Bannier]

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




src/csi/state.proto
Lines 62-67 (original), 62-77 (patched)


Any reason we cannot use a single field containing the `bootId` of the last 
transition? A single field would cut down on the number of possible message 
permutations, and also allow simpler handling (branching a changed `boot_id`, 
triggering `state`-dependent handling). We could set such a `boot_id` whenever 
there is a state transition.



src/resource_provider/storage/provider.cpp
Line 817 (original), 827 (patched)


Is us not using a sequence anymore related to what is being done here?

Here and below.


- Benjamin Bannier


On Feb. 5, 2019, 8:40 a.m., Chun-Hung Hsiao wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/69892/
> ---
> 
> (Updated Feb. 5, 2019, 8:40 a.m.)
> 
> 
> Review request for mesos, Benjamin Bannier, James DeFelice, and Jie Yu.
> 
> 
> Bugs: MESOS-9544
> https://issues.apache.org/jira/browse/MESOS-9544
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> If a CSI volume has been node-published before a reboot, SLRP will now
> try to bring it back to node-published again. This is important to
> perform synchronous persistent volume cleanup for `DESTROY`.
> 
> To achieve this, in addition to keeping track of the boot ID when a CSI
> volume is node-staged in `VolumeState.vol_ready_boot_id` (formerly
> `VolumeState.boot_id`), SLRP now also keeps track of the boot ID when
> the volume is node-published. This helps SLRP to better determine if a
> volume has been published before reboot.
> 
> 
> Diffs
> -
> 
>   src/csi/state.proto 264a5657dd37605a6f3bdadd0e8d18ba9673191a 
>   src/resource_provider/storage/provider.cpp 
> d6e20a549ede189c757ae3ae922ab7cb86d2be2c 
>   src/tests/storage_local_resource_provider_tests.cpp 
> e8ed20f818ed7f1a3ce15758ea3c366520443377 
> 
> 
> Diff: https://reviews.apache.org/r/69892/diff/1/
> 
> 
> Testing
> ---
> 
> `make check`
> 
> Testing for publish failures will be done later in chain.
> 
> 
> Thanks,
> 
> Chun-Hung Hsiao
> 
>

Re: Review Request 69854: Set status update UUID in MockResourceProvider.

[Greg Mann]

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


Ship it!




Ship It!

- Greg Mann


On Jan. 29, 2019, 1:20 p.m., Benjamin Bannier wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/69854/
> ---
> 
> (Updated Jan. 29, 2019, 1:20 p.m.)
> 
> 
> Review request for mesos, Greg Mann and Jan Schlicht.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Set status update UUID in MockResourceProvider.
> 
> 
> Diffs
> -
> 
>   src/tests/mesos.hpp c2a5e5531de7498241e537ef1699e1a5e669b550 
> 
> 
> Diff: https://reviews.apache.org/r/69854/diff/1/
> 
> 
> Testing
> ---
> 
> `make check`
> 
> 
> Thanks,
> 
> Benjamin Bannier
> 
>

Re: Review Request 69891: Sent operation updates to schedulers when agents are removed.

[Greg Mann]

> On Feb. 5, 2019, 4:57 p.m., Vinod Kone wrote:
> > src/master/master.cpp
> > Lines 10950 (patched)
> > 
> >
> > I don't know if sending UNREACHABLE for all the 3 cases when 
> > `_removeSlave` is called is the right way. I think we need to have a 
> > discussion around the agent state / task state / operation state for each 
> > of the cases.
> > 
> > What happens if a framework reconciles the operation after this code 
> > gets executed. Will it always get OPERATION_UNREACHABLE? If not, then that 
> > would be cnofusing.
> > 
> > Also, the operation status is not changed in-memory here. Is that 
> > intentional?

Continuing this discussion on Slack: 
https://mesos.slack.com/archives/C8NN4M0CT/p1549066656027000

Yep, I intended not to update the operation status in this patch. The JIRA 
issue is purely for sending updates to frameworks, and I intend to address all 
of the operation state updates as part of 
https://issues.apache.org/jira/browse/MESOS-9546 since that involves more 
significant code changes.


- Greg


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


On Feb. 5, 2019, 4:24 p.m., Greg Mann wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/69891/
> ---
> 
> (Updated Feb. 5, 2019, 4:24 p.m.)
> 
> 
> Review request for mesos, Gastón Kleiman and Joseph Wu.
> 
> 
> Bugs: MESOS-9541
> https://issues.apache.org/jira/browse/MESOS-9541
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This patch makes the master send operation updates when
> an agent is removed and a framework has requested
> feedback for a pending operation on that agent.
> 
> 
> Diffs
> -
> 
>   src/master/master.cpp f74b7c280569e1c24e0940463bb28bd795d429d5 
>   src/tests/master_tests.cpp acc6096239e4992bdca084d0d644ab4a2385 
> 
> 
> Diff: https://reviews.apache.org/r/69891/diff/1/
> 
> 
> Testing
> ---
> 
> `make check`
> `bin/mesos-tests.sh --gtest_filter="*OperationUpdatesAfterAgentShutdown*" 
> --gtest_repeat=-1 --gtest_break_on_failure`
> 
> 
> Thanks,
> 
> Greg Mann
> 
>

Re: Review Request 69894: Disallowed `DESTROY_DISK` on persistent volumes.

[Benjamin Bannier]

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


Fix it, then Ship it!





src/master/validation.cpp
Lines 2577-2579 (patched)


Maybe make this more in line with what we already have for `UNRESERVE` of 
persistent volumes?
```
A disk resource containing a persistent volume {source} cannot be destroyed 
directly. Please destroy the persistent volume first then destroy the disk 
resource.
```
Also, spaces either at start or end of line, not mixed. At the start seems 
popular.


- Benjamin Bannier


On Feb. 5, 2019, 8:43 a.m., Chun-Hung Hsiao wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/69894/
> ---
> 
> (Updated Feb. 5, 2019, 8:43 a.m.)
> 
> 
> Review request for mesos, Benjamin Bannier, James DeFelice, and Jie Yu.
> 
> 
> Bugs: MESOS-9544
> https://issues.apache.org/jira/browse/MESOS-9544
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> `DESTROY_DISK` would bypass persistent volume cleanup and directly ask
> the CSI plugin to delete the backed volume. Since the CSI spec does not
> require the plugin to do data cleanup, to avoid data leakage, we require
> that if there is persistent volume on the CSI volume, it should be
> destroyed first.
> 
> 
> Diffs
> -
> 
>   src/master/validation.cpp a71edeb7827b534e27ad3e3398abe555bf36e741 
>   src/tests/master_validation_tests.cpp 
> c00e8bb315c28bdf438da2089dd81f5e348982e5 
> 
> 
> Diff: https://reviews.apache.org/r/69894/diff/1/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Chun-Hung Hsiao
> 
>

Re: Review Request 69680: Have master acknowledge operation updates of completed frameworks.

[Benjamin Bannier]

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

(Updated Feb. 5, 2019, 6:02 p.m.)


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


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


Repository: mesos


Description
---

After a framework was removed and has unacknowledged operations status
updates, it was impossible to remove terminal operations as nobody could
acknowledge them.

In this patch we make the master acknowledge operation status updates
for frameworks it knows are removed so that e.g., terminal operations
can be removed. Since masters do not persist completed frameworks this
is not reliable (e.g., an agent was partitioned for a long time and
still tracks a completed framework's `FrameworkInfo`, and comes back
only after the master knowing about the framework's completion has
failed over). We merely extend the existing master behavior (e.g., send
`ShutdownFrameworkMessage` to all currently registered agents) to
operations.


Diffs
-

  src/master/master.cpp f74b7c280569e1c24e0940463bb28bd795d429d5 
  src/tests/master_tests.cpp acc6096239e4992bdca084d0d644ab4a2385 


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


Testing
---

* `make check`
* tested on a number of configurations in internal CI
* ran added test in repetition, both with and without additional stress


Thanks,

Benjamin Bannier

Re: Review Request 69891: Sent operation updates to schedulers when agents are removed.

[Vinod Kone]

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




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


I don't know if sending UNREACHABLE for all the 3 cases when `_removeSlave` 
is called is the right way. I think we need to have a discussion around the 
agent state / task state / operation state for each of the cases.

What happens if a framework reconciles the operation after this code gets 
executed. Will it always get OPERATION_UNREACHABLE? If not, then that would be 
cnofusing.

Also, the operation status is not changed in-memory here. Is that 
intentional?


- Vinod Kone


On Feb. 5, 2019, 4:24 p.m., Greg Mann wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/69891/
> ---
> 
> (Updated Feb. 5, 2019, 4:24 p.m.)
> 
> 
> Review request for mesos, Gastón Kleiman and Joseph Wu.
> 
> 
> Bugs: MESOS-9541
> https://issues.apache.org/jira/browse/MESOS-9541
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This patch makes the master send operation updates when
> an agent is removed and a framework has requested
> feedback for a pending operation on that agent.
> 
> 
> Diffs
> -
> 
>   src/master/master.cpp f74b7c280569e1c24e0940463bb28bd795d429d5 
>   src/tests/master_tests.cpp acc6096239e4992bdca084d0d644ab4a2385 
> 
> 
> Diff: https://reviews.apache.org/r/69891/diff/1/
> 
> 
> Testing
> ---
> 
> `make check`
> `bin/mesos-tests.sh --gtest_filter="*OperationUpdatesAfterAgentShutdown*" 
> --gtest_repeat=-1 --gtest_break_on_failure`
> 
> 
> Thanks,
> 
> Greg Mann
> 
>

Re: Review Request 69821: Enforced minimal allocatable resources in the hierarchical allocator.

[Benjamin Bannier]

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

(Updated Feb. 5, 2019, 5:38 p.m.)


Review request for mesos, Benjamin Mahler and Meng Zhu.


Changes
---

Adress a couple issues raised by Ben


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


Repository: mesos


Description
---

This patch modifies the hierarchical allocator to take
framework-specified minimal allocatable resources into account.

While previously the allocator was inspecting the minimal allocatable
resources specified in its global options, it can now also inspects
framework-specific resource requirements. With that frameworks can e.g.,
configure resource requirements above the default minimal allocatable
resource, or opt into receiving resources considered too small to be
allocatable by the allocator in its default behavior.

For that we change the hierarchical allocator's `allocatable` function
to be framework and role-specific. As that does in some places not allow
us to abort iterating over candidate resource consumers like before an
additional check on whether any resources are left in an allocation
cycle is added as a break condition.


Diffs (updated)
-

  src/master/allocator/mesos/hierarchical.hpp 
1420c2638786d85f7b04379e5d79e59990c3e6cf 
  src/master/allocator/mesos/hierarchical.cpp 
bb9a9c95979f36c0564af5b3babb1c43077a363b 


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

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


Testing
---

`make check`


File Attachments


Ratio new/old timings
  
https://reviews.apache.org/media/uploaded/files/2019/01/31/d76189de-8882-4aff-956b-090dab729358__new_over_old.png


Thanks,

Benjamin Bannier

Re: Review Request 69680: Have master acknowledge operation updates of completed frameworks.

[Benjamin Bannier]

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

(Updated Feb. 5, 2019, 5:40 p.m.)


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


Changes
---

Fixerize comment as suggested by Greg


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


Repository: mesos


Description
---

After a framework was removed and has unacknowledged operations status
updates, it was impossible to remove terminal operations as nobody could
acknowledge them.

In this patch we make the master acknowledge operation status updates
for frameworks it knows are removed so that e.g., terminal operations
can be removed. Since masters do not persist completed frameworks this
is not reliable (e.g., an agent was partitioned for a long time and
still tracks a completed framework's `FrameworkInfo`, and comes back
only after the master knowing about the framework's completion has
failed over). We merely extend the existing master behavior (e.g., send
`ShutdownFrameworkMessage` to all currently registered agents) to
operations.


Diffs (updated)
-

  src/master/master.cpp f74b7c280569e1c24e0940463bb28bd795d429d5 
  src/tests/master_tests.cpp acc6096239e4992bdca084d0d644ab4a2385 


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

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


Testing
---

* `make check`
* tested on a number of configurations in internal CI
* ran added test in repetition, both with and without additional stress


Thanks,

Benjamin Bannier

Re: Review Request 69821: Enforced minimal allocatable resources in the hierarchical allocator.

[Benjamin Bannier]

> On Feb. 4, 2019, 9:58 p.m., Benjamin Mahler wrote:
> > src/master/allocator/mesos/hierarchical.cpp
> > Lines 2041-2042 (original), 2075-2076 (patched)
> > 
> >
> > Hm.. isn't the framework capability stripping messing with our break 
> > condition?

Hmm, it seems like it, yes. This should be an issue already now where we 
`break` in even more scenarios. This should be fixed outside of this change. I 
created https://issues.apache.org/jira/browse/MESOS-9554.

Dropping here.


> On Feb. 4, 2019, 9:58 p.m., Benjamin Mahler wrote:
> > src/master/allocator/mesos/hierarchical.cpp
> > Lines 2058-2060 (original), 2092-2094 (patched)
> > 
> >
> > We lost the comment here about why it's safe to break? It still seems 
> > relevant

Hmm, not sure what you are after. My patch `continue`s instead of `break`s. The 
way the code is structured now we cannot `break` here, but instead must iterate 
over all frameworks. We could `break` if we'd e.g., made `allocatable` 
independent of framework settings like before (e.g., by computing a minimal 
allocatable resources given all framework information), but we'd likely reject 
many allocation decision last minute that way in the same spot where we 
currently check filters. The code as proposed here looks simpler to me.

Can we drop this?


> On Feb. 4, 2019, 9:58 p.m., Benjamin Mahler wrote:
> > src/master/allocator/mesos/hierarchical.cpp
> > Lines 2064-2070 (original), 2096-2102 (patched)
> > 
> >
> > I'm left confused by the two checks now that they both continue, and I 
> > think the comment is now inaccurate and confusing? It is written based on 
> > break vs continue

I went over all the comments around resource emptiness and `allocatable` checks.


> On Feb. 4, 2019, 9:58 p.m., Benjamin Mahler wrote:
> > src/master/allocator/mesos/hierarchical.cpp
> > Lines 2090-2091 (original), 2122-2125 (patched)
> > 
> >
> > It seems more readable if this is a member function of `Framework`

In that case we'd have to pass in the allocator's `minAllocatableResources` 
(either here, or if they could change here). I'd suggest to keep this as is.

Dropping for now, feel free to reopen if you think this requires more 
discussion.


- Benjamin


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


On Feb. 5, 2019, 5:38 p.m., Benjamin Bannier wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/69821/
> ---
> 
> (Updated Feb. 5, 2019, 5:38 p.m.)
> 
> 
> Review request for mesos, Benjamin Mahler and Meng Zhu.
> 
> 
> Bugs: MESOS-9523
> https://issues.apache.org/jira/browse/MESOS-9523
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This patch modifies the hierarchical allocator to take
> framework-specified minimal allocatable resources into account.
> 
> While previously the allocator was inspecting the minimal allocatable
> resources specified in its global options, it can now also inspects
> framework-specific resource requirements. With that frameworks can e.g.,
> configure resource requirements above the default minimal allocatable
> resource, or opt into receiving resources considered too small to be
> allocatable by the allocator in its default behavior.
> 
> For that we change the hierarchical allocator's `allocatable` function
> to be framework and role-specific. As that does in some places not allow
> us to abort iterating over candidate resource consumers like before an
> additional check on whether any resources are left in an allocation
> cycle is added as a break condition.
> 
> 
> Diffs
> -
> 
>   src/master/allocator/mesos/hierarchical.hpp 
> 1420c2638786d85f7b04379e5d79e59990c3e6cf 
>   src/master/allocator/mesos/hierarchical.cpp 
> bb9a9c95979f36c0564af5b3babb1c43077a363b 
> 
> 
> Diff: https://reviews.apache.org/r/69821/diff/8/
> 
> 
> Testing
> ---
> 
> `make check`
> 
> 
> File Attachments
> 
> 
> Ratio new/old timings
>   
> https://reviews.apache.org/media/uploaded/files/2019/01/31/d76189de-8882-4aff-956b-090dab729358__new_over_old.png
> 
> 
> Thanks,
> 
> Benjamin Bannier
> 
>

Re: Review Request 69871: Added more documentation for operation feedback.

[Greg Mann]

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

(Updated Feb. 5, 2019, 4:33 p.m.)


Review request for mesos, Benjamin Bannier, Gastón Kleiman, and James DeFelice.


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


Repository: mesos


Description
---

Added more documentation for operation feedback.


Diffs (updated)
-

  docs/high-availability-framework-guide.md 
1bf58e166c76af9ef9d7182d9d4e413bc02e253c 
  docs/reconciliation.md bfb117c11969f97127ef2c55321191232814a1c2 
  docs/scheduler-http-api.md 84f6187943b60d4472a4b7e0c667678ab055e482 


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

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


Testing
---


Thanks,

Greg Mann

Review Request 69891: Sent operation updates to schedulers when agents are removed.

[Greg Mann]

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

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


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


Repository: mesos


Description
---

This patch makes the master send operation updates when
an agent is removed and a framework has requested
feedback for a pending operation on that agent.


Diffs
-

  src/master/master.cpp f74b7c280569e1c24e0940463bb28bd795d429d5 
  src/tests/master_tests.cpp acc6096239e4992bdca084d0d644ab4a2385 


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


Testing
---

`make check`
`bin/mesos-tests.sh --gtest_filter="*OperationUpdatesAfterAgentShutdown*" 
--gtest_repeat=-1 --gtest_break_on_failure`


Thanks,

Greg Mann

Re: Review Request 69894: Disallowed `DESTROY_DISK` on persistent volumes.

[James DeFelice]

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


Ship it!




Ship It!

- James DeFelice


On Feb. 5, 2019, 7:43 a.m., Chun-Hung Hsiao wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/69894/
> ---
> 
> (Updated Feb. 5, 2019, 7:43 a.m.)
> 
> 
> Review request for mesos, Benjamin Bannier, James DeFelice, and Jie Yu.
> 
> 
> Bugs: MESOS-9544
> https://issues.apache.org/jira/browse/MESOS-9544
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> `DESTROY_DISK` would bypass persistent volume cleanup and directly ask
> the CSI plugin to delete the backed volume. Since the CSI spec does not
> require the plugin to do data cleanup, to avoid data leakage, we require
> that if there is persistent volume on the CSI volume, it should be
> destroyed first.
> 
> 
> Diffs
> -
> 
>   src/master/validation.cpp a71edeb7827b534e27ad3e3398abe555bf36e741 
>   src/tests/master_validation_tests.cpp 
> c00e8bb315c28bdf438da2089dd81f5e348982e5 
> 
> 
> Diff: https://reviews.apache.org/r/69894/diff/1/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Chun-Hung Hsiao
> 
>

Re: Review Request 69858: Persisted intentionally dropped operations in SLRP.

[Benjamin Bannier]

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




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


Why is this always true? Probably a good idea to document this here if we 
make this assertion. I'd suggest to not make this assertion and instead 
simplify the control flow, see below.



src/resource_provider/storage/provider.cpp
Lines 2888-2892 (original), 2898-2910 (patched)


What do you think about constructing a single update outside of any 
condition (like was done previously), and then only have branching depending on 
whether we want to use the status update manager or send unreliably?

I think this would read much better if we'd:
* gather all required input to create the status update in variables,
* construct the status update, and
* a single `if/else` for the sending.
* update metrics in a single place



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


We could introduce a variable for this.



src/resource_provider/storage/provider_process.hpp
Line 298 (original), 298-300 (patched)


It isn't clear from the second sentence whether you document behavior or 
give a prescription on how to use this function.


- Benjamin Bannier


On Jan. 30, 2019, 11:35 p.m., Chun-Hung Hsiao wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/69858/
> ---
> 
> (Updated Jan. 30, 2019, 11:35 p.m.)
> 
> 
> Review request for mesos, Benjamin Bannier, Gastón Kleiman, and Greg Mann.
> 
> 
> Bugs: MESOS-9537
> https://issues.apache.org/jira/browse/MESOS-9537
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> If an operation is dropped intentionally (e.g., because of a resource
> version mismatch), the operation should be persisted so no conflicting
> status update would be generated for operation reconciliation.
> 
> 
> Diffs
> -
> 
>   src/resource_provider/storage/provider.cpp 
> d6e20a549ede189c757ae3ae922ab7cb86d2be2c 
>   src/resource_provider/storage/provider_process.hpp PRE-CREATION 
> 
> 
> Diff: https://reviews.apache.org/r/69858/diff/3/
> 
> 
> Testing
> ---
> 
> `make check`
> 
> Additional test MESOS-9537 is added later in chain.
> 
> 
> Thanks,
> 
> Chun-Hung Hsiao
> 
>

Re: Review Request 69866: Updated SLRP test `ProfileDisappeared` to request operation feedbacks.

[Benjamin Bannier]

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


Fix it, then Ship it!





src/tests/storage_local_resource_provider_tests.cpp
Line 1166 (original), 1178 (patched)


`s/feedbacks/feedback/g`.

Here and everywhere else.



src/tests/storage_local_resource_provider_tests.cpp
Line 1288 (original), 1318 (patched)


`s/Creates/Create/`



src/tests/storage_local_resource_provider_tests.cpp
Lines 1342-1343 (original), 1391-1392 (patched)


This seems weird as `UpdateSlaveMessages` should only be triggered for 
speculation failures and `DESTROY_DISK` should never be speculated.

Don't we see the update due to the profile changes (which triggered a 
storage pool reconciliation)?


- Benjamin Bannier


On Feb. 5, 2019, 8:38 a.m., Chun-Hung Hsiao wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/69866/
> ---
> 
> (Updated Feb. 5, 2019, 8:38 a.m.)
> 
> 
> Review request for mesos, Benjamin Bannier, Gastón Kleiman, and Greg Mann.
> 
> 
> Bugs: MESOS-9537
> https://issues.apache.org/jira/browse/MESOS-9537
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This patch updates `StorageLocalResourceProviderTest.ProfileDisappeared`
> to use the v1 scheduler API to request operation feedbacks, so
> MESOS-9537 would be triggered when an outstanding `UPDATE_STATE` call
> from the resource provider races with an offer operation.
> 
> 
> Diffs
> -
> 
>   src/tests/mesos.hpp c2a5e5531de7498241e537ef1699e1a5e669b550 
>   src/tests/storage_local_resource_provider_tests.cpp 
> e8ed20f818ed7f1a3ce15758ea3c366520443377 
> 
> 
> Diff: https://reviews.apache.org/r/69866/diff/3/
> 
> 
> Testing
> ---
> 
> `make check`
> 
> Ran the test ~1000 times.
> 
> This test will fail without r/69858:
> ```
> E0130 22:48:02.790385 24040 provider.cpp:2903] Failed to update status of 
> operation (uuid: 2bf6813c-bde9-4c0e-8831-a779f5dc50ac): Mismatched framework 
> ID for operation status update OPERATION_DROPPED (Status UUID: 
> 8a625c40-96e8-46e2-8050-69843032969a) for operation UUID 
> 2bf6813c-bde9-4c0e-8831-a779f5dc50ac on agent 
> bfa6e505-54b7-4bb8-a8aa-99fb12291fb1-S0 (expected 
> bfa6e505-54b7-4bb8-a8aa-99fb12291fb1- got no framework ID)
> I0130 22:48:02.791590 24042 manager.cpp:163] Terminating resource provider 
> 765391c2-9485-4614-a6e5-45e67ef7a92c
> ...
> ../../src/tests/storage_local_resource_provider_tests.cpp:1387: Failure
>   Expected: v1::OPERATION_FINISHED
>   Which is: OPERATION_FINISHED
> To be equal to: update->status().state()
>   Which is: OPERATION_ERROR
> ../../src/tests/storage_local_resource_provider_tests.cpp:1393: Failure
> Failed to wait 15secs for offers
> ...
> ../../src/tests/storage_local_resource_provider_tests.cpp:1361: Failure
> Actual function call count doesn't match EXPECT_CALL(*scheduler, offers(_, 
> v1::scheduler::OffersHaveAnyResource( std::bind(isStoragePool, lambda::_1, 
> "test2"...
>  Expected: to be called once
>Actual: never called - unsatisfied and active
> ```
> 
> 
> Thanks,
> 
> Chun-Hung Hsiao
> 
>

Re: Review Request 69898: Updated `CreateDestroyDisk*` SLRP tests to test pipelined operations.

[Mesos Reviewbot Windows]

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



PASS: Mesos patch 69898 was successfully built and tested.

Reviews applied: `['69858', '69866', '69892', '69893', '69894', '69895', 
'69896', '69897', '69898']`

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

- Mesos Reviewbot Windows


On Feb. 5, 2019, 8:06 a.m., Chun-Hung Hsiao wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/69898/
> ---
> 
> (Updated Feb. 5, 2019, 8:06 a.m.)
> 
> 
> Review request for mesos and Benjamin Bannier.
> 
> 
> Bugs: MESOS-9544
> https://issues.apache.org/jira/browse/MESOS-9544
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This patch extends the code coverage of the `CreateDestroyDisk` and
> `CreateDestroyDiskRecovery` tests by testing pipelined `RESERVE`,
> `CREATE`, `DESTROY` and `UNRESERVE` operations along with `CREATE_DISK`
> and `DESTROY_DISK`. It also renames `CreateDestroyDiskRecovery` to
> `CreateDestroyDiskWithRecovery` for consistency.
> 
> 
> Diffs
> -
> 
>   src/tests/storage_local_resource_provider_tests.cpp 
> e8ed20f818ed7f1a3ce15758ea3c366520443377 
> 
> 
> Diff: https://reviews.apache.org/r/69898/diff/1/
> 
> 
> Testing
> ---
> 
> `make check`
> 
> Run in repetition
> 
> 
> Thanks,
> 
> Chun-Hung Hsiao
> 
>

Re: Review Request 69892: Made SLRP recover node-published volumes after reboot.

[Mesos Reviewbot Windows]

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



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

Reviews applied: `['69858', '69866', '69892']`

Failed command: `Start-MesosCITesting`

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

Relevant logs:

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

```
I0205 08:43:13.151952 32844 master.cpp:11317] Removing task 
83482d1a-9498-4eb7-af15-05073d895a62 with resources cpus(allocated: *):4; 
mem(allocated: *):2048; disk(allocated: *):1024; ports(allocated: 
*):[31000-32000] of framework 9bfba4c6-997f-4c2e-bff6-5867256f8ba7- on 
agent 9bfba4c6-997f-4c2e-bff6-5867256f8ba7-S0 at slave(477)@192.10.1.6:59972 
(windows-02.chtsmhjxogyevckjfayqqcnjda.xx.internal.cloudapp.net)
I0205 08:43:13.153949 33920 master.cpp:1269] Agent 
9bfba4c6-997f-4c2e-bff6-5867256f8ba7-S0 at slave(477)@192.10.1.6:59972 
(windows-02.chtsmhjxogyevckjfayqqcnjda.xx.internal.cloudapp.net) disconnected
I0205 08:43:13.154947 33920 master.cpp:3272] Disconnecting agent 
9bfba4c6-997f-4c2e-bff6-5867256f8ba7-S0 at slave(477)@192.10.1.6:59972 
(windows-02.chtsmhjxogyevckjfayqqcnjda.xx.internal.cloudapp.net)
I0205 08:43:13.154947 33920 master.cpp:3291] Deactivating agent 
9bfba4c6-997f-4c2e-bff6-5867256f8ba7-S0 at slave(477)@192.10.1.6:59972 
(windows-02.chtsmhjxogyevckjfayqqcnjda.xx.internal.cloudapp.net)
I0205 08:43:13.154947 25360 hierarchical.cpp:358] Removed framework 
9bfba4c6-997f-4c2e-bff6-5867256f8ba7-
I0205 08:43:13.154947 25360 hierarchical.cpp:793] Agent 
9bfba4c6-997f-4c2e-bff6-5867256f8ba7-S0 deactivated
I0205 08:43:13.155947 34176 containerizer.cpp:2477] Destroying container 
7995f0b2-2fba-430c-98a1-abd316d3485a in RUNNING state
I0205 08:43:13.155947 34176 containerizer.cpp:3144] Transitioning the state of 
container 7995f0b2-2fba-430c-98a1-abd316d3485a from RUNNING to DESTROYING
I0205 08:43:13.156953 3417[   OK ] 
IsolationFlag/MemoryIsolatorTest.ROOT_MemUsage/0 (687 ms)
[--] 1 test from IsolationFlag/MemoryIsolatorTest (704 ms total)

[--] Global test environment tear-down
[==] 1095 tests from 104 test cases ran. (506130 ms total)
[  PASSED  ] 1094 tests.
[  FAILED  ] 1 test, listed below:
[  FAILED  ] DockerFetcherPluginTest.INTERNET_CURL_InvokeFetchByName

 1 FAILED TEST
  YOU HAVE 231 DISABLED TESTS

6 launcher.cpp:161] Asked to destroy container 
7995f0b2-2fba-430c-98a1-abd316d3485a
W0205 08:43:13.157941 30808 process.cpp:1423] Failed to recv on socket 
WindowsFD::Type::SOCKET=3456 to peer '192.10.1.6:61876': IO failed with error 
code: The specified network name is no longer available.

W0205 08:43:13.157941 30808 process.cpp:838] Failed to recv on socket 
WindowsFD::Type::SOCKET=3512 to peer '192.10.1.6:61877': IO failed with error 
code: The specified network name is no longer available.

I0205 08:43:13.255445 33920 containerizer.cpp:2983] Container 
7995f0b2-2fba-430c-98a1-abd316d3485a has exited
I0205 08:43:13.283437 34472 master.cpp:1109] Master terminating
I0205 08:43:13.285444 30556 hierarchical.cpp:644] Removed agent 
9bfba4c6-997f-4c2e-bff6-5867256f8ba7-S0
I0205 08:43:13.722429 30808 process.cpp:927] Stopped the socket accept loop
```

- Mesos Reviewbot Windows


On Feb. 5, 2019, 7:40 a.m., Chun-Hung Hsiao wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/69892/
> ---
> 
> (Updated Feb. 5, 2019, 7:40 a.m.)
> 
> 
> Review request for mesos, Benjamin Bannier, James DeFelice, and Jie Yu.
> 
> 
> Bugs: MESOS-9544
> https://issues.apache.org/jira/browse/MESOS-9544
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> If a CSI volume has been node-published before a reboot, SLRP will now
> try to bring it back to node-published again. This is important to
> perform synchronous persistent volume cleanup for `DESTROY`.
> 
> To achieve this, in addition to keeping track of the boot ID when a CSI
> volume is node-staged in `VolumeState.vol_ready_boot_id` (formerly
> `VolumeState.boot_id`), SLRP now also keeps track of the boot ID when
> the volume is node-published. This helps SLRP to better determine if a
> volume has been published before reboot.
> 
> 
> Diffs
> -
> 
>   src/csi/state.proto 264a5657dd37605a6f3bdadd0e8d18ba9673191a 
>   src/resource_provider/storage/provider.cpp 
> d6e20a549ede189c757ae3ae922ab7cb86d2be2c 
>   src/tests/storage_local_resource_provider_tests.cpp 
> e8ed20f818ed7f1a3ce15758ea3c366520443377 
> 
> 
> Diff: https://reviews.apache.org/r/69892/diff/1/

Re: Review Request 69885: Sped up some resource benchmark test instantiations.

[Benjamin Bannier]

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

(Updated Feb. 5, 2019, 9:27 a.m.)


Review request for mesos, Benjamin Mahler and Klaus Ma.


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


Repository: mesos


Description
---

This patch moves computation of some resource benchmark test parameters
from test instantiation time to test execution time. This prevents us
from having to perform the expensive calculation of test parameters even
when not executing the benchmark.

As a result the startup time of the Mesos tests binary is improved,
while the total wall time required to run these particular benchmarks is
degraded accordingly.


Diffs (updated)
-

  src/tests/resources_tests.cpp f762d17376cc5c29e8556ef5aa2b981e8fe19985 


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

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


Testing
---

Benchmarked `./src/mesos-tests --gtest_list_tests` with clang-9.0.0, lld-2.27. 
Overall execution time is improved, especially for not optimized builds.

```
Benchmark #1: Before patch, debug
  Time (mean ± ?):  2.706 s ±  0.018 s[User: 2.472 s, System: 0.168 s]
  Range (min … max):2.690 s …  2.732 s10 runs
Benchmark #2: After patch, debug
  Time (mean ± ?): 683.7 ms ±  18.1 ms[User: 474.2 ms, System: 152.9 ms]
  Range (min … max):   673.4 ms … 734.2 ms10 runs
```

```
Benchmark #3: Before patch, optimized
  Time (mean ± ?): 783.0 ms ±  15.0 ms[User: 537.4 ms, System: 144.9 ms]
  Range (min … max):   772.2 ms … 815.5 ms10 runs
Benchmark #4: After patch, optimized
  Time (mean ± ?): 572.5 ms ±   6.7 ms[User: 343.3 ms, System: 138.4 ms]
  Range (min … max):   562.2 ms … 588.7 ms10 runs
```

Remaining time is due to the long list of filters `mesos-tests` uses.


Thanks,

Benjamin Bannier

Review Request 69898: Updated `CreateDestroyDisk*` SLRP tests to test pipelined operations.

[Chun-Hung Hsiao]

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

Review request for mesos and Benjamin Bannier.


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


Repository: mesos


Description
---

This patch extends the code coverage of the `CreateDestroyDisk` and
`CreateDestroyDiskRecovery` tests by testing pipelined `RESERVE`,
`CREATE`, `DESTROY` and `UNRESERVE` operations along with `CREATE_DISK`
and `DESTROY_DISK`. It also renames `CreateDestroyDiskRecovery` to
`CreateDestroyDiskWithRecovery` for consistency.


Diffs
-

  src/tests/storage_local_resource_provider_tests.cpp 
e8ed20f818ed7f1a3ce15758ea3c366520443377 


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


Testing
---

`make check`

Run in repetition


Thanks,

Chun-Hung Hsiao

Review Request 69897: Made SLRP `PublishResourcesRecovery` test to check volume cleanup.

[Chun-Hung Hsiao]

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

Review request for mesos and Benjamin Bannier.


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


Repository: mesos


Description
---

This patch renames the `ROOT_PublishResourcesRecovery` test to
`ROOT_CreateDestroyPersistentMountVolumeWithRecovery` and makes it
verify that the persistent volume is cleaned up with `DESTROY` after
recovery.


Diffs
-

  src/tests/storage_local_resource_provider_tests.cpp 
e8ed20f818ed7f1a3ce15758ea3c366520443377 


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


Testing
---

`make check`

Run in repetition with ~250 iterations. (It will fail with "too many open 
files" with more iterations because of 
https://issues.apache.org/jira/browse/MESOS-8428.


Thanks,

Chun-Hung Hsiao

Review Request 69896: Made SLRP `PublishResourcesReboot` test to check persistent volume cleanup.

[Chun-Hung Hsiao]

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

Review request for mesos and Benjamin Bannier.


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


Repository: mesos


Description
---

This patch renames the `ROOT_PublishResourcesReboot` test to
`ROOT_CreateDestroyPersistentMountVolumeWithReboot` and makes it verify
that the persistent volume is cleaned up with `DESTROY` after a reboot.


Diffs
-

  src/tests/storage_local_resource_provider_tests.cpp 
e8ed20f818ed7f1a3ce15758ea3c366520443377 


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


Testing
---

`make check`

Run in repetition with ~1000 repetitions.


Thanks,

Chun-Hung Hsiao