Re: Review Request 69211: Improved the code comments for `getContainerDevicesPath`.

[Mesos Reviewbot Windows]

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



PASS: Mesos patch 69211 was successfully built and tested.

Reviews applied: `['69086', '69210', '69211']`

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

- Mesos Reviewbot Windows


On Nov. 17, 2018, 1:50 a.m., James Peach wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/69211/
> ---
> 
> (Updated Nov. 17, 2018, 1:50 a.m.)
> 
> 
> Review request for mesos, Gilbert Song, Jason Lai, Jie Yu, and Jiang Yan Xu.
> 
> 
> Bugs: MESOS-9319
> https://issues.apache.org/jira/browse/MESOS-9319
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Improved the code comments for `getContainerDevicesPath`.
> 
> 
> Diffs
> -
> 
>   src/slave/containerizer/mesos/paths.hpp 
> de3981db7eb08e53901547037c947f594c8d46ab 
> 
> 
> Diff: https://reviews.apache.org/r/69211/diff/8/
> 
> 
> Testing
> ---
> 
> make check (Fedora 28)
> 
> 
> Thanks,
> 
> James Peach
> 
>

Re: Review Request 69211: Improved the code comments for `getContainerDevicesPath`.

[James Peach]

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

(Updated Nov. 17, 2018, 12:50 a.m.)


Review request for mesos, Gilbert Song, Jason Lai, Jie Yu, and Jiang Yan Xu.


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


Repository: mesos


Description
---

Improved the code comments for `getContainerDevicesPath`.


Diffs (updated)
-

  src/slave/containerizer/mesos/paths.hpp 
de3981db7eb08e53901547037c947f594c8d46ab 


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

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


Testing
---

make check (Fedora 28)


Thanks,

James Peach

Re: Review Request 69086: Moved container root construction to the isolators.

[James Peach]

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

(Updated Nov. 17, 2018, 12:49 a.m.)


Review request for mesos, Gilbert Song, Jason Lai, Jie Yu, and Jiang Yan Xu.


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


Repository: mesos


Description
---

Previously, if the container was configured with a root filesytem,
the root was populated by a combination of the `fs::chroot:prepare`
API and the various isolators. The implementation details of some
isolators had leaked into the chroot code, which had a special case
for adding GPU devices.

This change moves all the responsibility for defining the
root filesystem from the `fs::chroot::prepare()` API to the
`filesystem/linux` isolator. The `filesystem/linux` isolator is
now the single place that captures how to mount the container
pseudo-filesystems as well as how to construct a proper `/dev`
directory.

Since the `linux/filesystem` isolator is now entirely responsible
for creating and mounting the container `/dev`, any other isolators
that enable access to devices should populate device nodes in the
container devices directory and add a corresponding bind mount.


Diffs (updated)
-

  src/linux/fs.hpp 31969f6ba82bf5ee549bfdf9698a21adaa486a90 
  src/linux/fs.cpp 5cdffe1f4c7f00aee5b8f640e7cfa4a0018cfa0a 
  src/slave/containerizer/mesos/isolators/filesystem/linux.cpp 
c7d753ac2e5575a8d687600bfb9e0617fa72c990 
  src/slave/containerizer/mesos/isolators/gpu/isolator.hpp 
4645c625877d9451516133b24bd3959e0f49c0a9 
  src/slave/containerizer/mesos/isolators/gpu/isolator.cpp 
56d835779618fd965d928c6926664583e9141f79 
  src/slave/containerizer/mesos/isolators/linux/devices.cpp 
8f8ff95ec3856ba06647637a80315365d0e66e23 
  src/slave/containerizer/mesos/launch.cpp 
882bcdf89e2b0cca3d3f62e6d017849a51ceaead 


Diff: https://reviews.apache.org/r/69086/diff/11/

Changes: https://reviews.apache.org/r/69086/diff/10-11/


Testing
---

sudo make check (Fedora 28)


Thanks,

James Peach

Re: Review Request 69210: Used the MS_SILENT mount flag to elide unwanted logging.

[James Peach]

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

(Updated Nov. 17, 2018, 12:49 a.m.)


