Issue: Upon updating the network_name option of localnet port from one physical bridge to another, ovn-controller fails to cleanup the peer localnet patch port from the old bridge and ends up creating a duplicate peer localnet patch port which fails in the following ovsdb transaction:
``` "Transaction causes multiple rows in \"Interface\" table to have identical values (patch-localnet_a7d47490-8a90-4c4d-8266-2915ad07c185-to-br-int) ``` Current workflow: 1. Keep a set of all existing localnet ports on br-int. Let us call this set: existing_ports. 2. For each localnet port in SB: 2.1 Create a patch port on br-int. (if it already exists on br-int, do not create but remove the entry from exisitng_ports set). 2.2 Create a peer patch port on br-phys-x. (if it already exists on the bridge, do not create but remove the entry from exisitng_ports set). 3. Finally, cleanup all the ports and its peers from exisiting_ports. With the above workflow, upon network_name change of localnet LSP, since ports on br-int does not change, only peer port needs to be move from br-phys-x to br-phys-y, the localnet port is removed from exisiting_ports in #2.1 and its peer never becomes eligible for cleanup. Fix: Include both patch port on br-int and its peer port in the exisiting_ports set as part of #1. This make sures that the peer port is only removed from existing_ports only if it is already present on the correct bridge. (#2.1/#2.2) Otherwise, during the cleanup in #3 it will be considered. Fixes: b600316f252a ("Don't delete patch ports that don't belong to br-int") Signed-off-by: Priyankar Jain <priyankar.j...@nutanix.com> --- controller/patch.c | 32 +++++++------ tests/ovn.at | 109 +++++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 124 insertions(+), 17 deletions(-) diff --git a/controller/patch.c b/controller/patch.c index c1cd5060d..4fed6e375 100644 --- a/controller/patch.c +++ b/controller/patch.c @@ -314,6 +314,21 @@ patch_run(struct ovsdb_idl_txn *ovs_idl_txn, || smap_get(&port->external_ids, "ovn-l3gateway-port") || smap_get(&port->external_ids, "ovn-logical-patch-port")) { shash_add(&existing_ports, port->name, port); + /* Also add peer ports to the list. */ + for (size_t j = 0; j < port->n_interfaces; j++) { + struct ovsrec_interface *p_iface = port->interfaces[j]; + if (strcmp(p_iface->type, "patch")) { + continue; + } + const char *peer_name = smap_get(&p_iface->options, "peer"); + if (peer_name) { + const struct ovsrec_port *peer_port = + get_port(ovsrec_port_by_name, peer_name); + if (peer_port) { + shash_add(&existing_ports, peer_port->name, peer_port); + } + } + } } } @@ -336,23 +351,6 @@ patch_run(struct ovsdb_idl_txn *ovs_idl_txn, * ovn-controller. Otherwise it may cause unncessary dataplane * interruption during restart/upgrade. */ if (!daemon_started_recently()) { - /* delete peer patch port first */ - for (size_t i = 0; i < port->n_interfaces; i++) { - struct ovsrec_interface *iface = port->interfaces[i]; - if (strcmp(iface->type, "patch")) { - continue; - } - const char *iface_peer = smap_get(&iface->options, "peer"); - if (iface_peer) { - const struct ovsrec_port *peer_port = - get_port(ovsrec_port_by_name, iface_peer); - if (peer_port) { - remove_port(bridge_table, peer_port); - } - } - } - - /* now delete integration bridge patch port */ remove_port(bridge_table, port); } } diff --git a/tests/ovn.at b/tests/ovn.at index 4d0c7ad53..4b71e4916 100644 --- a/tests/ovn.at +++ b/tests/ovn.at @@ -36454,6 +36454,115 @@ OVN_CLEANUP([hv1]) AT_CLEANUP ]) +OVN_FOR_EACH_NORTHD([ +AT_SETUP([ovn-controller update network_name option for localnet port]) +ovn_start +net_add n1 + +sim_add hv1 +as hv1 + +# Bridge Topology +# Initial: br-int and br-phys-1 connected through ovn-localnet patch port. +# +# br-phys-1 -- br-int +# +# Final: br-int and br-phys-3 connected through ovn-localnet patch port. +# Similarly br-int-2 and br-hys-2 connected through localnet patch port +# but not owned by this controller. +# +# br-phys-1 br-int -- br-phys-3 +# br-int-2 -- br-phys-2 + +# Create bridges +check ovs-vsctl add-br br-int +check ovs-vsctl add-br br-int-2 +check ovs-vsctl add-br br-phys-1 +check ovs-vsctl add-br br-phys-2 +check ovs-vsctl add-br br-phys-3 +check ovs-vsctl set open . external-ids:ovn-bridge-mappings-hv1=phys-1:br-phys-1,phy-2:br-phys-2,phys-3:br-phys-3 + +ovn_attach n1 br-phys-1 192.168.1.1 24 + +check ovs-vsctl -- add-port br-int vif1 -- \ + set interface vif1 external-ids:iface-id=lp1 + +check ovn-nbctl ls-add ls + +check ovn-nbctl lsp-add ls lp1 +check ovn-nbctl lsp-set-addresses lp1 "00:00:00:00:00:01 10.0.0.1" + +check ovn-nbctl lsp-add ls ln_port +check ovn-nbctl lsp-set-addresses ln_port unknown +check ovn-nbctl lsp-set-type ln_port localnet +check ovn-nbctl lsp-set-options ln_port network_name=phys-1 +wait_for_ports_up +check ovn-nbctl --wait=hv sync + +# Allow controller to immediately clean patch ports up if any. +check ovn-appctl -t ovn-controller debug/ignore-startup-delay + +# check that patch port created on br-int and br-phys-1. +OVS_WAIT_UNTIL([ + test 1 = $(ovs-vsctl --columns _uuid --bare find Port name=patch-br-int-to-ln_port | wc -l) +]) +OVS_WAIT_UNTIL([ + test 1 = $(ovs-vsctl list-ports br-int | grep patch-br-int-to-ln_port | wc -l) +]) +OVS_WAIT_UNTIL([ + test 1 = $(ovs-vsctl --columns _uuid --bare find Port name=patch-ln_port-to-br-int | wc -l) +]) +OVS_WAIT_UNTIL([ + test 1 = $(ovs-vsctl list-ports br-phys-1 | grep patch-ln_port-to-br-int | wc -l) +]) + +check ovn-appctl debug/pause + +# Now move the localnet port from phys-1 to phys-3. +check ovn-nbctl lsp-set-options ln_port network_name=phys-3 + +# Also create fake localnet ports on br-int-2 +check ovs-vsctl -- add-port br-int-2 fake-int-2-to-phys-2 -- \ + set port fake-int-2-to-phys-2 external-ids:ovn-localnet-port=fake-port -- \ + set interface fake-int-2-to-phys-2 options:peer=fake-phys-2-to-int-2 type=patch +check ovs-vsctl -- add-port br-phys-2 fake-phys-2-to-int-2 -- \ + set port fake-phys-2-to-int-2 external-ids:ovn-localnet-port=fake-port -- \ + set interface fake-phys-2-to-int-2 options:peer=fake-int-2-to-phys-2 type=patch + +check ovn-appctl debug/resume +check ovn-nbctl --wait=hv sync + +# patch for port on br-phys-1 is cleanedup. +OVS_WAIT_UNTIL([ + test 0 = $(ovs-vsctl list-ports br-phys-1 | grep patch-ln_port-to-br-int | wc -l) +]) + +# Patch port created on br-int and br-phy-3 +OVS_WAIT_UNTIL([ + test 1 = $(ovs-vsctl --columns _uuid --bare find Port name=patch-br-int-to-ln_port | wc -l) +]) +OVS_WAIT_UNTIL([ + test 1 = $(ovs-vsctl list-ports br-int | grep patch-br-int-to-ln_port | wc -l) +]) +OVS_WAIT_UNTIL([ + test 1 = $(ovs-vsctl --columns _uuid --bare find Port name=patch-ln_port-to-br-int | wc -l) +]) +OVS_WAIT_UNTIL([ + test 1 = $(ovs-vsctl list-ports br-phys-3 | grep patch-ln_port-to-br-int | wc -l) +]) + +# check that patch port on a different bridge is not touched +OVS_WAIT_UNTIL([ + test 1 = $(ovs-vsctl --columns _uuid --bare find Port name=fake-int-2-to-phys-2 | wc -l) +]) +OVS_WAIT_UNTIL([ + test 1 = $(ovs-vsctl --columns _uuid --bare find Port name=fake-phys-2-to-int-2 | wc -l) +]) + +OVN_CLEANUP([hv1]) +AT_CLEANUP +]) + # NOTE: This test case runs two ovn-controllers inside the same sandbox (hv1). # Each controller uses a unique chassis name - hv1 and hv2 - and manage # different bridges with different ports. This is why all 'as' commands below -- 2.39.2 _______________________________________________ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev