Re: Review Request 62017: Allows port mapper plugin to have optional args.

[Mesos Reviewbot]

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



Bad patch!

Reviews applied: [62017]

Failed command: python support/apply-reviews.py -n -r 62017

Error:
2017-09-06 03:55:20 URL:https://reviews.apache.org/r/62017/diff/raw/ 
[1758/1758] -> "62017.patch" [1]
error: patch failed: 
src/slave/containerizer/mesos/isolators/network/cni/plugins/port_mapper/port_mapper.cpp:140
error: 
src/slave/containerizer/mesos/isolators/network/cni/plugins/port_mapper/port_mapper.cpp:
 patch does not apply

Full log: https://builds.apache.org/job/Mesos-Reviewbot/19217/console

- Mesos Reviewbot


On Sept. 6, 2017, 12:25 a.m., Deepak Goel wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/62017/
> ---
> 
> (Updated Sept. 6, 2017, 12:25 a.m.)
> 
> 
> Review request for mesos and Avinash sridharan.
> 
> 
> Bugs: mesos-7923
> https://issues.apache.org/jira/browse/mesos-7923
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Mesos port mapper cni plugin is a wrapper around bridge plugin
> to add port mapping functionality to bridge plugin. However, in
> certain cases the network creator doesn't need port mapping
> functionality and just want to access bridge plugin. In this case,
> the creator may not supply `args` in cni config which will makes
> mesos port mapper plugin to fail. This patch makes `args` in cni
> config optional for mesos port mapper plugin
> 
> 
> Diffs
> -
> 
>   
> src/slave/containerizer/mesos/isolators/network/cni/plugins/port_mapper/port_mapper.cpp
>  43cf3e44a55c56dc8195c9cd05f6edd8bf13d448 
> 
> 
> Diff: https://reviews.apache.org/r/62017/diff/4/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Deepak Goel
> 
>

Re: Review Request 62017: Allows port mapper plugin to have optional args.

[Deepak Goel]

> On Sept. 6, 2017, 2:09 a.m., Avinash sridharan wrote:
> > src/slave/containerizer/mesos/isolators/network/cni/plugins/port_mapper/port_mapper.cpp
> > Lines 149 (patched)
> > 
> >
> > Why are we setting this empty `args` here instead of setting up a 
> > `NetworkInfo networkInfo` object and passing it down into the constructor 
> > of the port-mapper?
> > 
> > the `port-mapper` really doesn't care about `args` it cares about 
> > `NetworkInfo`. This code seems to conflate the whole need for not setting 
> > up `args` here.
> > 
> > Also setting up `mesos.values` seems to tie the port-mapper plugin back 
> > to Mesos semantics of setting up the args which seems like a bit of a hack.

Yeah, you are right. Initially, I thought of doing that but then it led to 
making exceptions to various checks that happens before we reach to the 
constructor and the code looked even more hacky and error prone. This patch 
achieves the same goal with fewer lines of code change


- Deepak


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


On Sept. 6, 2017, 12:25 a.m., Deepak Goel wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/62017/
> ---
> 
> (Updated Sept. 6, 2017, 12:25 a.m.)
> 
> 
> Review request for mesos and Avinash sridharan.
> 
> 
> Bugs: mesos-7923
> https://issues.apache.org/jira/browse/mesos-7923
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Mesos port mapper cni plugin is a wrapper around bridge plugin
> to add port mapping functionality to bridge plugin. However, in
> certain cases the network creator doesn't need port mapping
> functionality and just want to access bridge plugin. In this case,
> the creator may not supply `args` in cni config which will makes
> mesos port mapper plugin to fail. This patch makes `args` in cni
> config optional for mesos port mapper plugin
> 
> 
> Diffs
> -
> 
>   
> src/slave/containerizer/mesos/isolators/network/cni/plugins/port_mapper/port_mapper.cpp
>  43cf3e44a55c56dc8195c9cd05f6edd8bf13d448 
> 
> 
> Diff: https://reviews.apache.org/r/62017/diff/4/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Deepak Goel
> 
>

Re: Review Request 62106: Enabled CRAM MD5 Authentication on Windows and associated tests.

[Mesos Reviewbot Windows]

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



FAIL: Mesos tests failed to build

Reviews applied: [62105, 62106]

Logs available here: 
http://dcos-win.westus.cloudapp.azure.com/mesos-build/review/62106/logs

- Mesos Reviewbot Windows


On Sept. 5, 2017, 7:05 p.m., John Kordich wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/62106/
> ---
> 
> (Updated Sept. 5, 2017, 7:05 p.m.)
> 
> 
> Review request for mesos, Andrew Schwartzmeyer, Jeff Coffler, and Joseph Wu.
> 
> 
> Bugs: MESOS-3384
> https://issues.apache.org/jira/browse/MESOS-3384
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Enabled CRAM MD5 Authentication on Windows and associated tests.
> 
> 
> Diffs
> -
> 
>   cmake/CompilationConfigure.cmake 4cb8dddf93694722af2377c4a55a195260c8a0cb 
>   docs/windows.md b47ba5a0db3d41d6f4bb002283b40622a2bb3e0f 
>   src/CMakeLists.txt 1a828c4351ded36f51ccbbe67147da2f50b9cdb1 
>   src/authentication/cram_md5/authenticatee.cpp 
> 7b3f767e29163de489018081089e67a6f22971c5 
>   src/authentication/cram_md5/authenticator.cpp 
> 2a12bb3f4aec79056ec6c82a8c896ee3af15bb4c 
>   src/authentication/cram_md5/auxprop.hpp 
> dedcbe514283dd86a924d92ccdd17173ebe878cb 
>   src/tests/CMakeLists.txt def233d13f627f58362fdc61ac3e9c104d65fe7e 
>   src/tests/authentication_tests.cpp b04be76d23788fbde337eaf85716ec7436ade691 
> 
> 
> Diff: https://reviews.apache.org/r/62106/diff/1/
> 
> 
> Testing
> ---
> 
> Ran the automake build and the cmake build on Linux, as well as the cmake 
> build on Windows, with no errors.  Ran tests on both platforms as well.  
> Note: The mesos-3rdparty pull request which contains the cyrus-sasl package 
> needs to be merged to master first before these patches will work. (Or, have 
> the cyrus-sasl tarball available in a local 3rdparty directory, configured 
> with mesos)
> 
> 
> Thanks,
> 
> John Kordich
> 
>

Re: Review Request 62017: Allows port mapper plugin to have optional args.

[Mesos Reviewbot Windows]

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



FAIL: Mesos tests failed to run

Reviews applied: [62017]

Logs available here: 
http://dcos-win.westus.cloudapp.azure.com/mesos-build/review/62017/logs

- Mesos Reviewbot Windows


On Sept. 6, 2017, 12:25 a.m., Deepak Goel wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/62017/
> ---
> 
> (Updated Sept. 6, 2017, 12:25 a.m.)
> 
> 
> Review request for mesos and Avinash sridharan.
> 
> 
> Bugs: mesos-7923
> https://issues.apache.org/jira/browse/mesos-7923
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Mesos port mapper cni plugin is a wrapper around bridge plugin
> to add port mapping functionality to bridge plugin. However, in
> certain cases the network creator doesn't need port mapping
> functionality and just want to access bridge plugin. In this case,
> the creator may not supply `args` in cni config which will makes
> mesos port mapper plugin to fail. This patch makes `args` in cni
> config optional for mesos port mapper plugin
> 
> 
> Diffs
> -
> 
>   
> src/slave/containerizer/mesos/isolators/network/cni/plugins/port_mapper/port_mapper.cpp
>  43cf3e44a55c56dc8195c9cd05f6edd8bf13d448 
> 
> 
> Diff: https://reviews.apache.org/r/62017/diff/4/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Deepak Goel
> 
>

Re: Review Request 62017: Allows port mapper plugin to have optional args.

[Avinash sridharan]

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




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


Why are we setting this empty `args` here instead of setting up a 
`NetworkInfo networkInfo` object and passing it down into the constructor of 
the port-mapper?

the `port-mapper` really doesn't care about `args` it cares about 
`NetworkInfo`. This code seems to conflate the whole need for not setting up 
`args` here.

Also setting up `mesos.values` seems to tie the port-mapper plugin back to 
Mesos semantics of setting up the args which seems like a bit of a hack.


- Avinash sridharan


