Re: Review Request 67677: Added doc for new pull latency metrics.

[Mesos Reviewbot Windows]

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



PASS: Mesos patch 67677 was successfully built and tested.

Reviews applied: `['53105', '66875', '67677']`

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

- Mesos Reviewbot Windows


On June 20, 2018, 8:26 p.m., Zhitao Li wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/67677/
> ---
> 
> (Updated June 20, 2018, 8:26 p.m.)
> 
> 
> Review request for mesos, Jason Lai and Qian Zhang.
> 
> 
> Bugs: MESOS-6451
> https://issues.apache.org/jira/browse/MESOS-6451
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added doc for new pull latency metrics.
> 
> 
> Diffs
> -
> 
>   docs/monitoring.md 2985f68f644e84cdc5337c516ee06c95abd6018b 
> 
> 
> Diff: https://reviews.apache.org/r/67677/diff/2/
> 
> 
> Testing
> ---
> 
> https://github.com/zhitaoli/mesos/blob/public/zhitao/time_containerizer_pulls/docs/monitoring.md#containerizers
> 
> 
> Thanks,
> 
> Zhitao Li
> 
>

Review Request 67681: Fixed flaky test `MasterAPITest.SubscribersReceiveHealthUpdates`.

[Meng Zhu]

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

Review request for mesos and Greg Mann.


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


Repository: mesos


Description
---

This patch fixes the flaky test by explicitly killing the
task before test tear-down, avoiding the race condition
between scheduler tear-down and scheduler getting/acknowledging
status update.


Diffs
-

  src/tests/api_tests.cpp 4d6b5b3e938faed934e875e23e29c67fd50b9d6f 


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


Testing
---

make check


Thanks,

Meng Zhu

Re: Review Request 67670: Added a unit test for disappeared profiles.

[Chun-Hung Hsiao]

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

(Updated June 21, 2018, 5:02 a.m.)


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


Changes
---

Addressed most Benjamin's comments.


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


Repository: mesos


Description
---

The `ROOT_ProfileDisappeared` tests if a `CREATE_VOLUME` consuming a RAW
disk with a disappeared profile will be dropped, and if the disk space
freed by a `DESTROY_VOLUME` destroying a volume with a disappeared
profile will be recovered with a newly appeared profile.


Diffs (updated)
-

  src/tests/storage_local_resource_provider_tests.cpp 
1afe8a8e0413ef225e952cd9cd6376e5d82774e5 


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

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


Testing
---

sudo make check


Thanks,

Chun-Hung Hsiao

Re: Review Request 67561: Refactored `struct Slave` in the allocator for better performance.

[Meng Zhu]

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

(Updated June 20, 2018, 9:56 p.m.)


Review request for mesos, Gastón Kleiman, Greg Mann, Kapil Arya, and Till 
Toenshoff.


Changes
---

Rebased.


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


Repository: mesos


Description
---

This patch refactors the `struct Slave` in the allocator.
In particular, it addresses the slowness of computing
agent's available resources. Instead of calculating it
every time on the fly, this patch "denormalizes" the agent
available resources by updating and persisting the field
each time when agent's allocated or total resources change.


Diffs (updated)
-

  src/master/allocator/mesos/hierarchical.hpp 
19aed2ddfb7d93f3d2a57817a29efc4fd1822ba6 
  src/master/allocator/mesos/hierarchical.cpp 
f631ce73d578a313dfdacf1ee97737dde90ae8f8 


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

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


Testing
---

make check

In our simulation environment, we observed 16% allocator performance improvment 
without quota, and 32% improvment when quota is used.


Thanks,

Meng Zhu

Re: Review Request 67615: Modified `createStrippedScalarQuantity()` to clear all metadata fields.

[Meng Zhu]

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

(Updated June 20, 2018, 9:56 p.m.)


Review request for mesos, Benjamin Mahler, Greg Mann, Kapil Arya, and Till 
Toenshoff.


Changes
---

Rebased.


Repository: mesos


Description
---

Currently `createStrippedScalarQuantity()` strips resource meta-data
except revocable and turns dynamic reservation into static reservation.
However, no current code depends on this behavior and this leads to
boilerplate code around call sites and performance overhead.

This patch further clears the above meta-data for simplicity
and performance.


Diffs (updated)
-

  include/mesos/resources.hpp bd6d6d61a8637ecb3ababaf7247b5fc49cc8b070 
  include/mesos/v1/resources.hpp c065dd16e7723ef093a8e58841495e2bd47ce9d8 
  src/common/resources.cpp b9f1c2ddbcd09ae1a1ab2caf9856d1fb9a589e89 
  src/master/allocator/mesos/hierarchical.cpp 
f631ce73d578a313dfdacf1ee97737dde90ae8f8 
  src/tests/resources_tests.cpp 14104239da7dec2aa6d6e78777249c69bd019a6d 
  src/tests/sorter_tests.cpp 9cdffd778967bca3d0055be8a04fbb702432e59d 
  src/v1/resources.cpp 3d06fc64a19ab57d2af5fb465d3dc06ba79afcc5 


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

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


Testing
---

make check


Thanks,

Meng Zhu

Re: Review Request 67669: Allowed allocation quantities to be changed when updating the sorter.

[Chun-Hung Hsiao]

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

(Updated June 21, 2018, 4:52 a.m.)


Review request for mesos, Benjamin Bannier, Benjamin Mahler, Jie Yu, and Meng 
Zhu.


Changes
---

Partially addressed Benjamin's comment.


Summary (updated)
-

Allowed allocation quantities to be changed when updating the sorter.


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


Repository: mesos


Description (updated)
---

This patch allows the allocator to change allocation quantities when
calling `Sorter::update`, so Mesos can support arbitrary resource
conversions.

An example usage is that we can make `DESTROY_VOLUME` to convert a
volume with a stale profile to an empty resource, so the disk space
will disappear instead of being offered out to other frameworks.


Diffs (updated)
-

  CHANGELOG 0e44277389e5a96c221d6c3d501da2bb93d27faa 
  docs/upgrades.md cc8a251945af27a432dbeb5585b998cd84e35b22 
  src/master/allocator/mesos/hierarchical.cpp 
b558228290e5ae6dbcc1b8a6e1fe69db9fd5874c 
  src/master/allocator/sorter/drf/sorter.cpp 
755a36ead42a91053c881fc46dc6acb39c819cac 
  src/master/allocator/sorter/sorter.hpp 
e5e86513dc4023d9c471753897635923c6c9203f 
  src/tests/sorter_tests.cpp 9cdffd778967bca3d0055be8a04fbb702432e59d 


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

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


Testing
---

sudo make check


Thanks,

Chun-Hung Hsiao

Re: Review Request 67669: Removed an invariant check when updating the hierarchical allocator.

[Chun-Hung Hsiao]

> On June 20, 2018, 2:20 p.m., Benjamin Bannier wrote:
> > src/master/allocator/mesos/hierarchical.cpp
> > Line 921 (original)
> > 
> >
> > While this might not be required by the allocator, generally an 
> > operation changing the resource quantity is not supported by the `Sorter` 
> > interface.
> > 
> > We already rely on the invariant being checked here in above calls to 
> > `Sorter::update` which explicit disallow such updates. In order to support 
> > operations changing the quantity of the stripped resources we need to do 
> > additional work.

I added a unit test and updated the description of `Sorter::update`, but I'm 
hesitating to add some more concrete checks. We discussed about checking the 
resources names don't change, but changing names are essentially removing the 
old name and adding a new one, so it seems not consistent to have such checks.

Another concern I have is that we now rely on the caller to update the 
allocation and the total resources synchronously, but I cannot find a good way 
to validate that. Suggestions?


- Chun-Hung


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


On June 20, 2018, 5:34 a.m., Chun-Hung Hsiao wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/67669/
> ---
> 
> (Updated June 20, 2018, 5:34 a.m.)
> 
> 
> Review request for mesos, Benjamin Bannier, Benjamin Mahler, Jie Yu, and Meng 
> Zhu.
> 
> 
> Bugs: MESOS-8995
> https://issues.apache.org/jira/browse/MESOS-8995
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> `HierarchicalAllocatorProcess::updateAllocation` checks that the given
> resource conversions should not change the allocation quantities of the
> specified framework. Although this is true for speculative operations,
> this might not hold for arbitrary resource conversions in general.
> 
> Since the allocator does not rely on this invariant, this patch relaxes
> the invariant to make resource conversions more future-proof.
> 
> 
> Diffs
> -
> 
>   src/master/allocator/mesos/hierarchical.cpp 
> b558228290e5ae6dbcc1b8a6e1fe69db9fd5874c 
> 
> 
> Diff: https://reviews.apache.org/r/67669/diff/1/
> 
> 
> Testing
> ---
> 
> sudo make check
> 
> 
> Thanks,
> 
> Chun-Hung Hsiao
> 
>

Re: Review Request 53105: Added an hourly timer for `containerizer/docker/image_pull`.

[Zhitao Li]

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

(Updated June 20, 2018, 9:49 p.m.)


Review request for mesos, Gilbert Song, Jason Lai, Jie Yu, and Qian Zhang.


Changes
---

Qian's comment.


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


Repository: mesos


Description
---

Added an hourly timer for `containerizer/docker/image_pull`.


Diffs (updated)
-

  src/slave/containerizer/docker.hpp 1ed47c85f765da6fb91a2a243fcc5030e9686190 
  src/slave/containerizer/docker.cpp f83036aa5a212d1d88e6f5aa41023abc125f7116 


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

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


Testing
---

Ran `mesos-execute` with a docker image and docker containerizer twice, 
observed the following metrics:
```
curl -s localhost:5051/metrics/snapshot | jq .
  "containerizer/docker/image_pull_ms": 6628.833024,
  "containerizer/docker/image_pull_ms/count": 2,
  "containerizer/docker/image_pull_ms/max": 8851.497216,
  "containerizer/docker/image_pull_ms/min": 6628.833024,
  "containerizer/docker/image_pull_ms/p50": 7740.16512,
  "containerizer/docker/image_pull_ms/p90": 8629.2307968,
  "containerizer/docker/image_pull_ms/p95": 8740.3640064,
  "containerizer/docker/image_pull_ms/p99": 8829.27057408,
  "containerizer/docker/image_pull_ms/p999": 8849.274551808,
  "containerizer/docker/image_pull_ms/p": 8851.2749495808,
```


Thanks,

Zhitao Li

Re: Review Request 67668: Fixed a bug in `TestCSIPlugin::DeleteVolume`.

[Chun-Hung Hsiao]

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

(Updated June 21, 2018, 4:35 a.m.)


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


Changes
---

Addressed Benjamin's comment.


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


Repository: mesos


Description
---

The test CSI plugin mistakenly reduce the available capacity instead of
expanding it when deleting a volume. This patch fixes this bug.


Diffs (updated)
-

  src/examples/test_csi_plugin.cpp 9c4da8811cc260bcf3bccfea3036a7964cb75697 


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

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


Testing
---

sudo make check


Thanks,

Chun-Hung Hsiao

Re: Review Request 67667: Renamed the `NewProfile` SLRP test and made it based on offers.

[Chun-Hung Hsiao]

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

(Updated June 21, 2018, 4:35 a.m.)


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


Changes
---

Addressed Benjamin's comments and used a `Promise` to synchronize the test 
better.


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


Repository: mesos


Description
---

The test is renamed to `ProfileAppeared` to reflect the fact that the
SLRP passively observed a profile showing up, not actively adding a
profile.

The test is changed from intercepting `UpdateSlaveMessage`s to checking
offers to make it more like an end-to-end test. Also with the
`DiskProfileServer` helper we can control the clock better.


Diffs (updated)
-

  src/tests/storage_local_resource_provider_tests.cpp 
1afe8a8e0413ef225e952cd9cd6376e5d82774e5 


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

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


Testing
---

sudo make check


Thanks,

Chun-Hung Hsiao

Re: Review Request 67666: Added a `TestDiskProfileServer` helper.

[Chun-Hung Hsiao]

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

(Updated June 21, 2018, 4:33 a.m.)


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


Changes
---

Addressed Benjamin's comments and added a `url()` method.


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


Repository: mesos


Description
---

This helper class will be used to test functionalities related to
profile changes.


Diffs (updated)
-

  src/Makefile.am 2bcee1e0e198e6be009174570cdaa1c8b8372a71 
  src/tests/disk_profile_adaptor_tests.cpp 
4485f1635f484ce6e1c7c532eedb277f5eee118b 
  src/tests/disk_profile_server.hpp PRE-CREATION 


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

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


Testing
---

sudo make check


Thanks,

Chun-Hung Hsiao

Re: Review Request 67664: Fixed a race between `UPDATE_STATE` and `UPDATE_OPERATION_STATUS`.

[Chun-Hung Hsiao]

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

(Updated June 21, 2018, 4:29 a.m.)


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


Changes
---

Addressed Benjamin's comments and move the update logic to a function as Jie 
suggested.


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


Repository: mesos


Description
---

Since a resource provider and its operation status update manager run in
different actors, the `UPDATE_OPERATION_STATUS` call of a completed
operation may race with an `UPDATE_STATE` call.

To deal with this race, the agent should update the latest statuses of
all completed operations received in the `UPDATE_STATE` call to avoid
erroneously applying those operations when receiving
`UPDATE_OPERATION_STATUS`es.


Diffs (updated)
-

  src/slave/slave.hpp bf14d3569e677b2be6790ef774985df6937ebb29 
  src/slave/slave.cpp 8edd652f7f410dbadaf6c2ca3736349065e4340a 
  src/tests/slave_tests.cpp b46fb8efc524852f62428040ff958bd44e9efe9f 


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

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


Testing
---

sudo make check


Thanks,

Chun-Hung Hsiao

Re: Review Request 67663: Made `UriDiskProfileAdaptor` be able to handle disappeared profiles.

[Chun-Hung Hsiao]

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

(Updated June 21, 2018, 4:27 a.m.)


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


Changes
---

Addressed Benjamin's comments.


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


Repository: mesos


Description
---

Now the `UriDiskProfileAdaptor` module can handle disappeared profiles
when polling the profile matrix, and performs a best-effort immutability
check for reappeared profiles.


Diffs (updated)
-

  src/resource_provider/storage/uri_disk_profile_adaptor.hpp 
0484933b42d0bd66c689b06cb48f492eef7bc606 
  src/resource_provider/storage/uri_disk_profile_adaptor.cpp 
614590ef7d1c0cc1df99b8e57c7fbd496793b5a7 
  src/tests/disk_profile_adaptor_tests.cpp 
4485f1635f484ce6e1c7c532eedb277f5eee118b 


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

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


Testing
---

sudo make check

A end-to-end test will be added later in the chain.


Thanks,

Chun-Hung Hsiao

Re: Review Request 67677: Added doc for new pull latency metrics.

[Mesos Reviewbot]

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



Patch looks great!

Reviews applied: [53105, 66875, 67677]

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

- Mesos Reviewbot


On June 20, 2018, 1:26 p.m., Zhitao Li wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/67677/
> ---
> 
> (Updated June 20, 2018, 1:26 p.m.)
> 
> 
> Review request for mesos, Jason Lai and Qian Zhang.
> 
> 
> Bugs: MESOS-6451
> https://issues.apache.org/jira/browse/MESOS-6451
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added doc for new pull latency metrics.
> 
> 
> Diffs
> -
> 
>   docs/monitoring.md 2985f68f644e84cdc5337c516ee06c95abd6018b 
> 
> 
> Diff: https://reviews.apache.org/r/67677/diff/1/
> 
> 
> Testing
> ---
> 
> https://github.com/zhitaoli/mesos/blob/public/zhitao/time_containerizer_pulls/docs/monitoring.md#containerizers
> 
> 
> Thanks,
> 
> Zhitao Li
> 
>

Re: Review Request 67667: Renamed the `NewProfile` SLRP test and made it based on offers.

[Chun-Hung Hsiao]

> On June 20, 2018, 12:43 p.m., Benjamin Bannier wrote:
> > src/tests/storage_local_resource_provider_tests.cpp
> > Line 529 (original), 534 (patched)
> > 
> >
> > Just wondering what actually requires `ROOT` in this test. I see we 
> > enable filesystem isolation, but is that required?
> 
> Chun-Hung Hsiao wrote:
> The SLRP launches the CSI plugin in a standalone container, and it seems 
> safer to enable the isolation. In contrast, I could disable this isolation 
> and manually do an `unmountAll` during teardown. Do you prefer the latter? If 
> yes I can have a followup patch to do this.

