| Message ID | a72b6db5e9bd17f6687e86eb4f6b218e5758884f.1664905230.git.lorenzo.bianconi@redhat.com |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | [ovs-dev,v2] controller: fix ipv6 prefix delegation in gw router mode | expand |
| Context | Check | Description |
|---|---|---|
| ovsrobot/apply-robot | fail | apply and check: fail |
On Tue, Oct 4, 2022 at 11:00 AM Lorenzo Bianconi < lorenzo.bianconi@redhat.com> wrote: > > Fix regression in ipv6 prefix delegation running in gw router mode > introduce by the following commit '04292cc2dc2c ("controller: fix > potential segmentation violation when removing ports")'. > > Fixes: 04292cc2dc2c ("controller: fix potential segmentation violation when removing ports") > Reoported-at: https://bugzilla.redhat.com/show_bug.cgi?id=2129244 > Reoported-at: https://bugzilla.redhat.com/show_bug.cgi?id=2129247 > Acked-by: Mark Michelson <mmichels@redhat.com> > Signed-off-by: Lorenzo Bianconi <lorenzo.bianconi@redhat.com> > --- > Changes since v1: > - rename need_add_patch_peer_to_local in need_add_peer_to_local > - refactor IPv6 PD system-test > --- > controller/binding.c | 4 +-- > controller/local_data.c | 13 ++++--- > controller/local_data.h | 2 +- > tests/system-ovn.at | 79 +++++++++++++++++++++++++++-------------- > 4 files changed, 64 insertions(+), 34 deletions(-) > > diff --git a/controller/binding.c b/controller/binding.c > index 8f6b4b19d..c3d2b2e42 100644 > --- a/controller/binding.c > +++ b/controller/binding.c > @@ -2666,7 +2666,7 @@ consider_patch_port_for_local_datapaths(const struct sbrec_port_binding *pb, > get_local_datapath(b_ctx_out->local_datapaths, > peer->datapath->tunnel_key); > } > - if (peer_ld && need_add_patch_peer_to_local( > + if (peer_ld && need_add_peer_to_local( > b_ctx_in->sbrec_port_binding_by_name, peer, > b_ctx_in->chassis_rec)) { > add_local_datapath( > @@ -2681,7 +2681,7 @@ consider_patch_port_for_local_datapaths(const struct sbrec_port_binding *pb, > /* Add the peer datapath to the local datapaths if it's > * not present yet. > */ > - if (need_add_patch_peer_to_local( > + if (need_add_peer_to_local( > b_ctx_in->sbrec_port_binding_by_name, pb, > b_ctx_in->chassis_rec)) { > add_local_datapath_peer_port( > diff --git a/controller/local_data.c b/controller/local_data.c > index 9eee568d1..035f10fff 100644 > --- a/controller/local_data.c > +++ b/controller/local_data.c > @@ -115,14 +115,19 @@ local_datapath_destroy(struct local_datapath *ld) > free(ld); > } > > -/* Checks if pb is a patch port and the peer datapath should be added to local > - * datapaths. */ > +/* Checks if pb is running on local gw router or pb is a patch port > + * and the peer datapath should be added to local datapaths. */ > bool > -need_add_patch_peer_to_local( > +need_add_peer_to_local( > struct ovsdb_idl_index *sbrec_port_binding_by_name, > const struct sbrec_port_binding *pb, > const struct sbrec_chassis *chassis) > { > + /* This port is running on local gw router. */ > + if (!strcmp(pb->type, "l3gateway") && pb->chassis == chassis) { > + return true; > + } > + > /* If it is not a patch port, no peer to add. */ > if (strcmp(pb->type, "patch")) { > return false; > @@ -571,7 +576,7 @@ add_local_datapath__(struct ovsdb_idl_index *sbrec_datapath_binding_by_key, > peer_name); > > if (peer && peer->datapath) { > - if (need_add_patch_peer_to_local( > + if (need_add_peer_to_local( > sbrec_port_binding_by_name, pb, chassis)) { > struct local_datapath *peer_ld = > add_local_datapath__(sbrec_datapath_binding_by_key, > diff --git a/controller/local_data.h b/controller/local_data.h > index d898c8aa5..b5429eb58 100644 > --- a/controller/local_data.h > +++ b/controller/local_data.h > @@ -66,7 +66,7 @@ struct local_datapath *local_datapath_alloc( > struct local_datapath *get_local_datapath(const struct hmap *, > uint32_t tunnel_key); > bool > -need_add_patch_peer_to_local( > +need_add_peer_to_local( > struct ovsdb_idl_index *sbrec_port_binding_by_name, > const struct sbrec_port_binding *, > const struct sbrec_chassis *); > diff --git a/tests/system-ovn.at b/tests/system-ovn.at > index d6ecd6eee..a8bb05bd6 100644 > --- a/tests/system-ovn.at > +++ b/tests/system-ovn.at > @@ -5280,22 +5280,20 @@ AT_KEYWORDS([ovn-ipv6-prefix_d]) > ovn_start > OVS_TRAFFIC_VSWITCHD_START() > > -ADD_BR([br-int]) > -ADD_BR([br-ext]) > - > -ovs-ofctl add-flow br-ext action=normal > -# Set external-ids in br-int needed for ovn-controller > -ovs-vsctl \ > - -- set Open_vSwitch . external-ids:system-id=hv1 \ > - -- set Open_vSwitch . external-ids:ovn-remote=unix:$ovs_base/ovn-sb/ovn-sb.sock \ > - -- set Open_vSwitch . external-ids:ovn-encap-type=geneve \ > - -- set Open_vSwitch . external-ids:ovn-encap-ip=169.0.0.1 \ > - -- set bridge br-int fail-mode=secure other-config:disable-in-band=true > - > -# Start ovn-controller > -start_daemon ovn-controller > +cleanup_env () { > +ip netns exec server kill $(pidof dhcpd) > +ovn-nbctl lr-del R1 > +ovn-nbctl ls-del sw0 > +ovn-nbctl ls-del sw1 > +ovn-nbctl ls-del public > +} > > -ovn-nbctl lr-add R1 > +run_ipv6_pd_test () { > +if [[ "$1" == "gw-router" ]]; then > + ovn-nbctl create Logical_Router name=R1 options:chassis=hv1 > +else > + ovn-nbctl lr-add R1 > +fi > > ovn-nbctl ls-add sw0 > ovn-nbctl ls-add sw1 > @@ -5303,8 +5301,11 @@ ovn-nbctl ls-add public > > ovn-nbctl lrp-add R1 rp-sw0 00:00:01:01:02:03 192.168.1.1/24 > ovn-nbctl lrp-add R1 rp-sw1 00:00:03:01:02:03 192.168.2.1/24 > -ovn-nbctl lrp-add R1 rp-public 00:00:02:01:02:03 172.16.1.1/24 \ > - -- lrp-set-gateway-chassis rp-public hv1 > +ovn-nbctl lrp-add R1 rp-public 00:00:02:01:02:03 172.16.1.1/24 > + > +if [[ "$1" != "gw-router" ]]; then > + ovn-nbctl lrp-set-gateway-chassis rp-public hv1 > +fi > > ovn-nbctl lsp-add sw0 sw0-rp -- set Logical_Switch_Port sw0-rp \ > type=router options:router-port=rp-sw0 \ > @@ -5317,22 +5318,12 @@ ovn-nbctl lsp-add public public-rp -- set Logical_Switch_Port public-rp \ > type=router options:router-port=rp-public \ > -- lsp-set-addresses public-rp router > > -ADD_NAMESPACES(sw01) > -ADD_VETH(sw01, sw01, br-int, "192.168.1.2/24", "f0:00:00:01:02:03", \ > - "192.168.1.1") > ovn-nbctl lsp-add sw0 sw01 \ > -- lsp-set-addresses sw01 "f0:00:00:01:02:03 192.168.1.2" > > -ADD_NAMESPACES(sw11) > -ADD_VETH(sw11, sw11, br-int, "192.168.2.2/24", "f0:00:00:02:02:03", \ > - "192.168.2.1") > ovn-nbctl lsp-add sw1 sw11 \ > -- lsp-set-addresses sw11 "f0:00:00:02:02:03 192.168.2.2" > > -ADD_NAMESPACES(server) > -ADD_VETH(s1, server, br-ext, "2001:1db8:3333::2/64", "f0:00:00:01:02:05", \ > - "2001:1db8:3333::1") > - > OVS_WAIT_UNTIL([test "$(ip netns exec server ip a | grep 2001:1db8:3333::2 | grep tentative)" = ""]) > OVS_WAIT_UNTIL([test "$(ip netns exec server ip a | grep fe80 | grep tentative)" = ""]) > > @@ -5409,6 +5400,39 @@ OVS_WAIT_WHILE([test "$(ovn-nbctl get logical_router_port rp-sw0 ipv6_prefix | c > AT_CHECK([ovn-nbctl get logical_router_port rp-sw0 ipv6_prefix | cut -c3-16], [0], [dnl > [] > ]) > +} > + > +ADD_BR([br-int]) > +ADD_BR([br-ext]) > + > +ovs-ofctl add-flow br-ext action=normal > +# Set external-ids in br-int needed for ovn-controller > +ovs-vsctl \ > + -- set Open_vSwitch . external-ids:system-id=hv1 \ > + -- set Open_vSwitch . external-ids:ovn-remote=unix:$ovs_base/ovn-sb/ovn-sb.sock \ > + -- set Open_vSwitch . external-ids:ovn-encap-type=geneve \ > + -- set Open_vSwitch . external-ids:ovn-encap-ip=169.0.0.1 \ > + -- set bridge br-int fail-mode=secure other-config:disable-in-band=true > + > +# Start ovn-controller > +start_daemon ovn-controller > + > +ADD_NAMESPACES(sw01) > +ADD_VETH(sw01, sw01, br-int, "192.168.1.2/24", "f0:00:00:01:02:03", \ > + "192.168.1.1") > +ADD_NAMESPACES(sw11) > +ADD_VETH(sw11, sw11, br-int, "192.168.2.2/24", "f0:00:00:02:02:03", \ > + "192.168.2.1") > +ADD_NAMESPACES(server) > +ADD_VETH(s1, server, br-ext, "2001:1db8:3333::2/64", "f0:00:00:01:02:05", \ > + "2001:1db8:3333::1") > + > +# run test for distributed router mode > +run_ipv6_pd_test "distributed-router" > +# cleanup env > +cleanup_env > +# run test for gw router mode > +run_ipv6_pd_test "gw-router" > Thanks Lorenzo for the refactor. This is much cleaner than before! However, I think it would be better to define a function using "m4_define", and still keep two separate test cases (while sharing the same code). That way it is easier to tell the problem when one of the test cases fails. Sorry that I didn't make it more clear when posting the comment for v1. Please let me know if you would like to revise again, or I could do it when merging. In either case: Acked-by: Han Zhou <hzhou@ovn.org> > kill $(pidof ovn-controller) > > @@ -5423,6 +5447,7 @@ OVS_APP_EXIT_AND_WAIT([NORTHD_TYPE]) > > as > OVS_TRAFFIC_VSWITCHD_STOP(["/.*error receiving.*/d > +/failed to query port patch-.*/d > /.*terminating with signal 15.*/d"]) > AT_CLEANUP > ]) > -- > 2.37.3 >
Hi Lorenzo Thanks for providing this patch. One very minor typo: see below. Otherwise looks good to me Thanks Xavier On Tue, Oct 4, 2022 at 8:00 PM Lorenzo Bianconi <lorenzo.bianconi@redhat.com> wrote: > Fix regression in ipv6 prefix delegation running in gw router mode > introduce by the following commit '04292cc2dc2c ("controller: fix > potential segmentation violation when removing ports")'. > > Fixes: 04292cc2dc2c ("controller: fix potential segmentation violation > when removing ports") > Reoported-at: https://bugzilla.redhat.com/show_bug.cgi?id=2129244 > Reoported-at > <https://bugzilla.redhat.com/show_bug.cgi?id=2129244Reoported-at>: > https://bugzilla.redhat.com/show_bug.cgi?id=2129247 > <https://bugzilla.redhat.com/show_bug.cgi?id=2129247Acked-by> There is a typo on those two lines: should be Reported-at Acked-by <https://bugzilla.redhat.com/show_bug.cgi?id=2129247Acked-by>: > Mark Michelson <mmichels@redhat.com> > Signed-off-by: Lorenzo Bianconi <lorenzo.bianconi@redhat.com> > --- > Changes since v1: > - rename need_add_patch_peer_to_local in need_add_peer_to_local > - refactor IPv6 PD system-test > --- > controller/binding.c | 4 +-- > controller/local_data.c | 13 ++++--- > controller/local_data.h | 2 +- > tests/system-ovn.at | 79 +++++++++++++++++++++++++++-------------- > 4 files changed, 64 insertions(+), 34 deletions(-) > > diff --git a/controller/binding.c b/controller/binding.c > index 8f6b4b19d..c3d2b2e42 100644 > --- a/controller/binding.c > +++ b/controller/binding.c > @@ -2666,7 +2666,7 @@ consider_patch_port_for_local_datapaths(const struct > sbrec_port_binding *pb, > get_local_datapath(b_ctx_out->local_datapaths, > peer->datapath->tunnel_key); > } > - if (peer_ld && need_add_patch_peer_to_local( > + if (peer_ld && need_add_peer_to_local( > b_ctx_in->sbrec_port_binding_by_name, peer, > b_ctx_in->chassis_rec)) { > add_local_datapath( > @@ -2681,7 +2681,7 @@ consider_patch_port_for_local_datapaths(const struct > sbrec_port_binding *pb, > /* Add the peer datapath to the local datapaths if it's > * not present yet. > */ > - if (need_add_patch_peer_to_local( > + if (need_add_peer_to_local( > b_ctx_in->sbrec_port_binding_by_name, pb, > b_ctx_in->chassis_rec)) { > add_local_datapath_peer_port( > diff --git a/controller/local_data.c b/controller/local_data.c > index 9eee568d1..035f10fff 100644 > --- a/controller/local_data.c > +++ b/controller/local_data.c > @@ -115,14 +115,19 @@ local_datapath_destroy(struct local_datapath *ld) > free(ld); > } > > -/* Checks if pb is a patch port and the peer datapath should be added to > local > - * datapaths. */ > +/* Checks if pb is running on local gw router or pb is a patch port > + * and the peer datapath should be added to local datapaths. */ > bool > -need_add_patch_peer_to_local( > +need_add_peer_to_local( > struct ovsdb_idl_index *sbrec_port_binding_by_name, > const struct sbrec_port_binding *pb, > const struct sbrec_chassis *chassis) > { > + /* This port is running on local gw router. */ > + if (!strcmp(pb->type, "l3gateway") && pb->chassis == chassis) { > + return true; > + } > + > /* If it is not a patch port, no peer to add. */ > if (strcmp(pb->type, "patch")) { > return false; > @@ -571,7 +576,7 @@ add_local_datapath__(struct ovsdb_idl_index > *sbrec_datapath_binding_by_key, > peer_name); > > if (peer && peer->datapath) { > - if (need_add_patch_peer_to_local( > + if (need_add_peer_to_local( > sbrec_port_binding_by_name, pb, chassis)) { > struct local_datapath *peer_ld = > > add_local_datapath__(sbrec_datapath_binding_by_key, > diff --git a/controller/local_data.h b/controller/local_data.h > index d898c8aa5..b5429eb58 100644 > --- a/controller/local_data.h > +++ b/controller/local_data.h > @@ -66,7 +66,7 @@ struct local_datapath *local_datapath_alloc( > struct local_datapath *get_local_datapath(const struct hmap *, > uint32_t tunnel_key); > bool > -need_add_patch_peer_to_local( > +need_add_peer_to_local( > struct ovsdb_idl_index *sbrec_port_binding_by_name, > const struct sbrec_port_binding *, > const struct sbrec_chassis *); > diff --git a/tests/system-ovn.at b/tests/system-ovn.at > index d6ecd6eee..a8bb05bd6 100644 > --- a/tests/system-ovn.at > +++ b/tests/system-ovn.at > @@ -5280,22 +5280,20 @@ AT_KEYWORDS([ovn-ipv6-prefix_d]) > ovn_start > OVS_TRAFFIC_VSWITCHD_START() > > -ADD_BR([br-int]) > -ADD_BR([br-ext]) > - > -ovs-ofctl add-flow br-ext action=normal > -# Set external-ids in br-int needed for ovn-controller > -ovs-vsctl \ > - -- set Open_vSwitch . external-ids:system-id=hv1 \ > - -- set Open_vSwitch . > external-ids:ovn-remote=unix:$ovs_base/ovn-sb/ovn-sb.sock \ > - -- set Open_vSwitch . external-ids:ovn-encap-type=geneve \ > - -- set Open_vSwitch . external-ids:ovn-encap-ip=169.0.0.1 \ > - -- set bridge br-int fail-mode=secure > other-config:disable-in-band=true > - > -# Start ovn-controller > -start_daemon ovn-controller > +cleanup_env () { > +ip netns exec server kill $(pidof dhcpd) > +ovn-nbctl lr-del R1 > +ovn-nbctl ls-del sw0 > +ovn-nbctl ls-del sw1 > +ovn-nbctl ls-del public > +} > > -ovn-nbctl lr-add R1 > +run_ipv6_pd_test () { > +if [[ "$1" == "gw-router" ]]; then > + ovn-nbctl create Logical_Router name=R1 options:chassis=hv1 > +else > + ovn-nbctl lr-add R1 > +fi > > ovn-nbctl ls-add sw0 > ovn-nbctl ls-add sw1 > @@ -5303,8 +5301,11 @@ ovn-nbctl ls-add public > > ovn-nbctl lrp-add R1 rp-sw0 00:00:01:01:02:03 192.168.1.1/24 > ovn-nbctl lrp-add R1 rp-sw1 00:00:03:01:02:03 192.168.2.1/24 > -ovn-nbctl lrp-add R1 rp-public 00:00:02:01:02:03 172.16.1.1/24 \ > - -- lrp-set-gateway-chassis rp-public hv1 > +ovn-nbctl lrp-add R1 rp-public 00:00:02:01:02:03 172.16.1.1/24 > + > +if [[ "$1" != "gw-router" ]]; then > + ovn-nbctl lrp-set-gateway-chassis rp-public hv1 > +fi > > ovn-nbctl lsp-add sw0 sw0-rp -- set Logical_Switch_Port sw0-rp \ > type=router options:router-port=rp-sw0 \ > @@ -5317,22 +5318,12 @@ ovn-nbctl lsp-add public public-rp -- set > Logical_Switch_Port public-rp \ > type=router options:router-port=rp-public \ > -- lsp-set-addresses public-rp router > > -ADD_NAMESPACES(sw01) > -ADD_VETH(sw01, sw01, br-int, "192.168.1.2/24", "f0:00:00:01:02:03", \ > - "192.168.1.1") > ovn-nbctl lsp-add sw0 sw01 \ > -- lsp-set-addresses sw01 "f0:00:00:01:02:03 192.168.1.2" > > -ADD_NAMESPACES(sw11) > -ADD_VETH(sw11, sw11, br-int, "192.168.2.2/24", "f0:00:00:02:02:03", \ > - "192.168.2.1") > ovn-nbctl lsp-add sw1 sw11 \ > -- lsp-set-addresses sw11 "f0:00:00:02:02:03 192.168.2.2" > > -ADD_NAMESPACES(server) > -ADD_VETH(s1, server, br-ext, "2001:1db8:3333::2/64", "f0:00:00:01:02:05", > \ > - "2001:1db8:3333::1") > - > OVS_WAIT_UNTIL([test "$(ip netns exec server ip a | grep > 2001:1db8:3333::2 | grep tentative)" = ""]) > OVS_WAIT_UNTIL([test "$(ip netns exec server ip a | grep fe80 | grep > tentative)" = ""]) > > @@ -5409,6 +5400,39 @@ OVS_WAIT_WHILE([test "$(ovn-nbctl get > logical_router_port rp-sw0 ipv6_prefix | c > AT_CHECK([ovn-nbctl get logical_router_port rp-sw0 ipv6_prefix | cut > -c3-16], [0], [dnl > [] > ]) > +} > + > +ADD_BR([br-int]) > +ADD_BR([br-ext]) > + > +ovs-ofctl add-flow br-ext action=normal > +# Set external-ids in br-int needed for ovn-controller > +ovs-vsctl \ > + -- set Open_vSwitch . external-ids:system-id=hv1 \ > + -- set Open_vSwitch . > external-ids:ovn-remote=unix:$ovs_base/ovn-sb/ovn-sb.sock \ > + -- set Open_vSwitch . external-ids:ovn-encap-type=geneve \ > + -- set Open_vSwitch . external-ids:ovn-encap-ip=169.0.0.1 \ > + -- set bridge br-int fail-mode=secure > other-config:disable-in-band=true > + > +# Start ovn-controller > +start_daemon ovn-controller > + > +ADD_NAMESPACES(sw01) > +ADD_VETH(sw01, sw01, br-int, "192.168.1.2/24", "f0:00:00:01:02:03", \ > + "192.168.1.1") > +ADD_NAMESPACES(sw11) > +ADD_VETH(sw11, sw11, br-int, "192.168.2.2/24", "f0:00:00:02:02:03", \ > + "192.168.2.1") > +ADD_NAMESPACES(server) > +ADD_VETH(s1, server, br-ext, "2001:1db8:3333::2/64", "f0:00:00:01:02:05", > \ > + "2001:1db8:3333::1") > + > +# run test for distributed router mode > +run_ipv6_pd_test "distributed-router" > +# cleanup env > +cleanup_env > +# run test for gw router mode > +run_ipv6_pd_test "gw-router" > > kill $(pidof ovn-controller) > > @@ -5423,6 +5447,7 @@ OVS_APP_EXIT_AND_WAIT([NORTHD_TYPE]) > > as > OVS_TRAFFIC_VSWITCHD_STOP(["/.*error receiving.*/d > +/failed to query port patch-.*/d > /.*terminating with signal 15.*/d"]) > AT_CLEANUP > ]) > -- > 2.37.3 > >
> On Tue, Oct 4, 2022 at 11:00 AM Lorenzo Bianconi < > lorenzo.bianconi@redhat.com> wrote: > > > > Fix regression in ipv6 prefix delegation running in gw router mode > > introduce by the following commit '04292cc2dc2c ("controller: fix > > potential segmentation violation when removing ports")'. > > > > Fixes: 04292cc2dc2c ("controller: fix potential segmentation violation > when removing ports") > > Reoported-at: https://bugzilla.redhat.com/show_bug.cgi?id=2129244 > > Reoported-at: https://bugzilla.redhat.com/show_bug.cgi?id=2129247 > > Acked-by: Mark Michelson <mmichels@redhat.com> > > Signed-off-by: Lorenzo Bianconi <lorenzo.bianconi@redhat.com> > > --- > > Changes since v1: > > - rename need_add_patch_peer_to_local in need_add_peer_to_local > > - refactor IPv6 PD system-test > > --- > > controller/binding.c | 4 +-- > > controller/local_data.c | 13 ++++--- > > controller/local_data.h | 2 +- > > tests/system-ovn.at | 79 +++++++++++++++++++++++++++-------------- > > 4 files changed, 64 insertions(+), 34 deletions(-) > > > > diff --git a/controller/binding.c b/controller/binding.c > > index 8f6b4b19d..c3d2b2e42 100644 > > --- a/controller/binding.c > > +++ b/controller/binding.c > > @@ -2666,7 +2666,7 @@ consider_patch_port_for_local_datapaths(const > struct sbrec_port_binding *pb, > > get_local_datapath(b_ctx_out->local_datapaths, > > peer->datapath->tunnel_key); > > } > > - if (peer_ld && need_add_patch_peer_to_local( > > + if (peer_ld && need_add_peer_to_local( > > b_ctx_in->sbrec_port_binding_by_name, peer, > > b_ctx_in->chassis_rec)) { > > add_local_datapath( > > @@ -2681,7 +2681,7 @@ consider_patch_port_for_local_datapaths(const > struct sbrec_port_binding *pb, > > /* Add the peer datapath to the local datapaths if it's > > * not present yet. > > */ > > - if (need_add_patch_peer_to_local( > > + if (need_add_peer_to_local( > > b_ctx_in->sbrec_port_binding_by_name, pb, > > b_ctx_in->chassis_rec)) { > > add_local_datapath_peer_port( > > diff --git a/controller/local_data.c b/controller/local_data.c > > index 9eee568d1..035f10fff 100644 > > --- a/controller/local_data.c > > +++ b/controller/local_data.c > > @@ -115,14 +115,19 @@ local_datapath_destroy(struct local_datapath *ld) > > free(ld); > > } > > > > -/* Checks if pb is a patch port and the peer datapath should be added to > local > > - * datapaths. */ > > +/* Checks if pb is running on local gw router or pb is a patch port > > + * and the peer datapath should be added to local datapaths. */ > > bool > > -need_add_patch_peer_to_local( > > +need_add_peer_to_local( > > struct ovsdb_idl_index *sbrec_port_binding_by_name, > > const struct sbrec_port_binding *pb, > > const struct sbrec_chassis *chassis) > > { > > + /* This port is running on local gw router. */ > > + if (!strcmp(pb->type, "l3gateway") && pb->chassis == chassis) { > > + return true; > > + } > > + > > /* If it is not a patch port, no peer to add. */ > > if (strcmp(pb->type, "patch")) { > > return false; > > @@ -571,7 +576,7 @@ add_local_datapath__(struct ovsdb_idl_index > *sbrec_datapath_binding_by_key, > > peer_name); > > > > if (peer && peer->datapath) { > > - if (need_add_patch_peer_to_local( > > + if (need_add_peer_to_local( > > sbrec_port_binding_by_name, pb, chassis)) { > > struct local_datapath *peer_ld = > > > add_local_datapath__(sbrec_datapath_binding_by_key, > > diff --git a/controller/local_data.h b/controller/local_data.h > > index d898c8aa5..b5429eb58 100644 > > --- a/controller/local_data.h > > +++ b/controller/local_data.h > > @@ -66,7 +66,7 @@ struct local_datapath *local_datapath_alloc( > > struct local_datapath *get_local_datapath(const struct hmap *, > > uint32_t tunnel_key); > > bool > > -need_add_patch_peer_to_local( > > +need_add_peer_to_local( > > struct ovsdb_idl_index *sbrec_port_binding_by_name, > > const struct sbrec_port_binding *, > > const struct sbrec_chassis *); > > diff --git a/tests/system-ovn.at b/tests/system-ovn.at > > index d6ecd6eee..a8bb05bd6 100644 > > --- a/tests/system-ovn.at > > +++ b/tests/system-ovn.at > > @@ -5280,22 +5280,20 @@ AT_KEYWORDS([ovn-ipv6-prefix_d]) > > ovn_start > > OVS_TRAFFIC_VSWITCHD_START() > > > > -ADD_BR([br-int]) > > -ADD_BR([br-ext]) > > - > > -ovs-ofctl add-flow br-ext action=normal > > -# Set external-ids in br-int needed for ovn-controller > > -ovs-vsctl \ > > - -- set Open_vSwitch . external-ids:system-id=hv1 \ > > - -- set Open_vSwitch . > external-ids:ovn-remote=unix:$ovs_base/ovn-sb/ovn-sb.sock \ > > - -- set Open_vSwitch . external-ids:ovn-encap-type=geneve \ > > - -- set Open_vSwitch . external-ids:ovn-encap-ip=169.0.0.1 \ > > - -- set bridge br-int fail-mode=secure > other-config:disable-in-band=true > > - > > -# Start ovn-controller > > -start_daemon ovn-controller > > +cleanup_env () { > > +ip netns exec server kill $(pidof dhcpd) > > +ovn-nbctl lr-del R1 > > +ovn-nbctl ls-del sw0 > > +ovn-nbctl ls-del sw1 > > +ovn-nbctl ls-del public > > +} > > > > -ovn-nbctl lr-add R1 > > +run_ipv6_pd_test () { > > +if [[ "$1" == "gw-router" ]]; then > > + ovn-nbctl create Logical_Router name=R1 options:chassis=hv1 > > +else > > + ovn-nbctl lr-add R1 > > +fi > > > > ovn-nbctl ls-add sw0 > > ovn-nbctl ls-add sw1 > > @@ -5303,8 +5301,11 @@ ovn-nbctl ls-add public > > > > ovn-nbctl lrp-add R1 rp-sw0 00:00:01:01:02:03 192.168.1.1/24 > > ovn-nbctl lrp-add R1 rp-sw1 00:00:03:01:02:03 192.168.2.1/24 > > -ovn-nbctl lrp-add R1 rp-public 00:00:02:01:02:03 172.16.1.1/24 \ > > - -- lrp-set-gateway-chassis rp-public hv1 > > +ovn-nbctl lrp-add R1 rp-public 00:00:02:01:02:03 172.16.1.1/24 > > + > > +if [[ "$1" != "gw-router" ]]; then > > + ovn-nbctl lrp-set-gateway-chassis rp-public hv1 > > +fi > > > > ovn-nbctl lsp-add sw0 sw0-rp -- set Logical_Switch_Port sw0-rp \ > > type=router options:router-port=rp-sw0 \ > > @@ -5317,22 +5318,12 @@ ovn-nbctl lsp-add public public-rp -- set > Logical_Switch_Port public-rp \ > > type=router options:router-port=rp-public \ > > -- lsp-set-addresses public-rp router > > > > -ADD_NAMESPACES(sw01) > > -ADD_VETH(sw01, sw01, br-int, "192.168.1.2/24", "f0:00:00:01:02:03", \ > > - "192.168.1.1") > > ovn-nbctl lsp-add sw0 sw01 \ > > -- lsp-set-addresses sw01 "f0:00:00:01:02:03 192.168.1.2" > > > > -ADD_NAMESPACES(sw11) > > -ADD_VETH(sw11, sw11, br-int, "192.168.2.2/24", "f0:00:00:02:02:03", \ > > - "192.168.2.1") > > ovn-nbctl lsp-add sw1 sw11 \ > > -- lsp-set-addresses sw11 "f0:00:00:02:02:03 192.168.2.2" > > > > -ADD_NAMESPACES(server) > > -ADD_VETH(s1, server, br-ext, "2001:1db8:3333::2/64", > "f0:00:00:01:02:05", \ > > - "2001:1db8:3333::1") > > - > > OVS_WAIT_UNTIL([test "$(ip netns exec server ip a | grep > 2001:1db8:3333::2 | grep tentative)" = ""]) > > OVS_WAIT_UNTIL([test "$(ip netns exec server ip a | grep fe80 | grep > tentative)" = ""]) > > > > @@ -5409,6 +5400,39 @@ OVS_WAIT_WHILE([test "$(ovn-nbctl get > logical_router_port rp-sw0 ipv6_prefix | c > > AT_CHECK([ovn-nbctl get logical_router_port rp-sw0 ipv6_prefix | cut > -c3-16], [0], [dnl > > [] > > ]) > > +} > > + > > +ADD_BR([br-int]) > > +ADD_BR([br-ext]) > > + > > +ovs-ofctl add-flow br-ext action=normal > > +# Set external-ids in br-int needed for ovn-controller > > +ovs-vsctl \ > > + -- set Open_vSwitch . external-ids:system-id=hv1 \ > > + -- set Open_vSwitch . > external-ids:ovn-remote=unix:$ovs_base/ovn-sb/ovn-sb.sock \ > > + -- set Open_vSwitch . external-ids:ovn-encap-type=geneve \ > > + -- set Open_vSwitch . external-ids:ovn-encap-ip=169.0.0.1 \ > > + -- set bridge br-int fail-mode=secure > other-config:disable-in-band=true > > + > > +# Start ovn-controller > > +start_daemon ovn-controller > > + > > +ADD_NAMESPACES(sw01) > > +ADD_VETH(sw01, sw01, br-int, "192.168.1.2/24", "f0:00:00:01:02:03", \ > > + "192.168.1.1") > > +ADD_NAMESPACES(sw11) > > +ADD_VETH(sw11, sw11, br-int, "192.168.2.2/24", "f0:00:00:02:02:03", \ > > + "192.168.2.1") > > +ADD_NAMESPACES(server) > > +ADD_VETH(s1, server, br-ext, "2001:1db8:3333::2/64", > "f0:00:00:01:02:05", \ > > + "2001:1db8:3333::1") > > + > > +# run test for distributed router mode > > +run_ipv6_pd_test "distributed-router" > > +# cleanup env > > +cleanup_env > > +# run test for gw router mode > > +run_ipv6_pd_test "gw-router" > > > Thanks Lorenzo for the refactor. This is much cleaner than before! > However, I think it would be better to define a function using "m4_define", > and still keep two separate test cases (while sharing the same code). That > way it is easier to tell the problem when one of the test cases fails. > Sorry that I didn't make it more clear when posting the comment for v1. > > Please let me know if you would like to revise again, or I could do it when > merging. In either case: > Acked-by: Han Zhou <hzhou@ovn.org> Hi Han, I can do it, no worries. Do you want to move in m4_define function just environment configuration or all the test? Regards, Lorenzo > > > kill $(pidof ovn-controller) > > > > @@ -5423,6 +5447,7 @@ OVS_APP_EXIT_AND_WAIT([NORTHD_TYPE]) > > > > as > > OVS_TRAFFIC_VSWITCHD_STOP(["/.*error receiving.*/d > > +/failed to query port patch-.*/d > > /.*terminating with signal 15.*/d"]) > > AT_CLEANUP > > ]) > > -- > > 2.37.3 > >
On Oct 05, Xavier Simonart wrote: > Hi Lorenzo > > Thanks for providing this patch. > One very minor typo: see below. > Otherwise looks good to me > > Thanks > Xavier > > On Tue, Oct 4, 2022 at 8:00 PM Lorenzo Bianconi <lorenzo.bianconi@redhat.com> > wrote: > > > Fix regression in ipv6 prefix delegation running in gw router mode > > introduce by the following commit '04292cc2dc2c ("controller: fix > > potential segmentation violation when removing ports")'. > > > > Fixes: 04292cc2dc2c ("controller: fix potential segmentation violation > > when removing ports") > > Reoported-at: https://bugzilla.redhat.com/show_bug.cgi?id=2129244 > > Reoported-at > > <https://bugzilla.redhat.com/show_bug.cgi?id=2129244Reoported-at>: > > https://bugzilla.redhat.com/show_bug.cgi?id=2129247 > > <https://bugzilla.redhat.com/show_bug.cgi?id=2129247Acked-by> > > There is a typo on those two lines: should be Reported-at ops, I will fix it, thx. Regards, Lorenzo > > Acked-by <https://bugzilla.redhat.com/show_bug.cgi?id=2129247Acked-by>: > > Mark Michelson <mmichels@redhat.com> > > Signed-off-by: Lorenzo Bianconi <lorenzo.bianconi@redhat.com> > > --- > > Changes since v1: > > - rename need_add_patch_peer_to_local in need_add_peer_to_local > > - refactor IPv6 PD system-test > > --- > > controller/binding.c | 4 +-- > > controller/local_data.c | 13 ++++--- > > controller/local_data.h | 2 +- > > tests/system-ovn.at | 79 +++++++++++++++++++++++++++-------------- > > 4 files changed, 64 insertions(+), 34 deletions(-) > > > > diff --git a/controller/binding.c b/controller/binding.c > > index 8f6b4b19d..c3d2b2e42 100644 > > --- a/controller/binding.c > > +++ b/controller/binding.c > > @@ -2666,7 +2666,7 @@ consider_patch_port_for_local_datapaths(const struct > > sbrec_port_binding *pb, > > get_local_datapath(b_ctx_out->local_datapaths, > > peer->datapath->tunnel_key); > > } > > - if (peer_ld && need_add_patch_peer_to_local( > > + if (peer_ld && need_add_peer_to_local( > > b_ctx_in->sbrec_port_binding_by_name, peer, > > b_ctx_in->chassis_rec)) { > > add_local_datapath( > > @@ -2681,7 +2681,7 @@ consider_patch_port_for_local_datapaths(const struct > > sbrec_port_binding *pb, > > /* Add the peer datapath to the local datapaths if it's > > * not present yet. > > */ > > - if (need_add_patch_peer_to_local( > > + if (need_add_peer_to_local( > > b_ctx_in->sbrec_port_binding_by_name, pb, > > b_ctx_in->chassis_rec)) { > > add_local_datapath_peer_port( > > diff --git a/controller/local_data.c b/controller/local_data.c > > index 9eee568d1..035f10fff 100644 > > --- a/controller/local_data.c > > +++ b/controller/local_data.c > > @@ -115,14 +115,19 @@ local_datapath_destroy(struct local_datapath *ld) > > free(ld); > > } > > > > -/* Checks if pb is a patch port and the peer datapath should be added to > > local > > - * datapaths. */ > > +/* Checks if pb is running on local gw router or pb is a patch port > > + * and the peer datapath should be added to local datapaths. */ > > bool > > -need_add_patch_peer_to_local( > > +need_add_peer_to_local( > > struct ovsdb_idl_index *sbrec_port_binding_by_name, > > const struct sbrec_port_binding *pb, > > const struct sbrec_chassis *chassis) > > { > > + /* This port is running on local gw router. */ > > + if (!strcmp(pb->type, "l3gateway") && pb->chassis == chassis) { > > + return true; > > + } > > + > > /* If it is not a patch port, no peer to add. */ > > if (strcmp(pb->type, "patch")) { > > return false; > > @@ -571,7 +576,7 @@ add_local_datapath__(struct ovsdb_idl_index > > *sbrec_datapath_binding_by_key, > > peer_name); > > > > if (peer && peer->datapath) { > > - if (need_add_patch_peer_to_local( > > + if (need_add_peer_to_local( > > sbrec_port_binding_by_name, pb, chassis)) { > > struct local_datapath *peer_ld = > > > > add_local_datapath__(sbrec_datapath_binding_by_key, > > diff --git a/controller/local_data.h b/controller/local_data.h > > index d898c8aa5..b5429eb58 100644 > > --- a/controller/local_data.h > > +++ b/controller/local_data.h > > @@ -66,7 +66,7 @@ struct local_datapath *local_datapath_alloc( > > struct local_datapath *get_local_datapath(const struct hmap *, > > uint32_t tunnel_key); > > bool > > -need_add_patch_peer_to_local( > > +need_add_peer_to_local( > > struct ovsdb_idl_index *sbrec_port_binding_by_name, > > const struct sbrec_port_binding *, > > const struct sbrec_chassis *); > > diff --git a/tests/system-ovn.at b/tests/system-ovn.at > > index d6ecd6eee..a8bb05bd6 100644 > > --- a/tests/system-ovn.at > > +++ b/tests/system-ovn.at > > @@ -5280,22 +5280,20 @@ AT_KEYWORDS([ovn-ipv6-prefix_d]) > > ovn_start > > OVS_TRAFFIC_VSWITCHD_START() > > > > -ADD_BR([br-int]) > > -ADD_BR([br-ext]) > > - > > -ovs-ofctl add-flow br-ext action=normal > > -# Set external-ids in br-int needed for ovn-controller > > -ovs-vsctl \ > > - -- set Open_vSwitch . external-ids:system-id=hv1 \ > > - -- set Open_vSwitch . > > external-ids:ovn-remote=unix:$ovs_base/ovn-sb/ovn-sb.sock \ > > - -- set Open_vSwitch . external-ids:ovn-encap-type=geneve \ > > - -- set Open_vSwitch . external-ids:ovn-encap-ip=169.0.0.1 \ > > - -- set bridge br-int fail-mode=secure > > other-config:disable-in-band=true > > - > > -# Start ovn-controller > > -start_daemon ovn-controller > > +cleanup_env () { > > +ip netns exec server kill $(pidof dhcpd) > > +ovn-nbctl lr-del R1 > > +ovn-nbctl ls-del sw0 > > +ovn-nbctl ls-del sw1 > > +ovn-nbctl ls-del public > > +} > > > > -ovn-nbctl lr-add R1 > > +run_ipv6_pd_test () { > > +if [[ "$1" == "gw-router" ]]; then > > + ovn-nbctl create Logical_Router name=R1 options:chassis=hv1 > > +else > > + ovn-nbctl lr-add R1 > > +fi > > > > ovn-nbctl ls-add sw0 > > ovn-nbctl ls-add sw1 > > @@ -5303,8 +5301,11 @@ ovn-nbctl ls-add public > > > > ovn-nbctl lrp-add R1 rp-sw0 00:00:01:01:02:03 192.168.1.1/24 > > ovn-nbctl lrp-add R1 rp-sw1 00:00:03:01:02:03 192.168.2.1/24 > > -ovn-nbctl lrp-add R1 rp-public 00:00:02:01:02:03 172.16.1.1/24 \ > > - -- lrp-set-gateway-chassis rp-public hv1 > > +ovn-nbctl lrp-add R1 rp-public 00:00:02:01:02:03 172.16.1.1/24 > > + > > +if [[ "$1" != "gw-router" ]]; then > > + ovn-nbctl lrp-set-gateway-chassis rp-public hv1 > > +fi > > > > ovn-nbctl lsp-add sw0 sw0-rp -- set Logical_Switch_Port sw0-rp \ > > type=router options:router-port=rp-sw0 \ > > @@ -5317,22 +5318,12 @@ ovn-nbctl lsp-add public public-rp -- set > > Logical_Switch_Port public-rp \ > > type=router options:router-port=rp-public \ > > -- lsp-set-addresses public-rp router > > > > -ADD_NAMESPACES(sw01) > > -ADD_VETH(sw01, sw01, br-int, "192.168.1.2/24", "f0:00:00:01:02:03", \ > > - "192.168.1.1") > > ovn-nbctl lsp-add sw0 sw01 \ > > -- lsp-set-addresses sw01 "f0:00:00:01:02:03 192.168.1.2" > > > > -ADD_NAMESPACES(sw11) > > -ADD_VETH(sw11, sw11, br-int, "192.168.2.2/24", "f0:00:00:02:02:03", \ > > - "192.168.2.1") > > ovn-nbctl lsp-add sw1 sw11 \ > > -- lsp-set-addresses sw11 "f0:00:00:02:02:03 192.168.2.2" > > > > -ADD_NAMESPACES(server) > > -ADD_VETH(s1, server, br-ext, "2001:1db8:3333::2/64", "f0:00:00:01:02:05", > > \ > > - "2001:1db8:3333::1") > > - > > OVS_WAIT_UNTIL([test "$(ip netns exec server ip a | grep > > 2001:1db8:3333::2 | grep tentative)" = ""]) > > OVS_WAIT_UNTIL([test "$(ip netns exec server ip a | grep fe80 | grep > > tentative)" = ""]) > > > > @@ -5409,6 +5400,39 @@ OVS_WAIT_WHILE([test "$(ovn-nbctl get > > logical_router_port rp-sw0 ipv6_prefix | c > > AT_CHECK([ovn-nbctl get logical_router_port rp-sw0 ipv6_prefix | cut > > -c3-16], [0], [dnl > > [] > > ]) > > +} > > + > > +ADD_BR([br-int]) > > +ADD_BR([br-ext]) > > + > > +ovs-ofctl add-flow br-ext action=normal > > +# Set external-ids in br-int needed for ovn-controller > > +ovs-vsctl \ > > + -- set Open_vSwitch . external-ids:system-id=hv1 \ > > + -- set Open_vSwitch . > > external-ids:ovn-remote=unix:$ovs_base/ovn-sb/ovn-sb.sock \ > > + -- set Open_vSwitch . external-ids:ovn-encap-type=geneve \ > > + -- set Open_vSwitch . external-ids:ovn-encap-ip=169.0.0.1 \ > > + -- set bridge br-int fail-mode=secure > > other-config:disable-in-band=true > > + > > +# Start ovn-controller > > +start_daemon ovn-controller > > + > > +ADD_NAMESPACES(sw01) > > +ADD_VETH(sw01, sw01, br-int, "192.168.1.2/24", "f0:00:00:01:02:03", \ > > + "192.168.1.1") > > +ADD_NAMESPACES(sw11) > > +ADD_VETH(sw11, sw11, br-int, "192.168.2.2/24", "f0:00:00:02:02:03", \ > > + "192.168.2.1") > > +ADD_NAMESPACES(server) > > +ADD_VETH(s1, server, br-ext, "2001:1db8:3333::2/64", "f0:00:00:01:02:05", > > \ > > + "2001:1db8:3333::1") > > + > > +# run test for distributed router mode > > +run_ipv6_pd_test "distributed-router" > > +# cleanup env > > +cleanup_env > > +# run test for gw router mode > > +run_ipv6_pd_test "gw-router" > > > > kill $(pidof ovn-controller) > > > > @@ -5423,6 +5447,7 @@ OVS_APP_EXIT_AND_WAIT([NORTHD_TYPE]) > > > > as > > OVS_TRAFFIC_VSWITCHD_STOP(["/.*error receiving.*/d > > +/failed to query port patch-.*/d > > /.*terminating with signal 15.*/d"]) > > AT_CLEANUP > > ]) > > -- > > 2.37.3 > > > >
On Wed, Oct 5, 2022 at 6:06 AM Lorenzo Bianconi <lorenzo.bianconi@redhat.com> wrote: > > > On Tue, Oct 4, 2022 at 11:00 AM Lorenzo Bianconi < > > lorenzo.bianconi@redhat.com> wrote: > > > > > > Fix regression in ipv6 prefix delegation running in gw router mode > > > introduce by the following commit '04292cc2dc2c ("controller: fix > > > potential segmentation violation when removing ports")'. > > > > > > Fixes: 04292cc2dc2c ("controller: fix potential segmentation violation > > when removing ports") > > > Reoported-at: https://bugzilla.redhat.com/show_bug.cgi?id=2129244 > > > Reoported-at: https://bugzilla.redhat.com/show_bug.cgi?id=2129247 > > > Acked-by: Mark Michelson <mmichels@redhat.com> > > > Signed-off-by: Lorenzo Bianconi <lorenzo.bianconi@redhat.com> > > > --- > > > Changes since v1: > > > - rename need_add_patch_peer_to_local in need_add_peer_to_local > > > - refactor IPv6 PD system-test > > > --- > > > controller/binding.c | 4 +-- > > > controller/local_data.c | 13 ++++--- > > > controller/local_data.h | 2 +- > > > tests/system-ovn.at | 79 +++++++++++++++++++++++++++-------------- > > > 4 files changed, 64 insertions(+), 34 deletions(-) > > > > > > diff --git a/controller/binding.c b/controller/binding.c > > > index 8f6b4b19d..c3d2b2e42 100644 > > > --- a/controller/binding.c > > > +++ b/controller/binding.c > > > @@ -2666,7 +2666,7 @@ consider_patch_port_for_local_datapaths(const > > struct sbrec_port_binding *pb, > > > get_local_datapath(b_ctx_out->local_datapaths, > > > peer->datapath->tunnel_key); > > > } > > > - if (peer_ld && need_add_patch_peer_to_local( > > > + if (peer_ld && need_add_peer_to_local( > > > b_ctx_in->sbrec_port_binding_by_name, peer, > > > b_ctx_in->chassis_rec)) { > > > add_local_datapath( > > > @@ -2681,7 +2681,7 @@ consider_patch_port_for_local_datapaths(const > > struct sbrec_port_binding *pb, > > > /* Add the peer datapath to the local datapaths if it's > > > * not present yet. > > > */ > > > - if (need_add_patch_peer_to_local( > > > + if (need_add_peer_to_local( > > > b_ctx_in->sbrec_port_binding_by_name, pb, > > > b_ctx_in->chassis_rec)) { > > > add_local_datapath_peer_port( > > > diff --git a/controller/local_data.c b/controller/local_data.c > > > index 9eee568d1..035f10fff 100644 > > > --- a/controller/local_data.c > > > +++ b/controller/local_data.c > > > @@ -115,14 +115,19 @@ local_datapath_destroy(struct local_datapath *ld) > > > free(ld); > > > } > > > > > > -/* Checks if pb is a patch port and the peer datapath should be added to > > local > > > - * datapaths. */ > > > +/* Checks if pb is running on local gw router or pb is a patch port > > > + * and the peer datapath should be added to local datapaths. */ > > > bool > > > -need_add_patch_peer_to_local( > > > +need_add_peer_to_local( > > > struct ovsdb_idl_index *sbrec_port_binding_by_name, > > > const struct sbrec_port_binding *pb, > > > const struct sbrec_chassis *chassis) > > > { > > > + /* This port is running on local gw router. */ > > > + if (!strcmp(pb->type, "l3gateway") && pb->chassis == chassis) { > > > + return true; > > > + } > > > + > > > /* If it is not a patch port, no peer to add. */ > > > if (strcmp(pb->type, "patch")) { > > > return false; > > > @@ -571,7 +576,7 @@ add_local_datapath__(struct ovsdb_idl_index > > *sbrec_datapath_binding_by_key, > > > peer_name); > > > > > > if (peer && peer->datapath) { > > > - if (need_add_patch_peer_to_local( > > > + if (need_add_peer_to_local( > > > sbrec_port_binding_by_name, pb, chassis)) { > > > struct local_datapath *peer_ld = > > > > > add_local_datapath__(sbrec_datapath_binding_by_key, > > > diff --git a/controller/local_data.h b/controller/local_data.h > > > index d898c8aa5..b5429eb58 100644 > > > --- a/controller/local_data.h > > > +++ b/controller/local_data.h > > > @@ -66,7 +66,7 @@ struct local_datapath *local_datapath_alloc( > > > struct local_datapath *get_local_datapath(const struct hmap *, > > > uint32_t tunnel_key); > > > bool > > > -need_add_patch_peer_to_local( > > > +need_add_peer_to_local( > > > struct ovsdb_idl_index *sbrec_port_binding_by_name, > > > const struct sbrec_port_binding *, > > > const struct sbrec_chassis *); > > > diff --git a/tests/system-ovn.at b/tests/system-ovn.at > > > index d6ecd6eee..a8bb05bd6 100644 > > > --- a/tests/system-ovn.at > > > +++ b/tests/system-ovn.at > > > @@ -5280,22 +5280,20 @@ AT_KEYWORDS([ovn-ipv6-prefix_d]) > > > ovn_start > > > OVS_TRAFFIC_VSWITCHD_START() > > > > > > -ADD_BR([br-int]) > > > -ADD_BR([br-ext]) > > > - > > > -ovs-ofctl add-flow br-ext action=normal > > > -# Set external-ids in br-int needed for ovn-controller > > > -ovs-vsctl \ > > > - -- set Open_vSwitch . external-ids:system-id=hv1 \ > > > - -- set Open_vSwitch . > > external-ids:ovn-remote=unix:$ovs_base/ovn-sb/ovn-sb.sock \ > > > - -- set Open_vSwitch . external-ids:ovn-encap-type=geneve \ > > > - -- set Open_vSwitch . external-ids:ovn-encap-ip=169.0.0.1 \ > > > - -- set bridge br-int fail-mode=secure > > other-config:disable-in-band=true > > > - > > > -# Start ovn-controller > > > -start_daemon ovn-controller > > > +cleanup_env () { > > > +ip netns exec server kill $(pidof dhcpd) > > > +ovn-nbctl lr-del R1 > > > +ovn-nbctl ls-del sw0 > > > +ovn-nbctl ls-del sw1 > > > +ovn-nbctl ls-del public > > > +} > > > > > > -ovn-nbctl lr-add R1 > > > +run_ipv6_pd_test () { > > > +if [[ "$1" == "gw-router" ]]; then > > > + ovn-nbctl create Logical_Router name=R1 options:chassis=hv1 > > > +else > > > + ovn-nbctl lr-add R1 > > > +fi > > > > > > ovn-nbctl ls-add sw0 > > > ovn-nbctl ls-add sw1 > > > @@ -5303,8 +5301,11 @@ ovn-nbctl ls-add public > > > > > > ovn-nbctl lrp-add R1 rp-sw0 00:00:01:01:02:03 192.168.1.1/24 > > > ovn-nbctl lrp-add R1 rp-sw1 00:00:03:01:02:03 192.168.2.1/24 > > > -ovn-nbctl lrp-add R1 rp-public 00:00:02:01:02:03 172.16.1.1/24 \ > > > - -- lrp-set-gateway-chassis rp-public hv1 > > > +ovn-nbctl lrp-add R1 rp-public 00:00:02:01:02:03 172.16.1.1/24 > > > + > > > +if [[ "$1" != "gw-router" ]]; then > > > + ovn-nbctl lrp-set-gateway-chassis rp-public hv1 > > > +fi > > > > > > ovn-nbctl lsp-add sw0 sw0-rp -- set Logical_Switch_Port sw0-rp \ > > > type=router options:router-port=rp-sw0 \ > > > @@ -5317,22 +5318,12 @@ ovn-nbctl lsp-add public public-rp -- set > > Logical_Switch_Port public-rp \ > > > type=router options:router-port=rp-public \ > > > -- lsp-set-addresses public-rp router > > > > > > -ADD_NAMESPACES(sw01) > > > -ADD_VETH(sw01, sw01, br-int, "192.168.1.2/24", "f0:00:00:01:02:03", \ > > > - "192.168.1.1") > > > ovn-nbctl lsp-add sw0 sw01 \ > > > -- lsp-set-addresses sw01 "f0:00:00:01:02:03 192.168.1.2" > > > > > > -ADD_NAMESPACES(sw11) > > > -ADD_VETH(sw11, sw11, br-int, "192.168.2.2/24", "f0:00:00:02:02:03", \ > > > - "192.168.2.1") > > > ovn-nbctl lsp-add sw1 sw11 \ > > > -- lsp-set-addresses sw11 "f0:00:00:02:02:03 192.168.2.2" > > > > > > -ADD_NAMESPACES(server) > > > -ADD_VETH(s1, server, br-ext, "2001:1db8:3333::2/64", > > "f0:00:00:01:02:05", \ > > > - "2001:1db8:3333::1") > > > - > > > OVS_WAIT_UNTIL([test "$(ip netns exec server ip a | grep > > 2001:1db8:3333::2 | grep tentative)" = ""]) > > > OVS_WAIT_UNTIL([test "$(ip netns exec server ip a | grep fe80 | grep > > tentative)" = ""]) > > > > > > @@ -5409,6 +5400,39 @@ OVS_WAIT_WHILE([test "$(ovn-nbctl get > > logical_router_port rp-sw0 ipv6_prefix | c > > > AT_CHECK([ovn-nbctl get logical_router_port rp-sw0 ipv6_prefix | cut > > -c3-16], [0], [dnl > > > [] > > > ]) > > > +} > > > + > > > +ADD_BR([br-int]) > > > +ADD_BR([br-ext]) > > > + > > > +ovs-ofctl add-flow br-ext action=normal > > > +# Set external-ids in br-int needed for ovn-controller > > > +ovs-vsctl \ > > > + -- set Open_vSwitch . external-ids:system-id=hv1 \ > > > + -- set Open_vSwitch . > > external-ids:ovn-remote=unix:$ovs_base/ovn-sb/ovn-sb.sock \ > > > + -- set Open_vSwitch . external-ids:ovn-encap-type=geneve \ > > > + -- set Open_vSwitch . external-ids:ovn-encap-ip=169.0.0.1 \ > > > + -- set bridge br-int fail-mode=secure > > other-config:disable-in-band=true > > > + > > > +# Start ovn-controller > > > +start_daemon ovn-controller > > > + > > > +ADD_NAMESPACES(sw01) > > > +ADD_VETH(sw01, sw01, br-int, "192.168.1.2/24", "f0:00:00:01:02:03", \ > > > + "192.168.1.1") > > > +ADD_NAMESPACES(sw11) > > > +ADD_VETH(sw11, sw11, br-int, "192.168.2.2/24", "f0:00:00:02:02:03", \ > > > + "192.168.2.1") > > > +ADD_NAMESPACES(server) > > > +ADD_VETH(s1, server, br-ext, "2001:1db8:3333::2/64", > > "f0:00:00:01:02:05", \ > > > + "2001:1db8:3333::1") > > > + > > > +# run test for distributed router mode > > > +run_ipv6_pd_test "distributed-router" > > > +# cleanup env > > > +cleanup_env > > > +# run test for gw router mode > > > +run_ipv6_pd_test "gw-router" > > > > > Thanks Lorenzo for the refactor. This is much cleaner than before! > > However, I think it would be better to define a function using "m4_define", > > and still keep two separate test cases (while sharing the same code). That > > way it is easier to tell the problem when one of the test cases fails. > > Sorry that I didn't make it more clear when posting the comment for v1. > > > > Please let me know if you would like to revise again, or I could do it when > > merging. In either case: > > Acked-by: Han Zhou <hzhou@ovn.org> > > Hi Han, > > I can do it, no worries. Do you want to move in m4_define function just > environment configuration or all the test? It appeared to me that the whole test code is common and can be put into a m4_define function, such as: --- # The function takes an arg for test mode: DGP or GW_ROUTER m4_define([TEST_IPV6_PREFIX_DELEGATION], ... if [ $test_mode == "DGP" ]; then ... else: ... fi ... ) # define two test cases that calls the function AT_SETUP([IPv6 prefix delegation - gw router]) TEST_IPV6_PREFIX_DELEGATION([GW_ROUTER]) AT_CLEANUP AT_SETUP([IPv6 prefix delegation - dgp]) TEST_IPV6_PREFIX_DELEGATION([DGP]) AT_CLEANUP --- Does this look ok? Thanks, Han > > Regards, > Lorenzo > > > > > > kill $(pidof ovn-controller) > > > > > > @@ -5423,6 +5447,7 @@ OVS_APP_EXIT_AND_WAIT([NORTHD_TYPE]) > > > > > > as > > > OVS_TRAFFIC_VSWITCHD_STOP(["/.*error receiving.*/d > > > +/failed to query port patch-.*/d > > > /.*terminating with signal 15.*/d"]) > > > AT_CLEANUP > > > ]) > > > -- > > > 2.37.3 > > >
diff --git a/controller/binding.c b/controller/binding.c index 8f6b4b19d..c3d2b2e42 100644 --- a/controller/binding.c +++ b/controller/binding.c @@ -2666,7 +2666,7 @@ consider_patch_port_for_local_datapaths(const struct sbrec_port_binding *pb, get_local_datapath(b_ctx_out->local_datapaths, peer->datapath->tunnel_key); } - if (peer_ld && need_add_patch_peer_to_local( + if (peer_ld && need_add_peer_to_local( b_ctx_in->sbrec_port_binding_by_name, peer, b_ctx_in->chassis_rec)) { add_local_datapath( @@ -2681,7 +2681,7 @@ consider_patch_port_for_local_datapaths(const struct sbrec_port_binding *pb, /* Add the peer datapath to the local datapaths if it's * not present yet. */ - if (need_add_patch_peer_to_local( + if (need_add_peer_to_local( b_ctx_in->sbrec_port_binding_by_name, pb, b_ctx_in->chassis_rec)) { add_local_datapath_peer_port( diff --git a/controller/local_data.c b/controller/local_data.c index 9eee568d1..035f10fff 100644 --- a/controller/local_data.c +++ b/controller/local_data.c @@ -115,14 +115,19 @@ local_datapath_destroy(struct local_datapath *ld) free(ld); } -/* Checks if pb is a patch port and the peer datapath should be added to local - * datapaths. */ +/* Checks if pb is running on local gw router or pb is a patch port + * and the peer datapath should be added to local datapaths. */ bool -need_add_patch_peer_to_local( +need_add_peer_to_local( struct ovsdb_idl_index *sbrec_port_binding_by_name, const struct sbrec_port_binding *pb, const struct sbrec_chassis *chassis) { + /* This port is running on local gw router. */ + if (!strcmp(pb->type, "l3gateway") && pb->chassis == chassis) { + return true; + } + /* If it is not a patch port, no peer to add. */ if (strcmp(pb->type, "patch")) { return false; @@ -571,7 +576,7 @@ add_local_datapath__(struct ovsdb_idl_index *sbrec_datapath_binding_by_key, peer_name); if (peer && peer->datapath) { - if (need_add_patch_peer_to_local( + if (need_add_peer_to_local( sbrec_port_binding_by_name, pb, chassis)) { struct local_datapath *peer_ld = add_local_datapath__(sbrec_datapath_binding_by_key, diff --git a/controller/local_data.h b/controller/local_data.h index d898c8aa5..b5429eb58 100644 --- a/controller/local_data.h +++ b/controller/local_data.h @@ -66,7 +66,7 @@ struct local_datapath *local_datapath_alloc( struct local_datapath *get_local_datapath(const struct hmap *, uint32_t tunnel_key); bool -need_add_patch_peer_to_local( +need_add_peer_to_local( struct ovsdb_idl_index *sbrec_port_binding_by_name, const struct sbrec_port_binding *, const struct sbrec_chassis *); diff --git a/tests/system-ovn.at b/tests/system-ovn.at index d6ecd6eee..a8bb05bd6 100644 --- a/tests/system-ovn.at +++ b/tests/system-ovn.at @@ -5280,22 +5280,20 @@ AT_KEYWORDS([ovn-ipv6-prefix_d]) ovn_start OVS_TRAFFIC_VSWITCHD_START() -ADD_BR([br-int]) -ADD_BR([br-ext]) - -ovs-ofctl add-flow br-ext action=normal -# Set external-ids in br-int needed for ovn-controller -ovs-vsctl \ - -- set Open_vSwitch . external-ids:system-id=hv1 \ - -- set Open_vSwitch . external-ids:ovn-remote=unix:$ovs_base/ovn-sb/ovn-sb.sock \ - -- set Open_vSwitch . external-ids:ovn-encap-type=geneve \ - -- set Open_vSwitch . external-ids:ovn-encap-ip=169.0.0.1 \ - -- set bridge br-int fail-mode=secure other-config:disable-in-band=true - -# Start ovn-controller -start_daemon ovn-controller +cleanup_env () { +ip netns exec server kill $(pidof dhcpd) +ovn-nbctl lr-del R1 +ovn-nbctl ls-del sw0 +ovn-nbctl ls-del sw1 +ovn-nbctl ls-del public +} -ovn-nbctl lr-add R1 +run_ipv6_pd_test () { +if [[ "$1" == "gw-router" ]]; then + ovn-nbctl create Logical_Router name=R1 options:chassis=hv1 +else + ovn-nbctl lr-add R1 +fi ovn-nbctl ls-add sw0 ovn-nbctl ls-add sw1 @@ -5303,8 +5301,11 @@ ovn-nbctl ls-add public ovn-nbctl lrp-add R1 rp-sw0 00:00:01:01:02:03 192.168.1.1/24 ovn-nbctl lrp-add R1 rp-sw1 00:00:03:01:02:03 192.168.2.1/24 -ovn-nbctl lrp-add R1 rp-public 00:00:02:01:02:03 172.16.1.1/24 \ - -- lrp-set-gateway-chassis rp-public hv1 +ovn-nbctl lrp-add R1 rp-public 00:00:02:01:02:03 172.16.1.1/24 + +if [[ "$1" != "gw-router" ]]; then + ovn-nbctl lrp-set-gateway-chassis rp-public hv1 +fi ovn-nbctl lsp-add sw0 sw0-rp -- set Logical_Switch_Port sw0-rp \ type=router options:router-port=rp-sw0 \ @@ -5317,22 +5318,12 @@ ovn-nbctl lsp-add public public-rp -- set Logical_Switch_Port public-rp \ type=router options:router-port=rp-public \ -- lsp-set-addresses public-rp router -ADD_NAMESPACES(sw01) -ADD_VETH(sw01, sw01, br-int, "192.168.1.2/24", "f0:00:00:01:02:03", \ - "192.168.1.1") ovn-nbctl lsp-add sw0 sw01 \ -- lsp-set-addresses sw01 "f0:00:00:01:02:03 192.168.1.2" -ADD_NAMESPACES(sw11) -ADD_VETH(sw11, sw11, br-int, "192.168.2.2/24", "f0:00:00:02:02:03", \ - "192.168.2.1") ovn-nbctl lsp-add sw1 sw11 \ -- lsp-set-addresses sw11 "f0:00:00:02:02:03 192.168.2.2" -ADD_NAMESPACES(server) -ADD_VETH(s1, server, br-ext, "2001:1db8:3333::2/64", "f0:00:00:01:02:05", \ - "2001:1db8:3333::1") - OVS_WAIT_UNTIL([test "$(ip netns exec server ip a | grep 2001:1db8:3333::2 | grep tentative)" = ""]) OVS_WAIT_UNTIL([test "$(ip netns exec server ip a | grep fe80 | grep tentative)" = ""]) @@ -5409,6 +5400,39 @@ OVS_WAIT_WHILE([test "$(ovn-nbctl get logical_router_port rp-sw0 ipv6_prefix | c AT_CHECK([ovn-nbctl get logical_router_port rp-sw0 ipv6_prefix | cut -c3-16], [0], [dnl [] ]) +} + +ADD_BR([br-int]) +ADD_BR([br-ext]) + +ovs-ofctl add-flow br-ext action=normal +# Set external-ids in br-int needed for ovn-controller +ovs-vsctl \ + -- set Open_vSwitch . external-ids:system-id=hv1 \ + -- set Open_vSwitch . external-ids:ovn-remote=unix:$ovs_base/ovn-sb/ovn-sb.sock \ + -- set Open_vSwitch . external-ids:ovn-encap-type=geneve \ + -- set Open_vSwitch . external-ids:ovn-encap-ip=169.0.0.1 \ + -- set bridge br-int fail-mode=secure other-config:disable-in-band=true + +# Start ovn-controller +start_daemon ovn-controller + +ADD_NAMESPACES(sw01) +ADD_VETH(sw01, sw01, br-int, "192.168.1.2/24", "f0:00:00:01:02:03", \ + "192.168.1.1") +ADD_NAMESPACES(sw11) +ADD_VETH(sw11, sw11, br-int, "192.168.2.2/24", "f0:00:00:02:02:03", \ + "192.168.2.1") +ADD_NAMESPACES(server) +ADD_VETH(s1, server, br-ext, "2001:1db8:3333::2/64", "f0:00:00:01:02:05", \ + "2001:1db8:3333::1") + +# run test for distributed router mode +run_ipv6_pd_test "distributed-router" +# cleanup env +cleanup_env +# run test for gw router mode +run_ipv6_pd_test "gw-router" kill $(pidof ovn-controller) @@ -5423,6 +5447,7 @@ OVS_APP_EXIT_AND_WAIT([NORTHD_TYPE]) as OVS_TRAFFIC_VSWITCHD_STOP(["/.*error receiving.*/d +/failed to query port patch-.*/d /.*terminating with signal 15.*/d"]) AT_CLEANUP ])
Fix regression in ipv6 prefix delegation running in gw router mode introduce by the following commit '04292cc2dc2c ("controller: fix potential segmentation violation when removing ports")'. Fixes: 04292cc2dc2c ("controller: fix potential segmentation violation when removing ports") Reoported-at: https://bugzilla.redhat.com/show_bug.cgi?id=2129244 Reoported-at: https://bugzilla.redhat.com/show_bug.cgi?id=2129247 Acked-by: Mark Michelson <mmichels@redhat.com> Signed-off-by: Lorenzo Bianconi <lorenzo.bianconi@redhat.com> --- Changes since v1: - rename need_add_patch_peer_to_local in need_add_peer_to_local - refactor IPv6 PD system-test --- controller/binding.c | 4 +-- controller/local_data.c | 13 ++++--- controller/local_data.h | 2 +- tests/system-ovn.at | 79 +++++++++++++++++++++++++++-------------- 4 files changed, 64 insertions(+), 34 deletions(-)