From 3de7959b9018f53abd06320bc7f1a43ec216db7e Mon Sep 17 00:00:00 2001 From: Dumitru Ceara 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 Fixes: 4d3cb42b076b ("binding: Set Logical_Switch_Port.up when all OVS flows are installed.") Signed-off-by: Dumitru Ceara Signed-off-by: Numan Siddique (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