Created https://issues.apache.org/jira/browse/MESOS-9016 for this issue.


- Chun-Hung


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


On June 20, 2018, 5:33 a.m., Chun-Hung Hsiao wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/67667/
> ---
> 
> (Updated June 20, 2018, 5:33 a.m.)
> 
> 
> Review request for mesos, Benjamin Bannier, Jie Yu, and Jan Schlicht.
> 
> 
> Bugs: MESOS-8995
> https://issues.apache.org/jira/browse/MESOS-8995
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> The test is renamed to `ProfileAppeared` to reflect the fact that the
> SLRP passively observed a profile showing up, not actively adding a
> profile.
> 
> The test is changed from intercepting `UpdateSlaveMessage`s to checking
> offers to make it more like an end-to-end test. Also with the
> `DiskProfileServer` helper we can control the clock better.
> 
> 
> Diffs
> -
> 
>   src/tests/storage_local_resource_provider_tests.cpp 
> 1afe8a8e0413ef225e952cd9cd6376e5d82774e5 
> 
> 
> Diff: https://reviews.apache.org/r/67667/diff/1/
> 
> 
> Testing
> ---
> 
> sudo make check
> 
> 
> Thanks,
> 
> Chun-Hung Hsiao
> 
>

Re: Review Request 66881: Added benchmark test for master metrics.

[Mesos Reviewbot Windows]

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



PASS: Mesos patch 66881 was successfully built and tested.

Reviews applied: `['66882', '66819', '66820', '66822', '66823', '66845', 
'66825', '66846', '66847', '66881']`

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

- Mesos Reviewbot Windows


On June 21, 2018, 8:08 a.m., Gilbert Song wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/66881/
> ---
> 
> (Updated June 21, 2018, 8:08 a.m.)
> 
> 
> Review request for mesos, Benjamin Mahler, Gastón Kleiman, Greg Mann, Jie Yu, 
> and Vinod Kone.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added benchmark test for master metrics.
> 
> 
> Diffs
> -
> 
>   src/tests/master_benchmarks.cpp fbfffb69930c30b038f74e0b831fc0ae41c820f0 
> 
> 
> Diff: https://reviews.apache.org/r/66881/diff/4/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Gilbert Song
> 
>

Re: Review Request 66875: Added an hourly timer for docker store pull latency.

[Qian Zhang]

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


Fix it, then Ship it!





src/slave/containerizer/mesos/provisioner/docker/store.cpp
Lines 40 (patched)


Why do we need this header file?



src/slave/containerizer/mesos/provisioner/docker/store.cpp
Lines 104-106 (patched)


A space missed between `)` and `:`.

And can we merge these 3 line codes into 2 lines?
```
Metrics() : image_pull(
  "containerizer/mesos/provisioner/docker_store/image_pull", 
Hours(1))
```



src/slave/containerizer/mesos/provisioner/docker/store.cpp
Lines 111 (patched)


Kill this blank line.


- Qian Zhang


On June 21, 2018, 2:26 a.m., Zhitao Li wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/66875/
> ---
> 
> (Updated June 21, 2018, 2:26 a.m.)
> 
> 
> Review request for mesos, Gilbert Song, Jason Lai, Jie Yu, and Qian Zhang.
> 
> 
> Bugs: MESOS-6451
> https://issues.apache.org/jira/browse/MESOS-6451
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This patch added pull latency tracking for docker store, which can tell
> us both latency distribution of pull as well as number of pulls.
> 
> 
> Diffs
> -
> 
>   src/slave/containerizer/mesos/provisioner/docker/store.cpp 
> 6e7dc44321bff3198e5ffe69be1ba329be3ee31e 
> 
> 
> Diff: https://reviews.apache.org/r/66875/diff/3/
> 
> 
> Testing
> ---
> 
> Ran agent in command line and trigger several launches through 
> `mesos-execute`, observed following metrics from agent endpoint:
> 
> ```
>   "containerizer/mesos/provisioner/docker_store/image_pull_ms": 3619.53408,
>   "containerizer/mesos/provisioner/docker_store/image_pull_ms/count": 2,
>   "containerizer/mesos/provisioner/docker_store/image_pull_ms/max": 
> 4208.662016,
>   "containerizer/mesos/provisioner/docker_store/image_pull_ms/min": 
> 3619.53408,
>   "containerizer/mesos/provisioner/docker_store/image_pull_ms/p50": 
> 3914.098048,
>   "containerizer/mesos/provisioner/docker_store/image_pull_ms/p90": 
> 4149.7492224,
>   "containerizer/mesos/provisioner/docker_store/image_pull_ms/p95": 
> 4179.2056192,
>   "containerizer/mesos/provisioner/docker_store/image_pull_ms/p99": 
> 4202.77073664,
>   "containerizer/mesos/provisioner/docker_store/image_pull_ms/p999": 
> 4208.072888064,
>   "containerizer/mesos/provisioner/docker_store/image_pull_ms/p": 
> 4208.6031032064,
> ```
> 
> 
> Thanks,
> 
> Zhitao Li
> 
>

Re: Review Request 53105: Added an hourly timer for `containerizer/docker/image_pull`.

[Qian Zhang]

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


Fix it, then Ship it!





src/slave/containerizer/docker.hpp
Lines 177 (patched)


Better to change to:
```
  struct Metrics
  {
```


- Qian Zhang


On June 21, 2018, 2:25 a.m., Zhitao Li wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/53105/
> ---
> 
> (Updated June 21, 2018, 2:25 a.m.)
> 
> 
> Review request for mesos, Gilbert Song, Jason Lai, Jie Yu, and Qian Zhang.
> 
> 
> Bugs: MESOS-6451
> https://issues.apache.org/jira/browse/MESOS-6451
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added an hourly timer for `containerizer/docker/image_pull`.
> 
> 
> Diffs
> -
> 
>   src/slave/containerizer/docker.hpp 1ed47c85f765da6fb91a2a243fcc5030e9686190 
>   src/slave/containerizer/docker.cpp f83036aa5a212d1d88e6f5aa41023abc125f7116 
> 
> 
> Diff: https://reviews.apache.org/r/53105/diff/6/
> 
> 
> Testing
> ---
> 
> Ran `mesos-execute` with a docker image and docker containerizer twice, 
> observed the following metrics:
> ```
> curl -s localhost:5051/metrics/snapshot | jq .
>   "containerizer/docker/image_pull_ms": 6628.833024,
>   "containerizer/docker/image_pull_ms/count": 2,
>   "containerizer/docker/image_pull_ms/max": 8851.497216,
>   "containerizer/docker/image_pull_ms/min": 6628.833024,
>   "containerizer/docker/image_pull_ms/p50": 7740.16512,
>   "containerizer/docker/image_pull_ms/p90": 8629.2307968,
>   "containerizer/docker/image_pull_ms/p95": 8740.3640064,
>   "containerizer/docker/image_pull_ms/p99": 8829.27057408,
>   "containerizer/docker/image_pull_ms/p999": 8849.274551808,
>   "containerizer/docker/image_pull_ms/p": 8851.2749495808,
> ```
> 
> 
> Thanks,
> 
> Zhitao Li
> 
>

Re: Review Request 67680: Fixed archiver failure to extract hardlinks to another directory.

[Mesos Reviewbot Windows]

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



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

Reviews applied: `['67680']`

Failed command: `Start-MesosCITesting`

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

Relevant logs:

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

```

[--] 2 tests from SocketTests
[ RUN  ] SocketTests.InitSocket
[   OK ] SocketTests.InitSocket (1 ms)
[ RUN  ] SocketTests.IntFD
[   OK ] SocketTests.IntFD (1 ms)
[--] 2 tests from SocketTests (5 ms total)

[--] 2 tests from StrerrorTest
[ RUN  ] StrerrorTest.ValidErrno
[   OK ] StrerrorTest.ValidErrno (0 ms)
[ RUN  ] StrerrorTest.InvalidErrno
[   OK ] StrerrorTest.InvalidErrno (0 ms)
[--] 2 tests from StrerrorTest (1 ms total)

[--] 2 tests from OsSendfileTest
[ RUN  ] OsSendfileTest.Sendfile
[   OK ] OsSendfileTest.Sendfile (1069 ms)
[ RUN  ] OsSendfileTest.SendfileAsync
[   OK ] OsSendfileTest.SendfileAsync (18 ms)
[--] 2 tests from OsSendfileTest (1089 ms total)

[--] Global test environment tear-down
[==] 323 tests from 49 test cases ran. (5510 ms total)
[  PASSED  ] 322 tests.
[  FAILED  ] 1 test, listed below:
[  FAILED  ] ArchiverTest.ExtractTarGzFileWithLinks

 1 FAILED TEST
```

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

```
'invalid.command' is not recognized as an internal or external command,
operable program or batch file.
Subcommand 'subcommand' is not available
Usage: command  [OPTIONS]

Available subcommands:
help
subcommand2

Multiple subcommands have name 'subcommand'
```

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

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

[--] 2 tests from ContainerizerType/DefaultContainerDNSFlagTest
[ RUN  ] ContainerizerType/DefaultContainerDNSFlagTest.ValidateFlag/0
[   OK ] ContainerizerType/DefaultContainerDNSFlagTest.ValidateFlag/0 (42 
ms)
[ RUN  ] ContainerizerType/DefaultContainerDNSFlagTest.ValidateFlag/1
[   OK ] ContainerizerType/DefaultContainerDNSFlagTest.ValidateFlag/1 (47 
ms)
[--] 2 tests from ContainerizerType/DefaultContainerDNSFlagTest (90 ms 
total)

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

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

[--] Global test environment tear-down
[==] 988 tests from 97 test cases ran. (42 ms total)
[  PASSED  ] 987 tests.
[  FAILED  ] 1 test, listed below:
[  FAILED  ] MesosContainerizer/DefaultExecutorTest.SigkillExecutor/0, where 
GetParam() = "mesos"

 1 FAILED TEST
  YOU HAVE 220 DISABLED TESTS

```

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

```
I0621 01:01:03.989440 12752 master.cpp:10863] Updating the state of task 
85b423a0-601e-4446-bdf6-43e6b0baa9bf of framework 
99d97df4-ab49-4018-ab43-9522a83c228b- (latest state: TASK_KILLED, status 
update state: TASK_KILLED)
I0621 01:01:03.989440  9372 slave.cpp:3939] Shutting down framework 
99d97df4-ab49-4018-ab43-9522a83c228b-
I0621 01:01:03.989440  9372 slave.cpp:6660] Shutting down exeI0621 
01:01:03.830482  5280 exec.cpp:162] Version: 1.7.0
I0621 01:01:03.855453 11344 exec.cpp:236] Executor registered on agent 
99d97df4-ab49-4018-ab43-9522a83c228b-S0
I0621 01:01:03.859453  2560 executor.cpp:178] Received SUBSCRIBED event
I0621 01:01:03.863478  2560 executor.cpp:182] Subscribed executor on 
windows-02.enofukwu14ruplxn0gs3yzmsgf.xx.internal.cloudapp.net
I0621 01:01:03.863478  2560 executor.cpp:178] Received LAUNCH event
I0621 01:01:03.868466  2560 executor.cpp:665] Starting task 
85b423a0-601e-4446-bdf6-43e6b0baa9bf
I0621 01:01:03.948482  2560 executor.cpp:485] Running 
'D:\DCOS\mesos\src\mesos-containerizer.exe launch '
I0621 01:01:03.961462  2560 executor.cpp:678] Forked command at 10096
I0621 01:01:03.992444 13716 exec.cpp:445] Executor asked to shutdown
I0621 01:01:03.993446  2560 executor.cpp:178] Received

Re: Review Request 67513: Added a master flag to configure minimum allocatable resources.

[Greg Mann]

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




src/master/master.cpp
Lines 774-786 (patched)


Not indented far enough. I'll fix this while committing.


- Greg Mann


On June 20, 2018, 5:52 p.m., Meng Zhu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/67513/
> ---
> 
> (Updated June 20, 2018, 5:52 p.m.)
> 
> 
> Review request for mesos, Gastón Kleiman and Greg Mann.
> 
> 
> Bugs: MESOS-8935
> https://issues.apache.org/jira/browse/MESOS-8935
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This patch adds a new master flag `min_allocatable_resources`.
> It specifies one or more resources quantities that define the
> minimum allocatable resources for the allocator. The allocator
> will only offer resources that are more than at least one of
> the specified resources.
> 
> Also fixed all related tests and updated documentation.
> 
> 
> Diffs
> -
> 
>   docs/configuration/master.md 2ba612b319f28c60e8a73455952293db81c78528 
>   include/mesos/allocator/allocator.hpp 
> 647869286d46920b98521e219ce92c3260058c35 
>   src/master/allocator/mesos/allocator.hpp 
> c453c015b234deff7efd00269da25dcec8cbf1ae 
>   src/master/allocator/mesos/hierarchical.hpp 
> e9d1742bb35004735e3cb357286b4e5b17436a5c 
>   src/master/allocator/mesos/hierarchical.cpp 
> b558228290e5ae6dbcc1b8a6e1fe69db9fd5874c 
>   src/master/flags.hpp 94b8ac2211180416190448b71ea9c81c6c0cd7fc 
>   src/master/flags.cpp cc3317ee5d740cb1d58b51ae00ceeb8d55754b9d 
>   src/master/master.cpp 5db5a8da85f02323a5654c93ac47ec4aa7e711d2 
>   src/tests/allocator.hpp 341efa665ad0ce897e087fb8d73ec50fd041d559 
>   src/tests/api_tests.cpp 85635a8662469c7dbe4e77bb8da6eb450ec8f01c 
>   src/tests/hierarchical_allocator_tests.cpp 
> c97b2ba0884a7ded867c2d80e4749de54c89b5e4 
>   src/tests/master_allocator_tests.cpp 
> e1aef8a9625a805e7ad2dfad37bfeedee82f160d 
>   src/tests/master_quota_tests.cpp 94d85c5a4b70ef2bc4e8689b896fe99f3acfd4b9 
>   src/tests/reservation_tests.cpp 7d121bf56b913c3217dec00c57f81663e9831351 
>   src/tests/resource_offers_tests.cpp 
> 54aafdb4258ad7713c5f1a59956e7f76f0e84d5b 
>   src/tests/slave_recovery_tests.cpp 2a92acc193b4db5001ae5bca53e7333ba7203210 
> 
> 
> Diff: https://reviews.apache.org/r/67513/diff/4/
> 
> 
> Testing
> ---
> 
> make check
> Fixed all existing tests.
> Dedicate test added in a subsequent patch.
> 
> 
> Thanks,
> 
> Meng Zhu
> 
>

Re: Review Request 67517: Added a test to verify `min_allocatable_resources` flag.

[Greg Mann]

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




src/tests/master_allocator_tests.cpp
Lines 86 (patched)


This isn't necessary. I'll fix while committing.


- Greg Mann


On June 20, 2018, 10:22 p.m., Meng Zhu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/67517/
> ---
> 
> (Updated June 20, 2018, 10:22 p.m.)
> 
> 
> Review request for mesos, Gastón Kleiman and Greg Mann.
> 
> 
> Bugs: MESOS-8935
> https://issues.apache.org/jira/browse/MESOS-8935
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This test verifies that the allocator honors the
> `min_allocatable_resources` flag and only offers resources
> that are more than at least one of the specified resources quantity.
> 
> 
> Diffs
> -
> 
>   src/tests/master_allocator_tests.cpp 
> e1aef8a9625a805e7ad2dfad37bfeedee82f160d 
> 
> 
> Diff: https://reviews.apache.org/r/67517/diff/5/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Meng Zhu
> 
>

Re: Review Request 67597: Fixed an issue where agent may fail to recover.

[Mesos Reviewbot]

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



Patch looks great!

Reviews applied: [67597]

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

- Mesos Reviewbot


