On Wed, Oct 5, 2022 at 6:06 AM Lorenzo Bianconi <[email protected]>
wrote:
>
> > On Tue, Oct 4, 2022 at 11:00 AM Lorenzo Bianconi <
> > [email protected]> 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 <[email protected]>
> > > Signed-off-by: Lorenzo Bianconi <[email protected]>
> > > ---
> > > 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 <[email protected]>
>
> 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
> > >
_______________________________________________
dev mailing list
[email protected]
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
