Hi Dumitru and Xavier, thank you for the reviews! Btw I accidentally removed a test fix from my v1 that caused the CI to be completely red, so I will add that back in v3.
And yes Dumitru, you're right that the checking for ofport numbers should just be handled in patch.c. I had a big misunderstanding but after reading your review and doing more research I cleared it up, so thanks! That will be easy to fix in a v3. On 10/6/26 7:24 AM, Dumitru Ceara wrote: > 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. I agree with both of you that this same issue affects much more than just patch ports, and it would be nice to have a more general solution that fixes all of them at once. I guess for now I can post a v3 of this patch, and then plan future work to implement this broader change. It seems a little riskier as you said. The main thing I'm thinking about is how ovs-vsctl sets cur_cfg = next_cfg even when the patch port creation failed, and they have an ofport number of -1. So I would still have to check for their successful creation, even if the general solution is implemented, or alter this behavior in OVS. I'm sure other things would come up too, but I'm not sure at the moment what other big risks there would be, other than vswitchd hanging and not bumping cur_cfg, but that would be an issue for much more than just this change, so I don't think that's super relevant. What do you guys think about this? > >> >> 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. Ack. > >> 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. Ok, that makes sense to me. Thanks for catching that. > > 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. I will post a v3, but if you have any other thoughts in the meantime feel free to add them here, unless I've posted v3 by then. Thank you both! > > 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 >>> >>> >> > -- Rosemarie O'Riorden Boston & Lowell, MA, USA [email protected] _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
