diff mbox series

[ovs-dev,ovn] Fix virtual port binding when the parents are scheduled in the same chassis

Message ID 20191016170018.12596-1-numans@ovn.org
State Accepted
Headers show
Series [ovs-dev,ovn] Fix virtual port binding when the parents are scheduled in the same chassis | expand

Commit Message

Numan Siddique Oct. 16, 2019, 5 p.m. UTC 
From: Numan Siddique <numans@ovn.org>

If a virtual port has 2 parents and if both of them are scheduled
on the same chassis, then virtual port binding doesn't work.

For virtual port binding we have the below logical flows:
inport == p1 && !is_chassis_resident("vip-port") && arp .. actions=bind_vport(vip-port); next;
inport == p2 && !is_chassis_resident("vip-port") && arp .. actions=bind_vport(vip-port); next;

Since we have !is_chassis_resident, as soon as p1 binds the port, the corresponding OF flows
for both the logical flows above are deleted. Because of this, when p2 becomes the
parent of vip-port it can't bind it.

This patch fixes this issue by removing this condition.

Reported-at: https://bugzilla.redhat.com/show_bug.cgi?id=1762341
Signed-off-by: Numan Siddique <numans@ovn.org>
---
 northd/ovn-northd.8.xml |  2 +-
 northd/ovn-northd.c     |  3 +--
 tests/ovn.at            | 39 ++++++++++++++++++++++++++++++++-------
 3 files changed, 34 insertions(+), 10 deletions(-)

Comments

0-day Robot Oct. 16, 2019, 5:57 p.m. UTC | #1 
Bleep bloop.  Greetings Numan Siddique, I am a robot and I have tried out your patch.
Thanks for your contribution.

I encountered some error that I wasn't expecting.  See the details below.


checkpatch:
WARNING: Line is 201 characters long (recommended limit is 79)
#36 FILE: northd/ovn-northd.8.xml:583:
<code>inport == <var>P</var> &amp;&amp; &amp;&amp; ((arp.op == 1 &amp;&amp; arp.spa == <var>VIP</var> &amp;&amp; arp.tpa == <var>VIP</var>) || (arp.op == 2 &amp;&amp; arp.spa == <var>VIP</var>))</code>

Lines checked: 151, Warnings: 1, Errors: 0


Please check this out.  If you feel there has been an error, please email aconole@redhat.com

Thanks,
0-day Robot
Dumitru Ceara Oct. 17, 2019, 9:39 a.m. UTC | #2 
On Wed, Oct 16, 2019 at 7:01 PM <numans@ovn.org> wrote:
>
> From: Numan Siddique <numans@ovn.org>
>
> If a virtual port has 2 parents and if both of them are scheduled
> on the same chassis, then virtual port binding doesn't work.
>
> For virtual port binding we have the below logical flows:
> inport == p1 && !is_chassis_resident("vip-port") && arp .. actions=bind_vport(vip-port); next;
> inport == p2 && !is_chassis_resident("vip-port") && arp .. actions=bind_vport(vip-port); next;
>
> Since we have !is_chassis_resident, as soon as p1 binds the port, the corresponding OF flows
> for both the logical flows above are deleted. Because of this, when p2 becomes the
> parent of vip-port it can't bind it.
>
> This patch fixes this issue by removing this condition.
>
> Reported-at: https://bugzilla.redhat.com/show_bug.cgi?id=1762341
> Signed-off-by: Numan Siddique <numans@ovn.org>

Hi Numan,

It makes sense and it looks good to me. Also ran the tests and
everything passed.

Acked-by: Dumitru Ceara <dceara@redhat.com>

Thanks,
Dumitru

