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 |
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> && && ((arp.op == 1 && arp.spa == <var>VIP</var> && arp.tpa == <var>VIP</var>) || (arp.op == 2 && 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
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> && !is_chassis_resident(<var>V</var>) && ((arp.op == 1 && arp.spa == <var>VIP</var> && arp.tpa == <var>VIP</var>) || (arp.op == 2 && arp.spa == <var>VIP</var>))</code> > +<code>inport == <var>P</var> && && ((arp.op == 1 && arp.spa == <var>VIP</var> && arp.tpa == <var>VIP</var>) || (arp.op == 2 && 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
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> && > !is_chassis_resident(<var>V</var>) && ((arp.op == 1 && > arp.spa == <var>VIP</var> && arp.tpa == <var>VIP</var>) || (arp.op > == 2 && arp.spa == <var>VIP</var>))</code> > > +<code>inport == <var>P</var> && && ((arp.op == 1 > && arp.spa == <var>VIP</var> && arp.tpa == <var>VIP</var>) > || (arp.op == 2 && 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 > >
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> && !is_chassis_resident(<var>V</var>) && ((arp.op == 1 && arp.spa == <var>VIP</var> && arp.tpa == <var>VIP</var>) || (arp.op == 2 && arp.spa == <var>VIP</var>))</code> > +<code>inport == <var>P</var> && && ((arp.op == 1 && arp.spa == <var>VIP</var> && arp.tpa == <var>VIP</var>) || (arp.op == 2 && 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 --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> && !is_chassis_resident(<var>V</var>) && ((arp.op == 1 && arp.spa == <var>VIP</var> && arp.tpa == <var>VIP</var>) || (arp.op == 2 && arp.spa == <var>VIP</var>))</code> +<code>inport == <var>P</var> && && ((arp.op == 1 && arp.spa == <var>VIP</var> && arp.tpa == <var>VIP</var>) || (arp.op == 2 && 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