Re: Review Request 67195: Adding enforce_container_ports flag for network ports isolation.

2018-05-24 Thread Xudong Ni via Review Board 


> On May 24, 2018, 8:30 p.m., James Peach wrote:
> > Commit subject should be "Added ..."

subject changed


- Xudong


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


On May 23, 2018, 6:33 p.m., Xudong Ni wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/67195/
> ---
> 
> (Updated May 23, 2018, 6:33 p.m.)
> 
> 
> Review request for mesos, James Peach and Jiang Yan Xu.
> 
> 
> Bugs: MESOS-8340
> https://issues.apache.org/jira/browse/MESOS-8340
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> To reduce deployment risk, a nonenforce mode is added for network
> port isolator. When this flag is set as false(default is false),
> even task uses ports not in the container resources, the container
> will not raise any limitation.
> 
> Add new test for this flag and update the existing tests
> 
> 
> Diffs
> -
> 
>   docs/isolators/network-ports.md ea63968481ce52c46e0a98e242da49baf6962009 
>   src/slave/containerizer/mesos/isolators/network/ports.hpp 
> ba71087194a3ae74c7e40dffa9c108b02ffa10ad 
>   src/slave/containerizer/mesos/isolators/network/ports.cpp 
> 1f84ed4fb2a30fd095e2faec1038de1fa19a15c5 
>   src/slave/flags.hpp a839591a2b66444ad97fced0620201dde656352d 
>   src/slave/flags.cpp a319b5ea633c41fd8a252c5e1617ac52d1480ba5 
>   src/tests/containerizer/ports_isolator_tests.cpp 
> c5b9f926047792e7f9d1f0937fa5355b1dd77965 
> 
> 
> Diff: https://reviews.apache.org/r/67195/diff/3/
> 
> 
> Testing
> ---
> 
> New test added for the flag; Related unit tests passed.
> 
> [ OK ] NetworkPortsIsolatorTest.ROOT_NC_AllocatedPorts (1906 ms)
> [ OK ] NetworkPortsIsolatorTest.ROOT_NC_NoPortsResource (1788 ms)
> [ OK ] NetworkPortsIsolatorTest.ROOT_NC_NoPortEnforement (17001 ms)
> 
> 
> Thanks,
> 
> Xudong Ni
> 
>

Re: Review Request 67195: Adding enforce_container_ports flag for network ports isolation.

2018-05-24 Thread Xudong Ni via Review Board 


> On May 24, 2018, 5:58 p.m., Gilbert Song wrote:
> > we need to update `configure.md` correspondingly.

Updated configuration/agent.md


- Xudong


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


On May 23, 2018, 6:33 p.m., Xudong Ni wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/67195/
> ---
> 
> (Updated May 23, 2018, 6:33 p.m.)
> 
> 
> Review request for mesos, James Peach and Jiang Yan Xu.
> 
> 
> Bugs: MESOS-8340
> https://issues.apache.org/jira/browse/MESOS-8340
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> To reduce deployment risk, a nonenforce mode is added for network
> port isolator. When this flag is set as false(default is false),
> even task uses ports not in the container resources, the container
> will not raise any limitation.
> 
> Add new test for this flag and update the existing tests
> 
> 
> Diffs
> -
> 
>   docs/isolators/network-ports.md ea63968481ce52c46e0a98e242da49baf6962009 
>   src/slave/containerizer/mesos/isolators/network/ports.hpp 
> ba71087194a3ae74c7e40dffa9c108b02ffa10ad 
>   src/slave/containerizer/mesos/isolators/network/ports.cpp 
> 1f84ed4fb2a30fd095e2faec1038de1fa19a15c5 
>   src/slave/flags.hpp a839591a2b66444ad97fced0620201dde656352d 
>   src/slave/flags.cpp a319b5ea633c41fd8a252c5e1617ac52d1480ba5 
>   src/tests/containerizer/ports_isolator_tests.cpp 
> c5b9f926047792e7f9d1f0937fa5355b1dd77965 
> 
> 
> Diff: https://reviews.apache.org/r/67195/diff/3/
> 
> 
> Testing
> ---
> 
> New test added for the flag; Related unit tests passed.
> 
> [ OK ] NetworkPortsIsolatorTest.ROOT_NC_AllocatedPorts (1906 ms)
> [ OK ] NetworkPortsIsolatorTest.ROOT_NC_NoPortsResource (1788 ms)
> [ OK ] NetworkPortsIsolatorTest.ROOT_NC_NoPortEnforement (17001 ms)
> 
> 
> Thanks,
> 
> Xudong Ni
> 
>

