Re: Review Request 70330: Used `ResourceQuantities` in the sorter when appropriate.

[Mesos Reviewbot]

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



Patch looks great!

Reviews applied: [70318, 70319, 70320, 70329, 70330]

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

- Mesos Reviewbot


On March 28, 2019, 2:10 a.m., Meng Zhu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/70330/
> ---
> 
> (Updated March 28, 2019, 2:10 a.m.)
> 
> 
> Review request for mesos and Benjamin Mahler.
> 
> 
> Bugs: MESOS-9504
> https://issues.apache.org/jira/browse/MESOS-9504
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Replaced `Resources` with `ResourceQuantities` when
> appropriate in the sorter. This simplifies the code
> and improves performance.
> 
> 
> Diffs
> -
> 
>   src/master/allocator/mesos/hierarchical.cpp 
> 8bc749903b8ceb09a02e260919377483479302b5 
>   src/master/allocator/sorter/drf/sorter.hpp 
> 75f90f331fbe2ec514daa3fe00b0b05ad55932e1 
>   src/master/allocator/sorter/drf/sorter.cpp 
> 43c97671d692675df6a347e4482126d83d7e3f24 
>   src/master/allocator/sorter/random/sorter.hpp 
> 2031cb234cc3e29723f07ec7d3a7e8671a26a97f 
>   src/master/allocator/sorter/random/sorter.cpp 
> 6fcfc41f65bb6401cfb60af88866c2b02920887e 
>   src/master/allocator/sorter/sorter.hpp 
> 25ad48dff7e624e7d25072958bdd20513ab83d12 
>   src/tests/sorter_tests.cpp 1e2791f993af2fba592b0e76493864c096a0bb5f 
> 
> 
> Diff: https://reviews.apache.org/r/70330/diff/1/
> 
> 
> Testing
> ---
> 
> make check
> 
> # Benchmark Result
> Optimized build on a 2.2GHz/64GB box
> Each ran three times for average
> 
> **TL;DR: ~20% speedup for nonquota benchmark; ~30% speedup for quota 
> benchmark**
> 
> ## Non-quota allocation
> 
> Ran `BENCHMARK_HierarchicalAllocator_WithNonQuotaVsQuotaParam.NonQuotaVsQuota`
> Selected the Nonquota result
> 
> Before:
> 
> - 20 agents, 10 roles, 20 frameworks
> Made 20 allocations in 2.382283667ms
> 
> - 200 agents, 100 roles, 200 frameworks
> Made 200 allocations in 19.05821233ms
> 
> - 2000 agents, 1000 roles, 2000 frameworks
> Made 2000 allocations in 792.1674363ms
> 
> After:
> 
> - 20 agents, 10 roles, 20 frameworks
> Made 20 allocations in 2.329543667ms // 2% speedup
> 
> - 200 agents, 100 roles, 200 frameworks
> Made 200 allocations in 15.40222667ms // 19% speedup
> 
> - 2000 agents, 1000 roles, 2000 frameworks
> Made 2000 allocations in 644.5257763ms // 19% speedup
> 
> ## Quota Allocation
> 
> Ran `BENCHMARK_HierarchicalAllocator_WithQuotaParam.LargeAndSmallQuota`
> 
> Before:
>   
> - 30 agents, 30 roles, 30 frameworks
> Made 36 allocations in 9.656610667ms
> 
> - 300 agents, 300 roles, 300 frameworks
> Made 350 allocations in 292.0376493ms
>  
> - 3000 agents, 3000 roles, 3000 frameworks
> Made 3500 allocations in 22.74423621secs
> 
> After:
> 
> - 30 agents, 30 roles, 30 frameworks
> Made 36 allocations in 6.74207ms  // 30% speedup
> 
> - 300 agents, 300 roles, 300 frameworks
> Made 350 allocations in 187.105902ms  // 35% speedup
>  
> - 3000 agents, 3000 roles, 3000 frameworks
> Made 3500 allocations in 14.74580913secs // 36% speedup
> 
> 
> Thanks,
> 
> Meng Zhu
> 
>

Re: Review Request 70330: Used `ResourceQuantities` in the sorter when appropriate.

[Mesos Reviewbot Windows]

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



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

Reviews applied: `['70318', '70319', '70320', '70329', '70330']`

Failed command: `Start-MesosCITesting`

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

Relevant logs:

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

```
I0328 05:55:13.160986 17808 master.cpp:11685] Removing task 
e9cb7922-ca4c-4a91-a2cc-86d4e2d784a8 with resources cpus(allocated: *):4; 
mem(allocated: *):2048; disk(allocated: *):1024; ports(allocated: 
*):[31000-32000] of framework 6091452b-dbb2-4e9e-bd17-95fc87328817- on 
agent 6091452b-dbb2-4e9e-bd17-95fc87328817-S0 at slave(500)@192.10.1.4:58249 
(windows-01.chtsmhjxogyevckjfayqqcnjda.xx.internal.cloudapp.net)
I0328 05:55:13.163980 17808 master.cpp:1295] Agent 
6091452b-dbb2-4e9e-bd17-95fc87328817-S0 at slave(500)@192.10.1.4:58249 
(windows-01.chtsmhjxogyevckjfayqqcnjda.xx.internal.cloudapp.net) disconnected
I0328 05:55:13.163980 17808 master.cpp:3330] Disconnecting agent 
6091452b-dbb2-4e9e-bd17-95fc87328817-S0 at slave(500)@192.10.1.4:58249 
(windows-01.chtsmhjxogyevckjfayqqcnjda.xx.internal.cloudapp.net)
I0328 05:55:13.165027 17808 master.cpp:3349] Deactivating agent 
6091452b-dbb2-4e9e-bd17-95fc87328817-S0 at slave(500)@192.10.1.4:58249 
(windows-01.chtsmhjxogyevckjfayqqcnjda.xx.internal.cloudapp.net)
I0328 05:55:13.165027 19560 hierarchical.cpp:391] Removed framework 
6091452b-dbb2-4e9e-bd17-95fc87328817-
I0328 05:55:13.165027 18796 containerizer.cpp:2576] Destroying container 
58418bf9-0bf6-43c0-ae50-0c29efb7a31e in RUNNING state
I0328 05:55:13.165027 18796 containerizer.cpp:3278] Transitioning the state of 
container 58418bf9-0bf6-43c0-ae50-0c29efb7a31e from RUNNING to DESTROYING
I0328 05:55:13.165027 19560 hierarchical.cpp:828] Agent 
6091452b-dbb2-4e9e-bd17-95fc87328817-S0 deactivated
I0328 05:55:13.166059 18796 [   OK ] 
IsolationFlag/MemoryIsolatorTest.ROOT_MemUsage/0 (795 ms)
[--] 1 test from IsolationFlag/MemoryIsolatorTest (813 ms total)

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

 1 FAILED TEST
  YOU HAVE 231 DISABLED TESTS

launcher.cpp:161] Asked to destroy container 
58418bf9-0bf6-43c0-ae50-0c29efb7a31e
W0328 05:55:13.167023 16596 process.cpp:838] Failed to recv on socket 
WindowsFD::Type::SOCKET=3460 to peer '192.10.1.4:60638': IO failed with error 
code: The specified network name is no longer available.

W0328 05:55:13.167975 16596 process.cpp:1423] Failed to recv on socket 
WindowsFD::Type::SOCKET=4860 to peer '192.10.1.4:60637': IO failed with error 
code: The specified network name is no longer available.

I0328 05:55:13.255828 14096 containerizer.cpp:3117] Container 
58418bf9-0bf6-43c0-ae50-0c29efb7a31e has exited
I0328 05:55:13.289773 19524 master.cpp:1135] Master terminating
I0328 05:55:13.290771 14096 hierarchical.cpp:679] Removed agent 
6091452b-dbb2-4e9e-bd17-95fc87328817-S0
I0328 05:55:13.682775 16596 process.cpp:927] Stopped the socket accept loop
```

- Mesos Reviewbot Windows


On March 28, 2019, 2:10 a.m., Meng Zhu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/70330/
> ---
> 
> (Updated March 28, 2019, 2:10 a.m.)
> 
> 
> Review request for mesos and Benjamin Mahler.
> 
> 
> Bugs: MESOS-9504
> https://issues.apache.org/jira/browse/MESOS-9504
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Replaced `Resources` with `ResourceQuantities` when
> appropriate in the sorter. This simplifies the code
> and improves performance.
> 
> 
> Diffs
> -
> 
>   src/master/allocator/mesos/hierarchical.cpp 
> 8bc749903b8ceb09a02e260919377483479302b5 
>   src/master/allocator/sorter/drf/sorter.hpp 
> 75f90f331fbe2ec514daa3fe00b0b05ad55932e1 
>   src/master/allocator/sorter/drf/sorter.cpp 
> 43c97671d692675df6a347e4482126d83d7e3f24 
>   src/master/allocator/sorter/random/sorter.hpp 
> 2031cb234cc3e29723f07ec7d3a7e8671a26a97f 
>   src/master/allocator/sorter/random/sorter.cpp 
> 6fcfc41f65bb6401cfb60af88866c2b02920887e 
>   src/master/allocator/sorter/sorter.hpp 
> 25ad48dff7e624e7d25072958bdd20513ab83d12 
>   src/tests/sorter_tests.cpp 1e2791f993af2fba592b0e76493864c096a0bb5f 
> 
> 
> Diff: https://reviews.apache.org/r/70330/diff/1/
> 
> 
> Testing
> ---
> 
> make check
> 
> # 

Review Request 70331: Updated test `AgentRegisteredWithNewId` for better code coverage.

[Chun-Hung Hsiao]

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

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


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


Repository: mesos


Description
---

This patch makes the `AgentRegisteredWithNewId` SLRP test to test the
following scenarios:

  * Launch a task to access a volume imported through `CREATE_DISK`.

  * `CREATE_DISK` with a mismatched profile.

  * `DESTROY_DISK` with a `RAW` disk that has been published.

Together with the `CreateDestroyPreprovisionedVolume` SLRP test, most
scenarios of preprovisioned volumes are now covered.


Diffs
-

  src/tests/storage_local_resource_provider_tests.cpp 
797f89e3545965e2cf2fd5ec0ecd78fc77a4ea87 


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


Testing
---

sudo make check

Run in repetition.


Thanks,

Chun-Hung Hsiao

Re: Review Request 70316: Updated test `ImportPreprovisionedVolume` for better code coverage.

[Chun-Hung Hsiao]

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

(Updated March 28, 2019, 5:15 a.m.)


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


Changes
---

Addressed Benjamin's comments and fixed a flake.


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


Repository: mesos


Description
---

This patch renames `ImportPreprovisionedVolume` to
`CreateDestroyPreprovisionedVolume` add tests two additional scenarios:

* `CREATE_DISK` with an unknown profile.

* `DESTROY_DISK` with a `RAW` disk.


Diffs (updated)
-

  src/tests/storage_local_resource_provider_tests.cpp 
797f89e3545965e2cf2fd5ec0ecd78fc77a4ea87 


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

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


Testing (updated)
---

make check

Ran 1000 times.


Thanks,

Chun-Hung Hsiao

Re: Review Request 70315: Added authorization for applying `DESTROY_DISK` on `RAW` disks.

[Chun-Hung Hsiao]

> On March 27, 2019, 10:03 a.m., Benjamin Bannier wrote:
> > src/tests/authorization_tests.cpp
> > Lines 7052-7064 (patched)
> > 
> >
> > Once should be enough?

Aha. I copy-pasted from `DestroyMountDisk` ;) The same errors also occur in 
`CreateMountDisk`, `CreateBlockDisk` and `DestroyBlockDisk` as well. Let me fix 
them.


- Chun-Hung


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


On March 28, 2019, 5:13 a.m., Chun-Hung Hsiao wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/70315/
> ---
> 
> (Updated March 28, 2019, 5:13 a.m.)
> 
> 
> Review request for mesos, Benjamin Bannier, Jie Yu, and Jan Schlicht.
> 
> 
> Bugs: MESOS-9540
> https://issues.apache.org/jira/browse/MESOS-9540
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added authorization for applying `DESTROY_DISK` on `RAW` disks.
> 
> 
> Diffs
> -
> 
>   docs/examples/acls_template.json ad28cb80a504e9c3ef3167b87dc9b285f3832ffa 
>   include/mesos/authorizer/acls.proto 
> 4c3f2907a25c2920a0f382e3937898774bbf49fd 
>   include/mesos/authorizer/authorizer.proto 
> f9060531cf1f6bc60786e6d6e6b87310f1bc0927 
>   src/authorizer/local/authorizer.cpp 
> 85e18b958932fca74f7860bb19b178835a1636f9 
>   src/master/master.cpp c519d5a350208556bdf55c63daae38da745ddfc6 
>   src/tests/authorization_tests.cpp e85cdb681ae2d1a9f215ce9d07a56e85346e3dab 
> 
> 
> Diff: https://reviews.apache.org/r/70315/diff/2/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Chun-Hung Hsiao
> 
>

Re: Review Request 70315: Added authorization for applying `DESTROY_DISK` on `RAW` disks.

[Chun-Hung Hsiao]

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

(Updated March 28, 2019, 5:13 a.m.)


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


Changes
---

Addressed Benjamin's comments.


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


Repository: mesos


Description
---

Added authorization for applying `DESTROY_DISK` on `RAW` disks.


Diffs (updated)
-

  docs/examples/acls_template.json ad28cb80a504e9c3ef3167b87dc9b285f3832ffa 
  include/mesos/authorizer/acls.proto 4c3f2907a25c2920a0f382e3937898774bbf49fd 
  include/mesos/authorizer/authorizer.proto 
f9060531cf1f6bc60786e6d6e6b87310f1bc0927 
  src/authorizer/local/authorizer.cpp 85e18b958932fca74f7860bb19b178835a1636f9 
  src/master/master.cpp c519d5a350208556bdf55c63daae38da745ddfc6 
  src/tests/authorization_tests.cpp e85cdb681ae2d1a9f215ce9d07a56e85346e3dab 


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

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


Testing
---

make check


Thanks,

Chun-Hung Hsiao

Re: Review Request 70314: Supported destroying preprovisioned CSI volumes in SLRP.

[Chun-Hung Hsiao]

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

(Updated March 28, 2019, 5:11 a.m.)


Review request for mesos, Benjamin Bannier and Jie Yu.


Changes
---

Addressed Benjamin's comments.


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


Repository: mesos


Description
---

SLRP now accepts `DESTROY_DISK` on `RAW` disk resources with source IDs.
If the backed CSI plugin does have the `CREATE_DELETE_VOLUME` controller
capability, this operation will be a no-op; otherwise the underlying CSI
volume will be deprovisioned.


Diffs (updated)
-

  src/resource_provider/storage/provider.cpp 
2711503cdb58cb9b34af8c9fad0908c5f788a2af 


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

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


Testing
---

make check

The new code path is tested later in this chain.


Thanks,

Chun-Hung Hsiao

Re: Review Request 70320: Used `ResourceQuantities` in `__allocate()` when appropriate.

[Mesos Reviewbot]

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



Patch looks great!

Reviews applied: [70318, 70319, 70320]

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

- Mesos Reviewbot


On March 27, 2019, 5:31 a.m., Meng Zhu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/70320/
> ---
> 
> (Updated March 27, 2019, 5:31 a.m.)
> 
> 
> Review request for mesos and Benjamin Mahler.
> 
> 
> Bugs: MESOS-9504
> https://issues.apache.org/jira/browse/MESOS-9504
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Replaced `Resources` with `ResourceQuantities` when
> appropriate in `__allocate()`. This simplifies the
> code and improves performance.
> 
> 
> Diffs
> -
> 
>   src/master/allocator/mesos/hierarchical.hpp 
> 36bf90baf413e99c1580d516dfac0f074335d322 
>   src/master/allocator/mesos/hierarchical.cpp 
> 8bc749903b8ceb09a02e260919377483479302b5 
> 
> 
> Diff: https://reviews.apache.org/r/70320/diff/2/
> 
> 
> Testing
> ---
> 
> make check
> Benchmark results in r/70330
> 
> 
> Thanks,
> 
> Meng Zhu
> 
>

Review Request 70329: Added `==` and `!=` operator in `ResourceQuantities`.

[Meng Zhu]

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

Review request for mesos and Benjamin Mahler.


Repository: mesos


Description
---

Added `==` and `!=` operator in `ResourceQuantities`.


Diffs
-

  src/common/resource_quantities.hpp db4f5afb9f617e27bd08756020b4bcf0bd92dc66 
  src/common/resource_quantities.cpp 9ae5d9f4aefde81cde7463a7ce118c65a68fc0db 


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


Testing
---

make check
Also tested in r/70330


Thanks,

Meng Zhu

Review Request 70330: Used `ResourceQuantities` in the sorter when appropriate.

[Meng Zhu]

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

Review request for mesos and Benjamin Mahler.


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


Repository: mesos


Description
---

Replaced `Resources` with `ResourceQuantities` when
appropriate in the sorter. This simplifies the code
and improves performance.


Diffs
-

  src/master/allocator/mesos/hierarchical.cpp 
8bc749903b8ceb09a02e260919377483479302b5 
  src/master/allocator/sorter/drf/sorter.hpp 
75f90f331fbe2ec514daa3fe00b0b05ad55932e1 
  src/master/allocator/sorter/drf/sorter.cpp 
43c97671d692675df6a347e4482126d83d7e3f24 
  src/master/allocator/sorter/random/sorter.hpp 
2031cb234cc3e29723f07ec7d3a7e8671a26a97f 
  src/master/allocator/sorter/random/sorter.cpp 
6fcfc41f65bb6401cfb60af88866c2b02920887e 
  src/master/allocator/sorter/sorter.hpp 
25ad48dff7e624e7d25072958bdd20513ab83d12 
  src/tests/sorter_tests.cpp 1e2791f993af2fba592b0e76493864c096a0bb5f 


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


Testing
---

make check

# Benchmark Result
Optimized build on a 2.2GHz/64GB box
Each ran three times for average

**TL;DR: ~20% speedup for nonquota benchmark; ~30% speedup for quota benchmark**

## Non-quota allocation

Ran `BENCHMARK_HierarchicalAllocator_WithNonQuotaVsQuotaParam.NonQuotaVsQuota`
Selected the Nonquota result

Before:

- 20 agents, 10 roles, 20 frameworks
Made 20 allocations in 2.382283667ms

- 200 agents, 100 roles, 200 frameworks
Made 200 allocations in 19.05821233ms

- 2000 agents, 1000 roles, 2000 frameworks
Made 2000 allocations in 792.1674363ms

After:

- 20 agents, 10 roles, 20 frameworks
Made 20 allocations in 2.329543667ms // 2% speedup

- 200 agents, 100 roles, 200 frameworks
Made 200 allocations in 15.40222667ms // 19% speedup

- 2000 agents, 1000 roles, 2000 frameworks
Made 2000 allocations in 644.5257763ms // 19% speedup

## Quota Allocation

Ran `BENCHMARK_HierarchicalAllocator_WithQuotaParam.LargeAndSmallQuota`

Before:

- 30 agents, 30 roles, 30 frameworks
Made 36 allocations in 9.656610667ms

- 300 agents, 300 roles, 300 frameworks
Made 350 allocations in 292.0376493ms
 
- 3000 agents, 3000 roles, 3000 frameworks
Made 3500 allocations in 22.74423621secs

After:

- 30 agents, 30 roles, 30 frameworks
Made 36 allocations in 6.74207ms  // 30% speedup

- 300 agents, 300 roles, 300 frameworks
Made 350 allocations in 187.105902ms  // 35% speedup
 
- 3000 agents, 3000 roles, 3000 frameworks
Made 3500 allocations in 14.74580913secs // 36% speedup


Thanks,

Meng Zhu

Re: Review Request 70320: Used `ResourceQuantities` in `__allocate()` when appropriate.

[Meng Zhu]

> On March 27, 2019, 7:41 a.m., Benjamin Mahler wrote:
> > src/master/allocator/mesos/hierarchical.cpp
> > Lines 1928 (patched)
> > 
> >
> > Rather than adding this additional function, can't `shrinkResoures` 
> > take a `ResourceQuantities`?

We need to create a map-like a copy somewhere. `shrinkResoures` needs a copy of 
its own to do bookkeeping:
https://github.com/apache/mesos/blob/c10def96653fb99e309459476f5bc09c1da686ee/src/master/allocator/mesos/hierarchical.cpp#L1666

Or do you mean to create the map in the `shrinkResoures` function? My concern 
with that is currently `shrinkResources` has this semantic:
>If a resource does not have a target quantity provided, it will not be shrunk.

This is not compatible with `ResourceQuantities`. Given the semantic of 
`ResourceQuantities`, one would expect `shrinkResources` should drop (shrink to 
zero) for resources that are absent in the input `ResourceQuantities`.

Of course, since we are the only consumer of the function here, we could change 
the function semantic. But I do not see much benefits since, as mentioned 
above, we need to pass a map anyway.

Another thing to consider is we might need to get a copy of the scalar map 
somewhere else where direct mutations are needed.


> On March 27, 2019, 7:41 a.m., Benjamin Mahler wrote:
> > src/master/allocator/mesos/hierarchical.cpp
> > Lines 2088-2091 (original), 2079-2082 (patched)
> > 
> >
> > This looks like a `ResourceQuantities`?

Yes and no. Below:
```
if (!sufficientHeadroom) {
  toAllocate -= headroomToAllocate;
}
```
We need that as `Resources`.

But I do notice that we convert it to `ResourceQuantities` twice below. Might 
as well keep one in the scope.


- Meng


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


On March 26, 2019, 10:31 p.m., Meng Zhu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/70320/
> ---
> 
> (Updated March 26, 2019, 10:31 p.m.)
> 
> 
> Review request for mesos and Benjamin Mahler.
> 
> 
> Bugs: MESOS-9504
> https://issues.apache.org/jira/browse/MESOS-9504
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Replaced `Resources` with `ResourceQuantities` when
> appropriate in `__allocate()`. This simplifies the
> code and improves performance.
> 
> 
> Diffs
> -
> 
>   src/master/allocator/mesos/hierarchical.hpp 
> 36bf90baf413e99c1580d516dfac0f074335d322 
>   src/master/allocator/mesos/hierarchical.cpp 
> 8bc749903b8ceb09a02e260919377483479302b5 
> 
> 
> Diff: https://reviews.apache.org/r/70320/diff/2/
> 
> 
> Testing
> ---
> 
> make check
> Benchmark results in r/70330
> 
> 
> Thanks,
> 
> Meng Zhu
> 
>

Re: Review Request 70325: Updated the master to allocate recovered orphan operation resources.

[Greg Mann]

> On March 27, 2019, 11:55 p.m., Greg Mann wrote:
> > src/master/master.cpp
> > Lines 10538-10541 (patched)
> > 
> >
> > Looking at this again, I guess I should build up a `hashmap > std::pair>` and make just one `addAgentResources()` 
> > call per agent.

er... make that `hashmap>>`


- Greg


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


On March 27, 2019, 7:59 p.m., Greg Mann wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/70325/
> ---
> 
> (Updated March 27, 2019, 7:59 p.m.)
> 
> 
> Review request for mesos, Benjamin Mahler, Gastón Kleiman, Joseph Wu, and 
> Meng Zhu.
> 
> 
> Bugs: MESOS-9635
> https://issues.apache.org/jira/browse/MESOS-9635
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This patch updates the master's framework recovery code to use
> the allocator's `addAgentResources()` method rather than
> `updateSlave()` when recovering orphan operations, which has the
> benefit of tracking the allocation of the operations' consumed
> resources, avoiding situations in which those resources would be
> incorrectly offered to frameworks while the operation is still
> in a pending state.
> 
> 
> Diffs
> -
> 
>   src/master/master.cpp acc67d3763ddee9027e6cf375f1d495ff5805026 
> 
> 
> Diff: https://reviews.apache.org/r/70325/diff/1/
> 
> 
> Testing
> ---
> 
> `make check`
> 
> To verify the flaky test fix, the following command was executed both before 
> and after the patches were applied, while `stress -c ` 
> was being run:
> `bin/mesos-tests.sh 
> --gtest_filter="*AgentPendingOperationAfterMasterFailover*" --gtest_repeat=-1 
> --gtest_break_on_failure`
> 
> Before the patches were applied, the test would reliably fail after less than 
> 50 repetitions. After the patches are applied, the test can be run for 
> hundreds of repetitions with no failures.
> 
> 
> Thanks,
> 
> Greg Mann
> 
>

Re: Review Request 70325: Updated the master to allocate recovered orphan operation resources.

[Greg Mann]

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




src/master/master.cpp
Lines 10538-10541 (patched)


Looking at this again, I guess I should build up a `hashmap>` and make just one `addAgentResources()` call 
per agent.


- Greg Mann


On March 27, 2019, 7:59 p.m., Greg Mann wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/70325/
> ---
> 
> (Updated March 27, 2019, 7:59 p.m.)
> 
> 
> Review request for mesos, Benjamin Mahler, Gastón Kleiman, Joseph Wu, and 
> Meng Zhu.
> 
> 
> Bugs: MESOS-9635
> https://issues.apache.org/jira/browse/MESOS-9635
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This patch updates the master's framework recovery code to use
> the allocator's `addAgentResources()` method rather than
> `updateSlave()` when recovering orphan operations, which has the
> benefit of tracking the allocation of the operations' consumed
> resources, avoiding situations in which those resources would be
> incorrectly offered to frameworks while the operation is still
> in a pending state.
> 
> 
> Diffs
> -
> 
>   src/master/master.cpp acc67d3763ddee9027e6cf375f1d495ff5805026 
> 
> 
> Diff: https://reviews.apache.org/r/70325/diff/1/
> 
> 
> Testing
> ---
> 
> `make check`
> 
> To verify the flaky test fix, the following command was executed both before 
> and after the patches were applied, while `stress -c ` 
> was being run:
> `bin/mesos-tests.sh 
> --gtest_filter="*AgentPendingOperationAfterMasterFailover*" --gtest_repeat=-1 
> --gtest_break_on_failure`
> 
> Before the patches were applied, the test would reliably fail after less than 
> 50 repetitions. After the patches are applied, the test can be run for 
> hundreds of repetitions with no failures.
> 
> 
> Thanks,
> 
> Greg Mann
> 
>

Re: Review Request 70325: Updated the master to allocate recovered orphan operation resources.

[Greg Mann]

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

(Updated March 27, 2019, 7:59 p.m.)


Review request for mesos, Benjamin Mahler, Gastón Kleiman, Joseph Wu, and Meng 
Zhu.


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


Repository: mesos


Description
---

This patch updates the master's framework recovery code to use
the allocator's `addAgentResources()` method rather than
`updateSlave()` when recovering orphan operations, which has the
benefit of tracking the allocation of the operations' consumed
resources, avoiding situations in which those resources would be
incorrectly offered to frameworks while the operation is still
in a pending state.


Diffs
-

  src/master/master.cpp acc67d3763ddee9027e6cf375f1d495ff5805026 


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


Testing (updated)
---

`make check`

To verify the flaky test fix, the following command was executed both before 
and after the patches were applied, while `stress -c ` 
was being run:
`bin/mesos-tests.sh --gtest_filter="*AgentPendingOperationAfterMasterFailover*" 
--gtest_repeat=-1 --gtest_break_on_failure`

Before the patches were applied, the test would reliably fail after less than 
50 repetitions. After the patches are applied, the test can be run for hundreds 
of repetitions with no failures.


Thanks,

Greg Mann

Re: Review Request 70325: Updated the master to allocate recovered orphan operation resources.

[Greg Mann]

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

(Updated March 27, 2019, 7:57 p.m.)


Review request for mesos, Benjamin Mahler, Gastón Kleiman, Joseph Wu, and Meng 
Zhu.


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


Repository: mesos


Description (updated)
---

This patch updates the master's framework recovery code to use
the allocator's `addAgentResources()` method rather than
`updateSlave()` when recovering orphan operations, which has the
benefit of tracking the allocation of the operations' consumed
resources, avoiding situations in which those resources would be
incorrectly offered to frameworks while the operation is still
in a pending state.


Diffs
-

  src/master/master.cpp acc67d3763ddee9027e6cf375f1d495ff5805026 


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


Testing (updated)
---

`make check`

To verify the flaky test fix, the following command was executed both before 
and after the patches were applied, while `stress -c ` 
was being run:
`bin/mesos-tests.sh --gtest_filter="*AgentPendingOperationAfterMasterFailover*" 
--gtest_repeat=-1 --gtest_break_on_failure`


Thanks,

Greg Mann

Re: Review Request 70316: Updated test `ImportPreprovisionedVolume` for better code coverage.

[Chun-Hung Hsiao]

> On March 27, 2019, 11 a.m., Benjamin Bannier wrote:
> > src/tests/storage_local_resource_provider_tests.cpp
> > Lines 2931 (patched)
> > 
> >
> > Is this related to above comment block?

Yes, "the extra storage pool."


> On March 27, 2019, 11 a.m., Benjamin Bannier wrote:
> > src/tests/storage_local_resource_provider_tests.cpp
> > Lines 3050 (patched)
> > 
> >
> > Let's add an assertion that the range resulting from the filter is 
> > actually non-empty.

The "AllOf" expectation matcher should have ensured that.


- Chun-Hung


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


On March 27, 2019, 3:45 a.m., Chun-Hung Hsiao wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/70316/
> ---
> 
> (Updated March 27, 2019, 3:45 a.m.)
> 
> 
> Review request for mesos, Benjamin Bannier, Jie Yu, and Jan Schlicht.
> 
> 
> Bugs: MESOS-9540
> https://issues.apache.org/jira/browse/MESOS-9540
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This patch renames `ImportPreprovisionedVolume` to
> `CreateDestroyPreprovisionedVolume` add tests two additional scenarios:
> 
> * `CREATE_DISK` with an unknown profile.
> 
> * `DESTROY_DISK` with a `RAW` disk.
> 
> 
> Diffs
> -
> 
>   src/tests/storage_local_resource_provider_tests.cpp 
> 797f89e3545965e2cf2fd5ec0ecd78fc77a4ea87 
> 
> 
> Diff: https://reviews.apache.org/r/70316/diff/2/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Chun-Hung Hsiao
> 
>

Re: Review Request 70314: Supported destroying preprovisioned CSI volumes in SLRP.

[Chun-Hung Hsiao]

> On March 27, 2019, 9:51 a.m., Benjamin Bannier wrote:
> > src/resource_provider/storage/provider.cpp
> > Line 1 (original), 1 (patched)
> > 
> >
> > This patch should contain a test of introduced behavior.

Test done later in chain. I prefer splitting the fix that is targeting for 
backporting and the test, so the test needs not to be backported. Or do you 
think we should backport the test as well?


> On March 27, 2019, 9:51 a.m., Benjamin Bannier wrote:
> > src/resource_provider/storage/provider.cpp
> > Lines 2702 (patched)
> > 
> >
> > All this local checking of capabilities at call sites instead of in 
> > `call` is getting cumbersome and error-prone. Would be great to clean that 
> > up eventually by moving it to a single place.

`call` should be a darn simple wrapper. The call site has more context about 
what to check and how to react. In this example, the lack of this capability 
shouldn't result in an error.


> On March 27, 2019, 9:51 a.m., Benjamin Bannier wrote:
> > src/resource_provider/storage/provider.cpp
> > Lines 3202-3203 (original), 3194-3196 (patched)
> > 
> >
> > Let's use a `switch` now that we have three cases here.

Good point. These checks actually don't give us much value and I was thinking 
about removing them. Using a `switch` and moving it into the continuation 
instead, could enable the compiler to check if we forget to clear some field in 
the future.


> On March 27, 2019, 9:51 a.m., Benjamin Bannier wrote:
> > src/resource_provider/storage/provider.cpp
> > Lines 3237-3239 (original), 3235-3237 (patched)
> > 
> >
> > No quotes around `resource`; let's also fix the wrapping so that 
> > related entities are on the same line (`"resource provider"` and 
> > `info.id()`; `"resource"` and `resource`; opening and closing quotes).
> > ```
> > LOG(INFO)
> >   << "Reconciling storage pools for resource provider " << info.id()
> >   << " after resource '" << resource << "' has been freed";

We usually quote resoucre in the codebase.


- Chun-Hung


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


On March 27, 2019, 3:45 a.m., Chun-Hung Hsiao wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/70314/
> ---
> 
> (Updated March 27, 2019, 3:45 a.m.)
> 
> 
> Review request for mesos, Benjamin Bannier and Jie Yu.
> 
> 
> Bugs: MESOS-9540
> https://issues.apache.org/jira/browse/MESOS-9540
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> SLRP now accepts `DESTROY_DISK` on `RAW` disk resources with source IDs.
> If the backed CSI plugin does have the `CREATE_DELETE_VOLUME` controller
> capability, this operation will be a no-op; otherwise the underlying CSI
> volume will be deprovisioned.
> 
> 
> Diffs
> -
> 
>   src/resource_provider/storage/provider.cpp 
> 2711503cdb58cb9b34af8c9fad0908c5f788a2af 
> 
> 
> Diff: https://reviews.apache.org/r/70314/diff/2/
> 
> 
> Testing
> ---
> 
> make check
> 
> The new code path is tested later in this chain.
> 
> 
> Thanks,
> 
> Chun-Hung Hsiao
> 
>

Re: Review Request 70324: Added a new allocator method and deprecated an existing one.

[Mesos Reviewbot Windows]

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



PASS: Mesos patch 70324 was successfully built and tested.

Reviews applied: `['70324']`

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

- Mesos Reviewbot Windows


On March 27, 2019, 10 a.m., Greg Mann wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/70324/
> ---
> 
> (Updated March 27, 2019, 10 a.m.)
> 
> 
> Review request for mesos, Benjamin Mahler, Gastón Kleiman, Joseph Wu, and 
> Meng Zhu.
> 
> 
> Bugs: MESOS-9635
> https://issues.apache.org/jira/browse/MESOS-9635
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This patch deprecates the `addResourceProvider()` allocator method
> in favor of a new one, `addAgentResources()`, with exactly the same
> logic. The old method is left in place, but its logic is completely
> removed.
> 
> 
> Diffs
> -
> 
>   include/mesos/allocator/allocator.hpp 
> f1868387b28ea1d303cd3ed0176e32d66debd4c1 
>   src/master/allocator/mesos/allocator.hpp 
> 2d83f382eed9d9dca218adf84bc03883f7486349 
>   src/master/allocator/mesos/hierarchical.hpp 
> 36bf90baf413e99c1580d516dfac0f074335d322 
>   src/master/allocator/mesos/hierarchical.cpp 
> 8bc749903b8ceb09a02e260919377483479302b5 
>   src/master/master.cpp acc67d3763ddee9027e6cf375f1d495ff5805026 
>   src/tests/allocator.hpp 0718bef4de39a82dbcadebb9967d2ff1cceb1cae 
>   src/tests/hierarchical_allocator_tests.cpp 
> a34fa96fbc455d830c1fd7c1df83f3db72c96ee3 
> 
> 
> Diff: https://reviews.apache.org/r/70324/diff/1/
> 
> 
> Testing
> ---
> 
> `make check`
> 
> 
> Thanks,
> 
> Greg Mann
> 
>

Re: Review Request 70295: Enabled launcher sealing for RPM packages.

[Benjamin Bannier]

> On March 26, 2019, 2:21 a.m., Joseph Wu wrote:
> > support/packaging/centos/mesos.spec
> > Lines 94 (patched)
> > 
> >
> > I'd expect the `--disable-libtool-wrappers` flag too, but do rpm builds 
> > omit libtool wrappers anyway? 
> > 
> > It's not clear to me when exactly the wrappers are generated and when 
> > they aren't
> 
> Benjamin Bannier wrote:
> The libtool wrappers (which set up the env to consume dynamic libraries 
> built as part of the build) are generated so executables can be run from the 
> build tree, without installing. They are never installed. OTOH, executables 
> built in autotools projects without libtool wrappers cannot be installed as 
> they are not correctly linked (they'd depend on files from the build tree, 
> not from the install prefix).
> 
> Dropping.
> 
> Gilbert Song wrote:
> @Joseph, in this case, we don't need to disable libtool wrappers since 
> --enable-optimize would make sure there is no .sh init helper generated.

@gilbert, that is incorrect, whether libtool wrappers are used or not is 
completely orthogonal to the used optimization level.

One must never disable libtool wrappers when planning to install the created 
binary artifacts.


- Benjamin


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


On March 25, 2019, 4:09 p.m., Benjamin Bannier wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/70295/
> ---
> 
> (Updated March 25, 2019, 4:09 p.m.)
> 
> 
> Review request for mesos, Gilbert Song, Joseph Wu, and Till Toenshoff.
> 
> 
> Bugs: MESOS-9677
> https://issues.apache.org/jira/browse/MESOS-9677
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> We enable this flag since with it disabled certain public functions
> are not available making it hard to e.g., write modules against this
> version of Mesos.
> 
> While launcher sealing depends on a recent kernel, the platform we
> build RPMs for already satisfies the requirements.
> 
> 
> Diffs
> -
> 
>   support/packaging/centos/mesos.spec 
> de905fda3366be5904a0aec8249abcc53b0af9a0 
> 
> 
> Diff: https://reviews.apache.org/r/70295/diff/1/
> 
> 
> Testing
> ---
> 
> `make check`
> 
> 
> Thanks,
> 
> Benjamin Bannier
> 
>

Re: Review Request 70295: Enabled launcher sealing for RPM packages.

[Gilbert Song]

> On March 27, 2019, 5:57 a.m., James DeFelice wrote:
> > support/packaging/centos/mesos.spec
> > Lines 94 (patched)
> > 
> >
> > we probably want to conditionally enable this depending on the centos 
> > version. for example I don't think el6 ships w/ the necessary kernel 
> > support for launcher-sealing, but I think that el7 does.
> > 
> > see https://fedoraproject.org/wiki/Packaging:DistTag

+1


- Gilbert


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


On March 25, 2019, 8:09 a.m., Benjamin Bannier wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/70295/
> ---
> 
> (Updated March 25, 2019, 8:09 a.m.)
> 
> 
> Review request for mesos, Gilbert Song, Joseph Wu, and Till Toenshoff.
> 
> 
> Bugs: MESOS-9677
> https://issues.apache.org/jira/browse/MESOS-9677
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> We enable this flag since with it disabled certain public functions
> are not available making it hard to e.g., write modules against this
> version of Mesos.
> 
> While launcher sealing depends on a recent kernel, the platform we
> build RPMs for already satisfies the requirements.
> 
> 
> Diffs
> -
> 
>   support/packaging/centos/mesos.spec 
> de905fda3366be5904a0aec8249abcc53b0af9a0 
> 
> 
> Diff: https://reviews.apache.org/r/70295/diff/1/
> 
> 
> Testing
> ---
> 
> `make check`
> 
> 
> Thanks,
> 
> Benjamin Bannier
> 
>

Re: Review Request 70295: Enabled launcher sealing for RPM packages.

[Gilbert Song]

> On March 25, 2019, 6:21 p.m., Joseph Wu wrote:
> > support/packaging/centos/mesos.spec
> > Lines 94 (patched)
> > 
> >
> > I'd expect the `--disable-libtool-wrappers` flag too, but do rpm builds 
> > omit libtool wrappers anyway? 
> > 
> > It's not clear to me when exactly the wrappers are generated and when 
> > they aren't
> 
> Benjamin Bannier wrote:
> The libtool wrappers (which set up the env to consume dynamic libraries 
> built as part of the build) are generated so executables can be run from the 
> build tree, without installing. They are never installed. OTOH, executables 
> built in autotools projects without libtool wrappers cannot be installed as 
> they are not correctly linked (they'd depend on files from the build tree, 
> not from the install prefix).
> 
> Dropping.

@Joseph, in this case, we don't need to disable libtool wrappers since 
--enable-optimize would make sure there is no .sh init helper generated.


- Gilbert


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


On March 25, 2019, 8:09 a.m., Benjamin Bannier wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/70295/
> ---
> 
> (Updated March 25, 2019, 8:09 a.m.)
> 
> 
> Review request for mesos, Gilbert Song, Joseph Wu, and Till Toenshoff.
> 
> 
> Bugs: MESOS-9677
> https://issues.apache.org/jira/browse/MESOS-9677
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> We enable this flag since with it disabled certain public functions
> are not available making it hard to e.g., write modules against this
> version of Mesos.
> 
> While launcher sealing depends on a recent kernel, the platform we
> build RPMs for already satisfies the requirements.
> 
> 
> Diffs
> -
> 
>   support/packaging/centos/mesos.spec 
> de905fda3366be5904a0aec8249abcc53b0af9a0 
> 
> 
> Diff: https://reviews.apache.org/r/70295/diff/1/
> 
> 
> Testing
> ---
> 
> `make check`
> 
> 
> Thanks,
> 
> Benjamin Bannier
> 
>

[GitHub] [mesos] asekretenko edited a comment on issue #329: Extending the LogSink interface to be able to pass microseconds (on top of glog 0.3.3).

[GitBox]

asekretenko edited a comment on issue #329: Extending the LogSink interface to 
be able to pass microseconds (on top of glog 0.3.3).
URL: https://github.com/apache/mesos/pull/329#issuecomment-477291056
 
 
   I've ported the glog patch from https://github.com/google/glog/pull/441 for 
glog  v0.3.3.


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


With regards,
Apache Git Services

[GitHub] [mesos] asekretenko commented on issue #329: Extending the LogSink interface to be able to pass microseconds (on top of glog 0.3.3).

[GitBox]

asekretenko commented on issue #329: Extending the LogSink interface to be able 
to pass microseconds (on top of glog 0.3.3).
URL: https://github.com/apache/mesos/pull/329#issuecomment-477291056
 
 
   https://github.com/google/glog/pull/441


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


With regards,
Apache Git Services

[GitHub] [mesos] asekretenko opened a new pull request #329: Extending the LogSink interface to be able to pass microseconds (on top of glog 0.3.3).

[GitBox]

asekretenko opened a new pull request #329: Extending the LogSink interface to 
be able to pass microseconds (on top of glog 0.3.3).
URL: https://github.com/apache/mesos/pull/329
 
 
   


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


With regards,
Apache Git Services

Re: Review Request 70117: Added unit tests for offer operation feedback metrics.

[Mesos Reviewbot]

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



Bad review!

Reviews applied: [70117, 70116, 70156, 70115]

Error:
2019-03-27 17:53:05 URL:https://reviews.apache.org/r/70116/diff/raw/ 
[18838/18838] -> "70116.patch" [1]
error: patch failed: src/master/master.cpp:8781
error: src/master/master.cpp: patch does not apply

- Mesos Reviewbot


On March 27, 2019, 4:41 p.m., Benno Evers wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/70117/
> ---
> 
> (Updated March 27, 2019, 4:41 p.m.)
> 
> 
> Review request for mesos, Gastón Kleiman, Greg Mann, and Joseph Wu.
> 
> 
> Bugs: MESOS-8241
> https://issues.apache.org/jira/browse/MESOS-8241
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This adds a set of checks to verify the metrics introduced
> in the previous commit are working as intended.
> 
> 
> Diffs
> -
> 
>   src/tests/agent_operation_feedback_tests.cpp 
> 5a8f54c7c53272e90ed5fa6366e8154cedf1375f 
>   src/tests/api_tests.cpp f241064dc8597972299a424958e759588f7e4fd2 
>   src/tests/master_slave_reconciliation_tests.cpp 
> 002be27bf0731e2dba8937697b347cd1dd0a 
>   src/tests/master_tests.cpp 5a926831734e6acf0388a63dac3ea3559b44a6a9 
>   src/tests/operation_reconciliation_tests.cpp 
> 8bd1dc02a74c4e6c1b97b25e73098c0b75f2d38e 
>   src/tests/persistent_volume_endpoints_tests.cpp 
> 40d7e6a30c9c11eb84f4bd5aca92cfcecb3e0eb7 
>   src/tests/reservation_endpoints_tests.cpp 
> b1897592797c40574de7995b2335f2b4bc5fc699 
>   src/tests/scheduler_tests.cpp 5fb696061248c877bfa86727f146051aee26cb58 
>   src/tests/slave_tests.cpp 5ee5609af0861e9aecf02a5eaefafe137bd9b843 
>   src/tests/storage_local_resource_provider_tests.cpp 
> 797f89e3545965e2cf2fd5ec0ecd78fc77a4ea87 
> 
> 
> Diff: https://reviews.apache.org/r/70117/diff/7/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Benno Evers
> 
>

Re: Review Request 70184: Fixed flakiness in 'RetryRpcWithExponentialBackoff'.

[Chun-Hung Hsiao]

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




src/tests/storage_local_resource_provider_tests.cpp
Lines 4926 (patched)


I'm not sure if this is the most appropriate fix before understanding why 
the offer was filtered. Can you elaborate more on why this is needed?


- Chun-Hung Hsiao


On March 11, 2019, 3:39 p.m., Jan Schlicht wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/70184/
> ---
> 
> (Updated March 11, 2019, 3:39 p.m.)
> 
> 
> Review request for mesos and Chun-Hung Hsiao.
> 
> 
> Bugs: MESOS-9594
> https://issues.apache.org/jira/browse/MESOS-9594
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Under some circumstances, offers would be filtered, resulting in the
> test being stuck while waiting for offers. This has been resolved by
> removing all offer filters before waiting for offers.
> 
> 
> Diffs
> -
> 
>   src/tests/storage_local_resource_provider_tests.cpp 
> 7945384867f26fa15dc734a235ae509d5d6d350f 
> 
> 
> Diff: https://reviews.apache.org/r/70184/diff/1/
> 
> 
> Testing
> ---
> 
> make check
> 
> I could not reproduce the flaky behavior of this test case, hence only assume 
> that this patch resolves the flakiness.
> 
> 
> Thanks,
> 
> Jan Schlicht
> 
>

Review Request 70325: Updated the master to allocate recovered orphan operation resources.

[Greg Mann]

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

Review request for mesos, Benjamin Mahler, Gastón Kleiman, Joseph Wu, and Meng 
Zhu.


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


Repository: mesos


Description
---

Updated the master to allocate recovered orphan operation resources.


Diffs
-

  src/master/master.cpp acc67d3763ddee9027e6cf375f1d495ff5805026 


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


Testing
---

`make check`


Thanks,

Greg Mann

Review Request 70324: Added a new allocator method and deprecated an existing one.

[Greg Mann]

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

Review request for mesos, Benjamin Mahler, Gastón Kleiman, Joseph Wu, and Meng 
Zhu.


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


Repository: mesos


Description
---

This patch deprecates the `addResourceProvider()` allocator method
in favor of a new one, `addAgentResources()`, with exactly the same
logic. The old method is left in place, but its logic is completely
removed.


Diffs
-

  include/mesos/allocator/allocator.hpp 
f1868387b28ea1d303cd3ed0176e32d66debd4c1 
  src/master/allocator/mesos/allocator.hpp 
2d83f382eed9d9dca218adf84bc03883f7486349 
  src/master/allocator/mesos/hierarchical.hpp 
36bf90baf413e99c1580d516dfac0f074335d322 
  src/master/allocator/mesos/hierarchical.cpp 
8bc749903b8ceb09a02e260919377483479302b5 
  src/master/master.cpp acc67d3763ddee9027e6cf375f1d495ff5805026 
  src/tests/allocator.hpp 0718bef4de39a82dbcadebb9967d2ff1cceb1cae 
  src/tests/hierarchical_allocator_tests.cpp 
a34fa96fbc455d830c1fd7c1df83f3db72c96ee3 


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


Testing
---

`make check`


Thanks,

Greg Mann

Re: Review Request 70117: Added unit tests for offer operation feedback metrics.

[Mesos Reviewbot Windows]

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



FAIL: Failed to apply the dependent review: 70116.

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

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

Relevant logs:

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

```
error: patch failed: src/master/master.cpp:8781
error: src/master/master.cpp: patch does not apply
```

- Mesos Reviewbot Windows


On March 27, 2019, 4:41 p.m., Benno Evers wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/70117/
> ---
> 
> (Updated March 27, 2019, 4:41 p.m.)
> 
> 
> Review request for mesos, Gastón Kleiman, Greg Mann, and Joseph Wu.
> 
> 
> Bugs: MESOS-8241
> https://issues.apache.org/jira/browse/MESOS-8241
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This adds a set of checks to verify the metrics introduced
> in the previous commit are working as intended.
> 
> 
> Diffs
> -
> 
>   src/tests/agent_operation_feedback_tests.cpp 
> 5a8f54c7c53272e90ed5fa6366e8154cedf1375f 
>   src/tests/api_tests.cpp f241064dc8597972299a424958e759588f7e4fd2 
>   src/tests/master_slave_reconciliation_tests.cpp 
> 002be27bf0731e2dba8937697b347cd1dd0a 
>   src/tests/master_tests.cpp 5a926831734e6acf0388a63dac3ea3559b44a6a9 
>   src/tests/operation_reconciliation_tests.cpp 
> 8bd1dc02a74c4e6c1b97b25e73098c0b75f2d38e 
>   src/tests/persistent_volume_endpoints_tests.cpp 
> 40d7e6a30c9c11eb84f4bd5aca92cfcecb3e0eb7 
>   src/tests/reservation_endpoints_tests.cpp 
> b1897592797c40574de7995b2335f2b4bc5fc699 
>   src/tests/scheduler_tests.cpp 5fb696061248c877bfa86727f146051aee26cb58 
>   src/tests/slave_tests.cpp 5ee5609af0861e9aecf02a5eaefafe137bd9b843 
>   src/tests/storage_local_resource_provider_tests.cpp 
> 797f89e3545965e2cf2fd5ec0ecd78fc77a4ea87 
> 
> 
> Diff: https://reviews.apache.org/r/70117/diff/7/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Benno Evers
> 
>

Re: Review Request 70117: Added unit tests for offer operation feedback metrics.

[Benno Evers]

> On March 15, 2019, 12:01 a.m., Greg Mann wrote:
> > Could you also update a test to verify that metrics transition correctly? 
> > i.e., that operation metrics will start at `pending == 0` and `finished == 
> > 0`, then transition to `pending == 1` and `finished == 0`, then finally to 
> > `pending == 0` and `finished == 1`? Would be nice to do this for both the 
> > global and per-operation metrics.

Good point, I've added this check to 
`MasterTests.OperationUpdateDuringFailover`.

As an interesting aside, this was the *only* test in our whole test suite that 
I could find where an operation transitions out of the `PENDING` state, maybe 
that indicates we're missing some test to verify the basic operation lifecycle.


- Benno


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


On March 27, 2019, 4:41 p.m., Benno Evers wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/70117/
> ---
> 
> (Updated March 27, 2019, 4:41 p.m.)
> 
> 
> Review request for mesos, Gastón Kleiman, Greg Mann, and Joseph Wu.
> 
> 
> Bugs: MESOS-8241
> https://issues.apache.org/jira/browse/MESOS-8241
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This adds a set of checks to verify the metrics introduced
> in the previous commit are working as intended.
> 
> 
> Diffs
> -
> 
>   src/tests/agent_operation_feedback_tests.cpp 
> 5a8f54c7c53272e90ed5fa6366e8154cedf1375f 
>   src/tests/api_tests.cpp f241064dc8597972299a424958e759588f7e4fd2 
>   src/tests/master_slave_reconciliation_tests.cpp 
> 002be27bf0731e2dba8937697b347cd1dd0a 
>   src/tests/master_tests.cpp 5a926831734e6acf0388a63dac3ea3559b44a6a9 
>   src/tests/operation_reconciliation_tests.cpp 
> 8bd1dc02a74c4e6c1b97b25e73098c0b75f2d38e 
>   src/tests/persistent_volume_endpoints_tests.cpp 
> 40d7e6a30c9c11eb84f4bd5aca92cfcecb3e0eb7 
>   src/tests/reservation_endpoints_tests.cpp 
> b1897592797c40574de7995b2335f2b4bc5fc699 
>   src/tests/scheduler_tests.cpp 5fb696061248c877bfa86727f146051aee26cb58 
>   src/tests/slave_tests.cpp 5ee5609af0861e9aecf02a5eaefafe137bd9b843 
>   src/tests/storage_local_resource_provider_tests.cpp 
> 797f89e3545965e2cf2fd5ec0ecd78fc77a4ea87 
> 
> 
> Diff: https://reviews.apache.org/r/70117/diff/7/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Benno Evers
> 
>

Re: Review Request 70117: Added unit tests for offer operation feedback metrics.

[Benno Evers]

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

(Updated March 27, 2019, 4:41 p.m.)


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


Changes
---

Verify that 'pending' is correctly decreased; remove newline.


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


Repository: mesos


Description
---

This adds a set of checks to verify the metrics introduced
in the previous commit are working as intended.


Diffs (updated)
-

  src/tests/agent_operation_feedback_tests.cpp 
5a8f54c7c53272e90ed5fa6366e8154cedf1375f 
  src/tests/api_tests.cpp f241064dc8597972299a424958e759588f7e4fd2 
  src/tests/master_slave_reconciliation_tests.cpp 
002be27bf0731e2dba8937697b347cd1dd0a 
  src/tests/master_tests.cpp 5a926831734e6acf0388a63dac3ea3559b44a6a9 
  src/tests/operation_reconciliation_tests.cpp 
8bd1dc02a74c4e6c1b97b25e73098c0b75f2d38e 
  src/tests/persistent_volume_endpoints_tests.cpp 
40d7e6a30c9c11eb84f4bd5aca92cfcecb3e0eb7 
  src/tests/reservation_endpoints_tests.cpp 
b1897592797c40574de7995b2335f2b4bc5fc699 
  src/tests/scheduler_tests.cpp 5fb696061248c877bfa86727f146051aee26cb58 
  src/tests/slave_tests.cpp 5ee5609af0861e9aecf02a5eaefafe137bd9b843 
  src/tests/storage_local_resource_provider_tests.cpp 
797f89e3545965e2cf2fd5ec0ecd78fc77a4ea87 


Diff: https://reviews.apache.org/r/70117/diff/7/

Changes: https://reviews.apache.org/r/70117/diff/6-7/


Testing
---


Thanks,

Benno Evers

Re: Review Request 70116: Added metrics for offer operation feedback.

[Benno Evers]

> On March 15, 2019, 12:03 a.m., Greg Mann wrote:
> > src/master/metrics.hpp
> > Lines 82-84 (patched)
> > 
> >
> > Hmm am I missing something, or are there no per-framework metrics 
> > added? I'm fine with not adding them, but this comment seems to be 
> > incorrect?
> 
> Benno Evers wrote:
> They're not added in this review because they already exist :) See line 
> 816-836 inside `FrameworkMetrics::FrameworkMetrics()`.
> 
> Greg Mann wrote:
> Those metrics are slightly different - they provide counters for the 
> offer operations submitted by a particular framework, but don't track the 
> operation states. It might be worth mentioning in this comment that offer 
> operation counts are tracked on a per-framework basis.

Oh, right. Updated the comment.


- Benno


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


On March 27, 2019, 4:39 p.m., Benno Evers wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/70116/
> ---
> 
> (Updated March 27, 2019, 4:39 p.m.)
> 
> 
> Review request for mesos, Gastón Kleiman, Greg Mann, and Joseph Wu.
> 
> 
> Bugs: MESOS-8241
> https://issues.apache.org/jira/browse/MESOS-8241
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This commit adds additional metrics counting the
> number of operations in each state.
> 
> Unit tests are added in the subsequent commit.
> 
> 
> Diffs
> -
> 
>   docs/monitoring.md 54b872f579dbc68ca5f67f4cc1ba34065a09aee2 
>   src/master/master.cpp 9c4a9e83da94535873d72c902835f229c4f96320 
>   src/master/metrics.hpp 4495e65b6bb11f7236335a702c4f61e7c3f9b0aa 
>   src/master/metrics.cpp 4dd73fb18a06ce8f75c4c1435dba84ade123bee9 
> 
> 
> Diff: https://reviews.apache.org/r/70116/diff/6/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Benno Evers
> 
>

Re: Review Request 70116: Added metrics for offer operation feedback.

[Benno Evers]

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

(Updated March 27, 2019, 4:39 p.m.)


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


Changes
---

Clarified comment; clearly distinguished between gauges and counters in 
documentation.


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


Repository: mesos


Description
---

This commit adds additional metrics counting the
number of operations in each state.

Unit tests are added in the subsequent commit.


Diffs (updated)
-

  docs/monitoring.md 54b872f579dbc68ca5f67f4cc1ba34065a09aee2 
  src/master/master.cpp 9c4a9e83da94535873d72c902835f229c4f96320 
  src/master/metrics.hpp 4495e65b6bb11f7236335a702c4f61e7c3f9b0aa 
  src/master/metrics.cpp 4dd73fb18a06ce8f75c4c1435dba84ade123bee9 


Diff: https://reviews.apache.org/r/70116/diff/6/

Changes: https://reviews.apache.org/r/70116/diff/5-6/


Testing
---


Thanks,

Benno Evers

Re: Review Request 70323: Bumped nokogiri and rack site dependencies.

[Till Toenshoff via Review Board]

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


Ship it!




Ship It!

- Till Toenshoff


On March 27, 2019, 4:21 p.m., Benjamin Bannier wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/70323/
> ---
> 
> (Updated March 27, 2019, 4:21 p.m.)
> 
> 
> Review request for mesos, Benjamin Mahler and Till Toenshoff.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This bumps nokogiri to >=1.8.5 for CVE-2018-16471 and rack to
> >=1.6.11 for CVE-2018-16471.
> 
> 
> Diffs
> -
> 
>   site/Gemfile.lock b3f364945b7dea7cd4696cbb7e956c3ed7bfb59a 
> 
> 
> Diff: https://reviews.apache.org/r/70323/diff/1/
> 
> 
> Testing
> ---
> 
> `bundle install && bundle exec middleman`
> 
> 
> Thanks,
> 
> Benjamin Bannier
> 
>

Re: Review Request 70323: Bumped nokogiri and rack site dependencies.

[Benno Evers]

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


Ship it!




- Benno Evers


On March 27, 2019, 4:21 p.m., Benjamin Bannier wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/70323/
> ---
> 
> (Updated March 27, 2019, 4:21 p.m.)
> 
> 
> Review request for mesos, Benjamin Mahler and Till Toenshoff.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This bumps nokogiri to >=1.8.5 for CVE-2018-16471 and rack to
> >=1.6.11 for CVE-2018-16471.
> 
> 
> Diffs
> -
> 
>   site/Gemfile.lock b3f364945b7dea7cd4696cbb7e956c3ed7bfb59a 
> 
> 
> Diff: https://reviews.apache.org/r/70323/diff/1/
> 
> 
> Testing
> ---
> 
> `bundle install && bundle exec middleman`
> 
> 
> Thanks,
> 
> Benjamin Bannier
> 
>

Re: Review Request 70116: Added metrics for offer operation feedback.

[Greg Mann]

> On March 15, 2019, 12:03 a.m., Greg Mann wrote:
> > src/master/metrics.hpp
> > Lines 82-84 (patched)
> > 
> >
> > Hmm am I missing something, or are there no per-framework metrics 
> > added? I'm fine with not adding them, but this comment seems to be 
> > incorrect?
> 
> Benno Evers wrote:
> They're not added in this review because they already exist :) See line 
> 816-836 inside `FrameworkMetrics::FrameworkMetrics()`.

Those metrics are slightly different - they provide counters for the offer 
operations submitted by a particular framework, but don't track the operation 
states. It might be worth mentioning in this comment that offer operation 
counts are tracked on a per-framework basis.


- Greg


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


On March 26, 2019, 5:56 p.m., Benno Evers wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/70116/
> ---
> 
> (Updated March 26, 2019, 5:56 p.m.)
> 
> 
> Review request for mesos, Gastón Kleiman, Greg Mann, and Joseph Wu.
> 
> 
> Bugs: MESOS-8241
> https://issues.apache.org/jira/browse/MESOS-8241
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This commit adds additional metrics counting the
> number of operations in each state.
> 
> Unit tests are added in the subsequent commit.
> 
> 
> Diffs
> -
> 
>   docs/monitoring.md 54b872f579dbc68ca5f67f4cc1ba34065a09aee2 
>   src/master/master.cpp 9c4a9e83da94535873d72c902835f229c4f96320 
>   src/master/metrics.hpp 4495e65b6bb11f7236335a702c4f61e7c3f9b0aa 
>   src/master/metrics.cpp 4dd73fb18a06ce8f75c4c1435dba84ade123bee9 
> 
> 
> Diff: https://reviews.apache.org/r/70116/diff/5/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Benno Evers
> 
>

Review Request 70323: Bumped nokogiri and rack site dependencies.

[Benjamin Bannier]

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

Review request for mesos, Benjamin Mahler and Till Toenshoff.


Repository: mesos


Description
---

This bumps nokogiri to >=1.8.5 for CVE-2018-16471 and rack to
>=1.6.11 for CVE-2018-16471.


Diffs
-

  site/Gemfile.lock b3f364945b7dea7cd4696cbb7e956c3ed7bfb59a 


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


Testing
---

`bundle install && bundle exec middleman`


Thanks,

Benjamin Bannier

Re: Review Request 70321: Cleaned up `mesos::csi::v0::Client` interface.

[Mesos Reviewbot Windows]

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



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

Reviews applied: `['70313', '70314', '70315', '70316', '70245', '70168', 
'70247', '70248', '70213', '70214', '70216', '70215', '70217', '70284', 
'70222', '70285', '70223', '70225', '70302', '70303', '70321']`

Failed command: `Start-MesosCITesting`

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

Relevant logs:

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

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

Re: Review Request 70302: Adjusted CSI v0 proto compilation.

[Chun-Hung Hsiao]

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

(Updated March 27, 2019, 3:27 p.m.)


Review request for mesos, Benjamin Bannier and Joseph Wu.


Changes
---

Rebased.


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


Repository: mesos


Description
---

This patch makes the following adjustments so we can build CSI v0 and v1
in the future:

* Standardize placements for third-party proto files: `csi.proto` from
  CSI v0 will be in `build/include/csi/v0/` and that from v0 will be in
  `build/include/csi/v1` in the future. Dependent proto files should
  import `csi/v0/csi.proto` or `csi/v1/csi.proto`.

* Moved `include/csi/spec.hpp` to `include/mesos/csi/v0.hpp`. In the
  future, CSI v1 proto headers will be in `include/mesos/csi/v1.hpp`.
  Since we don't insteall CSI headers, for the installed `v0.hpp` and
  `v1.hpp` headers to work, users must ensure that the CSI headers can
  be found through `csi/v0/csi*.pb.h` and `csi/v1/csi*.pb.h`.


Diffs (updated)
-

  3rdparty/CMakeLists.txt 1999dd20964da96bc5acfbd47cb80d4ca6f734b9 
  3rdparty/Makefile.am 98a2623b34132885279bea6135c0da4ffc8c59cf 
  3rdparty/cmake/Versions.cmake 972c706a18bfce45af6c2632b326606052888b02 
  3rdparty/versions.am 24381073f4c77d92c5ba62f54bb2194bf041ee6c 
  include/csi/spec.hpp 19d9445fe1da7be6e41b484b5a78dcd10e5ece52 
  src/CMakeLists.txt 3397c3b1d4e8a7900b2e5f870679cc7aa30b4be2 
  src/Makefile.am bcafe48b2105575371464a29783bc6f3f1c2cf8d 
  src/cmake/MesosProtobuf.cmake 09074d77a1df5076c6e51292523ce994a77a88f2 
  src/csi/client.hpp c2583cf4c0d2abc38d65ddc801cae3696a8080a9 
  src/csi/rpc.hpp b2502ceb319638038b4d151965f6226db675f96b 
  src/csi/utils.hpp 9145c6795c3ecdde5de5859a852763fe9aeb1ddf 
  src/examples/test_csi_plugin.cpp 73a6c43e72afec0dd124b0fe2f8ef0e45acb307f 
  src/resource_provider/storage/disk_profile.proto 
1c97e9c62fb3154d6048d0e55352a1276adcbca8 
  src/resource_provider/storage/disk_profile_utils.hpp 
8a83a15ba555ce66bbb86b8df72178bce17a615a 
  src/tests/csi_utils_tests.cpp PRE-CREATION 
  src/tests/disk_profile_adaptor_tests.cpp 
0ccbc79d7ffb82a68b7ed5aeab930bcd8e6e770e 
  src/tests/mock_csi_plugin.hpp 6897fbc878f1e2f5b9e9c402b09358c187af79a0 


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

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


Testing
---

make check


Thanks,

Chun-Hung Hsiao

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

[Chun-Hung Hsiao]

> On March 27, 2019, 12:01 p.m., James DeFelice wrote:
> > src/csi/state.proto
> > Line 19 (original), 19 (patched)
> > 
> >
> > are the protos in src/csi considered part of the "stable" mesos v1 API? 
> > if so, should they follow the same directory naming convention as the other 
> > mesos v1 APIs?

No. Protos in `src/` are for internal use. Specically, this one is for 
checkpointing. It needs to be "stable" in the backward-compatible way, but 
never exposed in any API. This is the pattern we use in may other places: 
public-facing in `include`, internal use in `src`. Dropping.


- Chun-Hung


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


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

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

[Benjamin Bannier]

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


Ship it!




Looks good. Any testing you'd like to document?

- Benjamin Bannier


On March 22, 2019, 5:57 p.m., Benno Evers wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/70281/
> ---
> 
> (Updated March 22, 2019, 5:57 p.m.)
> 
> 
> Review request for mesos, Gastón Kleiman, Greg Mann, and Joseph Wu.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This commit adds a new overload
> 
> ostream& operator<<(ostream&, const OperationStatus&);
> 
> to make the printing of OperationStatus messages
> more convenient.
> 
> 
> Diffs
> -
> 
>   include/mesos/v1/mesos.hpp dc328bf45d8b69dcf096785b98031513b7ea9099 
>   src/v1/mesos.cpp 704ad7697689d9cb21d6ed2675ac6c0044ce18c8 
> 
> 
> Diff: https://reviews.apache.org/r/70281/diff/1/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Benno Evers
> 
>

Re: Review Request 70116: Added metrics for offer operation feedback.

[Benno Evers]

> On March 15, 2019, 12:03 a.m., Greg Mann wrote:
> > src/master/metrics.hpp
> > Lines 82-84 (patched)
> > 
> >
> > Hmm am I missing something, or are there no per-framework metrics 
> > added? I'm fine with not adding them, but this comment seems to be 
> > incorrect?

They're not added in this review because they already exist :) See line 816-836 
inside `FrameworkMetrics::FrameworkMetrics()`.


- Benno


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


On March 26, 2019, 5:56 p.m., Benno Evers wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/70116/
> ---
> 
> (Updated March 26, 2019, 5:56 p.m.)
> 
> 
> Review request for mesos, Gastón Kleiman, Greg Mann, and Joseph Wu.
> 
> 
> Bugs: MESOS-8241
> https://issues.apache.org/jira/browse/MESOS-8241
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This commit adds additional metrics counting the
> number of operations in each state.
> 
> Unit tests are added in the subsequent commit.
> 
> 
> Diffs
> -
> 
>   docs/monitoring.md 54b872f579dbc68ca5f67f4cc1ba34065a09aee2 
>   src/master/master.cpp 9c4a9e83da94535873d72c902835f229c4f96320 
>   src/master/metrics.hpp 4495e65b6bb11f7236335a702c4f61e7c3f9b0aa 
>   src/master/metrics.cpp 4dd73fb18a06ce8f75c4c1435dba84ade123bee9 
> 
> 
> Diff: https://reviews.apache.org/r/70116/diff/5/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Benno Evers
> 
>

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

[Greg Mann]

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




src/examples/operation_feedback_framework.cpp
Lines 16 (patched)


Ah, sorry, gave a ship-it too soon :) How about adding a shell script for 
this example framework like we have for the others, in 'src/tests'? This will 
serve as a unit test that the example framework code is sane, as well as 
additional testing for the operation feedback feature.


- Greg Mann


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

Re: Review Request 70319: Added a `createQuantitiesMap` method in `ResourceQuantities`.

[Benjamin Mahler]

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



See my comment in the next review about avoiding this

- Benjamin Mahler


On March 27, 2019, 5:21 a.m., Meng Zhu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/70319/
> ---
> 
> (Updated March 27, 2019, 5:21 a.m.)
> 
> 
> Review request for mesos and Benjamin Mahler.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> The method returns a `hashmap` of name and quantity pairs.
> This is used where callers want to have their own copy of the
> quantities and apply direct mutation.
> 
> 
> Diffs
> -
> 
>   src/common/resource_quantities.hpp db4f5afb9f617e27bd08756020b4bcf0bd92dc66 
>   src/common/resource_quantities.cpp 9ae5d9f4aefde81cde7463a7ce118c65a68fc0db 
> 
> 
> Diff: https://reviews.apache.org/r/70319/diff/1/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Meng Zhu
> 
>

Re: Review Request 70318: Added `empty()` method in `class ResourceQuantities`.

[Benjamin Mahler]

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


Ship it!




Ship It!

- Benjamin Mahler


On March 27, 2019, 5:21 a.m., Meng Zhu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/70318/
> ---
> 
> (Updated March 27, 2019, 5:21 a.m.)
> 
> 
> Review request for mesos and Benjamin Mahler.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added `empty()` method in `class ResourceQuantities`.
> 
> 
> Diffs
> -
> 
>   src/common/resource_quantities.hpp db4f5afb9f617e27bd08756020b4bcf0bd92dc66 
> 
> 
> Diff: https://reviews.apache.org/r/70318/diff/1/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Meng Zhu
> 
>

Re: Review Request 70320: Used `ResourceQuantities` in `__allocate()` when possible.

[Benjamin Mahler]

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



Looks good, just noticed another headroom variable that seems to warrant 
`ResourceQuantities`. Also made a suggestion to avoid the createQuantitiesMap 
helper.

I saw the suggestion of posting benchmark results in subsequent patches, when 
are those patches going to get posted with the results? If not so soon, then we 
may just want to post the benchmark results in this patch so that we can ship 
it on its own sooner?


src/master/allocator/mesos/hierarchical.cpp
Line 1714 (original), 1713 (patched)


s/Let sorter/Update sorter to/



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


s/Let sorter/Update sorter to/



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


s/Let sorter/Update sorter to/



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


Rather than adding this additional function, can't `shrinkResoures` take a 
`ResourceQuantities`?



src/master/allocator/mesos/hierarchical.cpp
Lines 1956-1957 (original), 1949-1950 (patched)


Clearer name, thanks!



src/master/allocator/mesos/hierarchical.cpp
Line 1962 (original), 1952 (patched)


in the headroom surplus,



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


Ditto here for just passing `ResourceQuantities`



src/master/allocator/mesos/hierarchical.cpp
Lines 2088-2091 (original), 2079-2082 (patched)


This looks like a `ResourceQuantities`?


- Benjamin Mahler


On March 27, 2019, 5:31 a.m., Meng Zhu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/70320/
> ---
> 
> (Updated March 27, 2019, 5:31 a.m.)
> 
> 
> Review request for mesos and Benjamin Mahler.
> 
> 
> Bugs: MESOS-9504
> https://issues.apache.org/jira/browse/MESOS-9504
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Replaced `Resources` with `ResourceQuantities` when possible
> in `__allocate()`. This simplifies the code and improves
> performance.
> 
> 
> Diffs
> -
> 
>   src/master/allocator/mesos/hierarchical.hpp 
> 36bf90baf413e99c1580d516dfac0f074335d322 
>   src/master/allocator/mesos/hierarchical.cpp 
> 8bc749903b8ceb09a02e260919377483479302b5 
> 
> 
> Diff: https://reviews.apache.org/r/70320/diff/1/
> 
> 
> Testing
> ---
> 
> make check
> Benchmark results coming in subsequent patches.
> 
> 
> Thanks,
> 
> Meng Zhu
> 
>

Re: Review Request 70295: Enabled launcher sealing for RPM packages.

[Benjamin Bannier]

> On March 26, 2019, 2:21 a.m., Joseph Wu wrote:
> > support/packaging/centos/mesos.spec
> > Lines 94 (patched)
> > 
> >
> > I'd expect the `--disable-libtool-wrappers` flag too, but do rpm builds 
> > omit libtool wrappers anyway? 
> > 
> > It's not clear to me when exactly the wrappers are generated and when 
> > they aren't

The libtool wrappers (which set up the env to consume dynamic libraries built 
as part of the build) are generated so executables can be run from the build 
tree, without installing. They are never installed. OTOH, executables built in 
autotools projects without libtool wrappers cannot be installed as they are not 
correctly linked (they'd depend on files from the build tree, not from the 
install prefix).

Dropping.


- Benjamin


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


On March 25, 2019, 4:09 p.m., Benjamin Bannier wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/70295/
> ---
> 
> (Updated March 25, 2019, 4:09 p.m.)
> 
> 
> Review request for mesos, Gilbert Song, Joseph Wu, and Till Toenshoff.
> 
> 
> Bugs: MESOS-9677
> https://issues.apache.org/jira/browse/MESOS-9677
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> We enable this flag since with it disabled certain public functions
> are not available making it hard to e.g., write modules against this
> version of Mesos.
> 
> While launcher sealing depends on a recent kernel, the platform we
> build RPMs for already satisfies the requirements.
> 
> 
> Diffs
> -
> 
>   support/packaging/centos/mesos.spec 
> de905fda3366be5904a0aec8249abcc53b0af9a0 
> 
> 
> Diff: https://reviews.apache.org/r/70295/diff/1/
> 
> 
> Testing
> ---
> 
> `make check`
> 
> 
> Thanks,
> 
> Benjamin Bannier
> 
>

Re: Review Request 70295: Enabled launcher sealing for RPM packages.

[James DeFelice]

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




support/packaging/centos/mesos.spec
Lines 94 (patched)


we probably want to conditionally enable this depending on the centos 
version. for example I don't think el6 ships w/ the necessary kernel support 
for launcher-sealing, but I think that el7 does.

see https://fedoraproject.org/wiki/Packaging:DistTag


- James DeFelice


On March 25, 2019, 3:09 p.m., Benjamin Bannier wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/70295/
> ---
> 
> (Updated March 25, 2019, 3:09 p.m.)
> 
> 
> Review request for mesos, Gilbert Song, Joseph Wu, and Till Toenshoff.
> 
> 
> Bugs: MESOS-9677
> https://issues.apache.org/jira/browse/MESOS-9677
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> We enable this flag since with it disabled certain public functions
> are not available making it hard to e.g., write modules against this
> version of Mesos.
> 
> While launcher sealing depends on a recent kernel, the platform we
> build RPMs for already satisfies the requirements.
> 
> 
> Diffs
> -
> 
>   support/packaging/centos/mesos.spec 
> de905fda3366be5904a0aec8249abcc53b0af9a0 
> 
> 
> Diff: https://reviews.apache.org/r/70295/diff/1/
> 
> 
> Testing
> ---
> 
> `make check`
> 
> 
> Thanks,
> 
> Benjamin Bannier
> 
>

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

[James DeFelice]

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




src/csi/state.proto
Line 19 (original), 19 (patched)


are the protos in src/csi considered part of the "stable" mesos v1 API? if 
so, should they follow the same directory naming convention as the other mesos 
v1 APIs?


- James DeFelice


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

Re: Review Request 70316: Updated test `ImportPreprovisionedVolume` for better code coverage.

[Benjamin Bannier]

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


Fix it, then Ship it!





src/tests/storage_local_resource_provider_tests.cpp
Lines 2922-2923 (patched)


`s/reregistration/registration/`?



src/tests/storage_local_resource_provider_tests.cpp
Lines 2931 (patched)


Is this related to above comment block?



src/tests/storage_local_resource_provider_tests.cpp
Lines 3028 (patched)


Add a comment here documenting what triggered this allocation.



src/tests/storage_local_resource_provider_tests.cpp
Lines 3050 (patched)


Let's add an assertion that the range resulting from the filter is actually 
non-empty.



src/tests/storage_local_resource_provider_tests.cpp
Line 3007 (original), 3054 (patched)


Ditto.



src/tests/storage_local_resource_provider_tests.cpp
Line 3008 (original), 3071 (patched)


Document what triggered the allocation.


- Benjamin Bannier


On March 27, 2019, 4:45 a.m., Chun-Hung Hsiao wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/70316/
> ---
> 
> (Updated March 27, 2019, 4:45 a.m.)
> 
> 
> Review request for mesos, Benjamin Bannier, Jie Yu, and Jan Schlicht.
> 
> 
> Bugs: MESOS-9540
> https://issues.apache.org/jira/browse/MESOS-9540
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This patch renames `ImportPreprovisionedVolume` to
> `CreateDestroyPreprovisionedVolume` add tests two additional scenarios:
> 
> * `CREATE_DISK` with an unknown profile.
> 
> * `DESTROY_DISK` with a `RAW` disk.
> 
> 
> Diffs
> -
> 
>   src/tests/storage_local_resource_provider_tests.cpp 
> 797f89e3545965e2cf2fd5ec0ecd78fc77a4ea87 
> 
> 
> Diff: https://reviews.apache.org/r/70316/diff/2/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Chun-Hung Hsiao
> 
>

Re: Review Request 70315: Added authorization for applying `DESTROY_DISK` on `RAW` disks.

[Benjamin Bannier]

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


Fix it, then Ship it!





include/mesos/authorizer/acls.proto
Line 1 (original), 1 (patched)


Looks like we neglected updating `docs/examples/acls_template.json` 
previously, could you fix that?



src/tests/authorization_tests.cpp
Lines 7052-7064 (patched)


Once should be enough?


- Benjamin Bannier


On March 27, 2019, 3:09 a.m., Chun-Hung Hsiao wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/70315/
> ---
> 
> (Updated March 27, 2019, 3:09 a.m.)
> 
> 
> Review request for mesos, Benjamin Bannier, Jie Yu, and Jan Schlicht.
> 
> 
> Bugs: MESOS-9540
> https://issues.apache.org/jira/browse/MESOS-9540
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added authorization for applying `DESTROY_DISK` on `RAW` disks.
> 
> 
> Diffs
> -
> 
>   include/mesos/authorizer/acls.proto 
> 4c3f2907a25c2920a0f382e3937898774bbf49fd 
>   include/mesos/authorizer/authorizer.proto 
> f9060531cf1f6bc60786e6d6e6b87310f1bc0927 
>   src/authorizer/local/authorizer.cpp 
> 85e18b958932fca74f7860bb19b178835a1636f9 
>   src/master/master.cpp b9db4ffd4ee8ea4a8e44a35d1afb6c1b8e03d74d 
>   src/tests/authorization_tests.cpp e85cdb681ae2d1a9f215ce9d07a56e85346e3dab 
> 
> 
> Diff: https://reviews.apache.org/r/70315/diff/1/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Chun-Hung Hsiao
> 
>

Re: Review Request 70314: Supported destroying preprovisioned CSI volumes in SLRP.

[Benjamin Bannier]

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


Fix it, then Ship it!





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


This patch should contain a test of introduced behavior.



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


Add a comment here documenting what this branch models.



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


All this local checking of capabilities at call sites instead of in `call` 
is getting cumbersome and error-prone. Would be great to clean that up 
eventually by moving it to a single place.



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


Great, should have been here all along.



src/resource_provider/storage/provider.cpp
Lines 3202-3203 (original), 3194-3196 (patched)


Let's use a `switch` now that we have three cases here.



src/resource_provider/storage/provider.cpp
Lines 3237-3239 (original), 3235-3237 (patched)


No quotes around `resource`; let's also fix the wrapping so that related 
entities are on the same line (`"resource provider"` and `info.id()`; 
`"resource"` and `resource`; opening and closing quotes).
```
LOG(INFO)
  << "Reconciling storage pools for resource provider " << info.id()
  << " after resource '" << resource << "' has been freed";