On June 20, 2018, 1:17 p.m., bin zheng wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/67597/
> ---
> 
> (Updated June 20, 2018, 1:17 p.m.)
> 
> 
> Review request for mesos and Gilbert Song.
> 
> 
> Bugs: MESOS-8871
> https://issues.apache.org/jira/browse/MESOS-8871
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Fixed an issue where agent may fail to recover if the agent dies before image 
> store cache to the checkpoint. when images file is empty, ignore it and 
> continue.
> 
> 
> Diffs
> -
> 
>   src/slave/containerizer/mesos/provisioner/docker/metadata_manager.cpp 
> 98c8fc769f2525c66539f08e2aa82506912e8a59 
>   src/tests/containerizer/provisioner_docker_tests.cpp 
> 71247c308b205de3d20a41ceb06eed6aa70fb25d 
> 
> 
> Diff: https://reviews.apache.org/r/67597/diff/3/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> bin zheng
> 
>

Re: Review Request 66881: Added benchmark test for master metrics.

[Gilbert Song]

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

(Updated June 20, 2018, 5:08 p.m.)


Review request for mesos, Benjamin Mahler, Gastón Kleiman, Greg Mann, Jie Yu, 
and Vinod Kone.


Repository: mesos


Description
---

Added benchmark test for master metrics.


Diffs (updated)
-

  src/tests/master_benchmarks.cpp fbfffb69930c30b038f74e0b831fc0ae41c820f0 


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

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


Testing
---


Thanks,

Gilbert Song

Re: Review Request 67680: Fixed archiver failure to extract hardlinks to another directory.

[Kapil Arya]

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


Ship it!




Looks good module a minor comment. Feel free to drop if it's too much.

- Kapil Arya


On June 20, 2018, 7:55 p.m., Joseph Wu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/67680/
> ---
> 
> (Updated June 20, 2018, 7:55 p.m.)
> 
> 
> Review request for mesos, Andrew Schwartzmeyer and Kapil Arya.
> 
> 
> Bugs: MESOS-9008
> https://issues.apache.org/jira/browse/MESOS-9008
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> The archiver utility may fail to extract hardlinks when the destination
> target is different than the current working directory.  This happens
> because hardlinks are generally expressed in relative terms within
> an archive, but the target should be changed to point to the extracted
> location during extraction.
> 
> This updates the archiver utility to handle links correctly and adds
> a regression test.
> 
> 
> Diffs
> -
> 
>   3rdparty/stout/include/stout/archiver.hpp 
> f66da36de94f46c799cb1028b6e8c0ed26c9f3ae 
>   3rdparty/stout/tests/archiver_tests.cpp 
> eddda0872f02a4918cffc22ebc0da2c46b172fd9 
> 
> 
> Diff: https://reviews.apache.org/r/67680/diff/1/
> 
> 
> Testing
> ---
> 
> make check
> 
> The tarball was generated with the following:
> ```
> echo "I'm a link target" > target
> ln target hardlink
> ln -s target symlink
> tar -czf foo.tar.gz target hardlink symlink
> ```
> 
> 
> Thanks,
> 
> Joseph Wu
> 
>

Re: Review Request 67680: Fixed archiver failure to extract hardlinks to another directory.

[Kapil Arya]

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




3rdparty/stout/tests/archiver_tests.cpp
Lines 388-389 (patched)


Should we add a short comment on how the contents were generated? 
Basically, copy-paste commands from the Testing Done section of this RR?


- Kapil Arya


On June 20, 2018, 7:55 p.m., Joseph Wu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/67680/
> ---
> 
> (Updated June 20, 2018, 7:55 p.m.)
> 
> 
> Review request for mesos, Andrew Schwartzmeyer and Kapil Arya.
> 
> 
> Bugs: MESOS-9008
> https://issues.apache.org/jira/browse/MESOS-9008
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> The archiver utility may fail to extract hardlinks when the destination
> target is different than the current working directory.  This happens
> because hardlinks are generally expressed in relative terms within
> an archive, but the target should be changed to point to the extracted
> location during extraction.
> 
> This updates the archiver utility to handle links correctly and adds
> a regression test.
> 
> 
> Diffs
> -
> 
>   3rdparty/stout/include/stout/archiver.hpp 
> f66da36de94f46c799cb1028b6e8c0ed26c9f3ae 
>   3rdparty/stout/tests/archiver_tests.cpp 
> eddda0872f02a4918cffc22ebc0da2c46b172fd9 
> 
> 
> Diff: https://reviews.apache.org/r/67680/diff/1/
> 
> 
> Testing
> ---
> 
> make check
> 
> The tarball was generated with the following:
> ```
> echo "I'm a link target" > target
> ln target hardlink
> ln -s target symlink
> tar -czf foo.tar.gz target hardlink symlink
> ```
> 
> 
> Thanks,
> 
> Joseph Wu
> 
>

Review Request 67680: Fixed archiver failure to extract hardlinks to another directory.

[Joseph Wu]

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

Review request for mesos, Andrew Schwartzmeyer and Kapil Arya.


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


Repository: mesos


Description
---

The archiver utility may fail to extract hardlinks when the destination
target is different than the current working directory.  This happens
because hardlinks are generally expressed in relative terms within
an archive, but the target should be changed to point to the extracted
location during extraction.

This updates the archiver utility to handle links correctly and adds
a regression test.


Diffs
-

  3rdparty/stout/include/stout/archiver.hpp 
f66da36de94f46c799cb1028b6e8c0ed26c9f3ae 
  3rdparty/stout/tests/archiver_tests.cpp 
eddda0872f02a4918cffc22ebc0da2c46b172fd9 


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


Testing
---

make check

The tarball was generated with the following:
```
echo "I'm a link target" > target
ln target hardlink
ln -s target symlink
tar -czf foo.tar.gz target hardlink symlink
```


Thanks,

Joseph Wu

Re: Review Request 67517: Added a test to verify `min_allocatable_resources` flag.

[Mesos Reviewbot Windows]

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



PASS: Mesos patch 67517 was successfully built and tested.

Reviews applied: `['67510', '67516', '67513', '67517']`

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

- Mesos Reviewbot Windows


On June 20, 2018, 10:22 p.m., Meng Zhu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/67517/
> ---
> 
> (Updated June 20, 2018, 10:22 p.m.)
> 
> 
> Review request for mesos, Gastón Kleiman and Greg Mann.
> 
> 
> Bugs: MESOS-8935
> https://issues.apache.org/jira/browse/MESOS-8935
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This test verifies that the allocator honors the
> `min_allocatable_resources` flag and only offers resources
> that are more than at least one of the specified resources quantity.
> 
> 
> Diffs
> -
> 
>   src/tests/master_allocator_tests.cpp 
> e1aef8a9625a805e7ad2dfad37bfeedee82f160d 
> 
> 
> Diff: https://reviews.apache.org/r/67517/diff/5/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Meng Zhu
> 
>

Re: Review Request 67677: Added doc for new pull latency metrics.

[Mesos Reviewbot Windows]

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



PASS: Mesos patch 67677 was successfully built and tested.

Reviews applied: `['53105', '66875', '67677']`

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

- Mesos Reviewbot Windows


On June 20, 2018, 1:26 p.m., Zhitao Li wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/67677/
> ---
> 
> (Updated June 20, 2018, 1:26 p.m.)
> 
> 
> Review request for mesos, Jason Lai and Qian Zhang.
> 
> 
> Bugs: MESOS-6451
> https://issues.apache.org/jira/browse/MESOS-6451
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added doc for new pull latency metrics.
> 
> 
> Diffs
> -
> 
>   docs/monitoring.md 2985f68f644e84cdc5337c516ee06c95abd6018b 
> 
> 
> Diff: https://reviews.apache.org/r/67677/diff/1/
> 
> 
> Testing
> ---
> 
> https://github.com/zhitaoli/mesos/blob/public/zhitao/time_containerizer_pulls/docs/monitoring.md#containerizers
> 
> 
> Thanks,
> 
> Zhitao Li
> 
>

Re: Review Request 67597: Fixed an issue where agent may fail to recover.

[Mesos Reviewbot Windows]

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



PASS: Mesos patch 67597 was successfully built and tested.

Reviews applied: `['67597']`

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

- Mesos Reviewbot Windows


On June 20, 2018, 8:17 p.m., bin zheng wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/67597/
> ---
> 
> (Updated June 20, 2018, 8:17 p.m.)
> 
> 
> Review request for mesos and Gilbert Song.
> 
> 
> Bugs: MESOS-8871
> https://issues.apache.org/jira/browse/MESOS-8871
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Fixed an issue where agent may fail to recover if the agent dies before image 
> store cache to the checkpoint. when images file is empty, ignore it and 
> continue.
> 
> 
> Diffs
> -
> 
>   src/slave/containerizer/mesos/provisioner/docker/metadata_manager.cpp 
> 98c8fc769f2525c66539f08e2aa82506912e8a59 
>   src/tests/containerizer/provisioner_docker_tests.cpp 
> 71247c308b205de3d20a41ceb06eed6aa70fb25d 
> 
> 
> Diff: https://reviews.apache.org/r/67597/diff/3/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> bin zheng
> 
>

Re: Review Request 67517: Added a test to verify `min_allocatable_resources` flag.

[Meng Zhu]

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

(Updated June 20, 2018, 3:22 p.m.)


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


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


Repository: mesos


Description
---

This test verifies that the allocator honors the
`min_allocatable_resources` flag and only offers resources
that are more than at least one of the specified resources quantity.


Diffs (updated)
-

  src/tests/master_allocator_tests.cpp e1aef8a9625a805e7ad2dfad37bfeedee82f160d 


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

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


Testing
---

make check


Thanks,

Meng Zhu

Re: Review Request 67673: Removed an unused member variable `hierarchies` from cgroups isolator.

[Mesos Reviewbot]

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



Patch looks great!

Reviews applied: [67662, 67526, 67564, 67565, 67673]

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

- Mesos Reviewbot


On June 20, 2018, 2:33 p.m., Qian Zhang wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/67673/
> ---
> 
> (Updated June 20, 2018, 2:33 p.m.)
> 
> 
> Review request for mesos, Gilbert Song and Jason Lai.
> 
> 
> Bugs: MESOS-8327
> https://issues.apache.org/jira/browse/MESOS-8327
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Removed an unused member variable `hierarchies` from cgroups isolator.
> 
> 
> Diffs
> -
> 
>   src/slave/containerizer/mesos/isolators/cgroups/cgroups.hpp 
> 81c934318dcc2bcc9df594af0ee25f0334541a65 
>   src/slave/containerizer/mesos/isolators/cgroups/cgroups.cpp 
> 6d146729123b85e46f580a594fb9f9ac37b542f7 
> 
> 
> Diff: https://reviews.apache.org/r/67673/diff/1/
> 
> 
> Testing
> ---
> 
> sudo make check
> 
> 
> Thanks,
> 
> Qian Zhang
> 
>

Re: Review Request 67663: Made `UriDiskProfileAdaptor` be able to handle disappeared profiles.

[Chun-Hung Hsiao]

> On June 20, 2018, 1:54 p.m., Benjamin Bannier wrote:
> > src/resource_provider/storage/uri_disk_profile_adaptor.hpp
> > Lines 242 (patched)
> > 
> >
> > Is this field needed for a later change? It looks like it should be 
> > fine to just remove entries from `profileMatrix` and then drive logic with 
> > e.g., `contains` checks.
> 
> Chun-Hung Hsiao wrote:
> The profile data is kept in `profileMatrix` even for inactive profiles 
> because I'd like to have a best-effort check on the profile immutability, 
> i.e., if the profile is removed and added back, its capability and parameters 
> should remain the same.
> 
> There's a related TODO to consider checkpointing `profileMatrix` to make 
> the check more robust.

In fact, the following unit test is changed to test this: "profile" is missing 
in the second poll, but appeared in the third one (with a mutated capability) 
and the fourth one (with the same capability). The third poll is ignored but 
the fourth one is taken.


- Chun-Hung


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


On June 20, 2018, 5:32 a.m., Chun-Hung Hsiao wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/67663/
> ---
> 
> (Updated June 20, 2018, 5:32 a.m.)
> 
> 
> Review request for mesos, Benjamin Bannier, James DeFelice, Jie Yu, and 
> Joseph Wu.
> 
> 
> Bugs: MESOS-8825
> https://issues.apache.org/jira/browse/MESOS-8825
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Now the `UriDiskProfileAdaptor` module can handle disappeared profiles
> when polling the profile matrix, and performs a best-effort immutability
> check for reappeared profiles.
> 
> 
> Diffs
> -
> 
>   src/resource_provider/storage/uri_disk_profile_adaptor.hpp 
> 0484933b42d0bd66c689b06cb48f492eef7bc606 
>   src/resource_provider/storage/uri_disk_profile_adaptor.cpp 
> 614590ef7d1c0cc1df99b8e57c7fbd496793b5a7 
>   src/tests/disk_profile_adaptor_tests.cpp 
> 4485f1635f484ce6e1c7c532eedb277f5eee118b 
> 
> 
> Diff: https://reviews.apache.org/r/67663/diff/1/
> 
> 
> Testing
> ---
> 
> sudo make check
> 
> A end-to-end test will be added later in the chain.
> 
> 
> Thanks,
> 
> Chun-Hung Hsiao
> 
>

Re: Review Request 67670: Added a unit test for disappeared profiles.

[Chun-Hung Hsiao]

> On June 20, 2018, 1:14 p.m., Benjamin Bannier wrote:
> >
> 
> Benjamin Bannier wrote:
> Does it make sense to combine this patch with e.g., 
> https://reviews.apache.org/r/67663/?

This is an end-to-end test involving not just the module but other 
modifications, such as r65976, r67664 and r67669, so I prefer keep this patch 
split. I'll commit this with them atomically.


> On June 20, 2018, 1:14 p.m., Benjamin Bannier wrote:
> > src/tests/storage_local_resource_provider_tests.cpp
> > Lines 1047 (patched)
> > 
> >
> > If this number depends on e.g., the allocation interval or the number 
> > of allocation cycles we perform below, it might be worthwhile to document 
> > that. I am not sure if there's any dependency.

This should be sufficiently greater than two times the allocation interval so 
it won't be triggered when we advance the clock to get another offer.

Instead, I could add a proper synchronization to decouple this time from the 
allocation interval, e.g., returning a promise future in the mock function. Let 
me do the latter.


> On June 20, 2018, 1:14 p.m., Benjamin Bannier wrote:
> > src/tests/storage_local_resource_provider_tests.cpp
> > Lines 1058 (patched)
> > 
> >
> > Let's pass this to `StartMaster`.

Oops again.


> On June 20, 2018, 1:14 p.m., Benjamin Bannier wrote:
> > src/tests/storage_local_resource_provider_tests.cpp
> > Lines 1165-1168 (patched)
> > 
> >
> > This relies strongly on the way the master interprets a zero time, and  
> > how the allocation interval is related to the default value the master 
> > would currently use. Can we set a value calculated from 
> > `masterFlags.allocation_interval` instead to decouple this?

I prefer leaving this as is for consistency since we set 0 refuse seconds in 
other tests as well. What is the value in your mind? Like just setting it to 
`masterFlags.allocation_interval`? But then for event-based allocations the 
resources might still be refused?


- Chun-Hung


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


On June 20, 2018, 5:34 a.m., Chun-Hung Hsiao wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/67670/
> ---
> 
> (Updated June 20, 2018, 5:34 a.m.)
> 
> 
> Review request for mesos, Benjamin Bannier, Jie Yu, and Jan Schlicht.
> 
> 
> Bugs: MESOS-8995
> https://issues.apache.org/jira/browse/MESOS-8995
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> The `ROOT_ProfileDisappeared` tests if a `CREATE_VOLUME` consuming a RAW
> disk with a disappeared profile will be dropped, and if the disk space
> freed by a `DESTROY_VOLUME` destroying a volume with a disappeared
> profile will be recovered with a newly appeared profile.
> 
> 
> Diffs
> -
> 
>   src/tests/storage_local_resource_provider_tests.cpp 
> 1afe8a8e0413ef225e952cd9cd6376e5d82774e5 
> 
> 
> Diff: https://reviews.apache.org/r/67670/diff/1/
> 
> 
> Testing
> ---
> 
> sudo make check
> 
> 
> Thanks,
> 
> Chun-Hung Hsiao
> 
>

Re: Review Request 67517: Added a test to verify `min_allocatable_resources` flag.

[Greg Mann]

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


Fix it, then Ship it!





src/tests/master_allocator_tests.cpp
Lines 1281-1283 (patched)


