> On Aug. 8, 2017, 11:31 p.m., Qian Zhang wrote: > > src/slave/containerizer/mesos/isolators/network/ports.cpp > > Lines 201 (patched) > > <https://reviews.apache.org/r/60495/diff/7/?file=1785519#file1785519line201> > > > > We should call `os::realpath()` to expand the symbol link. > > James Peach wrote: > This (and the use of raw `opendir`) is done for performance reasons. > Since we could be doing this for a lot of sockets, I don't want to take the > hit of also doing extra system calls and memory allocation. > > Qian Zhang wrote: > Understand your concern. However > `NetworkPortsIsolatorProcess::processSockets()` is used to process sockets > for a single pid, so I do not expect there will be a huge number files under > `/proc/<pid>/fd/`. You can take a look at `os::pids()` (Linux version), it > will call `os::ls()` directly on `/proc` which should have much more entries > than `/proc/<pid>/fd/`. > > And we never call the raw `opendir()`/`readdir()`/`readlinkat()` in Mesos > code, instead we always call the wrapper in `os` namespace which will also > simplify your code in this method. > > James Peach wrote: > This is doing a `readlink` for every open file on the system. There could > easily be millions of these, so it is worth making an effort to apply the > obvious optimizations. I don't call `os::ls(/proc)` because I'm only > interested in processes that are specifically managed by Mesos. I don't want > to know about arbitrary system processes. I see your point about not using > the abstractions, but in this case the abstractions aren't really suitable. > > Qian Zhang wrote: > > This is doing a readlink for every open file on the system. There could > easily be millions of these > > I think it is doing a readlink for every open file on *a single process* > rather than the system, so for a single process, is it possible for it to > have millions of open files? I think usually the open file limit per process > should be thousands (like 4096). > > James Peach wrote: > Yes, it is possible (though not common) for a process to have millions of > open file descriptors. We are scanning every process in every cgroup that we > know about. That's pretty close to every open file on the system. There is > upstream kernel support for doing better so in the future we will be able > make this efficient.
OK, then I think it should be OK to call raw `opendir()/readdir()/readlinkat()` in this isolator. - Qian ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/60495/#review182261 ----------------------------------------------------------- On Aug. 15, 2017, 7:40 a.m., James Peach wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/60495/ > ----------------------------------------------------------- > > (Updated Aug. 15, 2017, 7:40 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 network ports isolator socket utilities to find the > inode numbers of all the listening sockets, and the inodes of the > open sockets for a process. Together, these utilities can tell us > which sockets a process is listening on. > > > 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/60495/diff/13/ > > > Testing > ------- > > make check (Fedora 26) > > > Thanks, > > James Peach > >