Review request for mesos, Gilbert Song, Jason Lai, Jie Yu, and Jiang Yan Xu.


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


Repository: mesos


Description
---

After moving the container root filesystem mounts to the
`filesystem/linux` isolator, these mounts are not logged into the task
sandbox by the container launcher. Since these logs are not generally
useful to tasks, and we did't previously emit them, use the `MS_SILENT`
flag to indicate that they don't need to be logged. Since the kernel
doesn't use `MS_SILENT` for anything, we can safely use it internally.


Diffs (updated)
-

  src/slave/containerizer/mesos/isolators/filesystem/linux.cpp 
c7d753ac2e5575a8d687600bfb9e0617fa72c990 
  src/slave/containerizer/mesos/launch.cpp 
882bcdf89e2b0cca3d3f62e6d017849a51ceaead 


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

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


Testing
---

make check (Fedora 28)


Thanks,

James Peach

Re: Review Request 69373: Replaced a log consensus `CHECK()` with CHECK_GE()`.

[Mesos Reviewbot]

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



Patch looks great!

Reviews applied: [69373]

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 Nov. 16, 2018, 11:25 a.m., James Peach wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/69373/
> ---
> 
> (Updated Nov. 16, 2018, 11:25 a.m.)
> 
> 
> Review request for mesos and Jiang Yan Xu.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> In scale testing, this `CHECK` failed with the following message:
> 
>   F1116 17:50:04.868387 53766 consensus.cpp:771] Check failed:
> highestNackProposal >= proposal
> 
> Emitting the values for `highestNackProposal` and `proposal` may
> help in debugging this failure.
> 
> 
> Diffs
> -
> 
>   src/log/consensus.cpp bce7facb0fe0d89ee96951bcf144332e41eb683e 
> 
> 
> Diff: https://reviews.apache.org/r/69373/diff/1/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> James Peach
> 
>

Re: Review Request 69255: Updated PyInstaller requirement for new CLI to support Python 3.7.

[Mesos Reviewbot Windows]

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



PASS: Mesos patch 69255 was successfully built and tested.

Reviews applied: `['69374', '69255']`

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

- Mesos Reviewbot Windows


On Nov. 5, 2018, 6:13 p.m., Armand Grillet wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/69255/
> ---
> 
> (Updated Nov. 5, 2018, 6:13 p.m.)
> 
> 
> Review request for mesos, Benjamin Bannier and Kevin Klues.
> 
> 
> Bugs: MESOS-9370
> https://issues.apache.org/jira/browse/MESOS-9370
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Updated PyInstaller requirement for new CLI to support Python 3.7.
> 
> 
> Diffs
> -
> 
>   src/python/cli_new/pip-requirements.txt 
> 520dc6131cf05e8f4626a1c673be9b6a59766a8d 
> 
> 
> Diff: https://reviews.apache.org/r/69255/diff/2/
> 
> 
> Testing
> ---
> 
> ```
> $ ./bootstrap
> $ mkdir build
> $ cd build
> $ PYTHON_3=python37 ../configure --enable-new-cli
> $ make check
> $ ./src/mesos
> ```
> 
> 
> Thanks,
> 
> Armand Grillet
> 
>

Review Request 69374: Updated new CLI test step to use binary created by PyInstaller.

[Armand Grillet]

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

Review request for mesos, Benjamin Bannier and Kevin Klues.


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


Repository: mesos


Description
---

The integration tests for the new CLI running while building Mesos now
directly use the binary created during the build. That way we make sure
that the binary created using PyInstaller is usable, which is the
artifact that we want to distribute to users in the future.

Previously, we were only activating the virtual environment to run the
tests thus the binary created by PyInstaller was never properly tested.
To use the binary created by PyInstaller, we simply update the PATH
before running 'mesos-cli-tests'.


Diffs
-

  src/Makefile.am c17eae4ff1d019d515f67d81821e933ecb5dc190 
  src/python/cli_new/tests/CMakeLists.txt 
19119d1d1d640c10ef4ec8e245773920359ccb75 


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


Testing
---

The main test for this change is to make sure that we do not rely on the 
`mesos` in `src/python/cli_new/bin/` anymore. To do so, I have followed these 
steps:

For Autotools:
```
$ ./bootstrap
$ mkdir build
$ cd build
$ ../configure --enable-new-cli
$ make check
$ mv /home/agrillet/mesos/src/python/cli_new/bin/mesos 
/home/agrillet/mesos/src/python/cli_new/bin/mesos2
$ make check
```

Then I put `/home/agrillet/mesos/src/python/cli_new/bin/mesos` back.

For CMake:
```
$ ./bootstrap
$ mkdir build
$ cd build
$ cmake .. -DENABLE_NEW_CLI=1 -DPYTHON_3=python3
$ cmake --build . --target tests
$ ctest -R CLITests -V
$ mv /home/agrillet/mesos/src/python/cli_new/bin/mesos 
/home/agrillet/mesos/src/python/cli_new/bin/mesos2
$ ctest -R CLITests -V
```


Thanks,

Armand Grillet

Re: Review Request 69373: Replaced a log consensus `CHECK()` with CHECK_GE()`.

