You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@mesos.apache.org by Qian Zhang <zh...@gmail.com> on 2017/08/17 14:45:34 UTC
Re: Review Request 60496: Added socket checking to the network ports
isolator.
-----------------------------------------------------------
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
>
>
Re: Review Request 60496: Added socket checking to the network ports
isolator.
Posted by James Peach <jp...@apache.org>.
> On Aug. 17, 2017, 2:45 p.m., Qian Zhang wrote:
> > src/slave/containerizer/mesos/isolators/network/ports.cpp
> > Lines 148-150 (patched)
> > <https://reviews.apache.org/r/60496/diff/10/?file=1797108#file1797108line148>
> >
> > 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?
>
> James Peach wrote:
> I think we should keep the full address. There's no performance impact and it is helpful for code clarity and debugging.
Dropping this; let's talk further if you disagree :)
- James
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/60496/#review182691
-----------------------------------------------------------
On Sept. 8, 2017, 12:09 a.m., James Peach wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/60496/
> -----------------------------------------------------------
>
> (Updated Sept. 8, 2017, 12:09 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/18/
>
>
> Testing
> -------
>
> make check (Fedora 26)
>
>
> Thanks,
>
> James Peach
>
>
Re: Review Request 60496: Added socket checking to the network ports
isolator.
Posted by Qian Zhang <zh...@gmail.com>.
> On Aug. 17, 2017, 10:45 p.m., Qian Zhang wrote:
> > src/slave/containerizer/mesos/isolators/network/ports.cpp
> > Lines 156-157 (patched)
> > <https://reviews.apache.org/r/60496/diff/10/?file=1797108#file1797108line156>
> >
> > Do we really need this? I think showing pid like what you did in the `else` block below should be enough.
>
> James Peach wrote:
> Yes, I think this is definitely needed in order to understand why the isolator is killing processes. Any time you need to debug what is getting killed this will make is much easier to understand.
OK, then I think here in the `if` block we'd better to log pid as well, we should provide as much userful info as possible for user to debug.
- Qian
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/60496/#review182691
-----------------------------------------------------------
On Aug. 18, 2017, 1:36 a.m., James Peach wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/60496/
> -----------------------------------------------------------
>
> (Updated Aug. 18, 2017, 1:36 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/11/
>
>
> Testing
> -------
>
> make check (Fedora 26)
>
>
> Thanks,
>
> James Peach
>
>
Re: Review Request 60496: Added socket checking to the network ports
isolator.
Posted by James Peach <jp...@apache.org>.
> On Aug. 17, 2017, 2:45 p.m., Qian Zhang wrote:
> > src/slave/containerizer/mesos/isolators/network/ports.hpp
> > Lines 39 (patched)
> > <https://reviews.apache.org/r/60496/diff/10/?file=1797107#file1797107line39>
> >
> > 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)?
This is removed and replaces by a configuration option in [r/60592](https://reviews.apache.org/r/60592).
> On Aug. 17, 2017, 2:45 p.m., Qian Zhang wrote:
> > src/slave/containerizer/mesos/isolators/network/ports.cpp
> > Lines 128-133 (patched)
> > <https://reviews.apache.org/r/60496/diff/10/?file=1797108#file1797108line128>
> >
> > 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.
Dropped to `VLOG(1)` and commented.
> On Aug. 17, 2017, 2:45 p.m., Qian Zhang wrote:
> > src/slave/containerizer/mesos/isolators/network/ports.cpp
> > Lines 148-150 (patched)
> > <https://reviews.apache.org/r/60496/diff/10/?file=1797108#file1797108line148>
> >
> > 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?
I think we should keep the full address. There's no performance impact and it is helpful for code clarity and debugging.
> On Aug. 17, 2017, 2:45 p.m., Qian Zhang wrote:
> > src/slave/containerizer/mesos/isolators/network/ports.cpp
> > Lines 156-157 (patched)
> > <https://reviews.apache.org/r/60496/diff/10/?file=1797108#file1797108line156>
> >
> > Do we really need this? I think showing pid like what you did in the `else` block below should be enough.
Yes, I think this is definitely needed in order to understand why the isolator is killing processes. Any time you need to debug what is getting killed this will make is much easier to understand.
- James
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/60496/#review182691
-----------------------------------------------------------
On Aug. 17, 2017, 5:36 p.m., James Peach wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/60496/
> -----------------------------------------------------------
>
> (Updated Aug. 17, 2017, 5:36 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 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/11/
>
>
> Testing
> -------
>
> make check (Fedora 26)
>
>
> Thanks,
>
> James Peach
>
>