From e5b87cf915c0061355f9c4cdea0df1fe1c26cd38 Mon Sep 17 00:00:00 2001
From: Dumitru Ceara <dceara@redhat.com>
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 <dcbw@redhat.com>
Reported-at: https://bugzilla.redhat.com/1828637
Signed-off-by: Dumitru Ceara <dceara@redhat.com>
Acked-by: Numan Siddique <numans@ovn.org>
Signed-off-by: Han Zhou <hzhou@ovn.org>
(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