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 <[email protected]> Fixes: 4d3cb42b076b ("binding: Set Logical_Switch_Port.up when all OVS flows are installed.") Signed-off-by: Dumitru Ceara <[email protected]> --- controller/binding.c | 32 +++++++++++++++++++++++--------- northd/ovn-northd.c | 8 ++++++++ tests/ovn.at | 48 +++++++++++++++++++++++++++++++++++++++++++++++- 3 files changed, 78 insertions(+), 10 deletions(-) diff --git a/controller/binding.c b/controller/binding.c index d44f635..913c9d6 100644 --- a/controller/binding.c +++ b/controller/binding.c @@ -895,6 +895,12 @@ claim_lport(const struct sbrec_port_binding *pb, if (tracked_datapaths) { update_lport_tracking(pb, tracked_datapaths); } + } else if (!sb_readonly && pb->up && !(*pb->up)) { + /* For already claimed port bindings, potentially created by older + * versions of ovn-northd, make sure the 'pb->up' field gets updated + * only if it's explicitly set to 'false'. + */ + binding_iface_bound_add(pb->logical_port); } /* Check if the port encap binding, if any, has changed */ @@ -942,7 +948,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->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); @@ -2468,7 +2477,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->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)); @@ -2501,7 +2513,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; @@ -2519,12 +2530,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->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 4de79d9..d7149cd 100644 --- a/northd/ovn-northd.c +++ b/northd/ovn-northd.c @@ -3099,6 +3099,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->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 326b564..7a0f633 100644 --- a/tests/ovn.at +++ b/tests/ovn.at @@ -23806,7 +23806,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 @@ -23821,5 +23821,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 _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
