On Tue, Apr 14, 2026 at 3:51 PM Jacob Tanenbaum <[email protected]> wrote:
> When ovn-monitor-all is set to false ovn-controller sets ovn-installed > on OVS interfaces too early. ovn-controller needs to wait for the > response from the southbound database with the updates to the newly > monitored fields only then can it install flows and label the OVS > interface as installed. > > Reported-at: https://redhat.atlassian.net/browse/FDP-2887 > Signed-off-by: Jacob Tanenbaum <[email protected]> > > --- > v5->v6 > * simplified the logic as requested, Ales saw that we did not need to > save the seqno if we checked before the engine run. > * removing the extra state from the state machine > * removing some leftover logic that was seen. > > v4->v5 > * corrected a sanitizer error: used bool update_seqno without > initializing > > v3->v4 > * Added state OIF_WAITING_SB_COND to the state machine that manages > the adding of interfaces. This state waits until the Southbound has > updated the ovn-controller of relevent information about ports related > to it > * Addressed several nits > > v2->v3 > * adding the ld->monitor_updated required the additiona of checking for > monitor_all in update_sb_monitors. I didn't account for being able to > toggle on monitor_all > > v1->v2 > * if_status_mgr_run() will run everytime the conditional seqno is > changed so it should be safe to only skip when the expected_seqno and > seqno returned from ovn are strictly not equal, that way we do not > have to deal with overflow in the seqno. Additionally add a boolean to > the local_datapath in the event that the seqno wraps around at the > same time the datapath would go back into the state OIF_INSTALL_FLOWS. > * remove setting the state to itself for OIF_INSTALL_FLOWS in > if_status_mgr_update() > * added assert(pb) in if_status_mgr_run() > * removed a manual loop looking for the local_datapath and replaced with > get_local_datapath() in if_status_mgr_run > * remove a few nit spelling errors in the test case > > diff --git a/controller/if-status.c b/controller/if-status.c > index ee9337e63..df5580422 100644 > --- a/controller/if-status.c > +++ b/controller/if-status.c > @@ -18,6 +18,7 @@ > #include "binding.h" > #include "if-status.h" > #include "lib/ofctrl-seqno.h" > +#include "local_data.h" > #include "ovsport.h" > #include "simap.h" > > @@ -115,6 +116,7 @@ static const char *if_state_names[] = { > * | | | > | | | > * | | | mgr_update(when sb is rw i.e. pb->chassis) > | | | > * | | | has been updated > | | | > + * | | | and the sb has the required info > | | | > * | | release_iface | - request seqno > | | | > * | | | > | | | > * | | V > | | | > @@ -500,6 +502,7 @@ if_status_mgr_update(struct if_status_mgr *mgr, > const struct sbrec_chassis *chassis_rec, > const struct ovsrec_interface_table *iface_table, > const struct sbrec_port_binding_table *pb_table, > + const struct hmap *local_datapaths, > bool ovs_readonly, > bool sb_readonly) > { > @@ -622,9 +625,23 @@ if_status_mgr_update(struct if_status_mgr *mgr, > * in if_status_handle_claims or if_status_mgr_claim_iface > */ > if (iface->is_vif) { > - ovs_iface_set_state(mgr, iface, OIF_INSTALL_FLOWS); > - iface->install_seqno = mgr->iface_seqno + 1; > - new_ifaces = true; > + if (local_datapaths) { > + const struct sbrec_port_binding *pb = > + sbrec_port_binding_table_get_for_uuid(pb_table, > + > &iface->pb_uuid); > + ovs_assert(pb); > + struct local_datapath *ld = > + get_local_datapath(local_datapaths, > + pb->datapath->tunnel_key); > + if (!ld) { > + continue; > + } > + if (ld->is_sb_updated) { > + ovs_iface_set_state(mgr, iface, OIF_INSTALL_FLOWS); > + iface->install_seqno = mgr->iface_seqno + 1; > + new_ifaces = true; > + } > +} > } else { > ovs_iface_set_state(mgr, iface, OIF_MARK_UP); > } > diff --git a/controller/if-status.h b/controller/if-status.h > index d15ca3008..b8f1d6a2e 100644 > --- a/controller/if-status.h > +++ b/controller/if-status.h > @@ -43,6 +43,7 @@ void if_status_mgr_update(struct if_status_mgr *, struct > local_binding_data *, > const struct sbrec_chassis *chassis, > const struct ovsrec_interface_table > *iface_table, > const struct sbrec_port_binding_table *pb_table, > + const struct hmap *local_datapaths, > bool ovs_readonly, > bool sb_readonly); > void if_status_mgr_run(struct if_status_mgr *mgr, struct > local_binding_data *, > diff --git a/controller/local_data.h b/controller/local_data.h > index 948c1a935..2ca00c1fb 100644 > --- a/controller/local_data.h > +++ b/controller/local_data.h > @@ -53,6 +53,7 @@ struct local_datapath { > const struct sbrec_datapath_binding *datapath; > bool is_switch; > bool is_transit_switch; > + bool is_sb_updated; > > /* The localnet port in this datapath, if any (at most one is > allowed). */ > const struct sbrec_port_binding *localnet_port; > diff --git a/controller/ovn-controller.c b/controller/ovn-controller.c > index da43051ed..2964c54d2 100644 > --- a/controller/ovn-controller.c > +++ b/controller/ovn-controller.c > @@ -7842,6 +7842,21 @@ main(int argc, char *argv[]) > > bool recompute_allowed = (ovnsb_idl_txn && > !ofctrl_has_backlog()); > + > + if (runtime_data && > + ((ovnsb_cond_seqno == > ovnsb_expected_cond_seqno && > + ovnsb_expected_cond_seqno != UINT_MAX) || > + sb_monitor_all)) { > + > + struct local_datapath *ld; > + HMAP_FOR_EACH_SAFE (ld, > + hmap_node, > + > &runtime_data->local_datapaths) { > + > + ld->is_sb_updated = true; > + } > + } > + > engine_run(recompute_allowed); > tracked_acl_ids = engine_get_data(&en_acl_id); > > @@ -7973,6 +7988,21 @@ main(int argc, char *argv[]) > * a continuous reason for monitor > updates. */ > daemon_started_recently_countdown(); > } > + > + /* If sb_monitor_all is true we are > monitoring all > + * the southbound database tables already and > the > + * needed data will be avalible already */ > + if (sb_monitor_all) { > + struct local_datapath *ld; > + struct hmap *local_datapaths = > + &runtime_data->local_datapaths; > + > + HMAP_FOR_EACH (ld, > + hmap_node, > + local_datapaths) { > + ld->is_sb_updated = true; > + } > + } > } > /* If there is no new expected seqno we have > finished > * loading all needed data from southbound. We > then > @@ -8017,6 +8047,9 @@ main(int argc, char *argv[]) > ovs_idl_loop.idl), > sbrec_port_binding_table_get( > ovnsb_idl_loop.idl), > + runtime_data ? > + > &runtime_data->local_datapaths > + : NULL, > !ovs_idl_txn, > !ovnsb_idl_txn); > stopwatch_stop(IF_STATUS_MGR_UPDATE_STOPWATCH_NAME, > diff --git a/tests/ovn-controller.at b/tests/ovn-controller.at > index c98de9bc4..9dc7555ba 100644 > --- a/tests/ovn-controller.at > +++ b/tests/ovn-controller.at > @@ -3944,3 +3944,69 @@ OVN_CLEANUP([hv1], [hv2 > /already has encap ip.*cannot duplicate on/d]) > AT_CLEANUP > ]) > + > +OVN_FOR_EACH_NORTHD([ > +AT_SETUP([ovn-installed]) > +ovn_start > + > +net_add n1 > +sim_add hv1 > + > +as hv1 > +ovs-vsctl add-br br-phys > +ovn_attach n1 br-phys 192.168.0.1 > +ovn-appctl vlog/set dbg > +ovs-vsctl add-port br-int vif1 -- \ > + set Interface vif1 external-ids:iface-id=lsp1 > + > +check ovn-nbctl ls-add ls1 > +sleep_controller hv1 > +check ovn-nbctl --wait=sb lsp-add ls1 lsp1 -- \ > + lsp-set-addresses lsp1 "f0:00:00:00:00:01 > 10.0.0.1" > + > +sleep_sb > +wake_up_controller hv1 > + > +# Wait for pflow for lsp1 > +OVS_WAIT_UNTIL([ > + ofport=$(as hv1 ovs-vsctl --bare --columns ofport find Interface > name=vif1) > + echo "vif1 port=$ofport" > + test -n "$ofport" && test 1 -le $(as hv1 ovs-ofctl dump-flows br-int > | grep -c in_port=$ofport) > +]) > + > +# If ovn-installed in ovs, all flows should be installed. > +# In that case, there should be at least one flow with lsp1 address. > +OVS_WAIT_UNTIL([ > + ovn_installed=$(as hv1 ovs-vsctl get Interface vif1 > external_ids:ovn-installed) > + echo $ovn_installed > + flow_count=$(as hv1 ovs-ofctl dump-flows br-int | grep -Fc "10.0.0.1") > + # for the monitor-all=true case the flow gets installed because > ovn-controller is monitoring all > + # tables in OVN_SOUTHBOUND. > + if test -n "$ovn_installed"; then > + as hv1 ovs-ofctl dump-flows br-int > output > + test $flow_count -ge 1 > + else > + true > + fi > +]) > + > +wake_up_sb > +# After the southbound db has woken up and can send the update to the > +# ovn-controller not monitoring all tables in the southbound db it > +# should be able to install the interface. > +OVS_WAIT_UNTIL([ > + ovn_installed=$(as hv1 ovs-vsctl get Interface vif1 > external_ids:ovn-installed) > + flow_count=$(as hv1 ovs-ofctl dump-flows br-int | grep -Fc "10.0.0.1") > + echo "installed=$ovn_installed, count=$flow_count" > + if test -n "$ovn_installed"; then > + as hv1 ovs-ofctl dump-flows br-int > output > + test $flow_count -ge 1 > + else > + false > + fi > +]) > +wait_for_ports_up > + > +OVN_CLEANUP([hv1]) > +AT_CLEANUP > +]) > -- > 2.53.0 > > Moving v6 into Changes Requested based on the offline discussion. Thanks, Ales _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
