Hi Rosemarie
Thanks for the patch.
I am not sure the patch fully fixes the issue.
When a localnet port is added, I think that we have the following steps:
0. ovn-nbctl --wait=hv lsp-add ... ln-port
1. ovn-controller creates patch port in OVSDB.
2a. ovn-controller is notified about patch port creation.
2b. ovs-vswitchd is notified about port creation and assigns an ofport. It
writes the ofport back to OVSDB.
3. ovn-controller is notified about ofport creation. It computes flows
using that ofport and sends the flows plus a barrier to ovs-vswitchd.
4. ovn-controller sees barrier ack from ovs.
Before your patch, nbctl would return after step 1.
After the patch, it might return after step 2a.
I think it should only return after step 4, i.e., when all flows related to
the ln-port have been added.
WDYT?
I've created the following test which I think reproduces a possible race
condition:
OVN_FOR_EACH_NORTHD([
AT_SETUP([XXXX])
ovn_start
check ovn-nbctl ls-add ls1
check ovn-nbctl --wait=sb add Logical-Switch ls1 other_config
vlan-passthru=true
net_add n
sim_add hv1
as hv1
ovs-vsctl add-br br-phys
ovs-vsctl set open . external-ids:ovn-bridge-mappings=phys:br-phys
ovn_attach n br-phys 192.168.0.1
check ovn-nbctl --wait=hv sync
check ovn-nbctl lsp-add ls1 lsp1
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)
sleep_ovsdb hv1
ovn-appctl vlog/set dbg
check ovn-nbctl --wait=sb lsp-add-localnet-port ls1 ln1 phys
# Patch port should be created, but ovsdb sleeping ...
# Wait for ovs-vswitchd to handle flow addition (related to patch port and
unrelated to ofport(patch_port)).
echo "Waiting for $n_flows to increase"
OVS_WAIT_UNTIL([test "$(ovs-ofctl dump-flows br-int | wc -l)" -gt $n_flows])
# Now put ovs-vswitchd on pause, so it does not handle (yet) port creation
when reported by ovsdb.
sleep_ovs hv1
wake_up_ovsdb hv1
# ovn has now been notified about patch port creation, but ovs ofport is
not created yet as ovs is sleeping.
if ! ovn-nbctl --timeout=3 --wait=hv sync; then
# sync fails (as expected as previous lsp-add not full completed).
Resume ovs-vswitchd.
wake_up_ovs hv1
fi
check ovn-nbctl --wait=hv sync
# Sync succeeded. Pause ovn-controller as it should now have handled all
flow creations (as sync succeeded).
sleep_controller hv1
# Wake up ovs-vswitchd in case it was still stopped (so we can check flows).
wake_up_ovs hv1
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], [])
# Finally wake-up controller for proper cleanup.
wake_up_controller hv1
OVN_CLEANUP([hv1])
AT_CLEANUP
])
WDYT?
Thanks
Xavier
On Wed, Sep 23, 2026 at 4:59 AM Rosemarie O'Riorden <[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.
>
> To remedy this issue, functions performing patch port operations now
> report a status, and ovn-controller will not increment nb_cfg if they
> show not to be complete. This allows --wait=hv to actually wait as
> intended.
>
> 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
> Assisted-by: Claude Opus 5, Claude Code
> Signed-off-by: Rosemarie O'Riorden <[email protected]>
> ---
> controller/ovn-controller.c | 30 +++++++----
> controller/patch.c | 101 +++++++++++++++++++++++++-----------
> controller/patch.h | 2 +-
> tests/ovn.at | 21 +++++---
> 4 files changed, 105 insertions(+), 49 deletions(-)
>
> diff --git a/controller/ovn-controller.c b/controller/ovn-controller.c
> index c601f89dc..03d2fab53 100644
> --- a/controller/ovn-controller.c
> +++ b/controller/ovn-controller.c
> @@ -8485,15 +8485,17 @@ 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 = {
> @@ -8617,13 +8619,19 @@ main(int argc, char *argv[])
> * 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);
> + * moved on to by the time the barrier acks.
> + *
> + * Wait until the local OVS database matches the patch
> + * ports we need. Until then, the flows that use
> those
> + * ports can't be installed, so reporting nb_cfg would
> + * claim the configuration is applied before it is. */
> + if (patch_ports_synced) {
> + 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..7238b8222 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,
> @@ -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) {
> + goto exit;
> + }
> +
> 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,8 @@ 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);
> +exit:
> + return true;
> }
>
> static void
> @@ -165,7 +174,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 +192,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 +241,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
> + * the local bridge mappings. Removes any patch ports, for bridge
> mappings
> + * that 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 +281,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 +294,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 +319,14 @@ get_port(struct ovsdb_idl_index *ovsrec_port_by_name,
> const char *name)
> return port;
> }
>
> -void
> +/* 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. */
> +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 +336,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"
> @@ -335,9 +372,14 @@ patch_run(struct ovsdb_idl_txn *ovs_idl_txn,
> /* 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 (!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 +392,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.at b/tests/ovn.at
> index 6e05809f4..9445c40e3 100644
> --- a/tests/ovn.at
> +++ b/tests/ovn.at
> @@ -40238,14 +40238,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
> -
> -# Controller should now have created the patch port, but is not yet
> notified of the ofport of those interfaces
> +# 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 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
> --
> 2.55.0
>
>
_______________________________________________
dev mailing list
[email protected]
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
