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