[Mesos Reviewbot Windows]

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



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

Reviews applied: `['69373']`

Failed command: `Start-MesosCITesting`

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

Relevant logs:

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

```
I1116 20:43:38.808976 40648 containerizer.cpp:3132] Transitioning the state of 
container 161a84ea-94ff-4a1d-89b6-f1572d7e1317 from RUNNING to DESTROYING
I1116 20:43:38.808976 40648 launcher.cpp:161] Asked to destroy container 
161a84ea-94ff-4a1d-89b6-f1572d7e1317
W1116 20:43:38.809969 41048 process.cpp:838] Failed to recv on socket 
WindowsFD::Type::[   OK ] IsolationFlag/MemoryIsolatorTest.ROOT_MemUsage/0 
(682 ms)
[--] 1 test from IsolationFlag/MemoryIsolatorTest (702 ms total)

[--] Global test environment tear-down
d:\dcos\mesos\mesos\src\tests\environment.cpp(1126): error: Failed
Tests completed with child processes remaining:
-+- 34964 001C105FE72C
 |--- 40448 001C105FE72C
 --- 40828 001C105FE72C
[==] 1053 tests from 103 test cases ran. (617587 ms total)
[  PASSED  ] 1050 tests.
[  FAILED  ] 3 tests, listed below:
[  FAILED  ] DockerFetcherPluginTest.INTERNET_CURL_FetchBlob
[  FAILED  ] DockerFetcherPluginTest.INTERNET_CURL_FetchImage
[  FAILED  ] DockerFetcherPluginTest.INTERNET_CURL_InvokeFetchByName

 3 FAILED TESTS
  YOU HAVE 231 DISABLED TESTS

SOCKET=9296 to peer '192.10.1.5:59429': IO failed with error code: The 
specified network name is no longer available.

W1116 20:43:38.810983 41048 process.cpp:1423] Failed to recv on socket 
WindowsFD::Type::SOCKET=9336 to peer '192.10.1.5:59428': IO failed with error 
code: The specified network name is no longer available.

I1116 20:43:38.870992 38812 containerizer.cpp:2971] Container 
161a84ea-94ff-4a1d-89b6-f1572d7e1317 has exited
I1116 20:43:38.901079 37228 master.cpp:1115] Master terminating
I1116 20:43:38.902979 41468 hierarchical.cpp:643] Removed agent 
43a5331b-45e1-45ab-a811-c5fd553e1141-S0
I1116 20:43:39.483969 41048 process.cpp:927] Stopped the socket accept loop
```

- Mesos Reviewbot Windows


On Nov. 16, 2018, 7:25 p.m., James Peach wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/69373/
> ---
> 
> (Updated Nov. 16, 2018, 7:25 p.m.)
> 
> 
> Review request for mesos and Jiang Yan Xu.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> In scale testing, this `CHECK` failed with the following message:
> 
>   F1116 17:50:04.868387 53766 consensus.cpp:771] Check failed:
> highestNackProposal >= proposal
> 
> Emitting the values for `highestNackProposal` and `proposal` may
> help in debugging this failure.
> 
> 
> Diffs
> -
> 
>   src/log/consensus.cpp bce7facb0fe0d89ee96951bcf144332e41eb683e 
> 
> 
> Diff: https://reviews.apache.org/r/69373/diff/1/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> James Peach
> 
>

Re: Review Request 69373: Replaced a log consensus `CHECK()` with CHECK_GE()`.

[Jiang Yan Xu]

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


Ship it!




Ship It!

- Jiang Yan Xu


On Nov. 16, 2018, 11:25 a.m., James Peach wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/69373/
> ---
> 
> (Updated Nov. 16, 2018, 11:25 a.m.)
> 
> 
> Review request for mesos and Jiang Yan Xu.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> In scale testing, this `CHECK` failed with the following message:
> 
>   F1116 17:50:04.868387 53766 consensus.cpp:771] Check failed:
> highestNackProposal >= proposal
> 
> Emitting the values for `highestNackProposal` and `proposal` may
> help in debugging this failure.
> 
> 
> Diffs
> -
> 
>   src/log/consensus.cpp bce7facb0fe0d89ee96951bcf144332e41eb683e 
> 
> 
> Diff: https://reviews.apache.org/r/69373/diff/1/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> James Peach
> 
>

Review Request 69373: Replaced a log consensus `CHECK()` with CHECK_GE()`.

[James Peach]

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

Review request for mesos and Jiang Yan Xu.


Repository: mesos


Description
---

In scale testing, this `CHECK` failed with the following message:

  F1116 17:50:04.868387 53766 consensus.cpp:771] Check failed:
highestNackProposal >= proposal

Emitting the values for `highestNackProposal` and `proposal` may
help in debugging this failure.


Diffs
-

  src/log/consensus.cpp bce7facb0fe0d89ee96951bcf144332e41eb683e 


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


Testing
---

make check


Thanks,

James Peach

Re: Review Request 69363: Cleaned up `include/mesos/type_utils.hpp`.

[Benjamin Bannier]

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




include/mesos/type_utils.hpp
Lines 490 (patched)


Not yours, just making an issue for visibility. Feel free to drop.

Fits on one line.

This also performs two lookups for `it->first` into `right` which can both 
cost `O(log N)`. I think in general one should cache iterators,
```
for (auto l = left.begin(); l != left.end(); ++l) {
auto r = right.find(l->first);
 
if (r == right.end()) {
  return false;
}
 
if (r->first != l->first) {
  return false;
}
}
```



include/mesos/type_utils.hpp
Lines 527 (patched)


Adding definitions to namespace `std` is undefined behavior 
(specializations like below are fine, though).

I'd suggest to keep this in the `mesos` namespace.


- Benjamin Bannier


On Nov. 16, 2018, 1:47 a.m., Chun-Hung Hsiao wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/69363/
> ---
> 
> (Updated Nov. 16, 2018, 1:47 a.m.)
> 
> 
> Review request for mesos and Benjamin Bannier.
> 
> 
> Bugs: MESOS-9275
> https://issues.apache.org/jira/browse/MESOS-9275
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This patch does the following cleanups:
> 
> 1. Moved `google::protobuf::Map` equality operator to `type_utils.hpp`.
> 2. Moved the type helper templates that do not involve mesos protobufs
>into appropriate namespaces so ADL works appropriately.
> 3. Removed the type helpers in 2 from `mesos/v1/mesos.hpp` to avoid
>redefinition.
> 
> 
> Diffs
> -
> 
>   include/mesos/type_utils.hpp aa61c0c88be6a885d4392b1e0dedf52150fb1e31 
>   include/mesos/v1/mesos.hpp 452bcf2699506c15f410434100525b3ced7c4405 
>   src/resource_provider/storage/uri_disk_profile_adaptor.cpp 
> 6c998ef81ce369ea8677beeb3a0c9fdbb789 
> 
> 
> Diff: https://reviews.apache.org/r/69363/diff/1/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Chun-Hung Hsiao
> 
>

Re: Review Request 69360: Rewrote test `ConvertPreExistingVolume` for `CREATE_DISK`.

[Benjamin Bannier]

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



This test does not pass for me, could you fix that?


src/tests/storage_local_resource_provider_tests.cpp
Line 2736 (original), 2723 (patched)