Re: Review Request 67195: Adding enforce_container_ports flag for network ports isolation.

2018-05-24 Thread James Peach 

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


Fix it, then Ship it!




Commit subject should be "Added ..."


docs/isolators/network-ports.md
Lines 49 (patched)


How about this:

```
The `--enforce_container_ports` flag, specifies
whether the network ports isolator should terminate
tasks that listen on ports they have not been assigned.
If enforcement is disabled, the isolator will log violations
but will not terminate tasks. By default, network port enforcement
is disabled.
```



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


"while port enforcement is disabled"



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


Newline after the '}'.



src/slave/flags.cpp
Lines 1123 (patched)


"is used by the `network/ports` isolator."

Note the backquotes and fill stop.



src/tests/containerizer/ports_isolator_tests.cpp
Lines 834 (patched)


Needs a full-stop "is false."



src/tests/containerizer/ports_isolator_tests.cpp
Lines 835 (patched)


`ROOT_NC_NoPortEnforcement`



src/tests/containerizer/ports_isolator_tests.cpp
Lines 929 (patched)


Add a comment here:
```
// Since container ports are not being enforced, we expect that the task
// should still be running after the check and that we should be able to
// explicitly kill it.
```


- James Peach


On May 23, 2018, 6:33 p.m., Xudong Ni wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/67195/
> ---
> 
> (Updated May 23, 2018, 6:33 p.m.)
> 
> 
> Review request for mesos, James Peach and Jiang Yan Xu.
> 
> 
> Bugs: MESOS-8340
> https://issues.apache.org/jira/browse/MESOS-8340
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> To reduce deployment risk, a nonenforce mode is added for network
> port isolator. When this flag is set as false(default is false),
> even task uses ports not in the container resources, the container
> will not raise any limitation.
> 
> Add new test for this flag and update the existing tests
> 
> 
> Diffs
> -
> 
>   docs/isolators/network-ports.md ea63968481ce52c46e0a98e242da49baf6962009 
>   src/slave/containerizer/mesos/isolators/network/ports.hpp 
> ba71087194a3ae74c7e40dffa9c108b02ffa10ad 
>   src/slave/containerizer/mesos/isolators/network/ports.cpp 
> 1f84ed4fb2a30fd095e2faec1038de1fa19a15c5 
>   src/slave/flags.hpp a839591a2b66444ad97fced0620201dde656352d 
>   src/slave/flags.cpp a319b5ea633c41fd8a252c5e1617ac52d1480ba5 
>   src/tests/containerizer/ports_isolator_tests.cpp 
> c5b9f926047792e7f9d1f0937fa5355b1dd77965 
> 
> 
> Diff: https://reviews.apache.org/r/67195/diff/3/
> 
> 
> Testing
> ---
> 
> New test added for the flag; Related unit tests passed.
> 
> [ OK ] NetworkPortsIsolatorTest.ROOT_NC_AllocatedPorts (1906 ms)
> [ OK ] NetworkPortsIsolatorTest.ROOT_NC_NoPortsResource (1788 ms)
> [ OK ] NetworkPortsIsolatorTest.ROOT_NC_NoPortEnforement (17001 ms)
> 
> 
> Thanks,
> 
> Xudong Ni
> 
>

Re: Review Request 67195: Adding enforce_container_ports flag for network ports isolation.

2018-05-24 Thread Gilbert Song 

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



we need to update `configure.md` correspondingly.

- Gilbert Song