On Sept. 6, 2017, 12:25 a.m., Deepak Goel wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/62017/
> ---
> 
> (Updated Sept. 6, 2017, 12:25 a.m.)
> 
> 
> Review request for mesos and Avinash sridharan.
> 
> 
> Bugs: mesos-7923
> https://issues.apache.org/jira/browse/mesos-7923
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Mesos port mapper cni plugin is a wrapper around bridge plugin
> to add port mapping functionality to bridge plugin. However, in
> certain cases the network creator doesn't need port mapping
> functionality and just want to access bridge plugin. In this case,
> the creator may not supply `args` in cni config which will makes
> mesos port mapper plugin to fail. This patch makes `args` in cni
> config optional for mesos port mapper plugin
> 
> 
> Diffs
> -
> 
>   
> src/slave/containerizer/mesos/isolators/network/cni/plugins/port_mapper/port_mapper.cpp
>  43cf3e44a55c56dc8195c9cd05f6edd8bf13d448 
> 
> 
> Diff: https://reviews.apache.org/r/62017/diff/4/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Deepak Goel
> 
>

Re: Review Request 62106: Enabled CRAM MD5 Authentication on Windows and associated tests.

[Mesos Reviewbot]

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



Patch looks great!

Reviews applied: [62105, 62106]

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

- Mesos Reviewbot


On Sept. 6, 2017, 12:05 a.m., John Kordich wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/62106/
> ---
> 
> (Updated Sept. 6, 2017, 12:05 a.m.)
> 
> 
> Review request for mesos, Andrew Schwartzmeyer, Jeff Coffler, and Joseph Wu.
> 
> 
> Bugs: MESOS-3384
> https://issues.apache.org/jira/browse/MESOS-3384
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Enabled CRAM MD5 Authentication on Windows and associated tests.
> 
> 
> Diffs
> -
> 
>   cmake/CompilationConfigure.cmake 4cb8dddf93694722af2377c4a55a195260c8a0cb 
>   docs/windows.md b47ba5a0db3d41d6f4bb002283b40622a2bb3e0f 
>   src/CMakeLists.txt 1a828c4351ded36f51ccbbe67147da2f50b9cdb1 
>   src/authentication/cram_md5/authenticatee.cpp 
> 7b3f767e29163de489018081089e67a6f22971c5 
>   src/authentication/cram_md5/authenticator.cpp 
> 2a12bb3f4aec79056ec6c82a8c896ee3af15bb4c 
>   src/authentication/cram_md5/auxprop.hpp 
> dedcbe514283dd86a924d92ccdd17173ebe878cb 
>   src/tests/CMakeLists.txt def233d13f627f58362fdc61ac3e9c104d65fe7e 
>   src/tests/authentication_tests.cpp b04be76d23788fbde337eaf85716ec7436ade691 
> 
> 
> Diff: https://reviews.apache.org/r/62106/diff/1/
> 
> 
> Testing
> ---
> 
> Ran the automake build and the cmake build on Linux, as well as the cmake 
> build on Windows, with no errors.  Ran tests on both platforms as well.  
> Note: The mesos-3rdparty pull request which contains the cyrus-sasl package 
> needs to be merged to master first before these patches will work. (Or, have 
> the cyrus-sasl tarball available in a local 3rdparty directory, configured 
> with mesos)
> 
> 
> Thanks,
> 
> John Kordich
> 
>

Re: Review Request 62017: Allows port mapper plugin to have optional args.

[Deepak Goel]

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

(Updated Sept. 6, 2017, 12:25 a.m.)


Review request for mesos and Avinash sridharan.


Bugs: mesos-7923
https://issues.apache.org/jira/browse/mesos-7923


Repository: mesos


Description
---

Mesos port mapper cni plugin is a wrapper around bridge plugin
to add port mapping functionality to bridge plugin. However, in
certain cases the network creator doesn't need port mapping
functionality and just want to access bridge plugin. In this case,
the creator may not supply `args` in cni config which will makes
mesos port mapper plugin to fail. This patch makes `args` in cni
config optional for mesos port mapper plugin


Diffs (updated)
-

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


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

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


Testing
---


Thanks,

Deepak Goel

Review Request 62106: Enabled CRAM MD5 Authentication on Windows and associated tests.

[John Kordich via Review Board]

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

Review request for mesos, Andrew Schwartzmeyer, Jeff Coffler, and Joseph Wu.


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


Repository: mesos


Description
---

Enabled CRAM MD5 Authentication on Windows and associated tests.


Diffs
-

  cmake/CompilationConfigure.cmake 4cb8dddf93694722af2377c4a55a195260c8a0cb 
  docs/windows.md b47ba5a0db3d41d6f4bb002283b40622a2bb3e0f 
  src/CMakeLists.txt 1a828c4351ded36f51ccbbe67147da2f50b9cdb1 
  src/authentication/cram_md5/authenticatee.cpp 
7b3f767e29163de489018081089e67a6f22971c5 
  src/authentication/cram_md5/authenticator.cpp 
2a12bb3f4aec79056ec6c82a8c896ee3af15bb4c 
  src/authentication/cram_md5/auxprop.hpp 
dedcbe514283dd86a924d92ccdd17173ebe878cb 
  src/tests/CMakeLists.txt def233d13f627f58362fdc61ac3e9c104d65fe7e 
  src/tests/authentication_tests.cpp b04be76d23788fbde337eaf85716ec7436ade691 


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


Testing
---

Ran the automake build and the cmake build on Linux, as well as the cmake build 
on Windows, with no errors.  Ran tests on both platforms as well.  Note: The 
mesos-3rdparty pull request which contains the cyrus-sasl package needs to be 
merged to master first before these patches will work. (Or, have the cyrus-sasl 
tarball available in a local 3rdparty directory, configured with mesos)


Thanks,

John Kordich

Review Request 62105: Enabled building of the Cyrus SASL CRAM MD5 lib on Windows.

[John Kordich via Review Board]

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

Review request for mesos, Andrew Schwartzmeyer, Jeff Coffler, and Joseph Wu.


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


Repository: mesos


Description
---

Enabled building of the Cyrus SASL CRAM MD5 lib on Windows.


Diffs
-

  3rdparty/CMakeLists.txt 2a183a4a46bf8bc97455fe02648407ba561b38e7 
  3rdparty/cmake/Versions.cmake 0b62ae96817ade616f8dfc52cf8df995f8fd8927 
  3rdparty/cyrus_sasl-2.1.27rc3.patch PRE-CREATION 
  src/slave/containerizer/mesos/containerizer.cpp 
4d5dc13f363f5d8886983d7dd06a5cecc177c345 
  src/slave/containerizer/mesos/isolators/filesystem/linux.cpp 
faf94909f995f7486b5f9cb7532af58a90a9eed3 
  src/slave/containerizer/mesos/isolators/volume/sandbox_path.cpp 
ee5ea3dee3be197e923be544aab96806f0adf1cf 
  src/tests/containerizer/linux_filesystem_isolator_tests.cpp 
76cc007963b4cbe162556d03d3e41a0a5e660167 
  src/tests/containerizer/volume_sandbox_path_isolator_tests.cpp 
2bc9b0890d384e9ce8d5d4679c974c5e88231eed 


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


Testing
---


Thanks,

John Kordich

Re: Review Request 61925: Utilized 'AuthorizationAcceptor' in agent endpoint handlers.

[Mesos Reviewbot Windows]

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



FAIL: Mesos tests failed to run

Reviews applied: [61922, 61923, 61924, 61925]

Logs available here: 
http://dcos-win.westus.cloudapp.azure.com/mesos-build/review/61925/logs

- Mesos Reviewbot Windows


On Sept. 5, 2017, 8:28 p.m., Greg Mann wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/61925/
> ---
> 
> (Updated Sept. 5, 2017, 8:28 p.m.)
> 
> 
> Review request for mesos and Greg Mann.
> 
> 
> Bugs: MESOS-7914
> https://issues.apache.org/jira/browse/MESOS-7914
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This patch updates the agent endpoint handlers to make use of
> the `AuthorizationAcceptor` exclusively for authorization,
> eliminating the need to explicitly create authorization
> subjects and objects.
> 
> Endpoint-related slave authorization tests are also updated to
> accommodate this change.
> 
> 
> Diffs
> -
> 
>   src/slave/http.hpp 44a95dec4c9b8bb65d712c5538bbd7afffe2cf7b 
>   src/slave/http.cpp 3ea7829df8c1c35d2fa3a44f19a60b7e261042ce 
>   src/tests/slave_authorization_tests.cpp 
> 30eceae0920351cffc9c3393c6d08917c4041c1a 
> 
> 
> Diff: https://reviews.apache.org/r/61925/diff/2/
> 
> 
> Testing
> ---
> 
> `make check`
> 
> 
> Thanks,
> 
> Greg Mann
> 
>

Re: Review Request 62017: Allows port mapper plugin to have optional args.

[Mesos Reviewbot Windows]

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