> ---
>  northd/ovn-northd.8.xml |  2 +-
>  northd/ovn-northd.c     |  3 +--
>  tests/ovn.at            | 39 ++++++++++++++++++++++++++++++++-------
>  3 files changed, 34 insertions(+), 10 deletions(-)
>
> diff --git a/northd/ovn-northd.8.xml b/northd/ovn-northd.8.xml
> index b5dfcd183..d3e0e5ef2 100644
> --- a/northd/ovn-northd.8.xml
> +++ b/northd/ovn-northd.8.xml
> @@ -580,7 +580,7 @@
>            column with the match
>          </p>
>          <pre>
> -<code>inport == <var>P</var> &amp;&amp; !is_chassis_resident(<var>V</var>) &amp;&amp; ((arp.op == 1 &amp;&amp; arp.spa == <var>VIP</var> &amp;&amp; arp.tpa == <var>VIP</var>) || (arp.op == 2 &amp;&amp; arp.spa == <var>VIP</var>))</code>
> +<code>inport == <var>P</var> &amp;&amp; &amp;&amp; ((arp.op == 1 &amp;&amp; arp.spa == <var>VIP</var> &amp;&amp; arp.tpa == <var>VIP</var>) || (arp.op == 2 &amp;&amp; arp.spa == <var>VIP</var>))</code>
>          </pre>
>
>          <p>
> diff --git a/northd/ovn-northd.c b/northd/ovn-northd.c
> index d0844dd64..ea8ad7c2d 100644
> --- a/northd/ovn-northd.c
> +++ b/northd/ovn-northd.c
> @@ -5250,11 +5250,10 @@ build_lswitch_flows(struct hmap *datapaths, struct hmap *ports,
>
>                  ds_clear(&match);
>                  ds_put_format(&match, "inport == \"%s\" && "
> -                              "!is_chassis_resident(%s) && "
>                                "((arp.op == 1 && arp.spa == %s && "
>                                "arp.tpa == %s) || (arp.op == 2 && "
>                                "arp.spa == %s))",
> -                              vparent, op->json_key, virtual_ip, virtual_ip,
> +                              vparent, virtual_ip, virtual_ip,
>                                virtual_ip);
>                  ds_clear(&actions);
>                  ds_put_format(&actions,
> diff --git a/tests/ovn.at b/tests/ovn.at
> index d14136748..6bdb9e8ed 100644
> --- a/tests/ovn.at
> +++ b/tests/ovn.at
> @@ -15637,7 +15637,7 @@ ovn-nbctl lsp-set-addresses sw0-vir "50:54:00:00:00:10 10.0.0.10"
>  ovn-nbctl lsp-set-port-security sw0-vir "50:54:00:00:00:10 10.0.0.10"
>  ovn-nbctl lsp-set-type sw0-vir virtual
>  ovn-nbctl set logical_switch_port sw0-vir options:virtual-ip=10.0.0.10
> -ovn-nbctl set logical_switch_port sw0-vir options:virtual-parents=sw0-p1,sw0-p2
> +ovn-nbctl set logical_switch_port sw0-vir options:virtual-parents=sw0-p1,sw0-p2,sw0-p3
>
>  ovn-nbctl lsp-add sw0 sw0-p1
>  ovn-nbctl lsp-set-addresses sw0-p1 "50:54:00:00:00:03 10.0.0.3"
> @@ -15649,7 +15649,7 @@ ovn-nbctl lsp-set-port-security sw0-p2 "50:54:00:00:00:04 10.0.0.4 10.0.0.10"
>
>  ovn-nbctl lsp-add sw0 sw0-p3
>  ovn-nbctl lsp-set-addresses sw0-p3 "50:54:00:00:00:05 10.0.0.5"
> -ovn-nbctl lsp-set-port-security sw0-p3 "50:54:00:00:00:05 10.0.0.5"
> +ovn-nbctl lsp-set-port-security sw0-p3 "50:54:00:00:00:05 10.0.0.5 10.0.0.10"
>
>  # Create the second logical switch with one port
>  ovn-nbctl ls-add sw1
> @@ -15680,8 +15680,9 @@ ovn-nbctl --wait=hv sync
>  ovn-sbctl dump-flows sw0 | grep ls_in_arp_rsp | grep bind_vport > lflows.txt
>
>  AT_CHECK([cat lflows.txt], [0], [dnl
> -  table=11(ls_in_arp_rsp      ), priority=100  , match=(inport == "sw0-p1" && !is_chassis_resident("sw0-vir") && ((arp.op == 1 && arp.spa == 10.0.0.10 && arp.tpa == 10.0.0.10) || (arp.op == 2 && arp.spa == 10.0.0.10))), action=(bind_vport("sw0-vir", inport); next;)
> -  table=11(ls_in_arp_rsp      ), priority=100  , match=(inport == "sw0-p2" && !is_chassis_resident("sw0-vir") && ((arp.op == 1 && arp.spa == 10.0.0.10 && arp.tpa == 10.0.0.10) || (arp.op == 2 && arp.spa == 10.0.0.10))), action=(bind_vport("sw0-vir", inport); next;)
> +  table=11(ls_in_arp_rsp      ), priority=100  , match=(inport == "sw0-p1" && ((arp.op == 1 && arp.spa == 10.0.0.10 && arp.tpa == 10.0.0.10) || (arp.op == 2 && arp.spa == 10.0.0.10))), action=(bind_vport("sw0-vir", inport); next;)
> +  table=11(ls_in_arp_rsp      ), priority=100  , match=(inport == "sw0-p2" && ((arp.op == 1 && arp.spa == 10.0.0.10 && arp.tpa == 10.0.0.10) || (arp.op == 2 && arp.spa == 10.0.0.10))), action=(bind_vport("sw0-vir", inport); next;)
> +  table=11(ls_in_arp_rsp      ), priority=100  , match=(inport == "sw0-p3" && ((arp.op == 1 && arp.spa == 10.0.0.10 && arp.tpa == 10.0.0.10) || (arp.op == 2 && arp.spa == 10.0.0.10))), action=(bind_vport("sw0-vir", inport); next;)
>  ])
>
>  ovn-sbctl dump-flows lr0 | grep lr_in_arp_resolve | grep "reg0 == 10.0.0.10" \
> @@ -15729,6 +15730,29 @@ AT_CHECK([cat lflows.txt], [0], [dnl
>    table=11(lr_in_arp_resolve  ), priority=100  , match=(outport == "lr0-sw0" && reg0 == 10.0.0.10), action=(eth.dst = 50:54:00:00:00:03; next;)
>  ])
>
> +# From sw0-p3 send GARP for 10.0.0.10. hv1 should claim sw0-vir
> +# and sw0-p3 should be its virtual_parent.
> +eth_src=505400000005
> +eth_dst=ffffffffffff
> +spa=$(ip_to_hex 10 0 0 10)
> +tpa=$(ip_to_hex 10 0 0 10)
> +send_garp 1 2 $eth_src $eth_dst $spa $tpa
> +
> +OVS_WAIT_UNTIL([test x$(ovn-sbctl --bare --columns chassis find port_binding \
> +logical_port=sw0-vir) = x$hv1_ch_uuid], [0], [])
> +
> +OVS_WAIT_UNTIL([test x$(ovn-sbctl --bare --columns virtual_parent find port_binding \
> +logical_port=sw0-vir) = xsw0-p3])
> +
> +ovn-sbctl dump-flows lr0 | grep lr_in_arp_resolve | grep "reg0 == 10.0.0.10" \
> +> lflows.txt
> +
> +# There should be an arp resolve flow to resolve the virtual_ip with the
> +# sw0-p2's MAC.
> +AT_CHECK([cat lflows.txt], [0], [dnl
> +  table=11(lr_in_arp_resolve  ), priority=100  , match=(outport == "lr0-sw0" && reg0 == 10.0.0.10), action=(eth.dst = 50:54:00:00:00:05; next;)
> +])
> +
>  # send the garp from sw0-p2 (in hv2). hv2 should claim sw0-vir
>  # and sw0-p2 shpuld be its virtual_parent.
>  eth_src=505400000004
> @@ -15747,7 +15771,7 @@ ovn-sbctl dump-flows lr0 | grep lr_in_arp_resolve | grep "reg0 == 10.0.0.10" \
>  > lflows.txt
>
>  # There should be an arp resolve flow to resolve the virtual_ip with the
> -# sw0-p2's MAC.
> +# sw0-p3's MAC.
>  AT_CHECK([cat lflows.txt], [0], [dnl
>    table=11(lr_in_arp_resolve  ), priority=100  , match=(outport == "lr0-sw0" && reg0 == 10.0.0.10), action=(eth.dst = 50:54:00:00:00:04; next;)
>  ])
> @@ -15792,7 +15816,7 @@ AT_CHECK([cat lflows.txt], [0], [dnl
>  ])
>
>  # Now send arp reply from sw0-p2. hv2 should claim sw0-vir
> -# and sw0-p2 shpuld be its virtual_parent.
> +# and sw0-p2 should be its virtual_parent.
>  eth_src=505400000004
>  eth_dst=ffffffffffff
>  spa=$(ip_to_hex 10 0 0 10)
> @@ -15835,7 +15859,8 @@ ovn-nbctl --wait=hv set logical_switch_port sw0-vir options:virtual-ip=10.0.0.10
>  ovn-sbctl dump-flows sw0 | grep ls_in_arp_rsp | grep bind_vport > lflows.txt
>
>  AT_CHECK([cat lflows.txt], [0], [dnl
> -  table=11(ls_in_arp_rsp      ), priority=100  , match=(inport == "sw0-p1" && !is_chassis_resident("sw0-vir") && ((arp.op == 1 && arp.spa == 10.0.0.10 && arp.tpa == 10.0.0.10) || (arp.op == 2 && arp.spa == 10.0.0.10))), action=(bind_vport("sw0-vir", inport); next;)
> +  table=11(ls_in_arp_rsp      ), priority=100  , match=(inport == "sw0-p1" && ((arp.op == 1 && arp.spa == 10.0.0.10 && arp.tpa == 10.0.0.10) || (arp.op == 2 && arp.spa == 10.0.0.10))), action=(bind_vport("sw0-vir", inport); next;)
> +  table=11(ls_in_arp_rsp      ), priority=100  , match=(inport == "sw0-p3" && ((arp.op == 1 && arp.spa == 10.0.0.10 && arp.tpa == 10.0.0.10) || (arp.op == 2 && arp.spa == 10.0.0.10))), action=(bind_vport("sw0-vir", inport); next;)
>  ])
>
>  ovn-nbctl --wait=hv remove logical_switch_port sw0-vir options virtual-parents
> --
> 2.21.0
>
> _______________________________________________
> dev mailing list
> dev@openvswitch.org
> https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Numan Siddique Oct. 17, 2019, 9:45 a.m. UTC | #3 
On Thu, Oct 17, 2019 at 3:09 PM Dumitru Ceara <dceara@redhat.com> wrote:

> On Wed, Oct 16, 2019 at 7:01 PM <numans@ovn.org> wrote:
> >
> > From: Numan Siddique <numans@ovn.org>
> >
> > If a virtual port has 2 parents and if both of them are scheduled
> > on the same chassis, then virtual port binding doesn't work.
> >
> > For virtual port binding we have the below logical flows:
> > inport == p1 && !is_chassis_resident("vip-port") && arp ..
> actions=bind_vport(vip-port); next;
> > inport == p2 && !is_chassis_resident("vip-port") && arp ..
> actions=bind_vport(vip-port); next;
> >
> > Since we have !is_chassis_resident, as soon as p1 binds the port, the
> corresponding OF flows
> > for both the logical flows above are deleted. Because of this, when p2
> becomes the
> > parent of vip-port it can't bind it.
> >
> > This patch fixes this issue by removing this condition.
> >
> > Reported-at: https://bugzilla.redhat.com/show_bug.cgi?id=1762341
> > Signed-off-by: Numan Siddique <numans@ovn.org>
>
> Hi Numan,
>
> It makes sense and it looks good to me. Also ran the tests and
> everything passed.
>
> Acked-by: Dumitru Ceara <dceara@redhat.com>
>

Thanks for the review and testing it out. I applied this to master.

Numan


>
> Thanks,
> Dumitru
>
> > ---
> >  northd/ovn-northd.8.xml |  2 +-
> >  northd/ovn-northd.c     |  3 +--
> >  tests/ovn.at            | 39 ++++++++++++++++++++++++++++++++-------
> >  3 files changed, 34 insertions(+), 10 deletions(-)
> >
> > diff --git a/northd/ovn-northd.8.xml b/northd/ovn-northd.8.xml
> > index b5dfcd183..d3e0e5ef2 100644
> > --- a/northd/ovn-northd.8.xml
> > +++ b/northd/ovn-northd.8.xml
> > @@ -580,7 +580,7 @@
> >            column with the match
> >          </p>
> >          <pre>
> > -<code>inport == <var>P</var> &amp;&amp;
> !is_chassis_resident(<var>V</var>) &amp;&amp; ((arp.op == 1 &amp;&amp;
> arp.spa == <var>VIP</var> &amp;&amp; arp.tpa == <var>VIP</var>) || (arp.op
> == 2 &amp;&amp; arp.spa == <var>VIP</var>))</code>
> > +<code>inport == <var>P</var> &amp;&amp; &amp;&amp; ((arp.op == 1
> &amp;&amp; arp.spa == <var>VIP</var> &amp;&amp; arp.tpa == <var>VIP</var>)
> || (arp.op == 2 &amp;&amp; arp.spa == <var>VIP</var>))</code>
> >          </pre>
> >
> >          <p>
> > diff --git a/northd/ovn-northd.c b/northd/ovn-northd.c
> > index d0844dd64..ea8ad7c2d 100644
> > --- a/northd/ovn-northd.c
> > +++ b/northd/ovn-northd.c
> > @@ -5250,11 +5250,10 @@ build_lswitch_flows(struct hmap *datapaths,
> struct hmap *ports,
> >
> >                  ds_clear(&match);
> >                  ds_put_format(&match, "inport == \"%s\" && "
> > -                              "!is_chassis_resident(%s) && "
> >                                "((arp.op == 1 && arp.spa == %s && "
> >                                "arp.tpa == %s) || (arp.op == 2 && "
> >                                "arp.spa == %s))",
> > -                              vparent, op->json_key, virtual_ip,
> virtual_ip,
> > +                              vparent, virtual_ip, virtual_ip,
> >                                virtual_ip);
> >                  ds_clear(&actions);
> >                  ds_put_format(&actions,
> > diff --git a/tests/ovn.at b/tests/ovn.at
> > index d14136748..6bdb9e8ed 100644
> > --- a/tests/ovn.at
> > +++ b/tests/ovn.at
> > @@ -15637,7 +15637,7 @@ ovn-nbctl lsp-set-addresses sw0-vir
> "50:54:00:00:00:10 10.0.0.10"
> >  ovn-nbctl lsp-set-port-security sw0-vir "50:54:00:00:00:10 10.0.0.10"
> >  ovn-nbctl lsp-set-type sw0-vir virtual
> >  ovn-nbctl set logical_switch_port sw0-vir options:virtual-ip=10.0.0.10
> > -ovn-nbctl set logical_switch_port sw0-vir
> options:virtual-parents=sw0-p1,sw0-p2
> > +ovn-nbctl set logical_switch_port sw0-vir
> options:virtual-parents=sw0-p1,sw0-p2,sw0-p3
> >
> >  ovn-nbctl lsp-add sw0 sw0-p1
> >  ovn-nbctl lsp-set-addresses sw0-p1 "50:54:00:00:00:03 10.0.0.3"
> > @@ -15649,7 +15649,7 @@ ovn-nbctl lsp-set-port-security sw0-p2
> "50:54:00:00:00:04 10.0.0.4 10.0.0.10"
> >
> >  ovn-nbctl lsp-add sw0 sw0-p3
> >  ovn-nbctl lsp-set-addresses sw0-p3 "50:54:00:00:00:05 10.0.0.5"
> > -ovn-nbctl lsp-set-port-security sw0-p3 "50:54:00:00:00:05 10.0.0.5"
> > +ovn-nbctl lsp-set-port-security sw0-p3 "50:54:00:00:00:05 10.0.0.5
> 10.0.0.10"
> >
> >  # Create the second logical switch with one port
> >  ovn-nbctl ls-add sw1
> > @@ -15680,8 +15680,9 @@ ovn-nbctl --wait=hv sync
> >  ovn-sbctl dump-flows sw0 | grep ls_in_arp_rsp | grep bind_vport >
> lflows.txt
> >
> >  AT_CHECK([cat lflows.txt], [0], [dnl
> > -  table=11(ls_in_arp_rsp      ), priority=100  , match=(inport ==
> "sw0-p1" && !is_chassis_resident("sw0-vir") && ((arp.op == 1 && arp.spa ==
> 10.0.0.10 && arp.tpa == 10.0.0.10) || (arp.op == 2 && arp.spa ==
> 10.0.0.10))), action=(bind_vport("sw0-vir", inport); next;)
> > -  table=11(ls_in_arp_rsp      ), priority=100  , match=(inport ==
> "sw0-p2" && !is_chassis_resident("sw0-vir") && ((arp.op == 1 && arp.spa ==
> 10.0.0.10 && arp.tpa == 10.0.0.10) || (arp.op == 2 && arp.spa ==
> 10.0.0.10))), action=(bind_vport("sw0-vir", inport); next;)
> > +  table=11(ls_in_arp_rsp      ), priority=100  , match=(inport ==
> "sw0-p1" && ((arp.op == 1 && arp.spa == 10.0.0.10 && arp.tpa == 10.0.0.10)
> || (arp.op == 2 && arp.spa == 10.0.0.10))), action=(bind_vport("sw0-vir",
> inport); next;)
> > +  table=11(ls_in_arp_rsp      ), priority=100  , match=(inport ==
> "sw0-p2" && ((arp.op == 1 && arp.spa == 10.0.0.10 && arp.tpa == 10.0.0.10)
> || (arp.op == 2 && arp.spa == 10.0.0.10))), action=(bind_vport("sw0-vir",
> inport); next;)
> > +  table=11(ls_in_arp_rsp      ), priority=100  , match=(inport ==
> "sw0-p3" && ((arp.op == 1 && arp.spa == 10.0.0.10 && arp.tpa == 10.0.0.10)
> || (arp.op == 2 && arp.spa == 10.0.0.10))), action=(bind_vport("sw0-vir",
> inport); next;)
> >  ])
> >
> >  ovn-sbctl dump-flows lr0 | grep lr_in_arp_resolve | grep "reg0 ==
> 10.0.0.10" \
> > @@ -15729,6 +15730,29 @@ AT_CHECK([cat lflows.txt], [0], [dnl
> >    table=11(lr_in_arp_resolve  ), priority=100  , match=(outport ==
> "lr0-sw0" && reg0 == 10.0.0.10), action=(eth.dst = 50:54:00:00:00:03; next;)
> >  ])
> >
> > +# From sw0-p3 send GARP for 10.0.0.10. hv1 should claim sw0-vir
> > +# and sw0-p3 should be its virtual_parent.
> > +eth_src=505400000005
> > +eth_dst=ffffffffffff
> > +spa=$(ip_to_hex 10 0 0 10)
> > +tpa=$(ip_to_hex 10 0 0 10)
> > +send_garp 1 2 $eth_src $eth_dst $spa $tpa
> > +
> > +OVS_WAIT_UNTIL([test x$(ovn-sbctl --bare --columns chassis find
> port_binding \
> > +logical_port=sw0-vir) = x$hv1_ch_uuid], [0], [])
> > +
> > +OVS_WAIT_UNTIL([test x$(ovn-sbctl --bare --columns virtual_parent find
> port_binding \
> > +logical_port=sw0-vir) = xsw0-p3])
> > +
> > +ovn-sbctl dump-flows lr0 | grep lr_in_arp_resolve | grep "reg0 ==
> 10.0.0.10" \
> > +> lflows.txt
> > +
> > +# There should be an arp resolve flow to resolve the virtual_ip with the
> > +# sw0-p2's MAC.
> > +AT_CHECK([cat lflows.txt], [0], [dnl
> > +  table=11(lr_in_arp_resolve  ), priority=100  , match=(outport ==
> "lr0-sw0" && reg0 == 10.0.0.10), action=(eth.dst = 50:54:00:00:00:05; next;)
> > +])
> > +
> >  # send the garp from sw0-p2 (in hv2). hv2 should claim sw0-vir
> >  # and sw0-p2 shpuld be its virtual_parent.
> >  eth_src=505400000004
> > @@ -15747,7 +15771,7 @@ ovn-sbctl dump-flows lr0 | grep
> lr_in_arp_resolve | grep "reg0 == 10.0.0.10" \
> >  > lflows.txt
> >
> >  # There should be an arp resolve flow to resolve the virtual_ip with the
> > -# sw0-p2's MAC.
> > +# sw0-p3's MAC.
> >  AT_CHECK([cat lflows.txt], [0], [dnl
> >    table=11(lr_in_arp_resolve  ), priority=100  , match=(outport ==
> "lr0-sw0" && reg0 == 10.0.0.10), action=(eth.dst = 50:54:00:00:00:04; next;)
> >  ])
> > @@ -15792,7 +15816,7 @@ AT_CHECK([cat lflows.txt], [0], [dnl
> >  ])
> >
> >  # Now send arp reply from sw0-p2. hv2 should claim sw0-vir
> > -# and sw0-p2 shpuld be its virtual_parent.
> > +# and sw0-p2 should be its virtual_parent.
> >  eth_src=505400000004
> >  eth_dst=ffffffffffff
> >  spa=$(ip_to_hex 10 0 0 10)
> > @@ -15835,7 +15859,8 @@ ovn-nbctl --wait=hv set logical_switch_port
> sw0-vir options:virtual-ip=10.0.0.10
> >  ovn-sbctl dump-flows sw0 | grep ls_in_arp_rsp | grep bind_vport >
> lflows.txt
> >
> >  AT_CHECK([cat lflows.txt], [0], [dnl
> > -  table=11(ls_in_arp_rsp      ), priority=100  , match=(inport ==
> "sw0-p1" && !is_chassis_resident("sw0-vir") && ((arp.op == 1 && arp.spa ==
> 10.0.0.10 && arp.tpa == 10.0.0.10) || (arp.op == 2 && arp.spa ==
> 10.0.0.10))), action=(bind_vport("sw0-vir", inport); next;)
> > +  table=11(ls_in_arp_rsp      ), priority=100  , match=(inport ==
> "sw0-p1" && ((arp.op == 1 && arp.spa == 10.0.0.10 && arp.tpa == 10.0.0.10)
> || (arp.op == 2 && arp.spa == 10.0.0.10))), action=(bind_vport("sw0-vir",
> inport); next;)
> > +  table=11(ls_in_arp_rsp      ), priority=100  , match=(inport ==
> "sw0-p3" && ((arp.op == 1 && arp.spa == 10.0.0.10 && arp.tpa == 10.0.0.10)
> || (arp.op == 2 && arp.spa == 10.0.0.10))), action=(bind_vport("sw0-vir",
> inport); next;)
> >  ])
> >
> >  ovn-nbctl --wait=hv remove logical_switch_port sw0-vir options
> virtual-parents
> > --
> > 2.21.0
> >
> > _______________________________________________
> > dev mailing list
> > dev@openvswitch.org
> > https://mail.openvswitch.org/mailman/listinfo/ovs-dev
>
>
Lorenzo Bianconi Oct. 17, 2019, 10:38 a.m. UTC | #4 
On Oct 16, numans@ovn.org wrote:
> From: Numan Siddique <numans@ovn.org>
> 
> If a virtual port has 2 parents and if both of them are scheduled
> on the same chassis, then virtual port binding doesn't work.
> 
> For virtual port binding we have the below logical flows:
> inport == p1 && !is_chassis_resident("vip-port") && arp .. actions=bind_vport(vip-port); next;
> inport == p2 && !is_chassis_resident("vip-port") && arp .. actions=bind_vport(vip-port); next;
> 
> Since we have !is_chassis_resident, as soon as p1 binds the port, the corresponding OF flows
> for both the logical flows above are deleted. Because of this, when p2 becomes the
> parent of vip-port it can't bind it.
> 
> This patch fixes this issue by removing this condition.
> 
> Reported-at: https://bugzilla.redhat.com/show_bug.cgi?id=1762341
> Signed-off-by: Numan Siddique <numans@ovn.org>

Acked-by: Lorenzo Bianconi <lorenzo.bianconi@redhat.com>

> ---
>  northd/ovn-northd.8.xml |  2 +-
>  northd/ovn-northd.c     |  3 +--
>  tests/ovn.at            | 39 ++++++++++++++++++++++++++++++++-------
>  3 files changed, 34 insertions(+), 10 deletions(-)
> 
> diff --git a/northd/ovn-northd.8.xml b/northd/ovn-northd.8.xml
> index b5dfcd183..d3e0e5ef2 100644
> --- a/northd/ovn-northd.8.xml
> +++ b/northd/ovn-northd.8.xml
> @@ -580,7 +580,7 @@
>            column with the match
>          </p>
>          <pre>
> -<code>inport == <var>P</var> &amp;&amp; !is_chassis_resident(<var>V</var>) &amp;&amp; ((arp.op == 1 &amp;&amp; arp.spa == <var>VIP</var> &amp;&amp; arp.tpa == <var>VIP</var>) || (arp.op == 2 &amp;&amp; arp.spa == <var>VIP</var>))</code>
> +<code>inport == <var>P</var> &amp;&amp; &amp;&amp; ((arp.op == 1 &amp;&amp; arp.spa == <var>VIP</var> &amp;&amp; arp.tpa == <var>VIP</var>) || (arp.op == 2 &amp;&amp; arp.spa == <var>VIP</var>))</code>
>          </pre>
>  
>          <p>
> diff --git a/northd/ovn-northd.c b/northd/ovn-northd.c
> index d0844dd64..ea8ad7c2d 100644
> --- a/northd/ovn-northd.c
> +++ b/northd/ovn-northd.c
> @@ -5250,11 +5250,10 @@ build_lswitch_flows(struct hmap *datapaths, struct hmap *ports,
>  
>                  ds_clear(&match);
>                  ds_put_format(&match, "inport == \"%s\" && "
> -                              "!is_chassis_resident(%s) && "
>                                "((arp.op == 1 && arp.spa == %s && "
>                                "arp.tpa == %s) || (arp.op == 2 && "
>                                "arp.spa == %s))",
> -                              vparent, op->json_key, virtual_ip, virtual_ip,
> +                              vparent, virtual_ip, virtual_ip,
>                                virtual_ip);
>                  ds_clear(&actions);
>                  ds_put_format(&actions,
> diff --git a/tests/ovn.at b/tests/ovn.at
> index d14136748..6bdb9e8ed 100644
> --- a/tests/ovn.at
> +++ b/tests/ovn.at
> @@ -15637,7 +15637,7 @@ ovn-nbctl lsp-set-addresses sw0-vir "50:54:00:00:00:10 10.0.0.10"
>  ovn-nbctl lsp-set-port-security sw0-vir "50:54:00:00:00:10 10.0.0.10"
>  ovn-nbctl lsp-set-type sw0-vir virtual
>  ovn-nbctl set logical_switch_port sw0-vir options:virtual-ip=10.0.0.10
> -ovn-nbctl set logical_switch_port sw0-vir options:virtual-parents=sw0-p1,sw0-p2
> +ovn-nbctl set logical_switch_port sw0-vir options:virtual-parents=sw0-p1,sw0-p2,sw0-p3
>  
>  ovn-nbctl lsp-add sw0 sw0-p1
>  ovn-nbctl lsp-set-addresses sw0-p1 "50:54:00:00:00:03 10.0.0.3"
> @@ -15649,7 +15649,7 @@ ovn-nbctl lsp-set-port-security sw0-p2 "50:54:00:00:00:04 10.0.0.4 10.0.0.10"
>  
>  ovn-nbctl lsp-add sw0 sw0-p3
>  ovn-nbctl lsp-set-addresses sw0-p3 "50:54:00:00:00:05 10.0.0.5"
> -ovn-nbctl lsp-set-port-security sw0-p3 "50:54:00:00:00:05 10.0.0.5"
> +ovn-nbctl lsp-set-port-security sw0-p3 "50:54:00:00:00:05 10.0.0.5 10.0.0.10"
>  
>  # Create the second logical switch with one port
>  ovn-nbctl ls-add sw1
> @@ -15680,8 +15680,9 @@ ovn-nbctl --wait=hv sync
>  ovn-sbctl dump-flows sw0 | grep ls_in_arp_rsp | grep bind_vport > lflows.txt
>  
>  AT_CHECK([cat lflows.txt], [0], [dnl
> -  table=11(ls_in_arp_rsp      ), priority=100  , match=(inport == "sw0-p1" && !is_chassis_resident("sw0-vir") && ((arp.op == 1 && arp.spa == 10.0.0.10 && arp.tpa == 10.0.0.10) || (arp.op == 2 && arp.spa == 10.0.0.10))), action=(bind_vport("sw0-vir", inport); next;)
> -  table=11(ls_in_arp_rsp      ), priority=100  , match=(inport == "sw0-p2" && !is_chassis_resident("sw0-vir") && ((arp.op == 1 && arp.spa == 10.0.0.10 && arp.tpa == 10.0.0.10) || (arp.op == 2 && arp.spa == 10.0.0.10))), action=(bind_vport("sw0-vir", inport); next;)
> +  table=11(ls_in_arp_rsp      ), priority=100  , match=(inport == "sw0-p1" && ((arp.op == 1 && arp.spa == 10.0.0.10 && arp.tpa == 10.0.0.10) || (arp.op == 2 && arp.spa == 10.0.0.10))), action=(bind_vport("sw0-vir", inport); next;)
> +  table=11(ls_in_arp_rsp      ), priority=100  , match=(inport == "sw0-p2" && ((arp.op == 1 && arp.spa == 10.0.0.10 && arp.tpa == 10.0.0.10) || (arp.op == 2 && arp.spa == 10.0.0.10))), action=(bind_vport("sw0-vir", inport); next;)
> +  table=11(ls_in_arp_rsp      ), priority=100  , match=(inport == "sw0-p3" && ((arp.op == 1 && arp.spa == 10.0.0.10 && arp.tpa == 10.0.0.10) || (arp.op == 2 && arp.spa == 10.0.0.10))), action=(bind_vport("sw0-vir", inport); next;)
>  ])
>  
>  ovn-sbctl dump-flows lr0 | grep lr_in_arp_resolve | grep "reg0 == 10.0.0.10" \
> @@ -15729,6 +15730,29 @@ AT_CHECK([cat lflows.txt], [0], [dnl
>    table=11(lr_in_arp_resolve  ), priority=100  , match=(outport == "lr0-sw0" && reg0 == 10.0.0.10), action=(eth.dst = 50:54:00:00:00:03; next;)
>  ])
>  
> +# From sw0-p3 send GARP for 10.0.0.10. hv1 should claim sw0-vir
> +# and sw0-p3 should be its virtual_parent.
> +eth_src=505400000005
> +eth_dst=ffffffffffff
> +spa=$(ip_to_hex 10 0 0 10)
> +tpa=$(ip_to_hex 10 0 0 10)
> +send_garp 1 2 $eth_src $eth_dst $spa $tpa
> +
> +OVS_WAIT_UNTIL([test x$(ovn-sbctl --bare --columns chassis find port_binding \
> +logical_port=sw0-vir) = x$hv1_ch_uuid], [0], [])
> +
> +OVS_WAIT_UNTIL([test x$(ovn-sbctl --bare --columns virtual_parent find port_binding \
> +logical_port=sw0-vir) = xsw0-p3])
> +
> +ovn-sbctl dump-flows lr0 | grep lr_in_arp_resolve | grep "reg0 == 10.0.0.10" \
> +> lflows.txt
> +
> +# There should be an arp resolve flow to resolve the virtual_ip with the
> +# sw0-p2's MAC.
> +AT_CHECK([cat lflows.txt], [0], [dnl
> +  table=11(lr_in_arp_resolve  ), priority=100  , match=(outport == "lr0-sw0" && reg0 == 10.0.0.10), action=(eth.dst = 50:54:00:00:00:05; next;)
> +])
> +
>  # send the garp from sw0-p2 (in hv2). hv2 should claim sw0-vir
>  # and sw0-p2 shpuld be its virtual_parent.
>  eth_src=505400000004
> @@ -15747,7 +15771,7 @@ ovn-sbctl dump-flows lr0 | grep lr_in_arp_resolve | grep "reg0 == 10.0.0.10" \
>  > lflows.txt
>  
>  # There should be an arp resolve flow to resolve the virtual_ip with the
> -# sw0-p2's MAC.
> +# sw0-p3's MAC.
>  AT_CHECK([cat lflows.txt], [0], [dnl
>    table=11(lr_in_arp_resolve  ), priority=100  , match=(outport == "lr0-sw0" && reg0 == 10.0.0.10), action=(eth.dst = 50:54:00:00:00:04; next;)
>  ])
> @@ -15792,7 +15816,7 @@ AT_CHECK([cat lflows.txt], [0], [dnl
>  ])
>  
>  # Now send arp reply from sw0-p2. hv2 should claim sw0-vir
> -# and sw0-p2 shpuld be its virtual_parent.
> +# and sw0-p2 should be its virtual_parent.
>  eth_src=505400000004
>  eth_dst=ffffffffffff
>  spa=$(ip_to_hex 10 0 0 10)
> @@ -15835,7 +15859,8 @@ ovn-nbctl --wait=hv set logical_switch_port sw0-vir options:virtual-ip=10.0.0.10
>  ovn-sbctl dump-flows sw0 | grep ls_in_arp_rsp | grep bind_vport > lflows.txt
>  
>  AT_CHECK([cat lflows.txt], [0], [dnl
> -  table=11(ls_in_arp_rsp      ), priority=100  , match=(inport == "sw0-p1" && !is_chassis_resident("sw0-vir") && ((arp.op == 1 && arp.spa == 10.0.0.10 && arp.tpa == 10.0.0.10) || (arp.op == 2 && arp.spa == 10.0.0.10))), action=(bind_vport("sw0-vir", inport); next;)
> +  table=11(ls_in_arp_rsp      ), priority=100  , match=(inport == "sw0-p1" && ((arp.op == 1 && arp.spa == 10.0.0.10 && arp.tpa == 10.0.0.10) || (arp.op == 2 && arp.spa == 10.0.0.10))), action=(bind_vport("sw0-vir", inport); next;)
> +  table=11(ls_in_arp_rsp      ), priority=100  , match=(inport == "sw0-p3" && ((arp.op == 1 && arp.spa == 10.0.0.10 && arp.tpa == 10.0.0.10) || (arp.op == 2 && arp.spa == 10.0.0.10))), action=(bind_vport("sw0-vir", inport); next;)
>  ])
>  
>  ovn-nbctl --wait=hv remove logical_switch_port sw0-vir options virtual-parents
> -- 
> 2.21.0
> 
> _______________________________________________
> dev mailing list
> dev@openvswitch.org
> https://mail.openvswitch.org/mailman/listinfo/ovs-dev
diff mbox series

Patch

diff --git a/northd/ovn-northd.8.xml b/northd/ovn-northd.8.xml
index b5dfcd183..d3e0e5ef2 100644
--- a/northd/ovn-northd.8.xml
+++ b/northd/ovn-northd.8.xml
@@ -580,7 +580,7 @@ 
           column with the match
         </p>
         <pre>
-<code>inport == <var>P</var> &amp;&amp; !is_chassis_resident(<var>V</var>) &amp;&amp; ((arp.op == 1 &amp;&amp; arp.spa == <var>VIP</var> &amp;&amp; arp.tpa == <var>VIP</var>) || (arp.op == 2 &amp;&amp; arp.spa == <var>VIP</var>))</code>
+<code>inport == <var>P</var> &amp;&amp; &amp;&amp; ((arp.op == 1 &amp;&amp; arp.spa == <var>VIP</var> &amp;&amp; arp.tpa == <var>VIP</var>) || (arp.op == 2 &amp;&amp; arp.spa == <var>VIP</var>))</code>
         </pre>
 
         <p>
diff --git a/northd/ovn-northd.c b/northd/ovn-northd.c
index d0844dd64..ea8ad7c2d 100644
--- a/northd/ovn-northd.c
+++ b/northd/ovn-northd.c
@@ -5250,11 +5250,10 @@  build_lswitch_flows(struct hmap *datapaths, struct hmap *ports,
 
                 ds_clear(&match);
                 ds_put_format(&match, "inport == \"%s\" && "
-                              "!is_chassis_resident(%s) && "
                               "((arp.op == 1 && arp.spa == %s && "
                               "arp.tpa == %s) || (arp.op == 2 && "
                               "arp.spa == %s))",
-                              vparent, op->json_key, virtual_ip, virtual_ip,
+                              vparent, virtual_ip, virtual_ip,
                               virtual_ip);
                 ds_clear(&actions);
                 ds_put_format(&actions,
diff --git a/tests/ovn.at b/tests/ovn.at
index d14136748..6bdb9e8ed 100644
--- a/tests/ovn.at
+++ b/tests/ovn.at
@@ -15637,7 +15637,7 @@  ovn-nbctl lsp-set-addresses sw0-vir "50:54:00:00:00:10 10.0.0.10"
 ovn-nbctl lsp-set-port-security sw0-vir "50:54:00:00:00:10 10.0.0.10"
 ovn-nbctl lsp-set-type sw0-vir virtual
 ovn-nbctl set logical_switch_port sw0-vir options:virtual-ip=10.0.0.10
-ovn-nbctl set logical_switch_port sw0-vir options:virtual-parents=sw0-p1,sw0-p2
+ovn-nbctl set logical_switch_port sw0-vir options:virtual-parents=sw0-p1,sw0-p2,sw0-p3
 
 ovn-nbctl lsp-add sw0 sw0-p1
 ovn-nbctl lsp-set-addresses sw0-p1 "50:54:00:00:00:03 10.0.0.3"
@@ -15649,7 +15649,7 @@  ovn-nbctl lsp-set-port-security sw0-p2 "50:54:00:00:00:04 10.0.0.4 10.0.0.10"
 
 ovn-nbctl lsp-add sw0 sw0-p3
 ovn-nbctl lsp-set-addresses sw0-p3 "50:54:00:00:00:05 10.0.0.5"
-ovn-nbctl lsp-set-port-security sw0-p3 "50:54:00:00:00:05 10.0.0.5"
+ovn-nbctl lsp-set-port-security sw0-p3 "50:54:00:00:00:05 10.0.0.5 10.0.0.10"
 
 # Create the second logical switch with one port
 ovn-nbctl ls-add sw1
@@ -15680,8 +15680,9 @@  ovn-nbctl --wait=hv sync
 ovn-sbctl dump-flows sw0 | grep ls_in_arp_rsp | grep bind_vport > lflows.txt
 
 AT_CHECK([cat lflows.txt], [0], [dnl
-  table=11(ls_in_arp_rsp      ), priority=100  , match=(inport == "sw0-p1" && !is_chassis_resident("sw0-vir") && ((arp.op == 1 && arp.spa == 10.0.0.10 && arp.tpa == 10.0.0.10) || (arp.op == 2 && arp.spa == 10.0.0.10))), action=(bind_vport("sw0-vir", inport); next;)
-  table=11(ls_in_arp_rsp      ), priority=100  , match=(inport == "sw0-p2" && !is_chassis_resident("sw0-vir") && ((arp.op == 1 && arp.spa == 10.0.0.10 && arp.tpa == 10.0.0.10) || (arp.op == 2 && arp.spa == 10.0.0.10))), action=(bind_vport("sw0-vir", inport); next;)
+  table=11(ls_in_arp_rsp      ), priority=100  , match=(inport == "sw0-p1" && ((arp.op == 1 && arp.spa == 10.0.0.10 && arp.tpa == 10.0.0.10) || (arp.op == 2 && arp.spa == 10.0.0.10))), action=(bind_vport("sw0-vir", inport); next;)
+  table=11(ls_in_arp_rsp      ), priority=100  , match=(inport == "sw0-p2" && ((arp.op == 1 && arp.spa == 10.0.0.10 && arp.tpa == 10.0.0.10) || (arp.op == 2 && arp.spa == 10.0.0.10))), action=(bind_vport("sw0-vir", inport); next;)
+  table=11(ls_in_arp_rsp      ), priority=100  , match=(inport == "sw0-p3" && ((arp.op == 1 && arp.spa == 10.0.0.10 && arp.tpa == 10.0.0.10) || (arp.op == 2 && arp.spa == 10.0.0.10))), action=(bind_vport("sw0-vir", inport); next;)
 ])
 
 ovn-sbctl dump-flows lr0 | grep lr_in_arp_resolve | grep "reg0 == 10.0.0.10" \
@@ -15729,6 +15730,29 @@  AT_CHECK([cat lflows.txt], [0], [dnl
   table=11(lr_in_arp_resolve  ), priority=100  , match=(outport == "lr0-sw0" && reg0 == 10.0.0.10), action=(eth.dst = 50:54:00:00:00:03; next;)
 ])
 
+# From sw0-p3 send GARP for 10.0.0.10. hv1 should claim sw0-vir
+# and sw0-p3 should be its virtual_parent.
+eth_src=505400000005
+eth_dst=ffffffffffff
+spa=$(ip_to_hex 10 0 0 10)
+tpa=$(ip_to_hex 10 0 0 10)
+send_garp 1 2 $eth_src $eth_dst $spa $tpa
+
+OVS_WAIT_UNTIL([test x$(ovn-sbctl --bare --columns chassis find port_binding \
+logical_port=sw0-vir) = x$hv1_ch_uuid], [0], [])
+
+OVS_WAIT_UNTIL([test x$(ovn-sbctl --bare --columns virtual_parent find port_binding \
+logical_port=sw0-vir) = xsw0-p3])
+
+ovn-sbctl dump-flows lr0 | grep lr_in_arp_resolve | grep "reg0 == 10.0.0.10" \
+> lflows.txt
+
+# There should be an arp resolve flow to resolve the virtual_ip with the
+# sw0-p2's MAC.
+AT_CHECK([cat lflows.txt], [0], [dnl
+  table=11(lr_in_arp_resolve  ), priority=100  , match=(outport == "lr0-sw0" && reg0 == 10.0.0.10), action=(eth.dst = 50:54:00:00:00:05; next;)
+])
+
 # send the garp from sw0-p2 (in hv2). hv2 should claim sw0-vir
 # and sw0-p2 shpuld be its virtual_parent.
 eth_src=505400000004
@@ -15747,7 +15771,7 @@  ovn-sbctl dump-flows lr0 | grep lr_in_arp_resolve | grep "reg0 == 10.0.0.10" \
 > lflows.txt
 
 # There should be an arp resolve flow to resolve the virtual_ip with the
-# sw0-p2's MAC.
+# sw0-p3's MAC.
 AT_CHECK([cat lflows.txt], [0], [dnl
   table=11(lr_in_arp_resolve  ), priority=100  , match=(outport == "lr0-sw0" && reg0 == 10.0.0.10), action=(eth.dst = 50:54:00:00:00:04; next;)
 ])
@@ -15792,7 +15816,7 @@  AT_CHECK([cat lflows.txt], [0], [dnl
 ])
 
 # Now send arp reply from sw0-p2. hv2 should claim sw0-vir
-# and sw0-p2 shpuld be its virtual_parent.
+# and sw0-p2 should be its virtual_parent.
 eth_src=505400000004
 eth_dst=ffffffffffff
 spa=$(ip_to_hex 10 0 0 10)
@@ -15835,7 +15859,8 @@  ovn-nbctl --wait=hv set logical_switch_port sw0-vir options:virtual-ip=10.0.0.10
 ovn-sbctl dump-flows sw0 | grep ls_in_arp_rsp | grep bind_vport > lflows.txt
 
 AT_CHECK([cat lflows.txt], [0], [dnl
-  table=11(ls_in_arp_rsp      ), priority=100  , match=(inport == "sw0-p1" && !is_chassis_resident("sw0-vir") && ((arp.op == 1 && arp.spa == 10.0.0.10 && arp.tpa == 10.0.0.10) || (arp.op == 2 && arp.spa == 10.0.0.10))), action=(bind_vport("sw0-vir", inport); next;)
+  table=11(ls_in_arp_rsp      ), priority=100  , match=(inport == "sw0-p1" && ((arp.op == 1 && arp.spa == 10.0.0.10 && arp.tpa == 10.0.0.10) || (arp.op == 2 && arp.spa == 10.0.0.10))), action=(bind_vport("sw0-vir", inport); next;)
+  table=11(ls_in_arp_rsp      ), priority=100  , match=(inport == "sw0-p3" && ((arp.op == 1 && arp.spa == 10.0.0.10 && arp.tpa == 10.0.0.10) || (arp.op == 2 && arp.spa == 10.0.0.10))), action=(bind_vport("sw0-vir", inport); next;)
 ])
 
 ovn-nbctl --wait=hv remove logical_switch_port sw0-vir options virtual-parents