On May 23, 2018, 11:33 a.m., Xudong Ni wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/67195/
> ---
> 
> (Updated May 23, 2018, 11:33 a.m.)
> 
> 
> Review request for mesos, James Peach and Jiang Yan Xu.
> 
> 
> Bugs: MESOS-8340
> https://issues.apache.org/jira/browse/MESOS-8340
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> To reduce deployment risk, a nonenforce mode is added for network
> port isolator. When this flag is set as false(default is false),
> even task uses ports not in the container resources, the container
> will not raise any limitation.
> 
> Add new test for this flag and update the existing tests
> 
> 
> Diffs
> -
> 
>   docs/isolators/network-ports.md ea63968481ce52c46e0a98e242da49baf6962009 
>   src/slave/containerizer/mesos/isolators/network/ports.hpp 
> ba71087194a3ae74c7e40dffa9c108b02ffa10ad 
>   src/slave/containerizer/mesos/isolators/network/ports.cpp 
> 1f84ed4fb2a30fd095e2faec1038de1fa19a15c5 
>   src/slave/flags.hpp a839591a2b66444ad97fced0620201dde656352d 
>   src/slave/flags.cpp a319b5ea633c41fd8a252c5e1617ac52d1480ba5 
>   src/tests/containerizer/ports_isolator_tests.cpp 
> c5b9f926047792e7f9d1f0937fa5355b1dd77965 
> 
> 
> Diff: https://reviews.apache.org/r/67195/diff/3/
> 
> 
> Testing
> ---
> 
> New test added for the flag; Related unit tests passed.
> 
> [ OK ] NetworkPortsIsolatorTest.ROOT_NC_AllocatedPorts (1906 ms)
> [ OK ] NetworkPortsIsolatorTest.ROOT_NC_NoPortsResource (1788 ms)
> [ OK ] NetworkPortsIsolatorTest.ROOT_NC_NoPortEnforement (17001 ms)
> 
> 
> Thanks,
> 
> Xudong Ni
> 
>

Re: Review Request 67195: Adding enforce_container_ports flag for network ports isolation.

2018-05-23 Thread Mesos Reviewbot 

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



Patch looks great!

Reviews applied: [67271, 67195]

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

- Mesos Reviewbot


On May 23, 2018, 6:33 p.m., Xudong Ni wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/67195/
> ---
> 
> (Updated May 23, 2018, 6:33 p.m.)
> 
> 
> Review request for mesos, James Peach and Jiang Yan Xu.
> 
> 
> Bugs: MESOS-8340
> https://issues.apache.org/jira/browse/MESOS-8340
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> To reduce deployment risk, a nonenforce mode is added for network
> port isolator. When this flag is set as false(default is false),
> even task uses ports not in the container resources, the container
> will not raise any limitation.
> 
> Add new test for this flag and update the existing tests
> 
> 
> Diffs
> -
> 
>   docs/isolators/network-ports.md ea63968481ce52c46e0a98e242da49baf6962009 
>   src/slave/containerizer/mesos/isolators/network/ports.hpp 
> ba71087194a3ae74c7e40dffa9c108b02ffa10ad 
>   src/slave/containerizer/mesos/isolators/network/ports.cpp 
> 1f84ed4fb2a30fd095e2faec1038de1fa19a15c5 
>   src/slave/flags.hpp a839591a2b66444ad97fced0620201dde656352d 
>   src/slave/flags.cpp a319b5ea633c41fd8a252c5e1617ac52d1480ba5 
>   src/tests/containerizer/ports_isolator_tests.cpp 
> c5b9f926047792e7f9d1f0937fa5355b1dd77965 
> 
> 
> Diff: https://reviews.apache.org/r/67195/diff/3/
> 
> 
> Testing
> ---
> 
> New test added for the flag; Related unit tests passed.
> 
> [ OK ] NetworkPortsIsolatorTest.ROOT_NC_AllocatedPorts (1906 ms)
> [ OK ] NetworkPortsIsolatorTest.ROOT_NC_NoPortsResource (1788 ms)
> [ OK ] NetworkPortsIsolatorTest.ROOT_NC_NoPortEnforement (17001 ms)
> 
> 
> Thanks,
> 
> Xudong Ni
> 
>

Re: Review Request 67195: Adding enforce_container_ports flag for network ports isolation.

2018-05-23 Thread Mesos Reviewbot Windows 

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



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

Reviews applied: `['67271', '67195']`

Failed command: `Start-MesosCITesting`

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

Relevant logs:

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

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

[--] 2 tests from ContainerizerType/DefaultContainerDNSFlagTest
[ RUN  ] ContainerizerType/DefaultContainerDNSFlagTest.ValidateFlag/0
[   OK ] ContainerizerType/DefaultContainerDNSFlagTest.ValidateFlag/0 (33 
ms)
[ RUN  ] ContainerizerType/DefaultContainerDNSFlagTest.ValidateFlag/1
[   OK ] ContainerizerType/DefaultContainerDNSFlagTest.ValidateFlag/1 (38 
ms)
[--] 2 tests from ContainerizerType/DefaultContainerDNSFlagTest (73 ms 
total)

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

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

[--] Global test environment tear-down
[==] 981 tests from 95 test cases ran. (439137 ms total)
[  PASSED  ] 980 tests.
[  FAILED  ] 1 test, listed below:
[  FAILED  ] 
DockerContainerizerHealthCheckTest.ROOT_DOCKER_DockerHealthStatusChange

 1 FAILED TEST
  YOU HAVE 220 DISABLED TESTS

