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 <[email protected]> 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 <[email protected]> > --- > 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? > @@ -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 > [email protected] > https://mail.openvswitch.org/mailman/listinfo/ovs-dev > _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
