From 3de7959b9018f53abd06320bc7f1a43ec216db7e Mon Sep 17 00:00:00 2001
From: Dumitru Ceara <dceara@redhat.com>
Date: Wed, 3 Feb 2021 20:36:41 +0100
Subject: [PATCH 2/4] binding: Set Port_Binding.up only if supported.
The supported upgrade procedure is to always upgrade ovn-controllers
first and OVN DBs and ovn-northd later. This leads however to the
situation when ovn-controller might try to set the Port_Binding.up field
while the Southbound DB isn't yet aware of this field which would
trigger transaction failures and control plane/data plane outages.
To avoid such situations ovn-controller only sets the Port_Binding.up
field if it was explicitly set to 'false'. This ensures that the SB
database was already upgraded.
On the ovn-northd side, as soon as ovn-northd is upgraded it will update
all existing Port_Bindings and explicitly set 'Port_Binding.up' to
false, implicitly notifying ovn-controller that it is safe to write to
the field.
Reported-by: Numan Siddique <numans@ovn.org>
Fixes: 4d3cb42b076b ("binding: Set Logical_Switch_Port.up when all OVS flows are installed.")
Signed-off-by: Dumitru Ceara <dceara@redhat.com>
Signed-off-by: Numan Siddique <numans@ovn.org>
(cherry picked from upstream commit 8b45fc9b2269df68566e47eee9cdd5f043b595d3)
Change-Id: I6fb8b59419466bbe43f0d993966d8316320c6327
---
controller-vtep/binding.c | 6 ++++--
controller/binding.c | 33 ++++++++++++++++++++++----------
northd/ovn-northd.c | 8 ++++++++
tests/ovn.at | 48 ++++++++++++++++++++++++++++++++++++++++++++++-
4 files changed, 82 insertions(+), 13 deletions(-)
diff --git a/controller-vtep/binding.c b/controller-vtep/binding.c
index d28a598..01d5a16 100644
--- a/controller-vtep/binding.c
+++ b/controller-vtep/binding.c
@@ -110,9 +110,11 @@ update_pb_chassis(const struct sbrec_port_binding *port_binding_rec,
chassis_rec->name);
}
- bool up = true;
sbrec_port_binding_set_chassis(port_binding_rec, chassis_rec);
- sbrec_port_binding_set_up(port_binding_rec, &up, 1);
+ if (port_binding_rec->n_up) {
+ bool up = true;
+ sbrec_port_binding_set_up(port_binding_rec, &up, 1);
+ }
}
}
diff --git a/controller/binding.c b/controller/binding.c
index 353debe..efaa109 100644
--- a/controller/binding.c
+++ b/controller/binding.c
@@ -870,6 +870,11 @@ get_lport_type(const struct sbrec_port_binding *pb)
* container and virtual ports).
* Otherwise request a notification to be sent when the OVS flows
* corresponding to 'pb' have been installed.
+ *
+ * Note:
+ * Updates (directly or through a notification) the 'pb->up' field only if
+ * it's explicitly set to 'false'.
+ * This is to ensure compatibility with older versions of ovn-northd.
*/
static void
claimed_lport_set_up(const struct sbrec_port_binding *pb,
@@ -885,7 +890,7 @@ claimed_lport_set_up(const struct sbrec_port_binding *pb,
return;
}
- if (pb->chassis != chassis_rec) {
+ if (pb->chassis != chassis_rec || (pb->n_up && !pb->up[0])) {
binding_iface_bound_add(pb->logical_port);
}
}
@@ -973,7 +978,10 @@ release_lport(const struct sbrec_port_binding *pb, bool sb_readonly,
sbrec_port_binding_set_virtual_parent(pb, NULL);
}
- sbrec_port_binding_set_up(pb, NULL, 0);
+ if (pb->n_up) {
+ bool up = false;
+ sbrec_port_binding_set_up(pb, &up, 1);
+ }
update_lport_tracking(pb, tracked_datapaths);
binding_iface_released_add(pb->logical_port);
VLOG_INFO("Releasing lport %s from this chassis.", pb->logical_port);
@@ -2503,7 +2511,10 @@ binding_seqno_run(struct shash *local_bindings)
ovsrec_interface_update_external_ids_delkey(
lb->iface, OVN_INSTALLED_EXT_ID);
}
- sbrec_port_binding_set_up(lb->pb, NULL, 0);
+ if (lb->pb->n_up) {
+ bool up = false;
+ sbrec_port_binding_set_up(lb->pb, &up, 1);
+ }
simap_put(&binding_iface_seqno_map, lb->name, new_seqno);
}
sset_delete(&binding_iface_bound_set, SSET_NODE_FROM_NAME(iface_id));
@@ -2536,7 +2547,6 @@ binding_seqno_install(struct shash *local_bindings)
SIMAP_FOR_EACH_SAFE (node, node_next, &binding_iface_seqno_map) {
struct shash_node *lb_node = shash_find(local_bindings, node->name);
- bool up = true;
if (!lb_node) {
goto del_seqno;
@@ -2554,12 +2564,15 @@ binding_seqno_install(struct shash *local_bindings)
ovsrec_interface_update_external_ids_setkey(lb->iface,
OVN_INSTALLED_EXT_ID,
"true");
- sbrec_port_binding_set_up(lb->pb, &up, 1);
-
- struct shash_node *child_node;
- SHASH_FOR_EACH (child_node, &lb->children) {
- struct local_binding *lb_child = child_node->data;
- sbrec_port_binding_set_up(lb_child->pb, &up, 1);
+ if (lb->pb->n_up) {
+ bool up = true;
+
+ sbrec_port_binding_set_up(lb->pb, &up, 1);
+ struct shash_node *child_node;
+ SHASH_FOR_EACH (child_node, &lb->children) {
+ struct local_binding *lb_child = child_node->data;
+ sbrec_port_binding_set_up(lb_child->pb, &up, 1);
+ }
}
del_seqno:
diff --git a/northd/ovn-northd.c b/northd/ovn-northd.c
index 307ee9c..0dc920b 100644
--- a/northd/ovn-northd.c
+++ b/northd/ovn-northd.c
@@ -3329,6 +3329,14 @@ ovn_port_update_sbrec(struct northd_context *ctx,
if (op->tunnel_key != op->sb->tunnel_key) {
sbrec_port_binding_set_tunnel_key(op->sb, op->tunnel_key);
}
+
+ /* ovn-controller will update 'Port_Binding.up' only if it was explicitly
+ * set to 'false'.
+ */
+ if (!op->sb->n_up) {
+ bool up = false;
+ sbrec_port_binding_set_up(op->sb, &up, 1);
+ }
}
/* Remove mac_binding entries that refer to logical_ports which are
diff --git a/tests/ovn.at b/tests/ovn.at
index 1956f5c..2ef056b 100644
--- a/tests/ovn.at
+++ b/tests/ovn.at
@@ -23697,7 +23697,7 @@ check ovn-nbctl ls-add ls
AS_BOX([add OVS port for existing LSP])
check ovn-nbctl lsp-add ls lsp1
check ovn-nbctl --wait=hv sync
-check_column "[]" Port_Binding up logical_port=lsp1
+check_column "false" Port_Binding up logical_port=lsp1
check ovs-vsctl add-port br-int lsp1 -- set Interface lsp1 external-ids:iface-id=lsp1
check_column "true" Port_Binding up logical_port=lsp1
@@ -23712,5 +23712,51 @@ check_column "true" Port_Binding up logical_port=lsp2
wait_column "true" nb:Logical_Switch_Port up name=lsp2
OVS_WAIT_UNTIL([test `ovs-vsctl get Interface lsp2 external_ids:ovn-installed` = '"true"'])
+AS_BOX([ovn-controller should not reset Port_Binding.up without northd])
+# Pause northd and clear the "up" field to simulate older ovn-northd
+# versions writing to the Southbound DB.
+as northd ovn-appctl -t ovn-northd pause
+as northd-backup ovn-appctl -t ovn-northd pause
+
+as hv1 ovn-appctl -t ovn-controller debug/pause
+check ovn-sbctl clear Port_Binding lsp1 up
+as hv1 ovn-appctl -t ovn-controller debug/resume
+
+# Forcefully release the Port_Binding so ovn-controller reclaims it.
+# Make sure the Port_Binding.up field is not updated though.
+check ovn-sbctl clear Port_Binding lsp1 chassis
+hv1_uuid=$(fetch_column Chassis _uuid name=hv1)
+wait_column "$hv1_uuid" Port_Binding chassis logical_port=lsp1
+check_column "" Port_Binding up logical_port=lsp1
+
+# Once northd should explicitly set the Port_Binding.up field to 'false' and
+# ovn-controller sets it to 'true' as soon as the update is processed.
+as northd ovn-appctl -t ovn-northd resume
+as northd-backup ovn-appctl -t ovn-northd resume
+wait_column "true" Port_Binding up logical_port=lsp1
+wait_column "true" nb:Logical_Switch_Port up name=lsp1
+
+AS_BOX([ovn-controller should reset Port_Binding.up - from NULL])
+# If Port_Binding.up is cleared externally, ovn-northd resets it to 'false'
+# and ovn-controller finally sets it to 'true' once the update is processed.
+as hv1 ovn-appctl -t ovn-controller debug/pause
+check ovn-sbctl clear Port_Binding lsp1 up
+check ovn-nbctl --wait=sb sync
+wait_column "false" nb:Logical_Switch_Port up name=lsp1
+as hv1 ovn-appctl -t ovn-controller debug/resume
+wait_column "true" Port_Binding up logical_port=lsp1
+wait_column "true" nb:Logical_Switch_Port up name=lsp1
+
+AS_BOX([ovn-controller should reset Port_Binding.up - from false])
+# If Port_Binding.up is externally set to 'false', ovn-controller should sets
+# it to 'true' once the update is processed.
+as hv1 ovn-appctl -t ovn-controller debug/pause
+check ovn-sbctl set Port_Binding lsp1 up=false
+check ovn-nbctl --wait=sb sync
+wait_column "false" nb:Logical_Switch_Port up name=lsp1
+as hv1 ovn-appctl -t ovn-controller debug/resume
+wait_column "true" Port_Binding up logical_port=lsp1
+wait_column "true" nb:Logical_Switch_Port up name=lsp1
+
OVN_CLEANUP([hv1])
AT_CLEANUP
--
1.8.3.1