----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/60496/#review182691 -----------------------------------------------------------
src/slave/containerizer/mesos/isolators/network/ports.hpp Lines 39 (patched) <https://reviews.apache.org/r/60496/#comment258891> Should be a `static` variable. Or do we want to make it configurable by introducing an agent flag (like the existing one `--container_disk_watch_interval` for `disk/du` isolator)? src/slave/containerizer/mesos/isolators/network/ports.cpp Line 41 (original), 57-58 (patched) <https://reviews.apache.org/r/60496/#comment258889> Switch these two lines. src/slave/containerizer/mesos/isolators/network/ports.cpp Lines 88-90 (patched) <https://reviews.apache.org/r/60496/#comment258890> These 3 lines should be merged to a single line. src/slave/containerizer/mesos/isolators/network/ports.cpp Lines 128-133 (patched) <https://reviews.apache.org/r/60496/#comment259133> I think it is possible for `cgroups::processes()` to return some pids but the corresponding proccesses do not exsit, and it is normal rather than an error case, right? If so, that will cause `NetworkPortsIsolatorProcess::getProcessSockets()` fails since the process does not exist, then I think `LOG(ERROR)` may not be needed since it is a normal case. src/slave/containerizer/mesos/isolators/network/ports.cpp Lines 148-150 (patched) <https://reviews.apache.org/r/60496/#comment259136> It seems we only care about `port`, so it might not be needed to construct this oject. What about just using `ntohs(socketInfo.sourcePort.get())` in the code below? src/slave/containerizer/mesos/isolators/network/ports.cpp Lines 156-157 (patched) <https://reviews.apache.org/r/60496/#comment258901> Do we really need this? I think showing pid like what you did in the `else` block below should be enough. src/slave/containerizer/mesos/isolators/network/ports.cpp Lines 211 (patched) <https://reviews.apache.org/r/60496/#comment259145> s/allocator/isolator ? src/slave/containerizer/mesos/isolators/network/ports.cpp Lines 333 (patched) <https://reviews.apache.org/r/60496/#comment259143> It seems this method is only called in `NetworkPortsIsolatorProcess::_check()` and it is pretty straightfoward, so I think we may not need this method, just puting its logic into `NetworkPortsIsolatorProcess::_check()` should be OK. src/slave/containerizer/mesos/isolators/network/ports.cpp Lines 346 (patched) <https://reviews.apache.org/r/60496/#comment258905> s/isare/is/ src/slave/containerizer/mesos/isolators/network/ports.cpp Lines 361-362 (patched) <https://reviews.apache.org/r/60496/#comment259142> Here `stringify(ports)` includes both allocated and unallocated ports, right? I think we want to only log unallocated ports. - Qian Zhang On July 17, 2017, 8:04 a.m., James Peach wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/60496/ > ----------------------------------------------------------- > > (Updated July 17, 2017, 8:04 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/10/ > > > Testing > ------- > > make check (Fedora 26) > > > Thanks, > > James Peach > >