FAIL: Mesos tests failed to run

Reviews applied: [62017]

Logs available here: 
http://dcos-win.westus.cloudapp.azure.com/mesos-build/review/62017/logs

- Mesos Reviewbot Windows


On Sept. 5, 2017, 9:47 p.m., Deepak Goel wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/62017/
> ---
> 
> (Updated Sept. 5, 2017, 9:47 p.m.)
> 
> 
> Review request for mesos and Avinash sridharan.
> 
> 
> Bugs: mesos-7923
> https://issues.apache.org/jira/browse/mesos-7923
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Mesos port mapper cni plugin is a wrapper around bridge plugin
> to add port mapping functionality to bridge plugin. However, in
> certain cases the network creator doesn't need port mapping
> functionality and just want to access bridge plugin. In this case,
> the creator may not supply `args` in cni config which will makes
> mesos port mapper plugin to fail. This patch makes `args` in cni
> config optional for mesos port mapper plugin
> 
> 
> Diffs
> -
> 
>   
> src/slave/containerizer/mesos/isolators/network/cni/plugins/port_mapper/port_mapper.cpp
>  43cf3e44a55c56dc8195c9cd05f6edd8bf13d448 
> 
> 
> Diff: https://reviews.apache.org/r/62017/diff/3/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Deepak Goel
> 
>

Re: Review Request 62017: Allows port mapper plugin to have optional args.

[Qian Zhang]

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


Fix it, then Ship it!





src/slave/containerizer/mesos/isolators/network/cni/plugins/port_mapper/port_mapper.cpp
Lines 145-150 (original), 148-159 (patched)


What about changing it to the below?
```
  if (args.isError()) {
return PluginError(
"Failed to get the required field 'args': " + args.error(),
ERROR_BAD_ARGS);
  } else if (args.isNone()) {
JSON::Object _args;
JSON::Object mesos;
mesos.values["network_info"] = JSON::Object();
_args.values["org.apache.mesos"] = mesos;
args = _args;
  }
```


- Qian Zhang


On Sept. 6, 2017, 5:47 a.m., Deepak Goel wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/62017/
> ---
> 
> (Updated Sept. 6, 2017, 5:47 a.m.)
> 
> 
> Review request for mesos and Avinash sridharan.
> 
> 
> Bugs: mesos-7923
> https://issues.apache.org/jira/browse/mesos-7923
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Mesos port mapper cni plugin is a wrapper around bridge plugin
> to add port mapping functionality to bridge plugin. However, in
> certain cases the network creator doesn't need port mapping
> functionality and just want to access bridge plugin. In this case,
> the creator may not supply `args` in cni config which will makes
> mesos port mapper plugin to fail. This patch makes `args` in cni
> config optional for mesos port mapper plugin
> 
> 
> Diffs
> -
> 
>   
> src/slave/containerizer/mesos/isolators/network/cni/plugins/port_mapper/port_mapper.cpp
>  43cf3e44a55c56dc8195c9cd05f6edd8bf13d448 
> 
> 
> Diff: https://reviews.apache.org/r/62017/diff/3/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Deepak Goel
> 
>

Re: Review Request 62017: Allows port mapper plugin to have optional args.

[Deepak Goel]

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

(Updated Sept. 5, 2017, 9:47 p.m.)


Review request for mesos and Avinash sridharan.


Bugs: mesos-7923
https://issues.apache.org/jira/browse/mesos-7923


Repository: mesos


Description
---

Mesos port mapper cni plugin is a wrapper around bridge plugin
to add port mapping functionality to bridge plugin. However, in
certain cases the network creator doesn't need port mapping
functionality and just want to access bridge plugin. In this case,
the creator may not supply `args` in cni config which will makes
mesos port mapper plugin to fail. This patch makes `args` in cni
config optional for mesos port mapper plugin


Diffs (updated)
-

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


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

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


Testing
---


Thanks,

Deepak Goel

Re: Review Request 62094: Updated CMake version in docker build helper.

[Mesos Reviewbot Windows]

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



FAIL: Mesos tests failed to run

Reviews applied: [62094]

Logs available here: 
http://dcos-win.westus.cloudapp.azure.com/mesos-build/review/62094/logs

- Mesos Reviewbot Windows


On Sept. 5, 2017, 7:29 p.m., Joseph Wu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/62094/
> ---
> 
> (Updated Sept. 5, 2017, 7:29 p.m.)
> 
> 
> Review request for mesos, Alexander Rukletsov and Vinod Kone.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Since the minimum version of CMake was bumped to 3.7,
> this updates the docker build helper to download CMake 3.8
> (stable release that has been out for some time).  The reason
> we do not use a package repository for CMake is that none
> of them offer a version of CMake beyond 3.5.
> 
> 
> Diffs
> -
> 
>   support/docker-build.sh d17cc765a11097227f25b1897f61676677f83b87 
> 
> 
> Diff: https://reviews.apache.org/r/62094/diff/1/
> 
> 
> Testing
> ---
> 
> Ran this until it got past the CMake version check.
> 
> OS="ubuntu:14.04" BUILDTOOL=cmake COMPILER=gcc CONFIGURATION="--disable-java 
> --disable-python" ENVIRONMENT="MESOS_VERBOSE=1" sudo -E 
> ./support/docker-build.sh
> 
> 
> Thanks,
> 
> Joseph Wu
> 
>

Re: Review Request 62069: Moved SANDBOX_PATH volume tests to the right place.

[Joseph Wu]

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


Ship it!




Ship It!

- Joseph Wu


On Sept. 4, 2017, 10:50 a.m., Jie Yu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/62069/
> ---
> 
> (Updated Sept. 4, 2017, 10:50 a.m.)
> 
> 
> Review request for mesos, Gilbert Song and Joseph Wu.
> 
> 
> Bugs: MESOS-7936
> https://issues.apache.org/jira/browse/MESOS-7936
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This patch moved the SANDBOX_PATH volume tests from linux filesystem
> isolator tests to volume sandbox path isolator tests.
> 
> 
> Diffs
> -
> 
>   src/tests/containerizer/linux_filesystem_isolator_tests.cpp 
> 1ad9464e99b7dd4dfd2c9bd1a4c6c9c5cce615bd 
>   src/tests/containerizer/volume_sandbox_path_isolator_tests.cpp 
> 0da01c11ea538185dae1b714a80d81a14add0f0c 
> 
> 
> Diff: https://reviews.apache.org/r/62069/diff/1/
> 
> 
> Testing
> ---
> 
> sudo make check
> 
> 
> Thanks,
> 
> Jie Yu
> 
>

Re: Review Request 60546: Harden Mesos when building with cmake.

[Joseph Wu]

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


Ship it!




I'm going to apply this review in spirit (because the CMake build was rewritten 
underneath it).

I'll make the below changes before committing.


cmake/CompilationConfigure.cmake
Lines 202-212 (patched)


Going to replace this:
`set(CMAKE_CXX_FLAGS "${CMAKE_CXX_FLAGS} `

With this:
`string(APPEND CMAKE_CXX_FLAGS " `



cmake/CompilationConfigure.cmake
Lines 219-223 (patched)


Going to replace this with the cross-platform equivalent:
```
set(CMAKE_POSITION_INDEPENDENT_CODE TRUE)
```

See: 
https://cmake.org/cmake/help/v3.0/prop_tgt/POSITION_INDEPENDENT_CODE.html



cmake/MesosConfigure.cmake
Lines 65-70 (patched)


Ditto with above.


- Joseph Wu


On July 5, 2017, 9:06 a.m., Aaron Wood wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/60546/
> ---
> 
> (Updated July 5, 2017, 9:06 a.m.)
> 
> 
> Review request for mesos, Andrew Schwartzmeyer, Benjamin Bannier, and Joseph 
> Wu.
> 
> 
> Bugs: MESOS-7737
> https://issues.apache.org/jira/browse/MESOS-7737
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Apply stack protectors (stronger stack protectors with newer compilers), 
> position independent code suitable for linking into executables, security 
> warnings, and generate position independent executables.
> 
> 
> Diffs
> -
> 
>   cmake/CompilationConfigure.cmake 3fa2e2f3b 
>   cmake/MesosConfigure.cmake 5ecad2c0f 
> 
> 
> Diff: https://reviews.apache.org/r/60546/diff/2/
> 
> 
> Testing
> ---
> 
> `mkdir build && cd build && cmake .. && make -j$(nproc) && make check 
> -j$(nproc)`
> 
> 
> Thanks,
> 
> Aaron Wood
> 
>

Re: Review Request 61925: Utilized 'AuthorizationAcceptor' in agent endpoint handlers.

