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
