Re: Review Request 69149: Automatically remounted read-only bind mounts.

[Mesos Reviewbot Windows]

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



PASS: Mesos patch 69149 was successfully built and tested.

Reviews applied: `['69149']`

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

- Mesos Reviewbot Windows


On Oct. 26, 2018, 10:58 p.m., James Peach wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/69149/
> ---
> 
> (Updated Oct. 26, 2018, 10:58 p.m.)
> 
> 
> Review request for mesos, Gilbert Song, Ilya Pronin, Jie Yu, and Jiang Yan Xu.
> 
> 
> Bugs: MESOS-9354
> https://issues.apache.org/jira/browse/MESOS-9354
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> To make a bind mount read-only, you have to first make the bind mount,
> then remount it with the read-only flag. This is a bit arcane, which is
> why `mount(8)` does it automatically.
> 
> This change updates `fs::mount()` to do the read-only remount
> automatically when it is making a read-only bind mount so that every
> caller doesn't have to carry special code to make it work correctly. All
> the callers that make an explicit remount are updated to simply pass
> the `MS_READONLY` flag if necessary.
> 
> 
> Diffs
> -
> 
>   src/examples/test_csi_plugin.cpp 7fa325e94304aec8927e345ebdac380105f624a8 
>   src/linux/fs.hpp 502f85c4a32d8658bdd701975dd5ac3d802d308e 
>   src/linux/fs.cpp 9055ef42edd1fb90e1026d1d603a9ba902cfc1fd 
>   src/slave/containerizer/docker.cpp 192dc29576a99fdc671bb842c01f50cd30dc20e1 
>   src/slave/containerizer/mesos/isolators/docker/volume/isolator.cpp 
> 24c9fd6beb9657b80b33dc31c2939083c1aa9110 
>   src/slave/containerizer/mesos/isolators/filesystem/linux.cpp 
> a47899cb528eef103f299def3bd3466905ac5b51 
>   src/slave/containerizer/mesos/isolators/gpu/isolator.cpp 
> dbbf92ffbe4a46cedca5b53f6ba172bfb308100e 
>   src/slave/containerizer/mesos/isolators/network/cni/cni.cpp 
> 64271dfbb5c074ad3ac8d2a64c3943d739c0fffa 
>   src/slave/containerizer/mesos/isolators/volume/host_path.cpp 
> 2e03ef50a290c046ae2b02b332d3d007b572429d 
>   src/slave/containerizer/mesos/isolators/volume/image.cpp 
> 53cbaefeef7a6e10149e241e07d6e9cb8d510fc9 
>   src/slave/containerizer/mesos/isolators/volume/sandbox_path.cpp 
> 21d9528c23d9142eccec456184b42d085b57d12c 
>   src/slave/containerizer/mesos/provisioner/backends/bind.cpp 
> 7d564dc7de3e3de8726c7fd42a301d979c4a2574 
>   src/tests/containerizer/fs_tests.cpp 
> 23cad35b5db81a70b43bec1c1dbafe008c8dd4da 
> 
> 
> Diff: https://reviews.apache.org/r/69149/diff/2/
> 
> 
> Testing
> ---
> 
> sudo make check (Fedora 28)
> 
> 
> Thanks,
> 
> James Peach
> 
>

Re: Review Request 69194: Fixed test `MesosContainerizerSlaveRecoveryTest.ResourceStatistics`.

[Mesos Reviewbot Windows]

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



PASS: Mesos patch 69194 was successfully built and tested.

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

- Mesos Reviewbot Windows


On Oct. 26, 2018, 10:34 p.m., Meng Zhu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/69194/
> ---
> 
> (Updated Oct. 26, 2018, 10:34 p.m.)
> 
> 
> Review request for mesos and Joseph Wu.
> 
> 
> Bugs: MESOS-5048
> https://issues.apache.org/jira/browse/MESOS-5048
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> `MesosContainerizerSlaveRecoveryTest.ResourceStatistics` is flaky
> due to a race between agent restarting in the test and agent
> delivering queued tasks to registered executor. If queued task is
> delivered before agent restarts, an unexpected
> `MesosContainerizerProcess::update()` would be triggered, thus
> failing the test. This patch eliminates the race by explicitly
> waiting for the `update()` call before triggering the agent restart.
> 
> 
> Diffs
> -
> 
>   src/tests/slave_recovery_tests.cpp 4bf0229a51b4cf60ddd84d15e84d6ce9fddd2608 
> 
> 
> Diff: https://reviews.apache.org/r/69194/diff/1/
> 
> 
> Testing
> ---
> 
> ran `MesosContainerizerSlaveRecoveryTest.ResourceStatistics` continuously 
> without failure.
> 
> 
> Thanks,
> 
> Meng Zhu
> 
>

Re: Review Request 69172: Added `FetcherCacheTest.LocalCachedMissing` test.

[Mesos Reviewbot]

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



Patch looks great!

Reviews applied: [69171, 69172]

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 Oct. 25, 2018, 1:47 p.m., Andrei Budnik wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/69172/
> ---
> 
> (Updated Oct. 25, 2018, 1:47 p.m.)
> 
> 
> Review request for mesos, Gilbert Song, James Peach, and Joseph Wu.
> 
> 
> Bugs: MESOS-7474
> https://issues.apache.org/jira/browse/MESOS-7474
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This test verifies that the fetcher retries downloading URI when the
> cache file is missing.
> 
> 
> Diffs
> -
> 
>   src/tests/fetcher_cache_tests.cpp 22a7c0f36fb070e51b0c02da27016e6b71c297dd 
> 
> 
> Diff: https://reviews.apache.org/r/69172/diff/1/
> 
> 
> Testing
> ---
> 
> 1. sudo make check (Fedora 25)
> 2. internal CI
> 
> 
> Thanks,
> 
> Andrei Budnik
> 
>

Re: Review Request 69110: Added task health check definitions to master API responses.

[Mesos Reviewbot Windows]

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



PASS: Mesos patch 69110 was successfully built and tested.

Reviews applied: `['69110']`

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

- Mesos Reviewbot Windows


