| Message ID | 20261002221640.3151217-1-rosemarie@redhat.com |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | [ovs-dev,v2] controller: Wait for patch ports before nb_cfg. | expand |
| Context | Check | Description |
|---|---|---|
| ovsrobot/apply-robot | warning | apply and check: warning |
| ovsrobot/github-robot-_Build_and_Test | fail | github build: failed |
| ovsrobot/github-robot-_ovn-kubernetes | success | github build: passed |
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 <rosemarie@redhat.com> > --- > 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
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 <dceara@redhat.com> 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 <rosemarie@redhat.com> > > --- > > 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 > >
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 <dceara@redhat.com> 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 <rosemarie@redhat.com> >>> --- >>> 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 >> >> >
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 <dceara@redhat.com> 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 <rosemarie@redhat.com> >>>> --- >>>> 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 >>> >>> >> >
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 <dceara@redhat.com> 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 <rosemarie@redhat.com> >>>>> --- >>>>> 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 >>>> >>>> >>> >> >
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. */ + 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); + + /* 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; diff --git a/controller/patch.c b/controller/patch.c index 4fed6e375..0db3e375a 100644 --- a/controller/patch.c +++ b/controller/patch.c @@ -71,10 +71,13 @@ match_patch_port(const struct ovsrec_port *port, const char *peer) /* Creates a patch port in bridge 'src' named 'src_name', whose peer is * 'dst_name' in bridge 'dst'. Initializes the patch port's external-ids:'key' - * to 'key'. + * to 'key'. The port is only created if 'ovs_idl_txn' is non-NULL. * - * If such a patch port already exists, removes it from 'existing_ports'. */ -static void + * If such a patch port already exists in 'src', removes it from + * 'existing_ports' and returns false. + * + * Otherwise, creates port (if 'ovs_idl_txn'), and returns true. */ +static bool create_patch_port(struct ovsdb_idl_txn *ovs_idl_txn, const char *key, const char *value, const struct ovsrec_bridge *src, const char *src_name, @@ -85,10 +88,14 @@ create_patch_port(struct ovsdb_idl_txn *ovs_idl_txn, if (match_patch_port(src->ports[i], dst_name)) { /* Patch port already exists on 'src'. */ shash_find_and_delete(existing_ports, src->ports[i]->name); - return; + return false; } } + if (!ovs_idl_txn) { + return true; + } + ovsdb_idl_txn_add_comment(ovs_idl_txn, "ovn-controller: creating patch port '%s' from '%s' to '%s'", src_name, src->name, dst->name); @@ -97,6 +104,7 @@ create_patch_port(struct ovsdb_idl_txn *ovs_idl_txn, const struct smap port_ids = SMAP_CONST1(&port_ids, key, value); ovsport_create(ovs_idl_txn, src, src_name, "patch", &port_ids, NULL, &if_options, 0); + return true; } static void @@ -165,7 +173,12 @@ add_ovs_bridge_mappings(const struct ovsrec_open_vswitch_table *ovs_table, } } -static void +/* Adds the patch ports needed by the port bindings of type 'pb_type' that are + * local to this chassis. + * + * Returns true if all of those patch ports are already present in the + * database. */ +static bool add_bridge_mappings_by_type(struct ovsdb_idl_txn *ovs_idl_txn, struct ovsdb_idl_index *sbrec_port_binding_by_type, const struct ovsrec_bridge *br_int, @@ -178,6 +191,8 @@ add_bridge_mappings_by_type(struct ovsdb_idl_txn *ovs_idl_txn, { struct sbrec_port_binding *target = sbrec_port_binding_index_init_row(sbrec_port_binding_by_type); + bool synced = true; + sbrec_port_binding_index_set_type(target, pb_type); const struct sbrec_port_binding *binding; @@ -225,20 +240,32 @@ add_bridge_mappings_by_type(struct ovsdb_idl_txn *ovs_idl_txn, char *name1 = patch_port_name(br_int->name, binding->logical_port); char *name2 = patch_port_name(binding->logical_port, br_int->name); - create_patch_port(ovs_idl_txn, patch_port_id, binding->logical_port, - br_int, name1, br_ln, name2, existing_ports); - create_patch_port(ovs_idl_txn, patch_port_id, binding->logical_port, - br_ln, name2, br_int, name1, existing_ports); + bool br_int_port_exists = + !create_patch_port(ovs_idl_txn, patch_port_id, + binding->logical_port, + br_int, name1, br_ln, name2, + existing_ports); + bool br_ln_port_exists = + !create_patch_port(ovs_idl_txn, patch_port_id, + binding->logical_port, + br_ln, name2, br_int, name1, + existing_ports); + + synced = synced && br_int_port_exists && br_ln_port_exists; free(name1); free(name2); } sbrec_port_binding_index_destroy_row(target); + return synced; } /* Obtains external-ids:ovn-bridge-mappings from OVSDB and adds patch ports for * the local bridge mappings. Removes any patch ports for bridge mappings that - * already existed from 'existing_ports'. */ -static void + * already existed from 'existing_ports'. + * + * Returns true if all of the required patch ports are already present in the + * database. */ +static bool add_bridge_mappings(struct ovsdb_idl_txn *ovs_idl_txn, struct ovsdb_idl_index *sbrec_port_binding_by_type, const struct ovsrec_bridge_table *bridge_table, @@ -253,10 +280,12 @@ add_bridge_mappings(struct ovsdb_idl_txn *ovs_idl_txn, add_ovs_bridge_mappings(ovs_table, bridge_table, &bridge_mappings); - add_bridge_mappings_by_type(ovs_idl_txn, sbrec_port_binding_by_type, - br_int, existing_ports, chassis, - &bridge_mappings, "l2gateway", - "ovn-l2gateway-port", local_datapaths, true); + bool l2gateway_synced = + add_bridge_mappings_by_type(ovs_idl_txn, sbrec_port_binding_by_type, + br_int, existing_ports, chassis, + &bridge_mappings, "l2gateway", + "ovn-l2gateway-port", local_datapaths, + true); /* Since having localnet ports that are not mapped on some chassis is a * supported configuration used to implement multisegment switches with @@ -264,11 +293,15 @@ add_bridge_mappings(struct ovsdb_idl_txn *ovs_idl_txn, * run but don't unnecessarily pollute the log file; pass * 'log_missing_bridge = false'. */ - add_bridge_mappings_by_type(ovs_idl_txn, sbrec_port_binding_by_type, - br_int, existing_ports, NULL, - &bridge_mappings, "localnet", - "ovn-localnet-port", local_datapaths, false); + bool localnet_synced = + add_bridge_mappings_by_type(ovs_idl_txn, sbrec_port_binding_by_type, + br_int, existing_ports, NULL, + &bridge_mappings, "localnet", + "ovn-localnet-port", local_datapaths, + false); + shash_destroy(&bridge_mappings); + return l2gateway_synced && localnet_synced; } static const struct ovsrec_port * @@ -286,26 +319,14 @@ get_port(struct ovsdb_idl_index *ovsrec_port_by_name, const char *name) } void -patch_run(struct ovsdb_idl_txn *ovs_idl_txn, - struct ovsdb_idl_index *sbrec_port_binding_by_type, - const struct ovsrec_bridge_table *bridge_table, - const struct ovsrec_open_vswitch_table *ovs_table, - struct ovsdb_idl_index *ovsrec_port_by_name, - const struct ovsrec_bridge *br_int, - const struct sbrec_chassis *chassis, - const struct hmap *local_datapaths) +find_patch_ports(struct ovsdb_idl_index *ovsrec_port_by_name, + const struct ovsrec_bridge *br_int, + struct shash *existing_ports) { - if (!ovs_idl_txn) { - return; - } - - /* Figure out what patch ports already exist. - * - * ovn-controller does not create or use ports of type "ovn-l3gateway-port" + /* ovn-controller does not create or use ports of type "ovn-l3gateway-port" * or "ovn-logical-patch-port", but older version did. We still recognize - * them here, so that we delete them at the end of this function, to avoid - * leaving useless ports on upgrade. */ - struct shash existing_ports = SHASH_INITIALIZER(&existing_ports); + * them here, so they can be marked for deletion, to avoid leaving useless + * ports on upgrade. */ const struct ovsrec_port *port; for (size_t i = 0; i < br_int->n_ports; i++) { port = br_int->ports[i]; @@ -313,7 +334,7 @@ patch_run(struct ovsdb_idl_txn *ovs_idl_txn, || smap_get(&port->external_ids, "ovn-l2gateway-port") || smap_get(&port->external_ids, "ovn-l3gateway-port") || smap_get(&port->external_ids, "ovn-logical-patch-port")) { - shash_add(&existing_ports, port->name, port); + shash_add(existing_ports, port->name, port); /* Also add peer ports to the list. */ for (size_t j = 0; j < port->n_interfaces; j++) { struct ovsrec_interface *p_iface = port->interfaces[j]; @@ -325,19 +346,51 @@ patch_run(struct ovsdb_idl_txn *ovs_idl_txn, const struct ovsrec_port *peer_port = get_port(ovsrec_port_by_name, peer_name); if (peer_port) { - shash_add(&existing_ports, peer_port->name, peer_port); + shash_add(existing_ports, peer_port->name, peer_port); } } } } } +} + +/* Adds to the local OVS database the patch ports required by the localnet + * and l2gateway ports that are local to this chassis and removes the ones + * that are no longer needed. The database is only updated if 'ovs_idl_txn' + * is non-NULL. + * + * Returns true if the database already reflects the required set of patch + * ports. */ +bool +patch_run(struct ovsdb_idl_txn *ovs_idl_txn, + struct ovsdb_idl_index *sbrec_port_binding_by_type, + const struct ovsrec_bridge_table *bridge_table, + const struct ovsrec_open_vswitch_table *ovs_table, + struct ovsdb_idl_index *ovsrec_port_by_name, + const struct ovsrec_bridge *br_int, + const struct sbrec_chassis *chassis, + const struct hmap *local_datapaths) +{ + if (!ovs_idl_txn) { + return true; + } + + /* Figure out what patch ports already exist. */ + struct shash existing_ports = SHASH_INITIALIZER(&existing_ports); + const struct ovsrec_port *port; + find_patch_ports(ovsrec_port_by_name, br_int, &existing_ports); /* Create in the database any patch ports that should exist. Remove from * 'existing_ports' any patch ports that do exist in the database and * should be there. */ - add_bridge_mappings(ovs_idl_txn, sbrec_port_binding_by_type, bridge_table, - ovs_table, br_int, &existing_ports, chassis, - local_datapaths); + bool synced = add_bridge_mappings(ovs_idl_txn, sbrec_port_binding_by_type, + bridge_table, ovs_table, br_int, + &existing_ports, chassis, + local_datapaths); + + if (!shash_is_empty(&existing_ports)) { + synced = false; + } /* Now 'existing_ports' only still contains patch ports that exist in the * database but shouldn't. Delete them from the database. */ @@ -355,4 +408,5 @@ patch_run(struct ovsdb_idl_txn *ovs_idl_txn, } } shash_destroy(&existing_ports); + return synced; } diff --git a/controller/patch.h b/controller/patch.h index db4a888e6..28d0895d7 100644 --- a/controller/patch.h +++ b/controller/patch.h @@ -36,7 +36,10 @@ struct shash; void add_ovs_bridge_mappings(const struct ovsrec_open_vswitch_table *ovs_table, const struct ovsrec_bridge_table *bridge_table, struct shash *bridge_mappings); -void patch_run(struct ovsdb_idl_txn *ovs_idl_txn, +void find_patch_ports(struct ovsdb_idl_index *ovsrec_port_by_name, + const struct ovsrec_bridge *br_int, + struct shash *existing_ports); +bool patch_run(struct ovsdb_idl_txn *ovs_idl_txn, struct ovsdb_idl_index *sbrec_port_binding_by_type, const struct ovsrec_bridge_table *, const struct ovsrec_open_vswitch_table *, diff --git a/tests/ovn.at b/tests/ovn.at index 136b825f9..f818d3d45 100644 --- a/tests/ovn.at +++ b/tests/ovn.at @@ -47624,3 +47624,69 @@ AT_CHECK([grep "skipping output to input port" \ OVN_CLEANUP([hv1]) AT_CLEANUP ]) + +OVN_FOR_EACH_NORTHD([ +AT_SETUP([--wait=hv waits for patch port flows]) + +ovn_start + +check ovn-nbctl ls-add ls1 +net_add n + +sim_add hv1 +as hv1 +check ovs-vsctl add-br br-phys +check ovs-vsctl set open . external-ids:ovn-bridge-mappings=phys:br-phys +ovn_attach n br-phys 192.168.0.1 + +check ovn-nbctl lsp-add ls1 lsp1 +check as hv1 ovs-vsctl --no-wait -- add-port br-int vif1 \ + -- set Interface vif1 external_ids:iface-id=lsp1 \ + -- set Interface vif1 type=internal +wait_for_ports_up +check ovn-nbctl --wait=hv sync +n_flows=$(ovs-ofctl dump-flows br-int | wc -l) +nb_cfg=$(fetch_column nb:NB_Global nb_cfg) + +sleep_ovsdb hv1 + +# Add a localnet port. ovn-controller will try to create patch ports, but +# ovsdb-server is asleep so the transaction cannot commit. +check ovn-nbctl --wait=sb lsp-add-localnet-port ls1 ln1 phys + +# Wait for ovn-controller to process the localnet port change before +# pausing ovs-vswitchd. +OVS_WAIT_UNTIL([test "$(ovs-ofctl dump-flows br-int | wc -l)" -gt $n_flows]) + +# Pause ovs-vswitchd so it cannot assign ofport when ovsdb wakes up. +sleep_ovs hv1 +wake_up_ovsdb hv1 + +# ovn-controller is now notified about patch port creation, but +# ovs-vswitchd is asleep so ofport is not assigned. --wait=hv must +# not succeed yet. +if ! ovn-nbctl --timeout=1 --wait=hv sync; then + wake_up_ovs hv1 +fi + +check ovn-nbctl --wait=hv sync + +# Verify that nb_cfg has advanced past the value from before the localnet port +# was added. +check_row_count Chassis_Private 1 name=hv1 nb_cfg\>${nb_cfg} + +# Pause ovn-controller so we can inspect the flow table. +sleep_controller hv1 +wake_up_ovs hv1 + +# Verify that the patch port has an ofport and a corresponding flow. +check ovs-vsctl wait-until Interface patch-br-int-to-ln1 "ofport>0" +ofport=$(ovs-vsctl --bare --columns ofport \ + find Interface name=patch-br-int-to-ln1) +AT_CHECK([ovs-ofctl dump-flows br-int table=OFTABLE_PHY_TO_LOG \ + | grep -q in_port=$ofport], [0], []) + +wake_up_controller hv1 +OVN_CLEANUP([hv1]) +AT_CLEANUP +])
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. 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 <rosemarie@redhat.com> --- 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(-)