Since we assume the first offer contains the preprovisioned disks on the 
next line, we should also explicitly assert this here, e.g.,
```
ASSERT_EQ(1, rawDiskOffers->size());
```



src/tests/storage_local_resource_provider_tests.cpp
Line 2821 (original), 2761 (patched)


Ditto (assert exactly one offer).



src/tests/storage_local_resource_provider_tests.cpp
Line 2842 (original), 2786 (patched)


This could be `EXPECT_FALSE`.


- Benjamin Bannier


On Nov. 16, 2018, 1:02 a.m., Chun-Hung Hsiao wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/69360/
> ---
> 
> (Updated Nov. 16, 2018, 1:02 a.m.)
> 
> 
> Review request for mesos, Benjamin Bannier, James DeFelice, and Jie Yu.
> 
> 
> Bugs: MESOS-9275
> https://issues.apache.org/jira/browse/MESOS-9275
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Due to the changes of the `CREATE_DISK` semantics, this test is
> rewritten to convert a preprovisioned volume to a profile volumes, and
> then to destroy it to return the space back to the storage pool.
> 
> 
> Diffs
> -
> 
>   src/tests/storage_local_resource_provider_tests.cpp 
> 077a46585bd56181ba199dc529e09f38f4971338 
> 
> 
> Diff: https://reviews.apache.org/r/69360/diff/1/
> 
> 
> Testing
> ---
> 
> make check
> 
> This test passes iff the next patch is applied.
> 
> 
> Thanks,
> 
> Chun-Hung Hsiao
> 
>

Re: Review Request 69356: Added valiadtion for `Offer.Operation.CreateDisk.target_profile`.

[Benjamin Bannier]

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



This patch breaks a number of tests. Could you make sure that they are updated 
with this patch or earlier?

Please spell-check the commit message.


src/master/validation.cpp
Lines 2531-2535 (patched)


I wonder whether performing this validation in the master is the right 
thing to do.

Profiles are currently applied by SLRP in the agent, so doing SLRP-specific 
validation here not only scatters logic in different places, but could also 
make making breaking changes in the future harder than necessary (agent and 
master are upgraded independently).

Should we move this into the agent, e.g., SLRP?


- Benjamin Bannier


On Nov. 16, 2018, 12:55 a.m., Chun-Hung Hsiao wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/69356/
> ---
> 
> (Updated Nov. 16, 2018, 12:55 a.m.)
> 
> 
> Review request for mesos, Benjamin Bannier, James DeFelice, and Jie Yu.
> 
> 
> Bugs: MESOS-9275
> https://issues.apache.org/jira/browse/MESOS-9275
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added valiadtion for `Offer.Operation.CreateDisk.target_profile`.
> 
> 
> Diffs
> -
> 
>   src/master/validation.cpp 5768ac8fe802f28855fbd7be135c75532771 
>   src/tests/master_validation_tests.cpp 
> aa7c8f70c09459be32c6c415497e95fcdc218efd 
> 
> 
> Diff: https://reviews.apache.org/r/69356/diff/1/
> 
> 
> Testing
> ---
> 
> Tests fixed later in chain.
> 
> 
> Thanks,
> 
> Chun-Hung Hsiao
> 
>

Re: Review Request 69359: Rewrote test `ReconcileDroppedOperation` for `CREATE_DISK`.

[Benjamin Bannier]

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



Please move these test fixes to the patches causing the failures so we can 
avoid introducing broken states in the history (useful for e.g., `git-bisect`).


src/tests/storage_local_resource_provider_tests.cpp
Line 3899 (original), 3895 (patched)


Before dereferencing `begin` we should `ASSERT_FALSE(raw.empty())`.



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


Let's us a `static_cast` since this is C++.



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


Let's check before dereferencing `begin()` that `raw - source1` is not 
empty.



src/tests/storage_local_resource_provider_tests.cpp
Line 3965 (original), 3972 (patched)


nit: If we'd break the line after `EXPECT_CALL(` all args would fit on a 
single line.


- Benjamin Bannier