On Oct. 26, 2018, 3:08 p.m., Greg Mann wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/69110/
> ---
> 
> (Updated Oct. 26, 2018, 3:08 p.m.)
> 
> 
> Review request for mesos, Alexander Rukletsov, Gastón Kleiman, and Vinod Kone.
> 
> 
> Bugs: MESOS-8780
> https://issues.apache.org/jira/browse/MESOS-8780
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> The Task protobuf message is updated to include the health check
> definition of a task when it is specified. Associated helpers are
> also updated along with a test which verifies that this field is
> reflected in master API responses.
> 
> 
> Diffs
> -
> 
>   include/mesos/mesos.proto 72966da75bc90d4b5d891e209e786e5326155d17 
>   include/mesos/v1/mesos.proto c6c1dae5494d9270a7a43a2d838769574ae47872 
>   src/common/http.cpp 80848aafe27eb0c35222ad236e671a32c4ab10aa 
>   src/common/protobuf_utils.cpp 77139d8a3931dc1e9e00fcea83d77d96244a34f3 
>   src/tests/api_tests.cpp 97fd0bb891235302beda37f2eb8418677317df93 
>   src/tests/health_check_tests.cpp c972b9003196b4b4272d7c34311c84c117b1ae3d 
> 
> 
> Diff: https://reviews.apache.org/r/69110/diff/6/
> 
> 
> Testing
> ---
> 
> `make check`
> 
> 
> Thanks,
> 
> Greg Mann
> 
>

Re: Review Request 69194: Fixed test `MesosContainerizerSlaveRecoveryTest.ResourceStatistics`.

[Joseph Wu]

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


Ship it!




Ship It!

- Joseph Wu


On Oct. 26, 2018, 3:34 p.m., Meng Zhu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/69194/
> ---
> 
> (Updated Oct. 26, 2018, 3:34 p.m.)
> 
> 
> Review request for mesos and Joseph Wu.
> 
> 
> Bugs: MESOS-5048
> https://issues.apache.org/jira/browse/MESOS-5048
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> `MesosContainerizerSlaveRecoveryTest.ResourceStatistics` is flaky
> due to a race between agent restarting in the test and agent
> delivering queued tasks to registered executor. If queued task is
> delivered before agent restarts, an unexpected
> `MesosContainerizerProcess::update()` would be triggered, thus
> failing the test. This patch eliminates the race by explicitly
> waiting for the `update()` call before triggering the agent restart.
> 
> 
> Diffs
> -
> 
>   src/tests/slave_recovery_tests.cpp 4bf0229a51b4cf60ddd84d15e84d6ce9fddd2608 
> 
> 
> Diff: https://reviews.apache.org/r/69194/diff/1/
> 
> 
> Testing
> ---
> 
> ran `MesosContainerizerSlaveRecoveryTest.ResourceStatistics` continuously 
> without failure.
> 
> 
> Thanks,
> 
> Meng Zhu
> 
>

Re: Review Request 69110: Added task health check definitions to master API responses.

[Gastón Kleiman]

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


Ship it!




Ship It!

- Gastón Kleiman


On Oct. 26, 2018, 3:08 p.m., Greg Mann wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/69110/
> ---
> 
> (Updated Oct. 26, 2018, 3:08 p.m.)
> 
> 
> Review request for mesos, Alexander Rukletsov, Gastón Kleiman, and Vinod Kone.
> 
> 
> Bugs: MESOS-8780
> https://issues.apache.org/jira/browse/MESOS-8780
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> The Task protobuf message is updated to include the health check
> definition of a task when it is specified. Associated helpers are
> also updated along with a test which verifies that this field is
> reflected in master API responses.
> 
> 
> Diffs
> -
> 
>   include/mesos/mesos.proto 72966da75bc90d4b5d891e209e786e5326155d17 
>   include/mesos/v1/mesos.proto c6c1dae5494d9270a7a43a2d838769574ae47872 
>   src/common/http.cpp 80848aafe27eb0c35222ad236e671a32c4ab10aa 
>   src/common/protobuf_utils.cpp 77139d8a3931dc1e9e00fcea83d77d96244a34f3 
>   src/tests/api_tests.cpp 97fd0bb891235302beda37f2eb8418677317df93 
>   src/tests/health_check_tests.cpp c972b9003196b4b4272d7c34311c84c117b1ae3d 
> 
> 
> Diff: https://reviews.apache.org/r/69110/diff/6/
> 
> 
> Testing
> ---
> 
> `make check`
> 
> 
> Thanks,
> 
> Greg Mann
> 
>

Re: Review Request 69178: Fixed test `MasterTestPrePostReservationRefinement.LaunchGroup`.

[Joseph Wu]

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


Ship it!




Ship It!

- Joseph Wu


On Oct. 26, 2018, 4 p.m., Meng Zhu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/69178/
> ---
> 
> (Updated Oct. 26, 2018, 4 p.m.)
> 
> 
> Review request for mesos and Benno Evers.
> 
> 
> Bugs: MESOS-9079
> https://issues.apache.org/jira/browse/MESOS-9079
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added the missing scheduler acknowledgment calls so that the
> two expected task status updates can be delivered.
> 
> 
> Diffs
> -
> 
>   src/tests/master_tests.cpp 1db8ed7d81acbcd8bad4b7ca77c501d1d99cc135 
> 
> 
> Diff: https://reviews.apache.org/r/69178/diff/2/
> 
> 
> Testing
> ---
> 
> run `MasterTestPrePostReservationRefinement.LaunchGroup` continously without 
> failure
> 
> 
> Thanks,
> 
> Meng Zhu
> 
>

Re: Review Request 69172: Added `FetcherCacheTest.LocalCachedMissing` test.

[Joseph Wu]

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


Ship it!




Ship It!

- Joseph Wu


On Oct. 25, 2018, 1:47 p.m., Andrei Budnik wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/69172/
> ---
> 
> (Updated Oct. 25, 2018, 1:47 p.m.)
> 
> 
> Review request for mesos, Gilbert Song, James Peach, and Joseph Wu.
> 
> 
> Bugs: MESOS-7474
> https://issues.apache.org/jira/browse/MESOS-7474
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This test verifies that the fetcher retries downloading URI when the
> cache file is missing.
> 
> 
> Diffs
> -
> 
>   src/tests/fetcher_cache_tests.cpp 22a7c0f36fb070e51b0c02da27016e6b71c297dd 
> 
> 
> Diff: https://reviews.apache.org/r/69172/diff/1/
> 
> 
> Testing
> ---
> 
> 1. sudo make check (Fedora 25)
> 2. internal CI
> 
> 
> Thanks,
> 
> Andrei Budnik
> 
>

Re: Review Request 69171: Added validation of cache files to the URI Fetcher.

[Joseph Wu]

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




src/slave/containerizer/fetcher.cpp
Lines 1030-1031 (patched)


Hum... I think this will work as intended, but the logic and comment are 
slight off.

