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

[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)
<https://reviews.apache.org/r/67195/#comment285796>

    Call this
    ```
    Option<IntervalSet<uint16_t>> allocatedPorts;
    Option<IntervalSet<uint16_t>> 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)
<https://reviews.apache.org/r/67195/#comment285795>

    Call this `enforceContainerPorts`.



src/slave/containerizer/mesos/isolators/network/ports.cpp
Lines 577 (patched)
<https://reviews.apache.org/r/67195/#comment285794>

    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)
<https://reviews.apache.org/r/67195/#comment285811>

    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
> 
>