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 not to be complete. This allows --wait=hv to actually wait as
intended.
Fixes: 84748d0 ("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]>
---
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 | 68 +++++++++++++-----
controller/patch.c | 138 +++++++++++++++++++++++++-----------
controller/patch.h | 5 +-
tests/ovn.at | 66 +++++++++++++++++
4 files changed, 218 insertions(+), 59 deletions(-)
diff --git a/controller/ovn-controller.c b/controller/ovn-controller.c
index d57ff316d..cbe21cf44 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,51 @@ 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);
+ /* Check if the patch ports have been assigned ofport
+ * numbers by OVS. */
+ struct shash patch_ports = SHASH_INITIALIZER(&patch_ports);
+ find_patch_ports(ovsrec_port_by_name, br_int,
+ &patch_ports);
+ bool patch_ports_installed = true;
+ struct shash_node *port_node;
+ SHASH_FOR_EACH_SAFE (port_node, &patch_ports) {
+ const struct ovsrec_port *port = port_node->data;
+ for (size_t i = 0; i < port->n_interfaces; i++) {
+ if (port->interfaces[i]->n_ofport) {
+ if (*(port->interfaces[i]->ofport) < 1) {
+ /* ofport is 0 (not yet assigned)
+ * or -1 (failed). */
+ patch_ports_installed = false;
+ break;
+ }
+ } else {
+ /* OVS is not aware of this port yet. */
+ patch_ports_installed = false;
+ break;
+ }
+ }
+ if (!patch_ports_installed) {
+ break;
+ }
+ }
+ shash_destroy(&patch_ports);
+
+ /* 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_installed && 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..0db3e375a 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) {
+ 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 *
@@ -286,26 +319,14 @@ get_port(struct ovsdb_idl_index *ovsrec_port_by_name,
const char *name)
}
void
-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,
- const struct ovsrec_open_vswitch_table *ovs_table,
- struct ovsdb_idl_index *ovsrec_port_by_name,
- const struct ovsrec_bridge *br_int,
- const struct sbrec_chassis *chassis,
- const struct hmap *local_datapaths)
+find_patch_ports(struct ovsdb_idl_index *ovsrec_port_by_name,
+ const struct ovsrec_bridge *br_int,
+ struct shash *existing_ports)
{
- 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"
+ /* ovn-controller does not create or use ports of type "ovn-l3gateway-port"
* or "ovn-logical-patch-port", but older version did. We still recognize
- * them here, so that we delete them at the end of this function, to avoid
- * leaving useless ports on upgrade. */
- struct shash existing_ports = SHASH_INITIALIZER(&existing_ports);
+ * them here, so they can be marked for deletion, to avoid leaving useless
+ * ports on upgrade. */
const struct ovsrec_port *port;
for (size_t i = 0; i < br_int->n_ports; i++) {
port = br_int->ports[i];
@@ -313,7 +334,7 @@ patch_run(struct ovsdb_idl_txn *ovs_idl_txn,
|| smap_get(&port->external_ids, "ovn-l2gateway-port")
|| smap_get(&port->external_ids, "ovn-l3gateway-port")
|| smap_get(&port->external_ids, "ovn-logical-patch-port")) {
- shash_add(&existing_ports, port->name, port);
+ shash_add(existing_ports, port->name, port);
/* Also add peer ports to the list. */
for (size_t j = 0; j < port->n_interfaces; j++) {
struct ovsrec_interface *p_iface = port->interfaces[j];
@@ -325,19 +346,51 @@ patch_run(struct ovsdb_idl_txn *ovs_idl_txn,
const struct ovsrec_port *peer_port =
get_port(ovsrec_port_by_name, peer_name);
if (peer_port) {
- shash_add(&existing_ports, peer_port->name, peer_port);
+ shash_add(existing_ports, peer_port->name, peer_port);
}
}
}
}
}
+}
+
+/* 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,
+ const struct ovsrec_open_vswitch_table *ovs_table,
+ struct ovsdb_idl_index *ovsrec_port_by_name,
+ const struct ovsrec_bridge *br_int,
+ const struct sbrec_chassis *chassis,
+ const struct hmap *local_datapaths)
+{
+ if (!ovs_idl_txn) {
+ return true;
+ }
+
+ /* Figure out what patch ports already exist. */
+ struct shash existing_ports = SHASH_INITIALIZER(&existing_ports);
+ const struct ovsrec_port *port;
+ find_patch_ports(ovsrec_port_by_name, br_int, &existing_ports);
/* 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. */
@@ -355,4 +408,5 @@ patch_run(struct ovsdb_idl_txn *ovs_idl_txn,
}
}
shash_destroy(&existing_ports);
+ return synced;
}
diff --git a/controller/patch.h b/controller/patch.h
index db4a888e6..28d0895d7 100644
--- a/controller/patch.h
+++ b/controller/patch.h
@@ -36,7 +36,10 @@ 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,
+void find_patch_ports(struct ovsdb_idl_index *ovsrec_port_by_name,
+ const struct ovsrec_bridge *br_int,
+ struct shash *existing_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 *,
const struct ovsrec_open_vswitch_table *,
diff --git a/tests/ovn.at b/tests/ovn.at
index 136b825f9..f818d3d45 100644
--- a/tests/ovn.at
+++ b/tests/ovn.at
@@ -47624,3 +47624,69 @@ 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
+OVN_CLEANUP([hv1])
+AT_CLEANUP
+])
--
2.55.0
_______________________________________________
dev mailing list
[email protected]
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