A cache should be considered downloaded if `completion().isReady()`.

In case the download fails, `!completion.isPending()` will be true.  
However every invocation of `entry.get()->fail()` is synchronized inside the 
`FetcherProcess`.  And every time `fail()` is called, the line is followed by a 
`cache.remove(...)`, so it should be impossible to find a Failed completion 
here.

Perhaps this will be slight more consistent:
```
// The FetcherProcess will always remove a failed download
// synchronously after marking this future as failed.
CHECK(!entry.get().completion().isFailed());

// Validate the cache file, if it has been downloaded.
if (entry.get().completion().isReady()) {
  ...
```



src/slave/containerizer/fetcher.cpp
Lines 1070-1074 (original), 1083-1087 (patched)


The new case will have a similar reason to (3). Maybe something like:
```
In (4) we explicitly only validate a cache file if the future is ready (i.e 
the file has been downloaded).
```


- Joseph Wu


On Oct. 25, 2018, 1:47 p.m., Andrei Budnik wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/69171/
> ---
> 
> (Updated Oct. 25, 2018, 1:47 p.m.)
> 
> 
> Review request for mesos, Gilbert Song, James Peach, and Joseph Wu.
> 
> 
> Bugs: MESOS-7474
> https://issues.apache.org/jira/browse/MESOS-7474
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Previously, missing cache files could lead to task launch failures.
> This patch introduces validation of cache files which happens each
> time a downloaded cache entry is requested via the `get()` method.
> If validation fails, `get()` returns `None()`, so that the fetcher
> retries downloading URI. Currently, we only check the existence of
> the cache file during validation.
> 
> 
> Diffs
> -
> 
>   src/slave/containerizer/fetcher.cpp 
> e848c86261b75e5549e80276541e5932162fc012 
>   src/slave/containerizer/fetcher_process.hpp 
> 56ed6e5455b7d23b4ce84873fe6734b4213df691 
> 
> 
> Diff: https://reviews.apache.org/r/69171/diff/1/
> 
> 
> Testing
> ---
> 
> 1. sudo make check (Fedora 25)
> 2. internal CI
> 
> 
> Thanks,
> 
> Andrei Budnik
> 
>

Re: Review Request 69185: Added streaming header support for /api/v1 SUBSCRIBE.

[Mesos Reviewbot]

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



Patch looks great!

Reviews applied: [69180, 69181, 69182, 69183, 69184, 69185]

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 Oct. 26, 2018, 12:56 a.m., Joseph Wu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/69185/
> ---
> 
> (Updated Oct. 26, 2018, 12:56 a.m.)
> 
> 
> Review request for mesos, Gastón Kleiman and Greg Mann.
> 
> 
> Bugs: MESOS-7974 and MESOS-9258
> https://issues.apache.org/jira/browse/MESOS-7974
> https://issues.apache.org/jira/browse/MESOS-9258
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> The existing API allowed the client to send 'Accept' and 'Content-Type'
> headers of 'application/json' or 'application/protobuf'.
> 
> The SUBSCRIBE endpoint would also take a 'Content-Type' of those two
> types, but always return content with 'application/recordio' (because
> this is a streaming response).  This maintains that behavior while
> also allowing the user to use streaming headers correctly.
> 
> 
> Diffs
> -
> 
>   src/master/http.cpp 75ab6ea422a77e25049cd6afe007507d11650a06 
>   src/master/master.hpp ea7e9242b62fe6c2cc0e717f9a9f2f0c1cc0a390 
> 
> 
> Diff: https://reviews.apache.org/r/69185/diff/1/
> 
> 
> Testing
> ---
> 
> make
> 
> See later in chain.
> 
> 
> Thanks,
> 
> Joseph Wu
> 
>

Re: Review Request 69178: Fixed test `MasterTestPrePostReservationRefinement.LaunchGroup`.

[Meng Zhu]

> On Oct. 26, 2018, 3:30 p.m., Joseph Wu wrote:
> > src/tests/master_tests.cpp
> > Lines 9562-9566 (original), 9562-9566 (patched)
> > 
> >
> > Let's do this instead:
> > ```
> >   testing::Sequence updateSequence;
> >   Future startingUpdate;
> >   Future runningUpdate;
> > 
> >   EXPECT_CALL(
> >   *scheduler,
> >   update(_, AllOf(
> >   TaskStatusUpdateTaskIdEq(taskInfo),
> >   TaskStatusUpdateStateEq(v1::TASK_STARTING
> > .InSequence(updateSequence)
> > .WillOnce(DoAll(
> > FutureArg<1>(&startingUpdate), 
> > v1::scheduler::SendAcknowledge(frameworkId, agentId)));
> > 
> >   EXPECT_CALL(
> >   *scheduler,
> >   update(_, AllOf(
> >   TaskStatusUpdateTaskIdEq(taskInfo),
> >   TaskStatusUpdateStateEq(v1::TASK_RUNNING
> > .InSequence(updateSequence)
> > .WillOnce(DoAll(
> > FutureArg<1>(&runningUpdate),
> > v1::scheduler::SendAcknowledge(frameworkId, agentId));
> > ```

Sounds good. 

One thing though, I do not think we should `SendAcknowledge()` in the second 
expectation. We have seen this being a source flakiness where the scheduler 
teardown could race against the `send()` call. But that is not needed here 
anyway, so I will leave that part out.


- Meng


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


On Oct. 26, 2018, 4 p.m., Meng Zhu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/69178/
> ---
> 
> (Updated Oct. 26, 2018, 4 p.m.)
> 
> 
> Review request for mesos and Benno Evers.
> 
> 
> Bugs: MESOS-9079
> https://issues.apache.org/jira/browse/MESOS-9079
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added the missing scheduler acknowledgment calls so that the
> two expected task status updates can be delivered.
> 
> 
> Diffs
> -
> 
>   src/tests/master_tests.cpp 1db8ed7d81acbcd8bad4b7ca77c501d1d99cc135 
> 
> 
> Diff: https://reviews.apache.org/r/69178/diff/2/
> 
> 
> Testing
> ---
> 
> run `MasterTestPrePostReservationRefinement.LaunchGroup` continously without 
> failure
> 
> 
> Thanks,
> 
> Meng Zhu
> 
>

Re: Review Request 69178: Fixed test `MasterTestPrePostReservationRefinement.LaunchGroup`.

[Meng Zhu]

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

(Updated Oct. 26, 2018, 4 p.m.)


Review request for mesos and Benno Evers.