```

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

```
I0523 19:42:12.129156 17984 master.cpp:10843] Updating the state of task 
766b978d-4132-4fff-9475-853ecc1020de of framework 
5ee9a2b0-5ffe-4b17-a4e2-2eb35c1e3212- (latest state: TASK_KILLED, status 
update state: TASK_KILLED)
I0523 19:42:12.129156  5424 slave.cpp:3935] Shutting down framework 
5ee9a2b0-5ffe-4b17-a4e2-2eb35c1e3212-
I0523 19:42:12.129156  5424 slave.cpp:6656] Shutting down executor 
'766b978d-4132-4fff-9475-853ecc1020de' of framework 
5ee9a2b0-5ffe-4b17-a4e2-2eb35c1e3212- aI0523 19:42:11.962154 16028 
exec.cpp:162] Version: 1.7.0
I0523 19:42:11.987149 16740 exec.cpp:236] Executor registered on agent 
5ee9a2b0-5ffe-4b17-a4e2-2eb35c1e3212-S0
I0523 19:42:11.991149  7324 executor.cpp:178] Received SUBSCRIBED event
I0523 19:42:11.995149  7324 executor.cpp:182] Subscribed executor on 
windows-02.enofukwu14ruplxn0gs3yzmsgf.xx.internal.cloudapp.net
I0523 19:42:11.996148  7324 executor.cpp:178] Received LAUNCH event
I0523 19:42:12.000149  7324 executor.cpp:665] Starting task 
766b978d-4132-4fff-9475-853ecc1020de
I0523 19:42:12.082157  7324 executor.cpp:485] Running 
'D:\DCOS\mesos\src\mesos-containerizer.exe launch '
I0523 19:42:12.101158  7324 executor.cpp:678] Forked command at 17100
I0523 19:42:12.131152  3204 exec.cpp:445] Executor asked to shutdown
I0523 19:42:12.132161  7232 executor.cpp:178] Received SHUTDOWN event
I0523 19:42:12.132161  7232 executor.cpp:781] Shutting down
I0523 19:42:12.132161  7232 executor.cpp:894] Sending SIGTERM to process tree 
at pid 17t executor(1)@192.10.1.6:63562
I0523 19:42:12.131152  5424 slave.cpp:929] Agent terminating
W0523 19:42:12.131152  5424 slave.cpp:3931] Ignoring shutdown framework 
5ee9a2b0-5ffe-4b17-a4e2-2eb35c1e3212- because it is terminating
I0523 19:42:12.131152 17984 master.cpp:10942] Removing task 
766b978d-4132-4fff-9475-853ecc1020de with resources cpus(allocated: *):4; 
mem(allocated: *):2048; disk(allocated: *):1024; ports(allocated: 
*):[31000-32000] of framework 5ee9a2b0-5ffe-4b17-a4e2-2eb35c1e3212- on 
agent 5ee9a2b0-5ffe-4b17-a4e2-2eb35c1e3212-S0 at slave(448)@192.10.1.6:63541 
(windows-02.enofukwu14ruplxn0gs3yzmsgf.xx.internal.cloudapp.net)
I0523 19:42:12.134155 17984 master.cpp:1293] Agent 
5ee9a2b0-5ffe-4b17-a4e2-2eb35c1e3212-S0 at slave(448)@192.10.1.6:63541 
(windows-02.enofukwu14ruplxn0gs3yzmsgf.xx.internal.cloudapp.net) disconnected
I0523 19:42:12.135154 17984 master.cpp:3303] Disconnecting agent 
5ee9a2b0-5ffe-4b17-a4e2-2eb35c1e3212-S0 at slave(448)@192.10.1.6:63541 
(windows-02.enofukwu14ruplxn0gs3yzmsgf.xx.internal.cloudapp.net)
I0523 19:42:12.135154 17288 hierarchical.cpp:344] Removed framework 
5ee9a2b0-5ffe-4b17-a4e2-2eb35c1e3212-
I0523 19:42:12.135154 17984 master.cpp:3322] Deactivating agent 
5ee9a2b0-5ffe-4b17-a4e2-2eb35c1e3212-S0 at

Re: Review Request 67195: Adding enforce_container_ports flag for network ports isolation.

2018-05-23 Thread Xudong Ni via Review Board 

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

(Updated May 23, 2018, 6:33 p.m.)


Review request for mesos, James Peach and Jiang Yan Xu.


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


Repository: mesos


Description
---

