Hi Jacob, On 3/26/26 6:45 PM, Jacob Tanenbaum wrote: > Sorry I looked at it, and added a check to solve one potential problem > between v1 and v2 and caused another without running all the tests. Sorry > for not rerunning the tests. >
No worries, it can happen, that's why we have CI. :) Regards, Dumitru > Jacob > > On Thu, Mar 26, 2026 at 6:36 AM Dumitru Ceara <[email protected]> wrote: > >> On 3/25/26 5:22 PM, Jacob Tanenbaum via dev wrote: >>> when ovn-monitor-all is set to false the 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 southbound fields and after that wait for flows to be >>> installed in OVS before labeling as installed. >>> >>> Reported-at: https://redhat.atlassian.net/browse/FDP-2887 >>> Signed-off-by: Jacob Tanenbaum <[email protected]> >>> --- >> >> Hi Jacob, >> >> Thanks for the patch. I didn't review it but it seems it causes quite a >> few CI failures: >> >> https://github.com/ovsrobot/ovn/actions/runs/23552721612 >> >> Could you please look into it and post a v3? >> >> Thanks, >> Dumitru >> >>> 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..d3383253b 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" >>> >>> @@ -590,12 +591,17 @@ if_status_mgr_update(struct if_status_mgr *mgr, >>> } >>> } >>> >>> + bool update_seqno = false; >>> /* Update pb->chassis in case it's not set (previous update still >> in fly >>> * or pb->chassis was overwitten by another chassis. >>> */ >>> if (!sb_readonly) { >>> HMAPX_FOR_EACH_SAFE (node, >> &mgr->ifaces_per_state[OIF_INSTALL_FLOWS]) { >>> struct ovs_iface *iface = node->data; >>> + if (iface->is_vif) { >>> + iface->install_seqno = mgr->iface_seqno + 1; >>> + update_seqno = true; >>> + } >>> if (!local_bindings_pb_chassis_is_set(bindings, iface->id, >>> chassis_rec)) { >>> long long int now = time_msec(); >>> @@ -614,7 +620,6 @@ if_status_mgr_update(struct if_status_mgr *mgr, >>> >>> /* Move newly claimed interfaces from OIF_CLAIMED to >> OIF_INSTALL_FLOWS. >>> */ >>> - bool new_ifaces = false; >>> if (!sb_readonly) { >>> HMAPX_FOR_EACH_SAFE (node, &mgr->ifaces_per_state[OIF_CLAIMED]) >> { >>> struct ovs_iface *iface = node->data; >>> @@ -624,7 +629,7 @@ if_status_mgr_update(struct if_status_mgr *mgr, >>> if (iface->is_vif) { >>> ovs_iface_set_state(mgr, iface, OIF_INSTALL_FLOWS); >>> iface->install_seqno = mgr->iface_seqno + 1; >>> - new_ifaces = true; >>> + update_seqno = true; >>> } else { >>> ovs_iface_set_state(mgr, iface, OIF_MARK_UP); >>> } >>> @@ -659,7 +664,7 @@ if_status_mgr_update(struct if_status_mgr *mgr, >>> * Request a seqno update when the flows for new interfaces have >> been >>> * installed in OVS. >>> */ >>> - if (new_ifaces) { >>> + if (update_seqno) { >>> mgr->iface_seqno++; >>> ofctrl_seqno_update_create(mgr->iface_seq_type_pb_cfg, >>> mgr->iface_seqno); >>> @@ -694,6 +699,8 @@ if_status_mgr_run(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, >>> + unsigned int ovnsb_cond_seqno, >>> bool sb_readonly, bool ovs_readonly) >>> { >>> struct ofctrl_acked_seqnos *acked_seqnos = >>> @@ -703,10 +710,27 @@ if_status_mgr_run(struct if_status_mgr *mgr, >>> /* Move interfaces from state OIF_INSTALL_FLOWS to OIF_MARK_UP if a >>> * notification has been received aabout their flows being installed >>> * in OVS. >>> + * >>> + * In the ovn-monitor-all=false case it is possible that we have not >>> + * received the update that the southbound database is monitoring a >> new >>> + * datapath. Check for the update before continuing. >>> */ >>> HMAPX_FOR_EACH_SAFE (node, >> &mgr->ifaces_per_state[OIF_INSTALL_FLOWS]) { >>> struct ovs_iface *iface = node->data; >>> >>> + 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->monitor_updated && >>> + ld->expected_cond_seqno != ovnsb_cond_seqno) { >>> + continue; >>> + } >>> + ld->monitor_updated = true; >>> + } >>> if (!ofctrl_acked_seqnos_contains(acked_seqnos, >>> iface->install_seqno)) { >>> continue; >>> diff --git a/controller/if-status.h b/controller/if-status.h >>> index d15ca3008..a877ebe2b 100644 >>> --- a/controller/if-status.h >>> +++ b/controller/if-status.h >>> @@ -49,6 +49,8 @@ void if_status_mgr_run(struct if_status_mgr *mgr, >> struct local_binding_data *, >>> const struct sbrec_chassis *, >>> const struct ovsrec_interface_table *iface_table, >>> const struct sbrec_port_binding_table *pb_table, >>> + const struct hmap *local_datapaths, >>> + const unsigned int ovnsb_cond_seqno, >>> bool sb_readonly, bool ovs_readonly); >>> void if_status_mgr_get_memory_usage(struct if_status_mgr *mgr, >>> struct simap *usage); >>> diff --git a/controller/local_data.h b/controller/local_data.h >>> index 948c1a935..51dcca416 100644 >>> --- a/controller/local_data.h >>> +++ b/controller/local_data.h >>> @@ -65,6 +65,11 @@ struct local_datapath { >>> >>> struct shash external_ports; >>> struct shash multichassis_ports; >>> + >>> + /* the expected seqno from the sb to be fully udpated for this >> datapath */ >>> + unsigned int expected_cond_seqno; >>> + /* If the monitor has been updated for this datapath */ >>> + bool monitor_updated; >>> }; >>> >>> struct local_datapath *local_datapath_alloc( >>> diff --git a/controller/ovn-controller.c b/controller/ovn-controller.c >>> index 4161fe2b3..89efb5e49 100644 >>> --- a/controller/ovn-controller.c >>> +++ b/controller/ovn-controller.c >>> @@ -443,6 +443,15 @@ out:; >>> expected_cond_seqno = MAX(expected_cond_seqno, cond_seqnos[i]); >>> } >>> >>> + if (!monitor_all && local_datapaths) { >>> + struct local_datapath *ld; >>> + HMAP_FOR_EACH (ld, hmap_node, local_datapaths) { >>> + if (!ld->monitor_updated) { >>> + ld->expected_cond_seqno = expected_cond_seqno; >>> + } >>> + } >>> + } >>> + >>> ovsdb_idl_condition_destroy(&pb); >>> ovsdb_idl_condition_destroy(&lf); >>> ovsdb_idl_condition_destroy(&ldpg); >>> @@ -7903,6 +7912,10 @@ 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, >>> + ovnsb_cond_seqno, >>> !ovnsb_idl_txn, !ovs_idl_txn); >>> stopwatch_stop(IF_STATUS_MGR_RUN_STOPWATCH_NAME, >>> time_msec()); >>> diff --git a/tests/ovn-controller.at b/tests/ovn-controller.at >>> index c98de9bc4..1d0c02290 100644 >>> --- a/tests/ovn-controller.at >>> +++ b/tests/ovn-controller.at >>> @@ -3944,3 +3944,67 @@ 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) >>> + 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 >>> +]) >> >> > _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
