Re: [ovs-dev] [PATCH ovn v3] controller: Wait for patch ports before nb_cfg++.

[Rosemarie O'Riorden via dev]

Hi Mark, thanks so much for the review! Response inline below.
On 10/14/26 4:39 PM, Mark Michelson wrote:
Hi Rosemarie, the logic of this change looks 100% correct to me. I
have one small finding below.

On Tue, Oct 13, 2026 at 9:18 PM Rosemarie O'Riorden via dev
<ovs-dev@openvswitch.org> wrote:

ovn-controller would continue with the next iteration before waiting
for patch ports to finish updating.

Thus when using --wait=hv for a localnet port operation, ovn-nbctl would
not actually wait for patch ports. This sometimes led to failures in
the "localnet port change and chassisredirect bridged redirect" test,
making it flaky.

To remedy this issue, functions performing patch port operations now
report a status, and ovn-controller will not increment nb_cfg if they
show to be incomplete. This allows --wait=hv to actually wait as
intended.

Also remove OVN_WAIT_PATCH_PORT_FLOWS, which is no longer needed
now that --wait=hv properly waits for patch ports, and replace all
callers with "check ovn-nbctl --wait=hv sync".

Fixes: 84748d013155 ("ovn: Make it possible for CMS to detect when the OVN system is 
up-to-date.")
Reported-at: https://issues.redhat.com/browse/FDP-4156
Signed-off-by: Rosemarie O'Riorden <rosema...@redhat.com>
---
As discussed on the mailing list, this is intended as a temporary fix for a
larger issue. In the future I intend to post another patch fixing this same
issue more generally with ofctrl-seqno, as this affects more than just patch
ports.

v3:
  - Move loop checking ofport numbers into patch_run().
  - Remove test macro OVN_WAIT_PATCH_PORT_FLOWS and replace it with
    "check ovn-nbctl --wait=hv sync".
  - Add localnet port deletion to test.
  - Re-add fix for broken test that was handled in v1.
  - Fix Fixes tag.

v2:
  - Also check that patch port interfaces have a positive ofport (not just that
    patch_run() didn't create/delete ports), so nb_cfg is held until OVS
    actually installs the ports.
  - Extract find_patch_ports() from patch_run() for use in ovn-controller.c.
  - Replace modified existing test with a new dedicated test.
--
  controller/ovn-controller.c |  39 ++++++----
  controller/patch.c          | 127 ++++++++++++++++++++++++-------
  controller/patch.h          |   2 +-
  tests/ovn-macros.at         |  22 ------
  tests/ovn.at                | 147 ++++++++++++++++++++++++++----------
  tests/system-ovn.at         |   2 +-
  6 files changed, 233 insertions(+), 106 deletions(-)

diff --git a/controller/ovn-controller.c b/controller/ovn-controller.c
index d57ff316d..2ebe26631 100644
--- a/controller/ovn-controller.c
+++ b/controller/ovn-controller.c
@@ -8484,16 +8484,19 @@ main(int argc, char *argv[])
                          }
                      }

+                    bool patch_ports_synced = true;
+
                      runtime_data = engine_get_data(&en_runtime_data);
                      if (runtime_data) {
                          stopwatch_start(PATCH_RUN_STOPWATCH_NAME, 
time_msec());
-                        patch_run(ovs_idl_txn,
-                            sbrec_port_binding_by_type,
+                        patch_ports_synced = patch_run(
+                            ovs_idl_txn, sbrec_port_binding_by_type,
                              ovsrec_bridge_table_get(ovs_idl_loop.idl),
                              ovsrec_open_vswitch_table_get(ovs_idl_loop.idl),
-                            ovsrec_port_by_name,
-                            br_int, chassis, &runtime_data->local_datapaths);
+                            ovsrec_port_by_name, br_int, chassis,
+                            &runtime_data->local_datapaths);
                          stopwatch_stop(PATCH_RUN_STOPWATCH_NAME, time_msec());
+
                          if (vif_plug_provider_has_providers() && ovs_idl_txn) 
{
                              struct vif_plug_ctx_in vif_plug_ctx_in = {
                                  .ovs_idl_txn = ovs_idl_txn,
@@ -8611,18 +8614,22 @@ main(int argc, char *argv[])
                                       chassis, mac_cache_data);
                      }

-                    /* Snapshot (nb_cfg, sb_ts) atomically from SB_Global
-                     * and pair them through the barrier ack so the
-                     * eventual completion can be attributed to the
-                     * timestamp that corresponded to this exact nb_cfg
-                     * generation -- not whatever SB_Global value has
-                     * moved on to by the time the barrier acks. */
-                    struct nb_cfg_snap snap = get_nb_cfg(
-                        sbrec_sb_global_table_get(ovnsb_idl_loop.idl),
-                        ovnsb_cond_seqno, ovnsb_expected_cond_seqno);
-                    ofctrl_stamped_seqno_update_create(ofctrl_seq_type_nb_cfg,
-                                                      snap.nb_cfg,
-                                                      snap.ts);
+                    /* Wait for patch ports to be installed and synced before
+                     * incrementing nb_cfg so that --wait=hv properly waits
+                     * for patch ports. */
+                    if (patch_ports_synced) {
+                        /* Snapshot (nb_cfg, sb_ts) atomically from SB_Global
+                         * and pair them through the barrier ack so the
+                         * eventual completion can be attributed to the
+                         * timestamp that corresponded to this exact nb_cfg
+                         * generation -- not whatever SB_Global value has
+                         * moved on to by the time the barrier acks. */
+                        struct nb_cfg_snap snap = get_nb_cfg(
+                            sbrec_sb_global_table_get(ovnsb_idl_loop.idl),
+                            ovnsb_cond_seqno, ovnsb_expected_cond_seqno);
+                        ofctrl_stamped_seqno_update_create(
+                            ofctrl_seq_type_nb_cfg, snap.nb_cfg, snap.ts);
+                    }

                      struct local_binding_data *binding_data =
                          runtime_data ? &runtime_data->lbinding_data : NULL;
diff --git a/controller/patch.c b/controller/patch.c
index 4fed6e375..78acfc8b9 100644
--- a/controller/patch.c
+++ b/controller/patch.c
@@ -71,10 +71,13 @@ match_patch_port(const struct ovsrec_port *port, const char 
*peer)

  /* Creates a patch port in bridge 'src' named 'src_name', whose peer is
   * 'dst_name' in bridge 'dst'.  Initializes the patch port's 
external-ids:'key'
- * to 'key'.
+ * to 'key'.  The port is only created if 'ovs_idl_txn' is non-NULL.
   *
- * If such a patch port already exists, removes it from 'existing_ports'. */
-static void
+ * If such a patch port already exists in 'src', removes it from
+ * 'existing_ports' and returns false.
+ *
+ * Otherwise, creates port (if 'ovs_idl_txn'), and returns true. */
+static bool
  create_patch_port(struct ovsdb_idl_txn *ovs_idl_txn,
                    const char *key, const char *value,
                    const struct ovsrec_bridge *src, const char *src_name,

This is one of those cases where changing the return type to bool
feels a bit shoehorned in. The previous function was void because it
didn't really matter if the patch port existed already or if it was
being added. Now there are essentially three outcomes:
1) The patch port already exists.
2) We can't add a patch port since our transaction is NULL.
3) We insert a patch port as part of the transaction.