- Benjamin Bannier


On March 27, 2019, 4:45 a.m., Chun-Hung Hsiao wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/70314/
> ---
> 
> (Updated March 27, 2019, 4:45 a.m.)
> 
> 
> Review request for mesos, Benjamin Bannier and Jie Yu.
> 
> 
> Bugs: MESOS-9540
> https://issues.apache.org/jira/browse/MESOS-9540
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> SLRP now accepts `DESTROY_DISK` on `RAW` disk resources with source IDs.
> If the backed CSI plugin does have the `CREATE_DELETE_VOLUME` controller
> capability, this operation will be a no-op; otherwise the underlying CSI
> volume will be deprovisioned.
> 
> 
> Diffs
> -
> 
>   src/resource_provider/storage/provider.cpp 
> 2711503cdb58cb9b34af8c9fad0908c5f788a2af 
> 
> 
> Diff: https://reviews.apache.org/r/70314/diff/2/
> 
> 
> Testing
> ---
> 
> make check
> 
> The new code path is tested later in this chain.
> 
> 
> Thanks,
> 
> Chun-Hung Hsiao
> 
>

Re: Review Request 70321: Cleaned up `mesos::csi::v0::Client` interface.

[Mesos Reviewbot]

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



Bad review!

Reviews applied: [70321, 70303, 70302, 70248, 70247, 70168, 70245, 70316, 
70315, 70314, 70313]

Error:
2019-03-27 09:49:10 URL:https://reviews.apache.org/r/70302/diff/raw/ 
[25366/25366] -> "70302.patch" [1]
error: patch failed: src/Makefile.am:1582
error: src/Makefile.am: patch does not apply

- Mesos Reviewbot


On March 26, 2019, 11:09 p.m., Chun-Hung Hsiao wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/70321/
> ---
> 
> (Updated March 26, 2019, 11:09 p.m.)
> 
> 
> Review request for mesos, Benjamin Bannier, Jie Yu, and Jan Schlicht.
> 
> 
> Bugs: MESOS-9624
> https://issues.apache.org/jira/browse/MESOS-9624
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Since per-CSI-call metrics are no longer implemented, there is very less
> value to keep the `mesos::csi::v0::RPC` enum, which is tightly coupled
> with `mesos::csi::v0::Client`. Therefore, the enum and its header are
> removed.
> 
> The header and implementation file for `mesos::csi::v0::Client` is also
> renamed for future CSI v1 support.
> 
> 
> Diffs
> -
> 
>   src/CMakeLists.txt 3397c3b1d4e8a7900b2e5f870679cc7aa30b4be2 
>   src/Makefile.am bcafe48b2105575371464a29783bc6f3f1c2cf8d 
>   src/csi/client.hpp c2583cf4c0d2abc38d65ddc801cae3696a8080a9 
>   src/csi/client.cpp 9e17f5bcb902470045dac704f2471d2962336e48 
>   src/csi/rpc.hpp b2502ceb319638038b4d151965f6226db675f96b 
>   src/csi/rpc.cpp e8a2770a6d8ae48a717ed93fa872e9edee45e618 
>   src/csi/service_manager.cpp PRE-CREATION 
>   src/csi/v0_client.hpp PRE-CREATION 
>   src/csi/v0_volume_manager.cpp PRE-CREATION 
>   src/csi/v0_volume_manager_process.hpp PRE-CREATION 
>   src/tests/csi_client_tests.cpp c8f3f04e6bff40d62dd0f85fcc146d71fa9e5d34 
>   src/tests/storage_local_resource_provider_tests.cpp 
> 797f89e3545965e2cf2fd5ec0ecd78fc77a4ea87 
> 
> 
> Diff: https://reviews.apache.org/r/70321/diff/1/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Chun-Hung Hsiao
> 
>