On Nov. 16, 2018, 1 a.m., Chun-Hung Hsiao wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/69359/
> ---
> 
> (Updated Nov. 16, 2018, 1 a.m.)
> 
> 
> Review request for mesos, Benjamin Bannier, Gastón Kleiman, and Greg Mann.
> 
> 
> Bugs: MESOS-9275
> https://issues.apache.org/jira/browse/MESOS-9275
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Previously the `ReconcileDroppedOperation` test relies on converting
> preprovisioned volumes. To adapt the new semantics for `CREATE_DISK`,
> this test is rewritten to create two disks from a storage pool, with one
> operation dropped.
> 
> 
> Diffs
> -
> 
>   src/tests/storage_local_resource_provider_tests.cpp 
> 077a46585bd56181ba199dc529e09f38f4971338 
> 
> 
> Diff: https://reviews.apache.org/r/69359/diff/1/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Chun-Hung Hsiao
> 
>

Re: Review Request 69357: Added profiles to storage pools in tests for `CREATE_DISK`.

[Benjamin Bannier]

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



Can you make sure that this patch does not produce any failing tests (might be 
triggered by earlier patch)?


src/tests/master_tests.cpp
Lines 8870-8875 (original), 8870-8875 (patched)


nit: fits on a single line.



src/tests/storage_local_resource_provider_tests.cpp
Lines 2974-2975 (original), 2974 (patched)


Unrelated to this patch?



src/tests/storage_local_resource_provider_tests.cpp
Lines 3129-3130 (original), 3128 (patched)


Ditto.


- Benjamin Bannier


On Nov. 16, 2018, 12:58 a.m., Chun-Hung Hsiao wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/69357/
> ---
> 
> (Updated Nov. 16, 2018, 12:58 a.m.)
> 
> 
> Review request for mesos, Benjamin Bannier, Greg Mann, and Jan Schlicht.
> 
> 
> Bugs: MESOS-9275
> https://issues.apache.org/jira/browse/MESOS-9275
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This patch adds a new `targetProfile` parameter to the `CREATE_DISK`
> test helper, and add profiles to all storage pools in tests.
> 
> Tests relying on preprovisioned volumes are fixed in subsequent patches.
> 
> 
> Diffs
> -
> 
>   src/tests/api_tests.cpp fdd9f871f75617fc26a28679e2a1e41f506c6133 
>   src/tests/master_tests.cpp ac6bf379c5906cf9612284911c121c9457f648a0 
>   src/tests/mesos.hpp 576f4bde88c069ee2fa0dd33912a034437338e7e 
>   src/tests/operation_reconciliation_tests.cpp 
> 37d38b3df8c162bd1baa5ce557e54baa5c23a006 
>   src/tests/resource_provider_manager_tests.cpp 
> 5bb740edc7c3cc8698aede4f2ed57c21232fe378 
>   src/tests/storage_local_resource_provider_tests.cpp 
> 077a46585bd56181ba199dc529e09f38f4971338 
> 
> 
> Diff: https://reviews.apache.org/r/69357/diff/1/
> 
> 
> Testing
> ---
> 
> `make check`
> 
> Tests `ReconcileDroppedOperation` and `ConvertPreExistingVolume` are fixed 
> later in chain.
> 
> 
> Thanks,
> 
> Chun-Hung Hsiao
> 
>

Re: Review Request 69369: Added collectAuthorizations helper to master.hpp.

[Alexander Rojas]

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


Ship it!




Ship It!

- Alexander Rojas


On Nov. 16, 2018, 2:54 a.m., Till Toenshoff wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/69369/
> ---
> 
> (Updated Nov. 16, 2018, 2:54 a.m.)
> 
> 
> Review request for mesos, Alexander Rukletsov, Alexander Rojas, and Benjamin 
> Bannier.
> 
> 
> Bugs: MESOS-9317
> https://issues.apache.org/jira/browse/MESOS-9317
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Adds the helper function 'collectAuthorizations' to master.hpp. This
> function allows for a simple way to collect authorization futures and
> only if all supplied futures result in an approved authorization will
> the returned future return true.
> All identified areas that were formally triggering MESOS-9317 are
> being updated to make use of this new helper.
> A helper function has been chosen and preferred over copying this
> pattern into the areas that needed a fix to allow for an efficient and
> complete test coverage.
> Additionally we are adding a test validating that new helper.
> 
> 
> Diffs
> -
> 
>   src/master/master.hpp e77babf22126838c63cd05e483875c9beb3ac5ff 
>   src/master/master.cpp 1e326ec42a7f79a0835529a4655e7ec272f1cf40 
>   src/master/weights_handler.cpp 222ec754e216da195250d1895a728294a076ee5d 
>   src/tests/master_tests.cpp ac6bf379c5906cf9612284911c121c9457f648a0 
> 
> 
> Diff: https://reviews.apache.org/r/69369/diff/1/
> 
> 
> Testing
> ---
> 
> `make check` and internal CI validation.
> 
> 
> Thanks,
> 
> Till Toenshoff
> 
>