Trying to represent those three outcomes as two boolean values only
really works because the caller of create_patch_port() only cares
about outcome (1). If anyone cared about outcomes (2) or (3), then
they would not be able to know from the return value which of those
two outcomes happened.

There are two potential ways to fix this.

Option one is to break create_patch_port() into two separate
functions. The first function, which we'll call find_patch_port(),
does the loops through the ports on src and calls match_patch_port()
on each one. If a match is found, then find_patch_port() returns true.
If not, it returns false. Then create_patch_port() can attempt to
insert the new patch port if necessary. It can return true if it
successfully inserts the patch port, or false if it can't (i.e. the
transaction is false).

Callers would do something like this:

bool synced = find_patch_port(src, dst_name);
if (!synced) {
     bool inserted = create_patch_port(<all the usual values);
}

Then callers can make decisions based on whether the patch already
existed, or if the patch was inserted properly.
The downside to this approach is that callers must follow a certain
order of operations, or else they can accidentally attempt to insert a
patch port that already exists.

Option two is to change the return type of create_patch_port to an
enum instead. This would look something like this:

enum create_patch_port_result {
     PATCH_PORT_EXISTS,
     PATCH_PORT_INSERTED,
     PATCH_PORT_INSERTION_FAILED,
};

Now the three spots where create_patch_port() returns can return a
distinct result.

add_bridge_mappings_by_type() can then work like this:

enum create_patch_port_result br_int_result;
enum create_patch_port_result br_ln_result;
br_int_result = create_patch_port(<the usual parameters>)
br_ln_result = create_patch_port(<the usual parameters>)

synced = synced && (br_int_result == PATCH_PORT_EXISTS) &&
(br_ln_result == PATCH_PORT_EXISTS);

I think this is better than option 1 since callers of
create_patch_port() don't have to call functions in a specific order
in order not to cause errors. I was also a bit confused by the logic
inversion that was happening based on the return value of
create_patch_port() in add_bridge_mappings_by_type(). This makes the
intent much more clear.

What do you think?

I completely agree with your assessment and I'm glad you brought this 
up! I also prefer the enum return option. It will make it much more 
readable and logically sound.


@@ -85,10 +88,14 @@ create_patch_port(struct ovsdb_idl_txn *ovs_idl_txn,
          if (match_patch_port(src->ports[i], dst_name)) {
              /* Patch port already exists on 'src'. */
              shash_find_and_delete(existing_ports, src->ports[i]->name);
-            return;
+            return false;
          }
      }

+    if (!ovs_idl_txn) {
+        return true;
+    }
+
      ovsdb_idl_txn_add_comment(ovs_idl_txn,
              "ovn-controller: creating patch port '%s' from '%s' to '%s'",
              src_name, src->name, dst->name);
