| Message ID | 20220816105632.220502-1-mheib@redhat.com |
|---|---|
| State | Accepted |
| Headers | show |
| Series | [ovs-dev,v3] binding.c: update ld->peers when lsp type updated | expand |
| Context | Check | Description |
|---|---|---|
| ovsrobot/apply-robot | success | apply and check: success |
| ovsrobot/github-robot-_Build_and_Test | success | github build: passed |
| ovsrobot/github-robot-_ovn-kubernetes | success | github build: passed |
On Tue, Aug 16, 2022 at 12:57 PM Mohammad Heib <mheib@redhat.com> wrote: > The local_datapath->peer_ports list contains peers pointers > to lsp<-->lrp ports that are supposed to be router end ports, > those pointers are added and deleted to the local_datapath->peer_ports > when logical switch port of type router are added or deleted from the > database. > > The deletion and creation of those ports are handled very well when the > LSP type > is a router, but if in any case, the user has changed the LSP type from > router > port to any other LSP type the ld->peer_ports will keep pointing to this > port > and if it was deleted it will keep pointing to invalid memory regions and > that > could lead to invalid memory access in the ovn-controller. > > To solve the above issue this patch will update the > local_dataoath->peer_ports > whenever a lport is updated. > > Reported-at: https://bugzilla.redhat.com/show_bug.cgi?id=2077078 > Co-authored-by: Xavier Simonart <xsimonar@redhat.com> > Signed-off-by: Mohammad Heib <mheib@redhat.com> > Signed-off-by: Xavier Simonart <xsimonar@redhat.com> > Signed-off-by: Mohammad Heib <mheib@redhat.com> > --- > controller/binding.c | 37 +++++++++++++++++++++++++++++++++++++ > tests/ovn.at | 37 +++++++++++++++++++++++++++++++++++++ > 2 files changed, 74 insertions(+) > > diff --git a/controller/binding.c b/controller/binding.c > index 9f5393a92..1221419a9 100644 > --- a/controller/binding.c > +++ b/controller/binding.c > @@ -566,6 +566,39 @@ remove_related_lport(const struct sbrec_port_binding > *pb, > } > } > > +/* > + * Update local_datapath peers when port type changed > + * and remove irrelevant ports from this list. > + */ > +static void > +update_ld_peers(const struct sbrec_port_binding *pb, > + struct hmap *local_datapaths) > +{ > + struct local_datapath *ld = > + get_local_datapath(local_datapaths, pb->datapath->tunnel_key); > + > + if (!ld) { > + return; > + } > + > + /* > + * This will handle cases where the pb type was explicitly > + * changed from router type to any other port type and will > + * remove it from the ld peers list. > + */ > + enum en_lport_type type = get_lport_type(pb); > + int num_peers = ld->n_peer_ports; > + if (type != LP_PATCH) { > + remove_local_datapath_peer_port(pb, ld, local_datapaths); > + if (num_peers != ld->n_peer_ports) { > + static struct vlog_rate_limit rl = VLOG_RATE_LIMIT_INIT(1, 5); > + VLOG_DBG_RL(&rl, > + "removing lport %s from the ld peers list", > + pb->logical_port); > + } > + } > +} > + > static void > delete_active_pb_ras_pd(const struct sbrec_port_binding *pb, > struct shash *ras_pd_map) > @@ -2585,6 +2618,10 @@ handle_updated_port(struct binding_ctx_in *b_ctx_in, > return true; > } > > + if (sbrec_port_binding_is_updated(pb, SBREC_PORT_BINDING_COL_TYPE)) { > + update_ld_peers(pb, b_ctx_out->local_datapaths); > + } > + > update_active_pb_ras_pd(pb, b_ctx_out->local_active_ports_ipv6_pd, > "ipv6_prefix_delegation"); > > diff --git a/tests/ovn.at b/tests/ovn.at > index c8cc8cde4..79eda21d3 100644 > --- a/tests/ovn.at > +++ b/tests/ovn.at > @@ -32404,3 +32404,40 @@ AT_CHECK([test $(ovn-sbctl list fdb | grep -c > "00:00:00:00:10:30") = 0]) > OVN_CLEANUP([hv1]) > AT_CLEANUP > ]) > + > +OVN_FOR_EACH_NORTHD([ > +AT_SETUP([router port type update and then remove]) > +ovn_start > +net_add n1 > + > +sim_add hv1 > +as hv1 > +ovs-vsctl add-br br-phys > +ovn_attach n1 br-phys 192.168.0.1 > +ovs-vsctl -- add-port br-int hv1-vif1 -- \ > + set interface hv1-vif1 external-ids:iface-id=sw0-p1 \ > + options:tx_pcap=hv1/vif1-tx.pcap \ > + options:rxq_pcap=hv1/vif1-rx.pcap \ > + ofport-request=1 > + > +check ovn-nbctl ls-add sw0 > +check ovn-nbctl lr-add ro0 > +check ovn-nbctl lsp-add sw0 sw0-p1 > +check ovn-nbctl lsp-add sw0 lsp > +check ovn-nbctl lsp-set-type lsp router > +check ovn-nbctl lsp-set-options lsp router-port=lrp > +check ovn-nbctl lsp-set-addresses lsp 00:00:00:00:00:1 > +check ovn-nbctl lrp-add ro0 lrp 00:00:00:00:00:1 aef0:0:0:0:0:0:0:1/64 > +check ovn-nbctl set Logical_Router_Port lrp > ipv6_ra_configs:send_periodic=true \ > + -- set Logical_Router_Port lrp ipv6_ra_configs:address_mode=slaac > \ > + -- set Logical_Router_Port lrp ipv6_ra_configs:mtu=1280 \ > + -- set Logical_Router_Port lrp ipv6_ra_configs:max_interval=2 \ > + -- set Logical_Router_Port lrp ipv6_ra_configs:min_interval=1 > +check ovn-nbctl lsp-set-type lsp localnet > +check ovn-nbctl --wait=hv sync > +check ovn-nbctl lsp-del lsp > +check ovn-nbctl lrp-del lrp > +check ovn-nbctl --wait=hv sync > +OVN_CLEANUP([hv1]) > +AT_CLEANUP > +]) > -- > 2.34.1 > > _______________________________________________ > dev mailing list > dev@openvswitch.org > https://mail.openvswitch.org/mailman/listinfo/ovs-dev > > Thanks! Acked-by: Ales Musil <amusil@redhat.com>
Hi Mohammad. The code changes look good, but I'm a bit confused about what the test case is intending to prove. It updates the type of the LSP, but there is no check to ensure that the change was successful, as far as I can tell. On 8/16/22 06:56, Mohammad Heib wrote: > The local_datapath->peer_ports list contains peers pointers > to lsp<-->lrp ports that are supposed to be router end ports, > those pointers are added and deleted to the local_datapath->peer_ports > when logical switch port of type router are added or deleted from the database. > > The deletion and creation of those ports are handled very well when the LSP type > is a router, but if in any case, the user has changed the LSP type from router > port to any other LSP type the ld->peer_ports will keep pointing to this port > and if it was deleted it will keep pointing to invalid memory regions and that > could lead to invalid memory access in the ovn-controller. > > To solve the above issue this patch will update the local_dataoath->peer_ports > whenever a lport is updated. > > Reported-at: https://bugzilla.redhat.com/show_bug.cgi?id=2077078 > Co-authored-by: Xavier Simonart <xsimonar@redhat.com> > Signed-off-by: Mohammad Heib <mheib@redhat.com> > Signed-off-by: Xavier Simonart <xsimonar@redhat.com> > Signed-off-by: Mohammad Heib <mheib@redhat.com> > --- > controller/binding.c | 37 +++++++++++++++++++++++++++++++++++++ > tests/ovn.at | 37 +++++++++++++++++++++++++++++++++++++ > 2 files changed, 74 insertions(+) > > diff --git a/controller/binding.c b/controller/binding.c > index 9f5393a92..1221419a9 100644 > --- a/controller/binding.c > +++ b/controller/binding.c > @@ -566,6 +566,39 @@ remove_related_lport(const struct sbrec_port_binding *pb, > } > } > > +/* > + * Update local_datapath peers when port type changed > + * and remove irrelevant ports from this list. > + */ > +static void > +update_ld_peers(const struct sbrec_port_binding *pb, > + struct hmap *local_datapaths) > +{ > + struct local_datapath *ld = > + get_local_datapath(local_datapaths, pb->datapath->tunnel_key); > + > + if (!ld) { > + return; > + } > + > + /* > + * This will handle cases where the pb type was explicitly > + * changed from router type to any other port type and will > + * remove it from the ld peers list. > + */ > + enum en_lport_type type = get_lport_type(pb); > + int num_peers = ld->n_peer_ports; > + if (type != LP_PATCH) { > + remove_local_datapath_peer_port(pb, ld, local_datapaths); > + if (num_peers != ld->n_peer_ports) { > + static struct vlog_rate_limit rl = VLOG_RATE_LIMIT_INIT(1, 5); > + VLOG_DBG_RL(&rl, > + "removing lport %s from the ld peers list", > + pb->logical_port); > + } > + } > +} > + > static void > delete_active_pb_ras_pd(const struct sbrec_port_binding *pb, > struct shash *ras_pd_map) > @@ -2585,6 +2618,10 @@ handle_updated_port(struct binding_ctx_in *b_ctx_in, > return true; > } > > + if (sbrec_port_binding_is_updated(pb, SBREC_PORT_BINDING_COL_TYPE)) { > + update_ld_peers(pb, b_ctx_out->local_datapaths); > + } > + > update_active_pb_ras_pd(pb, b_ctx_out->local_active_ports_ipv6_pd, > "ipv6_prefix_delegation"); > > diff --git a/tests/ovn.at b/tests/ovn.at > index c8cc8cde4..79eda21d3 100644 > --- a/tests/ovn.at > +++ b/tests/ovn.at > @@ -32404,3 +32404,40 @@ AT_CHECK([test $(ovn-sbctl list fdb | grep -c "00:00:00:00:10:30") = 0]) > OVN_CLEANUP([hv1]) > AT_CLEANUP > ]) > + > +OVN_FOR_EACH_NORTHD([ > +AT_SETUP([router port type update and then remove]) > +ovn_start > +net_add n1 > + > +sim_add hv1 > +as hv1 > +ovs-vsctl add-br br-phys > +ovn_attach n1 br-phys 192.168.0.1 > +ovs-vsctl -- add-port br-int hv1-vif1 -- \ > + set interface hv1-vif1 external-ids:iface-id=sw0-p1 \ > + options:tx_pcap=hv1/vif1-tx.pcap \ > + options:rxq_pcap=hv1/vif1-rx.pcap \ > + ofport-request=1 > + > +check ovn-nbctl ls-add sw0 > +check ovn-nbctl lr-add ro0 > +check ovn-nbctl lsp-add sw0 sw0-p1 > +check ovn-nbctl lsp-add sw0 lsp > +check ovn-nbctl lsp-set-type lsp router > +check ovn-nbctl lsp-set-options lsp router-port=lrp > +check ovn-nbctl lsp-set-addresses lsp 00:00:00:00:00:1 > +check ovn-nbctl lrp-add ro0 lrp 00:00:00:00:00:1 aef0:0:0:0:0:0:0:1/64 > +check ovn-nbctl set Logical_Router_Port lrp ipv6_ra_configs:send_periodic=true \ > + -- set Logical_Router_Port lrp ipv6_ra_configs:address_mode=slaac \ > + -- set Logical_Router_Port lrp ipv6_ra_configs:mtu=1280 \ > + -- set Logical_Router_Port lrp ipv6_ra_configs:max_interval=2 \ > + -- set Logical_Router_Port lrp ipv6_ra_configs:min_interval=1 > +check ovn-nbctl lsp-set-type lsp localnet > +check ovn-nbctl --wait=hv sync > +check ovn-nbctl lsp-del lsp > +check ovn-nbctl lrp-del lrp > +check ovn-nbctl --wait=hv sync > +OVN_CLEANUP([hv1]) > +AT_CLEANUP > +])
Hi Mark, Thank you for reviewing the patch. actually, the test here is less interested in the type update but more in the LSP deletion from the ld->peer_ports list i don't have a way to see if the ld->n_peer_ports updated properly from the tests units when we change the port type, but i know that if something wrong happens in the ovn-controller and ld->peer_ports keeps pointing to the deleted LSP( which is what this patch comes to fix ) the command* "check ovn-nbctl lrp-del lrp"* will cause some invalid memory access and that will lead to a core dump in the ovn-controller and then the test case will fail with dump in the ovn-controller logs. i know it is a bad way to test it like that but as i mentioned i don't have a way to see the content of ld->peer_ports from the test :(. see comments in the tests below. On Thu, Aug 18, 2022 at 9:18 PM Mark Michelson <mmichels@redhat.com> wrote: > Hi Mohammad. The code changes look good, but I'm a bit confused about > what the test case is intending to prove. It updates the type of the > LSP, but there is no check to ensure that the change was successful, as > far as I can tell. > > > On 8/16/22 06:56, Mohammad Heib wrote: > > The local_datapath->peer_ports list contains peers pointers > > to lsp<-->lrp ports that are supposed to be router end ports, > > those pointers are added and deleted to the local_datapath->peer_ports > > when logical switch port of type router are added or deleted from the > database. > > > > The deletion and creation of those ports are handled very well when the > LSP type > > is a router, but if in any case, the user has changed the LSP type from > router > > port to any other LSP type the ld->peer_ports will keep pointing to this > port > > and if it was deleted it will keep pointing to invalid memory regions > and that > > could lead to invalid memory access in the ovn-controller. > > > > To solve the above issue this patch will update the > local_dataoath->peer_ports > > whenever a lport is updated. > > > > Reported-at: https://bugzilla.redhat.com/show_bug.cgi?id=2077078 > > Co-authored-by: Xavier Simonart <xsimonar@redhat.com> > > Signed-off-by: Mohammad Heib <mheib@redhat.com> > > Signed-off-by: Xavier Simonart <xsimonar@redhat.com> > > Signed-off-by: Mohammad Heib <mheib@redhat.com> > > --- > > controller/binding.c | 37 +++++++++++++++++++++++++++++++++++++ > > tests/ovn.at | 37 +++++++++++++++++++++++++++++++++++++ > > 2 files changed, 74 insertions(+) > > > > diff --git a/controller/binding.c b/controller/binding.c > > index 9f5393a92..1221419a9 100644 > > --- a/controller/binding.c > > +++ b/controller/binding.c > > @@ -566,6 +566,39 @@ remove_related_lport(const struct > sbrec_port_binding *pb, > > } > > } > > > > +/* > > + * Update local_datapath peers when port type changed > > + * and remove irrelevant ports from this list. > > + */ > > +static void > > +update_ld_peers(const struct sbrec_port_binding *pb, > > + struct hmap *local_datapaths) > > +{ > > + struct local_datapath *ld = > > + get_local_datapath(local_datapaths, pb->datapath->tunnel_key); > > + > > + if (!ld) { > > + return; > > + } > > + > > + /* > > + * This will handle cases where the pb type was explicitly > > + * changed from router type to any other port type and will > > + * remove it from the ld peers list. > > + */ > > + enum en_lport_type type = get_lport_type(pb); > > + int num_peers = ld->n_peer_ports; > > + if (type != LP_PATCH) { > > + remove_local_datapath_peer_port(pb, ld, local_datapaths); > > + if (num_peers != ld->n_peer_ports) { > > + static struct vlog_rate_limit rl = VLOG_RATE_LIMIT_INIT(1, > 5); > > + VLOG_DBG_RL(&rl, > > + "removing lport %s from the ld peers list", > > + pb->logical_port); > > + } > > + } > > +} > > + > > static void > > delete_active_pb_ras_pd(const struct sbrec_port_binding *pb, > > struct shash *ras_pd_map) > > @@ -2585,6 +2618,10 @@ handle_updated_port(struct binding_ctx_in > *b_ctx_in, > > return true; > > } > > > > + if (sbrec_port_binding_is_updated(pb, SBREC_PORT_BINDING_COL_TYPE)) > { > > + update_ld_peers(pb, b_ctx_out->local_datapaths); > > + } > > + > > update_active_pb_ras_pd(pb, b_ctx_out->local_active_ports_ipv6_pd, > > "ipv6_prefix_delegation"); > > > > diff --git a/tests/ovn.at b/tests/ovn.at > > index c8cc8cde4..79eda21d3 100644 > > --- a/tests/ovn.at > > +++ b/tests/ovn.at > > @@ -32404,3 +32404,40 @@ AT_CHECK([test $(ovn-sbctl list fdb | grep -c > "00:00:00:00:10:30") = 0]) > > OVN_CLEANUP([hv1]) > > AT_CLEANUP > > ]) > > + > > +OVN_FOR_EACH_NORTHD([ > > +AT_SETUP([router port type update and then remove]) > > +ovn_start > > +net_add n1 > > + > > +sim_add hv1 > > +as hv1 > > +ovs-vsctl add-br br-phys > > +ovn_attach n1 br-phys 192.168.0.1 > > +ovs-vsctl -- add-port br-int hv1-vif1 -- \ > > + set interface hv1-vif1 external-ids:iface-id=sw0-p1 \ > > + options:tx_pcap=hv1/vif1-tx.pcap \ > > + options:rxq_pcap=hv1/vif1-rx.pcap \ > > + ofport-request=1 > > + > > +check ovn-nbctl ls-add sw0 > > +check ovn-nbctl lr-add ro0 > > +check ovn-nbctl lsp-add sw0 sw0-p1 > > +check ovn-nbctl lsp-add sw0 lsp > > +check ovn-nbctl lsp-set-type lsp router > > +check ovn-nbctl lsp-set-options lsp router-port=lrp > > +check ovn-nbctl lsp-set-addresses lsp 00:00:00:00:00:1 > > +check ovn-nbctl lrp-add ro0 lrp 00:00:00:00:00:1 aef0:0:0:0:0:0:0:1/64 > > +check ovn-nbctl set Logical_Router_Port lrp > ipv6_ra_configs:send_periodic=true \ > > + -- set Logical_Router_Port lrp > ipv6_ra_configs:address_mode=slaac \ > > + -- set Logical_Router_Port lrp ipv6_ra_configs:mtu=1280 \ > > + -- set Logical_Router_Port lrp ipv6_ra_configs:max_interval=2 \ > > + -- set Logical_Router_Port lrp ipv6_ra_configs:min_interval=1 > > > +check ovn-nbctl lsp-set-type lsp localnet <-------------- > this will delete the lsp from the ld->peer_ports and will delete its peer > lrp from the same list > > +check ovn-nbctl --wait=hv sync > > +check ovn-nbctl lsp-del lsp <--------- > normall LSP deletion ( without this patch the ld->peer_ports will keeps > pointing to deleted LSP invalid memory area) > > +check ovn-nbctl lrp-del lrp <--------- > noremal LRP deletions > > +check ovn-nbctl --wait=hv sync > > +OVN_CLEANUP([hv1]) > > +AT_CLEANUP > > +]) > > thanks
OK, thanks for the explanation, Mohammad. I added the following comment
to the test to explain what is being tested.
# The below commands change the LSP from "router" type in order to
ensure that
# no incorrect memory accesses occur. The test passes because there
is no crash
# in ovn-controller.
I added my Ack and pushed the change to branch-22.03, branch-22.06, and
main.
On 8/18/22 14:58, Mohammad Heib wrote:
> Hi Mark,
> Thank you for reviewing the patch.
> actually, the test here is less interested in the type update but more
> in the LSP deletion from theld->peer_ports list
> i don't have a way to see if theld->n_peer_portsupdated properly from
> the tests units when we change the port type, but i know that
> if something wrong happens in the ovn-controller and ld->peer_ports
> keeps pointing to the deleted LSP( which is what this patch
> comes to fix ) the command*"check ovn-nbctl lrp-del lrp"* will cause
> some invalid memory access and that will lead to a core dump in the
> ovn-controller
> and then the test case will fail with dump in the ovn-controller logs.
> i know it is a bad way to test it like that but as i mentioned i don't
> have a way to see the content of ld->peer_ports from the test :(.
>
> see comments in the tests below.
>
> On Thu, Aug 18, 2022 at 9:18 PM Mark Michelson <mmichels@redhat.com
> <mailto:mmichels@redhat.com>> wrote:
>
> Hi Mohammad. The code changes look good, but I'm a bit confused about
> what the test case is intending to prove. It updates the type of the
> LSP, but there is no check to ensure that the change was successful, as
> far as I can tell.
>
>
> On 8/16/22 06:56, Mohammad Heib wrote:
> > The local_datapath->peer_ports list contains peers pointers
> > to lsp<-->lrp ports that are supposed to be router end ports,
> > those pointers are added and deleted to the
> local_datapath->peer_ports
> > when logical switch port of type router are added or deleted from
> the database.
> >
> > The deletion and creation of those ports are handled very well
> when the LSP type
> > is a router, but if in any case, the user has changed the LSP
> type from router
> > port to any other LSP type the ld->peer_ports will keep pointing
> to this port
> > and if it was deleted it will keep pointing to invalid memory
> regions and that
> > could lead to invalid memory access in the ovn-controller.
> >
> > To solve the above issue this patch will update the
> local_dataoath->peer_ports
> > whenever a lport is updated.
> >
> > Reported-at: https://bugzilla.redhat.com/show_bug.cgi?id=2077078
> <https://bugzilla.redhat.com/show_bug.cgi?id=2077078>
> > Co-authored-by: Xavier Simonart <xsimonar@redhat.com
> <mailto:xsimonar@redhat.com>>
> > Signed-off-by: Mohammad Heib <mheib@redhat.com
> <mailto:mheib@redhat.com>>
> > Signed-off-by: Xavier Simonart <xsimonar@redhat.com
> <mailto:xsimonar@redhat.com>>
> > Signed-off-by: Mohammad Heib <mheib@redhat.com
> <mailto:mheib@redhat.com>>
> > ---
> > controller/binding.c | 37 +++++++++++++++++++++++++++++++++++++
> > tests/ovn.at <http://ovn.at> | 37
> +++++++++++++++++++++++++++++++++++++
> > 2 files changed, 74 insertions(+)
> >
> > diff --git a/controller/binding.c b/controller/binding.c
> > index 9f5393a92..1221419a9 100644
> > --- a/controller/binding.c
> > +++ b/controller/binding.c
> > @@ -566,6 +566,39 @@ remove_related_lport(const struct
> sbrec_port_binding *pb,
> > }
> > }
> >
> > +/*
> > + * Update local_datapath peers when port type changed
> > + * and remove irrelevant ports from this list.
> > + */
> > +static void
> > +update_ld_peers(const struct sbrec_port_binding *pb,
> > + struct hmap *local_datapaths)
> > +{
> > + struct local_datapath *ld =
> > + get_local_datapath(local_datapaths,
> pb->datapath->tunnel_key);
> > +
> > + if (!ld) {
> > + return;
> > + }
> > +
> > + /*
> > + * This will handle cases where the pb type was explicitly
> > + * changed from router type to any other port type and will
> > + * remove it from the ld peers list.
> > + */
> > + enum en_lport_type type = get_lport_type(pb);
> > + int num_peers = ld->n_peer_ports;
> > + if (type != LP_PATCH) {
> > + remove_local_datapath_peer_port(pb, ld, local_datapaths);
> > + if (num_peers != ld->n_peer_ports) {
> > + static struct vlog_rate_limit rl =
> VLOG_RATE_LIMIT_INIT(1, 5);
> > + VLOG_DBG_RL(&rl,
> > + "removing lport %s from the ld peers list",
> > + pb->logical_port);
> > + }
> > + }
> > +}
> > +
> > static void
> > delete_active_pb_ras_pd(const struct sbrec_port_binding *pb,
> > struct shash *ras_pd_map)
> > @@ -2585,6 +2618,10 @@ handle_updated_port(struct binding_ctx_in
> *b_ctx_in,
> > return true;
> > }
> >
> > + if (sbrec_port_binding_is_updated(pb,
> SBREC_PORT_BINDING_COL_TYPE)) {
> > + update_ld_peers(pb, b_ctx_out->local_datapaths);
> > + }
> > +
> > update_active_pb_ras_pd(pb,
> b_ctx_out->local_active_ports_ipv6_pd,
> > "ipv6_prefix_delegation");
> >
> > diff --git a/tests/ovn.at <http://ovn.at> b/tests/ovn.at
> <http://ovn.at>
> > index c8cc8cde4..79eda21d3 100644
> > --- a/tests/ovn.at <http://ovn.at>
> > +++ b/tests/ovn.at <http://ovn.at>
> > @@ -32404,3 +32404,40 @@ AT_CHECK([test $(ovn-sbctl list fdb |
> grep -c "00:00:00:00:10:30") = 0])
> > OVN_CLEANUP([hv1])
> > AT_CLEANUP
> > ])
> > +
> > +OVN_FOR_EACH_NORTHD([
> > +AT_SETUP([router port type update and then remove])
> > +ovn_start
> > +net_add n1
> > +
> > +sim_add hv1
> > +as hv1
> > +ovs-vsctl add-br br-phys
> > +ovn_attach n1 br-phys 192.168.0.1
> > +ovs-vsctl -- add-port br-int hv1-vif1 -- \
> > + set interface hv1-vif1 external-ids:iface-id=sw0-p1 \
> > + options:tx_pcap=hv1/vif1-tx.pcap \
> > + options:rxq_pcap=hv1/vif1-rx.pcap \
> > + ofport-request=1
> > +
> > +check ovn-nbctl ls-add sw0
> > +check ovn-nbctl lr-add ro0
> > +check ovn-nbctl lsp-add sw0 sw0-p1
> > +check ovn-nbctl lsp-add sw0 lsp
> > +check ovn-nbctl lsp-set-type lsp router
> > +check ovn-nbctl lsp-set-options lsp router-port=lrp
> > +check ovn-nbctl lsp-set-addresses lsp 00:00:00:00:00:1
> > +check ovn-nbctl lrp-add ro0 lrp 00:00:00:00:00:1
> aef0:0:0:0:0:0:0:1/64
> > +check ovn-nbctl set Logical_Router_Port lrp
> ipv6_ra_configs:send_periodic=true \
> > + -- set Logical_Router_Port lrp
> ipv6_ra_configs:address_mode=slaac \
> > + -- set Logical_Router_Port lrp ipv6_ra_configs:mtu=1280 \
> > + -- set Logical_Router_Port lrp
> ipv6_ra_configs:max_interval=2 \
> > + -- set Logical_Router_Port lrp
> ipv6_ra_configs:min_interval=1
>
> > +check ovn-nbctl lsp-set-type lsp localnet
> <-------------- this will delete the lsp from the ld->peer_ports and
> will delete its peer lrp from the same list
>
> > +check ovn-nbctl --wait=hv sync
> > +check ovn-nbctl lsp-del lsp
> <--------- normall LSP deletion ( without this patch
> theld->peer_ports will keeps pointing to deleted LSP invalid memory
> area)
>
> > +check ovn-nbctl lrp-del lrp
> <--------- noremal LRP deletions
> > +check ovn-nbctl --wait=hv sync
> > +OVN_CLEANUP([hv1])
> > +AT_CLEANUP
> > +])
>
> thanks
diff --git a/controller/binding.c b/controller/binding.c index 9f5393a92..1221419a9 100644 --- a/controller/binding.c +++ b/controller/binding.c @@ -566,6 +566,39 @@ remove_related_lport(const struct sbrec_port_binding *pb, } } +/* + * Update local_datapath peers when port type changed + * and remove irrelevant ports from this list. + */ +static void +update_ld_peers(const struct sbrec_port_binding *pb, + struct hmap *local_datapaths) +{ + struct local_datapath *ld = + get_local_datapath(local_datapaths, pb->datapath->tunnel_key); + + if (!ld) { + return; + } + + /* + * This will handle cases where the pb type was explicitly + * changed from router type to any other port type and will + * remove it from the ld peers list. + */ + enum en_lport_type type = get_lport_type(pb); + int num_peers = ld->n_peer_ports; + if (type != LP_PATCH) { + remove_local_datapath_peer_port(pb, ld, local_datapaths); + if (num_peers != ld->n_peer_ports) { + static struct vlog_rate_limit rl = VLOG_RATE_LIMIT_INIT(1, 5); + VLOG_DBG_RL(&rl, + "removing lport %s from the ld peers list", + pb->logical_port); + } + } +} + static void delete_active_pb_ras_pd(const struct sbrec_port_binding *pb, struct shash *ras_pd_map) @@ -2585,6 +2618,10 @@ handle_updated_port(struct binding_ctx_in *b_ctx_in, return true; } + if (sbrec_port_binding_is_updated(pb, SBREC_PORT_BINDING_COL_TYPE)) { + update_ld_peers(pb, b_ctx_out->local_datapaths); + } + update_active_pb_ras_pd(pb, b_ctx_out->local_active_ports_ipv6_pd, "ipv6_prefix_delegation"); diff --git a/tests/ovn.at b/tests/ovn.at index c8cc8cde4..79eda21d3 100644 --- a/tests/ovn.at +++ b/tests/ovn.at @@ -32404,3 +32404,40 @@ AT_CHECK([test $(ovn-sbctl list fdb | grep -c "00:00:00:00:10:30") = 0]) OVN_CLEANUP([hv1]) AT_CLEANUP ]) + +OVN_FOR_EACH_NORTHD([ +AT_SETUP([router port type update and then remove]) +ovn_start +net_add n1 + +sim_add hv1 +as hv1 +ovs-vsctl add-br br-phys +ovn_attach n1 br-phys 192.168.0.1 +ovs-vsctl -- add-port br-int hv1-vif1 -- \ + set interface hv1-vif1 external-ids:iface-id=sw0-p1 \ + options:tx_pcap=hv1/vif1-tx.pcap \ + options:rxq_pcap=hv1/vif1-rx.pcap \ + ofport-request=1 + +check ovn-nbctl ls-add sw0 +check ovn-nbctl lr-add ro0 +check ovn-nbctl lsp-add sw0 sw0-p1 +check ovn-nbctl lsp-add sw0 lsp +check ovn-nbctl lsp-set-type lsp router +check ovn-nbctl lsp-set-options lsp router-port=lrp +check ovn-nbctl lsp-set-addresses lsp 00:00:00:00:00:1 +check ovn-nbctl lrp-add ro0 lrp 00:00:00:00:00:1 aef0:0:0:0:0:0:0:1/64 +check ovn-nbctl set Logical_Router_Port lrp ipv6_ra_configs:send_periodic=true \ + -- set Logical_Router_Port lrp ipv6_ra_configs:address_mode=slaac \ + -- set Logical_Router_Port lrp ipv6_ra_configs:mtu=1280 \ + -- set Logical_Router_Port lrp ipv6_ra_configs:max_interval=2 \ + -- set Logical_Router_Port lrp ipv6_ra_configs:min_interval=1 +check ovn-nbctl lsp-set-type lsp localnet +check ovn-nbctl --wait=hv sync +check ovn-nbctl lsp-del lsp +check ovn-nbctl lrp-del lrp +check ovn-nbctl --wait=hv sync +OVN_CLEANUP([hv1]) +AT_CLEANUP +])