[Greg Mann]

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

(Updated Sept. 5, 2017, 8:28 p.m.)


Review request for mesos and Greg Mann.


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


Repository: mesos


Description
---

This patch updates the agent endpoint handlers to make use of
the `AuthorizationAcceptor` exclusively for authorization,
eliminating the need to explicitly create authorization
subjects and objects.

Endpoint-related slave authorization tests are also updated to
accommodate this change.


Diffs (updated)
-

  src/slave/http.hpp 44a95dec4c9b8bb65d712c5538bbd7afffe2cf7b 
  src/slave/http.cpp 3ea7829df8c1c35d2fa3a44f19a60b7e261042ce 
  src/tests/slave_authorization_tests.cpp 
30eceae0920351cffc9c3393c6d08917c4041c1a 


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

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


Testing
---

`make check`


Thanks,

Greg Mann

Re: Review Request 61922: Added a move constructor for `Owned`.

[Greg Mann]

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

(Updated Sept. 5, 2017, 8:04 p.m.)


Review request for mesos and Greg Mann.


Repository: mesos


Description
---

Added a move constructor for `Owned`.


Diffs (updated)
-

  3rdparty/libprocess/include/process/owned.hpp 
e1ae6dcc771cf2244bc16bbab19e9f05fa1704cc 


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

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


Testing
---

`make check`


Thanks,

Greg Mann

Re: Review Request 62053: Removed garbage collector.

[Mesos Reviewbot Windows]

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



FAIL: Mesos tests failed to run

Reviews applied: [62053]

Logs available here: 
http://dcos-win.westus.cloudapp.azure.com/mesos-build/review/62053/logs

- Mesos Reviewbot Windows


On Sept. 2, 2017, 3:07 p.m., Benjamin Hindman wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/62053/
> ---
> 
> (Updated Sept. 2, 2017, 3:07 p.m.)
> 
> 
> Review request for mesos, Benjamin Mahler and Jiang Yan Xu.
> 
> 
> Bugs: MESOS-7921
> https://issues.apache.org/jira/browse/MESOS-7921
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> The garbage collector had at least two bugs:
> 
> (1) If someone dispatched `manage()` twice in a row the process we're
> waiting for will get overwritten which can wreak havoc depending
> on when the calls to `link()` happen.
> 
> (2) The garbage collector was deleting after an exited event rather
> than actually doing a `wait()`.
> 
> The simpler implementation that this patch introduces is to just
> delete the process after doing `ProcessManager::cleanup()`.
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/include/Makefile.am 
> c5dc0bb0d2d77987531ead50277940700c62b84f 
>   3rdparty/libprocess/include/process/gc.hpp 
> 603bb8bb084a8d2774ab1077da671f659a22a376 
>   3rdparty/libprocess/include/process/process.hpp 
> 8cca782ae89727bc5570afc4ed96c556f14c8c68 
>   3rdparty/libprocess/src/process.cpp 
> afa53537a5c7d4d8b0f4e3b8e04d7d0f2c4c6631 
>   3rdparty/libprocess/src/tests/process_tests.cpp 
> 8d36600701a795a7fa8d73a844657ff98eee6aa7 
> 
> 
> Diff: https://reviews.apache.org/r/62053/diff/2/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Benjamin Hindman
> 
>

Review Request 62094: Updated CMake version in docker build helper.

[Joseph Wu]

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

Review request for mesos, Alexander Rukletsov and Vinod Kone.


Repository: mesos


Description
---

Since the minimum version of CMake was bumped to 3.7,
this updates the docker build helper to download CMake 3.8
(stable release that has been out for some time).  The reason
we do not use a package repository for CMake is that none
of them offer a version of CMake beyond 3.5.


Diffs
-

  support/docker-build.sh d17cc765a11097227f25b1897f61676677f83b87 


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


Testing
---

Ran this until it got past the CMake version check.

OS="ubuntu:14.04" BUILDTOOL=cmake COMPILER=gcc CONFIGURATION="--disable-java 
--disable-python" ENVIRONMENT="MESOS_VERBOSE=1" sudo -E 
./support/docker-build.sh


Thanks,

Joseph Wu

Re: Review Request 60766: Ignored containers that join CNI networks.

[James Peach]

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

(Updated Sept. 5, 2017, 5:57 p.m.)


Review request for mesos, Qian Zhang and Jiang Yan Xu.


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


Repository: mesos


Description
---

Working on the assumption that containers with CNI networks will
get their own IP addresses and don't need port isolation, ignore
any containers that are joining CNI networks.


Diffs (updated)
-

  src/slave/containerizer/mesos/containerizer.cpp 
4ff014e278100ea99ad93f02c6688ec9ac047059 
  src/slave/containerizer/mesos/isolators/network/ports.hpp PRE-CREATION 
  src/slave/containerizer/mesos/isolators/network/ports.cpp PRE-CREATION 


Diff: https://reviews.apache.org/r/60766/diff/15/

Changes: https://reviews.apache.org/r/60766/diff/14-15/


Testing
---

make check (Fedora 26).


Thanks,

James Peach

Re: Review Request 60496: Added socket checking to the network ports isolator.

[James Peach]

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

(Updated Sept. 5, 2017, 5:39 p.m.)


Review request for mesos, Qian Zhang and Jiang Yan Xu.


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


Repository: mesos


Description
---

Implemented ports resource restrictions in the network ports isolator.
Periodically, scan for listening sockets and match them up to all
the open sockets in the containers we are tracking in the network.
Check any sockets we find against the ports resource and trigger a
resource limitation if the port has not been allocated.


Diffs (updated)
-

  src/slave/containerizer/mesos/isolators/network/ports.hpp PRE-CREATION 
  src/slave/containerizer/mesos/isolators/network/ports.cpp PRE-CREATION 


Diff: https://reviews.apache.org/r/60496/diff/17/

Changes: https://reviews.apache.org/r/60496/diff/16-17/


Testing
---

make check (Fedora 26)


Thanks,

James Peach

Re: Review Request 60496: Added socket checking to the network ports isolator.

[James Peach]

> On Sept. 5, 2017, 7:12 a.m., Qian Zhang wrote:
> > src/slave/containerizer/mesos/isolators/network/ports.cpp
> > Lines 271-284 (patched)
> > 
> >
> > Mind to explain why running the loop inside the isolator process (since 
> > you supplied `pid` as the first parameter of `process::loop()`) and making 
> > `collectContainerListeners()` a static function?
> > 
> > Can we follow the similar way of what DRF allocator process does? I 
> > mean we can run the loop outside the isolator process and make 
> > `collectContainerListeners()` a member method of 
> > `NetworkPortsIsolatorProcess`, and then invoke 
> > `collectContainerListeners()` with a `dispatch()`. In this way, we do not 
> > need to pass those parameters (like `portsProcess->infos.keys()`) to 
> > `collectContainerListeners()` since as a member method it can access them 
> > naturally.

> Why running the loop inside the isolator process

We need to sample the updated set of container IDs (`info.keys()`), which means 
that at some point we need to dispatch onto the isolator process (to ensure 
serialized access). If we ran the `loop` on a separate process, we would need a 
helper method returning a `Future` on the isolator that we 
could `dispatch` to. This would be equivalent to the code we have here. 
`collectContainerListeners()` is a static function because of your previous 
feedback that you didn't want to see a separate class to manage the additional 
process.

> Can we follow the similar way of what DRF allocator process does?

Collecting the ports is expensive (as discussed elsewhere we need to scan a 
large number of `/proc/fd` entries). If we `dispatch` this onto the main 
process, then we block the isolator process for a (potentially) long time. The 
very first version of this patch series implemented this suggestion and I 
re-wrote it because we agreed that we didn't want to block the isolator process.


- James


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


On Aug. 30, 2017, 11:19 p.m., James Peach wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/60496/
> ---
> 
> (Updated Aug. 30, 2017, 11:19 p.m.)
> 
> 
> Review request for mesos, Qian Zhang and Jiang Yan Xu.
> 
> 
> Bugs: MESOS-7675
> https://issues.apache.org/jira/browse/MESOS-7675
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Implemented ports resource restrictions in the network ports isolator.
> Periodically, scan for listening sockets and match them up to all
> the open sockets in the containers we are tracking in the network.
> Check any sockets we find against the ports resource and trigger a
> resource limitation if the port has not been allocated.
> 
> 
> Diffs
> -
> 
>   src/slave/containerizer/mesos/isolators/network/ports.hpp PRE-CREATION 
>   src/slave/containerizer/mesos/isolators/network/ports.cpp PRE-CREATION 
> 
> 
> Diff: https://reviews.apache.org/r/60496/diff/16/
> 
> 
> Testing
> ---
> 
> make check (Fedora 26)
> 
> 
> Thanks,
> 
> James Peach
> 
>

