> On Sept. 5, 2017, 3:12 p.m., Qian Zhang wrote: > > src/slave/containerizer/mesos/isolators/network/ports.cpp > > Lines 271-284 (patched) > > <https://reviews.apache.org/r/60496/diff/16/?file=1808222#file1808222line271> > > > > 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. > > James Peach wrote: > > 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<vector<ContainerID>>` 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.
Agree. And I think `initialize()` should be a better place to start this loop since it will be invoked when the process is spawned: (https://github.com/apache/mesos/blob/1.3.1/3rdparty/libprocess/include/process/process.hpp#L94:L97) So I think we should move this logic there like this: ``` void initialize() { // Start a loop to run periodically reconcile listening ports // against allocated resources. process::loop( self(), [=]() { return process::after(flags.container_ports_watch_interval); }, [=](const Nothing&) { return process::async( &collectContainerListeners, flags.cgroups_root, freezerHierarchy.get(), agentPorts, infos.keys()) .then(defer(self(), &NetworkPortsIsolatorProcess::check, lambda::_1)) .then([]() -> ControlFlow<Nothing> { return Continue(); }); }); } ``` - Qian ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/60496/#review184492 ----------------------------------------------------------- On Sept. 6, 2017, 1:39 a.m., James Peach wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/60496/ > ----------------------------------------------------------- > > (Updated Sept. 6, 2017, 1:39 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/17/ > > > Testing > ------- > > make check (Fedora 26) > > > Thanks, > > James Peach > >
