On 10/3/26 12:16 AM, Rosemarie O'Riorden via dev 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 not to be complete. This allows --wait=hv to actually wait as
> intended.
>
Hi Rosemarie, Xavier,
Thanks for the fix and reviews until now! Please see some comments from
my side.
> 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. */
I'm confused a bit about why patch_run() can't just return false (we
call it above) if some OF ports have no assigned port number.
> + 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);
This "inline" loop here makes the already extremely hard to follow code
look even scarier than it did.
But if you change it so that patch_run() returns false in this case I
guess we don't need it anymore.
> +
> + /* 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;
Regards,
Dumitru
_______________________________________________
dev mailing list
[email protected]
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
