Recheck-request: github-robot-_Build_and_Test
On Fri, Mar 20, 2026 at 9:50 AM Jacob Tanenbaum <[email protected]> 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]> > > diff --git a/controller/if-status.c b/controller/if-status.c > index ee9337e63..a9f31fe50 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,18 @@ 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) { > + ovs_iface_set_state(mgr, iface, OIF_INSTALL_FLOWS); > + 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 +621,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 +630,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 +665,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 +700,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 +711,34 @@ 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; > > + bool sb_missing_update = false; > + if (local_datapaths) { > + const struct sbrec_port_binding *pb = > + sbrec_port_binding_table_get_for_uuid(pb_table, > + &iface->pb_uuid); > + const struct local_datapath *ld; > + HMAP_FOR_EACH (ld, hmap_node, local_datapaths) { > + if (uuid_equals(&ld->datapath->header_.uuid, > + &pb->datapath->header_.uuid) && > + ld->expected_cond_seqno > ovnsb_cond_seqno) { > + > + sb_missing_update = true; > + break; > + } > + } > + } > + if (sb_missing_update) { > + continue; > + } > + > 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..01dd9516a 100644 > --- a/controller/local_data.h > +++ b/controller/local_data.h > @@ -65,6 +65,9 @@ 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; > }; > > struct local_datapath *local_datapath_alloc( > diff --git a/controller/ovn-controller.c b/controller/ovn-controller.c > index 4161fe2b3..81f6bdb47 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->expected_cond_seqno == 0) { > + 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,8 @@ main(int argc, char *argv[]) > ovs_idl_loop.idl), > sbrec_port_binding_table_get( > ovnsb_idl_loop.idl), > + &runtime_data->local_datapaths, > + 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..f3a83176f 100644 > --- a/tests/ovn-controller.at > +++ b/tests/ovn-controller.at > @@ -3944,3 +3944,68 @@ 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 > + # tavles 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 woekn 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 > > _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