Changes
---

Addressed Joseph's comment.


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


Repository: mesos


Description
---

Added the missing scheduler acknowledgment calls so that the
two expected task status updates can be delivered.


Diffs (updated)
-

  src/tests/master_tests.cpp 1db8ed7d81acbcd8bad4b7ca77c501d1d99cc135 


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

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


Testing
---

run `MasterTestPrePostReservationRefinement.LaunchGroup` continously without 
failure


Thanks,

Meng Zhu

Re: Review Request 69149: Automatically remounted read-only bind mounts.

[James Peach]

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

(Updated Oct. 26, 2018, 10:58 p.m.)


Review request for mesos, Gilbert Song, Ilya Pronin, Jie Yu, and Jiang Yan Xu.


Changes
---

Addressed review feedback.


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


Repository: mesos


Description
---

To make a bind mount read-only, you have to first make the bind mount,
then remount it with the read-only flag. This is a bit arcane, which is
why `mount(8)` does it automatically.

This change updates `fs::mount()` to do the read-only remount
automatically when it is making a read-only bind mount so that every
caller doesn't have to carry special code to make it work correctly. All
the callers that make an explicit remount are updated to simply pass
the `MS_READONLY` flag if necessary.


Diffs (updated)
-

  src/examples/test_csi_plugin.cpp 7fa325e94304aec8927e345ebdac380105f624a8 
  src/linux/fs.hpp 502f85c4a32d8658bdd701975dd5ac3d802d308e 
  src/linux/fs.cpp 9055ef42edd1fb90e1026d1d603a9ba902cfc1fd 
  src/slave/containerizer/docker.cpp 192dc29576a99fdc671bb842c01f50cd30dc20e1 
  src/slave/containerizer/mesos/isolators/docker/volume/isolator.cpp 
24c9fd6beb9657b80b33dc31c2939083c1aa9110 
  src/slave/containerizer/mesos/isolators/filesystem/linux.cpp 
a47899cb528eef103f299def3bd3466905ac5b51 
  src/slave/containerizer/mesos/isolators/gpu/isolator.cpp 
dbbf92ffbe4a46cedca5b53f6ba172bfb308100e 
  src/slave/containerizer/mesos/isolators/network/cni/cni.cpp 
64271dfbb5c074ad3ac8d2a64c3943d739c0fffa 
  src/slave/containerizer/mesos/isolators/volume/host_path.cpp 
2e03ef50a290c046ae2b02b332d3d007b572429d 
  src/slave/containerizer/mesos/isolators/volume/image.cpp 
53cbaefeef7a6e10149e241e07d6e9cb8d510fc9 
  src/slave/containerizer/mesos/isolators/volume/sandbox_path.cpp 
21d9528c23d9142eccec456184b42d085b57d12c 
  src/slave/containerizer/mesos/provisioner/backends/bind.cpp 
7d564dc7de3e3de8726c7fd42a301d979c4a2574 
  src/tests/containerizer/fs_tests.cpp 23cad35b5db81a70b43bec1c1dbafe008c8dd4da 


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

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


Testing
---

sudo make check (Fedora 28)


Thanks,

James Peach

Re: Review Request 69186: Added tests for master SUBSCRIBE heartbeating.

[Mesos Reviewbot Windows]

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



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

Reviews applied: `['69180', '69181', '69182', '69183', '69184', '69185', 
'69186']`

Failed command: `Start-MesosCITesting`

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

Relevant logs:

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

```
W1026 22:56:44.519311  6088 slave.cpp:3923] Ignoring shutdown framework 
1cbdd316-57e8-41b2-975d-0a21ffdf99b0- because it is terminating
I1026 22:56:44.521322  7984 master.cpp:1284] Agent 
1cbdd316-57e8-41b2-975d-0a21ffdf99b0-S0 at slave(461)@192.10.1.8:64341 
(windows-05.aa0q4n2kgcyefckmv0xukjvy4f.xx.internal.cloudapp.net) disconnected
I1026 22:56:44.521322  7984 master.cpp:3300] Disconnecting agent 
1cbdd316-57e8-41b2-975d-0a21ffdf99b0-S0 at slave(461)@192.10.1.8:64341 
(windows-05.aa0q4n2kgcyefckmv0xukjvy4f.xx.internal.cloudapp.net)
I1026 22:56:44.521322  7984 master.cpp:3319] Deactivating agent 
1cbdd316-57e8-41b2-975d-0a21ffdf99b0-S0 at slave(461)@192.10.1.8:64341 
(windows-05.aa0q4n2kgcyefckmv0xukjvy4f.xx.internal.cloudapp.net)
I1026 22:56:44.522310  3360 hierarchical.cpp:357] Removed framework 
1cbdd316-57e8-41b2-975d-0a21ffdf99b0-
I1026 22:56:44.522310  3360 hierarchical.cpp:801] Agent 
1cbdd316-57e8-41b2-975d-0a21ffdf99b0-S0 deactivated
I1026 22:56:44.523324  9420 containerizer.cpp:2455] Destroying container 
75a86323-6c9e-4b2a-9fd3-d2e3a675e47c in RUNNING state
I1026 22:56:44.523324  9420 containerizer.cpp:3122] Transitioning the state of 
container 75a86323-6c9e-4b2a-9fd3-d2e3a675e47c from RUNNING to DESTROYING
I1026 22:56:44.523324  9420 launcher.cpp:166] Asked to destroy container 
75a86323-6c9e-4b2a-9fd3-d2e3a675e47c
W1026 22:56:44.524359 10184 process.cpp:1423] Failed to recv on socket 
WindowsFD::Type::SOCKET=4192 to peer '192.10.1.8:49786': IO failed with error 
code: The specified network name is no longer available.

W1026 22:56:44.525312 10184 process.cpp:838] Failed to recv on socket [   
OK ] IsolationFlag/MemoryIsolatorTest.ROOT_MemUsage/0 (582 ms)
[--] 1 test from IsolationFlag/MemoryIsolatorTest (600 ms total)

[--] Global test environment tear-down
[==] 1056 tests from 103 test cases ran. (483124 ms total)
[  PASSED  ] 1055 tests.
[  FAILED  ] 1 test, listed below:
[  FAILED  ] DockerFetcherPluginTest.INTERNET_CURL_InvokeFetchByName

 1 FAILED TEST
  YOU HAVE 232 DISABLED TESTS

WindowsFD::Type::SOCKET=4736 to peer '192.10.1.8:49787': IO failed with error 
code: The specified network name is no longer available.

I1026 22:56:44.539360  3360 containerizer.cpp:2961] Container 
75a86323-6c9e-4b2a-9fd3-d2e3a675e47c has exited
I1026 22:56:44.567327  7788 master.cpp:1126] Master terminating
I1026 22:56:44.568326  3360 hierarchical.cpp:643] Removed agent 
1cbdd316-57e8-41b2-975d-0a21ffdf99b0-S0
I1026 22:56:44.785321 10184 process.cpp:927] Stopped the socket accept loop
```

