On 3/26/26 6:43 PM, Jacob Tanenbaum 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,
> 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
>
Thanks for the new revision! I do have some concerns though.
> 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..2114305d1 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;
> + }
This looked very weird to me intially. We unconditionally force
ovn-controller to wait for two "round trips" to OVS now for each new
interface it binds.
That's regardless of ovn-monitor-all=true/false and regardless whether
the datapath was already local or not.
The latency we add might be noticeable in scaled setups. At lower scale
(e.g., in our tests) this is harder to notice because we only bump the
install_seqno here if the SB is not readonly (so if no SB transaction is
in flight). But in practice it might be quite visible.
> 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);
Missing NULL check?
> + if (!ld->monitor_updated &&
> + ld->expected_cond_seqno != ovnsb_cond_seqno) {
> + continue;
This is a bit problematic, and I guess that's exactly the reason why you
had to bump the expected install_seqno above for interfaces in state
OIF_INSTALL_FLOWS.
The problem is that just after this loop we destroy the set of
"acked_seqnos":
ofctrl_acked_seqnos_destroy(acked_seqnos);
And because we didn't "ack" the interfaces whose datapath is not local
the next time we end up here, with new acked_seqnos we would fail to
find the install_seqnos of those interfaces.
I think the idea of delaying declaring "up" for interfaces that are part
of datapaths that are not yet "fully processed" (matching
ovnsb_cond_seqno) is probably fine but we might need to do it in a
different way.
What if we add a new state to if-status, e.g.:
CLAIMED ---> WAITING_SB_COND ---> INSTALL_FLOWS ---> ....
Where the new state is WAITING_SB_COND.
We'd advance to WAITING_SB_COND from CLAIMED only if the interface
datapath is not "fully processed" (sb cond seqno mismatch). Otherwise
advance to INSTALL_FLOWS like we do today.
We only set the expected iface->install_seqno on the transition to
INSTALL_FLOWS.
Like that we wouldn't affect any interfaces that are claimed when the
datapath is already "fully procesed" (or when ovn-monitor-all=true).
> + }
> + 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
> */
Typo: udpated
Nit: s/the expected/The expected/
Nit: missing period.
> + unsigned int expected_cond_seqno;
> + /* If the monitor has been updated for this datapath */
Nit: missing period.
> + bool monitor_updated;
> };
>
> struct local_datapath *local_datapath_alloc(
> diff --git a/controller/ovn-controller.c b/controller/ovn-controller.c
> index 4161fe2b3..6a65d50d9 100644
> --- a/controller/ovn-controller.c
> +++ b/controller/ovn-controller.c
> @@ -443,6 +443,18 @@ out:;
> expected_cond_seqno = MAX(expected_cond_seqno, cond_seqnos[i]);
> }
>
> + if (local_datapaths) {
> + struct local_datapath *ld;
> + HMAP_FOR_EACH (ld, hmap_node, local_datapaths) {
> + if (monitor_all) {
> + ld->monitor_updated = true;
> + }
> + 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 +7915,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.
Nit: s/if/If/
> +# 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
Nit: s/after/After
I'd also split this comment to make it fit in 80 columns.
> +# 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
> +])
Regards,
Dumitru
_______________________________________________
dev mailing list
[email protected]
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
