On 10/6/26 1:08 PM, Xavier Simonart wrote: > Hi Rosemarie, Dumitru >
Hi Xavier, > 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 ? I guess this could be handled in a similar way as Rosemarie is doing here for patch ports. > - 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 Right, this seems like a potential candidate for using the ofctrl-seqno mechanism. > ? This might be seen as a different issue. I guess that can be handled as a follow up patch, right. > > 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? > On this front, our docs say: <p> With <code>--wait=hv</code>, before <code>ovn-nbctl</code> exits, it additionally waits for all OVN chassis (hypervisors and gateways) to become up-to-date with the northbound database updates. (This can become an indefinite wait if any chassis is malfunctioning.) </p> So I'm of the opinion that (intentional or not) this is an OK behavior. > 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. +1, removing the macro and replacing its call sites with --wait=hv would be ideal. I'm not sure how many other things this will uncover though. I'm also OK with considering that as follow-up cleanup if it turns out to be a lot of additional work. As it seems that there are some things that need to change anyway in this patch I'll be moving this to "changes requested" in patchwork but we can continue the discussion here or on a v3, whatever you prefer. Regards, Dumitru > > 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