@@ -97,6 +104,7 @@ create_patch_port(struct ovsdb_idl_txn *ovs_idl_txn,
      const struct smap port_ids = SMAP_CONST1(&port_ids, key, value);
      ovsport_create(ovs_idl_txn, src, src_name, "patch", &port_ids, NULL,
                     &if_options, 0);
+    return true;
  }

  static void
@@ -165,7 +173,12 @@ add_ovs_bridge_mappings(const struct 
ovsrec_open_vswitch_table *ovs_table,
      }
  }

-static void
+/* Adds the patch ports needed by the port bindings of type 'pb_type' that are
+ * local to this chassis.
+ *
+ * Returns true if all of those patch ports are already present in the
+ * database. */
+static bool
  add_bridge_mappings_by_type(struct ovsdb_idl_txn *ovs_idl_txn,
                              struct ovsdb_idl_index 
*sbrec_port_binding_by_type,
                              const struct ovsrec_bridge *br_int,
@@ -178,6 +191,8 @@ add_bridge_mappings_by_type(struct ovsdb_idl_txn 
*ovs_idl_txn,
  {
      struct sbrec_port_binding *target =
          sbrec_port_binding_index_init_row(sbrec_port_binding_by_type);
+    bool synced = true;
+
      sbrec_port_binding_index_set_type(target, pb_type);

      const struct sbrec_port_binding *binding;
@@ -225,20 +240,32 @@ add_bridge_mappings_by_type(struct ovsdb_idl_txn 
*ovs_idl_txn,

          char *name1 = patch_port_name(br_int->name, binding->logical_port);
          char *name2 = patch_port_name(binding->logical_port, br_int->name);
-        create_patch_port(ovs_idl_txn, patch_port_id, binding->logical_port,
-                          br_int, name1, br_ln, name2, existing_ports);
-        create_patch_port(ovs_idl_txn, patch_port_id, binding->logical_port,
-                          br_ln, name2, br_int, name1, existing_ports);
+        bool br_int_port_exists =
+            !create_patch_port(ovs_idl_txn, patch_port_id,
+                               binding->logical_port,
+                               br_int, name1, br_ln, name2,
+                               existing_ports);
+        bool br_ln_port_exists =
+            !create_patch_port(ovs_idl_txn, patch_port_id,
+                               binding->logical_port,
+                               br_ln, name2, br_int, name1,
+                               existing_ports);
+
+        synced = synced && br_int_port_exists && br_ln_port_exists;
          free(name1);
          free(name2);
      }
      sbrec_port_binding_index_destroy_row(target);
+    return synced;
  }

  /* Obtains external-ids:ovn-bridge-mappings from OVSDB and adds patch ports 
for
   * the local bridge mappings.  Removes any patch ports for bridge mappings 
that
- * already existed from 'existing_ports'. */
-static void
+ * already existed from 'existing_ports'.
+ *
+ * Returns true if all of the required patch ports are already present in the
+ * database. */
+static bool
  add_bridge_mappings(struct ovsdb_idl_txn *ovs_idl_txn,
                      struct ovsdb_idl_index *sbrec_port_binding_by_type,
                      const struct ovsrec_bridge_table *bridge_table,
@@ -253,10 +280,12 @@ add_bridge_mappings(struct ovsdb_idl_txn *ovs_idl_txn,

      add_ovs_bridge_mappings(ovs_table, bridge_table, &bridge_mappings);

-    add_bridge_mappings_by_type(ovs_idl_txn, sbrec_port_binding_by_type,
-                                br_int, existing_ports, chassis,
-                                &bridge_mappings, "l2gateway",
-                                "ovn-l2gateway-port", local_datapaths, true);
+    bool l2gateway_synced =
+        add_bridge_mappings_by_type(ovs_idl_txn, sbrec_port_binding_by_type,
+                                    br_int, existing_ports, chassis,
+                                    &bridge_mappings, "l2gateway",
+                                    "ovn-l2gateway-port", local_datapaths,
+                                    true);

      /* Since having localnet ports that are not mapped on some chassis is a
       * supported configuration used to implement multisegment switches with
@@ -264,11 +293,15 @@ add_bridge_mappings(struct ovsdb_idl_txn *ovs_idl_txn,
       * run but don't unnecessarily pollute the log file; pass
       * 'log_missing_bridge = false'.
       */
-    add_bridge_mappings_by_type(ovs_idl_txn, sbrec_port_binding_by_type,
-                                br_int, existing_ports, NULL,
-                                &bridge_mappings, "localnet",
-                                "ovn-localnet-port", local_datapaths, false);
+    bool localnet_synced =
+        add_bridge_mappings_by_type(ovs_idl_txn, sbrec_port_binding_by_type,
+                                    br_int, existing_ports, NULL,
+                                    &bridge_mappings, "localnet",
+                                    "ovn-localnet-port", local_datapaths,
+                                    false);
+
      shash_destroy(&bridge_mappings);
+    return l2gateway_synced && localnet_synced;
  }

  static const struct ovsrec_port *
@@ -285,7 +318,31 @@ get_port(struct ovsdb_idl_index *ovsrec_port_by_name, 
const char *name)
      return port;
  }

-void
+/* Returns true if ovs-vswitchd has assigned an ofport number to every
+ * interface in 'port'. */
+static bool
+patch_port_is_installed(const struct ovsrec_port *port)
+{
+    for (size_t i = 0; i < port->n_interfaces; i++) {
+        const struct ovsrec_interface *iface = port->interfaces[i];
+
+        /* An ofport of 0 means that it has not been assigned yet and
+         * -1 means that ovs-vswitchd failed to add the interface. */
+        if (!iface->n_ofport || *iface->ofport < 1) {
+            return false;
+        }
+    }
+    return true;
+}
+
+/* Adds to the local OVS database the patch ports required by the localnet
+ * and l2gateway ports that are local to this chassis and removes the ones
+ * that are no longer needed.  The database is only updated if 'ovs_idl_txn'
+ * is non-NULL.
+ *
+ * Returns true if the database already reflects the required set of patch
+ * ports and ovs-vswitchd has assigned an ofport number to each of them. */
+bool
  patch_run(struct ovsdb_idl_txn *ovs_idl_txn,
            struct ovsdb_idl_index *sbrec_port_binding_by_type,
            const struct ovsrec_bridge_table *bridge_table,
@@ -295,10 +352,6 @@ patch_run(struct ovsdb_idl_txn *ovs_idl_txn,
            const struct sbrec_chassis *chassis,
            const struct hmap *local_datapaths)
  {
-    if (!ovs_idl_txn) {
-        return;
-    }
-
      /* Figure out what patch ports already exist.
       *
       * ovn-controller does not create or use ports of type 
"ovn-l3gateway-port"
@@ -332,12 +385,29 @@ patch_run(struct ovsdb_idl_txn *ovs_idl_txn,
          }
      }

+    /* The physical flows that use a patch port can't be installed until
+     * ovs-vswitchd assigns it an ofport number, so the configuration is not
+     * applied yet even though the port is already in the database. */
+    bool ports_installed = true;
+    struct shash_node *node;
+    SHASH_FOR_EACH (node, &existing_ports) {
+        if (!patch_port_is_installed(node->data)) {
+            ports_installed = false;
+            break;
+        }
+    }
+
      /* Create in the database any patch ports that should exist.  Remove from
       * 'existing_ports' any patch ports that do exist in the database and
       * should be there. */
-    add_bridge_mappings(ovs_idl_txn, sbrec_port_binding_by_type, bridge_table,
-                        ovs_table, br_int, &existing_ports, chassis,
-                        local_datapaths);
+    bool synced = add_bridge_mappings(ovs_idl_txn, sbrec_port_binding_by_type,
+                                      bridge_table, ovs_table, br_int,
+                                      &existing_ports, chassis,
+                                      local_datapaths);
+
+    if (!ports_installed || !shash_is_empty(&existing_ports)) {
+        synced = false;
+    }

      /* Now 'existing_ports' only still contains patch ports that exist in the
       * database but shouldn't.  Delete them from the database. */
@@ -350,9 +420,10 @@ patch_run(struct ovsdb_idl_txn *ovs_idl_txn,
           * data is not completely downloaded yet after last restart of
           * ovn-controller.  Otherwise it may cause unncessary dataplane
           * interruption during restart/upgrade. */
-        if (!daemon_started_recently()) {
+        if (!daemon_started_recently() && ovs_idl_txn) {
              remove_port(bridge_table, port);
          }
      }
      shash_destroy(&existing_ports);
+    return synced;
  }
diff --git a/controller/patch.h b/controller/patch.h
index db4a888e6..26ebf401c 100644
--- a/controller/patch.h
+++ b/controller/patch.h
@@ -36,7 +36,7 @@ struct shash;
  void add_ovs_bridge_mappings(const struct ovsrec_open_vswitch_table 
*ovs_table,
                               const struct ovsrec_bridge_table *bridge_table,
                               struct shash *bridge_mappings);
-void patch_run(struct ovsdb_idl_txn *ovs_idl_txn,
+bool patch_run(struct ovsdb_idl_txn *ovs_idl_txn,
                 struct ovsdb_idl_index *sbrec_port_binding_by_type,
                 const struct ovsrec_bridge_table *,
                 const struct ovsrec_open_vswitch_table *,
diff --git a/tests/ovn-macros.at b/tests/ovn-macros.at
index 7a19c0130..e0f70bcde 100644
--- a/tests/ovn-macros.at
+++ b/tests/ovn-macros.at
@@ -84,25 +84,6 @@ m4_divert_text([PREPARE_TESTS],
         [dump_diff__ "$rcv_pcap" "$exp_text"])
     }

-   ovn_wait_patch_port_flows () {
-     for localnet in $1; do
-       patch_port="patch-br-int-to-$localnet"
-       for hv in $2; do
-         echo "$3: waiting for flows for $patch_port on $hv"
-         # Patch port might be created after ports are reported up
-         OVS_WAIT_UNTIL([
-             test 1 = $(as $hv ovs-vsctl show | grep "Port \b$patch_port\b" | 
wc -l)
-         ])
-         # Wait for a flow outputing to patch port
-         OVS_WAIT_UNTIL([
-             hv_patch_ofport=$(as $hv ovs-vsctl --bare --columns ofport find 
Interface name=$patch_port)
-             echo "$patch_port=$hv_patch_ofport"
-             test -n "$hv_patch_ofport" && test 1 -le $(as $hv ovs-ofctl dump-flows 
br-int | grep -c "output:\b$hv_patch_ofport\b")
-         ])
-       done
-     done
-   }
-
     ovn_wait_remote_output_flows () {
       hv1=$1
       hv2=$2
@@ -180,9 +161,6 @@ m4_define([OVN_CHECK_PACKETS_CONTAIN],
  m4_define([OVN_CHECK_PACKETS_UNIQ],
    [ovn_wait_packets_uniq__ "$1" "$2" "__file__:__line__" $3])

-m4_define([OVN_WAIT_PATCH_PORT_FLOWS],
-  [ovn_wait_patch_port_flows "$1" "$2" "__file__:__line__"])
-
  m4_define([OVN_WAIT_REMOTE_OUTPUT_FLOWS],
    [ovn_wait_remote_output_flows "$1" "$2" "__file__:__line__"])

diff --git a/tests/ovn.at b/tests/ovn.at
index 1a86d9a3d..49674266f 100644
--- a/tests/ovn.at
+++ b/tests/ovn.at
@@ -3375,7 +3375,7 @@ for i in 1 2; do
      done
  done
  wait_for_ports_up
-OVN_WAIT_PATCH_PORT_FLOWS(["ln1" "ln2" "ln3"], ["hv1"] ["hv2"])
+check ovn-nbctl --wait=hv sync

  check ovn-nbctl --wait=sb sync
  ovn-sbctl dump-flows > sbflows
@@ -3542,8 +3542,7 @@ done


  wait_for_ports_up
-check ovn-nbctl --wait=sb sync
-OVN_WAIT_PATCH_PORT_FLOWS(["ln-11" "ln-21"], ["hv-1" "hv-2"])
+check ovn-nbctl --wait=hv sync

  ovn-sbctl dump-flows > sbflows
  AT_CAPTURE_FILE([sbflows])
@@ -3831,7 +3830,7 @@ for i in 1 2; do
      check ovn-nbctl lsp-set-port-security $lsp_name f0:00:00:00:00:0$i

      OVS_WAIT_UNTIL([test x`ovn-nbctl lsp-get-up $lsp_name` = xup])
-    OVN_WAIT_PATCH_PORT_FLOWS(["ln-$i-20"], ["hv-$i"])
+    check ovn-nbctl --wait=hv sync
  done

  wait_for_ports_up
@@ -4083,7 +4082,7 @@ for i in 1 2; do

      # Patch port might be created after ports are reported up
      # Wait for a flow outputing to patch port
-    OVN_WAIT_PATCH_PORT_FLOWS(["ln-100"], ["hv-$i"])
+    check ovn-nbctl --wait=hv sync
  done

  test_packet() {
@@ -4151,7 +4150,7 @@ for i in 1 2; do

      # Patch port might be created after ports are reported up
      # Wait for a flow outputing to patch port
-    OVN_WAIT_PATCH_PORT_FLOWS(["ln-100"], ["hv-$i"])
+    check ovn-nbctl --wait=hv sync
  done
  # create taps on fabric to check vlan encapsulation there
  for i in 1 2; do
@@ -4233,7 +4232,7 @@ for i in 1 2; do

      # Patch port might be created after ports are reported up
      # Wait for a flow outputing to patch port
-    OVN_WAIT_PATCH_PORT_FLOWS(["ln"], ["hv-$i"])
+    check ovn-nbctl --wait=hv sync
  done

  for i in 1 2; do
@@ -4455,7 +4454,7 @@ for tag in 10 20; do
          check ovn-nbctl lsp-set-port-security $lsp_name f0:00:00:00:0$i:$tag

          OVS_WAIT_UNTIL([test x`ovn-nbctl lsp-get-up $lsp_name` = xup])
-        OVN_WAIT_PATCH_PORT_FLOWS(["ln-$tag"], ["hv-$tag-$i"])
+        check ovn-nbctl --wait=hv sync
      done
  done
  wait_for_ports_up
@@ -10146,9 +10145,9 @@ AT_CAPTURE_FILE([sbflows])

  # Wait until the patch ports are created in hv1 and hv2 to connect br-int to 
br-eth0
  AT_CHECK([ovn-nbctl set logical_router lr0 options:chassis=hv1])
-OVN_WAIT_PATCH_PORT_FLOWS(["ln_port"], ["hv1"])
+check ovn-nbctl --wait=hv sync
  AT_CHECK([ovn-nbctl set logical_router lr0 options:chassis=hv2])
-OVN_WAIT_PATCH_PORT_FLOWS(["ln_port"], ["hv2"])
+check ovn-nbctl --wait=hv sync

  # Temporarily remove lr0 chassis
  # Wait for hv confirmation to make sure chassis is removed before we reset 
pcap
@@ -12871,7 +12870,6 @@ for i in 1 2; do
          > bfd$i.expected
  done

-OVN_WAIT_PATCH_PORT_FLOWS([ln-ext], [gw1 gw2])
  check ovn-nbctl --wait=hv sync

  check_bfd_packets() {
@@ -13310,8 +13308,7 @@ check as ext1 ovs-vsctl set open . 
external-ids:ovn-bridge-mappings=phys:br-phys

  wait_for_ports_up
  check ovn-nbctl --wait=sb sync
-OVN_WAIT_PATCH_PORT_FLOWS(["ln-alice"], ["gw1"] ["gw2"],["gw3"])
-OVN_WAIT_PATCH_PORT_FLOWS(["ln-outside"], ["ext1"])
+check ovn-nbctl --wait=hv sync

  ovn-sbctl dump-flows > sbflows
  AT_CAPTURE_FILE([sbflows])
@@ -13555,7 +13552,6 @@ check as hv3 ovs-vsctl set open . 
external-ids:ovn-bridge-mappings=phys:br-phys
  dnl Allow some time for ovn-northd and ovn-controller to catch up.
  wait_for_ports_up
  check ovn-nbctl --wait=hv sync
-OVN_WAIT_PATCH_PORT_FLOWS(["ln-outside"], ["hv3"])

  (echo "---------NB dump-----"
   ovn-nbctl show
@@ -13670,7 +13666,7 @@ OVN_CHECK_PACKETS_UNIQ([hv3/vif1-tx.pcap], 
[hv3-vif1.expected])
  as hv2 ovs-vsctl set open . external-ids:ovn-bridge-mappings=phys:br-phys

  # Wait until the patch ports are created in hv2 to connect br-int to br-phys
-OVN_WAIT_PATCH_PORT_FLOWS(["ln-alice"], ["hv2"])
+check ovn-nbctl --wait=hv sync

  dnl Allow some time for ovn-controller to catch up.
  wait_for_ports_up
@@ -13742,7 +13738,6 @@ check ovn-nbctl lsp-add foo foo2 \
  # Allow some time for ovn-northd and ovn-controller to catch up.
  wait_for_ports_up
  check ovn-nbctl --wait=hv sync
-OVN_WAIT_PATCH_PORT_FLOWS(["ln-alice"], ["hv1"])

  : > hv1-vif2.expected

@@ -13842,7 +13837,7 @@ OVN_CHECK_PACKETS([hv1/snoopvif-tx.pcap], [packets])
  AT_CHECK([as hv2 ovs-vsctl set Open_vSwitch . 
external-ids:ovn-bridge-mappings=physnet1:br-phys])

  # Wait until the patch ports are created in hv2 to connect br-int to br-phys
-OVN_WAIT_PATCH_PORT_FLOWS(["ln_port"], ["hv2"])
+check ovn-nbctl --wait=hv sync

  # Wait for packets to be received.
  OVS_WAIT_UNTIL([test `wc -c < "hv1/snoopvif-tx.pcap"` -ge 100])
@@ -13870,7 +13865,7 @@ ovs-vsctl -- add-port br-int hv3-vif2 -- \
  AT_CHECK([as hv3 ovs-vsctl set Open_vSwitch . 
external-ids:ovn-bridge-mappings=physnet1:br-phys])

  # Wait until the patch ports are created in hv3 to connect br-int to br-phys
-OVN_WAIT_PATCH_PORT_FLOWS(["ln_port"], ["hv3"])
+check ovn-nbctl --wait=hv sync

  # Re-add nat-addresses option
  check ovn-nbctl lsp-set-options lrp0-rp router-port=lrp0 
nat-addresses="router"
@@ -15550,7 +15545,6 @@ AT_CHECK([ovn-nbctl lsp-add-localnet-port ls0 ln_port 
physnet1])
  # wait for earlier changes to take effect
  wait_for_ports_up
  check ovn-nbctl --wait=hv sync
-OVN_WAIT_PATCH_PORT_FLOWS(["ln_port"], ["hv2"] ["hv3"])

  # reset hv1/snoopvif after hv2/br-phys_n1 and hv3/br-phys_n1 as we wait
  # for packets to be received on hv1/snoopvif-tx.
@@ -16880,8 +16874,7 @@ reset_env
  # Start port migration hv1 -> hv2: both hypervisors are now bound
  check ovn-nbctl --wait=hv lsp-set-options migrator requested-chassis=hv1,hv2
  wait_for_ports_up
-OVN_WAIT_PATCH_PORT_FLOWS(["public"], ["hv1"])
-OVN_WAIT_PATCH_PORT_FLOWS(["public"], ["hv2"])
+check ovn-nbctl --wait=hv sync
  OVN_WAIT_REMOTE_OUTPUT_FLOWS(["hv1"],["hv2"])
  OVN_WAIT_REMOTE_OUTPUT_FLOWS(["hv2"],["hv1"])
  OVN_WAIT_REMOTE_OUTPUT_FLOWS(["hv3"],["hv1"])
@@ -23920,7 +23913,7 @@ check ovn-nbctl --wait=sb sync
  #ovn-sbctl dump-flows
  for i in 1 2; do
      for j in 1 2; do
-        OVN_WAIT_PATCH_PORT_FLOWS(["ln$j"], ["hv$i"])
+        check ovn-nbctl --wait=hv sync
      done
  done

@@ -26364,8 +26357,7 @@ m4_define([DVR_N_S_ARP_HANDLING],
     check ovn-nbctl lrp-set-gateway-chassis router-to-underlay hv3
     wait_for_ports_up
     check ovn-nbctl --wait=sb sync
-   OVN_WAIT_PATCH_PORT_FLOWS(["ln1"] ["ln2"], ["hv1"] ["hv2"] ["hv3"])
-   OVN_WAIT_PATCH_PORT_FLOWS(["ln4"], ["hv4"])
+   check ovn-nbctl --wait=hv sync

     wait_row_count Port_Binding 1 logical_port=cr-router-to-underlay

@@ -26589,8 +26581,7 @@ m4_define([DVR_N_S_PING],

     wait_for_ports_up
     check ovn-nbctl --wait=sb sync
-   OVN_WAIT_PATCH_PORT_FLOWS(["ln1"] ["ln2"], ["hv1"] ["hv2"] ["hv3"])
-   OVN_WAIT_PATCH_PORT_FLOWS(["ln4"], ["hv4"])
+   check ovn-nbctl --wait=hv sync

     OVN_POPULATE_ARP

@@ -27328,8 +27319,7 @@ check ovn-nbctl lrp-set-redirect-type 
router-to-underlay bridged
  wait_for_ports_up
  check ovn-nbctl --wait=sb sync

-OVN_WAIT_PATCH_PORT_FLOWS(["ln1"] ["ln2"] ["ln3"], ["hv1"] ["hv2"] ["hv3"])
-OVN_WAIT_PATCH_PORT_FLOWS(["ln4"], ["hv4"])
+check ovn-nbctl --wait=hv sync

  OVN_POPULATE_ARP

@@ -40418,7 +40408,7 @@ for i in 1 2; do

      # Patch port might be created after ports are reported up
      # Wait for a flow outputing to patch port
-    OVN_WAIT_PATCH_PORT_FLOWS(["public0"], ["hv-$i"])
+    check ovn-nbctl --wait=hv sync
  done

  check ovn-nbctl set Logical_Switch_Port lsp1 options:qos_min_rate=200000
@@ -40936,14 +40926,19 @@ check ovn-nbctl --wait=hv sync

  sleep_ovsdb hv1

-# Do following operations through one nbctl command
-# Otherwise, they would probably result in multiple sb updates to 
ovn-controller
-# As ovsdb is sleeping those multiple updates would resulti in requests to 
ovsdb being
-# postponed as ovsdb becomes ro.
-# Hence, patch port creation would be delayed after ovsdb becomes rw.
-check ovn-nbctl --wait=hv lsp-add-localnet-port ls1 ln1 phys
+# Create a localnet port and wait for ovn-controller to process the change.
+# --wait=hv cannot be used here because hv1's ovsdb is asleep.
+# Make sure ovn-controller has not updated nb_cfg.
+runtime_data_runs=$(as hv1 ovn-appctl -t ovn-controller inc-engine/show-stats 
runtime_data compute)
+nb_cfg="$(fetch_column nb:NB_Global nb_cfg)"
+
+check ovn-nbctl --wait=sb lsp-add-localnet-port ls1 ln1 phys
+
+OVS_WAIT_UNTIL([test $(as hv1 ovn-appctl -t ovn-controller 
inc-engine/show-stats runtime_data compute) -gt $runtime_data_runs])
+check_row_count Chassis_Private 1 name=hv1 nb_cfg="${nb_cfg}"

-# Controller should now have created the patch port, but is not yet notified 
of the ofport of those interfaces
+# Controller should now have sent transaction to create the patch port, but it
+# has not yet been received by ovsdb-server.
  sleep_controller hv1
  wake_up_ovsdb hv1
  check as hv1 ovs-vsctl remove Interface vif1 external_ids iface-id
@@ -41746,7 +41741,7 @@ as hv1 check ovs-vsctl set Open_vSwitch . 
external-ids:garp-max-timeout-sec=2

  # Wait until the patch ports are created in hv1 to connect br-int to br-eth0
  check ovn-nbctl set logical_router lr0 options:chassis=hv1
-OVN_WAIT_PATCH_PORT_FLOWS(["ln_port"], ["hv1"])
+check ovn-nbctl --wait=hv sync

  # sleep for 12s to get a garp every ~ 2s
  sleep 12
@@ -44707,7 +44702,7 @@ AT_CHECK([as hv1 ovs-ofctl dump-flows br-int 
table=OFTABLE_PHY_TO_LOG | grep -v
  # Add localnet port to sw0
  check ovn-nbctl --wait=hv lsp-add-localnet-port sw0 ln-sw0 physnet1 -- set 
logical_switch_port ln-sw0 tag_request=100

-OVN_WAIT_PATCH_PORT_FLOWS(["ln-sw0"], ["hv1"])
+check ovn-nbctl --wait=hv sync
  AT_CHECK([as hv1 ovs-ofctl dump-flows br-int table=OFTABLE_PHY_TO_LOG | grep -v NXST_FLOW | grep 
"in_port=" | grep -v "in_port=$of1" | grep -v "in_port=$of2" | wc -l], [0], [dnl
  3
  ])
@@ -47968,3 +47963,79 @@ AT_CHECK([grep "skipping output to input port" \
  OVN_CLEANUP([hv1])
  AT_CLEANUP
  ])
+
+OVN_FOR_EACH_NORTHD([
+AT_SETUP([--wait=hv waits for patch port flows])
+
+ovn_start
+
+check ovn-nbctl ls-add ls1
+net_add n
+
+sim_add hv1
+as hv1
+check ovs-vsctl add-br br-phys
+check ovs-vsctl set open . external-ids:ovn-bridge-mappings=phys:br-phys
+ovn_attach n br-phys 192.168.0.1
+
+check ovn-nbctl lsp-add ls1 lsp1
+check as hv1 ovs-vsctl --no-wait -- add-port br-int vif1 \
+                    -- set Interface vif1 external_ids:iface-id=lsp1 \
+                    -- set Interface vif1 type=internal
+wait_for_ports_up
+check ovn-nbctl --wait=hv sync
+n_flows=$(ovs-ofctl dump-flows br-int | wc -l)
+nb_cfg=$(fetch_column nb:NB_Global nb_cfg)
+
+sleep_ovsdb hv1
+
+# Add a localnet port.  ovn-controller will try to create patch ports, but
+# ovsdb-server is asleep so the transaction cannot commit.
+check ovn-nbctl --wait=sb lsp-add-localnet-port ls1 ln1 phys
+
+# Wait for ovn-controller to process the localnet port change before
+# pausing ovs-vswitchd.
+OVS_WAIT_UNTIL([test "$(ovs-ofctl dump-flows br-int | wc -l)" -gt $n_flows])
+
+# Pause ovs-vswitchd so it cannot assign ofport when ovsdb wakes up.
+sleep_ovs hv1
+wake_up_ovsdb hv1
+
+# ovn-controller is now notified about patch port creation, but
+# ovs-vswitchd is asleep so ofport is not assigned.  --wait=hv must
+# not succeed yet.
+if ! ovn-nbctl --timeout=1 --wait=hv sync; then
+    wake_up_ovs hv1
+fi
+
+check ovn-nbctl --wait=hv sync
+
+# Verify that nb_cfg has advanced past the value from before the localnet port
+# was added.
+check_row_count Chassis_Private 1 name=hv1 nb_cfg\>${nb_cfg}
+
+# Pause ovn-controller so we can inspect the flow table.
+sleep_controller hv1
+wake_up_ovs hv1
+
+# Verify that the patch port has an ofport and a corresponding flow.
+check ovs-vsctl wait-until Interface patch-br-int-to-ln1 "ofport>0"
+ofport=$(ovs-vsctl --bare --columns ofport \
+    find Interface name=patch-br-int-to-ln1)
+AT_CHECK([ovs-ofctl dump-flows br-int table=OFTABLE_PHY_TO_LOG \
+    | grep -q in_port=$ofport], [0], [])
+
+wake_up_controller hv1
+
+# Delete the localnet port and verify --wait=hv waits for cleanup.
+check ovn-nbctl --wait=hv lsp-del ln1
+
+# Verify that the patch port and its flow are gone.
+AT_CHECK([ovs-vsctl show | grep -c patch-br-int-to-ln1], [1], [0
+])
+AT_CHECK([ovs-ofctl dump-flows br-int table=OFTABLE_PHY_TO_LOG \
+    | grep in_port=$ofport], [1], [])
+
+OVN_CLEANUP([hv1])
+AT_CLEANUP
+])
diff --git a/tests/system-ovn.at b/tests/system-ovn.at
index 13e62bf9b..3d8a4fd09 100644
--- a/tests/system-ovn.at
+++ b/tests/system-ovn.at
@@ -14211,7 +14211,7 @@ check ovn-nbctl lsp-add public public-rp -- set 
Logical_Switch_Port public-rp \
      -- lsp-set-addresses public-rp router

  # Wait for a flow outputing to patch port, so we know ovn-controller handled 
the patch port creation in OVS.
-OVN_WAIT_PATCH_PORT_FLOWS(["ln1"], ["./"])
+check ovn-nbctl --wait=hv sync

  check ovn-nbctl lrp-add R1 rp-sw0 00:00:01:01:02:03 192.168.1.1/24

--
2.56.0

_______________________________________________
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev



_______________________________________________
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev