Re: Review Request 60495: Added network ports isolator listen socket utilities.

[Qian Zhang]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/60495/#review185769
---


Ship it!




Ship It!

- Qian Zhang


On Sept. 20, 2017, 12:29 a.m., James Peach wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/60495/
> ---
> 
> (Updated Sept. 20, 2017, 12:29 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/18/
> 
> 
> Testing
> ---
> 
> make check (Fedora 26)
> 
> 
> Thanks,
> 
> James Peach
> 
>

Re: Review Request 60495: Added network ports isolator listen socket utilities.

[James Peach]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/60495/
---

(Updated Sept. 19, 2017, 4:29 p.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 (updated)
-

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

Changes: https://reviews.apache.org/r/60495/diff/16-17/


Testing
---

make check (Fedora 26)


Thanks,

James Peach

Re: Review Request 60495: Added network ports isolator listen socket utilities.

[James Peach]

> On Sept. 19, 2017, 2:18 a.m., Qian Zhang wrote:
> > src/slave/containerizer/mesos/isolators/network/ports.cpp
> > Line 112 (original), 112-117 (patched)
> > 
> >
> > Can we change this to `while ((entry = readdir(dir)) != nullptr) {` and 
> > do `errno = 0` in the end of the while loop? There is already a `errno = 0` 
> > (line 106) right before the while loop, so I think we may not need to do it 
> > again at the beginning of the loop, instead it would be better to do it in 
> > the end of the loop.

I removed the extraneous `errno` write, and pulled the code that checks for 
`readdir` failure up into the loop so the error checking happens near the call 
site.


- James


---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/60495/#review185635
---


On Sept. 19, 2017, 12:19 a.m., James Peach wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/60495/
> ---
> 
> (Updated Sept. 19, 2017, 12:19 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/16/
> 
> 
> Testing
> ---
> 
> make check (Fedora 26)
> 
> 
> Thanks,
> 
> James Peach
> 
>

Re: Review Request 60495: Added network ports isolator listen socket utilities.

[Qian Zhang]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/60495/#review185635
---




src/slave/containerizer/mesos/isolators/network/ports.cpp
Line 112 (original), 112-117 (patched)


Can we change this to `while ((entry = readdir(dir)) != nullptr) {` and do 
`errno = 0` in the end of the while loop? There is already a `errno = 0` (line 
106) right before the while loop, so I think we may not need to do it again at 
the beginning of the loop, instead it would be better to do it in the end of 
the loop.


- Qian Zhang


On Sept. 19, 2017, 8:19 a.m., James Peach wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/60495/
> ---
> 
> (Updated Sept. 19, 2017, 8:19 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/16/
> 
> 
> Testing
> ---
> 
> make check (Fedora 26)
> 
> 
> Thanks,
> 
> James Peach
> 
>

Re: Review Request 60495: Added network ports isolator listen socket utilities.

[James Peach]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/60495/
---

(Updated Sept. 19, 2017, 12:19 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 (updated)
-

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

Changes: https://reviews.apache.org/r/60495/diff/15-16/


Testing
---

make check (Fedora 26)


Thanks,

James Peach

Re: Review Request 60495: Added network ports isolator listen socket utilities.

[Qian Zhang]

> On Sept. 8, 2017, 3:43 p.m., Qian Zhang wrote:
> > src/slave/containerizer/mesos/isolators/network/ports.cpp
> > Line 112 (original), 112 (patched)
> > 
> >
> > Why do we need a `for` loop like this? I think the previous `while` 
> > loop is good enough.
> 
> James Peach wrote:
> If we want to depend on `errno`, we need to guarantee that it is `0` 
> before we call the `readddir`. Since there is non-trivial logic inside the 
> loop, it is difficult to be confident that nothing we call is going to change 
> it, particularly if this code is ever updated.

OK, then what about a `while` loop and setting `errno` to 0 in the end of each 
iteration? Setting `errno` to 0 in the `for` loop seems a bit strange to me.


- Qian


---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/60495/#review184953
---


On Sept. 8, 2017, 5:50 a.m., James Peach wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/60495/
> ---
> 
> (Updated Sept. 8, 2017, 5:50 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/15/
> 
> 
> Testing
> ---
> 
> make check (Fedora 26)
> 
> 
> Thanks,
> 
> James Peach
> 
>

Re: Review Request 60495: Added network ports isolator listen socket utilities.

[James Peach]

> On Sept. 8, 2017, 7:43 a.m., Qian Zhang wrote:
> > src/slave/containerizer/mesos/isolators/network/ports.cpp
> > Line 112 (original), 112 (patched)
> > 
> >
> > Why do we need a `for` loop like this? I think the previous `while` 
> > loop is good enough.

If we want to depend on `errno`, we need to guarantee that it is `0` before we 
call the `readddir`. Since there is non-trivial logic inside the loop, it is 
difficult to be confident that nothing we call is going to change it, 
particularly if this code is ever updated.


> On Sept. 8, 2017, 7:43 a.m., Qian Zhang wrote:
> > src/slave/containerizer/mesos/isolators/network/ports.cpp
> > Line 124 (original), 125 (patched)
> > 
> >
> > This seems too strict, since here we are already going to return an 
> > error out, I think we can simply ignore the error of `closedir()`.

The only way `closedir` can fail is if we give it a bad pointer, so if we check 
this I think `CHECK` is appropriate. I don't understand the rationale of 
wanting to check the `readdir` but not the `closedir` since the error 
conditions for these are identical.


> On Sept. 8, 2017, 7:43 a.m., Qian Zhang wrote:
> > src/slave/containerizer/mesos/isolators/network/ports.cpp
> > Lines 144 (patched)
> > 
> >
> > Better to change this to:
> > ```
> >   if (closedir(dir) == -1) {
> > return ErrnoError("Failed to close directory '" + fdPath + "'");
> >   }
> > ```
> > This is also aligned with how you handle the error of `opendir()` in 
> > the above.

I don't understand why we want to check the error return for this one but not 
the others. Since the `closedir` can't fail, IMHO we should just either assert 
or ignore it consistently.


- James


---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/60495/#review184953
---


On Sept. 7, 2017, 9:50 p.m., James Peach wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/60495/
> ---
> 
> (Updated Sept. 7, 2017, 9:50 p.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/15/
> 
> 
> Testing
> ---
> 
> make check (Fedora 26)
> 
> 
> Thanks,
> 
> James Peach
> 
>

Re: Review Request 60495: Added network ports isolator listen socket utilities.

[Qian Zhang]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/60495/#review184953
---




src/slave/containerizer/mesos/isolators/network/ports.cpp
Line 112 (original), 112 (patched)


Why do we need a `for` loop like this? I think the previous `while` loop is 
good enough.



src/slave/containerizer/mesos/isolators/network/ports.cpp
Line 124 (original), 125 (patched)


This seems too strict, since here we are already going to return an error 
out, I think we can simply ignore the error of `closedir()`.



src/slave/containerizer/mesos/isolators/network/ports.cpp
Lines 140 (patched)


Ditto.



src/slave/containerizer/mesos/isolators/network/ports.cpp
Lines 144 (patched)


Better to change this to:
```
  if (closedir(dir) == -1) {
return ErrnoError("Failed to close directory '" + fdPath + "'");
  }
```
This is also aligned with how you handle the error of `opendir()` in the 
above.


- Qian Zhang


On Sept. 8, 2017, 5:50 a.m., James Peach wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/60495/
> ---
> 
> (Updated Sept. 8, 2017, 5:50 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/15/
> 
> 
> Testing
> ---
> 
> make check (Fedora 26)
> 
> 
> Thanks,
> 
> James Peach
> 
>

Re: Review Request 60495: Added network ports isolator listen socket utilities.

[James Peach]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/60495/
---

(Updated Sept. 7, 2017, 9:50 p.m.)


Review request for mesos, Qian Zhang and Jiang Yan Xu.


Changes
---

Addressed review feedback.


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 (updated)
-

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

Changes: https://reviews.apache.org/r/60495/diff/13-14/


Testing
---

make check (Fedora 26)


Thanks,

James Peach

Re: Review Request 60495: Added network ports isolator listen socket utilities.

[Qian Zhang]

> On Aug. 8, 2017, 11:31 p.m., Qian Zhang wrote:
> > src/slave/containerizer/mesos/isolators/network/ports.cpp
> > Lines 201 (patched)
> > 
> >
> > 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//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//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
> 
>

Re: Review Request 60495: Added network ports isolator listen socket utilities.

[Qian Zhang]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/60495/#review184508
---




src/slave/containerizer/mesos/isolators/network/ports.cpp
Lines 96 (patched)


s/gived/given/


- Qian Zhang


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

Re: Review Request 60495: Added network ports isolator listen socket utilities.

[James Peach]

> On Aug. 8, 2017, 3:31 p.m., Qian Zhang wrote:
> > src/slave/containerizer/mesos/isolators/network/ports.cpp
> > Lines 201 (patched)
> > 
> >
> > 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//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//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).

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.


- James


---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/60495/#review182261
---


On Aug. 14, 2017, 11:40 p.m., James Peach wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/60495/
> ---
> 
> (Updated Aug. 14, 2017, 11:40 p.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
> 
>

Re: Review Request 60495: Added network ports isolator listen socket utilities.

[Qian Zhang]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/60495/#review184478
---




src/slave/containerizer/mesos/isolators/network/ports.cpp
Lines 118-119 (patched)


A newline between.



src/slave/containerizer/mesos/isolators/network/ports.cpp
Lines 120-124 (patched)


Better to change to:
```
Error error = ErrnoError("Failed to read symbolic link '" + 
path::join(fdPath, entry->d_name) + "'"");
closedir(dir);
return error;
```



src/slave/containerizer/mesos/isolators/network/ports.cpp
Lines 124 (patched)


s/link/symbolic link/



src/slave/containerizer/mesos/isolators/network/ports.cpp
Lines 133 (patched)


I think here we need to add some code to handle the case that `readdir()` 
fails (i.e., it returns `nullptr`).



src/slave/containerizer/mesos/isolators/network/ports.cpp
Lines 134 (patched)


We need to check if `closedir()` succeeds or not.


- Qian Zhang


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

Re: Review Request 60495: Added network ports isolator listen socket utilities.

[Qian Zhang]

> On Aug. 8, 2017, 11:31 p.m., Qian Zhang wrote:
> > src/slave/containerizer/mesos/isolators/network/ports.cpp
> > Lines 201 (patched)
> > 
> >
> > 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//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//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.

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


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

Re: Review Request 60495: Added network ports isolator listen socket utilities.

[James Peach]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/60495/
---

(Updated Aug. 14, 2017, 11:40 p.m.)


Review request for mesos, Qian Zhang and Jiang Yan Xu.


Changes
---

Rebased and addressed review feedback.


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 (updated)
-

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

Changes: https://reviews.apache.org/r/60495/diff/8-9/


Testing
---

make check (Fedora 26)


Thanks,

James Peach

Re: Review Request 60495: Added network ports isolator listen socket utilities.

[Qian Zhang]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/60495/#review182665
---




src/slave/containerizer/mesos/isolators/network/ports.cpp
Lines 181 (patched)


Remove this blank line.



src/slave/containerizer/mesos/isolators/network/ports.cpp
Lines 183 (patched)


Add a newline before.


- Qian Zhang


On Aug. 10, 2017, 4:19 a.m., James Peach wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/60495/
> ---
> 
> (Updated Aug. 10, 2017, 4:19 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/8/
> 
> 
> Testing
> ---
> 
> make check (Fedora 26)
> 
> 
> Thanks,
> 
> James Peach
> 
>

Re: Review Request 60495: Added network ports isolator listen socket utilities.

[James Peach]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/60495/
---

(Updated Aug. 9, 2017, 8:19 p.m.)


Review request for mesos, Qian Zhang and Jiang Yan Xu.


Changes
---

Rebased and addressed review feedback.


Summary (updated)
-

Added network ports isolator listen socket utilities.


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 (updated)
-

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

Changes: https://reviews.apache.org/r/60495/diff/7-8/


Testing
---

make check (Fedora 26)


Thanks,

James Peach