Nit: could you make the ordering of these two lines the same here as it is 
down below for the allocatable agents?



src/tests/master_allocator_tests.cpp
Lines 1333 (patched)


Nit: `offers->size()`


- Greg Mann


On June 20, 2018, 8:40 p.m., Meng Zhu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/67517/
> ---
> 
> (Updated June 20, 2018, 8:40 p.m.)
> 
> 
> Review request for mesos, Gastón Kleiman and Greg Mann.
> 
> 
> Bugs: MESOS-8935
> https://issues.apache.org/jira/browse/MESOS-8935
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This test verifies that the allocator honors the
> `min_allocatable_resources` flag and only offers resources
> that are more than at least one of the specified resources quantity.
> 
> 
> Diffs
> -
> 
>   src/tests/master_allocator_tests.cpp 
> e1aef8a9625a805e7ad2dfad37bfeedee82f160d 
> 
> 
> Diff: https://reviews.apache.org/r/67517/diff/4/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Meng Zhu
> 
>

Re: Review Request 66875: Added an hourly timer for docker store pull latency.

[Mesos Reviewbot Windows]

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



PASS: Mesos patch 66875 was successfully built and tested.

Reviews applied: `['53105', '66875']`

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

- Mesos Reviewbot Windows


On June 20, 2018, 11:26 a.m., Zhitao Li wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/66875/
> ---
> 
> (Updated June 20, 2018, 11:26 a.m.)
> 
> 
> Review request for mesos, Gilbert Song, Jason Lai, Jie Yu, and Qian Zhang.
> 
> 
> Bugs: MESOS-6451
> https://issues.apache.org/jira/browse/MESOS-6451
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This patch added pull latency tracking for docker store, which can tell
> us both latency distribution of pull as well as number of pulls.
> 
> 
> Diffs
> -
> 
>   src/slave/containerizer/mesos/provisioner/docker/store.cpp 
> 6e7dc44321bff3198e5ffe69be1ba329be3ee31e 
> 
> 
> Diff: https://reviews.apache.org/r/66875/diff/3/
> 
> 
> Testing
> ---
> 
> Ran agent in command line and trigger several launches through 
> `mesos-execute`, observed following metrics from agent endpoint:
> 
> ```
>   "containerizer/mesos/provisioner/docker_store/image_pull_ms": 3619.53408,
>   "containerizer/mesos/provisioner/docker_store/image_pull_ms/count": 2,
>   "containerizer/mesos/provisioner/docker_store/image_pull_ms/max": 
> 4208.662016,
>   "containerizer/mesos/provisioner/docker_store/image_pull_ms/min": 
> 3619.53408,
>   "containerizer/mesos/provisioner/docker_store/image_pull_ms/p50": 
> 3914.098048,
>   "containerizer/mesos/provisioner/docker_store/image_pull_ms/p90": 
> 4149.7492224,
>   "containerizer/mesos/provisioner/docker_store/image_pull_ms/p95": 
> 4179.2056192,
>   "containerizer/mesos/provisioner/docker_store/image_pull_ms/p99": 
> 4202.77073664,
>   "containerizer/mesos/provisioner/docker_store/image_pull_ms/p999": 
> 4208.072888064,
>   "containerizer/mesos/provisioner/docker_store/image_pull_ms/p": 
> 4208.6031032064,
> ```
> 
> 
> Thanks,
> 
> Zhitao Li
> 
>

Re: Review Request 67517: Added a test to verify `min_allocatable_resources` flag.

[Meng Zhu]

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

(Updated June 20, 2018, 1:40 p.m.)


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


Changes
---

Updated tests to pause clock.


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


Repository: mesos


Description
---

This test verifies that the allocator honors the
`min_allocatable_resources` flag and only offers resources
that are more than at least one of the specified resources quantity.


Diffs (updated)
-

  src/tests/master_allocator_tests.cpp e1aef8a9625a805e7ad2dfad37bfeedee82f160d 


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

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


Testing
---

make check


Thanks,

Meng Zhu

Re: Review Request 65875: Improved logging for offers and inverse offers.

[Greg Mann]

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


Ship it!




Ship It!

- Greg Mann


On June 20, 2018, 5:37 a.m., Chun-Hung Hsiao wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/65875/
> ---
> 
> (Updated June 20, 2018, 5:37 a.m.)
> 
> 
> Review request for mesos, Alexander Rukletsov, Benjamin Bannier, Gastón 
> Kleiman, and Greg Mann.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Printed offer IDs and inverse offer IDs when sending out offers and
> inverse offers so it is easier to match them to their ACCEPT or DECLINE
> calls and removals.
> 
> 
> Diffs
> -
> 
>   src/master/master.cpp 5db5a8da85f02323a5654c93ac47ec4aa7e711d2 
> 
> 
> Diff: https://reviews.apache.org/r/65875/diff/3/
> 
> 
> Testing
> ---
> 
> sudo make check
> 
> 
> Thanks,
> 
> Chun-Hung Hsiao
> 
>

Re: Review Request 67667: Renamed the `NewProfile` SLRP test and made it based on offers.

[Chun-Hung Hsiao]

> On June 20, 2018, 12:43 p.m., Benjamin Bannier wrote:
> > src/tests/storage_local_resource_provider_tests.cpp
> > Line 529 (original), 534 (patched)
> > 
> >
> > Just wondering what actually requires `ROOT` in this test. I see we 
> > enable filesystem isolation, but is that required?

The SLRP launches the CSI plugin in a standalone container, and it seems safer 
to enable the isolation. In contrast, I could disable this isolation and 
manually do an `unmountAll` during teardown. Do you prefer the latter? If yes I 
can have a followup patch to do this.


> On June 20, 2018, 12:43 p.m., Benjamin Bannier wrote:
> > src/tests/storage_local_resource_provider_tests.cpp
> > Lines 556 (patched)
> > 
> >
> > Let's pass this into `StartMaster`.

Oops.


> On June 20, 2018, 12:43 p.m., Benjamin Bannier wrote:
> > src/tests/storage_local_resource_provider_tests.cpp
> > Lines 621 (patched)
> > 
> >
> > Formatting.

Oops again :( Thanks for catching this!


> On June 20, 2018, 12:43 p.m., Benjamin Bannier wrote:
> > src/tests/storage_local_resource_provider_tests.cpp
> > Lines 634-638 (patched)
> > 
> >
> > Let's add at least one `Clock::settle` here for consistency even though 
> > it is implicitly called in e.g., `AWAIT_READY`.

You know my opinion on this ;) But sure I'll do it for consistency.


- Chun-Hung


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


On June 20, 2018, 5:33 a.m., Chun-Hung Hsiao wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/67667/
> ---
> 
> (Updated June 20, 2018, 5:33 a.m.)
> 
> 
> Review request for mesos, Benjamin Bannier, Jie Yu, and Jan Schlicht.
> 
> 
> Bugs: MESOS-8995
> https://issues.apache.org/jira/browse/MESOS-8995
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> The test is renamed to `ProfileAppeared` to reflect the fact that the
> SLRP passively observed a profile showing up, not actively adding a
> profile.
> 
> The test is changed from intercepting `UpdateSlaveMessage`s to checking
> offers to make it more like an end-to-end test. Also with the
> `DiskProfileServer` helper we can control the clock better.
> 
> 
> Diffs
> -
> 
>   src/tests/storage_local_resource_provider_tests.cpp 
> 1afe8a8e0413ef225e952cd9cd6376e5d82774e5 
> 
> 
> Diff: https://reviews.apache.org/r/67667/diff/1/
> 
> 
> Testing
> ---
> 
> sudo make check
> 
> 
> Thanks,
> 
> Chun-Hung Hsiao
> 
>

Review Request 67677: Added doc for new pull latency metrics.

[Zhitao Li]

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

Review request for mesos, Jason Lai and Qian Zhang.


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


Repository: mesos


Description
---

Added doc for new pull latency metrics.


Diffs
-

  docs/monitoring.md 2985f68f644e84cdc5337c516ee06c95abd6018b 


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


Testing
---

https://github.com/zhitaoli/mesos/blob/public/zhitao/time_containerizer_pulls/docs/monitoring.md#containerizers


Thanks,

Zhitao Li

Re: Review Request 67597: Fixed an issue where agent may fail to recover.

[bin zheng]

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

(Updated 六月 20, 2018, 8:17 p.m.)


Review request for mesos and Gilbert Song.


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


Repository: mesos


Description (updated)
---

Fixed an issue where agent may fail to recover if the agent dies before image 
store cache to the checkpoint. when images file is empty, ignore it and 
continue.


Diffs (updated)
-

  src/slave/containerizer/mesos/provisioner/docker/metadata_manager.cpp 
98c8fc769f2525c66539f08e2aa82506912e8a59 
  src/tests/containerizer/provisioner_docker_tests.cpp 
71247c308b205de3d20a41ceb06eed6aa70fb25d 


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

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


Testing
---


Thanks,

bin zheng

Re: Review Request 65875: Improved logging for offers and inverse offers.

[Benjamin Bannier]

> On June 20, 2018, 4:35 p.m., Benjamin Bannier wrote:
> > Would it make sense to add these log lines only for some higher log level 
> > (which one)? I am mostly worried about the amount of additional log output 
> > this could generate in huge clusters, we might e.g., log tens of thousands 
> > of offerIds per offer cycle.
> 
> Chun-Hung Hsiao wrote:
> My counter argument would be that we log offer IDs for every `ACCEPT` and 
> `DECLINE` call, so the amount of log output added by this line is similar to 
> those logs.

That makes sense.


- Benjamin


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


On June 20, 2018, 7:37 a.m., Chun-Hung Hsiao wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/65875/
> ---
> 
> (Updated June 20, 2018, 7:37 a.m.)
> 
> 
> Review request for mesos, Alexander Rukletsov, Benjamin Bannier, Gastón 
> Kleiman, and Greg Mann.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Printed offer IDs and inverse offer IDs when sending out offers and
> inverse offers so it is easier to match them to their ACCEPT or DECLINE
> calls and removals.
> 
> 
> Diffs
> -
> 
>   src/master/master.cpp 5db5a8da85f02323a5654c93ac47ec4aa7e711d2 
> 
> 
> Diff: https://reviews.apache.org/r/65875/diff/3/
> 
> 
> Testing
> ---
> 
> sudo make check
> 
> 
> Thanks,
> 
> Chun-Hung Hsiao
> 
>

Re: Review Request 65875: Improved logging for offers and inverse offers.

[Benjamin Bannier]

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


Ship it!




Ship It!

- Benjamin Bannier


On June 20, 2018, 7:37 a.m., Chun-Hung Hsiao wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/65875/
> ---
> 
> (Updated June 20, 2018, 7:37 a.m.)
> 
> 
> Review request for mesos, Alexander Rukletsov, Benjamin Bannier, Gastón 
> Kleiman, and Greg Mann.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Printed offer IDs and inverse offer IDs when sending out offers and
> inverse offers so it is easier to match them to their ACCEPT or DECLINE
> calls and removals.
> 
> 
> Diffs
> -
> 
>   src/master/master.cpp 5db5a8da85f02323a5654c93ac47ec4aa7e711d2 
> 
> 
> Diff: https://reviews.apache.org/r/65875/diff/3/
> 
> 
> Testing
> ---
> 
> sudo make check
> 
> 
> Thanks,
> 
> Chun-Hung Hsiao
> 
>

Re: Review Request 67517: Added a test to verify `min_allocatable_resources` flag.

[Mesos Reviewbot Windows]

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



PASS: Mesos patch 67517 was successfully built and tested.

Reviews applied: `['67510', '67516', '67513', '67517']`

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

- Mesos Reviewbot Windows


On June 16, 2018, 12:05 a.m., Meng Zhu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/67517/
> ---
> 
> (Updated June 16, 2018, 12:05 a.m.)
> 
> 
> Review request for mesos, Gastón Kleiman and Greg Mann.
> 
> 
> Bugs: MESOS-8935
> https://issues.apache.org/jira/browse/MESOS-8935
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This test verifies that the allocator honors the
> `min_allocatable_resources` flag and only offers resources
> that are more than at least one of the specified resources quantity.
> 
> 
> Diffs
> -
> 
>   src/tests/master_allocator_tests.cpp 
> e1aef8a9625a805e7ad2dfad37bfeedee82f160d 
> 
> 
> Diff: https://reviews.apache.org/r/67517/diff/3/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Meng Zhu
> 
>

Re: Review Request 67666: Added a `TestDiskProfileServer` helper.

[Chun-Hung Hsiao]

> On June 20, 2018, 11:01 a.m., Benjamin Bannier wrote:
> > src/tests/disk_profile_server.hpp
> > Lines 34-35 (patched)
> > 
> >
> > Nit: This class doesn't really fetch, but instead provides a mock 
> > remote.

It's a helper for "module configured to fetch from HTTP URIs", not "helper to 
fetch from HTTP URIs". I copied this from Joseph's comments, but TBH its not 
nontrivial for me to parse this sentence lol. I'll simplify the comment.


- Chun-Hung


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


On June 20, 2018, 5:33 a.m., Chun-Hung Hsiao wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/67666/
> ---
> 
> (Updated June 20, 2018, 5:33 a.m.)
> 
> 
> Review request for mesos, Benjamin Bannier, Jie Yu, Joseph Wu, and Jan 
> Schlicht.
> 
> 
> Bugs: MESOS-8995
> https://issues.apache.org/jira/browse/MESOS-8995
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This helper class will be used to test functionalities related to
> profile changes.
> 
> 
> Diffs
> -
> 
>   src/Makefile.am 2bcee1e0e198e6be009174570cdaa1c8b8372a71 
>   src/tests/disk_profile_adaptor_tests.cpp 
> 4485f1635f484ce6e1c7c532eedb277f5eee118b 
>   src/tests/disk_profile_server.hpp PRE-CREATION 
> 
> 
> Diff: https://reviews.apache.org/r/67666/diff/1/
> 
> 
> Testing
> ---
> 
> sudo make check
> 
> 
> Thanks,
> 
> Chun-Hung Hsiao
> 
>

Re: Review Request 65875: Improved logging for offers and inverse offers.

[Chun-Hung Hsiao]

> On June 20, 2018, 2:35 p.m., Benjamin Bannier wrote:
> > Would it make sense to add these log lines only for some higher log level 
> > (which one)? I am mostly worried about the amount of additional log output 
> > this could generate in huge clusters, we might e.g., log tens of thousands 
> > of offerIds per offer cycle.

My counter argument would be that we log offer IDs for every `ACCEPT` and 
`DECLINE` call, so the amount of log output added by this line is similar to 
those logs.


- Chun-Hung


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


On June 20, 2018, 5:37 a.m., Chun-Hung Hsiao wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/65875/
> ---
> 
> (Updated June 20, 2018, 5:37 a.m.)
> 
> 
> Review request for mesos, Alexander Rukletsov, Benjamin Bannier, Gastón 
> Kleiman, and Greg Mann.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Printed offer IDs and inverse offer IDs when sending out offers and
> inverse offers so it is easier to match them to their ACCEPT or DECLINE
> calls and removals.
> 
> 
> Diffs
> -
> 
>   src/master/master.cpp 5db5a8da85f02323a5654c93ac47ec4aa7e711d2 
> 
> 
> Diff: https://reviews.apache.org/r/65875/diff/3/
> 
> 
> Testing
> ---
> 
> sudo make check
> 
> 
> Thanks,
> 
> Chun-Hung Hsiao
> 
>

Re: Review Request 67663: Made `UriDiskProfileAdaptor` be able to handle disappeared profiles.

[Chun-Hung Hsiao]

> On June 20, 2018, 1:54 p.m., Benjamin Bannier wrote:
> > src/resource_provider/storage/uri_disk_profile_adaptor.hpp
> > Lines 242 (patched)
> > 
> >
> > Is this field needed for a later change? It looks like it should be 
> > fine to just remove entries from `profileMatrix` and then drive logic with 
> > e.g., `contains` checks.

The profile data is kept in `profileMatrix` even for inactive profiles because 
I'd like to have a best-effort check on the profile immutability, i.e., if the 
profile is removed and added back, its capability and parameters should remain 
the same.

There's a related TODO to consider checkpointing `profileMatrix` to make the 
check more robust.


