Hi Rosemarie, Dumitru Thanks for the patch.
In general, I think the patch fixes the issue described in the JIRA ticket. I've a few general questions, so I'll write them here. - Do not we have the same issues with geneve tunnels when a geneve tunnel is created, after a second hypervisor has been added ? - While this is/might be a different patch/JIRA issue, I think ovn-nbctl --wait=hv sync also does not work properly in other conf.db cases e.g., when adding a mirror, wait=hv returns as soon as ovn-controller run, while ovs-vswitchd might not have run at all. So any check (e.g. packet) might fail if it expects the mirror to be fully installed when wait=hv returns. I had in mind the use of next_cfg/cur_cfg as a more general potential fix, but this is more complex and riskier. WDYT ? This might be seen as a different issue. As catched by Claude, there is an interaction between this patch and daemon_started_recently: if ovn is restarted, patch port deletion is postponed. So right after startup, ovn-nbctl --wait=hv might be delayed longer than expected - until the startup-delay/count elapses. Is this intentional? Regarding testing, I think we should: - Add a test checking whether ovn-nbctl --wait=hv lsp-del works properly (this was the failing case reported by ther JIRA). - Maybe remove/replace OVN_WAIT_PATCH_PORT_FLOWS by check ovn-nbctl --wait=hv in the tests. That macro was usually added as a test workaround for this issue. Thanks Xavier On Tue, Oct 6, 2026 at 12:17 PM Dumitru Ceara <[email protected]> wrote: > 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
