On Fri, May 1, 2020 at 12:02 AM Dumitru Ceara <[email protected]> wrote:
> When purging stale SB Datapath_Binding records ovn-northd doesn't > properly clean records from other tables that might refer the > datapaths being deleted. > > One way to reproduce the issue is: > $ ovn-nbctl lr-add lr > $ ovn-nbctl lrp-add lr p 00:00:00:00:00:01 1.1.1.1/24 > $ ovn-nbctl --wait=sb sync > $ dp=$(ovn-sbctl --bare --columns _uuid list datapath .) > $ ovn-sbctl create mac_binding logical_port="p" ip="1.1.1.2" datapath="$dp" > $ ovn-nbctl lrp-del p -- lr-del lr -- \ > lr-add lr -- lrp-add lr p 00:00:00:00:00:01 1.1.1.1/24 > > Reported-by: Dan Williams <[email protected]> > Reported-at: https://bugzilla.redhat.com/1828637 > Signed-off-by: Dumitru Ceara <[email protected]> > Acked-by: Numan Siddique <[email protected]> Thanks Numan > --- > northd/ovn-northd.c | 45 ++++++++++++++++++++++++++++++++------------- > tests/ovn-northd.at | 24 ++++++++++++++++++++++++ > 2 files changed, 56 insertions(+), 13 deletions(-) > > diff --git a/northd/ovn-northd.c b/northd/ovn-northd.c > index e31794c..de59452 100644 > --- a/northd/ovn-northd.c > +++ b/northd/ovn-northd.c > @@ -638,6 +638,12 @@ ovn_datapath_find(struct hmap *datapaths, const > struct uuid *uuid) > return NULL; > } > > +static bool > +ovn_datapath_is_stale(const struct ovn_datapath *od) > +{ > + return !od->nbr && !od->nbs; > +} > + > static struct ovn_datapath * > ovn_datapath_from_sbrec(struct hmap *datapaths, > const struct sbrec_datapath_binding *sb) > @@ -3075,11 +3081,16 @@ ovn_port_update_sbrec(struct northd_context *ctx, > /* Remove mac_binding entries that refer to logical_ports which are > * deleted. */ > static void > -cleanup_mac_bindings(struct northd_context *ctx, struct hmap *ports) > +cleanup_mac_bindings(struct northd_context *ctx, struct hmap *datapaths, > + struct hmap *ports) > { > const struct sbrec_mac_binding *b, *n; > SBREC_MAC_BINDING_FOR_EACH_SAFE (b, n, ctx->ovnsb_idl) { > - if (!ovn_port_find(ports, b->logical_port)) { > + const struct ovn_datapath *od = > + ovn_datapath_from_sbrec(datapaths, b->datapath); > + > + if (!od || ovn_datapath_is_stale(od) || > + !ovn_port_find(ports, b->logical_port)) { > sbrec_mac_binding_delete(b); > } > } > @@ -3447,6 +3458,9 @@ build_ports(struct northd_context *ctx, > join_logical_ports(ctx, datapaths, ports, &chassis_qdisc_queues, > &tag_alloc_table, &sb_only, &nb_only, &both); > > + /* Purge stale Mac_Bindings if ports are deleted. */ > + bool remove_mac_bindings = !ovs_list_is_empty(&sb_only); > + > struct ovn_port *op, *next; > /* For logical ports that are in both databases, index the in-use > * tunnel_keys. */ > @@ -3461,6 +3475,12 @@ build_ports(struct northd_context *ctx, > * For logical ports that are in NB database, do any tag allocation > * needed. */ > LIST_FOR_EACH_SAFE (op, next, list, &both) { > + /* When reusing stale Port_Bindings, make sure that stale > + * Mac_Bindings are purged. > + */ > + if (op->od->sb != op->sb->datapath) { > + remove_mac_bindings = true; > + } > if (op->nbsp) { > tag_alloc_create_new_tag(&tag_alloc_table, op->nbsp); > } > @@ -3496,19 +3516,15 @@ build_ports(struct northd_context *ctx, > sbrec_port_binding_set_tunnel_key(op->sb, tunnel_key); > } > > - bool remove_mac_bindings = false; > - if (!ovs_list_is_empty(&sb_only)) { > - remove_mac_bindings = true; > - } > - > /* Delete southbound records without northbound matches. */ > LIST_FOR_EACH_SAFE(op, next, list, &sb_only) { > ovs_list_remove(&op->list); > sbrec_port_binding_delete(op->sb); > ovn_port_destroy(ports, op); > } > + > if (remove_mac_bindings) { > - cleanup_mac_bindings(ctx, ports); > + cleanup_mac_bindings(ctx, datapaths, ports); > } > > tag_alloc_destroy(&tag_alloc_table); > @@ -10293,7 +10309,8 @@ build_lflows(struct northd_context *ctx, struct > hmap *datapaths, > SBREC_LOGICAL_FLOW_FOR_EACH_SAFE (sbflow, next_sbflow, > ctx->ovnsb_idl) { > struct ovn_datapath *od > = ovn_datapath_from_sbrec(datapaths, > sbflow->logical_datapath); > - if (!od) { > + > + if (!od || ovn_datapath_is_stale(od)) { > sbrec_logical_flow_delete(sbflow); > continue; > } > @@ -10353,7 +10370,8 @@ build_lflows(struct northd_context *ctx, struct > hmap *datapaths, > SBREC_MULTICAST_GROUP_FOR_EACH_SAFE (sbmc, next_sbmc, ctx->ovnsb_idl) > { > struct ovn_datapath *od = ovn_datapath_from_sbrec(datapaths, > sbmc->datapath); > - if (!od) { > + > + if (!od || ovn_datapath_is_stale(od)) { > sbrec_multicast_group_delete(sbmc); > continue; > } > @@ -10835,8 +10853,8 @@ build_ip_mcast(struct northd_context *ctx, struct > hmap *datapaths) > const struct sbrec_ip_multicast *sb, *sb_next; > > SBREC_IP_MULTICAST_FOR_EACH_SAFE (sb, sb_next, ctx->ovnsb_idl) { > - if (!sb->datapath || > - !ovn_datapath_from_sbrec(datapaths, sb->datapath)) { > + od = ovn_datapath_from_sbrec(datapaths, sb->datapath); > + if (!od || ovn_datapath_is_stale(od)) { > sbrec_ip_multicast_delete(sb); > } > } > @@ -10905,7 +10923,8 @@ build_mcast_groups(struct northd_context *ctx, > /* If the datapath value is stale, purge the group. */ > struct ovn_datapath *od = > ovn_datapath_from_sbrec(datapaths, sb_igmp->datapath); > - if (!od) { > + > + if (!od || ovn_datapath_is_stale(od)) { > sbrec_igmp_group_delete(sb_igmp); > continue; > } > diff --git a/tests/ovn-northd.at b/tests/ovn-northd.at > index 8cc3f70..a4469c7 100644 > --- a/tests/ovn-northd.at > +++ b/tests/ovn-northd.at > @@ -1390,3 +1390,27 @@ AT_CHECK([test 0 = $(ovn-sbctl dump-flows lr0 | > grep lr_in_unsnat | \ > grep "ip4 && ip4.dst == 192.168.2.6 && tcp && tcp.dst == 8080" -c) ]) > > AT_CLEANUP > + > +AT_SETUP([ovn -- check reconcile stale Datapath_Binding]) > +ovn_start > + > +ovn-nbctl lr-add lr > +ovn-nbctl lrp-add lr p 00:00:00:00:00:01 1.1.1.1/24 > + > +AT_CHECK([ovn-nbctl --wait=sb sync], [0]) > + > +# Create a MAC_Binding referring the router datapath. > +dp=$(ovn-sbctl --bare --columns _uuid list datapath .) > +ovn-sbctl create mac_binding logical_port="p" ip="1.1.1.2" datapath="$dp" > + > +ovn-nbctl lrp-del p -- lr-del lr -- \ > + lr-add lr -- lrp-add lr p 00:00:00:00:00:01 1.1.1.1/24 > +AT_CHECK([ovn-nbctl --wait=sb sync], [0]) > + > +AT_CHECK([test 1 = $(ovn-sbctl --columns _uuid list Datapath_Binding | wc > -l)]) > + > +nb_uuid=$(ovn-sbctl get Datapath_Binding . external_ids:logical-router) > +lr_uuid=$(ovn-nbctl --columns _uuid list Logical_Router .) > +AT_CHECK[test ${nb_uuid} = ${lr_uuid}] > + > +AT_CLEANUP > > _______________________________________________ > dev mailing list > [email protected] > https://mail.openvswitch.org/mailman/listinfo/ovs-dev > > _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