- Mesos Reviewbot Windows


On Oct. 26, 2018, 9:57 p.m., Joseph Wu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/69186/
> ---
> 
> (Updated Oct. 26, 2018, 9:57 p.m.)
> 
> 
> Review request for mesos, Gastón Kleiman and Greg Mann.
> 
> 
> Bugs: MESOS-9258
> https://issues.apache.org/jira/browse/MESOS-9258
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> These test client->master heartbeats.  The existing tests exercise
> master->client heartbeats.
> 
> 
> Diffs
> -
> 
>   src/tests/api_tests.cpp 97fd0bb891235302beda37f2eb8418677317df93 
> 
> 
> Diff: https://reviews.apache.org/r/69186/diff/1/
> 
> 
> Testing
> ---
> 
> ```
> make check
> 
> src/mesos-tests --gtest_filter="*MasterAPITest.*Heartbeats*" 
> --gtest_repeat=1000 --gtest_break_on_failure
> ```
> 
> 
> Thanks,
> 
> Joseph Wu
> 
>

Review Request 69194: Fixed test `MesosContainerizerSlaveRecoveryTest.ResourceStatistics`.

[Meng Zhu]

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

Review request for mesos and Joseph Wu.


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


Repository: mesos


Description
---

`MesosContainerizerSlaveRecoveryTest.ResourceStatistics` is flaky
due to a race between agent restarting in the test and agent
delivering queued tasks to registered executor. If queued task is
delivered before agent restarts, an unexpected
`MesosContainerizerProcess::update()` would be triggered, thus
failing the test. This patch eliminates the race by explicitly
waiting for the `update()` call before triggering the agent restart.


Diffs
-

  src/tests/slave_recovery_tests.cpp 4bf0229a51b4cf60ddd84d15e84d6ce9fddd2608 


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


Testing
---

ran `MesosContainerizerSlaveRecoveryTest.ResourceStatistics` continuously 
without failure.


Thanks,

Meng Zhu

Re: Review Request 69178: Fixed test `MasterTestPrePostReservationRefinement.LaunchGroup`.

[Joseph Wu]

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




src/tests/master_tests.cpp
Lines 9562-9566 (original), 9562-9566 (patched)


Let's do this instead:
```
  testing::Sequence updateSequence;
  Future startingUpdate;
  Future runningUpdate;

  EXPECT_CALL(
  *scheduler,
  update(_, AllOf(
  TaskStatusUpdateTaskIdEq(taskInfo),
  TaskStatusUpdateStateEq(v1::TASK_STARTING
.InSequence(updateSequence)
.WillOnce(DoAll(
FutureArg<1>(&startingUpdate), 
v1::scheduler::SendAcknowledge(frameworkId, agentId)));

  EXPECT_CALL(
  *scheduler,
  update(_, AllOf(
  TaskStatusUpdateTaskIdEq(taskInfo),
  TaskStatusUpdateStateEq(v1::TASK_RUNNING
.InSequence(updateSequence)
.WillOnce(DoAll(
FutureArg<1>(&runningUpdate),
v1::scheduler::SendAcknowledge(frameworkId, agentId));
```



src/tests/master_tests.cpp
Lines 9588-9598 (original), 9588-9614 (patched)


Given the above suggestion, this can be replaced with:
```
AWAIT_READY(startingUpdate);
AWAIT_READY(runningUpdate);
```


- Joseph Wu


On Oct. 25, 2018, 4:11 p.m., Meng Zhu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/69178/
> ---
> 
> (Updated Oct. 25, 2018, 4:11 p.m.)
> 
> 
> Review request for mesos and Benno Evers.
> 
> 
> Bugs: MESOS-9079
> https://issues.apache.org/jira/browse/MESOS-9079
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added the missing scheduler acknowledgment calls so that the
> two expected task status updates can be delivered.
> 
> 
> Diffs
> -
> 
>   src/tests/master_tests.cpp 1db8ed7d81acbcd8bad4b7ca77c501d1d99cc135 
> 
> 
> Diff: https://reviews.apache.org/r/69178/diff/1/
> 
> 
> Testing
> ---
> 
> run `MasterTestPrePostReservationRefinement.LaunchGroup` continously without 
> failure
> 
> 
> Thanks,
> 
> Meng Zhu
> 
>

Re: Review Request 69176: Fixed flaky test `SchedulerHttpApiTest.UpdatePidToHttpScheduler`.

[Meng Zhu]

> On Oct. 26, 2018, 2:56 p.m., Joseph Wu wrote:
> > Good catch!
> > 
> > Do you want to ship this (have your gotten your commit bits?), or shall I?

Thanks! I am still waiting for the account creation. If you can push this, that 
would be great!


- Meng


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


On Oct. 25, 2018, 4:11 p.m., Meng Zhu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/69176/
> ---
> 
> (Updated Oct. 25, 2018, 4:11 p.m.)
> 
> 
> Review request for mesos and Vinod Kone.
> 
> 
> Bugs: MESOS-8343
> https://issues.apache.org/jira/browse/MESOS-8343
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> The test was flaky due to race between scheduler driver stopping
> during test teardown and the scheduler `error()` invocation.
> Adding the missing synchronization for the expectation should
> eliminate the race.
> 
> 
> Diffs
> -
> 
>   src/tests/scheduler_http_api_tests.cpp 
> 64c000650acab30647d0c1e7d685bec954aa7118 
> 
> 
> Diff: https://reviews.apache.org/r/69176/diff/1/
> 
> 
> Testing
> ---
> 
> ran `chedulerHttpApiTest.UpdatePidToHttpScheduler` continously without 
> failure.
> 
> 
> Thanks,
> 
> Meng Zhu
> 
>

Re: Review Request 69110: Added task health check definitions to master API responses.

[Greg Mann]

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

(Updated Oct. 26, 2018, 10:08 p.m.)


Review request for mesos, Alexander Rukletsov, Gastón Kleiman, and Vinod Kone.


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


Repository: mesos


Description
---

The Task protobuf message is updated to include the health check
definition of a task when it is specified. Associated helpers are
also updated along with a test which verifies that this field is
reflected in master API responses.


Diffs (updated)
-

  include/mesos/mesos.proto 72966da75bc90d4b5d891e209e786e5326155d17 
  include/mesos/v1/mesos.proto c6c1dae5494d9270a7a43a2d838769574ae47872 
  src/common/http.cpp 80848aafe27eb0c35222ad236e671a32c4ab10aa 
  src/common/protobuf_utils.cpp 77139d8a3931dc1e9e00fcea83d77d96244a34f3 
  src/tests/api_tests.cpp 97fd0bb891235302beda37f2eb8418677317df93 
  src/tests/health_check_tests.cpp c972b9003196b4b4272d7c34311c84c117b1ae3d 


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

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


Testing
---

`make check`


Thanks,

Greg Mann

Review Request 69186: Added tests for master SUBSCRIBE heartbeating.

[Joseph Wu]

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

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


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


Repository: mesos


Description
---

These test client->master heartbeats.  The existing tests exercise
master->client heartbeats.


Diffs
-

  src/tests/api_tests.cpp 97fd0bb891235302beda37f2eb8418677317df93 


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


Testing
---

```
make check

src/mesos-tests --gtest_filter="*MasterAPITest.*Heartbeats*" 
--gtest_repeat=1000 --gtest_break_on_failure
```


Thanks,

Joseph Wu

Re: Review Request 69176: Fixed flaky test `SchedulerHttpApiTest.UpdatePidToHttpScheduler`.

[Joseph Wu]

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


Ship it!




Good catch!

Do you want to ship this (have your gotten your commit bits?), or shall I?

- Joseph Wu


On Oct. 25, 2018, 4:11 p.m., Meng Zhu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/69176/
> ---
> 
> (Updated Oct. 25, 2018, 4:11 p.m.)
> 
> 
> Review request for mesos and Vinod Kone.
> 
> 
> Bugs: MESOS-8343
> https://issues.apache.org/jira/browse/MESOS-8343
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> The test was flaky due to race between scheduler driver stopping
> during test teardown and the scheduler `error()` invocation.
> Adding the missing synchronization for the expectation should
> eliminate the race.
> 
> 
> Diffs
> -
> 
>   src/tests/scheduler_http_api_tests.cpp 
> 64c000650acab30647d0c1e7d685bec954aa7118 
> 
> 
> Diff: https://reviews.apache.org/r/69176/diff/1/
> 
> 
> Testing
> ---
> 
> ran `chedulerHttpApiTest.UpdatePidToHttpScheduler` continously without 
> failure.
> 
> 
> Thanks,
> 
> Meng Zhu
> 
>

Re: Review Request 69149: Automatically remounted read-only bind mounts.

[Ilya Pronin]

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


Fix it, then Ship it!




Looks good to me! Nice simplification.


src/linux/fs.cpp
Lines 510 (patched)


Should not be critical, but maybe check that the user is not trying to 
remount in the first place? Like checking that `!(flags & MS_REMOUNT)`?



src/linux/fs.cpp
Line 738 (original), 739-740 (patched)


Delete the old line with `MS_BIND`.



src/linux/fs.cpp
Line 752 (original), 747-748 (patched)


Ditto.


- Ilya Pronin


On Oct. 24, 2018, 4:49 p.m., James Peach wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/69149/
> ---
> 
> (Updated Oct. 24, 2018, 4:49 p.m.)
> 
> 
> Review request for mesos, Gilbert Song, Ilya Pronin, Jie Yu, and Jiang Yan Xu.
> 
> 
> Bugs: MESOS-9354
> https://issues.apache.org/jira/browse/MESOS-9354
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> To make a bind mount read-only, you have to first make the bind mount,
> then remount it with the read-only flag. This is a bit arcane, which is
> why `mount(8)` does it automatically.
> 
> This change updates `fs::mount()` to do the read-only remount
> automatically when it is making a read-only bind mount so that every
> caller doesn't have to carry special code to make it work correctly. All
> the callers that make an explicit remount are updated to simply pass
> the `MS_READONLY` flag if necessary.
> 
> 
> Diffs
> -
> 
>   src/examples/test_csi_plugin.cpp 7fa325e94304aec8927e345ebdac380105f624a8 
>   src/linux/fs.hpp 502f85c4a32d8658bdd701975dd5ac3d802d308e 
>   src/linux/fs.cpp 9055ef42edd1fb90e1026d1d603a9ba902cfc1fd 
>   src/slave/containerizer/docker.cpp 192dc29576a99fdc671bb842c01f50cd30dc20e1 
>   src/slave/containerizer/mesos/isolators/docker/volume/isolator.cpp 
> 24c9fd6beb9657b80b33dc31c2939083c1aa9110 
>   src/slave/containerizer/mesos/isolators/filesystem/linux.cpp 
> a47899cb528eef103f299def3bd3466905ac5b51 
>   src/slave/containerizer/mesos/isolators/gpu/isolator.cpp 
> dbbf92ffbe4a46cedca5b53f6ba172bfb308100e 
>   src/slave/containerizer/mesos/isolators/network/cni/cni.cpp 
> 64271dfbb5c074ad3ac8d2a64c3943d739c0fffa 
>   src/slave/containerizer/mesos/isolators/volume/host_path.cpp 
> 2e03ef50a290c046ae2b02b332d3d007b572429d 
>   src/slave/containerizer/mesos/isolators/volume/image.cpp 
> 53cbaefeef7a6e10149e241e07d6e9cb8d510fc9 
>   src/slave/containerizer/mesos/isolators/volume/sandbox_path.cpp 
> 21d9528c23d9142eccec456184b42d085b57d12c 
>   src/slave/containerizer/mesos/provisioner/backends/bind.cpp 
> 7d564dc7de3e3de8726c7fd42a301d979c4a2574 
>   src/tests/containerizer/fs_tests.cpp 
> 23cad35b5db81a70b43bec1c1dbafe008c8dd4da 
> 
> 
> Diff: https://reviews.apache.org/r/69149/diff/1/
> 
> 
> Testing
> ---
> 
> sudo make check (Fedora 28)
> 
> 
> Thanks,
> 
> James Peach
> 
>

Re: Review Request 69193: Added filters info to the `DECLINE` call log message.

[Mesos Reviewbot Windows]

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



PASS: Mesos patch 69193 was successfully built and tested.

Reviews applied: `['69193']`

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

- Mesos Reviewbot Windows


On Oct. 26, 2018, 5:42 p.m., Meng Zhu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/69193/
> ---
> 
> (Updated Oct. 26, 2018, 5:42 p.m.)
> 
> 
> Review request for mesos, Benjamin Mahler and Gastón Kleiman.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This helps to investigate allocator allocation decisions
> without verbose logging.
> 
> 
> Diffs
> -
> 
>   src/master/master.cpp 0c95c438975efd949cbf86f7d8bfea940c20a43a 
> 
> 
> Diff: https://reviews.apache.org/r/69193/diff/1/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Meng Zhu
> 
>

Re: Review Request 69188: Fixed a wrong way to install callback for OOM notifier.

[Benjamin Mahler]

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


Ship it!




Perhaps a little more description would be helpful? E.g.

```
Ensured failed / discarded cgroup oom notification is logged.

Failed or discarded oom notificaitions in the cgroup memory
isolator were not being logged, due to the continuation being
accidentally set up using onReady rather than onAny.
```

This tells the user what functionally is changing (they will now see logging if 
it fails or is discarded).

- Benjamin Mahler


On Oct. 26, 2018, 3:26 a.m., Qian Zhang wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/69188/
> ---
> 
> (Updated Oct. 26, 2018, 3:26 a.m.)
> 
> 
> Review request for mesos and Benjamin Mahler.
> 
> 
> Bugs: MESOS-9334
> https://issues.apache.org/jira/browse/MESOS-9334
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Fixed a wrong way to install callback for OOM notifier.
> 
> 
> Diffs
> -
> 
>   src/slave/containerizer/mesos/isolators/cgroups/subsystems/memory.cpp 
> f4ed8337253995250cb533fa0a232909cb1a824b 
> 
> 
> Diff: https://reviews.apache.org/r/69188/diff/1/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Qian Zhang
> 
>

Re: Review Request 69180: Enabled streaming requests on the master /api/v1 endpoint.

[Joseph Wu]

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

(Updated Oct. 26, 2018, 11:29 a.m.)


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


Changes
---

Rebased on top of https://reviews.apache.org/r/68953/
which introduced a variable inside `master.cpp` with the same name as the one I 
added in this review.


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


Repository: mesos


Description
---

This change allows clients of the /api/v1 to specify the HTTP header
'Content-Type: application/recordio' to the master's /api/v1 endpoint.
For now, the only master API that will support streaming requests
is the SUBSCRIBE call.  Other APIs will disallow streaming.

This change makes the `Master::Http::api()` function closely resemble
the `Slave::Http::api()` function.  The main difference is that the
incoming stream reader is stored as a shared_ptr rather than a
Owned object, which to prevent the stream reader from going out of
scope while being passed around via lambda captures.


Diffs (updated)
-

  src/master/http.cpp 75ab6ea422a77e25049cd6afe007507d11650a06 
  src/master/master.hpp e77babf22126838c63cd05e483875c9beb3ac5ff 
  src/master/master.cpp 704dfc022ea79118ccd93cf59aac20c3ad45f7f9 


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

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


Testing
---

make

See later in chain.


Thanks,

Joseph Wu

Re: Review Request 69123: Fixed an early fd close in the cgroups event notifier.

[Benjamin Mahler]

> On Oct. 25, 2018, 8:07 a.m., Qian Zhang wrote:
> > src/linux/cgroups.cpp
> > Lines 1071-1080 (original), 1071-1089 (patched)
> > 
> >
> > I see we already have an onAny callback `_listen`, can we close the fd 
> > at the end of that method (i.e., when we are sure that we will not listen 
> > anymore)? And then we could change the code here like below:
> > ```
> > if (reading.isSome()) {
> >   reading->discard();
> > } else if (eventfd.isSome()) {
> >   Try unregister = unregisterNotifier(eventfd.get());
> >   if (unregister.isError()) {
> > LOG(ERROR) << "Failed to unregister eventfd: " << 
> > unregister.error();
> >   }
> > }
> > 
> > ```
> 
> Benjamin Mahler wrote:
> I originally tried to simplify the lifecycle of the listener, but it was 
> implemented this way because some callers call listen multiple times.
> 
> create listener
> listen
> listen
> listen
> terminate
> 
> Qian Zhang wrote:
> Yeah, I know we support that. So that's why I suggested to close the fd 
> (`unregisterNotifier(eventfd.get())`) at the end of `_listen`, and then even 
> the caller call `listen` again, we will actually do nothing but just return 
> `error` (since `error` has been set in `_listen`). I think if we are sure 
> that we will not listen again (i.e., the first time when `_listen` is call 
> with `error` set), we should close the fd immediately rather than closing it 
> until `finalize` is called.

The code that was suggested will leak the fd:

```
if (reading.isSome()) {
  reading->discard();
} else if (eventfd.isSome()) {
  Try unregister = unregisterNotifier(eventfd.get());
  if (unregister.isError()) {
LOG(ERROR) << "Failed to unregister eventfd: " << unregister.error();
  }
}
```

If we're in `finalize()` and `reading.isSome()`, this would never close the fd 
since the dispatch to `_listen` will be dropped (the Process terminated).

Are you suggesting something other than this code?


- Benjamin


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


On Oct. 26, 2018, 3:25 a.m., Benjamin Mahler wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/69123/
> ---
> 
> (Updated Oct. 26, 2018, 3:25 a.m.)
> 
> 
> Review request for mesos and Qian Zhang.
> 
> 
> Bugs: MESOS-9334
> https://issues.apache.org/jira/browse/MESOS-9334
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> The cgroups event notifier was closing the eventfd while an
> `io::read()` operation may be in progress. This can lead to
> bugs where the fd gets re-used and read from a stale io::read.
> 
> 
> Diffs
> -
> 
>   src/linux/cgroups.cpp 95efc1def3318dd93d30d91e9c8a91376cc610b8 
> 
> 
> Diff: https://reviews.apache.org/r/69123/diff/1/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Benjamin Mahler
> 
>

Re: Review Request 68812: Added example framework for inverse-offers.

[Mesos Reviewbot Windows]

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



PASS: Mesos patch 68812 was successfully built and tested.

Reviews applied: `['68812']`

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

- Mesos Reviewbot Windows


