From e5b87cf915c0061355f9c4cdea0df1fe1c26cd38 Mon Sep 17 00:00:00 2001 From: Dumitru Ceara Date: Thu, 30 Apr 2020 20:32:17 +0200 Subject: [PATCH 1/2] ovn-northd: Clear SB records depending on stale datapaths. 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 Reported-at: https://bugzilla.redhat.com/1828637 Signed-off-by: Dumitru Ceara Acked-by: Numan Siddique Signed-off-by: Han Zhou (cherry picked from upstream commit 6856adc616a7181723ce5201110cc95de1aba92b) Change-Id: I1cbcb5fc34927368e6655420126b2492c4fce9df --- 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 5ada3ae..5e649d0 100644 --- a/northd/ovn-northd.c +++ b/northd/ovn-northd.c @@ -634,6 +634,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) @@ -3067,11 +3073,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); } } @@ -3439,6 +3450,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. */ @@ -3453,6 +3467,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); } @@ -3488,19 +3508,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); @@ -10258,7 +10274,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; } @@ -10318,7 +10335,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; } @@ -10800,8 +10818,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); } } @@ -10870,7 +10888,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 d127152..94f892b 100644 --- a/tests/ovn-northd.at +++ b/tests/ovn-northd.at @@ -1326,3 +1326,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 -- 1.8.3.1