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/18 02:22:32 UTC
Review Request 39415: Error out when root qdisc already exists
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/39415/
-----------------------------------------------------------
Review request for mesos, Ian Downes and Jie Yu.
Repository: mesos
Description
-------
When we enable --egress_unique_flow_per_container, we need to install an fq_codel qdisc on root, but if a qdisc already exists on root, we should not continue since it could be not fq_codel at all, so just exit with error.
Diffs
-----
src/slave/containerizer/isolators/network/port_mapping.cpp e6bb75e6f5ba48a0c4cf6dd8f353e5f5843d0eef
Diff: https://reviews.apache.org/r/39415/diff/
Testing
-------
Manual tests, with a pre-installed HTB qdisc and without.
Thanks,
Cong Wang
Re: Review Request 39415: Error out when root qdisc already exists
Posted by Jie Yu <yu...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/39415/#review107270
-----------------------------------------------------------
Ship it!
Ship It!
- Jie Yu
On Nov. 19, 2015, 8:08 p.m., Cong Wang wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/39415/
> -----------------------------------------------------------
>
> (Updated Nov. 19, 2015, 8:08 p.m.)
>
>
> Review request for mesos, Ian Downes and Jie Yu.
>
>
> Repository: mesos
>
>
> Description
> -------
>
> When we enable --egress_unique_flow_per_container, we need to install an fq_codel qdisc on root, but if a qdisc already exists on root, we should not continue since it could be not fq_codel at all, so just exit with error.
>
>
> Diffs
> -----
>
> src/slave/containerizer/mesos/isolators/network/port_mapping.cpp cc0152ce44e9fa34210efda8a1c4a6374167eab1
>
> Diff: https://reviews.apache.org/r/39415/diff/
>
>
> Testing
> -------
>
> Manual tests, with a pre-installed HTB qdisc and without.
>
>
> Thanks,
>
> Cong Wang
>
>
Re: Review Request 39415: Error out when root qdisc already exists
Posted by Cong Wang <cw...@twopensource.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/39415/
-----------------------------------------------------------
(Updated Nov. 19, 2015, 8:08 p.m.)
Review request for mesos, Ian Downes and Jie Yu.
Changes
-------
Rebase
Repository: mesos
Description
-------
When we enable --egress_unique_flow_per_container, we need to install an fq_codel qdisc on root, but if a qdisc already exists on root, we should not continue since it could be not fq_codel at all, so just exit with error.
Diffs (updated)
-----
src/slave/containerizer/mesos/isolators/network/port_mapping.cpp cc0152ce44e9fa34210efda8a1c4a6374167eab1
Diff: https://reviews.apache.org/r/39415/diff/
Testing
-------
Manual tests, with a pre-installed HTB qdisc and without.
Thanks,
Cong Wang
Re: Review Request 39415: Error out when root qdisc already exists
Posted by Cong Wang <cw...@twopensource.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/39415/
-----------------------------------------------------------
(Updated Oct. 29, 2015, 12:11 a.m.)
Review request for mesos, Ian Downes and Jie Yu.
Changes
-------
Address review comments and rebase
Repository: mesos
Description
-------
When we enable --egress_unique_flow_per_container, we need to install an fq_codel qdisc on root, but if a qdisc already exists on root, we should not continue since it could be not fq_codel at all, so just exit with error.
Diffs (updated)
-----
src/slave/containerizer/mesos/isolators/network/port_mapping.cpp 1911ba6518190cbff6d72b56aaa477d508dbd391
Diff: https://reviews.apache.org/r/39415/diff/
Testing
-------
Manual tests, with a pre-installed HTB qdisc and without.
Thanks,
Cong Wang
Re: Review Request 39415: Error out when root qdisc already exists
Posted by Ian Downes <ia...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/39415/#review104185
-----------------------------------------------------------
Ship it!
Ship It!
- Ian Downes
On Oct. 17, 2015, 5:22 p.m., Cong Wang wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/39415/
> -----------------------------------------------------------
>
> (Updated Oct. 17, 2015, 5:22 p.m.)
>
>
> Review request for mesos, Ian Downes and Jie Yu.
>
>
> Repository: mesos
>
>
> Description
> -------
>
> When we enable --egress_unique_flow_per_container, we need to install an fq_codel qdisc on root, but if a qdisc already exists on root, we should not continue since it could be not fq_codel at all, so just exit with error.
>
>
> Diffs
> -----
>
> src/slave/containerizer/isolators/network/port_mapping.cpp e6bb75e6f5ba48a0c4cf6dd8f353e5f5843d0eef
>
> Diff: https://reviews.apache.org/r/39415/diff/
>
>
> Testing
> -------
>
> Manual tests, with a pre-installed HTB qdisc and without.
>
>
> Thanks,
>
> Cong Wang
>
>
Re: Review Request 39415: Error out when root qdisc already exists
Posted by Cong Wang <cw...@twopensource.com>.
> On Oct. 27, 2015, 5:51 p.m., Jie Yu wrote:
> > src/slave/containerizer/isolators/network/port_mapping.cpp, lines 1403-1404
> > <https://reviews.apache.org/r/39415/diff/1/?file=1100525#file1100525line1403>
> >
> > Can you explain more? What if the slave restarts after creating the qdisc, the slave will flap afterwards?
Hmm, if there any way to tell if this is a recover or normal start?
The problem I want to solve is:
1) create an HTB qdisc on root;
2) start mesos slave with this flag, which will try to create fq_codel on root too;
3) No error, because we ignore if root qdisc exists before us
4) But packets get dropped because our filters created for root qdisc are not supposed to be used for HTB
The silent drop is definitely not acceptable...
- Cong
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/39415/#review104187
-----------------------------------------------------------
On Oct. 18, 2015, 12:22 a.m., Cong Wang wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/39415/
> -----------------------------------------------------------
>
> (Updated Oct. 18, 2015, 12:22 a.m.)
>
>
> Review request for mesos, Ian Downes and Jie Yu.
>
>
> Repository: mesos
>
>
> Description
> -------
>
> When we enable --egress_unique_flow_per_container, we need to install an fq_codel qdisc on root, but if a qdisc already exists on root, we should not continue since it could be not fq_codel at all, so just exit with error.
>
>
> Diffs
> -----
>
> src/slave/containerizer/isolators/network/port_mapping.cpp e6bb75e6f5ba48a0c4cf6dd8f353e5f5843d0eef
>
> Diff: https://reviews.apache.org/r/39415/diff/
>
>
> Testing
> -------
>
> Manual tests, with a pre-installed HTB qdisc and without.
>
>
> Thanks,
>
> Cong Wang
>
>
Re: Review Request 39415: Error out when root qdisc already exists
Posted by Jie Yu <yu...@gmail.com>.
> On Oct. 27, 2015, 5:51 p.m., Jie Yu wrote:
> > src/slave/containerizer/isolators/network/port_mapping.cpp, lines 1403-1404
> > <https://reviews.apache.org/r/39415/diff/1/?file=1100525#file1100525line1403>
> >
> > Can you explain more? What if the slave restarts after creating the qdisc, the slave will flap afterwards?
>
> Cong Wang wrote:
> Hmm, if there any way to tell if this is a recover or normal start?
>
> The problem I want to solve is:
>
> 1) create an HTB qdisc on root;
> 2) start mesos slave with this flag, which will try to create fq_codel on root too;
> 3) No error, because we ignore if root qdisc exists before us
> 4) But packets get dropped because our filters created for root qdisc are not supposed to be used for HTB
>
> The silent drop is definitely not acceptable...
Can you check if the root qdisc is fq_codel if egress root qdisc exists? I thought we have fq_codel::exists(link, EGRESS_ROOT)?
- Jie
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/39415/#review104187
-----------------------------------------------------------
On Oct. 18, 2015, 12:22 a.m., Cong Wang wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/39415/
> -----------------------------------------------------------
>
> (Updated Oct. 18, 2015, 12:22 a.m.)
>
>
> Review request for mesos, Ian Downes and Jie Yu.
>
>
> Repository: mesos
>
>
> Description
> -------
>
> When we enable --egress_unique_flow_per_container, we need to install an fq_codel qdisc on root, but if a qdisc already exists on root, we should not continue since it could be not fq_codel at all, so just exit with error.
>
>
> Diffs
> -----
>
> src/slave/containerizer/isolators/network/port_mapping.cpp e6bb75e6f5ba48a0c4cf6dd8f353e5f5843d0eef
>
> Diff: https://reviews.apache.org/r/39415/diff/
>
>
> Testing
> -------
>
> Manual tests, with a pre-installed HTB qdisc and without.
>
>
> Thanks,
>
> Cong Wang
>
>
Re: Review Request 39415: Error out when root qdisc already exists
Posted by Jie Yu <yu...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/39415/#review104187
-----------------------------------------------------------
src/slave/containerizer/isolators/network/port_mapping.cpp (lines 1403 - 1404)
<https://reviews.apache.org/r/39415/#comment162475>
Can you explain more? What if the slave restarts after creating the qdisc, the slave will flap afterwards?
- Jie Yu
On Oct. 18, 2015, 12:22 a.m., Cong Wang wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/39415/
> -----------------------------------------------------------
>
> (Updated Oct. 18, 2015, 12:22 a.m.)
>
>
> Review request for mesos, Ian Downes and Jie Yu.
>
>
> Repository: mesos
>
>
> Description
> -------
>
> When we enable --egress_unique_flow_per_container, we need to install an fq_codel qdisc on root, but if a qdisc already exists on root, we should not continue since it could be not fq_codel at all, so just exit with error.
>
>
> Diffs
> -----
>
> src/slave/containerizer/isolators/network/port_mapping.cpp e6bb75e6f5ba48a0c4cf6dd8f353e5f5843d0eef
>
> Diff: https://reviews.apache.org/r/39415/diff/
>
>
> Testing
> -------
>
> Manual tests, with a pre-installed HTB qdisc and without.
>
>
> Thanks,
>
> Cong Wang
>
>