Re: Review Request 70313: API changes for supporting destroying `RAW` disks.

[Benjamin Bannier]

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


Ship it!




Ship It!

- Benjamin Bannier


On March 27, 2019, 3:38 a.m., Chun-Hung Hsiao wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/70313/
> ---
> 
> (Updated March 27, 2019, 3:38 a.m.)
> 
> 
> Review request for mesos, Benjamin Bannier, James DeFelice, and Jie Yu.
> 
> 
> Bugs: MESOS-9540
> https://issues.apache.org/jira/browse/MESOS-9540
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This patch enables a framework to apply `DESTROY_DISK` on a `RAW` disk
> backed by a preprovisioned CSI volume. This API improvement brings the
> following benefits:
> 
> * If a `CREATE_DISK` failed because of, e.g., agent gone, but the actual
>   CSI call was finished, `DESTROY_DISK` can be used to reclaim the
>   "leaked" volume.
> 
> * If there is an agent ID change, `DESTROY_DISK` can be used to clean up
>   volumes created by the previous agent's SLRP.
> 
> 
> Diffs
> -
> 
>   include/mesos/mesos.proto 35e10f19fcaae13aedd6f0d5429442f74b5ebfbe 
>   include/mesos/v1/mesos.proto 1d751f659cb55a386067d45271af6d77493c9130 
>   src/master/validation.cpp c5f0d78182a18b0891fb85009ae1be6052a41773 
>   src/tests/master_validation_tests.cpp 
> 7f3751aa04c127c769cfd73ecb47c714e916db41 
> 
> 
> Diff: https://reviews.apache.org/r/70313/diff/2/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Chun-Hung Hsiao
> 
>

Re: Review Request 70320: Used `ResourceQuantities` in `__allocate()` when possible.

[Mesos Reviewbot]

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



Patch looks great!

Reviews applied: [70318, 70319, 70320]

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

- Mesos Reviewbot


On March 27, 2019, 5:31 a.m., Meng Zhu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/70320/
> ---
> 
> (Updated March 27, 2019, 5:31 a.m.)
> 
> 
> Review request for mesos and Benjamin Mahler.
> 
> 
> Bugs: MESOS-9504
> https://issues.apache.org/jira/browse/MESOS-9504
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Replaced `Resources` with `ResourceQuantities` when possible
> in `__allocate()`. This simplifies the code and improves
> performance.
> 
> 
> Diffs
> -
> 
>   src/master/allocator/mesos/hierarchical.hpp 
> 36bf90baf413e99c1580d516dfac0f074335d322 
>   src/master/allocator/mesos/hierarchical.cpp 
> 8bc749903b8ceb09a02e260919377483479302b5 
> 
> 
> Diff: https://reviews.apache.org/r/70320/diff/1/
> 
> 
> Testing
> ---
> 
> make check
> Benchmark results coming in subsequent patches.
> 
> 
> Thanks,
> 
> Meng Zhu
> 
>

Re: Review Request 70320: Used `ResourceQuantities` in `__allocate()` when possible.

[Mesos Reviewbot Windows]

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



PASS: Mesos patch 70320 was successfully built and tested.

Reviews applied: `['70318', '70319', '70320']`

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

- Mesos Reviewbot Windows


On March 27, 2019, 6:31 a.m., Meng Zhu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/70320/
> ---
> 
> (Updated March 27, 2019, 6:31 a.m.)
> 
> 
> Review request for mesos and Benjamin Mahler.
> 
> 
> Bugs: MESOS-9504
> https://issues.apache.org/jira/browse/MESOS-9504
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Replaced `Resources` with `ResourceQuantities` when possible
> in `__allocate()`. This simplifies the code and improves
> performance.
> 
> 
> Diffs
> -
> 
>   src/master/allocator/mesos/hierarchical.hpp 
> 36bf90baf413e99c1580d516dfac0f074335d322 
>   src/master/allocator/mesos/hierarchical.cpp 
> 8bc749903b8ceb09a02e260919377483479302b5 
> 
> 
> Diff: https://reviews.apache.org/r/70320/diff/1/
> 
> 
> Testing
> ---
> 
> make check
> Benchmark results coming in subsequent patches.
> 
> 
> Thanks,
> 
> Meng Zhu
> 
>

Re: Review Request 70225: Made CSI plugin RPC metrics agnostic to CSI versions.

[Mesos Reviewbot Windows]

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



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

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

Failed command: `Start-MesosCITesting`

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

Relevant logs:

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

```
I0327 08:05:28.029320 24212 master.cpp:1295] Agent 
44693939-c920-413b-a64c-fe502a9a79de-S0 at slave(500)@192.10.1.7:58261 
(windows-03.chtsmhjxogyevckjfayqqcnjda.xx.internal.cloudapp.net) disconnected
I0327 08:05:28.029320 24212 master.cpp:3330] Disconnecting agent 
44693939-c920-413b-a64c-fe502a9a79de-S0 at slave(500)@192.10.1.7:58261 
(windows-03.chtsmhjxogyevckjfayqqcnjda.xx.internal.cloudapp.net)
I0327 08:05:28.029320 24212 master.cpp:3349] Deactivating agent 
44693939-c920-413b-a64c-fe502a9a79de-S0 at slave(500)@192.10.1.7:58261 
(windows-03.chtsmhjxogyevckjfayqqcnjda.xx.internal.cloudapp.net)
I0327 08:05:28.031342 24212 containerizer.cpp:2576] Destroying container 
74a8f195-10ca-41a0-bb89-f6d6c3e3e4f4 in RUNNING state
I0327 08:05:28.031342 24212 containerizer.cpp:3278] Transitioning the state of 
container 74a8f195-10ca-41a0-bb89-f6d6c3e3e4f4 from RUNNING to DESTROYING
I0327 08:05:28.031342 23996 hierarchical.cpp:391] Removed framework 
44693939-c920-413b-a64[   OK ] 
IsolationFlag/MemoryIsolatorTest.ROOT_MemUsage/0 (681 ms)
[--] 1 test from IsolationFlag/MemoryIsolatorTest (698 ms total)

[--] Global test environment tear-down
[==] 1132 tests from 108 test cases ran. (555018 ms total)
[  PASSED  ] 1130 tests.
[  FAILED  ] 2 tests, listed below:
[  FAILED  ] DockerFetcherPluginTest.INTERNET_CURL_FetchImage
[  FAILED  ] DockerFetcherPluginTest.INTERNET_CURL_InvokeFetchByName

 2 FAILED TESTS
  YOU HAVE 231 DISABLED TESTS

c-fe502a9a79de-
I0327 08:05:28.031342 24212 launcher.cpp:161] Asked to destroy container 
74a8f195-10ca-41a0-bb89-f6d6c3e3e4f4
I0327 08:05:28.031342 23996 hierarchical.cpp:828] Agent 
44693939-c920-413b-a64c-fe502a9a79de-S0 deactivated
W0327 08:05:28.032325 23176 process.cpp:838] Failed to recv on socket 
WindowsFD::Type::SOCKET=10840 to peer '192.10.1.7:60667': IO failed with error 
code: The specified network name is no longer available.

W0327 08:05:28.033329 23176 process.cpp:1423] Failed to recv on socket 
WindowsFD::Type::SOCKET=11144 to peer '192.10.1.7:60666': IO failed with error 
code: The specified network name is no longer available.

I0327 08:05:28.089020 23352 containerizer.cpp:3117] Container 
74a8f195-10ca-41a0-bb89-f6d6c3e3e4f4 has exited
I0327 08:05:28.117024 20072 master.cpp:1135] Master terminating
I0327 08:05:28.119022 20340 hierarchical.cpp:679] Removed agent 
44693939-c920-413b-a64c-fe502a9a79de-S0
I0327 08:05:28.478070 23176 process.cpp:927] Stopped the socket accept loop
```

- Mesos Reviewbot Windows


On March 16, 2019, 12:40 a.m., Chun-Hung Hsiao wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/70225/
> ---
> 
> (Updated March 16, 2019, 12:40 a.m.)
> 
> 
> Review request for mesos, Benjamin Bannier, James DeFelice, Jie Yu, and Jan 
> Schlicht.
> 
> 
> Bugs: MESOS-9639
> https://issues.apache.org/jira/browse/MESOS-9639
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Since the fine-grained version-specific per-CSI-call metrics are not
> very useful for operators, and most informations are highly correlated
> to per-operation metrics, these metrics are aggregated and made CSI
> version agnostic.
> 
> 
> Diffs
> -
> 
>   docs/monitoring.md 54b872f579dbc68ca5f67f4cc1ba34065a09aee2 
>   src/csi/metrics.hpp PRE-CREATION 
>   src/csi/metrics.cpp PRE-CREATION 
>   src/csi/service_manager.cpp PRE-CREATION 
>   src/csi/v0_volume_manager.cpp PRE-CREATION 
>   src/tests/storage_local_resource_provider_tests.cpp 
> 797f89e3545965e2cf2fd5ec0ecd78fc77a4ea87 
> 
> 
> Diff: https://reviews.apache.org/r/70225/diff/5/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Chun-Hung Hsiao
> 
>

Re: Review Request 70321: Cleaned up `mesos::csi::v0::Client` interface.

[Mesos Reviewbot Windows]

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



FAIL: Failed to apply the dependent review: 70302.

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

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

Relevant logs:

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

```
error: patch failed: src/Makefile.am:1582
error: src/Makefile.am: patch does not apply
```

- Mesos Reviewbot Windows


On March 27, 2019, 6:09 a.m., Chun-Hung Hsiao wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/70321/
> ---
> 
> (Updated March 27, 2019, 6:09 a.m.)
> 
> 
> Review request for mesos, Benjamin Bannier, Jie Yu, and Jan Schlicht.
> 
> 
> Bugs: MESOS-9624
> https://issues.apache.org/jira/browse/MESOS-9624
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Since per-CSI-call metrics are no longer implemented, there is very less
> value to keep the `mesos::csi::v0::RPC` enum, which is tightly coupled
> with `mesos::csi::v0::Client`. Therefore, the enum and its header are
> removed.
> 
> The header and implementation file for `mesos::csi::v0::Client` is also
> renamed for future CSI v1 support.
> 
> 
> Diffs
> -
> 
>   src/CMakeLists.txt 3397c3b1d4e8a7900b2e5f870679cc7aa30b4be2 
>   src/Makefile.am bcafe48b2105575371464a29783bc6f3f1c2cf8d 
>   src/csi/client.hpp c2583cf4c0d2abc38d65ddc801cae3696a8080a9 
>   src/csi/client.cpp 9e17f5bcb902470045dac704f2471d2962336e48 
>   src/csi/rpc.hpp b2502ceb319638038b4d151965f6226db675f96b 
>   src/csi/rpc.cpp e8a2770a6d8ae48a717ed93fa872e9edee45e618 
>   src/csi/service_manager.cpp PRE-CREATION 
>   src/csi/v0_client.hpp PRE-CREATION 
>   src/csi/v0_volume_manager.cpp PRE-CREATION 
>   src/csi/v0_volume_manager_process.hpp PRE-CREATION 
>   src/tests/csi_client_tests.cpp c8f3f04e6bff40d62dd0f85fcc146d71fa9e5d34 
>   src/tests/storage_local_resource_provider_tests.cpp 
> 797f89e3545965e2cf2fd5ec0ecd78fc77a4ea87 
> 
> 
> Diff: https://reviews.apache.org/r/70321/diff/1/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Chun-Hung Hsiao
> 
>