To reduce deployment risk, a nonenforce mode is added for network
port isolator. When this flag is set as false(default is false),
even task uses ports not in the container resources, the container
will not raise any limitation.

Add new test for this flag and update the existing tests


Diffs (updated)
-

  docs/isolators/network-ports.md ea63968481ce52c46e0a98e242da49baf6962009 
  src/slave/containerizer/mesos/isolators/network/ports.hpp 
ba71087194a3ae74c7e40dffa9c108b02ffa10ad 
  src/slave/containerizer/mesos/isolators/network/ports.cpp 
1f84ed4fb2a30fd095e2faec1038de1fa19a15c5 
  src/slave/flags.hpp a839591a2b66444ad97fced0620201dde656352d 
  src/slave/flags.cpp a319b5ea633c41fd8a252c5e1617ac52d1480ba5 
  src/tests/containerizer/ports_isolator_tests.cpp 
c5b9f926047792e7f9d1f0937fa5355b1dd77965 


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

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


Testing
---

New test added for the flag; Related unit tests passed.

[ OK ] NetworkPortsIsolatorTest.ROOT_NC_AllocatedPorts (1906 ms)
[ OK ] NetworkPortsIsolatorTest.ROOT_NC_NoPortsResource (1788 ms)
[ OK ] NetworkPortsIsolatorTest.ROOT_NC_NoPortEnforement (17001 ms)


Thanks,

Xudong Ni

Re: Review Request 67195: Adding enforce_container_ports flag for network ports isolation.

2018-05-23 Thread Xudong Ni via Review Board 

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

(Updated May 23, 2018, 5:57 p.m.)


Review request for mesos, James Peach and Jiang Yan Xu.


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


Repository: mesos


Description (updated)
---

To reduce deployment risk, a nonenforce mode is added for network
port isolator. When this flag is set as false(default is false),
even task uses ports not in the container resources, the container
will not raise any limitation.

Add new test for this flag and update the existing tests


Diffs (updated)
-

  docs/isolators/network-ports.md ea63968481ce52c46e0a98e242da49baf6962009 
  src/slave/containerizer/mesos/isolators/network/ports.hpp 
ba71087194a3ae74c7e40dffa9c108b02ffa10ad 
  src/slave/containerizer/mesos/isolators/network/ports.cpp 
1f84ed4fb2a30fd095e2faec1038de1fa19a15c5 
  src/slave/flags.hpp a839591a2b66444ad97fced0620201dde656352d 
  src/slave/flags.cpp a319b5ea633c41fd8a252c5e1617ac52d1480ba5 
  src/tests/containerizer/ports_isolator_tests.cpp 
c5b9f926047792e7f9d1f0937fa5355b1dd77965 


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

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


Testing
---

New test added for the flag; Related unit tests passed.

[ OK ] NetworkPortsIsolatorTest.ROOT_NC_AllocatedPorts (1906 ms)
[ OK ] NetworkPortsIsolatorTest.ROOT_NC_NoPortsResource (1788 ms)
[ OK ] NetworkPortsIsolatorTest.ROOT_NC_NoPortEnforement (17001 ms)


Thanks,

Xudong Ni

Re: Review Request 67195: Adding enforce_container_ports flag for network ports isolation.

2018-05-21 Thread Xudong Ni via Review Board 


> On May 21, 2018, 8:56 p.m., James Peach wrote:
> > src/slave/containerizer/mesos/isolators/network/ports.cpp
> > Lines 577 (patched)
> > 
> >
> > This is more complicated than it needs to be. You can simply do this:
> > 
> > ```
> > if (!enforceContainerPorts) {
> >   if (info->activePorts.isSome() &&
> >   info->activePorts == listeners) {
> > VLOG(2) << "Skipping container ports violation log";
> > continue;
> >   }
> > 
> >   // Cache the last listeners sample so that we will
> >   // only log new ports resource violations.
> >   info->activePorts = listeners; 
> > }
> > 
> > 
> > ```