> On June 20, 2018, 1:54 p.m., Benjamin Bannier wrote:
> > src/tests/disk_profile_adaptor_tests.cpp
> > Line 687 (original), 689 (patched)
> > 
> >
> > Not added here, but this is redundant since we always `resume` the 
> > clock when tearing down a test.

Yeah I am aware of this, but some people seems to prefer explicitly resuming 
the clock still, and that's why I kept it is as. Personally I prefer shorter 
test body so I'll remove this.


- Chun-Hung


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


On June 20, 2018, 5:32 a.m., Chun-Hung Hsiao wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/67663/
> ---
> 
> (Updated June 20, 2018, 5:32 a.m.)
> 
> 
> Review request for mesos, Benjamin Bannier, James DeFelice, Jie Yu, and 
> Joseph Wu.
> 
> 
> Bugs: MESOS-8825
> https://issues.apache.org/jira/browse/MESOS-8825
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Now the `UriDiskProfileAdaptor` module can handle disappeared profiles
> when polling the profile matrix, and performs a best-effort immutability
> check for reappeared profiles.
> 
> 
> Diffs
> -
> 
>   src/resource_provider/storage/uri_disk_profile_adaptor.hpp 
> 0484933b42d0bd66c689b06cb48f492eef7bc606 
>   src/resource_provider/storage/uri_disk_profile_adaptor.cpp 
> 614590ef7d1c0cc1df99b8e57c7fbd496793b5a7 
>   src/tests/disk_profile_adaptor_tests.cpp 
> 4485f1635f484ce6e1c7c532eedb277f5eee118b 
> 
> 
> Diff: https://reviews.apache.org/r/67663/diff/1/
> 
> 
> Testing
> ---
> 
> sudo make check
> 
> A end-to-end test will be added later in the chain.
> 
> 
> Thanks,
> 
> Chun-Hung Hsiao
> 
>

Re: Review Request 66875: Added an hourly timer for docker store pull latency.

[Zhitao Li]

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

(Updated June 20, 2018, 11:26 a.m.)


Review request for mesos, Gilbert Song, Jason Lai, Jie Yu, and Qian Zhang.


Changes
---

Qian's comments.


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


Repository: mesos


Description
---

This patch added pull latency tracking for docker store, which can tell
us both latency distribution of pull as well as number of pulls.


Diffs (updated)
-

  src/slave/containerizer/mesos/provisioner/docker/store.cpp 
6e7dc44321bff3198e5ffe69be1ba329be3ee31e 


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

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


Testing (updated)
---

Ran agent in command line and trigger several launches through `mesos-execute`, 
observed following metrics from agent endpoint:

```
  "containerizer/mesos/provisioner/docker_store/image_pull_ms": 3619.53408,
  "containerizer/mesos/provisioner/docker_store/image_pull_ms/count": 2,
  "containerizer/mesos/provisioner/docker_store/image_pull_ms/max": 4208.662016,
  "containerizer/mesos/provisioner/docker_store/image_pull_ms/min": 3619.53408,
  "containerizer/mesos/provisioner/docker_store/image_pull_ms/p50": 3914.098048,
  "containerizer/mesos/provisioner/docker_store/image_pull_ms/p90": 
4149.7492224,
  "containerizer/mesos/provisioner/docker_store/image_pull_ms/p95": 
4179.2056192,
  "containerizer/mesos/provisioner/docker_store/image_pull_ms/p99": 
4202.77073664,
  "containerizer/mesos/provisioner/docker_store/image_pull_ms/p999": 
4208.072888064,
  "containerizer/mesos/provisioner/docker_store/image_pull_ms/p": 
4208.6031032064,
```


Thanks,

Zhitao Li

Re: Review Request 53105: Added an hourly timer for `containerizer/docker/image_pull`.

[Zhitao Li]

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

(Updated June 20, 2018, 11:25 a.m.)


Review request for mesos, Gilbert Song, Jason Lai, Jie Yu, and Qian Zhang.


Changes
---

Qian's comments.


Summary (updated)
-

Added an hourly timer for `containerizer/docker/image_pull`.


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


Repository: mesos


Description (updated)
---

Added an hourly timer for `containerizer/docker/image_pull`.


Diffs (updated)
-

  src/slave/containerizer/docker.hpp 1ed47c85f765da6fb91a2a243fcc5030e9686190 
  src/slave/containerizer/docker.cpp f83036aa5a212d1d88e6f5aa41023abc125f7116 


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

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


Testing (updated)
---

Ran `mesos-execute` with a docker image and docker containerizer twice, 
observed the following metrics:
```
curl -s localhost:5051/metrics/snapshot | jq .
  "containerizer/docker/image_pull_ms": 6628.833024,
  "containerizer/docker/image_pull_ms/count": 2,
  "containerizer/docker/image_pull_ms/max": 8851.497216,
  "containerizer/docker/image_pull_ms/min": 6628.833024,
  "containerizer/docker/image_pull_ms/p50": 7740.16512,
  "containerizer/docker/image_pull_ms/p90": 8629.2307968,
  "containerizer/docker/image_pull_ms/p95": 8740.3640064,
  "containerizer/docker/image_pull_ms/p99": 8829.27057408,
  "containerizer/docker/image_pull_ms/p999": 8849.274551808,
  "containerizer/docker/image_pull_ms/p": 8851.2749495808,
```


Thanks,

Zhitao Li

Re: Review Request 67517: Added a test to verify `min_allocatable_resources` flag.

[Meng Zhu]

> On June 19, 2018, 4:08 p.m., Greg Mann wrote:
> > src/tests/master_allocator_tests.cpp
> > Lines 1270 (patched)
> > 
> >
> > Is there a reason you don't do
> > 
> > ```
> > flags.resources = resourcesString;
> > ```
> > 
> > ??

Ah, I think I copied from other places. Updated.


> On June 19, 2018, 4:08 p.m., Greg Mann wrote:
> > src/tests/master_allocator_tests.cpp
> > Lines 1279-1281 (patched)
> > 
> >
> > Could you leave a comment explaining why this is necessary?
> > 
> > Is there a reason we can't pause the clock for the entire duration of 
> > the test?

Done.


- Meng


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


On June 15, 2018, 3:05 p.m., Meng Zhu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/67517/
> ---
> 
> (Updated June 15, 2018, 3:05 p.m.)
> 
> 
> Review request for mesos, Gastón Kleiman and Greg Mann.
> 
> 
> Bugs: MESOS-8935
> https://issues.apache.org/jira/browse/MESOS-8935
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This test verifies that the allocator honors the
> `min_allocatable_resources` flag and only offers resources
> that are more than at least one of the specified resources quantity.
> 
> 
> Diffs
> -
> 
>   src/tests/master_allocator_tests.cpp 
> e1aef8a9625a805e7ad2dfad37bfeedee82f160d 
> 
> 
> Diff: https://reviews.apache.org/r/67517/diff/2/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Meng Zhu
> 
>

Re: Review Request 67513: Added a master flag to configure minimum allocatable resources.

[Meng Zhu]

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

(Updated June 20, 2018, 10:52 a.m.)


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


Changes
---

Updated indentation and removed redundant move().


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


Repository: mesos


Description (updated)
---

This patch adds a new master flag `min_allocatable_resources`.
It specifies one or more resources quantities that define the
minimum allocatable resources for the allocator. The allocator
will only offer resources that are more than at least one of
the specified resources.

Also fixed all related tests and updated documentation.


Diffs (updated)
-

  docs/configuration/master.md 2ba612b319f28c60e8a73455952293db81c78528 
  include/mesos/allocator/allocator.hpp 
647869286d46920b98521e219ce92c3260058c35 
  src/master/allocator/mesos/allocator.hpp 
c453c015b234deff7efd00269da25dcec8cbf1ae 
  src/master/allocator/mesos/hierarchical.hpp 
e9d1742bb35004735e3cb357286b4e5b17436a5c 
  src/master/allocator/mesos/hierarchical.cpp 
b558228290e5ae6dbcc1b8a6e1fe69db9fd5874c 
  src/master/flags.hpp 94b8ac2211180416190448b71ea9c81c6c0cd7fc 
  src/master/flags.cpp cc3317ee5d740cb1d58b51ae00ceeb8d55754b9d 
  src/master/master.cpp 5db5a8da85f02323a5654c93ac47ec4aa7e711d2 
  src/tests/allocator.hpp 341efa665ad0ce897e087fb8d73ec50fd041d559 
  src/tests/api_tests.cpp 85635a8662469c7dbe4e77bb8da6eb450ec8f01c 
  src/tests/hierarchical_allocator_tests.cpp 
c97b2ba0884a7ded867c2d80e4749de54c89b5e4 
  src/tests/master_allocator_tests.cpp e1aef8a9625a805e7ad2dfad37bfeedee82f160d 
  src/tests/master_quota_tests.cpp 94d85c5a4b70ef2bc4e8689b896fe99f3acfd4b9 
  src/tests/reservation_tests.cpp 7d121bf56b913c3217dec00c57f81663e9831351 
  src/tests/resource_offers_tests.cpp 54aafdb4258ad7713c5f1a59956e7f76f0e84d5b 
  src/tests/slave_recovery_tests.cpp 2a92acc193b4db5001ae5bca53e7333ba7203210 


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

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


Testing
---

make check
Fixed all existing tests.
Dedicate test added in a subsequent patch.


Thanks,

Meng Zhu

Re: Review Request 53105: Added an hourly timer for `slave/docker_containerizer/pull`.

[Zhitao Li]

> On June 20, 2018, 12:58 a.m., Qian Zhang wrote:
> > src/slave/containerizer/docker.hpp
> > Lines 140 (patched)
> > 
> >
> > It seems the convention in Mesos is to implement a `struct Metrics` for 
> > all the metrics rather than directly adding the metric here. And the name 
> > of the metric should be in underscore_case instead of camelCase, so I would 
> > suggest to name it `image_pull("containerizer/docker/image_pull", 
> > Hours(1))`.
> > 
> > And is 1 hour too short for this metric?

Ack for name and metrics struct

On window: In our operational experience, if something goes wrong on the docker 
registry side, we typicaly detect that within an hour or so, so I would not 
make this much longer and lose sensitivity. If anyone needs a longer view, they 
can usually pipe the metric into another time series database and perform out 
of bound aggregation from there.


- Zhitao


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


On May 21, 2018, 10:16 a.m., Zhitao Li wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/53105/
> ---
> 
> (Updated May 21, 2018, 10:16 a.m.)
> 
> 
> Review request for mesos, Gilbert Song, Jason Lai, Jie Yu, and Qian Zhang.
> 
> 
> Bugs: MESOS-6451
> https://issues.apache.org/jira/browse/MESOS-6451
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added an hourly timer for `slave/docker_containerizer/pull`.
> 
> 
> Diffs
> -
> 
>   src/slave/containerizer/docker.hpp 6bdf4c7f60384473927de437a061fafe94170557 
>   src/slave/containerizer/docker.cpp a4c9c10e91e75f406329cbb2086f39b549cbeed0 
> 
> 
> Diff: https://reviews.apache.org/r/53105/diff/5/
> 
> 
> Testing
> ---
> 
> Ran `mesos-execute` with a docker image and docker containerizer, observed 
> the following metrics:
> ```
> curl -s localhost:5051/metrics/snapshot | jq . | grep pull
>   "containerizer/docker/pull_ms/p999": 26209.36173824,
>   "containerizer/docker/pull_ms/p90": 21036.405248,
>   "containerizer/docker/pull_ms/p": 26256.388615424,
>   "containerizer/docker/pull_ms/p50": 135.570944,
>   "containerizer/docker/pull_ms/max": 26261.613824,
>   "containerizer/docker/pull_ms/p95": 23649.009536,
>   "containerizer/docker/pull_ms/min": 103.215872,
>   "containerizer/docker/pull_ms/p99": 25739.0929664,
>   "containerizer/docker/pull_ms": 26261.613824,
>   "containerizer/docker/pull_ms/count": 3
> ```
> 
> 
> Thanks,
> 
> Zhitao Li
> 
>

Re: Review Request 67391: Windows: Added CMake `ENABLE_LIBWINIO` flag for Windows IOCP backend.

[Andrew Schwartzmeyer]

> On June 8, 2018, 3:58 p.m., Andrew Schwartzmeyer wrote:
> > 3rdparty/libprocess/include/process/io.hpp
> > Lines 29-39 (original), 29-41 (patched)
> > 
> >
> > Should we re-use the header guard like this, or add a define to CMake?
> 
> Akash Gupta wrote:
> What's the difference between the two? I don't know enough about CMake.

IIRC the pattern is usually something like `option(ENABLE_LIBWINIO ...)`, `if 
(ENABLE_LIBWINIO) target_compile_definitions(libprocess PRIVATE 
ENABLE_LIBWINIO)` and then `#ifdef ENABLE_LIBWINIO`, rather than the side 
effect of the header guard getting defined when the files are included. Your 
way works too, and even seems simpler, just wondering if it'll confuse anyone.