Re: Review Request 69368: Added test reproducing crash on authorization failure.

[Alexander Rojas]

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


Ship it!




Ship It!


src/tests/master_tests.cpp
Lines 10095 (patched)


Feel free to ignore, but I prefer to use `Authorization` instead of `Authz` 
in the codebase, even if informally we call it authz.


- Alexander Rojas


On Nov. 16, 2018, 2:53 a.m., Till Toenshoff wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/69368/
> ---
> 
> (Updated Nov. 16, 2018, 2:53 a.m.)
> 
> 
> Review request for mesos, Alexander Rukletsov, Alexander Rojas, and Benjamin 
> Bannier.
> 
> 
> Bugs: MESOS-9317
> https://issues.apache.org/jira/browse/MESOS-9317
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This test reproduces the scenario as described in MESOS-9317. The test
> attempts to create a persistent volume by a web request to the
> authorized V1 operator endpoint. The test assures that the underlying
> authorization request fails as it can in production due to failures in
> the authorization backend.
> Without fixing MESOS-9317, this test crashes the master process as the
> code-path involved will attempt to access the contents of the awaited
> future even though the future had failed.
> 
> 
> Diffs
> -
> 
>   src/tests/master_tests.cpp ac6bf379c5906cf9612284911c121c9457f648a0 
> 
> 
> Diff: https://reviews.apache.org/r/69368/diff/1/
> 
> 
> Testing
> ---
> 
> `make check` before applying the fix for MESOS-9317 and after doing so.
> 
> 
> Thanks,
> 
> Till Toenshoff
> 
>

Re: Review Request 69366: Used `OperationID` instead of `string` in test helpers.

[Benjamin Bannier]

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


Fix it, then Ship it!




The commit message talks about `profile` -- could you fix that?


src/tests/mesos.hpp
Lines 1795-1797 (original), 1800-1803 (patched)


We could rewrap this as
```
return common::
  CREATE_DISK(
  std::forward(args)...);
```



src/tests/storage_local_resource_provider_tests.cpp
Lines 4323-4326 (original), 4325-4328 (patched)


Nit: Indented by 1 space too much? `clang-format` would have picked the old 
indent.


- Benjamin Bannier


On Nov. 16, 2018, 1:57 a.m., Chun-Hung Hsiao wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/69366/
> ---
> 
> (Updated Nov. 16, 2018, 1:57 a.m.)
> 
> 
> Review request for mesos, Benjamin Bannier, Gastón Kleiman, and Greg Mann.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This change makes the helper more consisent with other helpers, and
> can avoid one to pass in a string operation ID as a profile.
> 
> 
> Diffs
> -
> 
>   src/tests/master_slave_reconciliation_tests.cpp 
> 064b4d47598b1d0e18d7bef14ac62d7e5f2c1102 
>   src/tests/master_tests.cpp ac6bf379c5906cf9612284911c121c9457f648a0 
>   src/tests/mesos.hpp 576f4bde88c069ee2fa0dd33912a034437338e7e 
>   src/tests/operation_reconciliation_tests.cpp 
> 37d38b3df8c162bd1baa5ce557e54baa5c23a006 
>   src/tests/storage_local_resource_provider_tests.cpp 
> 077a46585bd56181ba199dc529e09f38f4971338 
> 
> 
> Diff: https://reviews.apache.org/r/69366/diff/1/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Chun-Hung Hsiao
> 
>