On 10/7/26 10:15 PM, Rosemarie O'Riorden wrote: > Hi Dumitru and Xavier, thank you for the reviews! >
Hi Rosemarie, > 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. > Cool! > 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. > Good point. But can't we be more conservative and do both? Wait for a non-zero ofport and then wait for another seqno->ovs round-trip? > 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? > Anyhow, all this would be a follow-up patch in my opinion. >> >>> >>> 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. > I don't have more at the moment. :) > Thank you both! > Thanks for working on this! Regards, Dumitru >> >> 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
