On Fri, May 3, 2024 at 11:00 AM Numan Siddique <[email protected]> wrote:
>
> On Mon, Apr 22, 2024 at 2:49 AM Priyankar Jain
> <[email protected]> wrote:
> >
> > 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 <[email protected]>
>
> Acked-by: Numan Siddique <[email protected]>
Applied this patch to main and backported till branch-23.03. It
doesn't apply cleanly for branch-22.12.
Thanks
Numan
>
> Numan
>
> > ---
> > 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
> > [email protected]
> > https://mail.openvswitch.org/mailman/listinfo/ovs-dev
> >
_______________________________________________
dev mailing list
[email protected]
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