> On June 8, 2018, 3:58 p.m., Andrew Schwartzmeyer wrote:
> > 3rdparty/libprocess/src/CMakeLists.txt
> > Lines 86-87 (patched)
> > 
> >
> > There shouldn't be headers in CMake source lists... I'm not sure why 
> > the others are in here either...
> 
> Akash Gupta wrote:
> Are they for the "private" headers? The headers I see here are all in 
> process/src/*.hpp.

I fixed it upstream; when you rebase you can delete these headers from CMake.


- Andrew


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


On May 30, 2018, 11:42 a.m., Akash Gupta wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/67391/
> ---
> 
> (Updated May 30, 2018, 11:42 a.m.)
> 
> 
> Review request for mesos, Andrew Schwartzmeyer, Benjamin Mahler, Eric Mumau, 
> John Kordich, Joseph Wu, and Radhika Jandhyala.
> 
> 
> Bugs: MESOS-8668
> https://issues.apache.org/jira/browse/MESOS-8668
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added Windows IOCP backend to the build system. Now, there are two async
> backends, which are libevent through `ENABLE_LIBEVENT` and the Windows
> IOCP (libwinio) through `ENABLE_LIBWINIO`.
> 
> 
> Diffs
> -
> 
>   3rdparty/CMakeLists.txt ecb6946401d9b81c6610cf9f33dcf2caa9ff0f04 
>   3rdparty/libprocess/include/process/io.hpp 
> cc2caf44e065bed40263f3820e95a4f7c378bb98 
>   3rdparty/libprocess/src/CMakeLists.txt 
> cf443dffd0663ecf02b7efd6f7094175b94aae19 
>   cmake/CompilationConfigure.cmake 2f92acb3a140faa48e3639c7000be7f43020ad7d 
> 
> 
> Diff: https://reviews.apache.org/r/67391/diff/1/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Akash Gupta
> 
>

Re: Review Request 67390: Windows: Integrated libwinio with libprocess code.

[Andrew Schwartzmeyer]

> On June 8, 2018, 3:46 p.m., Andrew Schwartzmeyer wrote:
> > 3rdparty/libprocess/src/libwinio_io.cpp
> > Lines 78-101 (patched)
> > 
> >
> > Wait... doesn't this belong in the `prepare_async` from earlier? Did 
> > something get moved?
> 
> Akash Gupta wrote:
> Hm what do you mean? There's two `prepare_asyncs`. One for POSIX 
> (polling), one for Windows (IOCP). This is the first time for the Windows one.

Now I know that `libprocess/src/io.cpp` isn't POSIX only... and it has:

```
Try prepare_async(int_fd fd)
{
  // TODO(akagup): Add windows iocp.
  return os::nonblock(fd);
}


Try is_async(int_fd fd)
{
  // TODO(akagup): Add windows iocp.
  return os::isNonblock(fd);
}
```

added in https://reviews.apache.org/r/67386/


> On June 8, 2018, 3:46 p.m., Andrew Schwartzmeyer wrote:
> > 3rdparty/libprocess/src/libwinio_socket.cpp
> > Lines 105 (patched)
> > 
> >
> > What is it, and why?
> 
> Akash Gupta wrote:
> It bascially combines a lot of small packets into a single big packet, so 
> it can improve throughout at the expense of latency. I only disable it 
> because the POSIX `PollSocketImpl` disables it. I think the reason why it is 
> disabled is because serving libprocess HTTP requets prefer latency over 
> throughout.

Gotcha. Do they have anything more than this comment in their implementation? 
Maybe we could check the blame and find an issue...


- Andrew


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


On May 30, 2018, 11:46 a.m., Akash Gupta wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/67390/
> ---
> 
> (Updated May 30, 2018, 11:46 a.m.)
> 
> 
> Review request for mesos, Andrew Schwartzmeyer, Benjamin Mahler, Eric Mumau, 
> John Kordich, Joseph Wu, and Radhika Jandhyala.
> 
> 
> Bugs: MESOS-8668, MESOS-8671 and MESOS-8672
> https://issues.apache.org/jira/browse/MESOS-8668
> https://issues.apache.org/jira/browse/MESOS-8671
> https://issues.apache.org/jira/browse/MESOS-8672
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> The libprocess eventloops are used to implement the `EventLoop`,
> SocketImpl` and `io::read/write` interfaces. Thus, by providing those
> implementations using libwinio, libprocess now supports the libwinio
> backend.
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/src/libwinio.hpp PRE-CREATION 
>   3rdparty/libprocess/src/libwinio_eventloop.cpp PRE-CREATION 
>   3rdparty/libprocess/src/libwinio_io.cpp PRE-CREATION 
>   3rdparty/libprocess/src/libwinio_socket.cpp PRE-CREATION 
> 
> 
> Diff: https://reviews.apache.org/r/67390/diff/1/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Akash Gupta
> 
>

Re: Review Request 67389: Windows: Implemented Windows IOCP async backend.

[Andrew Schwartzmeyer]

> On June 8, 2018, 2:28 p.m., Andrew Schwartzmeyer wrote:
> > 3rdparty/libprocess/src/libwinio_impl.cpp
> > Lines 91-119 (patched)
> > 
> >
> > Would this be cleaner with a templated struct?
> > 
> > ```
> > template 
> > struct OverlappedIO
> > {
> >   Promise* promise;
> >   OVERLAPPED overlapped;
> >   HANDLE handle;
> > };
> > 
> > struct OverlappedReadWrite : OverlappedIO {};
> > 
> > struct OverlappedAccept : OverlappedIO
> > {
> >   unsigned char buf[sizeof(SOCKADDR_STORAGE) + 16];
> > };
> > ```
> > 
> > Since they _all_ hold a `Promise promise`.
> 
> Akash Gupta wrote:
> Yeha that could work. You still need the IOType to determine what 
> Overlapped object you actually got in the IOCP loop since that is only 
> determined at runtime.
> 
> Akash Gupta wrote:
> Eh I realized why I didn't used templates. Since you need to use the 
> IOType to determine what overlapped object actually got returned from 
> GetQueuedCompletionStatus, you need to cast it to a some "base" type that has 
> the IOType field. Even with templates, you need to do this, so there wasn't 
> much benefit to it.

Aw okay.


> On June 8, 2018, 2:28 p.m., Andrew Schwartzmeyer wrote:
> > 3rdparty/libprocess/src/libwinio_impl.cpp
> > Lines 137-138 (patched)
> > 
> >
> > Wait, how? `OVERLAPPED` is a Windows type, and `IOOverlappedBase` is a 
> > type we defined... which has an `OVERLAPPED` field. I'm either missing 
> > something here or we're doing something weird...
> 
> Akash Gupta wrote:
> It's like C style "polymorphism". The Windows async functions only care 
> about the OVERLAPPED object, so you can put anything after the struct like 
> this:
> ```
> struct X {
>   OVERLAPPED overlapped;
>   
> };
> ```
> 
> Then you can reinterpret_cast `X*` to `OVERLAPPED*` when sending it to 
> the Win32 functions and then cast same `OVERLAPPED*` pointer received from 
> the IOCP loop to `X*` again when you are in your IOCP callbacks. Yes, this is 
> super type unsafe, but IOCP is type unsafe in general and this way is 
> basically expected style: 
> https://blogs.msdn.microsoft.com/oldnewthing/20101217-00/?p=11983 
> 
> Another (type unsafe) way is to pass the `&X->overlapped` address to the 
> Win32 functions and then use the `CONTAINING_RECORD` macro to determine the 
> address of the parent structure like this:
> ```
> #define OFFSET_OF(T, m) ((size_t) &(((T*) 0)->m))) // Gets offset of 
> field m in struct T.
> #define CONTAINERING_RECORD(p, T, m) (T*) ((char*) p - OFFSET_OF(T, m)) 
> // Get the address of parent struct of given a pointer one of the child 
> members (p = &T->m).
> 
>   
>  IOOverlappedBase* overlapped = CONTAINERING_RECORD(overlapped_, 
> IOOverlappedBase, overlapped_field);
> ```

Gotcha. I hate C... add some extra comments, at least at the point this is 
first introduced :)


> On June 8, 2018, 2:28 p.m., Andrew Schwartzmeyer wrote:
> > 3rdparty/libprocess/src/libwinio_impl.cpp
> > Lines 286 (patched)
> > 
> >
> > Should we really wait infinitely? Surely there should be a timeout at 
> > some point.
> 
> Akash Gupta wrote:
> This is the event loop that handles all Mesos IO and timer events, so 
> what would we do when we time out? Exit?

I guess nothing, it just feels weird.


> On June 8, 2018, 2:28 p.m., Andrew Schwartzmeyer wrote:
> > 3rdparty/libprocess/src/libwinio_impl.cpp
> > Lines 467-468 (patched)
> > 
> >
> > Should we do any sort of `CHECK` on this `void* buf`, or is that taken 
> > care of elsewhere?
> 
> Akash Gupta wrote:
> I'm not sure what you mean here. What we would `CHECK` `void* buf` for? 
> The size is `CHECK'd` in `os::read_async`.

Sorry, I guess my question (from this snippet of code) is who allocates and 
owns the memory under `void* buf`? I'm assuming the caller allocated it before 
sending it to us, we're just hoping here that was done correctly. For instance, 
if we're given `nullptr` we might explode. I need to go look at where this is 
called...


- Andrew


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


On May 30, 2018, 11:54 a.m., Akash Gupta wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/67389/
> ---
> 
> (Updated May 30, 2018, 11:54 a.m.)
> 
> 
> Review reques

Re: Review Request 67673: Removed an unused member variable `hierarchies` from cgroups isolator.

[Mesos Reviewbot Windows]

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



PASS: Mesos patch 67673 was successfully built and tested.

Reviews applied: `['67662', '67526', '67564', '67565', '67673']`

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

- Mesos Reviewbot Windows


On June 20, 2018, 2:33 p.m., Qian Zhang wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/67673/
> ---
> 
> (Updated June 20, 2018, 2:33 p.m.)
> 
> 
> Review request for mesos, Gilbert Song and Jason Lai.
> 
> 
> Bugs: MESOS-8327
> https://issues.apache.org/jira/browse/MESOS-8327
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Removed an unused member variable `hierarchies` from cgroups isolator.
> 
> 
> Diffs
> -
> 
>   src/slave/containerizer/mesos/isolators/cgroups/cgroups.hpp 
> 81c934318dcc2bcc9df594af0ee25f0334541a65 
>   src/slave/containerizer/mesos/isolators/cgroups/cgroups.cpp 
> 6d146729123b85e46f580a594fb9f9ac37b542f7 
> 
> 
> Diff: https://reviews.apache.org/r/67673/diff/1/
> 
> 
> Testing
> ---
> 
> sudo make check
> 
> 
> Thanks,
> 
> Qian Zhang
> 
>

Re: Review Request 67671: Prevented resource providers from changing their name or type.

[Mesos Reviewbot]

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



Patch looks great!

Reviews applied: [67671]

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

- Mesos Reviewbot


On June 20, 2018, 12:35 p.m., Benjamin Bannier wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/67671/
> ---
> 
> (Updated June 20, 2018, 12:35 p.m.)
> 
> 
> Review request for mesos, Chun-Hung Hsiao and Jan Schlicht.
> 
> 
> Bugs: MESOS-8838
> https://issues.apache.org/jira/browse/MESOS-8838
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Since the agent uses e.g., a resource provider's name or type to
> construct paths to persist resource provider state, changes to this
> information on resource provider resubscription are not supported.
> 
> This patch persists a resource provider's name and type in the
> resource provider registry and rejects a resource provider
> resubscription if incompatible changes are detected. Since we did not
> persist this information previous to mesos-1.7.0 we cannot and do not
> perform validation against resource provider registry information
> stored with earlier versions of Mesos.
> 
> 
> Diffs
> -
> 
>   src/resource_provider/manager.cpp 6400e707ee834c73d4bf18c1f8d8344e8cf714cc 
>   src/resource_provider/registrar.hpp 
> ded56e1c24a1f8b8db8dc70151682a7deb9e6544 
>   src/resource_provider/registrar.cpp 
> a855a2b61fdbfc96a12a133c702ecaf7af666d8b 
>   src/resource_provider/registry.hpp 4c6c4d40686e3af8bbc7affbe3fa673479cc2fbf 
>   src/resource_provider/registry.proto 
> 491263ef679cd4cea59338b002c6845d890f8eae 
>   src/tests/resource_provider_manager_tests.cpp 
> 58bdbf0e88f59b5cb0cad42e38b24029fc0f2b41 
> 
> 
> Diff: https://reviews.apache.org/r/67671/diff/1/
> 
> 
> Testing
> ---
> 
> `sudo make check`
> 
> 
> Thanks,
> 
> Benjamin Bannier
> 
>

Re: Review Request 67501: Added authorization for storage operations.

[Benjamin Bannier]

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


Fix it, then Ship it!





docs/authorization.md
Lines 334-365 (patched)


Let's move these up to the other actions related to offer operations, e.g., 
right below `resize_volume`.



docs/authorization.md
Lines 340 (patched)


Can you use basic forms here and below (create, destroy)?



docs/csi.md
Lines 788-793 (original), 788-800 (patched)


Not sure we should mention these operation ACLs is this document concerned 
with low-level CSI setup aspects.

Suggest to just drop the changes here.



include/mesos/authorizer/authorizer.proto
Lines 261-262 (patched)


These comments are incorrect as we will set a `Resource` object. Change to 

   // `FOO_BAR` will have an object with `Resource` set.
   
Here and below.



src/authorizer/local/authorizer.cpp
Lines 730 (patched)


We might want to add the comment you also added to `acls.proto`,

// TODO(nfnt): Consider allowing granular permission to act upon
// SOME resource provider types and names.



src/master/master.hpp
Lines 901 (patched)


Let's make the first sentence here more specific, e.g.,

Returns whether the `CREATE_VOLUME` operation ...

Here and below.


- Benjamin Bannier


On June 20, 2018, 12:44 p.m., Jan Schlicht wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/67501/
> ---
> 
> (Updated June 20, 2018, 12:44 p.m.)
> 
> 
> Review request for mesos, Benjamin Bannier and Chun-Hung Hsiao.
> 
> 
> Bugs: MESOS-7329
> https://issues.apache.org/jira/browse/MESOS-7329
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Framework operations `CREATE_VOLUME`, `DESTROY_VOLUME`, `CREATE_BLOCK`,
> `DESTROY_BLOCK` are authorized. Respective ACL actions have been added
> to the local authorizer. Currently access can only be given to either
> 'ANY' or 'NONE' resource providers.
> 
> 
> Diffs
> -
> 
>   docs/authorization.md cd8622b9848b7a020c079cc1901e3933fa6eb0c0 
>   docs/csi.md 7c38fc10633aa28d012606150099ab5cc4b60cb6 
>   include/mesos/authorizer/acls.proto 
> e4889939481dabe6c1c2876a54d654f98d00dec8 
>   include/mesos/authorizer/authorizer.proto 
> bb1010d7eb97de17807b0a730ce16a4b28bc2aa3 
>   src/authorizer/local/authorizer.cpp 
> 61e9ab5ce9f1ce4eee4a3f8502c9b60140efcb7e 
>   src/master/master.hpp 4180341e2c7b16503a4376c501f611bb78ba901c 
>   src/master/master.cpp 5db5a8da85f02323a5654c93ac47ec4aa7e711d2 
>   src/tests/authorization_tests.cpp f6f77692112d2299f3009fde4468f82bfd934c60 
> 
> 
> Diff: https://reviews.apache.org/r/67501/diff/2/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Jan Schlicht
> 
>

Re: Review Request 65875: Improved logging for offers and inverse offers.

[Benjamin Bannier]

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



Would it make sense to add these log lines only for some higher log level 
(which one)? I am mostly worried about the amount of additional log output this 
could generate in huge clusters, we might e.g., log tens of thousands of 
offerIds per offer cycle.

- Benjamin Bannier


On June 20, 2018, 7:37 a.m., Chun-Hung Hsiao wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/65875/
> ---
> 
> (Updated June 20, 2018, 7:37 a.m.)
> 
> 
> Review request for mesos, Alexander Rukletsov, Benjamin Bannier, Gastón 
> Kleiman, and Greg Mann.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Printed offer IDs and inverse offer IDs when sending out offers and
> inverse offers so it is easier to match them to their ACCEPT or DECLINE
> calls and removals.
> 
> 
> Diffs
> -
> 
>   src/master/master.cpp 5db5a8da85f02323a5654c93ac47ec4aa7e711d2 
> 
> 
> Diff: https://reviews.apache.org/r/65875/diff/3/
> 
> 
> Testing
> ---
> 
> sudo make check
> 
> 
> Thanks,
> 
> Chun-Hung Hsiao
> 
>

Re: Review Request 67673: Removed an unused member variable `hierarchies` from cgroups isolator.

[Qian Zhang]

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

(Updated June 20, 2018, 10:33 p.m.)


Review request for mesos, Gilbert Song and Jason Lai.


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


Repository: mesos


Description
---

Removed an unused member variable `hierarchies` from cgroups isolator.


Diffs
-

  src/slave/containerizer/mesos/isolators/cgroups/cgroups.hpp 
81c934318dcc2bcc9df594af0ee25f0334541a65 
  src/slave/containerizer/mesos/isolators/cgroups/cgroups.cpp 
6d146729123b85e46f580a594fb9f9ac37b542f7 


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


Testing (updated)
---

sudo make check


Thanks,

Qian Zhang

Re: Review Request 67665: Made the `UriDiskProfileAdaptor` module configurable in SLRP tests.

[Benjamin Bannier]

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


Ship it!





src/tests/storage_local_resource_provider_tests.cpp
Lines 257-260 (patched)


Since we know that `Try::get` checks valid state or would abort otherwise, 
we might as well directly return the result of `strings::format(..).get()` 
above instead of adding this code.

We often follow a similar pattern elsewhere in the test code, e.g., 
`Resources::parse(..).get()`.

OTOH, what you do here is nicely explicit.


- Benjamin Bannier


On June 20, 2018, 7:33 a.m., Chun-Hung Hsiao wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/67665/
> ---
> 
> (Updated June 20, 2018, 7:33 a.m.)
> 
> 
> Review request for mesos, Benjamin Bannier, Jie Yu, Joseph Wu, and Jan 
> Schlicht.
> 
> 
> Bugs: MESOS-8995
> https://issues.apache.org/jira/browse/MESOS-8995
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This patch changes the `setupDiskProfileMapping` helper used in SLRP
> tests to `createDiskProfileMapping`, which returns a JSON string with a
> configurable profile name instead of writing the JSON into a fixed path.
> This gives us more flexibility to test adding and removing profiles. In
> addition, the `loadUriDiskProfileAdaptor` helper now accept arguments to
> set up its poll URI and interval.
> 
> The "block-default" profile is removed since it does not have value to
> test block profiles.
> 
> 
> Diffs
> -
> 
>   src/tests/storage_local_resource_provider_tests.cpp 
> 1afe8a8e0413ef225e952cd9cd6376e5d82774e5 
> 
> 
> Diff: https://reviews.apache.org/r/67665/diff/1/
> 
> 
> Testing
> ---
> 
> sudo make check
> 
> 
> Thanks,
> 
> Chun-Hung Hsiao
> 
>

Review Request 67673: Removed an unused member variable `hierarchies` from cgroups isolator.

[Qian Zhang]

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

Review request for mesos, Gilbert Song and Jason Lai.


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


Repository: mesos


Description
---

Removed an unused member variable `hierarchies` from cgroups isolator.


Diffs
-

  src/slave/containerizer/mesos/isolators/cgroups/cgroups.hpp 
81c934318dcc2bcc9df594af0ee25f0334541a65 
  src/slave/containerizer/mesos/isolators/cgroups/cgroups.cpp 
6d146729123b85e46f580a594fb9f9ac37b542f7 


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


Testing
---


Thanks,

Qian Zhang

Re: Review Request 67669: Removed an invariant check when updating the hierarchical allocator.

[Benjamin Bannier]

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




src/master/allocator/mesos/hierarchical.cpp
Line 921 (original)


While this might not be required by the allocator, generally an operation 
changing the resource quantity is not supported by the `Sorter` interface.

We already rely on the invariant being checked here in above calls to 
`Sorter::update` which explicit disallow such updates. In order to support 
operations changing the quantity of the stripped resources we need to do 
additional work.


- Benjamin Bannier


On June 20, 2018, 7:34 a.m., Chun-Hung Hsiao wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/67669/
> ---
> 
> (Updated June 20, 2018, 7:34 a.m.)
> 
> 
> Review request for mesos, Benjamin Bannier, Benjamin Mahler, Jie Yu, and Meng 
> Zhu.
> 
> 
> Bugs: MESOS-8995
> https://issues.apache.org/jira/browse/MESOS-8995
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> `HierarchicalAllocatorProcess::updateAllocation` checks that the given
> resource conversions should not change the allocation quantities of the
> specified framework. Although this is true for speculative operations,
> this might not hold for arbitrary resource conversions in general.
> 
> Since the allocator does not rely on this invariant, this patch relaxes
> the invariant to make resource conversions more future-proof.
> 
> 
> Diffs
> -
> 
>   src/master/allocator/mesos/hierarchical.cpp 
> b558228290e5ae6dbcc1b8a6e1fe69db9fd5874c 
> 
> 
> Diff: https://reviews.apache.org/r/67669/diff/1/
> 
> 
> Testing
> ---
> 
> sudo make check
> 
> 
> Thanks,
> 
> Chun-Hung Hsiao
> 
>

Re: Review Request 67671: Prevented resource providers from changing their name or type.

[Mesos Reviewbot Windows]

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



PASS: Mesos patch 67671 was successfully built and tested.

Reviews applied: `['67671']`

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

- Mesos Reviewbot Windows


On June 20, 2018, 2:35 p.m., Benjamin Bannier wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/67671/
> ---
> 
> (Updated June 20, 2018, 2:35 p.m.)
> 
> 
> Review request for mesos, Chun-Hung Hsiao and Jan Schlicht.
> 
> 
> Bugs: MESOS-8838
> https://issues.apache.org/jira/browse/MESOS-8838
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Since the agent uses e.g., a resource provider's name or type to
> construct paths to persist resource provider state, changes to this
> information on resource provider resubscription are not supported.
> 
> This patch persists a resource provider's name and type in the
> resource provider registry and rejects a resource provider
> resubscription if incompatible changes are detected. Since we did not
> persist this information previous to mesos-1.7.0 we cannot and do not
> perform validation against resource provider registry information
> stored with earlier versions of Mesos.
> 
> 
> Diffs
> -
> 
>   src/resource_provider/manager.cpp 6400e707ee834c73d4bf18c1f8d8344e8cf714cc 
>   src/resource_provider/registrar.hpp 
> ded56e1c24a1f8b8db8dc70151682a7deb9e6544 
>   src/resource_provider/registrar.cpp 
> a855a2b61fdbfc96a12a133c702ecaf7af666d8b 
>   src/resource_provider/registry.hpp 4c6c4d40686e3af8bbc7affbe3fa673479cc2fbf 
>   src/resource_provider/registry.proto 
> 491263ef679cd4cea59338b002c6845d890f8eae 
>   src/tests/resource_provider_manager_tests.cpp 
> 58bdbf0e88f59b5cb0cad42e38b24029fc0f2b41 
> 
> 
> Diff: https://reviews.apache.org/r/67671/diff/1/
> 
> 
> Testing
> ---
> 
> `sudo make check`
> 
> 
> Thanks,
> 
> Benjamin Bannier
> 
>

Re: Review Request 67663: Made `UriDiskProfileAdaptor` be able to handle disappeared profiles.

[Benjamin Bannier]

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




src/resource_provider/storage/uri_disk_profile_adaptor.hpp
Lines 242 (patched)


Is this field needed for a later change? It looks like it should be fine to 
just remove entries from `profileMatrix` and then drive logic with e.g., 
`contains` checks.



src/tests/disk_profile_adaptor_tests.cpp
Line 671 (original), 674 (patched)


Not changed here, but this `settle` seems to belong to the `ASSERT` below.



src/tests/disk_profile_adaptor_tests.cpp
Line 687 (original), 689 (patched)


Not added here, but this is redundant since we always `resume` the clock 
when tearing down a test.


- Benjamin Bannier


On June 20, 2018, 7:32 a.m., Chun-Hung Hsiao wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/67663/
> ---
> 
> (Updated June 20, 2018, 7:32 a.m.)
> 
> 
> Review request for mesos, Benjamin Bannier, James DeFelice, Jie Yu, and 
> Joseph Wu.
> 
> 
> Bugs: MESOS-8825
> https://issues.apache.org/jira/browse/MESOS-8825
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Now the `UriDiskProfileAdaptor` module can handle disappeared profiles
> when polling the profile matrix, and performs a best-effort immutability
> check for reappeared profiles.
> 
> 
> Diffs
> -
> 
>   src/resource_provider/storage/uri_disk_profile_adaptor.hpp 
> 0484933b42d0bd66c689b06cb48f492eef7bc606 
>   src/resource_provider/storage/uri_disk_profile_adaptor.cpp 
> 614590ef7d1c0cc1df99b8e57c7fbd496793b5a7 
>   src/tests/disk_profile_adaptor_tests.cpp 
> 4485f1635f484ce6e1c7c532eedb277f5eee118b 
> 
> 
> Diff: https://reviews.apache.org/r/67663/diff/1/
> 
> 
> Testing
> ---
> 
> sudo make check
> 
> A end-to-end test will be added later in the chain.
> 
> 
> Thanks,
> 
> Chun-Hung Hsiao
> 
>

Re: Review Request 67670: Added a unit test for disappeared profiles.

[Benjamin Bannier]

> On June 20, 2018, 3:14 p.m., Benjamin Bannier wrote:
> >

Does it make sense to combine this patch with e.g., 
https://reviews.apache.org/r/67663/?


- Benjamin


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


On June 20, 2018, 7:34 a.m., Chun-Hung Hsiao wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/67670/
> ---
> 
> (Updated June 20, 2018, 7:34 a.m.)
> 
> 
> Review request for mesos, Benjamin Bannier, Jie Yu, and Jan Schlicht.
> 
> 
> Bugs: MESOS-8995
> https://issues.apache.org/jira/browse/MESOS-8995
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> The `ROOT_ProfileDisappeared` tests if a `CREATE_VOLUME` consuming a RAW
> disk with a disappeared profile will be dropped, and if the disk space
> freed by a `DESTROY_VOLUME` destroying a volume with a disappeared
> profile will be recovered with a newly appeared profile.
> 
> 
> Diffs
> -
> 
>   src/tests/storage_local_resource_provider_tests.cpp 
> 1afe8a8e0413ef225e952cd9cd6376e5d82774e5 
> 
> 
> Diff: https://reviews.apache.org/r/67670/diff/1/
> 
> 
> Testing
> ---
> 
> sudo make check
> 
> 
> Thanks,
> 
> Chun-Hung Hsiao
> 
>

Re: Review Request 67501: Added authorization for storage operations.

[Mesos Reviewbot]

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



Patch looks great!

Reviews applied: [67501]

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

- Mesos Reviewbot


On June 20, 2018, 10:44 a.m., Jan Schlicht wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/67501/
> ---
> 
> (Updated June 20, 2018, 10:44 a.m.)
> 
> 
> Review request for mesos, Benjamin Bannier and Chun-Hung Hsiao.
> 
> 
> Bugs: MESOS-7329
> https://issues.apache.org/jira/browse/MESOS-7329
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Framework operations `CREATE_VOLUME`, `DESTROY_VOLUME`, `CREATE_BLOCK`,
> `DESTROY_BLOCK` are authorized. Respective ACL actions have been added
> to the local authorizer. Currently access can only be given to either
> 'ANY' or 'NONE' resource providers.
> 
> 
> Diffs
> -
> 
>   docs/authorization.md cd8622b9848b7a020c079cc1901e3933fa6eb0c0 
>   docs/csi.md 7c38fc10633aa28d012606150099ab5cc4b60cb6 
>   include/mesos/authorizer/acls.proto 
> e4889939481dabe6c1c2876a54d654f98d00dec8 
>   include/mesos/authorizer/authorizer.proto 
> bb1010d7eb97de17807b0a730ce16a4b28bc2aa3 
>   src/authorizer/local/authorizer.cpp 
> 61e9ab5ce9f1ce4eee4a3f8502c9b60140efcb7e 
>   src/master/master.hpp 4180341e2c7b16503a4376c501f611bb78ba901c 
>   src/master/master.cpp 5db5a8da85f02323a5654c93ac47ec4aa7e711d2 
>   src/tests/authorization_tests.cpp f6f77692112d2299f3009fde4468f82bfd934c60 
> 
> 
> Diff: https://reviews.apache.org/r/67501/diff/2/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Jan Schlicht
> 
>

Re: Review Request 67670: Added a unit test for disappeared profiles.

[Benjamin Bannier]

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




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


If this number depends on e.g., the allocation interval or the number of 
allocation cycles we perform below, it might be worthwhile to document that. I 
am not sure if there's any dependency.



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


Let's pass this to `StartMaster`.



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


Why is this required? It looks like this is the only driver for making this 
a `ROOT` test. If it is required we should document why; else let's make this a 
non-`ROOT` test.

nit: We can remove the empty line below.



src/tests/storage_local_resource_provider_tests.cpp
Lines 1165-1168 (patched)


This relies strongly on the way the master interprets a zero time, and  how 
the allocation interval is related to the default value the master would 
currently use. Can we set a value calculated from 
`masterFlags.allocation_interval` instead to decouple this?



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


Let's `ASSERT` here that we were offered the 4GB `disk` we expect.



src/tests/storage_local_resource_provider_tests.cpp
Lines 1204-1205 (patched)


The point here seems to be to simulate a situation where an agent update 
and an operation cross each other's paths between master and agent (and them 
being incompatible).

Also: `s/hold/held/`



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


`storagePool.disk().get()`?


- Benjamin Bannier


On June 20, 2018, 7:34 a.m., Chun-Hung Hsiao wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/67670/
> ---
> 
> (Updated June 20, 2018, 7:34 a.m.)
> 
> 
> Review request for mesos, Benjamin Bannier, Jie Yu, and Jan Schlicht.
> 
> 
> Bugs: MESOS-8995
> https://issues.apache.org/jira/browse/MESOS-8995
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> The `ROOT_ProfileDisappeared` tests if a `CREATE_VOLUME` consuming a RAW
> disk with a disappeared profile will be dropped, and if the disk space
> freed by a `DESTROY_VOLUME` destroying a volume with a disappeared
> profile will be recovered with a newly appeared profile.
> 
> 
> Diffs
> -
> 
>   src/tests/storage_local_resource_provider_tests.cpp 
> 1afe8a8e0413ef225e952cd9cd6376e5d82774e5 
> 
> 
> Diff: https://reviews.apache.org/r/67670/diff/1/
> 
> 
> Testing
> ---
> 
> sudo make check
> 
> 
> Thanks,
> 
> Chun-Hung Hsiao
> 
>

Re: Review Request 67667: Renamed the `NewProfile` SLRP test and made it based on offers.

[Benjamin Bannier]

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


Fix it, then Ship it!





src/tests/storage_local_resource_provider_tests.cpp
Line 529 (original), 534 (patched)


Just wondering what actually requires `ROOT` in this test. I see we enable 
filesystem isolation, but is that required?



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


Let's pass this into `StartMaster`.



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


Formatting.



src/tests/storage_local_resource_provider_tests.cpp
Lines 634-638 (patched)


Let's add at least one `Clock::settle` here for consistency even though it 
is implicitly called in e.g., `AWAIT_READY`.


- Benjamin Bannier


On June 20, 2018, 7:33 a.m., Chun-Hung Hsiao wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/67667/
> ---
> 
> (Updated June 20, 2018, 7:33 a.m.)
> 
> 
> Review request for mesos, Benjamin Bannier, Jie Yu, and Jan Schlicht.
> 
> 
> Bugs: MESOS-8995
> https://issues.apache.org/jira/browse/MESOS-8995
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> The test is renamed to `ProfileAppeared` to reflect the fact that the
> SLRP passively observed a profile showing up, not actively adding a
> profile.
> 
> The test is changed from intercepting `UpdateSlaveMessage`s to checking
> offers to make it more like an end-to-end test. Also with the
> `DiskProfileServer` helper we can control the clock better.
> 
> 
> Diffs
> -
> 
>   src/tests/storage_local_resource_provider_tests.cpp 
> 1afe8a8e0413ef225e952cd9cd6376e5d82774e5 
> 
> 
> Diff: https://reviews.apache.org/r/67667/diff/1/
> 
> 
> Testing
> ---
> 
> sudo make check
> 
> 
> Thanks,
> 
> Chun-Hung Hsiao
> 
>

Review Request 67671: Prevented resource providers from changing their name or type.

[Benjamin Bannier]

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

Review request for mesos, Chun-Hung Hsiao and Jan Schlicht.


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


Repository: mesos


Description
---

Since the agent uses e.g., a resource provider's name or type to
construct paths to persist resource provider state, changes to this
information on resource provider resubscription are not supported.

This patch persists a resource provider's name and type in the
resource provider registry and rejects a resource provider
resubscription if incompatible changes are detected. Since we did not
persist this information previous to mesos-1.7.0 we cannot and do not
perform validation against resource provider registry information
stored with earlier versions of Mesos.


Diffs
-

  src/resource_provider/manager.cpp 6400e707ee834c73d4bf18c1f8d8344e8cf714cc 
  src/resource_provider/registrar.hpp ded56e1c24a1f8b8db8dc70151682a7deb9e6544 
  src/resource_provider/registrar.cpp a855a2b61fdbfc96a12a133c702ecaf7af666d8b 
  src/resource_provider/registry.hpp 4c6c4d40686e3af8bbc7affbe3fa673479cc2fbf 
  src/resource_provider/registry.proto 491263ef679cd4cea59338b002c6845d890f8eae 
  src/tests/resource_provider_manager_tests.cpp 
58bdbf0e88f59b5cb0cad42e38b24029fc0f2b41 


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


Testing
---

`sudo make check`


Thanks,

Benjamin Bannier

Re: Review Request 67501: Added authorization for storage operations.

[Mesos Reviewbot Windows]

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



PASS: Mesos patch 67501 was successfully built and tested.

Reviews applied: `['67501']`

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

- Mesos Reviewbot Windows


On June 20, 2018, 10:44 a.m., Jan Schlicht wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/67501/
> ---
> 
> (Updated June 20, 2018, 10:44 a.m.)
> 
> 
> Review request for mesos, Benjamin Bannier and Chun-Hung Hsiao.
> 
> 
> Bugs: MESOS-7329
> https://issues.apache.org/jira/browse/MESOS-7329
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Framework operations `CREATE_VOLUME`, `DESTROY_VOLUME`, `CREATE_BLOCK`,
> `DESTROY_BLOCK` are authorized. Respective ACL actions have been added
> to the local authorizer. Currently access can only be given to either
> 'ANY' or 'NONE' resource providers.
> 
> 
> Diffs
> -
> 
>   docs/authorization.md cd8622b9848b7a020c079cc1901e3933fa6eb0c0 
>   docs/csi.md 7c38fc10633aa28d012606150099ab5cc4b60cb6 
>   include/mesos/authorizer/acls.proto 
> e4889939481dabe6c1c2876a54d654f98d00dec8 
>   include/mesos/authorizer/authorizer.proto 
> bb1010d7eb97de17807b0a730ce16a4b28bc2aa3 
>   src/authorizer/local/authorizer.cpp 
> 61e9ab5ce9f1ce4eee4a3f8502c9b60140efcb7e 
>   src/master/master.hpp 4180341e2c7b16503a4376c501f611bb78ba901c 
>   src/master/master.cpp 5db5a8da85f02323a5654c93ac47ec4aa7e711d2 
>   src/tests/authorization_tests.cpp f6f77692112d2299f3009fde4468f82bfd934c60 
> 
> 
> Diff: https://reviews.apache.org/r/67501/diff/2/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Jan Schlicht
> 
>

Re: Review Request 67666: Added a `TestDiskProfileServer` helper.

[Benjamin Bannier]

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


Fix it, then Ship it!





src/tests/disk_profile_server.hpp
Lines 34-35 (patched)


Nit: This class doesn't really fetch, but instead provides a mock remote.



src/tests/disk_profile_server.hpp
Lines 43 (patched)


Why is this needed?



src/tests/disk_profile_server.hpp
Lines 62 (patched)


`Owned` cannot be copied (e.g., into the lambda below), we should probably 
use a `shared_ptr` here. 

Unfortunately this implementation detail would leak into the return value, 
but it is the correct thing to do I belive.



src/tests/disk_profile_server.hpp
Lines 74 (patched)


Either make this a `std::unique_ptr` or make the class non-copyable (we 
should reference MESOS-5122 in that case).


- Benjamin Bannier


On June 20, 2018, 7:33 a.m., Chun-Hung Hsiao wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/67666/
> ---
> 
> (Updated June 20, 2018, 7:33 a.m.)
> 
> 
> Review request for mesos, Benjamin Bannier, Jie Yu, Joseph Wu, and Jan 
> Schlicht.
> 
> 
> Bugs: MESOS-8995
> https://issues.apache.org/jira/browse/MESOS-8995
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This helper class will be used to test functionalities related to
> profile changes.
> 
> 
> Diffs
> -
> 
>   src/Makefile.am 2bcee1e0e198e6be009174570cdaa1c8b8372a71 
>   src/tests/disk_profile_adaptor_tests.cpp 
> 4485f1635f484ce6e1c7c532eedb277f5eee118b 
>   src/tests/disk_profile_server.hpp PRE-CREATION 
> 
> 
> Diff: https://reviews.apache.org/r/67666/diff/1/
> 
> 
> Testing
> ---
> 
> sudo make check
> 
> 
> Thanks,
> 
> Chun-Hung Hsiao
> 
>

Re: Review Request 67501: Added authorization for storage operations.

[Jan Schlicht]

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

(Updated June 20, 2018, 12:44 p.m.)


Review request for mesos, Benjamin Bannier and Chun-Hung Hsiao.


Changes
---

Renamed authorization actions.


Summary (updated)
-

Added authorization for storage operations.


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


Repository: mesos


Description
---

Framework operations `CREATE_VOLUME`, `DESTROY_VOLUME`, `CREATE_BLOCK`,
`DESTROY_BLOCK` are authorized. Respective ACL actions have been added
to the local authorizer. Currently access can only be given to either
'ANY' or 'NONE' resource providers.


Diffs (updated)
-

  docs/authorization.md cd8622b9848b7a020c079cc1901e3933fa6eb0c0 
  docs/csi.md 7c38fc10633aa28d012606150099ab5cc4b60cb6 
  include/mesos/authorizer/acls.proto e4889939481dabe6c1c2876a54d654f98d00dec8 
  include/mesos/authorizer/authorizer.proto 
bb1010d7eb97de17807b0a730ce16a4b28bc2aa3 
  src/authorizer/local/authorizer.cpp 61e9ab5ce9f1ce4eee4a3f8502c9b60140efcb7e 
  src/master/master.hpp 4180341e2c7b16503a4376c501f611bb78ba901c 
  src/master/master.cpp 5db5a8da85f02323a5654c93ac47ec4aa7e711d2 
  src/tests/authorization_tests.cpp f6f77692112d2299f3009fde4468f82bfd934c60 


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

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


Testing
---

make check


Thanks,

Jan Schlicht

Re: Review Request 67668: Fixed a bug in `TestCSIPlugin::DeleteVolume`.

[Benjamin Bannier]

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


Ship it!





src/examples/test_csi_plugin.cpp
Line 364 (original), 364 (patched)


Unrelated, but we might want to put a `CHECK` like the one on l.127 here.


- Benjamin Bannier


On June 20, 2018, 7:33 a.m., Chun-Hung Hsiao wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/67668/
> ---
> 
> (Updated June 20, 2018, 7:33 a.m.)
> 
> 
> Review request for mesos, Benjamin Bannier, Jie Yu, and Jan Schlicht.
> 
> 
> Bugs: MESOS-8995
> https://issues.apache.org/jira/browse/MESOS-8995
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> The test CSI plugin mistakenly reduce the available capacity instead of
> expanding it when deleting a volume. This patch fixes this bug.
> 
> 
> Diffs
> -
> 
>   src/examples/test_csi_plugin.cpp 9c4da8811cc260bcf3bccfea3036a7964cb75697 
> 
> 
> Diff: https://reviews.apache.org/r/67668/diff/1/
> 
> 
> Testing
> ---
> 
> sudo make check
> 
> 
> Thanks,
> 
> Chun-Hung Hsiao
> 
>

Re: Review Request 67664: Fixed a race between `UPDATE_STATE` and `UPDATE_OPERATION_STATUS`.

[Benjamin Bannier]

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




src/slave/slave.cpp
Lines 7750 (patched)


We might even extend this condition to only trigger when the update is 
terminal itself.


- Benjamin Bannier


On June 20, 2018, 7:33 a.m., Chun-Hung Hsiao wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/67664/
> ---
> 
> (Updated June 20, 2018, 7:33 a.m.)
> 
> 
> Review request for mesos, Benjamin Bannier, Greg Mann, Jie Yu, and Jan 
> Schlicht.
> 
> 
> Bugs: MESOS-9010
> https://issues.apache.org/jira/browse/MESOS-9010
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Since a resource provider and its operation status update manager run in
> different actors, the `UPDATE_OPERATION_STATUS` call of a completed
> operation may race with an `UPDATE_STATE` call.
> 
> To deal with this race, the agent should update the latest statuses of
> all completed operations received in the `UPDATE_STATE` call to avoid
> erroneously applying those operations when receiving
> `UPDATE_OPERATION_STATUS`es.
> 
> 
> Diffs
> -
> 
>   src/slave/slave.cpp 8edd652f7f410dbadaf6c2ca3736349065e4340a 
>   src/tests/slave_tests.cpp b46fb8efc524852f62428040ff958bd44e9efe9f 
> 
> 
> Diff: https://reviews.apache.org/r/67664/diff/1/
> 
> 
> Testing
> ---
> 
> sudo make check
> 
> 
> Thanks,
> 
> Chun-Hung Hsiao
> 
>

Re: Review Request 67664: Fixed a race between `UPDATE_STATE` and `UPDATE_OPERATION_STATUS`.

[Benjamin Bannier]

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


Fix it, then Ship it!





src/slave/slave.cpp
Lines 7738-7746 (patched)


Let's explicitly call out that the operation result is already reflected in 
the reported total, e.g.

// Handle operations known to both the agent and the resource provider.
//
// If an operation became terminal it is already reflected in the total
// reported by the resource provider and should not be applied again in
// e.g., the `UPDATE_OPERATION_STATUS` handler when a status update
// arrives.  Set the terminal `latest_status` here to prevent resource
// mutations elsewhere.
//
// NOTE: We only update the `latest_status` of a known operation if it
// is not terminal yet here; its `statuses` would be updated by an
// operation status update handler.


- Benjamin Bannier


On June 20, 2018, 7:33 a.m., Chun-Hung Hsiao wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/67664/
> ---
> 
> (Updated June 20, 2018, 7:33 a.m.)
> 
> 
> Review request for mesos, Benjamin Bannier, Greg Mann, Jie Yu, and Jan 
> Schlicht.
> 
> 
> Bugs: MESOS-9010
> https://issues.apache.org/jira/browse/MESOS-9010
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Since a resource provider and its operation status update manager run in
> different actors, the `UPDATE_OPERATION_STATUS` call of a completed
> operation may race with an `UPDATE_STATE` call.
> 
> To deal with this race, the agent should update the latest statuses of
> all completed operations received in the `UPDATE_STATE` call to avoid
> erroneously applying those operations when receiving
> `UPDATE_OPERATION_STATUS`es.
> 
> 
> Diffs
> -
> 
>   src/slave/slave.cpp 8edd652f7f410dbadaf6c2ca3736349065e4340a 
>   src/tests/slave_tests.cpp b46fb8efc524852f62428040ff958bd44e9efe9f 
> 
> 
> Diff: https://reviews.apache.org/r/67664/diff/1/
> 
> 
> Testing
> ---
> 
> sudo make check
> 
> 
> Thanks,
> 
> Chun-Hung Hsiao
> 
>

Re: Review Request 66875: Added an hourly timer for docker store pull latency.

[Qian Zhang]

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




src/slave/containerizer/mesos/provisioner/docker/store.cpp
Lines 86 (patched)


I'd name it 
`image_pull("containerizer/mesos/provisioner/docker_store/image_pull", 
Hours(1))`


- Qian Zhang


On May 22, 2018, 1:16 a.m., Zhitao Li wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/66875/
> ---
> 
> (Updated May 22, 2018, 1:16 a.m.)
> 
> 
> Review request for mesos, Gilbert Song, Jason Lai, Jie Yu, and Qian Zhang.
> 
> 
> Bugs: MESOS-6451
> https://issues.apache.org/jira/browse/MESOS-6451
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This patch added pull latency tracking for docker store, which can tell
> us both latency distribution of pull as well as number of pulls.
> 
> 
> Diffs
> -
> 
>   src/slave/containerizer/mesos/provisioner/docker/store.cpp 
> 8b3f07f5027cb90d4b4ed401960494709d3eda5f 
> 
> 
> Diff: https://reviews.apache.org/r/66875/diff/2/
> 
> 
> Testing
> ---
> 
> Ran agent in command line and trigger several launches through 
> `mesos-execute`, observed following metrics from agent endpoint:
> 
> ```
>   "containerizer/mesos/docker_store/pull_ms": 4084.254208,
>   "containerizer/mesos/docker_store/pull_ms/count": 2,
>   "containerizer/mesos/docker_store/pull_ms/max": 4084.254208,
>   "containerizer/mesos/docker_store/pull_ms/min": 3525.044736,
>   "containerizer/mesos/docker_store/pull_ms/p50": 3804.649472,
>   "containerizer/mesos/docker_store/pull_ms/p90": 4028.3332608,
>   "containerizer/mesos/docker_store/pull_ms/p95": 4056.2937344,
>   "containerizer/mesos/docker_store/pull_ms/p99": 4078.66211328,
>   "containerizer/mesos/docker_store/pull_ms/p999": 4083.694998528,
>   "containerizer/mesos/docker_store/pull_ms/p": 4084.1982870528,
> ```
> 
> 
> Thanks,
> 
> Zhitao Li
> 
>

Re: Review Request 53105: Added an hourly timer for `slave/docker_containerizer/pull`.

[Qian Zhang]

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




src/slave/containerizer/docker.hpp
Lines 27-28 (original), 27-30 (patched)


Should be:
```
#include 
#include 

#include 
#include 

```



src/slave/containerizer/docker.hpp
Lines 140 (patched)


It seems the convention in Mesos is to implement a `struct Metrics` for all 
the metrics rather than directly adding the metric here. And the name of the 
metric should be in underscore_case instead of camelCase, so I would suggest to 
name it `image_pull("containerizer/docker/image_pull", Hours(1))`.

And is 1 hour too short for this metric?



src/slave/containerizer/docker.cpp
Lines 448-453 (original), 448-454 (patched)


Can we do it in this way?
```
  Future future = pullTimer.time(docker->pull(
container->containerWorkDir,
image,
container->forcePullImage()));
```


- Qian Zhang


On May 22, 2018, 1:16 a.m., Zhitao Li wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/53105/
> ---
> 
> (Updated May 22, 2018, 1:16 a.m.)
> 
> 
> Review request for mesos, Gilbert Song, Jason Lai, Jie Yu, and Qian Zhang.
> 
> 
> Bugs: MESOS-6451
> https://issues.apache.org/jira/browse/MESOS-6451
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added an hourly timer for `slave/docker_containerizer/pull`.
> 
> 
> Diffs
> -
> 
>   src/slave/containerizer/docker.hpp 6bdf4c7f60384473927de437a061fafe94170557 
>   src/slave/containerizer/docker.cpp a4c9c10e91e75f406329cbb2086f39b549cbeed0 
> 
> 
> Diff: https://reviews.apache.org/r/53105/diff/5/
> 
> 
> Testing
> ---
> 
> Ran `mesos-execute` with a docker image and docker containerizer, observed 
> the following metrics:
> ```
> curl -s localhost:5051/metrics/snapshot | jq . | grep pull
>   "containerizer/docker/pull_ms/p999": 26209.36173824,
>   "containerizer/docker/pull_ms/p90": 21036.405248,
>   "containerizer/docker/pull_ms/p": 26256.388615424,
>   "containerizer/docker/pull_ms/p50": 135.570944,
>   "containerizer/docker/pull_ms/max": 26261.613824,
>   "containerizer/docker/pull_ms/p95": 23649.009536,
>   "containerizer/docker/pull_ms/min": 103.215872,
>   "containerizer/docker/pull_ms/p99": 25739.0929664,
>   "containerizer/docker/pull_ms": 26261.613824,
>   "containerizer/docker/pull_ms/count": 3
> ```
> 
> 
> Thanks,
> 
> Zhitao Li
> 
>

Re: Review Request 67670: Added a unit test for disappeared profiles.

[Mesos Reviewbot]

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



Patch looks great!

Reviews applied: [65974, 65594, 65995, 65975, 65976, 65640, 67663, 65875, 
67664, 67665, 67666, 67667, 67668, 67669, 67670]

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

- Mesos Reviewbot


On June 20, 2018, 5:34 a.m., Chun-Hung Hsiao wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/67670/
> ---
> 
> (Updated June 20, 2018, 5:34 a.m.)
> 
> 
> Review request for mesos, Benjamin Bannier, Jie Yu, and Jan Schlicht.
> 
> 
> Bugs: MESOS-8995
> https://issues.apache.org/jira/browse/MESOS-8995
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> The `ROOT_ProfileDisappeared` tests if a `CREATE_VOLUME` consuming a RAW
> disk with a disappeared profile will be dropped, and if the disk space
> freed by a `DESTROY_VOLUME` destroying a volume with a disappeared
> profile will be recovered with a newly appeared profile.
> 
> 
> Diffs
> -
> 
>   src/tests/storage_local_resource_provider_tests.cpp 
> 1afe8a8e0413ef225e952cd9cd6376e5d82774e5 
> 
> 
> Diff: https://reviews.apache.org/r/67670/diff/1/
> 
> 
> Testing
> ---
> 
> sudo make check
> 
> 
> Thanks,
> 
> Chun-Hung Hsiao
> 
>

Re: Review Request 67670: Added a unit test for disappeared profiles.

[Mesos Reviewbot Windows]

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



PASS: Mesos patch 67670 was successfully built and tested.

Reviews applied: `['65995', '65975', '65976', '65640', '67663', '65875', 
'67664', '67665', '67666', '67667', '67668', '67669', '67670']`

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

- Mesos Reviewbot Windows


On June 20, 2018, 5:34 a.m., Chun-Hung Hsiao wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/67670/
> ---
> 
> (Updated June 20, 2018, 5:34 a.m.)
> 
> 
> Review request for mesos, Benjamin Bannier, Jie Yu, and Jan Schlicht.
> 
> 
> Bugs: MESOS-8995
> https://issues.apache.org/jira/browse/MESOS-8995
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> The `ROOT_ProfileDisappeared` tests if a `CREATE_VOLUME` consuming a RAW
> disk with a disappeared profile will be dropped, and if the disk space
> freed by a `DESTROY_VOLUME` destroying a volume with a disappeared
> profile will be recovered with a newly appeared profile.
> 
> 
> Diffs
> -
> 
>   src/tests/storage_local_resource_provider_tests.cpp 
> 1afe8a8e0413ef225e952cd9cd6376e5d82774e5 
> 
> 
> Diff: https://reviews.apache.org/r/67670/diff/1/
> 
> 
> Testing
> ---
> 
> sudo make check
> 
> 
> Thanks,
> 
> Chun-Hung Hsiao
> 
>