Re: Review Request 61213: Added 'mesos task list' command to CLI.

[Mesos Reviewbot Windows]

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



FAIL: 'Mesos testes failed to run'

Reviews applied: [61212, 61213]

Logs available here: 
'http://mesos-logs.westus.cloudapp.azure.com/mesos-build/review/61211/logs'

- Mesos Reviewbot Windows


On Sept. 4, 2017, 2:49 p.m., Armand Grillet wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/61213/
> ---
> 
> (Updated Sept. 4, 2017, 2:49 p.m.)
> 
> 
> Review request for mesos and Kevin Klues.
> 
> 
> Bugs: MESOS-7840
> https://issues.apache.org/jira/browse/MESOS-7840
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This command displays the active tasks in a cluster
> by reaching the tasks endpoint of a master.
> 
> 
> Diffs
> -
> 
>   src/python/cli_new/bin/settings.py d42df04b0ff42bb6f466842e59223cd90a74d5c0 
>   src/python/cli_new/lib/cli/plugins/task/__init__.py PRE-CREATION 
>   src/python/cli_new/lib/cli/plugins/task/main.py PRE-CREATION 
>   src/python/cli_new/lib/cli/tests/__init__.py 
> 0daf28869e107263c51653ace39e3b1826871048 
>   src/python/cli_new/lib/cli/tests/task.py PRE-CREATION 
>   src/python/cli_new/tests/main.py 3e4d2e449a6485206700b4a490d325a393d31f90 
> 
> 
> Diff: https://reviews.apache.org/r/61213/diff/3/
> 
> 
> Testing
> ---
> 
> To test with one master:
> ```
> $ ./bootstrap
> $ source activate
> $ mesos-cli-tests
> ```
> 
> To test the command in High-Availability Mode I have:
> 1. Started ZooKeeper.
> 2. Started two masters using the flags `--zk` and `--quorum`.
> 3. Started one agent.
> 4. Launched one task using `./mesos-execute 
> --master='zk://127.0.0.1:2181/mesos' --name='test' --command='sleep 3600'`.
> 5. Checked that the output of 'mesos task list' was correct.
> 
> I also checked that the Python linter was still working.
> 
> 
> Thanks,
> 
> Armand Grillet
> 
>

Re: Review Request 62061: Revert "Removed core file during distclean.".

[Mesos Reviewbot Windows]

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



FAIL: 'Mesos testes failed to run'

Reviews applied: [61797, 61798, 61799, 61800, 61801, 62061]

Logs available here: 
'http://mesos-logs.westus.cloudapp.azure.com/mesos-build/review/61211/logs'

- Mesos Reviewbot Windows


