[ovs-dev] [PATCH ovn] binding: Set Port_Binding.up only if supported.
Numan Siddique
numans at ovn.org
Wed Feb 3 11:06:27 UTC 2021
On Tue, Feb 2, 2021 at 3:15 PM Dumitru Ceara <dceara at redhat.com> wrote:
>
> 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 at ovn.org>
> Fixes: 4d3cb42b076b ("binding: Set Logical_Switch_Port.up when all OVS flows are installed.")
> Signed-off-by: Dumitru Ceara <dceara at redhat.com>
Thanks Dumitru for addressing this issue.
I see a problem with the approach taken here when the central nodes
are upgraded first.
I do understand that OVN recommends updating/upgrading ovn-controllers
first. But from
what I have seen, Openstack tripleo (and possibly Openshift too)
update the central nodes
first.
Suppose if ovn-northd and DBs are upgraded first, then after this
patch, ovn-northd sets
the logical_switch_port.up to 'down' for all the logical ports until
the ovn-controllers are
upgraded. This could cause some control plane issues. Like Openstack
neutron may notify
Nova service of vif-unplugged events. We could see this issue even if
ovn-controllers are configured
with - "ovn-match-northd-version=true".
Another approach to solve this problem would be - ovn-controller will
not set the port_binding.up
to true if the internal version in SB DB is lesser than the version in
which this new column - port_binding.up
was added.
What do you think ?
Thanks
Numan
> ---
> 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
> dev at openvswitch.org
> https://mail.openvswitch.org/mailman/listinfo/ovs-dev
>
More information about the dev
mailing list