Re: Review Request 68248: Added futhre move support for `Resources` addition operations.

[Mesos Reviewbot Windows]

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



PASS: Mesos patch 68248 was successfully built and tested.

Reviews applied: `['68197', '68242', '68248']`

All the build artifacts available at: 
http://dcos-win.westus.cloudapp.azure.com/artifacts/mesos-reviewbot-testing/2108/mesos-review-68248

- Mesos Reviewbot Windows


On Aug. 7, 2018, 1 a.m., Meng Zhu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/68248/
> ---
> 
> (Updated Aug. 7, 2018, 1 a.m.)
> 
> 
> Review request for mesos and Benjamin Mahler.
> 
> 
> Bugs: MESOS-9110
> https://issues.apache.org/jira/browse/MESOS-9110
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This patch futher adds move support for `Resources`
> addition operations when the lhs is an rvalue reference.
> 
> 
> Diffs
> -
> 
>   include/mesos/resources.hpp 21aaf0d512bb74aa08398c9aa432f53fffdd3ff0 
>   include/mesos/v1/resources.hpp 2f9c704e92d00f55231272fd1ff5654ee8f69eec 
>   src/common/resources.cpp 253b8bcd720e38f485b5cd2f5b7666ac85e67d38 
>   src/v1/resources.cpp ab8fc3e738038b9b34d4902aed9f15a59b416217 
> 
> 
> Diff: https://reviews.apache.org/r/68248/diff/1/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Meng Zhu
> 
>

Review Request 68251: Pass compiler/linker flags when building the gRPC bundle in libprocess.

[Chun-Hung Hsiao]

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

Review request for mesos, Benjamin Bannier and James Peach.


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


Repository: mesos


Description
---

Since gRPC does not use Autotools, we have to manually pass compiler and
linker flags when issuing `make` to make the build flags consistent with
the Mesos build that initiate the gRPC bundle build.

With this fix, libprocess can be configured as follows to enable the
thread sanitizer:
  CFLAGS='-fsanitize=thread'
  CXXFLAGS='-fsanitize=thread'
  LDFLAGS='-fsanitize=thread'

Also, we now only compile `libglog.la`, as we do in Mesos builds, to
avoid the following GLOG compilation issue when enabling the thread
sanitizer: https://github.com/google/glog/issues/54


Diffs
-

  3rdparty/libprocess/3rdparty/Makefile.am 
55414190fd8fb8f9f2d220701ed7531b99b95797 


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


Testing
---

`make check` for standalone libprocess build.


Thanks,

Chun-Hung Hsiao

Review Request 68250: Pass compiler/linker flags when building the gRPC bundle in Mesos.

[Chun-Hung Hsiao]

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

Review request for mesos, Benjamin Bannier and James Peach.


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


Repository: mesos


Description
---

Since gRPC does not use Autotools, we have to manually pass compiler and
linker flags when issuing `make` to make the build flags consistent with
the Mesos build that initiate the gRPC bundle build.

With this fix, Mesos can be configured as follows to enable thread
sanitizer:
  CFLAGS='-fsanitize=thread'
  CXXFLAGS='-fsanitize=thread'
  LDFLAGS='-fsanitize=thread'


Diffs
-

  3rdparty/Makefile.am 6476b043090b24e8a70982a887b21096752b5581 


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


Testing
---

make check


Thanks,

Chun-Hung Hsiao

Re: Review Request 68239: Updated port mapper CNI test.

[Mesos Reviewbot Windows]

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



PASS: Mesos patch 68239 was successfully built and tested.

All the build artifacts available at: 
http://dcos-win.westus.cloudapp.azure.com/artifacts/mesos-reviewbot-testing/2104/mesos-review-68239

- Mesos Reviewbot Windows


On Aug. 6, 2018, 11:42 p.m., Jie Yu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/68239/
> ---
> 
> (Updated Aug. 6, 2018, 11:42 p.m.)
> 
> 
> Review request for mesos, Chun-Hung Hsiao and Greg Mann.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This patch updated the port mapper CNI test to launch multiple
> containers concurrently. This would allow us to catch the scenarios
> where multiple iptables commands are executed concurrently.
> 
> This test fails if the fix for MESOS-9125 is not included.
> 
> 
> Diffs
> -
> 
>   src/common/values.hpp 39487b955fc1a3c963f69de66ba0da869dd3ab2e 
>   src/tests/containerizer/cni_isolator_tests.cpp 
> 90d2d4103c8136d2dd883318acc135f7efca80d8 
> 
> 
> Diff: https://reviews.apache.org/r/68239/diff/3/
> 
> 
> Testing
> ---
> 
> sudo make check
> 
> 
> Thanks,
> 
> Jie Yu
> 
>

Re: Review Request 68242: Added tests for `Resources` move semantics.

[Benjamin Mahler]

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


Ship it!





src/tests/resources_tests.cpp
Lines 1081-1083 (original), 1122-1124 (patched)


A comment here to clarify why we're wrapping in a Resources()?

This should probably be Resource() non-plural to cover that case?



src/tests/resources_tests.cpp
Line 1251 (original), 1310 (patched)


Ditto above, comments on these would be helpful to the reader:

```
 r += Resources(ports2); // Test +=(Resources&&).
```



src/tests/resources_tests.cpp
Lines 2114 (patched)


Newline above the comment here and below



src/tests/resources_tests.cpp
Line 3538 (original), 3655 (patched)


comment here about why the temporary is used?


- Benjamin Mahler


On Aug. 6, 2018, 11:30 p.m., Meng Zhu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/68242/
> ---
> 
> (Updated Aug. 6, 2018, 11:30 p.m.)
> 
> 
> Review request for mesos and Benjamin Mahler.
> 
> 
> Bugs: MESOS-9110
> https://issues.apache.org/jira/browse/MESOS-9110
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> See summary
> 
> 
> Diffs
> -
> 
>   src/tests/resources_tests.cpp 9475b6ea014434b8d9c7a5c1ba793cd78b10536b 
> 
> 
> Diff: https://reviews.apache.org/r/68242/diff/2/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Meng Zhu
> 
>

Re: Review Request 68244: Propagated the verbose build option to bundled components.

[Mesos Reviewbot Windows]

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



PASS: Mesos patch 68244 was successfully built and tested.

Reviews applied: `['68244']`

All the build artifacts available at: 
http://dcos-win.westus.cloudapp.azure.com/artifacts/mesos-reviewbot-testing/2107/mesos-review-68244

- Mesos Reviewbot Windows


On Aug. 6, 2018, 8:22 p.m., James Peach wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/68244/
> ---
> 
> (Updated Aug. 6, 2018, 8:22 p.m.)
> 
> 
> Review request for mesos, Benjamin Bannier and Chun-Hung Hsiao.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Automake uses the V= variable to specify whether build rules should be
> silent or not. By propagating this flag down into bundled components,
> we can ensure that verbose build output is available when it is useful
> for build debugging.
> 
> 
> Diffs
> -
> 
>   3rdparty/Makefile.am 6476b043090b24e8a70982a887b21096752b5581 
> 
> 
> Diff: https://reviews.apache.org/r/68244/diff/1/
> 
> 
> Testing
> ---
> 
> manual comparison of make V=0 vs. make V=1
> 
> 
> Thanks,
> 
> James Peach
> 
>

Re: Review Request 68242: Added tests for `Resources` move semantics.

[Mesos Reviewbot Windows]

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



PASS: Mesos patch 68242 was successfully built and tested.

Reviews applied: `['68197', '68242']`

All the build artifacts available at: 
http://dcos-win.westus.cloudapp.azure.com/artifacts/mesos-reviewbot-testing/2105/mesos-review-68242

- Mesos Reviewbot Windows


On Aug. 6, 2018, 11:30 p.m., Meng Zhu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/68242/
> ---
> 
> (Updated Aug. 6, 2018, 11:30 p.m.)
> 
> 
> Review request for mesos and Benjamin Mahler.
> 
> 
> Bugs: MESOS-9110
> https://issues.apache.org/jira/browse/MESOS-9110
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> See summary
> 
> 
> Diffs
> -
> 
>   src/tests/resources_tests.cpp 9475b6ea014434b8d9c7a5c1ba793cd78b10536b 
> 
> 
> Diff: https://reviews.apache.org/r/68242/diff/2/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Meng Zhu
> 
>

Re: Review Request 68197: Added move support for `class Resources`.

[Benjamin Mahler]

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




src/common/resources.cpp
Lines 2046-2051 (patched)


Just realized that the following would be more efficient:

```
Resources Resources::operator+(Resources&& that) const
{
  Resources result = std::move(that);
  result += *this;
  return result;
}
```

That way we move then add instead of copy then add; if the resource names 
are the same (which is usually the case) then this is helpful.


- Benjamin Mahler


On Aug. 6, 2018, 11:15 p.m., Meng Zhu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/68197/
> ---
> 
> (Updated Aug. 6, 2018, 11:15 p.m.)
> 
> 
> Review request for mesos and Benjamin Mahler.
> 
> 
> Bugs: MESOS-9110
> https://issues.apache.org/jira/browse/MESOS-9110
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> See summary.
> 
> 
> Diffs
> -
> 
>   include/mesos/resources.hpp 21aaf0d512bb74aa08398c9aa432f53fffdd3ff0 
>   include/mesos/v1/resources.hpp 2f9c704e92d00f55231272fd1ff5654ee8f69eec 
>   src/common/resources.cpp 253b8bcd720e38f485b5cd2f5b7666ac85e67d38 
>   src/v1/resources.cpp ab8fc3e738038b9b34d4902aed9f15a59b416217 
> 
> 
> Diff: https://reviews.apache.org/r/68197/diff/3/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Meng Zhu
> 
>

Review Request 68248: Added futhre move support for `Resources` addition operations.

[Meng Zhu]

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

Review request for mesos and Benjamin Mahler.


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


Repository: mesos


Description
---

This patch futher adds move support for `Resources`
addition operations when the lhs is an rvalue reference.


Diffs
-

  include/mesos/resources.hpp 21aaf0d512bb74aa08398c9aa432f53fffdd3ff0 
  include/mesos/v1/resources.hpp 2f9c704e92d00f55231272fd1ff5654ee8f69eec 
  src/common/resources.cpp 253b8bcd720e38f485b5cd2f5b7666ac85e67d38 
  src/v1/resources.cpp ab8fc3e738038b9b34d4902aed9f15a59b416217 


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


Testing
---

make check


Thanks,

Meng Zhu

Re: Review Request 68022: Enabled Seccomp filter in the containerizer launcher.

[Mesos Reviewbot Windows]

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



FAIL: Failed to apply the dependent review: 67844.

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

All the build artifacts available at: 
http://dcos-win.westus.cloudapp.azure.com/artifacts/mesos-reviewbot-testing/2106/mesos-review-68022

Relevant logs:

