You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@mesos.apache.org by Cong Wang <cw...@twopensource.com> on 2015/10/20 20:57:20 UTC
Review Request 39490: Always create non-IP egress filters
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/39490/
-----------------------------------------------------------
Review request for mesos, Ian Downes and Jie Yu.
Repository: mesos
Description
-------
When rolling out the new flag --egress_unique_flow_per_container, I noticed, on some slaves, only IP egress filters were created as expected, the reset were not. Looking at the code, it looks like we skipped the creation if this is not the first container we create, this is wrong for this case, because egress filters were not created for previous containers yet. We should always create them and ignore error if they exist.
Diffs
-----
src/slave/containerizer/isolators/network/port_mapping.cpp e6bb75e6f5ba48a0c4cf6dd8f353e5f5843d0eef
Diff: https://reviews.apache.org/r/39490/diff/
Testing
-------
Manual tests
Thanks,
Cong Wang
Re: Review Request 39490: Always create non-IP egress filters
Posted by Jie Yu <yu...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/39490/#review107453
-----------------------------------------------------------
Ship it!
src/slave/containerizer/mesos/isolators/network/port_mapping.cpp (lines 2430 - 2432)
<https://reviews.apache.org/r/39490/#comment166560>
Please move this comments down (to where you increment the metrics)
src/slave/containerizer/mesos/isolators/network/port_mapping.cpp (line 2450)
<https://reviews.apache.org/r/39490/#comment166557>
Let's not increment this metrics. Otherwise, you'll have this metrics being increased everytime a new container is launched.
Instead, Please just add a comment and ignore this case.
src/slave/containerizer/mesos/isolators/network/port_mapping.cpp (line 2469)
<https://reviews.apache.org/r/39490/#comment166558>
Ditto here.
- Jie Yu
On Nov. 20, 2015, 11:36 p.m., Cong Wang wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/39490/
> -----------------------------------------------------------
>
> (Updated Nov. 20, 2015, 11:36 p.m.)
>
>
> Review request for mesos, Ian Downes and Jie Yu.
>
>
> Repository: mesos
>
>
> Description
> -------
>
> When rolling out the new flag --egress_unique_flow_per_container, I noticed, on some slaves, only IP egress filters were created as expected, the reset were not. Looking at the code, it looks like we skipped the creation if this is not the first container we create, this is wrong for this case, because egress filters were not created for previous containers yet. We should always create them and ignore error if they exist.
>
>
> Diffs
> -----
>
> src/slave/containerizer/mesos/isolators/network/port_mapping.cpp e50616fd609588c547c90bba6d7b3b9b3eb4c6a9
>
> Diff: https://reviews.apache.org/r/39490/diff/
>
>
> Testing
> -------
>
> Manual tests
>
>
> Thanks,
>
> Cong Wang
>
>
Re: Review Request 39490: Always create non-IP egress filters
Posted by Jie Yu <yu...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/39490/#review107455
-----------------------------------------------------------
I'll fix this for you.
- Jie Yu
On Nov. 20, 2015, 11:36 p.m., Cong Wang wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/39490/
> -----------------------------------------------------------
>
> (Updated Nov. 20, 2015, 11:36 p.m.)
>
>
> Review request for mesos, Ian Downes and Jie Yu.
>
>
> Repository: mesos
>
>
> Description
> -------
>
> When rolling out the new flag --egress_unique_flow_per_container, I noticed, on some slaves, only IP egress filters were created as expected, the reset were not. Looking at the code, it looks like we skipped the creation if this is not the first container we create, this is wrong for this case, because egress filters were not created for previous containers yet. We should always create them and ignore error if they exist.
>
>
> Diffs
> -----
>
> src/slave/containerizer/mesos/isolators/network/port_mapping.cpp e50616fd609588c547c90bba6d7b3b9b3eb4c6a9
>
> Diff: https://reviews.apache.org/r/39490/diff/
>
>
> Testing
> -------
>
> Manual tests
>
>
> Thanks,
>
> Cong Wang
>
>
Re: Review Request 39490: Always create non-IP egress filters
Posted by Cong Wang <cw...@twopensource.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/39490/
-----------------------------------------------------------
(Updated Nov. 20, 2015, 11:36 p.m.)
Review request for mesos, Ian Downes and Jie Yu.
Changes
-------
Rebase
Repository: mesos
Description
-------
When rolling out the new flag --egress_unique_flow_per_container, I noticed, on some slaves, only IP egress filters were created as expected, the reset were not. Looking at the code, it looks like we skipped the creation if this is not the first container we create, this is wrong for this case, because egress filters were not created for previous containers yet. We should always create them and ignore error if they exist.
Diffs (updated)
-----
src/slave/containerizer/mesos/isolators/network/port_mapping.cpp e50616fd609588c547c90bba6d7b3b9b3eb4c6a9
Diff: https://reviews.apache.org/r/39490/diff/
Testing
-------
Manual tests
Thanks,
Cong Wang
Re: Review Request 39490: Always create non-IP egress filters
Posted by Cong Wang <cw...@twopensource.com>.
> On Oct. 27, 2015, 5:38 p.m., Ian Downes wrote:
> > src/slave/containerizer/isolators/network/port_mapping.cpp, line 2431
> > <https://reviews.apache.org/r/39490/diff/1/?file=1102058#file1102058line2431>
> >
> > Why is this no longer a failure?
They could be already created by the slave if the flag were enabled before restarting.
- Cong
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/39490/#review104183
-----------------------------------------------------------
On Oct. 20, 2015, 6:57 p.m., Cong Wang wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/39490/
> -----------------------------------------------------------
>
> (Updated Oct. 20, 2015, 6:57 p.m.)
>
>
> Review request for mesos, Ian Downes and Jie Yu.
>
>
> Repository: mesos
>
>
> Description
> -------
>
> When rolling out the new flag --egress_unique_flow_per_container, I noticed, on some slaves, only IP egress filters were created as expected, the reset were not. Looking at the code, it looks like we skipped the creation if this is not the first container we create, this is wrong for this case, because egress filters were not created for previous containers yet. We should always create them and ignore error if they exist.
>
>
> Diffs
> -----
>
> src/slave/containerizer/isolators/network/port_mapping.cpp e6bb75e6f5ba48a0c4cf6dd8f353e5f5843d0eef
>
> Diff: https://reviews.apache.org/r/39490/diff/
>
>
> Testing
> -------
>
> Manual tests
>
>
> Thanks,
>
> Cong Wang
>
>
Re: Review Request 39490: Always create non-IP egress filters
Posted by Ian Downes <ia...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/39490/#review104183
-----------------------------------------------------------
src/slave/containerizer/isolators/network/port_mapping.cpp (line 2431)
<https://reviews.apache.org/r/39490/#comment162473>
Why is this no longer a failure?
src/slave/containerizer/isolators/network/port_mapping.cpp (line 2450)
<https://reviews.apache.org/r/39490/#comment162474>
ditto?
- Ian Downes
On Oct. 20, 2015, 11:57 a.m., Cong Wang wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/39490/
> -----------------------------------------------------------
>
> (Updated Oct. 20, 2015, 11:57 a.m.)
>
>
> Review request for mesos, Ian Downes and Jie Yu.
>
>
> Repository: mesos
>
>
> Description
> -------
>
> When rolling out the new flag --egress_unique_flow_per_container, I noticed, on some slaves, only IP egress filters were created as expected, the reset were not. Looking at the code, it looks like we skipped the creation if this is not the first container we create, this is wrong for this case, because egress filters were not created for previous containers yet. We should always create them and ignore error if they exist.
>
>
> Diffs
> -----
>
> src/slave/containerizer/isolators/network/port_mapping.cpp e6bb75e6f5ba48a0c4cf6dd8f353e5f5843d0eef
>
> Diff: https://reviews.apache.org/r/39490/diff/
>
>
> Testing
> -------
>
> Manual tests
>
>
> Thanks,
>
> Cong Wang
>
>
Re: Review Request 39490: Always create non-IP egress filters
Posted by Mesos ReviewBot <re...@mesos.apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/39490/#review103304
-----------------------------------------------------------
Patch looks great!
Reviews applied: [39490]
All tests passed.
- Mesos ReviewBot
On Oct. 20, 2015, 6:57 p.m., Cong Wang wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/39490/
> -----------------------------------------------------------
>
> (Updated Oct. 20, 2015, 6:57 p.m.)
>
>
> Review request for mesos, Ian Downes and Jie Yu.
>
>
> Repository: mesos
>
>
> Description
> -------
>
> When rolling out the new flag --egress_unique_flow_per_container, I noticed, on some slaves, only IP egress filters were created as expected, the reset were not. Looking at the code, it looks like we skipped the creation if this is not the first container we create, this is wrong for this case, because egress filters were not created for previous containers yet. We should always create them and ignore error if they exist.
>
>
> Diffs
> -----
>
> src/slave/containerizer/isolators/network/port_mapping.cpp e6bb75e6f5ba48a0c4cf6dd8f353e5f5843d0eef
>
> Diff: https://reviews.apache.org/r/39490/diff/
>
>
> Testing
> -------
>
> Manual tests
>
>
> Thanks,
>
> Cong Wang
>
>