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 _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