Re: Review Request 70319: Added a `createQuantitiesMap` method in `ResourceQuantities`.

[Mesos Reviewbot]

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



Patch looks great!

Reviews applied: [70318, 70319]

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

- Mesos Reviewbot


On March 27, 2019, 5:21 a.m., Meng Zhu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/70319/
> ---
> 
> (Updated March 27, 2019, 5:21 a.m.)
> 
> 
> Review request for mesos and Benjamin Mahler.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> The method returns a `hashmap` of name and quantity pairs.
> This is used where callers want to have their own copy of the
> quantities and apply direct mutation.
> 
> 
> Diffs
> -
> 
>   src/common/resource_quantities.hpp db4f5afb9f617e27bd08756020b4bcf0bd92dc66 
>   src/common/resource_quantities.cpp 9ae5d9f4aefde81cde7463a7ce118c65a68fc0db 
> 
> 
> Diff: https://reviews.apache.org/r/70319/diff/1/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Meng Zhu
> 
>

Re: Review Request 70319: Added a `createQuantitiesMap` method in `ResourceQuantities`.

[Mesos Reviewbot Windows]

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



PASS: Mesos patch 70319 was successfully built and tested.

Reviews applied: `['70318', '70319']`

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

- Mesos Reviewbot Windows


On March 26, 2019, 10:21 p.m., Meng Zhu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/70319/
> ---
> 
> (Updated March 26, 2019, 10:21 p.m.)
> 
> 
> Review request for mesos and Benjamin Mahler.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> The method returns a `hashmap` of name and quantity pairs.
> This is used where callers want to have their own copy of the
> quantities and apply direct mutation.
> 
> 
> Diffs
> -
> 
>   src/common/resource_quantities.hpp db4f5afb9f617e27bd08756020b4bcf0bd92dc66 
>   src/common/resource_quantities.cpp 9ae5d9f4aefde81cde7463a7ce118c65a68fc0db 
> 
> 
> Diff: https://reviews.apache.org/r/70319/diff/1/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Meng Zhu
> 
>

Review Request 70321: Cleaned up `mesos::csi::v0::Client` interface.

[Chun-Hung Hsiao]

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

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


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


Repository: mesos


Description
---

Since per-CSI-call metrics are no longer implemented, there is very less
value to keep the `mesos::csi::v0::RPC` enum, which is tightly coupled
with `mesos::csi::v0::Client`. Therefore, the enum and its header are
removed.

The header and implementation file for `mesos::csi::v0::Client` is also
renamed for future CSI v1 support.


Diffs
-

  src/CMakeLists.txt 3397c3b1d4e8a7900b2e5f870679cc7aa30b4be2 
  src/Makefile.am bcafe48b2105575371464a29783bc6f3f1c2cf8d 
  src/csi/client.hpp c2583cf4c0d2abc38d65ddc801cae3696a8080a9 
  src/csi/client.cpp 9e17f5bcb902470045dac704f2471d2962336e48 
  src/csi/rpc.hpp b2502ceb319638038b4d151965f6226db675f96b 
  src/csi/rpc.cpp e8a2770a6d8ae48a717ed93fa872e9edee45e618 
  src/csi/service_manager.cpp PRE-CREATION 
  src/csi/v0_client.hpp PRE-CREATION 
  src/csi/v0_volume_manager.cpp PRE-CREATION 
  src/csi/v0_volume_manager_process.hpp PRE-CREATION 
  src/tests/csi_client_tests.cpp c8f3f04e6bff40d62dd0f85fcc146d71fa9e5d34 
  src/tests/storage_local_resource_provider_tests.cpp 
797f89e3545965e2cf2fd5ec0ecd78fc77a4ea87 


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


Testing
---

make check


Thanks,

Chun-Hung Hsiao

Review Request 70303: Moved CSI v0 type helpers to the `mesos/csi/v0.hpp` header.

[Chun-Hung Hsiao]

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

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


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


Repository: mesos


Description
---

The equality check and output helpers for CSI v0 protobufs are now
declared in the `v0.hpp` header to ensure ADL works properly. The
implementation is also moved to a new `v0.cpp` file.

The header and implementation files for CSI v0 utility helpers are also
renamed for future CSI v1 support.


Diffs
-

  include/mesos/csi/v0.hpp 19d9445fe1da7be6e41b484b5a78dcd10e5ece52 
  src/CMakeLists.txt 3397c3b1d4e8a7900b2e5f870679cc7aa30b4be2 
  src/Makefile.am bcafe48b2105575371464a29783bc6f3f1c2cf8d 
  src/csi/service_manager.cpp PRE-CREATION 
  src/csi/utils.hpp 9145c6795c3ecdde5de5859a852763fe9aeb1ddf 
  src/csi/utils.cpp fd6f95d88caf69e2ae197cf940beb93c164565bc 
  src/csi/v0.cpp PRE-CREATION 
  src/csi/v0_volume_manager.cpp PRE-CREATION 
  src/csi/v0_volume_manager_process.hpp PRE-CREATION 
  src/examples/test_csi_plugin.cpp 73a6c43e72afec0dd124b0fe2f8ef0e45acb307f 
  src/resource_provider/storage/uri_disk_profile_adaptor.cpp 
cb574be2a4b4e443248b2001f822d739e5bbe7b9 
  src/tests/csi_utils_tests.cpp PRE-CREATION 


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


Testing
---

make check


Thanks,

Chun-Hung Hsiao

Re: Review Request 70302: Adjusted CSI v0 proto compilation.

[Chun-Hung Hsiao]

> On March 26, 2019, 3:44 p.m., Benjamin Bannier wrote:
> > 3rdparty/CMakeLists.txt
> > Line 1 (original), 1 (patched)
> > 
> >
> > I would suggest we move this patch in between 
> > https://reviews.apache.org/r/70248/ and https://reviews.apache.org/r/70247/.

This cannot be moved before r/72048 because it relies on that some `csi.proto` 
dependencies have been removed from r/70248. Dropping.


> On March 26, 2019, 3:44 p.m., Benjamin Bannier wrote:
> > include/csi/spec.hpp
> > Line 1 (original), 1 (patched)
> > 
> >
> > Let's explicitly include this file in `src/csi/volume_manager.hpp`.

Reordered the patches so no need to do the header inclusion. Let's keep 
`volume_manager.hpp` CSI-version agnostic.


- Chun-Hung


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


On March 26, 2019, 5:13 a.m., Chun-Hung Hsiao wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/70302/
> ---
> 
> (Updated March 26, 2019, 5:13 a.m.)
> 
> 
> Review request for mesos, Benjamin Bannier and Joseph Wu.
> 
> 
> Bugs: MESOS-9624
> https://issues.apache.org/jira/browse/MESOS-9624
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This patch makes the following adjustments so we can build CSI v0 and v1
> in the future:
> 
> * Standardize placements for third-party proto files: `csi.proto` from
>   CSI v0 will be in `build/include/csi/v0/` and that from v0 will be in
>   `build/include/csi/v1` in the future. Dependent proto files should
>   import `csi/v0/csi.proto` or `csi/v1/csi.proto`.
> 
> * Moved `include/csi/spec.hpp` to `include/mesos/csi/v0.hpp`. In the
>   future, CSI v1 proto headers will be in `include/mesos/csi/v1.hpp`.
>   Since we don't insteall CSI headers, for the installed `v0.hpp` and
>   `v1.hpp` headers to work, users must ensure that the CSI headers can
>   be found through `csi/v0/csi*.pb.h` and `csi/v1/csi*.pb.h`.
> 
> 
> Diffs
> -
> 
>   3rdparty/CMakeLists.txt 1999dd20964da96bc5acfbd47cb80d4ca6f734b9 
>   3rdparty/Makefile.am 98a2623b34132885279bea6135c0da4ffc8c59cf 
>   3rdparty/cmake/Versions.cmake 972c706a18bfce45af6c2632b326606052888b02 
>   3rdparty/versions.am 24381073f4c77d92c5ba62f54bb2194bf041ee6c 
>   include/csi/spec.hpp 19d9445fe1da7be6e41b484b5a78dcd10e5ece52 
>   src/CMakeLists.txt 3397c3b1d4e8a7900b2e5f870679cc7aa30b4be2 
>   src/Makefile.am d451d7cabe3bf5d4f5039cfac5de1b03ef891d07 
>   src/cmake/MesosProtobuf.cmake 09074d77a1df5076c6e51292523ce994a77a88f2 
>   src/csi/client.hpp c2583cf4c0d2abc38d65ddc801cae3696a8080a9 
>   src/csi/rpc.hpp b2502ceb319638038b4d151965f6226db675f96b 
>   src/csi/utils.hpp 9145c6795c3ecdde5de5859a852763fe9aeb1ddf 
>   src/examples/test_csi_plugin.cpp 73a6c43e72afec0dd124b0fe2f8ef0e45acb307f 
>   src/resource_provider/storage/disk_profile.proto 
> 1c97e9c62fb3154d6048d0e55352a1276adcbca8 
>   src/resource_provider/storage/disk_profile_utils.hpp 
> 8a83a15ba555ce66bbb86b8df72178bce17a615a 
>   src/tests/csi_utils_tests.cpp PRE-CREATION 
>   src/tests/disk_profile_adaptor_tests.cpp 
> 0ccbc79d7ffb82a68b7ed5aeab930bcd8e6e770e 
>   src/tests/mock_csi_plugin.hpp 6897fbc878f1e2f5b9e9c402b09358c187af79a0 
> 
> 
> Diff: https://reviews.apache.org/r/70302/diff/1/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Chun-Hung Hsiao
> 
>

Re: Review Request 70223: Made `StorageLocalResourceProviderProcess` no longer exposed.

[Chun-Hung Hsiao]

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

(Updated March 27, 2019, 6:04 a.m.)


Review request for mesos, Benjamin Bannier and Jan Schlicht.


Changes
---

Rebased and updated the description.


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


Repository: mesos


Description (updated)
---

Since we now moved the CSI calling logics to `v0::VolumeManagerProcess`,
SLRP tests no longer rely on intercepting member function dispatches of
`StorageLocalResourceProviderProcess`, so we undo the exposure made in
r/69827 (commit e9d2a296081).


Diffs (updated)
-

  src/Makefile.am bcafe48b2105575371464a29783bc6f3f1c2cf8d 
  src/resource_provider/storage/provider.cpp 
2711503cdb58cb9b34af8c9fad0908c5f788a2af 
  src/resource_provider/storage/provider_process.hpp 
a5536b3d735e01eb1c4dc52d0602d973155f3c93 


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

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


Testing
---

make check


Thanks,

Chun-Hung Hsiao

Re: Review Request 70223: Made `StorageLocalResourceProviderProcess` no longer exposed.

[Chun-Hung Hsiao]

> On March 26, 2019, 3:35 p.m., Benjamin Bannier wrote:
> > Looks good!
> > 
> > What do you think about referencing `e9d2a296081` instead of `r/69827` in 
> > the commit message?
> 
> Chun-Hung Hsiao wrote:
> Hmm the SHA might change if there's a rebase before committing it into 
> the master branch. How about having a complete URL of r/69827?

Wait I was mistaken. Yeah sure let me do that.


- Chun-Hung


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


On March 16, 2019, 12:02 a.m., Chun-Hung Hsiao wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/70223/
> ---
> 
> (Updated March 16, 2019, 12:02 a.m.)
> 
> 
> Review request for mesos, Benjamin Bannier and Jan Schlicht.
> 
> 
> Bugs: MESOS-9622
> https://issues.apache.org/jira/browse/MESOS-9622
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Since we now moved the CSI calling logics to `v0::VolumeManagerProcess`,
> SLRP tests no longer rely on intercepting member function dispatches of
> `StorageLocalResourceProviderProcess`, so we undo the exposure made in
> r/69827.
> 
> 
> Diffs
> -
> 
>   src/Makefile.am d451d7cabe3bf5d4f5039cfac5de1b03ef891d07 
>   src/resource_provider/storage/provider.cpp 
> fea623c292158deb1b4b4b9ab1ac208031471519 
>   src/resource_provider/storage/provider_process.hpp 
> a5536b3d735e01eb1c4dc52d0602d973155f3c93 
> 
> 
> Diff: https://reviews.apache.org/r/70223/diff/3/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Chun-Hung Hsiao
> 
>

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

[Chun-Hung Hsiao]

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

(Updated March 27, 2019, 6:02 a.m.)


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


Changes
---

Rebased.


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


Repository: mesos


Description
---

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


Diffs (updated)
-

  src/csi/v0_volume_manager.cpp PRE-CREATION 


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

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


Testing
---

make check


Thanks,

Chun-Hung Hsiao

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

[Chun-Hung Hsiao]

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

(Updated March 27, 2019, 6 a.m.)


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


Changes
---

Rebased.


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


Repository: mesos


Description
---

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

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


Diffs (updated)
-

  src/csi/v0_volume_manager.cpp PRE-CREATION 
  src/csi/v0_volume_manager_process.hpp PRE-CREATION 
  src/resource_provider/storage/provider.cpp 
2711503cdb58cb9b34af8c9fad0908c5f788a2af 
  src/resource_provider/storage/provider_process.hpp 
a5536b3d735e01eb1c4dc52d0602d973155f3c93 
  src/tests/storage_local_resource_provider_tests.cpp 
797f89e3545965e2cf2fd5ec0ecd78fc77a4ea87 


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

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


Testing
---

sudo make check


Thanks,

Chun-Hung Hsiao