You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@mesos.apache.org by Chi Zhang <ch...@gmail.com> on 2015/02/13 01:34:07 UTC
Review Request 30958: PortMappingIsolator: Remove ingress qdiscs on
eth0 and lo after the last container exists.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/30958/
-----------------------------------------------------------
Review request for mesos, Jie Yu and Cong Wang.
Repository: mesos
Description
-------
PortMappingIsolator: Remove ingress qdiscs on eth0 and lo after the last container exists.
Diffs
-----
src/slave/containerizer/isolators/network/port_mapping.cpp b860b17e59486cd6097183a4d3139fbd5c897b44
Diff: https://reviews.apache.org/r/30958/diff/
Testing
-------
Thanks,
Chi Zhang
Re: Review Request 30958: PortMappingIsolator: Remove ingress qdiscs
on eth0 and lo after the last container exists.
Posted by Chi Zhang <ch...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/30958/#review72315
-----------------------------------------------------------
src/slave/containerizer/isolators/network/port_mapping.cpp
<https://reviews.apache.org/r/30958/#comment118398>
This code is the same as the one in create. If an ingress qdisc already exists, Try will return a some, which is false. Since we only check if Try is an error, we should be fine. This is basically doing 'add them back if none exist now'.
- Chi Zhang
On Feb. 13, 2015, 12:48 a.m., Chi Zhang wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/30958/
> -----------------------------------------------------------
>
> (Updated Feb. 13, 2015, 12:48 a.m.)
>
>
> Review request for mesos, Jie Yu and Cong Wang.
>
>
> Repository: mesos
>
>
> Description
> -------
>
> PortMappingIsolator: Remove ingress qdiscs on eth0 and lo after the last container exits.
>
>
> Diffs
> -----
>
> src/slave/containerizer/isolators/network/port_mapping.cpp b860b17e59486cd6097183a4d3139fbd5c897b44
>
> Diff: https://reviews.apache.org/r/30958/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Chi Zhang
>
>
Re: Review Request 30958: PortMappingIsolator: Remove ingress qdiscs
on eth0 and lo after the last container exists.
Posted by Cong Wang <cw...@twopensource.com>.
> On Feb. 13, 2015, 2:14 a.m., Cong Wang wrote:
> > src/slave/containerizer/isolators/network/port_mapping.cpp, line 1622
> > <https://reviews.apache.org/r/30958/diff/1/?file=862684#file862684line1622>
> >
> > Unless I am on a wrong tree, why do you create this ingress qdisc again since we already do it in create()? Even if we really need it, you don't mention in subject or changelog.
>
> Chi Zhang wrote:
> This code is the same as the one in create. If an ingress qdisc already exists, Try will return a some, which is false. Since we only check if Try is an error, we should be fine. This is basically doing 'add them back if none exist now'.
I know. So in which case it is "none exist now"? And why do you add it in a patch with this $subject? IOW, why you add some code but hide it in changelog?
- Cong
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/30958/#review72312
-----------------------------------------------------------
On Feb. 13, 2015, 12:48 a.m., Chi Zhang wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/30958/
> -----------------------------------------------------------
>
> (Updated Feb. 13, 2015, 12:48 a.m.)
>
>
> Review request for mesos, Jie Yu and Cong Wang.
>
>
> Repository: mesos
>
>
> Description
> -------
>
> PortMappingIsolator: Remove ingress qdiscs on eth0 and lo after the last container exits.
>
>
> Diffs
> -----
>
> src/slave/containerizer/isolators/network/port_mapping.cpp b860b17e59486cd6097183a4d3139fbd5c897b44
>
> Diff: https://reviews.apache.org/r/30958/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Chi Zhang
>
>
Re: Review Request 30958: PortMappingIsolator: Remove ingress qdiscs
on eth0 and lo after the last container exists.
Posted by Chi Zhang <ch...@gmail.com>.
> On Feb. 13, 2015, 2:14 a.m., Cong Wang wrote:
> > src/slave/containerizer/isolators/network/port_mapping.cpp, line 1622
> > <https://reviews.apache.org/r/30958/diff/1/?file=862684#file862684line1622>
> >
> > Unless I am on a wrong tree, why do you create this ingress qdisc again since we already do it in create()? Even if we really need it, you don't mention in subject or changelog.
This code is the same as the one in create. If an ingress qdisc already exists, Try will return a some, which is false. Since we only check if Try is an error, we should be fine. This is basically doing 'add them back if none exist now'.
- Chi
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/30958/#review72312
-----------------------------------------------------------
On Feb. 13, 2015, 12:48 a.m., Chi Zhang wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/30958/
> -----------------------------------------------------------
>
> (Updated Feb. 13, 2015, 12:48 a.m.)
>
>
> Review request for mesos, Jie Yu and Cong Wang.
>
>
> Repository: mesos
>
>
> Description
> -------
>
> PortMappingIsolator: Remove ingress qdiscs on eth0 and lo after the last container exits.
>
>
> Diffs
> -----
>
> src/slave/containerizer/isolators/network/port_mapping.cpp b860b17e59486cd6097183a4d3139fbd5c897b44
>
> Diff: https://reviews.apache.org/r/30958/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Chi Zhang
>
>
Re: Review Request 30958: PortMappingIsolator: Better ingress qdisc
management on eth0 and lo.
Posted by Chi Zhang <ch...@gmail.com>.
> On Feb. 13, 2015, 2:14 a.m., Cong Wang wrote:
> > src/slave/containerizer/isolators/network/port_mapping.cpp, line 1622
> > <https://reviews.apache.org/r/30958/diff/1/?file=862684#file862684line1622>
> >
> > Unless I am on a wrong tree, why do you create this ingress qdisc again since we already do it in create()? Even if we really need it, you don't mention in subject or changelog.
>
> Chi Zhang wrote:
> This code is the same as the one in create. If an ingress qdisc already exists, Try will return a some, which is false. Since we only check if Try is an error, we should be fine. This is basically doing 'add them back if none exist now'.
>
> Cong Wang wrote:
> I know. So in which case it is "none exist now"? And why do you add it in a patch with this $subject? IOW, why you add some code but hide it in changelog?
The slave won't exit after the last executor but wait for the next container to come in, at which point new ingress qdiscs need to be added back. I have updated the commit message.
- Chi
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/30958/#review72312
-----------------------------------------------------------
On Feb. 13, 2015, 6:24 p.m., Chi Zhang wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/30958/
> -----------------------------------------------------------
>
> (Updated Feb. 13, 2015, 6:24 p.m.)
>
>
> Review request for mesos, Jie Yu and Cong Wang.
>
>
> Repository: mesos
>
>
> Description
> -------
>
> PortMappingIsolator: Better ingress qdisc management on eth0 and lo.
>
> Remove ingress qdiscs on eth0 and lo after the last container exits. They will
> be added back when the next container comes in. This makes sure after a clean
> shutdown, the sytem will be clean.
>
>
> Diffs
> -----
>
> src/slave/containerizer/isolators/network/port_mapping.cpp b860b17e59486cd6097183a4d3139fbd5c897b44
>
> Diff: https://reviews.apache.org/r/30958/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Chi Zhang
>
>
Re: Review Request 30958: PortMappingIsolator: Remove ingress qdiscs
on eth0 and lo after the last container exists.
Posted by Cong Wang <cw...@twopensource.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/30958/#review72312
-----------------------------------------------------------
src/slave/containerizer/isolators/network/port_mapping.cpp
<https://reviews.apache.org/r/30958/#comment118396>
Unless I am on a wrong tree, why do you create this ingress qdisc again since we already do it in create()? Even if we really need it, you don't mention in subject or changelog.
- Cong Wang
On Feb. 13, 2015, 12:48 a.m., Chi Zhang wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/30958/
> -----------------------------------------------------------
>
> (Updated Feb. 13, 2015, 12:48 a.m.)
>
>
> Review request for mesos, Jie Yu and Cong Wang.
>
>
> Repository: mesos
>
>
> Description
> -------
>
> PortMappingIsolator: Remove ingress qdiscs on eth0 and lo after the last container exits.
>
>
> Diffs
> -----
>
> src/slave/containerizer/isolators/network/port_mapping.cpp b860b17e59486cd6097183a4d3139fbd5c897b44
>
> Diff: https://reviews.apache.org/r/30958/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Chi Zhang
>
>
Re: Review Request 30958: PortMappingIsolator: Remove ingress qdiscs
on eth0 and lo after the last container exists.
Posted by Mesos ReviewBot <de...@mesos.apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/30958/#review72356
-----------------------------------------------------------
Patch looks great!
Reviews applied: [30958]
All tests passed.
- Mesos ReviewBot
On Feb. 13, 2015, 12:48 a.m., Chi Zhang wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/30958/
> -----------------------------------------------------------
>
> (Updated Feb. 13, 2015, 12:48 a.m.)
>
>
> Review request for mesos, Jie Yu and Cong Wang.
>
>
> Repository: mesos
>
>
> Description
> -------
>
> PortMappingIsolator: Remove ingress qdiscs on eth0 and lo after the last container exits.
>
>
> Diffs
> -----
>
> src/slave/containerizer/isolators/network/port_mapping.cpp b860b17e59486cd6097183a4d3139fbd5c897b44
>
> Diff: https://reviews.apache.org/r/30958/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Chi Zhang
>
>
Re: Review Request 30958: PortMappingIsolator: Better ingress qdisc
management on eth0 and lo.
Posted by Jie Yu <yu...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/30958/#review72640
-----------------------------------------------------------
src/slave/containerizer/isolators/network/port_mapping.cpp
<https://reviews.apache.org/r/30958/#comment118685>
Do you have a ticket explaining why this is needed?
src/slave/containerizer/isolators/network/port_mapping.cpp
<https://reviews.apache.org/r/30958/#comment118686>
If the ingress qdisc is not installed by mesos, will you be removing it? That being said, I want to understand the motivation for this in a JIRA.
- Jie Yu
On Feb. 13, 2015, 6:24 p.m., Chi Zhang wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/30958/
> -----------------------------------------------------------
>
> (Updated Feb. 13, 2015, 6:24 p.m.)
>
>
> Review request for mesos, Jie Yu and Cong Wang.
>
>
> Repository: mesos
>
>
> Description
> -------
>
> PortMappingIsolator: Better ingress qdisc management on eth0 and lo.
>
> Remove ingress qdiscs on eth0 and lo after the last container exits. They will
> be added back when the next container comes in. This makes sure after a clean
> shutdown, the sytem will be clean.
>
>
> Diffs
> -----
>
> src/slave/containerizer/isolators/network/port_mapping.cpp b860b17e59486cd6097183a4d3139fbd5c897b44
>
> Diff: https://reviews.apache.org/r/30958/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Chi Zhang
>
>
Re: Review Request 30958: PortMappingIsolator: Better ingress qdisc
management on eth0 and lo.
Posted by Dominic Hamon <dh...@twopensource.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/30958/#review72745
-----------------------------------------------------------
src/slave/containerizer/isolators/network/port_mapping.cpp
<https://reviews.apache.org/r/30958/#comment118800>
s/qiscs/qdiscs/
- Dominic Hamon
On Feb. 13, 2015, 10:24 a.m., Chi Zhang wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/30958/
> -----------------------------------------------------------
>
> (Updated Feb. 13, 2015, 10:24 a.m.)
>
>
> Review request for mesos, Jie Yu and Cong Wang.
>
>
> Repository: mesos
>
>
> Description
> -------
>
> PortMappingIsolator: Better ingress qdisc management on eth0 and lo.
>
> Remove ingress qdiscs on eth0 and lo after the last container exits. They will
> be added back when the next container comes in. This makes sure after a clean
> shutdown, the sytem will be clean.
>
>
> Diffs
> -----
>
> src/slave/containerizer/isolators/network/port_mapping.cpp b860b17e59486cd6097183a4d3139fbd5c897b44
>
> Diff: https://reviews.apache.org/r/30958/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Chi Zhang
>
>
Re: Review Request 30958: PortMappingIsolator: Better ingress qdisc
management on eth0 and lo.
Posted by Chi Zhang <ch...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/30958/
-----------------------------------------------------------
(Updated Feb. 25, 2015, 10:21 p.m.)
Review request for mesos, Jie Yu and Cong Wang.
Bugs: mesos-2370
https://issues.apache.org/jira/browse/mesos-2370
Repository: mesos
Description
-------
PortMappingIsolator: Better ingress qdisc management on eth0 and lo.
Remove ingress qdiscs on eth0 and lo after the last container exits. They will
be added back when the next container comes in. This makes sure after a clean
shutdown, the sytem will be clean.
Diffs
-----
src/slave/containerizer/isolators/network/port_mapping.cpp b860b17e59486cd6097183a4d3139fbd5c897b44
Diff: https://reviews.apache.org/r/30958/diff/
Testing
-------
Thanks,
Chi Zhang
Re: Review Request 30958: PortMappingIsolator: Better ingress qdisc
management on eth0 and lo.
Posted by Till Toenshoff <to...@me.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/30958/#review72496
-----------------------------------------------------------
Quick style fly-by-review... :) ...
src/slave/containerizer/isolators/network/port_mapping.cpp
<https://reviews.apache.org/r/30958/#comment118546>
This blank line can be removed, I guess.
- Till Toenshoff
On Feb. 13, 2015, 6:24 p.m., Chi Zhang wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/30958/
> -----------------------------------------------------------
>
> (Updated Feb. 13, 2015, 6:24 p.m.)
>
>
> Review request for mesos, Jie Yu and Cong Wang.
>
>
> Repository: mesos
>
>
> Description
> -------
>
> PortMappingIsolator: Better ingress qdisc management on eth0 and lo.
>
> Remove ingress qdiscs on eth0 and lo after the last container exits. They will
> be added back when the next container comes in. This makes sure after a clean
> shutdown, the sytem will be clean.
>
>
> Diffs
> -----
>
> src/slave/containerizer/isolators/network/port_mapping.cpp b860b17e59486cd6097183a4d3139fbd5c897b44
>
> Diff: https://reviews.apache.org/r/30958/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Chi Zhang
>
>
Re: Review Request 30958: PortMappingIsolator: Better ingress qdisc
management on eth0 and lo.
Posted by Mesos ReviewBot <de...@mesos.apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/30958/#review72480
-----------------------------------------------------------
Patch looks great!
Reviews applied: [30958]
All tests passed.
- Mesos ReviewBot
On Feb. 13, 2015, 6:24 p.m., Chi Zhang wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/30958/
> -----------------------------------------------------------
>
> (Updated Feb. 13, 2015, 6:24 p.m.)
>
>
> Review request for mesos, Jie Yu and Cong Wang.
>
>
> Repository: mesos
>
>
> Description
> -------
>
> PortMappingIsolator: Better ingress qdisc management on eth0 and lo.
>
> Remove ingress qdiscs on eth0 and lo after the last container exits. They will
> be added back when the next container comes in. This makes sure after a clean
> shutdown, the sytem will be clean.
>
>
> Diffs
> -----
>
> src/slave/containerizer/isolators/network/port_mapping.cpp b860b17e59486cd6097183a4d3139fbd5c897b44
>
> Diff: https://reviews.apache.org/r/30958/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Chi Zhang
>
>
Re: Review Request 30958: PortMappingIsolator: Better ingress qdisc
management on eth0 and lo.
Posted by Chi Zhang <ch...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/30958/
-----------------------------------------------------------
(Updated Feb. 13, 2015, 6:24 p.m.)
Review request for mesos, Jie Yu and Cong Wang.
Changes
-------
Better commmit messages.
Summary (updated)
-----------------
PortMappingIsolator: Better ingress qdisc management on eth0 and lo.
Repository: mesos
Description (updated)
-------
PortMappingIsolator: Better ingress qdisc management on eth0 and lo.
Remove ingress qdiscs on eth0 and lo after the last container exits. They will
be added back when the next container comes in. This makes sure after a clean
shutdown, the sytem will be clean.
Diffs (updated)
-----
src/slave/containerizer/isolators/network/port_mapping.cpp b860b17e59486cd6097183a4d3139fbd5c897b44
Diff: https://reviews.apache.org/r/30958/diff/
Testing
-------
Thanks,
Chi Zhang
Re: Review Request 30958: PortMappingIsolator: Remove ingress qdiscs
on eth0 and lo after the last container exists.
Posted by Chi Zhang <ch...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/30958/
-----------------------------------------------------------
(Updated Feb. 13, 2015, 12:48 a.m.)
Review request for mesos, Jie Yu and Cong Wang.
Repository: mesos
Description (updated)
-------
PortMappingIsolator: Remove ingress qdiscs on eth0 and lo after the last container exits.
Diffs
-----
src/slave/containerizer/isolators/network/port_mapping.cpp b860b17e59486cd6097183a4d3139fbd5c897b44
Diff: https://reviews.apache.org/r/30958/diff/
Testing
-------
Thanks,
Chi Zhang