On Oct. 26, 2018, 10:20 a.m., Till Toenshoff wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/68812/
> ---
> 
> (Updated Oct. 26, 2018, 10:20 a.m.)
> 
> 
> Review request for mesos and Joseph Wu.
> 
> 
> Bugs: MESOS-5827
> https://issues.apache.org/jira/browse/MESOS-5827
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Adds an example framework displaying how to handle inverse offers.
> This example is based on the original review request
> https://reviews.apache.org/r/50010/ by Joseph Wu.
> Some changes were applied adding framework authentication
> capabilites, updated PullGauge metrics and other minor adaptations
> following the other example frameworks.
> 
> 
> Diffs
> -
> 
>   src/Makefile.am 5795c7097a9ed1f659e169ad81a9f2c09481aa81 
>   src/examples/CMakeLists.txt a2c0dd12e34ae739be85885fd815514f7d3d59a5 
>   src/examples/inverse_offer_framework.cpp PRE-CREATION 
> 
> 
> Diff: https://reviews.apache.org/r/68812/diff/3/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Till Toenshoff
> 
>

Re: Review Request 69193: Added filters info to the `DECLINE` call log message.

[Vinod Kone]

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


Ship it!




Ship It!

- Vinod Kone


On Oct. 26, 2018, 5:42 p.m., Meng Zhu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/69193/
> ---
> 
> (Updated Oct. 26, 2018, 5:42 p.m.)
> 
> 
> Review request for mesos, Benjamin Mahler and Gastón Kleiman.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This helps to investigate allocator allocation decisions
> without verbose logging.
> 
> 
> Diffs
> -
> 
>   src/master/master.cpp 0c95c438975efd949cbf86f7d8bfea940c20a43a 
> 
> 
> Diff: https://reviews.apache.org/r/69193/diff/1/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Meng Zhu
> 
>

Review Request 69193: Added filters info to the `DECLINE` call log message.

[Meng Zhu]

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

Review request for mesos, Benjamin Mahler and Gastón Kleiman.


Repository: mesos


Description
---

This helps to investigate allocator allocation decisions
without verbose logging.


Diffs
-

  src/master/master.cpp 0c95c438975efd949cbf86f7d8bfea940c20a43a 


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


Testing
---

make check


Thanks,

Meng Zhu

Re: Review Request 68812: Added example framework for inverse-offers.

[Till Toenshoff via Review Board]

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

(Updated Oct. 26, 2018, 5:20 p.m.)


Review request for mesos and Joseph Wu.


Changes
---

Ooopsi!


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


Repository: mesos


Description
---

Adds an example framework displaying how to handle inverse offers.
This example is based on the original review request
https://reviews.apache.org/r/50010/ by Joseph Wu.
Some changes were applied adding framework authentication
capabilites, updated PullGauge metrics and other minor adaptations
following the other example frameworks.


Diffs (updated)
-

  src/Makefile.am 5795c7097a9ed1f659e169ad81a9f2c09481aa81 
  src/examples/CMakeLists.txt a2c0dd12e34ae739be85885fd815514f7d3d59a5 
  src/examples/inverse_offer_framework.cpp PRE-CREATION 


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

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


Testing
---

make check


Thanks,

Till Toenshoff

Re: Review Request 68812: Added example framework for inverse-offers.

[Till Toenshoff via Review Board]

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

(Updated Oct. 26, 2018, 4:45 p.m.)


Review request for mesos and Joseph Wu.


Changes
---

Addressed comments - thanks for the review!


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


Repository: mesos


Description
---

Adds an example framework displaying how to handle inverse offers.
This example is based on the original review request
https://reviews.apache.org/r/50010/ by Joseph Wu.
Some changes were applied adding framework authentication
capabilites, updated PullGauge metrics and other minor adaptations
following the other example frameworks.


Diffs (updated)
-

  src/Makefile.am 5795c7097a9ed1f659e169ad81a9f2c09481aa81 
  src/examples/CMakeLists.txt a2c0dd12e34ae739be85885fd815514f7d3d59a5 
  src/examples/inverse_offer_framework.cpp PRE-CREATION 


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

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


Testing
---

make check


Thanks,

Till Toenshoff

Re: Review Request 69160: Included corresponding header file first.

[Till Toenshoff via Review Board]

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


Ship it!




Ship It!

- Till Toenshoff


On Oct. 25, 2018, 10:54 a.m., Benjamin Bannier wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/69160/
> ---
> 
> (Updated Oct. 25, 2018, 10:54 a.m.)
> 
> 
> Review request for mesos and Chun-Hung Hsiao.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Included corresponding header file first.
> 
> 
> Diffs
> -
> 
>   src/common/protobuf_utils.cpp 77139d8a3931dc1e9e00fcea83d77d96244a34f3 
> 
> 
> Diff: https://reviews.apache.org/r/69160/diff/1/
> 
> 
> Testing
> ---
> 
> `make check`
> 
> 
> Thanks,
> 
> Benjamin Bannier
> 
>

Re: Review Request 69160: Included corresponding header file first.

[Jan Schlicht]

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


Ship it!




Ship It!

- Jan Schlicht


On Oct. 25, 2018, 12:54 p.m., Benjamin Bannier wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/69160/
> ---
> 
> (Updated Oct. 25, 2018, 12:54 p.m.)
> 
> 
> Review request for mesos and Chun-Hung Hsiao.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Included corresponding header file first.
> 
> 
> Diffs
> -
> 
>   src/common/protobuf_utils.cpp 77139d8a3931dc1e9e00fcea83d77d96244a34f3 
> 
> 
> Diff: https://reviews.apache.org/r/69160/diff/1/
> 
> 
> Testing
> ---
> 
> `make check`
> 
> 
> Thanks,
> 
> Benjamin Bannier
> 
>

Re: Review Request 69159: Used an alias for reoccuring complicated type.

[Jan Schlicht]

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


Fix it, then Ship it!





src/tests/mesos.hpp
Lines 2921-2930 (original), 2911-2920 (patched)


Indent with 4 spaces.


- Jan Schlicht


On Oct. 25, 2018, 12:54 p.m., Benjamin Bannier wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/69159/
> ---
> 
> (Updated Oct. 25, 2018, 12:54 p.m.)
> 
> 
> Review request for mesos, Chun-Hung Hsiao and Jan Schlicht.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Used an alias for reoccuring complicated type.
> 
> 
> Diffs
> -
> 
>   src/tests/mesos.hpp 40c63b0d6b272c4fb42ec2a126c1bcacb7abee57 
> 
> 
> Diff: https://reviews.apache.org/r/69159/diff/1/
> 
> 
> Testing
> ---
> 
> `make check`
> 
> 
> Thanks,
> 
> Benjamin Bannier
> 
>