> On May 21, 2018, 8:56 p.m., James Peach wrote: > > src/slave/containerizer/mesos/isolators/network/ports.cpp > > Lines 577 (patched) > > <https://reviews.apache.org/r/67195/diff/1/?file=2024899#file2024899line577> > > > > 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 > >