- 
[apply-review-67844.log](http://dcos-win.westus.cloudapp.azure.com/artifacts/mesos-reviewbot-testing/2106/mesos-review-68022/logs/apply-review-67844.log):

```
error: missing binary patch data for '3rdparty/libseccomp-2.3.3.tar.gz'
error: binary patch does not apply to '3rdparty/libseccomp-2.3.3.tar.gz'
error: 3rdparty/libseccomp-2.3.3.tar.gz: patch does not apply
```

- Mesos Reviewbot Windows


On Aug. 6, 2018, 6:39 a.m., Andrei Budnik wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/68022/
> ---
> 
> (Updated Aug. 6, 2018, 6:39 a.m.)
> 
> 
> Review request for mesos, Gilbert Song, Jie Yu, James Peach, and Qian Zhang.
> 
> 
> Bugs: MESOS-9106
> https://issues.apache.org/jira/browse/MESOS-9106
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Containerizer launcher creates an instance of `SeccompFilter`, which is
> used to setup Seccomp profile using `ContainerSeccompProfile` message
> prepared by the `linux/seccomp` isolator. The Seccomp filter is loaded
> right before calling `execve()`, so that a container will be running
> with a syscall filtering enabled.
> 
> 
> Diffs
> -
> 
>   src/slave/containerizer/mesos/launch.cpp 
> 7193da0a094df3e441e185c62b3a0379a0bdc4a2 
> 
> 
> Diff: https://reviews.apache.org/r/68022/diff/2/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Andrei Budnik
> 
>

Re: Review Request 68175: Windows: Change strings in Windows os code to `wstring`.

[Mesos Reviewbot Windows]

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



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

Reviews applied: `['68174', '68175']`

Failed command: `Start-MesosCITesting`

All the build artifacts available at: 
http://dcos-win.westus.cloudapp.azure.com/artifacts/mesos-reviewbot-testing/2103/mesos-review-68175

Relevant logs:

- 
[stout-tests.log](http://dcos-win.westus.cloudapp.azure.com/artifacts/mesos-reviewbot-testing/2103/mesos-review-68175/logs/stout-tests.log):

```
[ RUN  ] SocketTests.InitSocket
[   OK ] SocketTests.InitSocket (1 ms)
[ RUN  ] SocketTests.IntFD
[   OK ] SocketTests.IntFD (2 ms)
[--] 2 tests from SocketTests (3 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 (0 ms total)

[--] 2 tests from OsSendfileTest
[ RUN  ] OsSendfileTest.Sendfile
[   OK ] OsSendfileTest.Sendfile (1510 ms)
[ RUN  ] OsSendfileTest.SendfileAsync
[   OK ] OsSendfileTest.SendfileAsync (13 ms)
[--] 2 tests from OsSendfileTest (1523 ms total)

[--] Global test environment tear-down
[==] 333 tests from 52 test cases ran. (5225 ms total)
[  PASSED  ] 332 tests.
[  FAILED  ] 1 test, listed below:
[  FAILED  ] OsTest.SYMLINK_Size

 1 FAILED TEST
  YOU HAVE 1 DISABLED TEST

```

- Mesos Reviewbot Windows


On Aug. 2, 2018, 9:31 p.m., Liangyu Zhao wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/68175/
> ---
> 
> (Updated Aug. 2, 2018, 9:31 p.m.)
> 
> 
> Review request for mesos, Akash Gupta, Andrew Schwartzmeyer, and Joseph Wu.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Functions in `stout/internal/windows` and `stout/os/windows` now use
> `std::wstring` instead of `std::string`. Narrow string is still
> supported, but will converted to wide string for process. The return
> values for functions are unchanged.
> 
> 
> Diffs
> -
> 
>   3rdparty/stout/include/stout/internal/windows/attributes.hpp 
> 673b744588fee32aa4c462199be0675c392636fb 
>   3rdparty/stout/include/stout/internal/windows/longpath.hpp 
> 499eef30a3d16ac1f6c2e3334ef773e91e987a45 
>   3rdparty/stout/include/stout/internal/windows/reparsepoint.hpp 
> 280956ff9cfb0efcc5b0aa7ef35473122d9dd8c0 
>   3rdparty/stout/include/stout/internal/windows/symlink.hpp 
> def5515656a5a454655d6a484c7e52ea12be8a55 
>   3rdparty/stout/include/stout/os/constants.hpp 
> f81dbb3b7dcd929475324047f32cbabd873bea81 
>   3rdparty/stout/include/stout/os/windows/chdir.hpp 
> 523c7f75bd120b00f815c4c4aac946fdb5dbaaa5 
>   3rdparty/stout/include/stout/os/windows/copyfile.hpp 
> 34723bce9a151582de481d63a865509d4d29c02c 
>   3rdparty/stout/include/stout/os/windows/exists.hpp 
> 50bb92a1e4100a89a346d6bafc6d4133e6e40589 
>   3rdparty/stout/include/stout/os/windows/getcwd.hpp 
> daf131ac1da705724e85cd236c68685422d2f3c8 
>   3rdparty/stout/include/stout/os/windows/getenv.hpp 
> 034510cff3701f1063c4fa7ff5c0acb63b421717 
>   3rdparty/stout/include/stout/os/windows/ls.hpp 
> 68d5a6037c4c73d6233829259e4fceb4db613a0c 
>   3rdparty/stout/include/stout/os/windows/mkdir.hpp 
> 2aef22a47a80eddc9db0cd4e4ae3daf77c52c05b 
>   3rdparty/stout/include/stout/os/windows/mkdtemp.hpp 
> f742f0838cdc5132286f55775ca042ce8057c166 
>   3rdparty/stout/include/stout/os/windows/mktemp.hpp 
> b4f5279a58274f02b260bdb3b94c284547842a0f 
>   3rdparty/stout/include/stout/os/windows/open.hpp 
> b2f033c19027144083dc02cf3d4081b1fb26b501 
>   3rdparty/stout/include/stout/os/windows/realpath.hpp 
> 6bfaaf63f849129793b5b3e2d501623ec83e9d4f 
>   3rdparty/stout/include/stout/os/windows/rename.hpp 
> 523912ac3bf315f70f542e8eab7d2d02249909b4 
>   3rdparty/stout/include/stout/os/windows/rm.hpp 
> 2cacb28ec534e0f3e213d04ddf56168bc2061901 
>   3rdparty/stout/include/stout/os/windows/rmdir.hpp 
> 00821cdb62a02824e26ddb125eca66694165ce1f 
>   3rdparty/stout/include/stout/os/windows/shell.hpp 
> b9e06d667159d2fb5e266ce7f7e633deb1237a78 
>   3rdparty/stout/include/stout/os/windows/stat.hpp 
> 62256094b9a58779e28dd78f1281a7d8bd7955d0 
>   3rdparty/stout/include/stout/os/windows/su.hpp 
> ac3a8968f7b9fe6889e13aa47595ae45a094c891 
>   3rdparty/stout/include/stout/os/windows/temp.hpp 
> 4e8543ee6b67f0c2b50a858826e19eed211452bc 
>   3rdparty/stout/include/stout/windows/net.hpp 
> 74398637fe830af0655b1358d4ea52d259a97e9e 
>   3rdparty/stout/include/stout/windows/os.hpp 
> 4f26806e4e027c090c6e7ce07b1bb8d2dabfb531 
> 
> 
> Diff: https://reviews.apache.org/r/68175/diff/2/
> 
> 
> 

Re: Review Request 68239: Updated port mapper CNI test.

[Chun-Hung Hsiao]

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


Fix it, then Ship it!





src/tests/containerizer/cni_isolator_tests.cpp
Line 1946 (original), 1974 (patched)


Nit: Move the declaration close to the use.


- Chun-Hung Hsiao


On Aug. 6, 2018, 11:42 p.m., Jie Yu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/68239/
> ---
> 
> (Updated Aug. 6, 2018, 11:42 p.m.)
> 
> 
> Review request for mesos, Chun-Hung Hsiao and Greg Mann.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This patch updated the port mapper CNI test to launch multiple
> containers concurrently. This would allow us to catch the scenarios
> where multiple iptables commands are executed concurrently.
> 
> This test fails if the fix for MESOS-9125 is not included.
> 
> 
> Diffs
> -
> 
>   src/common/values.hpp 39487b955fc1a3c963f69de66ba0da869dd3ab2e 
>   src/tests/containerizer/cni_isolator_tests.cpp 
> 90d2d4103c8136d2dd883318acc135f7efca80d8 
> 
> 
> Diff: https://reviews.apache.org/r/68239/diff/3/
> 
> 
> Testing
> ---
> 
> sudo make check
> 
> 
> Thanks,
> 
> Jie Yu
> 
>

Re: Review Request 68239: Updated port mapper CNI test.

[Jie Yu]

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

(Updated Aug. 6, 2018, 11:42 p.m.)


Review request for mesos, Chun-Hung Hsiao and Greg Mann.


Changes
---

Addressed comments.


Repository: mesos


Description
---

This patch updated the port mapper CNI test to launch multiple
containers concurrently. This would allow us to catch the scenarios
where multiple iptables commands are executed concurrently.

This test fails if the fix for MESOS-9125 is not included.


Diffs (updated)
-

  src/common/values.hpp 39487b955fc1a3c963f69de66ba0da869dd3ab2e 
  src/tests/containerizer/cni_isolator_tests.cpp 
90d2d4103c8136d2dd883318acc135f7efca80d8 


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

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


Testing
---

sudo make check


Thanks,

Jie Yu

Re: Review Request 68158: Fixed the iptables deadlock in CNI port mapper plugin.

[Jie Yu]

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

(Updated Aug. 6, 2018, 11:42 p.m.)


Review request for mesos, Avinash sridharan, Chun-Hung Hsiao, and Greg Mann.


Changes
---

Addressed comments.


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


Repository: mesos


Description (updated)
---

It is possible that the port mapping cleanup command will cause iptables
to deadlock if there are a lot of entires in the iptables, because the
`sed` won't process the next line while executing `iptables -w -t nat -D
...`. But the executing of `iptables -w -t nat -D ...` might get stuck
if the first command `iptables -w -t nat -S ` didn't finish
(because the xtables lock is not released). The first command might not
finish if it has a lot of output, filling the pipe that `sed` hasn't had
a chance to process yet. See more details in MESOS-9127.

This patch fixed the issue by writing the commands to a file and then
executing them.


Diffs (updated)
-

  
src/slave/containerizer/mesos/isolators/network/cni/plugins/port_mapper/port_mapper.cpp
 f1a3d263b7baa3ccbf270426745022d42fcc66ed 


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

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


Testing
---

sudo make check
```
[   OK ] CniIsolatorPortMapperTest.ROOT_INTERNET_CURL_PortMapper (8827 ms)
```


Thanks,

Jie Yu

Re: Review Request 68242: Added tests for `Resources` move semantics.

[Meng Zhu]

> On Aug. 6, 2018, 2:27 p.m., Benjamin Mahler wrote:
> > src/tests/resources_tests.cpp
> > Line 1069 (original), 1136 (patched)
> > 
> >
> > Is it possible to instead augment any existing tests of addition (e.g. 
> > this one) to also do rvalue addition? It should be pretty easy?
> > 
> > E.g. here we could have r2 use += of rvalues, and have different sums 
> > as well:
> > 
> > ```
> >   Resources sum1 = r1 + r2;
> >   Resources sum2 = Resources(r1) + r2;
> >   Resources sum3 = r1 + Resources(r2);
> >   Resources sum4 = Resources(r1) + Resources(r2);
> > ```

Done.


- Meng


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


On Aug. 6, 2018, 4:30 p.m., Meng Zhu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/68242/
> ---
> 
> (Updated Aug. 6, 2018, 4:30 p.m.)
> 
> 
> Review request for mesos and Benjamin Mahler.
> 
> 
> Bugs: MESOS-9110
> https://issues.apache.org/jira/browse/MESOS-9110
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> See summary
> 
> 
> Diffs
> -
> 
>   src/tests/resources_tests.cpp 9475b6ea014434b8d9c7a5c1ba793cd78b10536b 
> 
> 
> Diff: https://reviews.apache.org/r/68242/diff/2/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Meng Zhu
> 
>

Re: Review Request 68242: Added tests for `Resources` move semantics.

[Meng Zhu]

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

(Updated Aug. 6, 2018, 4:30 p.m.)


Review request for mesos and Benjamin Mahler.


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


Repository: mesos


Description
---

See summary


Diffs (updated)
-

  src/tests/resources_tests.cpp 9475b6ea014434b8d9c7a5c1ba793cd78b10536b 


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

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


Testing
---

make check


Thanks,

Meng Zhu

Re: Review Request 68197: Added move support for `class Resources`.

[Meng Zhu]

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

(Updated Aug. 6, 2018, 4:15 p.m.)


Review request for mesos and Benjamin Mahler.


Changes
---

Added a TODO and adjust the declaration order per the review comments.


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


Repository: mesos


Description
---

See summary.


Diffs (updated)
-

  include/mesos/resources.hpp 21aaf0d512bb74aa08398c9aa432f53fffdd3ff0 
  include/mesos/v1/resources.hpp 2f9c704e92d00f55231272fd1ff5654ee8f69eec 
  src/common/resources.cpp 253b8bcd720e38f485b5cd2f5b7666ac85e67d38 
  src/v1/resources.cpp ab8fc3e738038b9b34d4902aed9f15a59b416217 


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

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


Testing
---

make check


Thanks,

Meng Zhu

Re: Review Request 68239: Updated port mapper CNI test.

[Jie Yu]

> On Aug. 6, 2018, 10:35 p.m., Chun-Hung Hsiao wrote:
> > src/tests/containerizer/cni_isolator_tests.cpp
> > Lines 1799 (patched)
> > 
> >
> > What's the reason of doing a pre-test cleanup first? To prevent 
> > residual rules from the previous run?

Yeah, that's the typical pattern we used in the test.


> On Aug. 6, 2018, 10:35 p.m., Chun-Hung Hsiao wrote:
> > src/tests/containerizer/cni_isolator_tests.cpp
> > Lines 1929-1932 (patched)
> > 
> >
> > Why do we need to consume a new port ID for the container port?

Because container is running on the host network `__MESOS_TEST__` network does 
not fork a new network namespace.


> On Aug. 6, 2018, 10:35 p.m., Chun-Hung Hsiao wrote:
> > src/tests/containerizer/cni_isolator_tests.cpp
> > Lines 1988 (patched)
> > 
> >
> > Nit: s/`containerId`/`containerIds`/
> > 
> > Also, it seems we don't use `containerId` in this test once we get them 
> > at all?

Removed.


> On Aug. 6, 2018, 10:35 p.m., Chun-Hung Hsiao wrote:
> > src/tests/containerizer/cni_isolator_tests.cpp
> > Lines 1957-1958 (original), 1990-1996 (patched)
> > 
> >
> > This test waits for all `TASK_RUNNING` before any `TASK_FINISHED`. What 
> > if the first `TASK_FINISHED` is delivered before the last `TASK_RUNNING`?
> 
> Chun-Hung Hsiao wrote:
> I missed the fact that the tasks won't finish until the `echo foo | nc` 
> commands happen. Dropping this.

I added some comment about the nc server behavior.


> On Aug. 6, 2018, 10:35 p.m., Chun-Hung Hsiao wrote:
> > src/tests/containerizer/cni_isolator_tests.cpp
> > Lines 2000-2004 (patched)
> > 
> >
> > Is it possible to just use this loop to get the container IDs and 
> > network infos?

It's logically a bit weird to combine the loop. I'll stick to the current way.


> On Aug. 6, 2018, 10:35 p.m., Chun-Hung Hsiao wrote:
> > src/tests/containerizer/cni_isolator_tests.cpp
> > Lines 2006-2012 (patched)
> > 
> >
> > How about setting up expectations for `lostExecutor` instead?

I'll use this for now since this is how it was the case before.


> On Aug. 6, 2018, 10:35 p.m., Chun-Hung Hsiao wrote:
> > src/tests/containerizer/cni_isolator_tests.cpp
> > Lines 1984-1987 (original), 2040-2043 (patched)
> > 
> >
> > If we use a matcher above, and use a`vector` instead of an array we can 
> > simply do `AWAIT_READY(collect(statusesFinished))` here. But I'm fine using 
> > loop here so not opening an issue.

Good idea!


> On Aug. 6, 2018, 10:35 p.m., Chun-Hung Hsiao wrote:
> > src/tests/containerizer/cni_isolator_tests.cpp
> > Line 2000 (original), 2045-2047 (patched)
> > 
> >
> > We could do `AWAIT_READY(collect(gcSchedule))` if `gcSchedule` is a 
> > vector.

Good idea!


- Jie


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


On Aug. 6, 2018, 8:31 p.m., Jie Yu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/68239/
> ---
> 
> (Updated Aug. 6, 2018, 8:31 p.m.)
> 
> 
> Review request for mesos, Chun-Hung Hsiao and Greg Mann.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This patch updated the port mapper CNI test to launch multiple
> containers concurrently. This would allow us to catch the scenarios
> where multiple iptables commands are executed concurrently.
> 
> This test fails if the fix for MESOS-9125 is not included.
> 
> 
> Diffs
> -
> 
>   src/common/values.hpp 39487b955fc1a3c963f69de66ba0da869dd3ab2e 
>   src/tests/containerizer/cni_isolator_tests.cpp 
> 90d2d4103c8136d2dd883318acc135f7efca80d8 
> 
> 
> Diff: https://reviews.apache.org/r/68239/diff/2/
> 
> 
> Testing
> ---
> 
> sudo make check
> 
> 
> Thanks,
> 
> Jie Yu
> 
>

Re: Review Request 68158: Fixed the iptables deadlock in CNI port mapper plugin.

[Jie Yu]

> On Aug. 6, 2018, 8:40 p.m., Chun-Hung Hsiao wrote:
> > src/slave/containerizer/mesos/isolators/network/cni/plugins/port_mapper/port_mapper.cpp
> > Lines 382 (patched)
> > 
> >
> > If `iptables` prints something then exits abnormally,
> > do we want to exit this script immediately, or run `sh $FILE` to do 
> > partial cleanup?

I'd rather fail immediately. The output is not reliable if the iptables command 
exits abnormally.


- Jie


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


On Aug. 6, 2018, 8:30 p.m., Jie Yu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/68158/
> ---
> 
> (Updated Aug. 6, 2018, 8:30 p.m.)
> 
> 
> Review request for mesos, Avinash sridharan, Chun-Hung Hsiao, and Greg Mann.
> 
> 
> Bugs: MESOS-9127
> https://issues.apache.org/jira/browse/MESOS-9127
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> It is possible that the port mapping cleanup command will cause iptables
> to deadlock if there are a lot of entires in the iptables, because the
> `sed` won't process the next line while executing 'iptables -w -t nat -D
> ...'. But the executing of 'iptables -w -t nat -D ...' might get stuck
> if the first command 'iptables -w -t nat -S %s' didn't finish (because
> the xtables lock is not released). The first command might not finish if
> it has a lot of output, filling the pipe that `sed` hasn't had a chance
> to process yet. See more details in MESOS-9127.
> 
> This patch fixed the issue by writing the commands to a file and then
> executing them.
> 
> 
> Diffs
> -
> 
>   
> src/slave/containerizer/mesos/isolators/network/cni/plugins/port_mapper/port_mapper.cpp
>  f1a3d263b7baa3ccbf270426745022d42fcc66ed 
> 
> 
> Diff: https://reviews.apache.org/r/68158/diff/4/
> 
> 
> Testing
> ---
> 
> sudo make check
> ```
> [   OK ] CniIsolatorPortMapperTest.ROOT_INTERNET_CURL_PortMapper (8827 ms)
> ```
> 
> 
> Thanks,
> 
> Jie Yu
> 
>

Re: Review Request 68239: Updated port mapper CNI test.

[Chun-Hung Hsiao]

> On Aug. 6, 2018, 10:35 p.m., Chun-Hung Hsiao wrote:
> > src/tests/containerizer/cni_isolator_tests.cpp
> > Lines 1957-1958 (original), 1990-1996 (patched)
> > 
> >
> > This test waits for all `TASK_RUNNING` before any `TASK_FINISHED`. What 
> > if the first `TASK_FINISHED` is delivered before the last `TASK_RUNNING`?

I missed the fact that the tasks won't finish until the `echo foo | nc` 
commands happen. Dropping this.


- Chun-Hung


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


On Aug. 6, 2018, 8:31 p.m., Jie Yu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/68239/
> ---
> 
> (Updated Aug. 6, 2018, 8:31 p.m.)
> 
> 
> Review request for mesos, Chun-Hung Hsiao and Greg Mann.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This patch updated the port mapper CNI test to launch multiple
> containers concurrently. This would allow us to catch the scenarios
> where multiple iptables commands are executed concurrently.
> 
> This test fails if the fix for MESOS-9125 is not included.
> 
> 
> Diffs
> -
> 
>   src/common/values.hpp 39487b955fc1a3c963f69de66ba0da869dd3ab2e 
>   src/tests/containerizer/cni_isolator_tests.cpp 
> 90d2d4103c8136d2dd883318acc135f7efca80d8 
> 
> 
> Diff: https://reviews.apache.org/r/68239/diff/2/
> 
> 
> Testing
> ---
> 
> sudo make check
> 
> 
> Thanks,
> 
> Jie Yu
> 
>

Re: Review Request 68239: Updated port mapper CNI test.

[Chun-Hung Hsiao]

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




src/tests/containerizer/cni_isolator_tests.cpp
Lines 1799 (patched)


What's the reason of doing a pre-test cleanup first? To prevent residual 
rules from the previous run?



src/tests/containerizer/cni_isolator_tests.cpp
Lines 1929-1932 (patched)


Why do we need to consume a new port ID for the container port?



src/tests/containerizer/cni_isolator_tests.cpp
Lines 1940-1941 (patched)


s/`hostPort`/`hostPorts`/
s/`containerPort`/`containerPorts`/



src/tests/containerizer/cni_isolator_tests.cpp
Line 1946 (original), 1975 (patched)


Nits:
s/`statusRunning`/`statusesRunning`/
s/`statusFinished`/`statusesFinished`/



src/tests/containerizer/cni_isolator_tests.cpp
Lines 1988 (patched)


Nit: s/`containerId`/`containerIds`/

Also, it seems we don't use `containerId` in this test once we get them at 
all?



src/tests/containerizer/cni_isolator_tests.cpp
Lines 1957-1958 (original), 1990-1996 (patched)


This test waits for all `TASK_RUNNING` before any `TASK_FINISHED`. What if 
the first `TASK_FINISHED` is delivered before the last `TASK_RUNNING`?



src/tests/containerizer/cni_isolator_tests.cpp
Lines 2000-2004 (patched)


Is it possible to just use this loop to get the container IDs and network 
infos?



src/tests/containerizer/cni_isolator_tests.cpp
Lines 2001 (patched)


We could use the matcher `TaskStatusStateEq(TASK_FINISHED)` to avoid the 
`EXPECT_EQ` below.



src/tests/containerizer/cni_isolator_tests.cpp
Lines 2006-2012 (patched)


How about setting up expectations for `lostExecutor` instead?



src/tests/containerizer/cni_isolator_tests.cpp
Lines 2008 (patched)


Nit: s/`gcSchedule`/`gcSchedules`/



src/tests/containerizer/cni_isolator_tests.cpp
Lines 1984-1987 (original), 2040-2043 (patched)


If we use a matcher above, and use a`vector` instead of an array we can 
simply do `AWAIT_READY(collect(statusesFinished))` here. But I'm fine using 
loop here so not opening an issue.



src/tests/containerizer/cni_isolator_tests.cpp
Line 2000 (original), 2045-2047 (patched)


We could do `AWAIT_READY(collect(gcSchedule))` if `gcSchedule` is a vector.


- Chun-Hung Hsiao


On Aug. 6, 2018, 8:31 p.m., Jie Yu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/68239/
> ---
> 
> (Updated Aug. 6, 2018, 8:31 p.m.)
> 
> 
> Review request for mesos, Chun-Hung Hsiao and Greg Mann.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This patch updated the port mapper CNI test to launch multiple
> containers concurrently. This would allow us to catch the scenarios
> where multiple iptables commands are executed concurrently.
> 
> This test fails if the fix for MESOS-9125 is not included.
> 
> 
> Diffs
> -
> 
>   src/common/values.hpp 39487b955fc1a3c963f69de66ba0da869dd3ab2e 
>   src/tests/containerizer/cni_isolator_tests.cpp 
> 90d2d4103c8136d2dd883318acc135f7efca80d8 
> 
> 
> Diff: https://reviews.apache.org/r/68239/diff/2/
> 
> 
> Testing
> ---
> 
> sudo make check
> 
> 
> Thanks,
> 
> Jie Yu
> 
>

Re: Review Request 68123: Avoided unnecessary `Resources::allocations()` call in the allocator.

[Benjamin Mahler]

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




src/master/allocator/mesos/hierarchical.cpp
Lines 807-815 (original), 807-815 (patched)


Let's use an option instead of a sentinel value:

```
Option role;
...

if (role.isNone()) { set }
else { CHECK_EQ(role, ...role()); }

CHECK_NOTNONE(role);
```

Did I forget to make this suggestion on a similar recent patch?


- Benjamin Mahler


On July 31, 2018, 5:17 a.m., Meng Zhu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/68123/
> ---
> 
> (Updated July 31, 2018, 5:17 a.m.)
> 
> 
> Review request for mesos and Benjamin Mahler.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> See summary.
> 
> 
> Diffs
> -
> 
>   src/master/allocator/mesos/hierarchical.cpp 
> 35992474eacb8b14ae57e1dc23307e1542f63cb5 
> 
> 
> Diff: https://reviews.apache.org/r/68123/diff/2/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Meng Zhu
> 
>

Re: Review Request 68088: Replaced `cgroups::DESTROY_TIMEOUT` with new agent flag.

[Benjamin Mahler]

> On Aug. 1, 2018, 8:40 p.m., James Peach wrote:
> > I don't have a good sense of whether this is going to fix the underlying 
> > problem, but making it a config option is definitely a benefit and the code 
> > looks fine.

I'm also puzzled about how making something configurable will fix this problem. 
My understanding is that one way MESOS-8038 occurs is via destroy timeout 
leading to a destroy failure, making the timeout configurable doesn't fix the 
failure handling?


- Benjamin


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


On July 30, 2018, 5:50 p.m., Zhitao Li wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/68088/
> ---
> 
> (Updated July 30, 2018, 5:50 p.m.)
> 
> 
> Review request for mesos, Gilbert Song, Jason Lai, and James Peach.
> 
> 
> Bugs: MESOS-8038
> https://issues.apache.org/jira/browse/MESOS-8038
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> The new agent flag can be used to reconfigure how long a container
> destroy is allowed to take on Mesos containerizer.
> 
> The default is also increased to 5 min based on suggestion from Gilbert
> because certain containers could have deep system calls which may not
> finish within previous 1 min timeout.
> 
> 
> Diffs
> -
> 
>   src/linux/cgroups.hpp 6a6f0e6df208bc0b0a888d132b3befd062755851 
>   src/slave/containerizer/mesos/isolators/cgroups/cgroups.cpp 
> 8fba6f9d335cc43a7cd0fcb51ed047ba0e7027ed 
>   src/slave/containerizer/mesos/linux_launcher.cpp 
> 3bddcece7028745cec6623ac33dbfcaced629629 
>   src/slave/flags.hpp eeb9708f9ec76d83b6719541f4a012544c7c0cbe 
>   src/slave/flags.cpp 58cdc0f1100fe244e5bf1036e1ccf39478d5d478 
> 
> 
> Diff: https://reviews.apache.org/r/68088/diff/2/
> 
> 
> Testing
> ---
> 
> `make` and `./bin/mesos-slave.sh --help`
> 
> 
> Thanks,
> 
> Zhitao Li
> 
>

Re: Review Request 68134: Avoided reviving on behalf of scheduler after agent reconfiguration.

[Benjamin Mahler]

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


Fix it, then Ship it!




Much appreciated Greg!

Looks like there was no test coverage of the unsuppression? Is there test 
coverage of the filter clearing?

W.r.t. to clearing filters:

(1) If we look at filters as filtering resources (explicitly) and attributes 
(implicitly), then we could make an argument around clearing filters when the 
attributes change. Although to be pendantic, if we were to match the resource 
semantics, we would keep the filter but do attribute subset checking (I don't 
think this makes sense for attributes).
(2) If we consider the use cases for attribute based filtering, my sense is 
that we need a mechanism than the offer filter. E.g. let the framework specify 
some global attribute constraints. Not sure if we'll go down this route since 
it's at conflict with shared state scheduling and it adds complexity.

I could see an argument with (1) for clearing filters when the agent's 
attributes change.


src/master/allocator/mesos/hierarchical.cpp
Lines 759-768 (original), 757-759 (patched)


Probably an explanation to the reader is warranted on why we do this?


- Benjamin Mahler


On July 31, 2018, 6:57 p.m., Greg Mann wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/68134/
> ---
> 
> (Updated July 31, 2018, 6:57 p.m.)
> 
> 
> Review request for mesos, Benno Evers and Benjamin Mahler.
> 
> 
> Bugs: MESOS-9124
> https://issues.apache.org/jira/browse/MESOS-9124
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> When agent reconfiguration was enabled in Mesos, the allocator was
> also updated to remove all offer filters associated with an agent when
> that agent's attributes change. In addition, whenever filters for an
> agent are removed, the framework is revived for all roles that it is
> registered in.
> 
> While this ensures that schedulers will have an opportunity to use
> resources on an agent after reconfiguration, modifying the scheduler's
> suppression may put the scheduler in an inconsistent state, where it
> believes it is suppressed in a particular role when it is not.
> 
> This patch eliminates the suppression modification code, while
> keeping the code which removes a reconfigured agent's offer filters.
> 
> 
> Diffs
> -
> 
>   src/master/allocator/mesos/hierarchical.cpp 
> 35992474eacb8b14ae57e1dc23307e1542f63cb5 
> 
> 
> Diff: https://reviews.apache.org/r/68134/diff/1/
> 
> 
> Testing
> ---
> 
> `make check`
> 
> 
> Thanks,
> 
> Greg Mann
> 
>

Re: Review Request 68242: Added tests for `Resources` move semantics.

[Benjamin Mahler]

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




src/tests/resources_tests.cpp
Lines 800 (patched)


I think `MoveConstruction` could the name for this and we only test all the 
move flavors (Resource, vector, repeated ptr field, Resources), so a bit of a 
merging of the two tests you have here.

It seems we should test addition separately per my other comment.



src/tests/resources_tests.cpp
Line 1069 (original), 1136 (patched)


Is it possible to instead augment any existing tests of addition (e.g. this 
one) to also do rvalue addition? It should be pretty easy?

E.g. here we could have r2 use += of rvalues, and have different sums as 
well:

```
  Resources sum1 = r1 + r2;
  Resources sum2 = Resources(r1) + r2;
  Resources sum3 = r1 + Resources(r2);
  Resources sum4 = Resources(r1) + Resources(r2);
```


- Benjamin Mahler


On Aug. 6, 2018, 7:01 p.m., Meng Zhu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/68242/
> ---
> 
> (Updated Aug. 6, 2018, 7:01 p.m.)
> 
> 
> Review request for mesos and Benjamin Mahler.
> 
> 
> Bugs: MESOS-9110
> https://issues.apache.org/jira/browse/MESOS-9110
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> See summary
> 
> 
> Diffs
> -
> 
>   src/tests/resources_tests.cpp 9475b6ea014434b8d9c7a5c1ba793cd78b10536b 
> 
> 
> Diff: https://reviews.apache.org/r/68242/diff/1/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Meng Zhu
> 
>

Re: Review Request 68197: Added move support for `class Resources`.

[Benjamin Mahler]

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


Fix it, then Ship it!




Looks good! Just a note below about the extra move case for the + operator that 
helps us avoid a copy, you can either incorporate it into this patch or leave a 
TODO here and we can land the additional + operators in a follow up.


include/mesos/v1/resources.hpp
Lines 603-607 (original), 615-623 (patched)


It probably makes more sense to group by type? that's also how I would want 
to read the definitions, side-by-side for the same type:

```
  Resources operator+(const Resource& that) const;
  Resources operator+(Resource&& that) const;

  Resources& operator+=(const Resource& that);
  Resources& operator+=(Resource&& that);
  
  Resources operator+(const Resources& that) const;
  Resources operator+(Resources&& that) const;
 
  Resources& operator+=(const Resources& that);
  Resources& operator+=(Resources&& that);
```

Looks like you already do this grouping in the definitions



include/mesos/v1/resources.hpp
Lines 620-621 (patched)


There are additional benefits to overloading for when the `lhs` is also an 
rvalue reference, e.g. non-member functions:

https://en.cppreference.com/w/cpp/string/basic_string/operator%2B

When a member function, it looks like this:


https://github.com/apache/mesos/blob/1.6.1/3rdparty/stout/include/stout/option.hpp#L118-L121

We don't need to do it in this patch, but we should at least add the TODO 
here if we defer it.


- Benjamin Mahler


On Aug. 6, 2018, 4:48 p.m., Meng Zhu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/68197/
> ---
> 
> (Updated Aug. 6, 2018, 4:48 p.m.)
> 
> 
> Review request for mesos and Benjamin Mahler.
> 
> 
> Bugs: MESOS-9110
> https://issues.apache.org/jira/browse/MESOS-9110
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> See summary.
> 
> 
> Diffs
> -
> 
>   include/mesos/resources.hpp 21aaf0d512bb74aa08398c9aa432f53fffdd3ff0 
>   include/mesos/v1/resources.hpp 2f9c704e92d00f55231272fd1ff5654ee8f69eec 
>   src/common/resources.cpp 253b8bcd720e38f485b5cd2f5b7666ac85e67d38 
>   src/v1/resources.cpp ab8fc3e738038b9b34d4902aed9f15a59b416217 
> 
> 
> Diff: https://reviews.apache.org/r/68197/diff/2/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Meng Zhu
> 
>

Re: Review Request 67505: Refactored verify-reviews.py to use commons.py and argparse.

[Armand Grillet]

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


Fix it, then Ship it!





support/python3/verify-reviews.py
Lines 75 (patched)


Pylint tells me that this needs 3 more spaces before `default_query`. The 
overall style of this call is quite heavy but I don't see how to make things 
clearer.



support/python3/verify-reviews.py
Line 138 (original), 122 (patched)


A third argument seems to be misssing to call 
`apply_reviews(review_request, reviews, handler)`.



support/python3/verify-reviews.py
Lines 291 (patched)


A second argument seems to be misssing to call 
`verify_review(review_request, handler)`.


- Armand Grillet


On July 18, 2018, 8:36 a.m., Dragos Schebesch wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/67505/
> ---
> 
> (Updated July 18, 2018, 8:36 a.m.)
> 
> 
> Review request for mesos, Andrew Schwartzmeyer and Armand Grillet.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Refactored verify-reviews.py to use commons.py and argparse.
> 
> 
> Diffs
> -
> 
>   support/python3/verify-reviews.py 2e925908ffb59dbcdfe99691c5bdbc36a3b7d855 
> 
> 
> Diff: https://reviews.apache.org/r/67505/diff/4/
> 
> 
> Testing
> ---
> 
> Sample run on this review request:
> ```
> ./support/python3/verify-reviews.py -u  -p  -r 67505 file -o 
> test.txt
> 
> 07-18-18_11:09:22 - Running support/python3/verify-reviews.py
> 0 review requests need verification
> ```
> 
> 
> Thanks,
> 
> Dragos Schebesch
> 
>

Re: Review Request 67504: Added support script to post build results.

[Armand Grillet]

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




support/python3/post-build-result.py
Lines 1 (patched)


Should be `python3`.


- Armand Grillet


On July 19, 2018, 3:39 p.m., Dragos Schebesch wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/67504/
> ---
> 
> (Updated July 19, 2018, 3:39 p.m.)
> 
> 
> Review request for mesos, Andrew Schwartzmeyer and Armand Grillet.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added support script to post build results.
> 
> 
> Diffs
> -
> 
>   support/python3/post-build-result.py PRE-CREATION 
> 
> 
> Diff: https://reviews.apache.org/r/67504/diff/4/
> 
> 
> Testing
> ---
> 
> Sample test run that can be seen on this review request:
> ```
> ./python3/post-build-result.py -r 67504 -u  -p  -m 'dummy' -o 
> http://dummy.com/artifact
> Posting to review request: https://reviews.apache.org/r/67504
> dummy
> 
> All the build artifacts available at: http://dummy.com/artifact
> ```
> 
> 
> Thanks,
> 
> Dragos Schebesch
> 
>

Re: Review Request 67502: Refactored ReviewBoard API functionality into separate module.

[Armand Grillet]

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




support/python3/common.py
Lines 146 (patched)


Pylint complains that there are too many local variables, you can remove 
`latest_diff_url` to have only 15 vars in the file.


- Armand Grillet


On July 19, 2018, 3:48 p.m., Dragos Schebesch wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/67502/
> ---
> 
> (Updated July 19, 2018, 3:48 p.m.)
> 
> 
> Review request for mesos, Andrew Schwartzmeyer and Armand Grillet.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Refactored API functionality into separate module -- this was done to clean 
> up the codebase and put all API calls in single place.
> 
> 
> Diffs
> -
> 
>   support/python3/common.py PRE-CREATION 
> 
> 
> Diff: https://reviews.apache.org/r/67502/diff/4/
> 
> 
> Testing
> ---
> 
> For example, to call the api on a specific review_url, with some data, we 
> would use the following code:
> 
> ```
> ReviewBoardHandler().api(review_url, data)
> ```
> 
> 
> Thanks,
> 
> Dragos Schebesch
> 
>

Re: Review Request 68095: Modified MesosContainerizer to GC nested container sandboxes.

[Joseph Wu]

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



Issues spotted while doing some manual testing:


src/slave/containerizer/mesos/containerizer.hpp
Lines 54-59 (original), 54-59 (patched)


It may be necessary to suppress certain log messages in the GC actor.  
Health checks launched via nested containers will also have their sandboxes 
scheduled for GC, which results in at least one log message per directory.

We could also consider avoiding scheduling health check nested containers 
for GC.



src/slave/containerizer/mesos/containerizer.cpp
Lines 2769-2770 (original), 2829-2830 (patched)


Need to unschedule directories for GC when they are removed.  It's possible 
that ContainerIDs will be reused after removal, and if we don't unschedule, a 
reused ContainerID/sandbox could be GC'd while it is in use.


- Joseph Wu


On July 27, 2018, 5:22 p.m., Joseph Wu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/68095/
> ---
> 
> (Updated July 27, 2018, 5:22 p.m.)
> 
> 
> Review request for mesos, Gilbert Song and Qian Zhang.
> 
> 
> Bugs: MESOS-7947
> https://issues.apache.org/jira/browse/MESOS-7947
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> When the --gc_non_executor_container_sandboxes agent flag is enabled,
> this commit changes the MesosContainerizer to schedule nested container
> sandboxes for garbage collection.  The GC policy is the same between
> the MesosContainerizer and the Agent.
> 
> 
> Diffs
> -
> 
>   src/slave/containerizer/mesos/containerizer.hpp 
> 7711d463c8ed92e2580c56e88d7f372c6dfaeb2b 
>   src/slave/containerizer/mesos/containerizer.cpp 
> 98129d006cda9b65804b518619b6addc8990410a 
> 
> 
> Diff: https://reviews.apache.org/r/68095/diff/1/
> 
> 
> Testing
> ---
> 
> See last patch in chain.
> 
> 
> Thanks,
> 
> Joseph Wu
> 
>

Re: Review Request 68158: Fixed the iptables deadlock in CNI port mapper plugin.

[Chun-Hung Hsiao]

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


Fix it, then Ship it!





src/slave/containerizer/mesos/isolators/network/cni/plugins/port_mapper/port_mapper.cpp
Lines 372 (patched)


Backticks instead of single-quotes? Ditto in the following two lines.



src/slave/containerizer/mesos/isolators/network/cni/plugins/port_mapper/port_mapper.cpp
Lines 374 (patched)


s/`%s`/``/



src/slave/containerizer/mesos/isolators/network/cni/plugins/port_mapper/port_mapper.cpp
Lines 382 (patched)


If `iptables` prints something then exits abnormally,
do we want to exit this script immediately, or run `sh $FILE` to do partial 
cleanup?


- Chun-Hung Hsiao


On Aug. 6, 2018, 8:30 p.m., Jie Yu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/68158/
> ---
> 
> (Updated Aug. 6, 2018, 8:30 p.m.)
> 
> 
> Review request for mesos, Avinash sridharan, Chun-Hung Hsiao, and Greg Mann.
> 
> 
> Bugs: MESOS-9127
> https://issues.apache.org/jira/browse/MESOS-9127
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> It is possible that the port mapping cleanup command will cause iptables
> to deadlock if there are a lot of entires in the iptables, because the
> `sed` won't process the next line while executing 'iptables -w -t nat -D
> ...'. But the executing of 'iptables -w -t nat -D ...' might get stuck
> if the first command 'iptables -w -t nat -S %s' didn't finish (because
> the xtables lock is not released). The first command might not finish if
> it has a lot of output, filling the pipe that `sed` hasn't had a chance
> to process yet. See more details in MESOS-9127.
> 
> This patch fixed the issue by writing the commands to a file and then
> executing them.
> 
> 
> Diffs
> -
> 
>   
> src/slave/containerizer/mesos/isolators/network/cni/plugins/port_mapper/port_mapper.cpp
>  f1a3d263b7baa3ccbf270426745022d42fcc66ed 
> 
> 
> Diff: https://reviews.apache.org/r/68158/diff/4/
> 
> 
> Testing
> ---
> 
> sudo make check
> ```
> [   OK ] CniIsolatorPortMapperTest.ROOT_INTERNET_CURL_PortMapper (8827 ms)
> ```
> 
> 
> Thanks,
> 
> Jie Yu
> 
>

Re: Review Request 67503: Added support helper for fetching review ids.

[Armand Grillet]

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


Fix it, then Ship it!




LGTM except the first line. FYI the commit header and message should contain no 
line that are more than 72 characters.


support/python3/get-review-ids.py
Lines 1 (patched)


Should be `python3`.


- Armand Grillet


On July 19, 2018, 3:48 p.m., Dragos Schebesch wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/67503/
> ---
> 
> (Updated July 19, 2018, 3:48 p.m.)
> 
> 
> Review request for mesos, Andrew Schwartzmeyer and Armand Grillet.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added helper for fetching review id, which is needed to decouple this logic 
> from verify-reviews.py. We need this for the windows build so we can trigger 
> a downstream job for each review request on another server.
> 
> 
> Diffs
> -
> 
>   support/python3/get-review-ids.py PRE-CREATION 
> 
> 
> Diff: https://reviews.apache.org/r/67503/diff/4/
> 
> 
> Testing
> ---
> 
> Sample run on this review request id:
> ```
> ./get-review-ids.py -r 67503
> Dependent review: https://reviews.apache.org/api/review-requests/67502/
> 67502
> 
> 67503
> ```
> 
> 
> Thanks,
> 
> Dragos Schebesch
> 
>

Re: Review Request 68158: Fixed the iptables deadlock in CNI port mapper plugin.

[Jie Yu]

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

(Updated Aug. 6, 2018, 8:30 p.m.)


Review request for mesos, Avinash sridharan, Chun-Hung Hsiao, and Greg Mann.


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


Repository: mesos


Description
---

It is possible that the port mapping cleanup command will cause iptables
to deadlock if there are a lot of entires in the iptables, because the
`sed` won't process the next line while executing 'iptables -w -t nat -D
...'. But the executing of 'iptables -w -t nat -D ...' might get stuck
if the first command 'iptables -w -t nat -S %s' didn't finish (because
the xtables lock is not released). The first command might not finish if
it has a lot of output, filling the pipe that `sed` hasn't had a chance
to process yet. See more details in MESOS-9127.

This patch fixed the issue by writing the commands to a file and then
executing them.


Diffs (updated)
-

  
src/slave/containerizer/mesos/isolators/network/cni/plugins/port_mapper/port_mapper.cpp
 f1a3d263b7baa3ccbf270426745022d42fcc66ed 


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

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


Testing
---

sudo make check
```
[   OK ] CniIsolatorPortMapperTest.ROOT_INTERNET_CURL_PortMapper (8827 ms)
```


Thanks,

Jie Yu

Re: Review Request 68239: Updated port mapper CNI test.

[Jie Yu]

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

(Updated Aug. 6, 2018, 8:31 p.m.)


Review request for mesos, Chun-Hung Hsiao and Greg Mann.


Repository: mesos


Description
---

This patch updated the port mapper CNI test to launch multiple
containers concurrently. This would allow us to catch the scenarios
where multiple iptables commands are executed concurrently.

This test fails if the fix for MESOS-9125 is not included.


Diffs (updated)
-

  src/common/values.hpp 39487b955fc1a3c963f69de66ba0da869dd3ab2e 
  src/tests/containerizer/cni_isolator_tests.cpp 
90d2d4103c8136d2dd883318acc135f7efca80d8 


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

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


Testing
---

sudo make check


Thanks,

Jie Yu

Review Request 68244: Propagated the verbose build option to bundled components.

[James Peach]

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

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


Repository: mesos


Description
---

Automake uses the V= variable to specify whether build rules should be
silent or not. By propagating this flag down into bundled components,
we can ensure that verbose build output is available when it is useful
for build debugging.


Diffs
-

  3rdparty/Makefile.am 6476b043090b24e8a70982a887b21096752b5581 


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


Testing
---

manual comparison of make V=0 vs. make V=1


Thanks,

James Peach

Re: Review Request 68158: Fixed the iptables deadlock in CNI port mapper plugin.

[Chun-Hung Hsiao]

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




src/slave/containerizer/mesos/isolators/network/cni/plugins/port_mapper/port_mapper.cpp
Lines 387 (patched)


You cannot use `%s` here, or you have to pass `getIptableRuleTag()` one 
more time (as the first argument) below.


- Chun-Hung Hsiao


On Aug. 6, 2018, 4:52 p.m., Jie Yu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/68158/
> ---
> 
> (Updated Aug. 6, 2018, 4:52 p.m.)
> 
> 
> Review request for mesos, Avinash sridharan, Chun-Hung Hsiao, and Greg Mann.
> 
> 
> Bugs: MESOS-9127
> https://issues.apache.org/jira/browse/MESOS-9127
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> It is possible that the port mapping cleanup command will cause iptables
> to deadlock if there are a lot of entires in the iptables, because the
> `sed` won't process the next line while executing 'iptables -w -t nat -D
> ...'. But the executing of 'iptables -w -t nat -D ...' might get stuck
> if the first command 'iptables -w -t nat -S %s' didn't finish (because
> the xtables lock is not released). The first command might not finish if
> it has a lot of output, filling the pipe that `sed` hasn't had a chance
> to process yet. See more details in MESOS-9127.
> 
> This patch fixed the issue by writing the commands to a file and then
> executing them.
> 
> 
> Diffs
> -
> 
>   
> src/slave/containerizer/mesos/isolators/network/cni/plugins/port_mapper/port_mapper.cpp
>  f1a3d263b7baa3ccbf270426745022d42fcc66ed 
> 
> 
> Diff: https://reviews.apache.org/r/68158/diff/3/
> 
> 
> Testing
> ---
> 
> sudo make check
> ```
> [   OK ] CniIsolatorPortMapperTest.ROOT_INTERNET_CURL_PortMapper (8827 ms)
> ```
> 
> 
> Thanks,
> 
> Jie Yu
> 
>

Re: Review Request 68238: Fixed `mesos-style.py` for Windows.

[Armand Grillet]

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


Ship it!




Ship It!

- Armand Grillet


On Aug. 6, 2018, 6:10 p.m., Andrew Schwartzmeyer wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/68238/
> ---
> 
> (Updated Aug. 6, 2018, 6:10 p.m.)
> 
> 
> Review request for mesos and Armand Grillet.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Due to an unfortunate platform difference, the `virtualenv` subfolder
> is `Script` instead of `bin` on Windows.
> 
> Some hacky inline scripting was done to mimic the shell script
> `build-virtualenv` to build the `virtualenv` on Windows.
> 
> 
> Diffs
> -
> 
>   support/python3/mesos-style.py 12fd33061fcb2cb04349ec1138844ea712610259 
> 
> 
> Diff: https://reviews.apache.org/r/68238/diff/1/
> 
> 
> Testing
> ---
> 
> Can commit using normal hooks on Windows.
> 
> 
> Thanks,
> 
> Andrew Schwartzmeyer
> 
>

Re: Review Request 68243: Updated Git repository URLs.

[James Peach]

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


Ship it!




Ship It!

- James Peach


On Aug. 6, 2018, 7 p.m., Chun-Hung Hsiao wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/68243/
> ---
> 
> (Updated Aug. 6, 2018, 7 p.m.)
> 
> 
> Review request for mesos, Gastón Kleiman, Gilbert Song, Jie Yu, and Vinod 
> Kone.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Updated Git repository URLs.
> 
> 
> Diffs
> -
> 
>   docs/agent-recovery.md 98fb6b6008a9bf29150270424fae3501672ee073 
>   docs/cquery.md 5838cee1879d0a511cd14eb4a4f190dc1373a2ac 
>   docs/windows.md 1e4deaffd1aaf0ca95cbda6eedde515e5865a05c 
>   site/source/blog/2015-07-29-mesos-0-23-0-released.md 
> 88bd6582ddb31442666757fe3bbefc7360816d1e 
>   site/source/blog/2015-09-21-mesos-0-24-0-released.md 
> b807756d4cd13b039aa8cf188bf0aca4c9eeb08a 
>   site/source/blog/2015-10-12-mesos-0-25-0-released.md 
> 60ff67f9bcdaf3491ffb41300a0091f098c808da 
>   site/source/blog/2015-12-16-mesos-0-26-0-released.md 
> f4b466dd52e5cc76721ab1aea33b61f2f5fc2fa2 
>   site/source/blog/2016-02-02-mesos-0-27-0-released.md 
> 3be096bf277dc2ebaa0e0c2ed1463b28a58304b3 
>   site/source/blog/2016-02-22-mesos-0-27-1-released.md 
> d130eac7d004f63ab48ff78c74b25017a9c234c7 
>   site/source/blog/2016-03-07-mesos-0-27-2-released.md 
> 31275602a35ea591141d26cde73070056d356451 
>   site/source/blog/2016-03-17-mesos-0-28-0-released.md 
> a0afdca919555d06c4b38fedfe7cb73492778f5d 
>   site/source/blog/2016-06-13-mesos-0-28-2-released.md 
> 64733af9efd3d0c1d05229582f31fca15158c13c 
>   site/source/blog/2016-06-19-mesos-0-27-3-released.md 
> 916d79a09af942f30e131a34bf11c884a68e0e8f 
>   site/source/blog/2016-08-23-mesos-1-0-1-released.md 
> ab7d2b64a70a7e4949d51c44563c19d46ae46a9a 
>   site/source/blog/2016-11-10-mesos-1-1-0-released.md 
> e78184144565935c5282d4d0fbb5664ed1d38258 
>   site/source/blog/2016-11-15-mesos-1-0-2-released.md 
> 1bf7a109384645a231a81105b37cf4c4e238b36b 
>   site/source/blog/2016-12-20-mesos-0-28-3-released.md 
> 340277d34a39014d5f3196d9b4508065d4cdb579 
>   site/source/blog/2017-02-06-mesos-1-0-3-released.md 
> bcb51d0e3afd0d8973e7a94786255624a3b836e0 
>   site/source/blog/2017-03-08-mesos-1-2-0-released.md 
> cf6866e6446d996c1546a3f2a830d865670939d6 
>   site/source/blog/2017-03-14-mesos-1-1-1-released.md 
> c7fd8469313bb93b5228c9771cdaffbcc87f459d 
>   site/source/blog/2017-05-03-mesos-1-0-4-released.md 
> 92ddc27187d2f0c5bb4270d34cbdcbf7832a3912 
>   site/source/blog/2017-06-07-mesos-1-3-0-released.md 
> 3eda181359cf4bb0cfa38d4e1e50adf863766793 
>   site/source/blog/2017-06-12-mesos-1-2-1-released.md 
> f60beaaeb4f314abb63b10782899f8c723ac7c6a 
>   site/source/blog/2017-08-11-mesos-1-2-2-released.md 
> f21e79526f97fa2f0fd42786f6a6d898a7a6a76b 
>   site/source/blog/2017-08-15-mesos-1-3-1-released.md 
> 74ab1d91373c6801c1479b9159f06975ef3afc18 
>   site/source/blog/2017-08-31-mesos-1-1-3-released.md 
> 5346448f87360b1df58299614f1da94c534d5d0f 
>   site/source/blog/2017-09-18-mesos-1-4-0-released.md 
> 1f6077ca432939c3726c57700a5d208e3c534ae2 
>   site/source/blog/2017-11-16-mesos-1-4-1-released.md 
> b680304db85ccc1b6b2416f6943e7443abc085d5 
>   site/source/blog/2017-12-01-mesos-1-2-3-released.md 
> a1efe4e893e59abd5c837da5f990eef849d52b88 
>   site/source/blog/2018-05-31-mesos-1-5-1-released.md 
> 13bef9b6b3fd172d3b9694da891d18aa26073295 
>   site/source/blog/2018-07-25-mesos-1-6-1-released.md 
> 2be9c0373a7de62e7401851b625a70a0617c1a36 
>   site/source/downloads.html.erb 758a55225b83d3d38c459d365c94b913f56425ae 
>   site/source/index.html.erb 4bde97fed4e40b5f78c77955eadb963b0a8b577b 
>   src/java/mesos.pom.in 318e986204ba28fe1b183fe6de0028945dc49a5d 
>   support/release.sh ced765b58b2b076ffa2f86fd7811a1fab173a994 
>   support/vote.sh 9a72525bc8b5b7ba18778ba76d7f563d99ef5a8b 
> 
> 
> Diff: https://reviews.apache.org/r/68243/diff/1/
> 
> 
> Testing
> ---
> 
> N/A
> 
> 
> Thanks,
> 
> Chun-Hung Hsiao
> 
>

Review Request 68242: Added tests for `Resources` move semantics.

[Meng Zhu]

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

Review request for mesos and Benjamin Mahler.


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


Repository: mesos


Description
---

See summary


Diffs
-

  src/tests/resources_tests.cpp 9475b6ea014434b8d9c7a5c1ba793cd78b10536b 


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


Testing
---

make check


Thanks,

Meng Zhu

Review Request 68243: Updated Git repository URLs.

[Chun-Hung Hsiao]

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

Review request for mesos, Gastón Kleiman, Jie Yu, and Vinod Kone.


Repository: mesos


Description
---

Updated Git repository URLs.


Diffs
-

  docs/agent-recovery.md 98fb6b6008a9bf29150270424fae3501672ee073 
  docs/cquery.md 5838cee1879d0a511cd14eb4a4f190dc1373a2ac 
  docs/windows.md 1e4deaffd1aaf0ca95cbda6eedde515e5865a05c 
  site/source/blog/2015-07-29-mesos-0-23-0-released.md 
88bd6582ddb31442666757fe3bbefc7360816d1e 
  site/source/blog/2015-09-21-mesos-0-24-0-released.md 
b807756d4cd13b039aa8cf188bf0aca4c9eeb08a 
  site/source/blog/2015-10-12-mesos-0-25-0-released.md 
60ff67f9bcdaf3491ffb41300a0091f098c808da 
  site/source/blog/2015-12-16-mesos-0-26-0-released.md 
f4b466dd52e5cc76721ab1aea33b61f2f5fc2fa2 
  site/source/blog/2016-02-02-mesos-0-27-0-released.md 
3be096bf277dc2ebaa0e0c2ed1463b28a58304b3 
  site/source/blog/2016-02-22-mesos-0-27-1-released.md 
d130eac7d004f63ab48ff78c74b25017a9c234c7 
  site/source/blog/2016-03-07-mesos-0-27-2-released.md 
31275602a35ea591141d26cde73070056d356451 
  site/source/blog/2016-03-17-mesos-0-28-0-released.md 
a0afdca919555d06c4b38fedfe7cb73492778f5d 
  site/source/blog/2016-06-13-mesos-0-28-2-released.md 
64733af9efd3d0c1d05229582f31fca15158c13c 
  site/source/blog/2016-06-19-mesos-0-27-3-released.md 
916d79a09af942f30e131a34bf11c884a68e0e8f 
  site/source/blog/2016-08-23-mesos-1-0-1-released.md 
ab7d2b64a70a7e4949d51c44563c19d46ae46a9a 
  site/source/blog/2016-11-10-mesos-1-1-0-released.md 
e78184144565935c5282d4d0fbb5664ed1d38258 
  site/source/blog/2016-11-15-mesos-1-0-2-released.md 
1bf7a109384645a231a81105b37cf4c4e238b36b 
  site/source/blog/2016-12-20-mesos-0-28-3-released.md 
340277d34a39014d5f3196d9b4508065d4cdb579 
  site/source/blog/2017-02-06-mesos-1-0-3-released.md 
bcb51d0e3afd0d8973e7a94786255624a3b836e0 
  site/source/blog/2017-03-08-mesos-1-2-0-released.md 
cf6866e6446d996c1546a3f2a830d865670939d6 
  site/source/blog/2017-03-14-mesos-1-1-1-released.md 
c7fd8469313bb93b5228c9771cdaffbcc87f459d 
  site/source/blog/2017-05-03-mesos-1-0-4-released.md 
92ddc27187d2f0c5bb4270d34cbdcbf7832a3912 
  site/source/blog/2017-06-07-mesos-1-3-0-released.md 
3eda181359cf4bb0cfa38d4e1e50adf863766793 
  site/source/blog/2017-06-12-mesos-1-2-1-released.md 
f60beaaeb4f314abb63b10782899f8c723ac7c6a 
  site/source/blog/2017-08-11-mesos-1-2-2-released.md 
f21e79526f97fa2f0fd42786f6a6d898a7a6a76b 
  site/source/blog/2017-08-15-mesos-1-3-1-released.md 
74ab1d91373c6801c1479b9159f06975ef3afc18 
  site/source/blog/2017-08-31-mesos-1-1-3-released.md 
5346448f87360b1df58299614f1da94c534d5d0f 
  site/source/blog/2017-09-18-mesos-1-4-0-released.md 
1f6077ca432939c3726c57700a5d208e3c534ae2 
  site/source/blog/2017-11-16-mesos-1-4-1-released.md 
b680304db85ccc1b6b2416f6943e7443abc085d5 
  site/source/blog/2017-12-01-mesos-1-2-3-released.md 
a1efe4e893e59abd5c837da5f990eef849d52b88 
  site/source/blog/2018-05-31-mesos-1-5-1-released.md 
13bef9b6b3fd172d3b9694da891d18aa26073295 
  site/source/blog/2018-07-25-mesos-1-6-1-released.md 
2be9c0373a7de62e7401851b625a70a0617c1a36 
  site/source/downloads.html.erb 758a55225b83d3d38c459d365c94b913f56425ae 
  site/source/index.html.erb 4bde97fed4e40b5f78c77955eadb963b0a8b577b 
  src/java/mesos.pom.in 318e986204ba28fe1b183fe6de0028945dc49a5d 
  support/release.sh ced765b58b2b076ffa2f86fd7811a1fab173a994 
  support/vote.sh 9a72525bc8b5b7ba18778ba76d7f563d99ef5a8b 


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


Testing
---

N/A


Thanks,

Chun-Hung Hsiao

Re: Review Request 68239: Updated port mapper CNI test.

[Jie Yu]

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




src/tests/containerizer/cni_isolator_tests.cpp
Lines 2002 (patched)


indent


- Jie Yu


On Aug. 6, 2018, 4:53 p.m., Jie Yu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/68239/
> ---
> 
> (Updated Aug. 6, 2018, 4:53 p.m.)
> 
> 
> Review request for mesos, Chun-Hung Hsiao and Greg Mann.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This patch updated the port mapper CNI test to launch multiple
> containers concurrently. This would allow us to catch the scenarios
> where multiple iptables commands are executed concurrently.
> 
> This test fails if the fix for MESOS-9125 is not included.
> 
> 
> Diffs
> -
> 
>   src/common/values.hpp 39487b955fc1a3c963f69de66ba0da869dd3ab2e 
>   src/tests/containerizer/cni_isolator_tests.cpp 
> 90d2d4103c8136d2dd883318acc135f7efca80d8 
> 
> 
> Diff: https://reviews.apache.org/r/68239/diff/1/
> 
> 
> Testing
> ---
> 
> sudo make check
> 
> 
> Thanks,
> 
> Jie Yu
> 
>

Review Request 68238: Fixed `mesos-style.py` for Windows.

[Andrew Schwartzmeyer]

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

Review request for mesos and Armand Grillet.


Repository: mesos


Description
---

Due to an unfortunate platform difference, the `virtualenv` subfolder
is `Script` instead of `bin` on Windows.

Some hacky inline scripting was done to mimic the shell script
`build-virtualenv` to build the `virtualenv` on Windows.


Diffs
-

  support/python3/mesos-style.py 12fd33061fcb2cb04349ec1138844ea712610259 


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


Testing
---

Can commit using normal hooks on Windows.


Thanks,

Andrew Schwartzmeyer

Re: Review Request 68158: Fixed the iptables deadlock in CNI port mapper plugin.

[Chun-Hung Hsiao]

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


Ship it!




Ship It!

- Chun-Hung Hsiao


On Aug. 6, 2018, 4:52 p.m., Jie Yu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/68158/
> ---
> 
> (Updated Aug. 6, 2018, 4:52 p.m.)
> 
> 
> Review request for mesos, Avinash sridharan, Chun-Hung Hsiao, and Greg Mann.
> 
> 
> Bugs: MESOS-9127
> https://issues.apache.org/jira/browse/MESOS-9127
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> It is possible that the port mapping cleanup command will cause iptables
> to deadlock if there are a lot of entires in the iptables, because the
> `sed` won't process the next line while executing 'iptables -w -t nat -D
> ...'. But the executing of 'iptables -w -t nat -D ...' might get stuck
> if the first command 'iptables -w -t nat -S %s' didn't finish (because
> the xtables lock is not released). The first command might not finish if
> it has a lot of output, filling the pipe that `sed` hasn't had a chance
> to process yet. See more details in MESOS-9127.
> 
> This patch fixed the issue by writing the commands to a file and then
> executing them.
> 
> 
> Diffs
> -
> 
>   
> src/slave/containerizer/mesos/isolators/network/cni/plugins/port_mapper/port_mapper.cpp
>  f1a3d263b7baa3ccbf270426745022d42fcc66ed 
> 
> 
> Diff: https://reviews.apache.org/r/68158/diff/3/
> 
> 
> Testing
> ---
> 
> sudo make check
> ```
> [   OK ] CniIsolatorPortMapperTest.ROOT_INTERNET_CURL_PortMapper (8827 ms)
> ```
> 
> 
> Thanks,
> 
> Jie Yu
> 
>

Re: Review Request 68225: Added 25th and 75th percentiles to `Statistics`.

[Benjamin Mahler]

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


Ship it!





3rdparty/libprocess/include/process/statistics.hpp
Line 80 (original), 80-82 (patched)


Do you want a TODO to expose these in metrics snapshots? E.g.

```
T p25; // TODO(alexr): Expose this in metrics snapshots.
T p75; // TODO(alexr): Expose this in metrics snapshots.
```

Or just expose them in this patch?



3rdparty/libprocess/src/tests/statistics_tests.cpp
Lines 93-94 (original), 95-98 (patched)


We should probably have a test that maps p25 to 25, pXX to XX, etc to be 
more obviously correct. But certainly no need to touch it in this patch.


- Benjamin Mahler


On Aug. 6, 2018, 10:30 a.m., Alexander Rukletsov wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/68225/
> ---
> 
> (Updated Aug. 6, 2018, 10:30 a.m.)
> 
> 
> Review request for mesos, Benno Evers and Benjamin Mahler.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> See summary.
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/include/process/statistics.hpp 
> e9f1fc23bf83f92a2e7de94dba0df48272cc3394 
>   3rdparty/libprocess/src/tests/statistics_tests.cpp 
> a2a780bf9de018c823b68aa48977fd9fd1b8a064 
> 
> 
> Diff: https://reviews.apache.org/r/68225/diff/1/
> 
> 
> Testing
> ---
> 
> https://reviews.apache.org/r/68132/
> 
> 
> Thanks,
> 
> Alexander Rukletsov
> 
>

Re: Review Request 68158: Fixed the iptables deadlock in CNI port mapper plugin.

[Deepak Goel]

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


Ship it!




Ship It!

- Deepak Goel


On Aug. 6, 2018, 4:52 p.m., Jie Yu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/68158/
> ---
> 
> (Updated Aug. 6, 2018, 4:52 p.m.)
> 
> 
> Review request for mesos, Avinash sridharan, Chun-Hung Hsiao, and Greg Mann.
> 
> 
> Bugs: MESOS-9127
> https://issues.apache.org/jira/browse/MESOS-9127
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> It is possible that the port mapping cleanup command will cause iptables
> to deadlock if there are a lot of entires in the iptables, because the
> `sed` won't process the next line while executing 'iptables -w -t nat -D
> ...'. But the executing of 'iptables -w -t nat -D ...' might get stuck
> if the first command 'iptables -w -t nat -S %s' didn't finish (because
> the xtables lock is not released). The first command might not finish if
> it has a lot of output, filling the pipe that `sed` hasn't had a chance
> to process yet. See more details in MESOS-9127.
> 
> This patch fixed the issue by writing the commands to a file and then
> executing them.
> 
> 
> Diffs
> -
> 
>   
> src/slave/containerizer/mesos/isolators/network/cni/plugins/port_mapper/port_mapper.cpp
>  f1a3d263b7baa3ccbf270426745022d42fcc66ed 
> 
> 
> Diff: https://reviews.apache.org/r/68158/diff/3/
> 
> 
> Testing
> ---
> 
> sudo make check
> ```
> [   OK ] CniIsolatorPortMapperTest.ROOT_INTERNET_CURL_PortMapper (8827 ms)
> ```
> 
> 
> Thanks,
> 
> Jie Yu
> 
>

[GitHub] jieyu closed pull request #306: Fixed the iptables deadlock in CNI port mapper plugin.

[GitBox]

jieyu closed pull request #306: Fixed the iptables deadlock in CNI port mapper 
plugin.
URL: https://github.com/apache/mesos/pull/306
 
 
   

This is a PR merged from a forked repository.
As GitHub hides the original diff on merge, it is displayed below for
the sake of provenance:

As this is a foreign pull request (from a fork), the diff is supplied
below (as it won't show otherwise due to GitHub magic):

diff --git a/src/common/values.hpp b/src/common/values.hpp
index 39487b955f..f87897faf9 100644
--- a/src/common/values.hpp
+++ b/src/common/values.hpp
@@ -19,6 +19,7 @@
 
 #include 
 #include 
+#include 
 
 #include 
 
@@ -54,6 +55,30 @@ Try> rangesToIntervalSet(const Value::Ranges& 
ranges)
 }
 
 
+template 
+Try> rangesToVector(const Value::Ranges& ranges)
+{
+  std::vector result;
+
+  static_assert(
+  std::is_integral::value,
+  "vector must use an integral type");
+
+  foreach (const Value::Range& range, ranges.range()) {
+if (range.begin() < std::numeric_limits::min() ||
+range.end() > std::numeric_limits::max()) {
+  return Error("Range is out of bounds");
+}
+
+for (T value = range.begin(); value <= range.end(); value++) {
+  result.push_back(value);
+}
+  }
+
+  return result;
+}
+
+
 // Convert IntervalSet value to Ranges value.
 template 
 Value::Ranges intervalSetToRanges(const IntervalSet& set)
diff --git 
a/src/slave/containerizer/mesos/isolators/network/cni/plugins/port_mapper/port_mapper.cpp
 
b/src/slave/containerizer/mesos/isolators/network/cni/plugins/port_mapper/port_mapper.cpp
index f1a3d263b7..d407fa7240 100644
--- 
a/src/slave/containerizer/mesos/isolators/network/cni/plugins/port_mapper/port_mapper.cpp
+++ 
b/src/slave/containerizer/mesos/isolators/network/cni/plugins/port_mapper/port_mapper.cpp
@@ -363,13 +363,34 @@ Try PortMapper::delPortMapping()
   string script = strings::format(
   R"~(
   #!/bin/sh
-  exec 1>&2
   set -x
 
+  FILE=$(mktemp)
+
+  cleanup() {
+rm -f "$FILE"
+  }
+
+  trap cleanup EXIT
+
   # The iptables command searches for the DNAT rules with tag
   # "container_id: ", and if it exists goes ahead
   # and deletes it.
-  iptables -w -t nat -S %s | sed "/%s/ s/-A/iptables -w -t nat -D/e")~",
+  #
+  # NOTE: We use a temp file here, instead of letting `sed`
+  # directly executing the iptables commands because otherwise, it
+  # is possible that the port mapping cleanup command will cause
+  # iptables to deadlock if there are a lot of entires in the
+  # iptables, because the `sed` won't process the next line while
+  # executing 'iptables -w -t nat -D ...'. But the executing of
+  # 'iptables -w -t nat -D ...' might get stuck if the first
+  # command 'iptables -w -t nat -S %s' didn't finish (because the
+  # xtables lock is not released). The first command might not
+  # finish if it has a lot of output, filling the pipe that `sed`
+  # hasn't had a chance to process yet. See details in MESOS-9127.
+  iptables -w -t nat -S %s | sed -n "/%s/ s/-A/iptables -w -t nat -D/p" > 
$FILE
+  sh $FILE
+  )~",
   chain,
   getIptablesRuleTag()).get();
 
diff --git a/src/tests/containerizer/cni_isolator_tests.cpp 
b/src/tests/containerizer/cni_isolator_tests.cpp
index 90d2d4103c..93e0299b7a 100644
--- a/src/tests/containerizer/cni_isolator_tests.cpp
+++ b/src/tests/containerizer/cni_isolator_tests.cpp
@@ -14,12 +14,16 @@
 // See the License for the specific language governing permissions and
 // limitations under the License.
 
+#include 
+
 #include 
 
 #include 
 
 #include 
 
+#include "common/values.hpp"
+
 #include "slave/gc_process.hpp"
 
 #include "slave/containerizer/fetcher.hpp"
@@ -1790,6 +1794,8 @@ class CniIsolatorPortMapperTest : public CniIsolatorTest
   {
 CniIsolatorTest::SetUp();
 
+cleanup();
+
 Try mockConfig = os::read(
 path::join(cniConfigDir, MESOS_MOCK_CNI_CONFIG));
 
@@ -1818,6 +1824,13 @@ class CniIsolatorPortMapperTest : public CniIsolatorTest
   }
 
   void TearDown() override
+  {
+cleanup();
+
+CniIsolatorTest::TearDown();
+  }
+
+  void cleanup()
   {
 // This is a best effort cleanup of the
 // `MESOS_TEST_PORT_MAPPER_CHAIN`. We shouldn't fail and bail on
@@ -1831,7 +1844,7 @@ class CniIsolatorPortMapperTest : public CniIsolatorTest
 iptables -w -t nat --list %s
 
 if [ $? -eq 0 ]; then
-  iptables -w -t nat -D OUTPUT ! -d 127.0.0.0/8 -m addrtype --dst-type 
LOCAL -j  %s
+  iptables -w -t nat -D OUTPUT ! -d 127.0.0.0/8 -m addrtype --dst-type 
LOCAL -j %s
   iptables -w -t nat -D PREROUTING -m addrtype --dst-type LOCAL -j %s
   iptables -w -t nat -F %s
   iptables -w -t nat -X %s
@@ -1849,14 +1862,14 @@ class CniIsolatorPortMapperTest : public CniIsolatorTest
  << stringify(MESOS_TEST_PORT_MAPPER_CHAIN)
  << ": " << result.error();
 }
-
-

[GitHub] jieyu commented on issue #306: Fixed the iptables deadlock in CNI port mapper plugin.

[GitBox]

jieyu commented on issue #306: Fixed the iptables deadlock in CNI port mapper 
plugin.
URL: https://github.com/apache/mesos/pull/306#issuecomment-410775862
 
 
   Closed this as the review has been moved to reviewboard.


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


With regards,
Apache Git Services

Review Request 68239: Updated port mapper CNI test.

[Jie Yu]

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

Review request for mesos, Chun-Hung Hsiao and Greg Mann.


Repository: mesos


Description
---

This patch updated the port mapper CNI test to launch multiple
containers concurrently. This would allow us to catch the scenarios
where multiple iptables commands are executed concurrently.

This test fails if the fix for MESOS-9125 is not included.


Diffs
-

  src/common/values.hpp 39487b955fc1a3c963f69de66ba0da869dd3ab2e 
  src/tests/containerizer/cni_isolator_tests.cpp 
90d2d4103c8136d2dd883318acc135f7efca80d8 


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


Testing
---

sudo make check


Thanks,

Jie Yu

Re: Review Request 68158: Fixed the iptables deadlock in CNI port mapper plugin.

[Jie Yu]

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

(Updated Aug. 6, 2018, 4:52 p.m.)


Review request for mesos, Avinash sridharan, Chun-Hung Hsiao, and Greg Mann.


Changes
---

Rebased.


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


Repository: mesos


Description
---

It is possible that the port mapping cleanup command will cause iptables
to deadlock if there are a lot of entires in the iptables, because the
`sed` won't process the next line while executing 'iptables -w -t nat -D
...'. But the executing of 'iptables -w -t nat -D ...' might get stuck
if the first command 'iptables -w -t nat -S %s' didn't finish (because
the xtables lock is not released). The first command might not finish if
it has a lot of output, filling the pipe that `sed` hasn't had a chance
to process yet. See more details in MESOS-9127.

This patch fixed the issue by writing the commands to a file and then
executing them.


Diffs (updated)
-

  
src/slave/containerizer/mesos/isolators/network/cni/plugins/port_mapper/port_mapper.cpp
 f1a3d263b7baa3ccbf270426745022d42fcc66ed 


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

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


Testing
---

sudo make check
```
[   OK ] CniIsolatorPortMapperTest.ROOT_INTERNET_CURL_PortMapper (8827 ms)
```


Thanks,

Jie Yu

Re: Review Request 68197: Added move support for `class Resources`.

[Meng Zhu]

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

(Updated Aug. 6, 2018, 9:48 a.m.)


Review request for mesos and Benjamin Mahler.


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


Repository: mesos


Description
---

See summary.


Diffs (updated)
-

  include/mesos/resources.hpp 21aaf0d512bb74aa08398c9aa432f53fffdd3ff0 
  include/mesos/v1/resources.hpp 2f9c704e92d00f55231272fd1ff5654ee8f69eec 
  src/common/resources.cpp 253b8bcd720e38f485b5cd2f5b7666ac85e67d38 
  src/v1/resources.cpp ab8fc3e738038b9b34d4902aed9f15a59b416217 


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

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


Testing
---

make check


Thanks,

Meng Zhu

Re: Review Request 68016: Added libseccomp to the build.

[Andrei Budnik]

> On July 31, 2018, 12:33 a.m., Andrew Schwartzmeyer wrote:
> > cmake/CompilationConfigure.cmake
> > Lines 538-541 (patched)
> > 
> >
> > What targets need `ENABLE_SECCOMP_ISOLATOR` defined? Presumably this is 
> > new code, so we don't have to leave this as a to-do. (And I'm guessing just 
> > `libmesos PRIVATE`?)

Yeah, `ENABLE_SECCOMP_ISOLATOR` is used only by `libmesos`.
What is the best way to pass `-DENABLE_SECCOMP_ISOLATOR` compiler flag into 
`libmesos` build, but omit it for `3rdparty`?


- Andrei


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


On Aug. 6, 2018, 1:38 p.m., Andrei Budnik wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/68016/
> ---
> 
> (Updated Aug. 6, 2018, 1:38 p.m.)
> 
> 
> Review request for mesos, Andrew Schwartzmeyer, Gilbert Song, Jie Yu, James 
> Peach, and Qian Zhang.
> 
> 
> Bugs: MESOS-9032
> https://issues.apache.org/jira/browse/MESOS-9032
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This library is needed to implement Seccomp syscall filtering in the
> Mesos containerizer. This patch introduces `seccomp-isolator` build
> flag, which is used to include or exclude sources related to Seccomp
> from the build. Since Seccomp is a Linux-specific feature, the flag
> is disabled by default. Enabling `seccomp-isolator` means either:
> 
> 1. Compiling and linking against the bundled version of libseccomp from
>sources (default).
> 
> 2. Linking against the libseccomp installed in the OS,
>if `--with-libseccomp` build flag is provided.
> 
> 
> Diffs
> -
> 
>   3rdparty/CMakeLists.txt 629b1968775da4d128b8d36c77d75efa303b0b7c 
>   3rdparty/Makefile.am 6476b043090b24e8a70982a887b21096752b5581 
>   3rdparty/cmake/Versions.cmake c65d7b3d2d48ce9d0b4777aeb12fdf5b605803ac 
>   3rdparty/versions.am 8fd8bd4d7482af5304ab0b56270e49f447e0b3f5 
>   cmake/CompilationConfigure.cmake 10cacfb99e2cff1ddd2285ae441730f61182e06d 
>   configure.ac d84581823708cdece68d52bdc023e01d72645bf8 
>   src/CMakeLists.txt a80b011bbd5c418ae66eb8dd7697d070168462bb 
>   src/Makefile.am cf0cf22b3f582d3d4427f13288af3845aef45263 
> 
> 
> Diff: https://reviews.apache.org/r/68016/diff/4/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Andrei Budnik
> 
>

Re: Review Request 67185: Added request_protobuf to mesos.http.

[Armand Grillet]

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



LGTM, I suggest merging this after the Python 3 update of mesos.http.


src/python/lib/mesos/http.py
Lines 31 (patched)


Please insert one new line before.



src/python/lib/mesos/http.py
Lines 41 (patched)


The `mesos.constants` imports should be before the `mesos.exception` ones 
alphabetically.



src/python/lib/mesos/http.py
Lines 196 (patched)


Why is it not `PROTOBUF_ENCODING` instead of `self.default_encoding` as we 
know that we want to encode a protobuf message?



src/python/lib/mesos/http.py
Lines 345 (patched)


Should these match the parameter names from `request()`?


- Armand Grillet


On June 28, 2018, 6:17 a.m., Eric Chung wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/67185/
> ---
> 
> (Updated June 28, 2018, 6:17 a.m.)
> 
> 
> Review request for mesos, Armand Grillet, Jason Lai, Kevin Klues, and Zhitao 
> Li.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This change adds the method `Resource.request_protobuf()` which assumes
> that both the request and response data are encoded in protobuf. This
> will be used later in the RPC client, which will also include a decoding
> step.
> 
> Testing Done:
> Ran unit tests with `tox` under `src/python/lib`
> 
> 
> Diffs
> -
> 
>   src/python/lib/mesos/constants.py PRE-CREATION 
>   src/python/lib/mesos/http.py 073c159dc6916fa5d7349a033db022d7175aef05 
>   src/python/lib/requirements.in 0742f3d846c99c1c4907d9628fb49845564563b2 
>   src/python/lib/tests/test_http.py 66dd6d7c6272d1828dd591829ca3543d3430f69b 
> 
> 
> Diff: https://reviews.apache.org/r/67185/diff/4/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Eric Chung
> 
>

Re: Review Request 68229: Removed unused local variable in `slave.cpp`.

[Alexander Rojas]

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


Ship it!




Ship It!

- Alexander Rojas


On Aug. 6, 2018, 4:14 p.m., Benno Evers wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/68229/
> ---
> 
> (Updated Aug. 6, 2018, 4:14 p.m.)
> 
> 
> Review request for mesos and Alexander Rojas.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Removed the computation of `subject` in function
> `Slave::authorizeSandboxAccess` since it was unused since the
> introduction of ObjectApprovers in commit 83dd7f8724.
> 
> 
> Diffs
> -
> 
>   src/slave/slave.cpp e574c249f81e0e77abe982c126fe210a6ee8b591 
> 
> 
> Diff: https://reviews.apache.org/r/68229/diff/1/
> 
> 
> Testing
> ---
> 
> `make check`
> 
> 
> Thanks,
> 
> Benno Evers
> 
>

Review Request 68229: Removed unused local variable in `slave.cpp`.

[Benno Evers]

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

Review request for mesos and Alexander Rojas.


Repository: mesos


Description
---

Removed the computation of `subject` in function
`Slave::authorizeSandboxAccess` since it was unused since the
introduction of ObjectApprovers in commit 83dd7f8724.


Diffs
-

  src/slave/slave.cpp e574c249f81e0e77abe982c126fe210a6ee8b591 


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


Testing
---

`make check`


Thanks,

Benno Evers

Re: Review Request 68022: Enabled Seccomp filter in the containerizer launcher. (WIP)

[Andrei Budnik]

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

(Updated Aug. 6, 2018, 1:39 p.m.)


Review request for mesos, Gilbert Song, Jie Yu, James Peach, and Qian Zhang.


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


Repository: mesos


Description
---

Containerizer launcher creates an instance of `SeccompFilter`, which is
used to setup Seccomp profile using `ContainerSeccompProfile` message
prepared by the `linux/seccomp` isolator. The Seccomp filter is loaded
right before calling `execve()`, so that a container will be running
with a syscall filtering enabled.


Diffs
-

  src/slave/containerizer/mesos/launch.cpp 
7193da0a094df3e441e185c62b3a0379a0bdc4a2 


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


Testing
---


Thanks,

Andrei Budnik

Re: Review Request 68021: Added `linux/seccomp` isolator.

[Andrei Budnik]

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

(Updated Aug. 6, 2018, 1:39 p.m.)


Review request for mesos, Gilbert Song, Jie Yu, James Peach, and Qian Zhang.


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


Repository: mesos


Description
---

This patch introduces `linux/seccomp` isolator which is used for
preparing `ContainerSeccompProfile` for the Mesos containerizer
launcher. If the `ContainerConfig` message has an info about Seccomp
profile name, then this info will be used to locate a Seccomp profile.
The given Seccomp profile is parsed and the resulting
`ContainerSeccompProfile` is stored in the `ContainerLaunchInfo`
message.


Diffs
-

  src/CMakeLists.txt 10b0946d6f49c7e9c201bad6f9f1b41cc8460fe5 
  src/Makefile.am ecb95ef152fd37e5e83c4f6415489f4cf3959578 
  src/slave/containerizer/mesos/containerizer.cpp 
98129d006cda9b65804b518619b6addc8990410a 
  src/slave/containerizer/mesos/isolators/linux/seccomp.hpp PRE-CREATION 
  src/slave/containerizer/mesos/isolators/linux/seccomp.cpp PRE-CREATION 


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


Testing
---


Thanks,

Andrei Budnik

Re: Review Request 68019: Added a parser for the Docker Seccomp config format.

[Andrei Budnik]

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

(Updated Aug. 6, 2018, 1:38 p.m.)


Review request for mesos, Gilbert Song, Jie Yu, James Peach, and Qian Zhang.


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


Repository: mesos


Description
---

Docker Seccomp config is a JSON file containing Seccomp filtering
rules. This patch introduces a parser for Docker Seccomp config format.
This parser accepts a JSON-string, parses and validates it, then
returns a prepared `ContainerSeccompProfile` message.


Diffs
-

  src/CMakeLists.txt 10b0946d6f49c7e9c201bad6f9f1b41cc8460fe5 
  src/Makefile.am ecb95ef152fd37e5e83c4f6415489f4cf3959578 
  src/linux/seccomp/seccomp_parser.hpp PRE-CREATION 
  src/linux/seccomp/seccomp_parser.cpp PRE-CREATION 


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


Testing
---


Thanks,

Andrei Budnik

Re: Review Request 68020: Added new `--seccomp_config_dir` flag to the agent.

[Andrei Budnik]

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

(Updated Aug. 6, 2018, 1:39 p.m.)


Review request for mesos, Gilbert Song, Jie Yu, James Peach, and Qian Zhang.


Repository: mesos


Description
---

This flag is used to specify a path to the directory containing
Seccomp profiles. This flag is used by the `linux/seccomp` isolator.


Diffs
-

  src/slave/flags.hpp eeb9708f9ec76d83b6719541f4a012544c7c0cbe 
  src/slave/flags.cpp 58cdc0f1100fe244e5bf1036e1ccf39478d5d478 


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


Testing
---


Thanks,

Andrei Budnik

Re: Review Request 68018: Added `SeccompFilter` class. (WIP)

[Andrei Budnik]

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

(Updated Aug. 6, 2018, 1:38 p.m.)


Review request for mesos, Gilbert Song, Jie Yu, James Peach, and Qian Zhang.


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


Repository: mesos


Description
---

`SeccompFilter` class is a wrapper for `libseccomp` API. Its main
purpose is to provide a translation of the `ContainerSeccompProfile`
message into calls of `libseccomp` API.


Diffs
-

  src/CMakeLists.txt 10b0946d6f49c7e9c201bad6f9f1b41cc8460fe5 
  src/Makefile.am ecb95ef152fd37e5e83c4f6415489f4cf3959578 
  src/linux/seccomp/seccomp.hpp PRE-CREATION 
  src/linux/seccomp/seccomp.cpp PRE-CREATION 


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


Testing
---


Thanks,

Andrei Budnik

Re: Review Request 68016: Added libseccomp to the build.

[Andrei Budnik]

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

(Updated Aug. 6, 2018, 1:38 p.m.)


Review request for mesos, Andrew Schwartzmeyer, Gilbert Song, Jie Yu, James 
Peach, and Qian Zhang.


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


Repository: mesos


Description
---

This library is needed to implement Seccomp syscall filtering in the
Mesos containerizer. This patch introduces `seccomp-isolator` build
flag, which is used to include or exclude sources related to Seccomp
from the build. Since Seccomp is a Linux-specific feature, the flag
is disabled by default. Enabling `seccomp-isolator` means either:

1. Compiling and linking against the bundled version of libseccomp from
   sources (default).

2. Linking against the libseccomp installed in the OS,
   if `--with-libseccomp` build flag is provided.


Diffs (updated)
-

  3rdparty/CMakeLists.txt 629b1968775da4d128b8d36c77d75efa303b0b7c 
  3rdparty/Makefile.am 6476b043090b24e8a70982a887b21096752b5581 
  3rdparty/cmake/Versions.cmake c65d7b3d2d48ce9d0b4777aeb12fdf5b605803ac 
  3rdparty/versions.am 8fd8bd4d7482af5304ab0b56270e49f447e0b3f5 
  cmake/CompilationConfigure.cmake 10cacfb99e2cff1ddd2285ae441730f61182e06d 
  configure.ac d84581823708cdece68d52bdc023e01d72645bf8 
  src/CMakeLists.txt a80b011bbd5c418ae66eb8dd7697d070168462bb 
  src/Makefile.am cf0cf22b3f582d3d4427f13288af3845aef45263 


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

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


Testing
---


Thanks,

Andrei Budnik

Re: Review Request 68017: Added Seccomp-related protobuf messages.

[Andrei Budnik]

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

(Updated Aug. 6, 2018, 1:38 p.m.)


Review request for mesos, Gilbert Song, Jie Yu, James Peach, and Qian Zhang.


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


Repository: mesos


Description
---

See summary.


Diffs (updated)
-

  include/mesos/mesos.proto 5a985fca39cdfb7e9b4775650a7e5dbe68c3b8ae 
  include/mesos/slave/containerizer.proto 
5b4dcdda0f55ea3355c78d1447c7be9ca54d9dc9 


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

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


Testing
---


Thanks,

Andrei Budnik

Re: Review Request 67844: Bundled libseccomp v2.3.3 into 3rdparty libraries.

[Andrei Budnik]

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

(Updated Aug. 6, 2018, 1:37 p.m.)


Review request for mesos, Gilbert Song, Jie Yu, James Peach, and Qian Zhang.


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


Repository: mesos


Description
---

The bundled library is downloaded from the `libseccomp` github page:
https://github.com/seccomp/libseccomp/releases/tag/v2.3.3

This library is bundled for Linux only, because Seccomp is a
Linux-specific feature.


Diffs (updated)
-

  3rdparty/libseccomp-2.3.3.tar.gz PRE-CREATION 


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

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


Testing
---


Thanks,

Andrei Budnik

Re: Review Request 68131: Added MasterPooledStateQuery_BENCHMARK_Test.

[Benno Evers]

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


Fix it, then Ship it!





src/tests/master_benchmarks.cpp
Lines 496 (patched)


Is it possible to store the last parameter directly as 
`process::Milliseconds`?



src/tests/master_benchmarks.cpp
Lines 628 (patched)


The `async()` seems unnecessary here, we can just call the function 
directly.



src/tests/master_benchmarks.cpp
Lines 634 (patched)


Maybe we should add one run here where we query only `stateEndpoint` 
without hitting `indicatorEndpoint` at the same time?


- Benno Evers


On Aug. 6, 2018, 10:30 a.m., Alexander Rukletsov wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/68131/
> ---
> 
> (Updated Aug. 6, 2018, 10:30 a.m.)
> 
> 
> Review request for mesos, Benno Evers and Benjamin Mahler.
> 
> 
> Bugs: MESOS-8975
> https://issues.apache.org/jira/browse/MESOS-8975
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> See summary.
> 
> 
> Diffs
> -
> 
>   src/tests/master_benchmarks.cpp b6d6dc7c1752491e2da854018966374b624d6682 
> 
> 
> Diff: https://reviews.apache.org/r/68131/diff/2/
> 
> 
> Testing
> ---
> 
> See https://reviews.apache.org/r/68132/
> 
> 
> Thanks,
> 
> Alexander Rukletsov
> 
>

Re: Review Request 68224: Augmented `Statistics` to work with any collection.

[Benno Evers]

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




3rdparty/libprocess/include/process/statistics.hpp
Lines 60 (patched)


Aren't you missing a `::value` after `std::is_same<>`, assuming you're 
going for SFINAE here?

Also, what's the reason for being so strict? Shouldn't 
`std::is_convertible<>` be enough?



3rdparty/libprocess/include/process/statistics.hpp
Lines 68 (patched)


I wonder if we should just reserve `std::distance(first, last)` if we don't 
get `InputIterator`s. 

Intuitively it feels like even in the worst case, doing an additional 
iteration should be faster than the additional memory allocations.



3rdparty/libprocess/src/tests/statistics_tests.cpp
Lines 28 (patched)


This is adding more characters than just writing out the namespace in the 
test ;)


- Benno Evers


On Aug. 6, 2018, 10:30 a.m., Alexander Rukletsov wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/68224/
> ---
> 
> (Updated Aug. 6, 2018, 10:30 a.m.)
> 
> 
> Review request for mesos, Benno Evers and Benjamin Mahler.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> See summary.
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/include/process/statistics.hpp 
> e9f1fc23bf83f92a2e7de94dba0df48272cc3394 
>   3rdparty/libprocess/src/tests/statistics_tests.cpp 
> a2a780bf9de018c823b68aa48977fd9fd1b8a064 
> 
> 
> Diff: https://reviews.apache.org/r/68224/diff/1/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Alexander Rukletsov
> 
>

Re: Review Request 68225: Added 25th and 75th percentiles to `Statistics`.

[Benno Evers]

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


Ship it!




Ship It!

- Benno Evers


On Aug. 6, 2018, 10:30 a.m., Alexander Rukletsov wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/68225/
> ---
> 
> (Updated Aug. 6, 2018, 10:30 a.m.)
> 
> 
> Review request for mesos, Benno Evers and Benjamin Mahler.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> See summary.
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/include/process/statistics.hpp 
> e9f1fc23bf83f92a2e7de94dba0df48272cc3394 
>   3rdparty/libprocess/src/tests/statistics_tests.cpp 
> a2a780bf9de018c823b68aa48977fd9fd1b8a064 
> 
> 
> Diff: https://reviews.apache.org/r/68225/diff/1/
> 
> 
> Testing
> ---
> 
> https://reviews.apache.org/r/68132/
> 
> 
> Thanks,
> 
> Alexander Rukletsov
> 
>

Re: Review Request 68132: Batch '/state' requests on Master.

[Alexander Rukletsov]

> On Aug. 1, 2018, 12:42 a.m., Benjamin Mahler wrote:
> > A couple of comments on the benchmark information before looking at the 
> > code, these probably belong on the previous review, but since the numbers 
> > are only shown in this one I'll leave these here:
> > 
> > * Can we compare percentiles (e.g. min, q1, q3, max) across the approaches 
> > instead of averages? i.e. how much better does min,q1,q3,max get? Averages 
> > are generally a poor fit for performance data because it doesn't tell us 
> > about the distribution (e.g. if we make p90 3x worse for a 10% benefit to 
> > average that's not ok), we can include p50 if we're interested in the 
> > half-way point.
> > * Can you include the cpu model of the box you ran this on? I'm interested 
> > in how many physical/virtual cores there are.
> > * Can you also include the regular state query benchmark measurements to 
> > make sure we're not regressing too much on the single request case? (no 
> > need to get the non-optimized build numbers).
> > * Some of the numbers don't look very good, e.g. Before `[min: 
> > 1.578161651secs, max: 8.789315237secs]` After: `[4.047655443secs, 
> > 6.00752698secs]`. Can we see the distribution here? Do you understand 
> > exactly why the lowest measurement is so much higher? Looking at the 
> > non-optimized numbers, the minimum didn't get worse? Is the data highly 
> > variable between runs?
> > * Can you also include perf data for the optimized run? 
> > http://mesos.apache.org/documentation/latest/performance-profiling/

* Sure. Updated https://reviews.apache.org/r/68131/ (see also preparatory work 
in https://reviews.apache.org/r/68224/ and https://reviews.apache.org/r/68225/).
* Done.
* Will do, stay tuned.
* This is fine and expected. Due to the batching there is an extra defer in the 
master queue, which affects the response time of the first request. Nothing to 
worry about, IMO.
* Will do, stay tuned.


- Alexander


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


On Aug. 6, 2018, 10:30 a.m., Alexander Rukletsov wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/68132/
> ---
> 
> (Updated Aug. 6, 2018, 10:30 a.m.)
> 
> 
> Review request for mesos, Benno Evers and Benjamin Mahler.
> 
> 
> Bugs: MESOS-9122
> https://issues.apache.org/jira/browse/MESOS-9122
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> With this patch handlers for '/state' requests are not scheduled
> directly after authorization, but are accumulated and then scheduled
> for later parallel processing.
> 
> This approach allows, if there are N '/state' requests in the Master's
> mailbox and T is the request response time, to block the Master actor
> only once for time O(T) instead of blocking it for time N*T prior to
> this patch.
> 
> This batching technique reduces both the time Master is spending
> answering '/state' requests and the average request response time
> in presence of multiple requests in the Master's mailbox. However,
> for seldom '/state' requests that don't accumulate in the Master's
> mailbox, the response time might increase due to an added trip
> through the mailbox.
> 
> The change preserves the read-your-writes consistency model.
> 
> 
> Diffs
> -
> 
>   src/master/http.cpp d43fbd689598612ec5946b46e2fa5e7f5e22cfa8 
>   src/master/master.hpp 209b998db8d2bad7a3812df44f0939458f48eb11 
> 
> 
> Diff: https://reviews.apache.org/r/68132/diff/2/
> 
> 
> Testing
> ---
> 
> `make check` on Mac OS 10.13.5 and various Linux distros.
> 
> Run `MasterStateQueryLoad_BENCHMARK_Test.v0State` benchmark?
> 
> **Setup**
> Processor: Intel i7-4980HQ 2.8 GHz with 6 MB on-chip L3 cache and 128 MB L4 
> cache (Crystalwell)
> Total Number of Cores: 4
> Total Number of Cores: 8
> L2 Cache (per Core): 256 KB  
> 
> Average improvement without optimization: 62%–70%.
> Average improvement with optimization: 17%–62%.
> 
> **[No batching, no 
> optimization](https://dobianchi.files.wordpress.com/2011/11/no-barrique-no-berlusconi.jpg?w=638)**
> ```
> Test setup: 100 agents with a total of 1 running tasks and 1 
> completed tasks; 10 '/state' and '/flags' requests will be sent with 200ms 
> interval
> Launching 10 '/state' requests in background
> Launching 10 '/flags' requests
> '/flags' response on average took 1.102349605secs, 10 responses are in 
> [2.662342ms, 2.143755433secs]
> '/state' response on average took 1.549122019secs, 10 responses are in 
> [494.278454ms, 2.633971927secs]
> 
> Test setup: 1000 agents with a total of 10 running tasks and 10 
> completed tasks; 10 '/state' and '/flags' requests will be sent with 200ms 
> interval
> Launching 10 

Re: Review Request 68132: Batch '/state' requests on Master.

[Alexander Rukletsov]

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

(Updated Aug. 6, 2018, 10:30 a.m.)


Review request for mesos, Benno Evers and Benjamin Mahler.


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


Repository: mesos


Description
---

With this patch handlers for '/state' requests are not scheduled
directly after authorization, but are accumulated and then scheduled
for later parallel processing.

This approach allows, if there are N '/state' requests in the Master's
mailbox and T is the request response time, to block the Master actor
only once for time O(T) instead of blocking it for time N*T prior to
this patch.

This batching technique reduces both the time Master is spending
answering '/state' requests and the average request response time
in presence of multiple requests in the Master's mailbox. However,
for seldom '/state' requests that don't accumulate in the Master's
mailbox, the response time might increase due to an added trip
through the mailbox.

The change preserves the read-your-writes consistency model.


Diffs (updated)
-

  src/master/http.cpp d43fbd689598612ec5946b46e2fa5e7f5e22cfa8 
  src/master/master.hpp 209b998db8d2bad7a3812df44f0939458f48eb11 


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

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


Testing
---

`make check` on Mac OS 10.13.5 and various Linux distros.

Run `MasterStateQueryLoad_BENCHMARK_Test.v0State` benchmark?

**Setup**
Processor: Intel i7-4980HQ 2.8 GHz with 6 MB on-chip L3 cache and 128 MB L4 
cache (Crystalwell)
Total Number of Cores: 4
Total Number of Cores: 8
L2 Cache (per Core): 256 KB  

Average improvement without optimization: 62%–70%.
Average improvement with optimization: 17%–62%.

**[No batching, no 
optimization](https://dobianchi.files.wordpress.com/2011/11/no-barrique-no-berlusconi.jpg?w=638)**
```
Test setup: 100 agents with a total of 1 running tasks and 1 completed 
tasks; 10 '/state' and '/flags' requests will be sent with 200ms interval
Launching 10 '/state' requests in background
Launching 10 '/flags' requests
'/flags' response on average took 1.102349605secs, 10 responses are in 
[2.662342ms, 2.143755433secs]
'/state' response on average took 1.549122019secs, 10 responses are in 
[494.278454ms, 2.633971927secs]

Test setup: 1000 agents with a total of 10 running tasks and 10 
completed tasks; 10 '/state' and '/flags' requests will be sent with 200ms 
interval
Launching 10 '/state' requests in background
Launching 10 '/flags' requests
'/flags' response on average took 18.436968137secs, 10 responses are in 
[2.578238ms, 33.210561732secs]
'/state' response on average took 23.916379537secs, 10 responses are in 
[5.170660597secs, 43.008091744secs]
```

**With batching but no optimization**
```
Test setup: 100 agents with a total of 1 running tasks and 1 completed 
tasks; 10 '/state' and '/flags' requests will be sent with 200ms interval
Launching 10 '/state' requests in background
Launching 10 '/flags' requests
'/flags' response on average took 417.211022ms, 10 responses are in 
[4.066901ms, 728.045442ms]
'/state' response on average took 830.351291ms, 10 responses are in 
[459.033455ms, 1.208880892secs]

Test setup: 1000 agents with a total of 10 running tasks and 10 
completed tasks; 10 '/state' and '/flags' requests will be sent with 200ms 
interval
Launching 10 '/state' requests in background
Launching 10 '/flags' requests
'/flags' response on average took 5.439950928secs, 10 responses are in 
[3.246906ms, 9.343994388secs]
'/state' response on average took 16.764607823secs, 10 responses are in 
[4.980333091secs, 18.461983916secs]
```

**No batching but `-O3` optimization**
```
Test setup: 100 agents with a total of 1 running tasks and 1 completed 
tasks; 10 '/state' and '/flags' requests will be sent with 200ms interval
Launching 10 '/state' requests in background
Launching 10 '/flags' requests
'/flags' response on average took 2.396221ms, 10 responses are in [1.628583ms, 
2.816639ms]
'/state' response on average took 113.469574ms, 10 responses are in 
[104.218099ms, 134.477062ms]

Test setup: 1000 agents with a total of 10 running tasks and 10 
completed tasks; 10 '/state' and '/flags' requests will be sent with 200ms 
interval
Launching 10 '/state' requests in background
Launching 10 '/flags' requests
'/flags' response on average took 3.892615876secs, 10 responses are in 
[2.480517ms, 7.630934838secs]
'/state' response on average took 5.205245306secs, 10 responses are in 
[1.578161651secs, 8.789315237secs]
```

**Batching and `-O3` optimization**
```
Test setup: 100 agents with a total of 1 running tasks and 1 completed 
tasks; 10 '/state' and '/flags' requests will be sent with 200ms interval
Launching 10 '/state' requests in 

Review Request 68224: Augmented `Statistics` to work with any collection.

[Alexander Rukletsov]

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

Review request for mesos, Benno Evers and Benjamin Mahler.


Repository: mesos


Description
---

See summary.


Diffs
-

  3rdparty/libprocess/include/process/statistics.hpp 
e9f1fc23bf83f92a2e7de94dba0df48272cc3394 
  3rdparty/libprocess/src/tests/statistics_tests.cpp 
a2a780bf9de018c823b68aa48977fd9fd1b8a064 


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


Testing
---

make check


Thanks,

Alexander Rukletsov

Re: Review Request 68131: Added MasterPooledStateQuery_BENCHMARK_Test.

[Alexander Rukletsov]

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

(Updated Aug. 6, 2018, 10:30 a.m.)


Review request for mesos, Benno Evers and Benjamin Mahler.


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


Repository: mesos


Description
---

See summary.


Diffs (updated)
-

  src/tests/master_benchmarks.cpp b6d6dc7c1752491e2da854018966374b624d6682 


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

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


Testing
---

See https://reviews.apache.org/r/68132/


Thanks,

Alexander Rukletsov

Review Request 68225: Added 25th and 75th percentiles to `Statistics`.

[Alexander Rukletsov]

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

Review request for mesos, Benno Evers and Benjamin Mahler.


Repository: mesos


Description
---

See summary.


Diffs
-

  3rdparty/libprocess/include/process/statistics.hpp 
e9f1fc23bf83f92a2e7de94dba0df48272cc3394 
  3rdparty/libprocess/src/tests/statistics_tests.cpp 
a2a780bf9de018c823b68aa48977fd9fd1b8a064 


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


Testing
---

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


Thanks,

Alexander Rukletsov

Re: Review Request 68227: Updated git repository url.

[Alexander Rukletsov]

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


Ship it!




Ship It!

- Alexander Rukletsov


On Aug. 6, 2018, 10:13 a.m., Benno Evers wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/68227/
> ---
> 
> (Updated Aug. 6, 2018, 10:13 a.m.)
> 
> 
> Review request for mesos.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> With the move to gitbox, the canonical upstream url changed
> from `git-wip-us.apache.org` to `gitbox.apache.org`.
> 
> 
> Diffs
> -
> 
>   docs/building.md 2b9f61dca2235bfdf2e9c1f106110d83bd9a6ce8 
> 
> 
> Diff: https://reviews.apache.org/r/68227/diff/1/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Benno Evers
> 
>

Review Request 68227: Updated git repository url.

[Benno Evers]

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

Review request for mesos.


Repository: mesos


Description
---

With the move to gitbox, the canonical upstream url changed
from `git-wip-us.apache.org` to `gitbox.apache.org`.


Diffs
-

  docs/building.md 2b9f61dca2235bfdf2e9c1f106110d83bd9a6ce8 


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


Testing
---


Thanks,

Benno Evers

Review Request 68222: Added 2 tests for `docker/volume` isolator to cover read-only volume.

[Qian Zhang]

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

Review request for mesos, Gilbert Song, Greg Mann, Ilya Pronin, and Jie Yu.


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


Repository: mesos


Description
---

Added 2 tests for `docker/volume` isolator to cover read-only volume.


Diffs
-

  src/tests/containerizer/docker_volume_isolator_tests.cpp 
c15a6fad642474765e4ad1952af6cd9ee937379e 


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


Testing
---

sudo make check


Thanks,

Qian Zhang

Review Request 68221: Updated `docker/volume` isolator to honor volume mode.

[Qian Zhang]

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

Review request for mesos, Gilbert Song, Greg Mann, Ilya Pronin, and Jie Yu.


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


Repository: mesos


Description
---

Updated `docker/volume` isolator to honor volume mode.


Diffs
-

  src/slave/containerizer/mesos/isolators/docker/volume/isolator.hpp 
76f1a5243c8d5028157f795d851b547a5ce57ac9 
  src/slave/containerizer/mesos/isolators/docker/volume/isolator.cpp 
ab749be6234a5eedc0617a131c126129f43f8d62 


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


Testing
---


Thanks,

Qian Zhang

Review Request 68220: Updated the test `ROOT_SecretInVolumeWithRootFilesystem`.

[Qian Zhang]

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

Review request for mesos, Gilbert Song, Greg Mann, Ilya Pronin, and Jie Yu.


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


Repository: mesos


Description
---

This test is updated to cover both read-write and read-only volume.


Diffs
-

  src/tests/containerizer/volume_secret_isolator_tests.cpp 
11cd3b627b056d1811ab481b8aa599c346181383 


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


Testing
---

sudo make check


Thanks,

Qian Zhang

Review Request 68219: Updated `volume/secret` isolator to honor volume mode.

[Qian Zhang]

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

Review request for mesos, Gilbert Song, Greg Mann, Ilya Pronin, and Jie Yu.


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


Repository: mesos


Description
---

Updated `volume/secret` isolator to honor volume mode.


Diffs
-

  src/slave/containerizer/mesos/isolators/volume/secret.cpp 
663aafc7173667f8f78b26bdc824a3ab2e04ed25 


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


Testing
---


Thanks,

Qian Zhang

Review Request 68218: Added a test `ROOT_ImageInReadOnlyVolumeWithoutRootFilesystem`.

[Qian Zhang]

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

Review request for mesos, Gilbert Song, Greg Mann, Ilya Pronin, and Jie Yu.


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


Repository: mesos


Description
---

Added a test `ROOT_ImageInReadOnlyVolumeWithoutRootFilesystem`.


Diffs
-

  src/tests/containerizer/volume_image_isolator_tests.cpp 
b49f0f98e3c31808d8d1e9edbb9a783bfe5231ce 


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


Testing
---

sudo make check


Thanks,

Qian Zhang

Review Request 68216: Updated `volume/image` isolator to honor volume mode.

[Qian Zhang]

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

Review request for mesos, Gilbert Song, Greg Mann, Ilya Pronin, and Jie Yu.


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


Repository: mesos


Description
---

Updated `volume/image` isolator to honor volume mode.


Diffs
-

  src/slave/containerizer/mesos/isolators/volume/image.hpp 
706b8ff28e1b1c8d15606d54d40622bc09885667 
  src/slave/containerizer/mesos/isolators/volume/image.cpp 
345772439b73b4816f71e15bb4e43a5d67c51c02 


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


Testing
---


Thanks,

Qian Zhang

Review Request 68215: Added a test `VolumeSandboxPathIsolatorTest.ROOT_SelfTypeReadOnly`.

[Qian Zhang]

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

Review request for mesos, Gilbert Song, Greg Mann, Ilya Pronin, and Jie Yu.


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


Repository: mesos


Description
---

Added a test `VolumeSandboxPathIsolatorTest.ROOT_SelfTypeReadOnly`.


Diffs
-

  src/tests/containerizer/volume_sandbox_path_isolator_tests.cpp 
97b35a4dfb4c5942858bad5fbc743fd205dd4c3c 


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


Testing
---

sudo make check


Thanks,

Qian Zhang

Review Request 68214: Updated `volume/sandbox_path` isolator to honor volume mode.

[Qian Zhang]

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

Review request for mesos, Gilbert Song, Greg Mann, Ilya Pronin, and Jie Yu.


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


Repository: mesos


Description
---

Updated `volume/sandbox_path` isolator to honor volume mode.


Diffs
-

  src/slave/containerizer/mesos/isolators/volume/sandbox_path.cpp 
4896c6811c2c59dcf00871b7a8b6b9b50da0f062 


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


Testing
---


Thanks,

Qian Zhang

Re: Review Request 68203: Updated volume isolators to honor volume mode.

[Qian Zhang]

> On Aug. 5, 2018, 1:46 p.m., Jie Yu wrote:
> > src/slave/containerizer/mesos/isolators/volume/host_path.cpp
> > Lines 318 (patched)
> > 
> >
> > I'd suggest we separate this patch, and have one patch for each 
> > isolator (we might want to backport a few, like host path volume).
> > 
> > Also, add unit test for each isolator.
> 
> Jie Yu wrote:
> NVM, i saw the subsequent tests. Let's separate this patch please. we 
> might want to backport a few to old releases
> 
> Qian Zhang wrote:
> Agree.

I will discard this patch and the subsequent test patch, and post one patch for 
each isolator.


- Qian


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


On Aug. 4, 2018, 5:58 p.m., Qian Zhang wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/68203/
> ---
> 
> (Updated Aug. 4, 2018, 5:58 p.m.)
> 
> 
> Review request for mesos, Gilbert Song, Greg Mann, Ilya Pronin, and Jie Yu.
> 
> 
> Bugs: MESOS-8814
> https://issues.apache.org/jira/browse/MESOS-8814
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Updated volume isolators to honor volume mode.
> 
> 
> Diffs
> -
> 
>   src/slave/containerizer/mesos/isolators/docker/volume/isolator.hpp 
> 76f1a5243c8d5028157f795d851b547a5ce57ac9 
>   src/slave/containerizer/mesos/isolators/docker/volume/isolator.cpp 
> ab749be6234a5eedc0617a131c126129f43f8d62 
>   src/slave/containerizer/mesos/isolators/volume/host_path.cpp 
> 9127cf4a9d8aa2b16bb5b9903103f1f76a1e2b1a 
>   src/slave/containerizer/mesos/isolators/volume/image.hpp 
> 706b8ff28e1b1c8d15606d54d40622bc09885667 
>   src/slave/containerizer/mesos/isolators/volume/image.cpp 
> 345772439b73b4816f71e15bb4e43a5d67c51c02 
>   src/slave/containerizer/mesos/isolators/volume/sandbox_path.cpp 
> 4896c6811c2c59dcf00871b7a8b6b9b50da0f062 
>   src/slave/containerizer/mesos/isolators/volume/secret.cpp 
> 663aafc7173667f8f78b26bdc824a3ab2e04ed25 
> 
> 
> Diff: https://reviews.apache.org/r/68203/diff/2/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Qian Zhang
> 
>

Review Request 68212: Updated `volume/host_path` isolator to honor volume mode.

[Qian Zhang]

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

Review request for mesos, Gilbert Song, Greg Mann, Ilya Pronin, and Jie Yu.


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


Repository: mesos


Description
---

Updated `volume/host_path` isolator to honor volume mode.


Diffs
-

  src/slave/containerizer/mesos/isolators/volume/host_path.cpp 
9127cf4a9d8aa2b16bb5b9903103f1f76a1e2b1a 


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


Testing
---


Thanks,

Qian Zhang

Re: Review Request 68203: Updated volume isolators to honor volume mode.

[Qian Zhang]

> On Aug. 5, 2018, 1:46 p.m., Jie Yu wrote:
> > src/slave/containerizer/mesos/isolators/volume/host_path.cpp
> > Lines 318 (patched)
> > 
> >
> > I'd suggest we separate this patch, and have one patch for each 
> > isolator (we might want to backport a few, like host path volume).
> > 
> > Also, add unit test for each isolator.
> 
> Jie Yu wrote:
> NVM, i saw the subsequent tests. Let's separate this patch please. we 
> might want to backport a few to old releases

Agree.


- Qian


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


On Aug. 4, 2018, 5:58 p.m., Qian Zhang wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/68203/
> ---
> 
> (Updated Aug. 4, 2018, 5:58 p.m.)
> 
> 
> Review request for mesos, Gilbert Song, Greg Mann, Ilya Pronin, and Jie Yu.
> 
> 
> Bugs: MESOS-8814
> https://issues.apache.org/jira/browse/MESOS-8814
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Updated volume isolators to honor volume mode.
> 
> 
> Diffs
> -
> 
>   src/slave/containerizer/mesos/isolators/docker/volume/isolator.hpp 
> 76f1a5243c8d5028157f795d851b547a5ce57ac9 
>   src/slave/containerizer/mesos/isolators/docker/volume/isolator.cpp 
> ab749be6234a5eedc0617a131c126129f43f8d62 
>   src/slave/containerizer/mesos/isolators/volume/host_path.cpp 
> 9127cf4a9d8aa2b16bb5b9903103f1f76a1e2b1a 
>   src/slave/containerizer/mesos/isolators/volume/image.hpp 
> 706b8ff28e1b1c8d15606d54d40622bc09885667 
>   src/slave/containerizer/mesos/isolators/volume/image.cpp 
> 345772439b73b4816f71e15bb4e43a5d67c51c02 
>   src/slave/containerizer/mesos/isolators/volume/sandbox_path.cpp 
> 4896c6811c2c59dcf00871b7a8b6b9b50da0f062 
>   src/slave/containerizer/mesos/isolators/volume/secret.cpp 
> 663aafc7173667f8f78b26bdc824a3ab2e04ed25 
> 
> 
> Diff: https://reviews.apache.org/r/68203/diff/2/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Qian Zhang
> 
>