On Sept. 4, 2017, 12:14 p.m., Andrei Budnik wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/62061/
> ---
> 
> (Updated Sept. 4, 2017, 12:14 p.m.)
> 
> 
> Review request for mesos, Kapil Arya and Vinod Kone.
> 
> 
> Bugs: MESOS-7722
> https://issues.apache.org/jira/browse/MESOS-7722
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This reverts commit fcf8618be59ada6d1607696b13bc362bf7e85b0b.
> 
> As we have fixed coredumps caused by passing an invalid path to
> executable to `subprocess` (see https://reviews.apache.org/r/61799),
> invocation of `perf` command, which isn't installed,
> won't cause coredumps.
> 
> 
> Diffs
> -
> 
>   src/Makefile.am a4f2ee008eba263fe52c4e58c305e3fb9ba42f6b 
> 
> 
> Diff: https://reviews.apache.org/r/62061/diff/1/
> 
> 
> Testing
> ---
> 
> make check
> apache CI
> 
> 
> Thanks,
> 
> Andrei Budnik
> 
>

Re: Review Request 62065: Added 'mesos agent list' command to CLI.

[Mesos Reviewbot Windows]

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



FAIL: 'Mesos testes failed to run'

Reviews applied: [61212, 62065]

Logs available here: 
'http://mesos-logs.westus.cloudapp.azure.com/mesos-build/review/61211/logs'

- Mesos Reviewbot Windows


On Sept. 4, 2017, 2:51 p.m., Armand Grillet wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/62065/
> ---
> 
> (Updated Sept. 4, 2017, 2:51 p.m.)
> 
> 
> Review request for mesos and Kevin Klues.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This command displays the agents in a cluster by
> reaching the slaves endpoint of a master. It also
> works when Mesos runs in High-Availability Mode.
> 
> 
> Diffs
> -
> 
>   src/python/cli_new/bin/settings.py d42df04b0ff42bb6f466842e59223cd90a74d5c0 
>   src/python/cli_new/lib/cli/plugins/agent/__init__.py PRE-CREATION 
>   src/python/cli_new/lib/cli/plugins/agent/main.py PRE-CREATION 
>   src/python/cli_new/lib/cli/tests/__init__.py 
> 0daf28869e107263c51653ace39e3b1826871048 
>   src/python/cli_new/lib/cli/tests/agent.py PRE-CREATION 
>   src/python/cli_new/tests/main.py 3e4d2e449a6485206700b4a490d325a393d31f90 
> 
> 
> Diff: https://reviews.apache.org/r/62065/diff/1/
> 
> 
> Testing
> ---
> 
> To test with one master:
> 
> $ ./bootstrap
> $ source activate
> $ mesos-cli-tests
> 
> 
> To test the command in High-Availability Mode I have:
> 1. Started ZooKeeper.
> 2. Started two masters using the flags --zk and --quorum.
> 3. Started one agent.
> 4. Launched one task using ./mesos-execute 
> --master='zk://127.0.0.1:2181/mesos' --name='test' --command='sleep 3600'.
> 5. Checked that the output of 'mesos agent list' was correct.
> 
> I also checked that the Python linter was still working.
> 
> 
> Thanks,
> 
> Armand Grillet
> 
>

Re: Review Request 62071: Added non-src files to CMake project to facilitate indexing.

[Mesos Reviewbot Windows]

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



FAIL: 'Mesos testes failed to run'

Reviews applied: [62070, 62071]

Logs available here: 
'http://mesos-logs.westus.cloudapp.azure.com/mesos-build/review/61211/logs'

- Mesos Reviewbot Windows


On Sept. 4, 2017, 6:01 p.m., Alexander Rukletsov wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/62071/
> ---
> 
> (Updated Sept. 4, 2017, 6:01 p.m.)
> 
> 
> Review request for mesos, Andrew Schwartzmeyer and Joseph Wu.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Some IDEs and indexing tools build the project structure from CMake
> target definitions. In this case files that are not essential for
> building the target and are omitted from the target declaration,
> do not appear in the project either and hence are not searchable
> inside the IDE or indexing tool.
> 
> 
> Diffs
> -
> 
>   CMakeLists.txt e08fcef746460b601c1bddf9e0f00cad31500ec9 
>   src/CMakeLists.txt 0562b2bf87fb6e2b65a2a512e694e89ee431738b 
> 
> 
> Diff: https://reviews.apache.org/r/62071/diff/1/
> 
> 
> Testing
> ---
> 
> Run CMake and ensured configure + generate passes;
> Run Qt Creator with the built-in CMake plugin and verified the change adds 
> files to the project structure and makes them searchable in Qt Creator.
> 
> 
> Thanks,
> 
> Alexander Rukletsov
> 
>

Re: Review Request 62069: Moved SANDBOX_PATH volume tests to the right place.

[Mesos Reviewbot Windows]

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



FAIL: 'Mesos testes failed to run'

Reviews applied: [62068, 62069]

Logs available here: 
'http://mesos-logs.westus.cloudapp.azure.com/mesos-build/review/61211/logs'

- Mesos Reviewbot Windows


On Sept. 4, 2017, 5:50 p.m., Jie Yu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/62069/
> ---
> 
> (Updated Sept. 4, 2017, 5:50 p.m.)
> 
> 
> Review request for mesos, Gilbert Song and Joseph Wu.
> 
> 
> Bugs: MESOS-7936
> https://issues.apache.org/jira/browse/MESOS-7936
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This patch moved the SANDBOX_PATH volume tests from linux filesystem
> isolator tests to volume sandbox path isolator tests.
> 
> 
> Diffs
> -
> 
>   src/tests/containerizer/linux_filesystem_isolator_tests.cpp 
> 1ad9464e99b7dd4dfd2c9bd1a4c6c9c5cce615bd 
>   src/tests/containerizer/volume_sandbox_path_isolator_tests.cpp 
> 0da01c11ea538185dae1b714a80d81a14add0f0c 
> 
> 
> Diff: https://reviews.apache.org/r/62069/diff/1/
> 
> 
> Testing
> ---
> 
> sudo make check
> 
> 
> Thanks,
> 
> Jie Yu
> 
>

Re: Review Request 61536: Added network ports isolator socket utilities tests.

[James Peach]

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

(Updated Sept. 5, 2017, 4:36 p.m.)


Review request for mesos, Qian Zhang and Jiang Yan Xu.


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


Repository: mesos


Description
---

Added some basic tests for the network ports isolator socket
utilities. We test that we can enumerate our own sockets and
use that to figure out what ports we are listening on.


Diffs (updated)
-

  src/Makefile.am a4f2ee008eba263fe52c4e58c305e3fb9ba42f6b 
  src/tests/containerizer/ports_isolator_tests.cpp PRE-CREATION 


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

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


Testing
---

make check (Fedora 26)


Thanks,

James Peach

Re: Review Request 62037: Added logging::initialize to main functions that use glog.

[Alexander Rukletsov]

> On Sept. 4, 2017, 1:39 p.m., Alexander Rukletsov wrote:
> > src/examples/dynamic_reservation_framework.cpp
> > Lines 366-375 (original), 366-375 (patched)
> > 
> >
> > Let's use the common pattern (described in MESOS-7586) instead of this 
> > blob.
> 
> Armand Grillet wrote:
> Even with the new pattern, I believe that it is still useful to add this 
> part after logging any flag warnings:
> 
> ```
>   if (flags.master.isNone()) {
> EXIT(EXIT_FAILURE) << flags.usage("Missing --master");
>   } else if (flags.role.isNone()) {
> EXIT(EXIT_FAILURE) << flags.usage("Missing --role");
>   } else if (flags.role.get() == "*") {
> EXIT(EXIT_FAILURE)
>   << flags.usage("Role is incorrect; the default '*' role cannot be 
> used");
>   }
> ```
> Do you agree?

Of course! We shall *not* change the functionality, but the form. We usually 
have a section that validates that all necessary parameters, hence it's fine to 
leave it here.


- Alexander


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


On Sept. 1, 2017, 3:47 p.m., Armand Grillet wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/62037/
> ---
> 
> (Updated Sept. 1, 2017, 3:47 p.m.)
> 
> 
> Review request for mesos, Andrei Budnik and Alexander Rukletsov.
> 
> 
> Bugs: MESOS-7586
> https://issues.apache.org/jira/browse/MESOS-7586
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This change reduces the number of messages "WARNING: Logging
> before InitGoogleLogging() is written to STDERR".
> 
> 
> Diffs
> -
> 
>   src/checks/tcp_connect.cpp 3cb9077b9f39f69fdc152280e5b180b88e772648 
>   src/cli/execute.cpp e844fa46a43bdfb08246be929b7913a7997f18df 
>   src/cli/resolve.cpp b3cba87f61dbff0e99291775a4d0908bb35ce811 
>   src/examples/balloon_framework.cpp 1a97f56c3e217c58b408949306b92f19caa5f6c6 
>   src/examples/disk_full_framework.cpp 
> f9d5af5e45d5736581ce2c5395c741c995332136 
>   src/examples/dynamic_reservation_framework.cpp 
> bb6f58ba8fb87ae37d4ae971c192a204d5a656c5 
>   src/slave/container_loggers/logrotate.cpp 
> 61484b18f4615e85925b26d999afb5ac3b7e32a5 
>   src/usage/main.cpp 5ad4a3ba7959e4a9b3fa05f5a2f49b02f94a0cf0 
> 
> 
> Diff: https://reviews.apache.org/r/62037/diff/1/
> 
> 
> Testing
> ---
> 
> ```
> make check
> ```
> 
> Had an issue with ExamplesTest.DiskFullFramework that is not related.
> 
> 
> Thanks,
> 
> Armand Grillet
> 
>

Re: Review Request 62037: Added logging::initialize to main functions that use glog.

[Armand Grillet]

> On Sept. 4, 2017, 1:39 p.m., Alexander Rukletsov wrote:
> > src/examples/dynamic_reservation_framework.cpp
> > Lines 366-375 (original), 366-375 (patched)
> > 
> >
> > Let's use the common pattern (described in MESOS-7586) instead of this 
> > blob.

Even with the new pattern, I believe that it is still useful to add this part 
after logging any flag warnings:

```
  if (flags.master.isNone()) {
EXIT(EXIT_FAILURE) << flags.usage("Missing --master");
  } else if (flags.role.isNone()) {
EXIT(EXIT_FAILURE) << flags.usage("Missing --role");
  } else if (flags.role.get() == "*") {
EXIT(EXIT_FAILURE)
  << flags.usage("Role is incorrect; the default '*' role cannot be used");
  }
```
Do you agree?


- Armand


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


On Sept. 1, 2017, 3:47 p.m., Armand Grillet wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/62037/
> ---
> 
> (Updated Sept. 1, 2017, 3:47 p.m.)
> 
> 
> Review request for mesos, Andrei Budnik and Alexander Rukletsov.
> 
> 
> Bugs: MESOS-7586
> https://issues.apache.org/jira/browse/MESOS-7586
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This change reduces the number of messages "WARNING: Logging
> before InitGoogleLogging() is written to STDERR".
> 
> 
> Diffs
> -
> 
>   src/checks/tcp_connect.cpp 3cb9077b9f39f69fdc152280e5b180b88e772648 
>   src/cli/execute.cpp e844fa46a43bdfb08246be929b7913a7997f18df 
>   src/cli/resolve.cpp b3cba87f61dbff0e99291775a4d0908bb35ce811 
>   src/examples/balloon_framework.cpp 1a97f56c3e217c58b408949306b92f19caa5f6c6 
>   src/examples/disk_full_framework.cpp 
> f9d5af5e45d5736581ce2c5395c741c995332136 
>   src/examples/dynamic_reservation_framework.cpp 
> bb6f58ba8fb87ae37d4ae971c192a204d5a656c5 
>   src/slave/container_loggers/logrotate.cpp 
> 61484b18f4615e85925b26d999afb5ac3b7e32a5 
>   src/usage/main.cpp 5ad4a3ba7959e4a9b3fa05f5a2f49b02f94a0cf0 
> 
> 
> Diff: https://reviews.apache.org/r/62037/diff/1/
> 
> 
> Testing
> ---
> 
> ```
> make check
> ```
> 
> Had an issue with ExamplesTest.DiskFullFramework that is not related.
> 
> 
> Thanks,
> 
> Armand Grillet
> 
>

Re: Review Request 62053: Removed garbage collector.

[Mesos Reviewbot]

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



Patch looks great!

Reviews applied: [62053]

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

- Mesos Reviewbot


On Sept. 2, 2017, 3:07 p.m., Benjamin Hindman wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/62053/
> ---
> 
> (Updated Sept. 2, 2017, 3:07 p.m.)
> 
> 
> Review request for mesos, Benjamin Mahler and Jiang Yan Xu.
> 
> 
> Bugs: MESOS-7921
> https://issues.apache.org/jira/browse/MESOS-7921
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> The garbage collector had at least two bugs:
> 
> (1) If someone dispatched `manage()` twice in a row the process we're
> waiting for will get overwritten which can wreak havoc depending
> on when the calls to `link()` happen.
> 
> (2) The garbage collector was deleting after an exited event rather
> than actually doing a `wait()`.
> 
> The simpler implementation that this patch introduces is to just
> delete the process after doing `ProcessManager::cleanup()`.
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/include/Makefile.am 
> c5dc0bb0d2d77987531ead50277940700c62b84f 
>   3rdparty/libprocess/include/process/gc.hpp 
> 603bb8bb084a8d2774ab1077da671f659a22a376 
>   3rdparty/libprocess/include/process/process.hpp 
> 8cca782ae89727bc5570afc4ed96c556f14c8c68 
>   3rdparty/libprocess/src/process.cpp 
> afa53537a5c7d4d8b0f4e3b8e04d7d0f2c4c6631 
>   3rdparty/libprocess/src/tests/process_tests.cpp 
> 8d36600701a795a7fa8d73a844657ff98eee6aa7 
> 
> 
> Diff: https://reviews.apache.org/r/62053/diff/2/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Benjamin Hindman
> 
>

Re: Review Request 62037: Added logging::initialize to main functions that use glog.

[Mesos Reviewbot Windows]

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



FAIL: 'Mesos testes failed to run'

Reviews applied: [62018, 62037]

Logs available here: 
'http://mesos-logs.westus.cloudapp.azure.com/mesos-build/review/62037/logs'

- Mesos Reviewbot Windows


On Sept. 1, 2017, 3:47 p.m., Armand Grillet wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/62037/
> ---
> 
> (Updated Sept. 1, 2017, 3:47 p.m.)
> 
> 
> Review request for mesos, Andrei Budnik and Alexander Rukletsov.
> 
> 
> Bugs: MESOS-7586
> https://issues.apache.org/jira/browse/MESOS-7586
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This change reduces the number of messages "WARNING: Logging
> before InitGoogleLogging() is written to STDERR".
> 
> 
> Diffs
> -
> 
>   src/checks/tcp_connect.cpp 3cb9077b9f39f69fdc152280e5b180b88e772648 
>   src/cli/execute.cpp e844fa46a43bdfb08246be929b7913a7997f18df 
>   src/cli/resolve.cpp b3cba87f61dbff0e99291775a4d0908bb35ce811 
>   src/examples/balloon_framework.cpp 1a97f56c3e217c58b408949306b92f19caa5f6c6 
>   src/examples/disk_full_framework.cpp 
> f9d5af5e45d5736581ce2c5395c741c995332136 
>   src/examples/dynamic_reservation_framework.cpp 
> bb6f58ba8fb87ae37d4ae971c192a204d5a656c5 
>   src/slave/container_loggers/logrotate.cpp 
> 61484b18f4615e85925b26d999afb5ac3b7e32a5 
>   src/usage/main.cpp 5ad4a3ba7959e4a9b3fa05f5a2f49b02f94a0cf0 
> 
> 
> Diff: https://reviews.apache.org/r/62037/diff/1/
> 
> 
> Testing
> ---
> 
> ```
> make check
> ```
> 
> Had an issue with ExamplesTest.DiskFullFramework that is not related.
> 
> 
> Thanks,
> 
> Armand Grillet
> 
>

Re: Review Request 61211: Added default configuration file for CLI tests.

[Mesos Reviewbot Windows]

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



FAIL: 'Mesos testes failed to run'

Reviews applied: [60088, 61211]

Logs available here: 
'http://mesos-logs.westus.cloudapp.azure.com/mesos-build/review/61211/logs'

- Mesos Reviewbot Windows


On Sept. 3, 2017, 8:36 a.m., Armand Grillet wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/61211/
> ---
> 
> (Updated Sept. 3, 2017, 8:36 a.m.)
> 
> 
> Review request for mesos and Kevin Klues.
> 
> 
> Bugs: MESOS-7840
> https://issues.apache.org/jira/browse/MESOS-7840
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This will be used by future tests, it overwrites
> the configuration file defined by the user.
> 
> 
> Diffs
> -
> 
>   src/python/cli_new/tests/default_config.toml PRE-CREATION 
>   src/python/cli_new/tests/main.py 3e4d2e449a6485206700b4a490d325a393d31f90 
> 
> 
> Diff: https://reviews.apache.org/r/61211/diff/2/
> 
> 
> Testing
> ---
> 
> ```
> $ ./bootstrap
> $ source activate
> $ mesos-cli-tests
> ```
> 
> I also checked that the Python linter was still working.
> 
> 
> Thanks,
> 
> Armand Grillet
> 
>

Re: Review Request 62065: Added 'mesos agent list' command to CLI.

[Mesos Reviewbot]

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



Bad patch!

Reviews applied: [62065, 61212]

Failed command: python support/apply-reviews.py -n -r 62065

Error:
2017-09-05 09:29:58 URL:https://reviews.apache.org/r/62065/diff/raw/ 
[8658/8658] -> "62065.patch" [1]
error: patch failed: src/python/cli_new/bin/settings.py:38
error: src/python/cli_new/bin/settings.py: patch does not apply
error: patch failed: src/python/cli_new/lib/cli/tests/__init__.py:20
error: src/python/cli_new/lib/cli/tests/__init__.py: patch does not apply
error: patch failed: src/python/cli_new/tests/main.py:27
error: src/python/cli_new/tests/main.py: patch does not apply

Full log: https://builds.apache.org/job/Mesos-Reviewbot/19203/console

- Mesos Reviewbot


On Sept. 4, 2017, 4:51 p.m., Armand Grillet wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/62065/
> ---
> 
> (Updated Sept. 4, 2017, 4:51 p.m.)
> 
> 
> Review request for mesos and Kevin Klues.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This command displays the agents in a cluster by
> reaching the slaves endpoint of a master. It also
> works when Mesos runs in High-Availability Mode.
> 
> 
> Diffs
> -
> 
>   src/python/cli_new/bin/settings.py d42df04b0ff42bb6f466842e59223cd90a74d5c0 
>   src/python/cli_new/lib/cli/plugins/agent/__init__.py PRE-CREATION 
>   src/python/cli_new/lib/cli/plugins/agent/main.py PRE-CREATION 
>   src/python/cli_new/lib/cli/tests/__init__.py 
> 0daf28869e107263c51653ace39e3b1826871048 
>   src/python/cli_new/lib/cli/tests/agent.py PRE-CREATION 
>   src/python/cli_new/tests/main.py 3e4d2e449a6485206700b4a490d325a393d31f90 
> 
> 
> Diff: https://reviews.apache.org/r/62065/diff/1/
> 
> 
> Testing
> ---
> 
> To test with one master:
> 
> $ ./bootstrap
> $ source activate
> $ mesos-cli-tests
> 
> 
> To test the command in High-Availability Mode I have:
> 1. Started ZooKeeper.
> 2. Started two masters using the flags --zk and --quorum.
> 3. Started one agent.
> 4. Launched one task using ./mesos-execute 
> --master='zk://127.0.0.1:2181/mesos' --name='test' --command='sleep 3600'.
> 5. Checked that the output of 'mesos agent list' was correct.
> 
> I also checked that the Python linter was still working.
> 
> 
> Thanks,
> 
> Armand Grillet
> 
>

Re: Review Request 60766: Ignored containers that join CNI networks.

[Qian Zhang]

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




src/slave/containerizer/mesos/isolators/network/ports.cpp
Lines 405-408 (patched)


We should call `update()` in this case as well.



src/slave/containerizer/mesos/isolators/network/ports.cpp
Lines 442-444 (patched)


Better to merge these 3 lines into 2 lines.


- Qian Zhang


On Aug. 31, 2017, 7:26 a.m., James Peach wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/60766/
> ---
> 
> (Updated Aug. 31, 2017, 7:26 a.m.)
> 
> 
> Review request for mesos, Qian Zhang and Jiang Yan Xu.
> 
> 
> Bugs: MESOS-7675
> https://issues.apache.org/jira/browse/MESOS-7675
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Working on the assumption that containers with CNI networks will
> get their own IP addresses and don't need port isolation, ignore
> any containers that are joining CNI networks.
> 
> 
> Diffs
> -
> 
>   src/slave/containerizer/mesos/containerizer.cpp 
> 5f9373a88819ae1e0ceae708dccbed5383684918 
>   src/slave/containerizer/mesos/isolators/network/ports.hpp PRE-CREATION 
>   src/slave/containerizer/mesos/isolators/network/ports.cpp PRE-CREATION 
> 
> 
> Diff: https://reviews.apache.org/r/60766/diff/14/
> 
> 
> Testing
> ---
> 
> make check (Fedora 26).
> 
> 
> Thanks,
> 
> James Peach
> 
>

Re: Review Request 60591: Optionally isolate only the agent network ports.

[Qian Zhang]

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


Ship it!




Ship It!

- Qian Zhang


On Aug. 31, 2017, 7:20 a.m., James Peach wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/60591/
> ---
> 
> (Updated Aug. 31, 2017, 7:20 a.m.)
> 
> 
> Review request for mesos, Qian Zhang and Jiang Yan Xu.
> 
> 
> Bugs: MESOS-7675
> https://issues.apache.org/jira/browse/MESOS-7675
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Normally, the `network/ports` isolator will kill any task that
> listens on a port that it does not have resources for. However,
> executors that are based on the libprocess API will always
> listen on a port in the ephemeral range, and we want to make
> it possible to use libprocess-based executors.
> 
> Added the `--check_agent_port_range_only` option to only kill
> tasks when they listen on un-allocated ports within the port
> range published by the agent resources. This still prevents
> port collisions between tasks, but doesn't kill them just
> because the executor is listening on a port.
> 
> 
> Diffs
> -
> 
>   src/slave/containerizer/mesos/isolators/network/ports.cpp PRE-CREATION 
>   src/slave/flags.hpp 2970fea0cfac6af275a758d4bfedfe9a943c2b60 
>   src/slave/flags.cpp 3b02f3e909a554f15104739832ae3f252926b45f 
> 
> 
> Diff: https://reviews.apache.org/r/60591/diff/17/
> 
> 
> Testing
> ---
> 
> make check (Fedora 26)
> 
> 
> Thanks,
> 
> James Peach
> 
>

Re: Review Request 60592: Configure the `network/ports` isolator watch interval.

[Qian Zhang]

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


Ship it!




Ship It!

- Qian Zhang


On Aug. 21, 2017, 11:50 p.m., James Peach wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/60592/
> ---
> 
> (Updated Aug. 21, 2017, 11:50 p.m.)
> 
> 
> Review request for mesos, Qian Zhang and Jiang Yan Xu.
> 
> 
> Bugs: MESOS-7675
> https://issues.apache.org/jira/browse/MESOS-7675
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added the `--container_ports_watch_interval` option to tune the
> interval at which the `network/ports` isolator scans for rogue
> listening ports.
> 
> 
> Diffs
> -
> 
>   src/slave/containerizer/mesos/isolators/network/ports.hpp PRE-CREATION 
>   src/slave/containerizer/mesos/isolators/network/ports.cpp PRE-CREATION 
>   src/slave/flags.hpp 2970fea0cfac6af275a758d4bfedfe9a943c2b60 
>   src/slave/flags.cpp 3b02f3e909a554f15104739832ae3f252926b45f 
> 
> 
> Diff: https://reviews.apache.org/r/60592/diff/14/
> 
> 
> Testing
> ---
> 
> make check (Fedora 26)
> 
> 
> Thanks,
> 
> James Peach
> 
>

Re: Review Request 62067: CLI: Added 'mesos container list' command.

[Mesos Reviewbot Windows]

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



Bad review!

Error:
Circular dependency detected for review 62067.Please fix the 'depends_on' field.

- Mesos Reviewbot Windows


On Sept. 4, 2017, 7:54 a.m., Armand Grillet wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/62067/
> ---
> 
> (Updated Sept. 4, 2017, 7:54 a.m.)
> 
> 
> Review request for mesos and Kevin Klues.
> 
> 
> Bugs: MESOS-7285
> https://issues.apache.org/jira/browse/MESOS-7285
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This command displays the containers running
> on all the agents or a specific one.
> 
> 
> Diffs
> -
> 
>   src/python/cli_new/bin/settings.py d42df04b0ff42bb6f466842e59223cd90a74d5c0 
>   src/python/cli_new/lib/cli/plugins/container/__init__.py PRE-CREATION 
>   src/python/cli_new/lib/cli/plugins/container/main.py PRE-CREATION 
>   src/python/cli_new/lib/cli/tests/__init__.py 
> 0daf28869e107263c51653ace39e3b1826871048 
>   src/python/cli_new/lib/cli/tests/container.py PRE-CREATION 
>   src/python/cli_new/tests/main.py 3e4d2e449a6485206700b4a490d325a393d31f90 
> 
> 
> Diff: https://reviews.apache.org/r/62067/diff/1/
> 
> 
> Testing
> ---
> 
> To test with one master:
> 
> $ ./bootstrap
> $ source activate
> $ mesos-cli-tests
> 
> 
> To test the command in High-Availability Mode I have:
> 1. Started ZooKeeper.
> 2. Started two masters using the flags --zk and --quorum.
> 3. Started one agent.
> 4. Launched one task using ./mesos-execute 
> --master='zk://127.0.0.1:2181/mesos' --name='test' --command='sleep 3600'.
> 5. Checked that the output of 'mesos container list' was correct.
> 
> I also checked that the Python linter was still working.
> 
> 
> Thanks,
> 
> Armand Grillet
> 
>

Re: Review Request 60496: Added socket checking to the network ports isolator.

[Qian Zhang]

> On Aug. 24, 2017, 10:53 a.m., Qian Zhang wrote:
> > src/slave/containerizer/mesos/isolators/network/ports.cpp
> > Lines 200-202 (original), 373-375 (patched)
> > 
> >
> > When framework launches a task group, this `update()` method will be 
> > called twice for the top-level container (executor):
> > 1. When the top-level container is launched. At this time, the 
> > `resources` is the top-level container's own resources.
> > 2. When the executor subscribes the agent 
> > (https://github.com/apache/mesos/blob/1.3.1/src/slave/slave.cpp#L3719). At 
> > this time, the `resources` is the top-level container's own resources + all 
> > nested containers resources, so in this `update()` method, the 
> > `info->ports` for the top-level container will be updated to include the 
> > ports of all nested containers. This seems not correct, since executor 
> > process will be allowed to listen on ports not assigned to it.
> 
> James Peach wrote:
> Fixed in [r/60766](https://reviews.apache.org/r/60766) by calling 
> `update()` in the root-level container pass.

So we actually can not handle the issue that I mentioned in the above, because 
currently Mesos has not supported resource isolation for nested container yet. 
All the resources are attached to the root container rather than each 
individual nested container, that means for port isolation, we can only do it 
in pod (task group)'s resource level rather than nested container's resource 
level. In future when Mesos supports resource isolation for nested container, 
we can revisit this `network/ports` isolator to make it can check ports for 
each individual nested container against its *own* resources rather than the 
root container's resources which is the best that we can do currently.


- Qian


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


On Aug. 31, 2017, 7:19 a.m., James Peach wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/60496/
> ---
> 
> (Updated Aug. 31, 2017, 7:19 a.m.)
> 
> 
> Review request for mesos, Qian Zhang and Jiang Yan Xu.
> 
> 
> Bugs: MESOS-7675
> https://issues.apache.org/jira/browse/MESOS-7675
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Implemented ports resource restrictions in the network ports isolator.
> Periodically, scan for listening sockets and match them up to all
> the open sockets in the containers we are tracking in the network.
> Check any sockets we find against the ports resource and trigger a
> resource limitation if the port has not been allocated.
> 
> 
> Diffs
> -
> 
>   src/slave/containerizer/mesos/isolators/network/ports.hpp PRE-CREATION 
>   src/slave/containerizer/mesos/isolators/network/ports.cpp PRE-CREATION 
> 
> 
> Diff: https://reviews.apache.org/r/60496/diff/16/
> 
> 
> Testing
> ---
> 
> make check (Fedora 26)
> 
> 
> Thanks,
> 
> James Peach
> 
>

Re: Review Request 60496: Added socket checking to the network ports isolator.

[Qian Zhang]

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




src/slave/containerizer/mesos/isolators/network/ports.cpp
Lines 137 (patched)


Kill this empty line.



src/slave/containerizer/mesos/isolators/network/ports.cpp
Lines 271-284 (patched)


Mind to explain why running the loop inside the isolator process (since you 
supplied `pid` as the first parameter of `process::loop()`) and making 
`collectContainerListeners()` a static function?

Can we follow the similar way of what DRF allocator process does? I mean we 
can run the loop outside the isolator process and make 
`collectContainerListeners()` a member method of `NetworkPortsIsolatorProcess`, 
and then invoke `collectContainerListeners()` with a `dispatch()`. In this way, 
we do not need to pass those parameters (like `portsProcess->infos.keys()`) to 
`collectContainerListeners()` since as a member method it can access them 
naturally.



src/slave/containerizer/mesos/isolators/network/ports.cpp
Lines 367-368 (patched)


I think this check is a bit confusing, we should instead do:
```
CHECK(resources.empty());
```
The reason is, as you mentioned in the above comment, the resources are 
attached to the root container, so the resources here for nested container must 
be empty.



src/slave/containerizer/mesos/isolators/network/ports.cpp
Lines 368-369 (patched)


A newline between.


- Qian Zhang


On Aug. 31, 2017, 7:19 a.m., James Peach wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/60496/
> ---
> 
> (Updated Aug. 31, 2017, 7:19 a.m.)
> 
> 
> Review request for mesos, Qian Zhang and Jiang Yan Xu.
> 
> 
> Bugs: MESOS-7675
> https://issues.apache.org/jira/browse/MESOS-7675
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Implemented ports resource restrictions in the network ports isolator.
> Periodically, scan for listening sockets and match them up to all
> the open sockets in the containers we are tracking in the network.
> Check any sockets we find against the ports resource and trigger a
> resource limitation if the port has not been allocated.
> 
> 
> Diffs
> -
> 
>   src/slave/containerizer/mesos/isolators/network/ports.hpp PRE-CREATION 
>   src/slave/containerizer/mesos/isolators/network/ports.cpp PRE-CREATION 
> 
> 
> Diff: https://reviews.apache.org/r/60496/diff/16/
> 
> 
> Testing
> ---
> 
> make check (Fedora 26)
> 
> 
> Thanks,
> 
> James Peach
> 
>