Not sure I fully understand the idea, correct me if I am wrong; In the hashmap, 
the listener ports is a set of ports which may have both allocated and 
unallocated ports(the same applied to loggedPorts and unloggedPorts, we can not 
either use "==" or contains() set function to compare the set. We need set 
operation substraction to get either unallocated or unlogged set.


- Xudong


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


On May 21, 2018, 4:14 p.m., Xudong Ni wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/67195/
> ---
> 
> (Updated May 21, 2018, 4:14 p.m.)
> 
> 
> Review request for mesos, James Peach and Jiang Yan Xu.
> 
> 
> Bugs: MESOS-8340
> https://issues.apache.org/jira/browse/MESOS-8340
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> To reduce deployment risk, a nonenforce mode is added for network
> port isolator. When this flag is set as false(default is false),
> even task uses ports not in the container resources, the container
> will not raise any limitation.
> 
> Add new test for this flag and update the existing tests
> 
> https://reviews.apache.org/r/67195/
> 
> 
> Diffs
> -
> 
>   docs/isolators/network-ports.md ea63968481ce52c46e0a98e242da49baf6962009 
>   src/slave/containerizer/mesos/isolators/network/ports.hpp 
> ba71087194a3ae74c7e40dffa9c108b02ffa10ad 
>   src/slave/containerizer/mesos/isolators/network/ports.cpp 
> 1f84ed4fb2a30fd095e2faec1038de1fa19a15c5 
>   src/slave/flags.hpp a839591a2b66444ad97fced0620201dde656352d 
>   src/slave/flags.cpp a319b5ea633c41fd8a252c5e1617ac52d1480ba5 
>   src/tests/containerizer/ports_isolator_tests.cpp 
> c5b9f926047792e7f9d1f0937fa5355b1dd77965 
> 
> 
> Diff: https://reviews.apache.org/r/67195/diff/1/
> 
> 
> Testing
> ---
> 
> New test added for the flag; Related unit tests passed.
> 
> [ OK ] NetworkPortsIsolatorTest.ROOT_NC_AllocatedPorts (1906 ms)
> [ OK ] NetworkPortsIsolatorTest.ROOT_NC_NoPortsResource (1788 ms)
> [ OK ] NetworkPortsIsolatorTest.ROOT_NC_NoPortEnforement (17001 ms)
> 
> 
> Thanks,
> 
> Xudong Ni
> 
>

Re: Review Request 67195: Adding enforce_container_ports flag for network ports isolation.

2018-05-21 Thread James Peach 

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



Also, please remove the review URL from the commit message. The 
`./support/post-reviews.py` script will manage this automatically for you.

- James Peach


On May 21, 2018, 4:14 p.m., Xudong Ni wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/67195/
> ---
> 
> (Updated May 21, 2018, 4:14 p.m.)
> 
> 
> Review request for mesos, James Peach and Jiang Yan Xu.
> 
> 
> Bugs: MESOS-8340
> https://issues.apache.org/jira/browse/MESOS-8340
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> To reduce deployment risk, a nonenforce mode is added for network
> port isolator. When this flag is set as false(default is false),
> even task uses ports not in the container resources, the container
> will not raise any limitation.
> 
> Add new test for this flag and update the existing tests
> 
> https://reviews.apache.org/r/67195/
> 
> 
> Diffs
> -
> 
>   docs/isolators/network-ports.md ea63968481ce52c46e0a98e242da49baf6962009 
>   src/slave/containerizer/mesos/isolators/network/ports.hpp 
> ba71087194a3ae74c7e40dffa9c108b02ffa10ad 
>   src/slave/containerizer/mesos/isolators/network/ports.cpp 
> 1f84ed4fb2a30fd095e2faec1038de1fa19a15c5 
>   src/slave/flags.hpp a839591a2b66444ad97fced0620201dde656352d 
>   src/slave/flags.cpp a319b5ea633c41fd8a252c5e1617ac52d1480ba5 
>   src/tests/containerizer/ports_isolator_tests.cpp 
> c5b9f926047792e7f9d1f0937fa5355b1dd77965 
> 
> 
> Diff: https://reviews.apache.org/r/67195/diff/1/
> 
> 
> Testing
> ---
> 
> New test added for the flag; Related unit tests passed.
> 
> [ OK ] NetworkPortsIsolatorTest.ROOT_NC_AllocatedPorts (1906 ms)
> [ OK ] NetworkPortsIsolatorTest.ROOT_NC_NoPortsResource (1788 ms)
> [ OK ] NetworkPortsIsolatorTest.ROOT_NC_NoPortEnforement (17001 ms)
> 
> 
> Thanks,
> 
> Xudong Ni
> 
>

Re: Review Request 67195: Adding enforce_container_ports flag for network ports isolation.

2018-05-21 Thread James Peach 

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




src/slave/containerizer/mesos/isolators/network/ports.hpp
Lines 96 (patched)


Call this
```
Option allocatedPorts;
Option activePorts;
```

Do the `/ports/allocatedPorts` in a separate patch to make is easy to 
distinguish from functional changes.



src/slave/containerizer/mesos/isolators/network/ports.hpp
Lines 102 (patched)


Call this `enforceContainerPorts`.



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


This is more complicated than it needs to be. You can simply do this:

```
if (!enforceContainerPorts) {
  if (info->activePorts.isSome() &&
  info->activePorts == listeners) {
VLOG(2) << "Skipping container ports violation log";
continue;
  }

  // Cache the last listeners sample so that we will
  // only log new ports resource violations.
  info->activePorts = listeners; 
}

```



src/tests/containerizer/ports_isolator_tests.cpp
Lines 941 (patched)


Double blank line before and after this function.


- James Peach


On May 21, 2018, 4:14 p.m., Xudong Ni wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/67195/
> ---
> 
> (Updated May 21, 2018, 4:14 p.m.)
> 
> 
> Review request for mesos, James Peach and Jiang Yan Xu.
> 
> 
> Bugs: MESOS-8340
> https://issues.apache.org/jira/browse/MESOS-8340
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> To reduce deployment risk, a nonenforce mode is added for network
> port isolator. When this flag is set as false(default is false),
> even task uses ports not in the container resources, the container
> will not raise any limitation.
> 
> Add new test for this flag and update the existing tests
> 
> https://reviews.apache.org/r/67195/
> 
> 
> Diffs
> -
> 
>   docs/isolators/network-ports.md ea63968481ce52c46e0a98e242da49baf6962009 
>   src/slave/containerizer/mesos/isolators/network/ports.hpp 
> ba71087194a3ae74c7e40dffa9c108b02ffa10ad 
>   src/slave/containerizer/mesos/isolators/network/ports.cpp 
> 1f84ed4fb2a30fd095e2faec1038de1fa19a15c5 
>   src/slave/flags.hpp a839591a2b66444ad97fced0620201dde656352d 
>   src/slave/flags.cpp a319b5ea633c41fd8a252c5e1617ac52d1480ba5 
>   src/tests/containerizer/ports_isolator_tests.cpp 
> c5b9f926047792e7f9d1f0937fa5355b1dd77965 
> 
> 
> Diff: https://reviews.apache.org/r/67195/diff/1/
> 
> 
> Testing
> ---
> 
> New test added for the flag; Related unit tests passed.
> 
> [ OK ] NetworkPortsIsolatorTest.ROOT_NC_AllocatedPorts (1906 ms)
> [ OK ] NetworkPortsIsolatorTest.ROOT_NC_NoPortsResource (1788 ms)
> [ OK ] NetworkPortsIsolatorTest.ROOT_NC_NoPortEnforement (17001 ms)
> 
> 
> Thanks,
> 
> Xudong Ni
> 
>

Re: Review Request 67195: Adding enforce_container_ports flag for network ports isolation.

2018-05-21 Thread Xudong Ni via Review Board 

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

(Updated May 21, 2018, 4:14 p.m.)


Review request for mesos, James Peach and Jiang Yan Xu.


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


Repository: mesos


Description
---

To reduce deployment risk, a nonenforce mode is added for network
port isolator. When this flag is set as false(default is false),
even task uses ports not in the container resources, the container
will not raise any limitation.

Add new test for this flag and update the existing tests

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


Diffs
-

  docs/isolators/network-ports.md ea63968481ce52c46e0a98e242da49baf6962009 
  src/slave/containerizer/mesos/isolators/network/ports.hpp 
ba71087194a3ae74c7e40dffa9c108b02ffa10ad 
  src/slave/containerizer/mesos/isolators/network/ports.cpp 
1f84ed4fb2a30fd095e2faec1038de1fa19a15c5 
  src/slave/flags.hpp a839591a2b66444ad97fced0620201dde656352d 
  src/slave/flags.cpp a319b5ea633c41fd8a252c5e1617ac52d1480ba5 
  src/tests/containerizer/ports_isolator_tests.cpp 
c5b9f926047792e7f9d1f0937fa5355b1dd77965 


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


Testing
---

New test added for the flag; Related unit tests passed.

[ OK ] NetworkPortsIsolatorTest.ROOT_NC_AllocatedPorts (1906 ms)
[ OK ] NetworkPortsIsolatorTest.ROOT_NC_NoPortsResource (1788 ms)
[ OK ] NetworkPortsIsolatorTest.ROOT_NC_NoPortEnforement (17001 ms)


Thanks,

Xudong Ni

Re: Review Request 67195: Adding enforce_container_ports flag for network ports isolation.

2018-05-18 Thread Mesos Reviewbot 

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



Patch looks great!

Reviews applied: [67195]

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

- Mesos Reviewbot


On May 17, 2018, 9:55 p.m., Xudong Ni wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/67195/
> ---
> 
> (Updated May 17, 2018, 9:55 p.m.)
> 
> 
> Review request for mesos, James Peach and Jiang Yan Xu.
> 
> 
> Bugs: mesos-8340
> https://issues.apache.org/jira/browse/mesos-8340
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> To reduce deployment risk, a nonenforce mode is added for network
> port isolator. When this flag is set as false(default is false),
> even task uses ports not in the container resources, the container
> will not raise any limitation.
> 
> Add new test for this flag and update the existing tests
> 
> https://reviews.apache.org/r/67195/
> 
> 
> Diffs
> -
> 
>   docs/isolators/network-ports.md ea63968481ce52c46e0a98e242da49baf6962009 
>   src/slave/containerizer/mesos/isolators/network/ports.hpp 
> ba71087194a3ae74c7e40dffa9c108b02ffa10ad 
>   src/slave/containerizer/mesos/isolators/network/ports.cpp 
> 1f84ed4fb2a30fd095e2faec1038de1fa19a15c5 
>   src/slave/flags.hpp a839591a2b66444ad97fced0620201dde656352d 
>   src/slave/flags.cpp a319b5ea633c41fd8a252c5e1617ac52d1480ba5 
>   src/tests/containerizer/ports_isolator_tests.cpp 
> c5b9f926047792e7f9d1f0937fa5355b1dd77965 
> 
> 
> Diff: https://reviews.apache.org/r/67195/diff/1/
> 
> 
> Testing
> ---
> 
> New test added for the flag; Related unit tests passed.
> 
> [ OK ] NetworkPortsIsolatorTest.ROOT_NC_AllocatedPorts (1906 ms)
> [ OK ] NetworkPortsIsolatorTest.ROOT_NC_NoPortsResource (1788 ms)
> [ OK ] NetworkPortsIsolatorTest.ROOT_NC_NoPortEnforement (17001 ms)
> 
> 
> Thanks,
> 
> Xudong Ni
> 
>

Re: Review Request 67195: Adding enforce_container_ports flag for network ports isolation.

2018-05-17 Thread Mesos Reviewbot Windows 

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



PASS: Mesos patch 67195 was successfully built and tested.

Reviews applied: `['67195']`

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

- Mesos Reviewbot Windows


On May 17, 2018, 9:55 p.m., Xudong Ni wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/67195/
> ---
> 
> (Updated May 17, 2018, 9:55 p.m.)
> 
> 
> Review request for mesos, James Peach and Jiang Yan Xu.
> 
> 
> Bugs: mesos-8340
> https://issues.apache.org/jira/browse/mesos-8340
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> To reduce deployment risk, a nonenforce mode is added for network
> port isolator. When this flag is set as false(default is false),
> even task uses ports not in the container resources, the container
> will not raise any limitation.
> 
> Add new test for this flag and update the existing tests
> 
> https://reviews.apache.org/r/67195/
> 
> 
> Diffs
> -
> 
>   docs/isolators/network-ports.md ea63968481ce52c46e0a98e242da49baf6962009 
>   src/slave/containerizer/mesos/isolators/network/ports.hpp 
> ba71087194a3ae74c7e40dffa9c108b02ffa10ad 
>   src/slave/containerizer/mesos/isolators/network/ports.cpp 
> 1f84ed4fb2a30fd095e2faec1038de1fa19a15c5 
>   src/slave/flags.hpp a839591a2b66444ad97fced0620201dde656352d 
>   src/slave/flags.cpp a319b5ea633c41fd8a252c5e1617ac52d1480ba5 
>   src/tests/containerizer/ports_isolator_tests.cpp 
> c5b9f926047792e7f9d1f0937fa5355b1dd77965 
> 
> 
> Diff: https://reviews.apache.org/r/67195/diff/1/
> 
> 
> Testing
> ---
> 
> New test added for the flag; Related unit tests passed.
> 
> [ OK ] NetworkPortsIsolatorTest.ROOT_NC_AllocatedPorts (1906 ms)
> [ OK ] NetworkPortsIsolatorTest.ROOT_NC_NoPortsResource (1788 ms)
> [ OK ] NetworkPortsIsolatorTest.ROOT_NC_NoPortEnforement (17001 ms)
> 
> 
> Thanks,
> 
> Xudong Ni
> 
>