On 5/6/26 4:37 PM, Jacob Tanenbaum wrote:
> 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]>
>
> ---
Hi Jacob,
Thanks for v10!
> v9->v10
> * added a bool for if the controller is in monitor-all
> * removed some code leftover from debugging
> * changed argument to if_status_mgr_update to dps_waiting_for_sb
> * removed ovs_assert in favor of NULL checking
> * removed unrelated new lines
>
> v8->v9
> * added the functionality of waiting for sb to update to the incremental
> processor
>
> v7->v8
> * removed printing in the system testcases that shouldn't be there and
> caused failure.
>
> v6->v7
> * added an sset that holds the uuid's of datapaths that are waiting for
> sb updates
> * added back the the state OIF_WAITING_SB_COND to the if_mgr state
> machine.
> * no longer hold if the datapath is updated in the local_datapaths
> struct as that is generated each transaction and cannot be relied
> upon.
> * removed some leftover logic from previous patch versions
>
> 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..d12ac5515 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"
>
> @@ -58,6 +59,11 @@ VLOG_DEFINE_THIS_MODULE(if_status);
> enum if_state {
> OIF_CLAIMED, /* Newly claimed interface. pb->chassis update not
> yet initiated. */
> + OIF_WAITING_SB_COND, /* Waiting for the Southbound database to update
> + * ovn-controller for a given datapath. We should
> + * only be waiting in this state when monitor_all
> + * is false AND it is the first time that we see
> + * a specific datapath. */
> OIF_INSTALL_FLOWS, /* Claimed interface with pb->chassis update sent
> to
> * SB (but update notification not confirmed, so
> the
> * update may be resent in any of the following
> @@ -87,6 +93,7 @@ enum if_state {
>
> static const char *if_state_names[] = {
> [OIF_CLAIMED] = "CLAIMED",
> + [OIF_WAITING_SB_COND] = "WAITING_SB_COND",
> [OIF_INSTALL_FLOWS] = "INSTALL_FLOWS",
> [OIF_REM_OLD_OVN_INST] = "REM_OLD_OVN_INST",
> [OIF_MARK_UP] = "MARK_UP",
> @@ -114,7 +121,18 @@ static const char *if_state_names[] = {
> * | | | +--+ | |
> |
> * | | | | |
> |
> * | | | mgr_update(when sb is rw i.e. pb->chassis) | |
> |
> - * | | | has been updated | |
> |
> + * | | V has been updated | |
> |
> + * | | +----------------------+ | |
> |
> + * | | | | | |
> |
> + * | | | WAITING_SB_COND | | |
> |
> + * | | | | | |
> |
> + * | | | | | |
> |
> + * | | +----------------------+ | |
> |
> + * | | | | |
> |
> + * | | | | |
> |
> + * | | | mgr_update(when sb_cond_seqno == expected) | |
> |
> + * | | | - request seqno | |
> |
> + * | | | | |
> |
> * | | release_iface | - request seqno | |
> |
> * | | | | |
> |
> * | | V | |
> |
> @@ -335,6 +353,7 @@ if_status_mgr_claim_iface(struct if_status_mgr *mgr,
>
> switch (iface->state) {
> case OIF_CLAIMED:
> + case OIF_WAITING_SB_COND:
> case OIF_INSTALL_FLOWS:
> case OIF_REM_OLD_OVN_INST:
> case OIF_MARK_UP:
> @@ -383,6 +402,7 @@ if_status_mgr_release_iface(struct if_status_mgr *mgr,
> const char *iface_id)
>
> switch (iface->state) {
> case OIF_CLAIMED:
> + case OIF_WAITING_SB_COND:
> case OIF_INSTALL_FLOWS:
> /* Not yet fully installed interfaces:
> * pb->chassis still need to be deleted.
> @@ -424,6 +444,7 @@ if_status_mgr_delete_iface(struct if_status_mgr *mgr,
> const char *iface_id,
>
> switch (iface->state) {
> case OIF_CLAIMED:
> + case OIF_WAITING_SB_COND:
> case OIF_INSTALL_FLOWS:
> /* Not yet fully installed interfaces:
> * pb->chassis still need to be deleted.
> @@ -500,6 +521,8 @@ 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,
> + const struct uuidset *dps_waiting_for_sb,
> bool ovs_readonly,
> bool sb_readonly)
> {
> @@ -510,6 +533,8 @@ if_status_mgr_update(struct if_status_mgr *mgr,
> return;
> }
>
> + ovs_assert(dps_waiting_for_sb);
> +
> struct shash *bindings = &binding_data->bindings;
> struct hmapx_node *node;
>
> @@ -622,9 +647,7 @@ if_status_mgr_update(struct if_status_mgr *mgr,
The comment just above this hunk needs to be updated too, now it says:
/* Move newly claimed interfaces from OIF_CLAIMED to OIF_INSTALL_FLOWS.
> * 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;
> + ovs_iface_set_state(mgr, iface, OIF_WAITING_SB_COND);
> } else {
> ovs_iface_set_state(mgr, iface, OIF_MARK_UP);
> }
> @@ -639,6 +662,33 @@ if_status_mgr_update(struct if_status_mgr *mgr,
> }
> }
>
> + if (!sb_readonly) {
> + HMAPX_FOR_EACH_SAFE (node,
> + &mgr->ifaces_per_state[OIF_WAITING_SB_COND]) {
> + 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);
> + if (!pb) {
> + continue;
> + }
> + struct local_datapath *ld =
> + get_local_datapath(local_datapaths,
> + pb->datapath->tunnel_key);
> + if (!ld) {
> + continue;
> + }
> + if (!uuidset_find(dps_waiting_for_sb,
> + &ld->datapath->header_.uuid)) {
> + ovs_iface_set_state(mgr, iface, OIF_INSTALL_FLOWS);
> + iface->install_seqno = mgr->iface_seqno + 1;
> + new_ifaces = true;
> + }
> + }
> + }
> + }
> +
> if (!sb_readonly) {
> HMAPX_FOR_EACH_SAFE (node, &mgr->ifaces_per_state[OIF_UPDATE_PORT]) {
> struct ovs_iface *iface = node->data;
> diff --git a/controller/if-status.h b/controller/if-status.h
> index d15ca3008..75c7bf71c 100644
> --- a/controller/if-status.h
> +++ b/controller/if-status.h
> @@ -18,6 +18,7 @@
>
> #include "openvswitch/shash.h"
> #include "lib/vswitch-idl.h"
> +#include "lib/uuidset.h"
>
> #include "binding.h"
> #include "lport.h"
> @@ -43,6 +44,8 @@ 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,
> + const struct uuidset *dps_waiting_for_sb,
> bool ovs_readonly,
> bool sb_readonly);
> void if_status_mgr_run(struct if_status_mgr *mgr, struct local_binding_data
> *,
> diff --git a/controller/ovn-controller.c b/controller/ovn-controller.c
> index 35a5cd0b4..3a8eeb4c0 100644
> --- a/controller/ovn-controller.c
> +++ b/controller/ovn-controller.c
> @@ -170,6 +170,8 @@ static char *unixctl_path;
> struct controller_engine_ctx {
> struct lflow_cache *lflow_cache;
> struct if_status_mgr *if_mgr;
> + const unsigned int *ovnsb_expected_cond_seqno;
> + const bool *sb_monitor_all;
> };
>
> /* Pending packet to be injected into connected OVS. */
> @@ -1145,6 +1147,50 @@ ctrl_register_ovs_idl(struct ovsdb_idl *ovs_idl)
> * track that column which should be addressed in the future. */
> }
>
> +struct ed_type_datapaths_updated {
> + struct uuidset waiting_sb_cond_update;
> +};
> +
> +static void *
> +en_datapaths_updated_init(struct engine_node *node OVS_UNUSED,
> + struct engine_arg *arg OVS_UNUSED)
> +{
> + struct ed_type_datapaths_updated *data = xzalloc(sizeof *data);
Nit: i'd add a newline.
Also, xmalloc is enough.
> + *data = (struct ed_type_datapaths_updated) {
> + .waiting_sb_cond_update =
> + UUIDSET_INITIALIZER(&data->waiting_sb_cond_update),
> + };
> + return data;
> +}
> +
> +static void
> +en_datapaths_updated_cleanup(void *data)
> +{
> + struct ed_type_datapaths_updated *sb_data = data;
Nit: i'd add a newline.
> + uuidset_destroy(&sb_data->waiting_sb_cond_update);
> +}
> +
> +static enum engine_node_state
> +en_datapaths_updated_run(struct engine_node *node OVS_UNUSED,
> + void *data)
> +{
> + struct controller_engine_ctx *ctrl_ctx =
> engine_get_context()->client_ctx;
> + struct ovsdb_idl_txn *ovnsb_idl_txn =
> engine_get_context()->ovnsb_idl_txn;
Nit: i'd add a newline.
> + if (!ovnsb_idl_txn) {
> + return EN_UNCHANGED;
> + }
Nit: i'd add a newline.
> + struct ovsdb_idl *ovnsb_idl = ovsdb_idl_txn_get_idl(ovnsb_idl_txn);
> + if (*ctrl_ctx->ovnsb_expected_cond_seqno ==
> + ovsdb_idl_get_condition_seqno(ovnsb_idl)) {
> + struct ed_type_datapaths_updated *dp_data = data;
Nit: i'd add a newline.
> + if (!uuidset_is_empty(&dp_data->waiting_sb_cond_update)) {
> + uuidset_clear(&dp_data->waiting_sb_cond_update);
> + return EN_UPDATED;
> + }
> + }
> + return EN_UNCHANGED;
> +}
> +
> struct ed_type_ofctrl_is_connected {
> bool connected;
> };
> @@ -1180,6 +1226,41 @@ en_ofctrl_is_connected_run(struct engine_node *node
> OVS_UNUSED, void *data)
> return EN_UNCHANGED;
> }
>
> +struct ed_type_sb_cond_seqno {
> + unsigned int last_sb_cond_seqno;
> +};
> +
> +static void *
> +en_sb_cond_seqno_init(struct engine_node *node OVS_UNUSED,
> + struct engine_arg *arg OVS_UNUSED)
> +{
> + struct ed_type_sb_cond_seqno *data = xzalloc(sizeof *data);
> + return data;
> +}
> +
> +static void en_sb_cond_seqno_cleanup(void *data OVS_UNUSED)
> +{
> +}
> +
> +static enum engine_node_state
> +en_sb_cond_seqno_run(struct engine_node *node OVS_UNUSED, void *data)
> +{
> + struct ed_type_sb_cond_seqno *sb_seqno_data = data;
> + struct ovsdb_idl_txn *ovnsb_idl_txn =
> engine_get_context()->ovnsb_idl_txn;
> + if (!ovnsb_idl_txn) {
> + return EN_UNCHANGED;
> + }
> +
> + unsigned int curr_seqno =
> + ovsdb_idl_get_condition_seqno(ovsdb_idl_txn_get_idl(ovnsb_idl_txn));
> +
> + if (sb_seqno_data->last_sb_cond_seqno != curr_seqno) {
> + sb_seqno_data->last_sb_cond_seqno = curr_seqno;
> + return EN_UPDATED;
> + }
> + return EN_UNCHANGED;
> +}
> +
> struct ed_type_if_status_mgr {
> const struct if_status_mgr *manager;
> const struct ovsrec_interface_table *iface_table;
> @@ -5204,6 +5285,15 @@ controller_output_acl_id_handler(struct engine_node
> *node OVS_UNUSED,
> return EN_HANDLED_UPDATED;
> }
>
> +static enum engine_input_handler_result
> +controlller_output_datapaths_updated_handler(
Typo: controlller
> + struct engine_node *node OVS_UNUSED,
> + void *data OVS_UNUSED)
> +{
> + return EN_HANDLED_UPDATED;
> +}
> +
> +
Nit: too many new lines.
> static enum engine_input_handler_result
> controller_output_route_exchange_handler(struct engine_node *node OVS_UNUSED,
> void *data OVS_UNUSED)
> @@ -6745,6 +6835,56 @@ evpn_arp_vtep_binding_handler(struct engine_node
> *node, void *data OVS_UNUSED)
> return EN_UNHANDLED;
> }
>
> +static enum engine_input_handler_result
> +datapaths_updated_runtime_data_handler(struct engine_node *node,
> + void *data)
This handler should be defined just after en_datapaths_updated_run(), as
mentioned in the v9 review.
> +{
> + enum engine_input_handler_result status = EN_HANDLED_UNCHANGED;
> + struct ed_type_datapaths_updated *dp_data = data;
> + struct ed_type_runtime_data *rt_data =
> + engine_get_input_data("runtime_data", node);
> + struct controller_engine_ctx *ctrl_ctx =
> engine_get_context()->client_ctx;
Nit: I'd add a new line here.
> + if (*ctrl_ctx->sb_monitor_all) {
> + if (!uuidset_is_empty(&dp_data->waiting_sb_cond_update)) {
> + uuidset_clear(&dp_data->waiting_sb_cond_update);
> + status = EN_HANDLED_UPDATED;
> + }
> + return status;
> + }
> +
I guess we're missing a:
if (!rt_data->tracked) {
return EN_UNHANDLED;
}
here.
> + struct tracked_datapath *tdp;
> + HMAP_FOR_EACH_SAFE (tdp, node, &rt_data->tracked_dp_bindings) {
> + if (tdp->tracked_type == TRACKED_RESOURCE_NEW) {
> + uuidset_insert(&dp_data->waiting_sb_cond_update,
> + &tdp->dp->header_.uuid);
> + status = EN_HANDLED_UPDATED;
> + }
> + }
> + return status;
> +}
> +
> +static enum engine_input_handler_result
> +datapaths_update_sb_cond_handler(struct engine_node *node OVS_UNUSED,
> + void *data)
> +{
This handler should be defined just after en_datapaths_updated_run(), as
mentioned in the v9 review.
> + struct controller_engine_ctx *ctrl_ctx =
> engine_get_context()->client_ctx;
> + struct ovsdb_idl_txn *ovnsb_idl_txn =
> engine_get_context()->ovnsb_idl_txn;
Nit: I'd add a new line here.
> + if (!ovnsb_idl_txn) {
> + return EN_HANDLED_UNCHANGED;
> + }
Nit: I'd add a new line here.
> + struct ovsdb_idl *ovnsb_idl = ovsdb_idl_txn_get_idl(ovnsb_idl_txn);
Nit: I'd add a new line here.
> + if (*ctrl_ctx->ovnsb_expected_cond_seqno ==
> + ovsdb_idl_get_condition_seqno(ovnsb_idl)) {
> + struct ed_type_datapaths_updated *dp_data = data;
> +
> + if (!uuidset_is_empty(&dp_data->waiting_sb_cond_update)) {
> + uuidset_clear(&dp_data->waiting_sb_cond_update);
> + return EN_HANDLED_UPDATED;
> + }
> + }
> + return EN_HANDLED_UNCHANGED;
> +}
> +
> /* Define engine node functions for nodes that represent SB tables.
> *
> * en_sb_<TABLE_NAME>_run()
> @@ -6867,6 +7007,8 @@ static ENGINE_NODE(neighbor_exchange_status);
> static ENGINE_NODE(evpn_vtep_binding, CLEAR_TRACKED_DATA);
> static ENGINE_NODE(evpn_fdb, CLEAR_TRACKED_DATA);
> static ENGINE_NODE(evpn_arp, CLEAR_TRACKED_DATA);
> +static ENGINE_NODE(datapaths_updated);
Thinking more about it, all we store in this new I-P node is the
waiting_sb_cond_update uuidset.
We can make that part of 'struct ed_type_runtime_data'.
> +static ENGINE_NODE(sb_cond_seqno);
>
> static void
> inc_proc_ovn_controller_init(
> @@ -6889,6 +7031,8 @@ inc_proc_ovn_controller_init(
> engine_add_input(&en_template_vars, &en_sb_chassis_template_var,
> template_vars_sb_chassis_template_var_handler);
>
> + engine_add_input(&en_datapaths_updated, &en_sb_cond_seqno,
> + datapaths_update_sb_cond_handler);
> engine_add_input(&en_lb_data, &en_sb_load_balancer,
> lb_data_sb_load_balancer_handler);
> engine_add_input(&en_lb_data, &en_template_vars,
> @@ -6975,6 +7119,9 @@ inc_proc_ovn_controller_init(
> engine_add_input(&en_pflow_output, &en_sb_sb_global,
> pflow_output_debug_handler);
>
> + engine_add_input(&en_datapaths_updated, &en_runtime_data,
> + datapaths_updated_runtime_data_handler);
> +
Handlers for a given node should be grouped together.
> engine_add_input(&en_northd_options, &en_sb_sb_global,
> en_northd_options_sb_sb_global_handler);
>
> @@ -7163,6 +7310,8 @@ inc_proc_ovn_controller_init(
> engine_add_input(&en_acl_id, &en_sb_acl_id, NULL);
> engine_add_input(&en_controller_output, &en_acl_id,
> controller_output_acl_id_handler);
> + engine_add_input(&en_controller_output, &en_datapaths_updated,
> + controlller_output_datapaths_updated_handler);
>
> struct engine_arg engine_arg = {
> .sb_idl = sb_idl_loop->idl,
> @@ -7556,6 +7705,8 @@ main(int argc, char *argv[])
> engine_get_internal_data(&en_evpn_fdb);
> struct ed_type_evpn_arp *earp_data =
> engine_get_internal_data(&en_evpn_arp);
> + struct ed_type_datapaths_updated *dp_updated_data =
> + engine_get_internal_data(&en_datapaths_updated);
>
> ofctrl_init(&lflow_output_data->group_table,
> &lflow_output_data->meter_table);
> @@ -7655,10 +7806,13 @@ main(int argc, char *argv[])
> unsigned int ovs_cond_seqno = UINT_MAX;
> unsigned int ovnsb_cond_seqno = UINT_MAX;
> unsigned int ovnsb_expected_cond_seqno = UINT_MAX;
> + bool sb_monitor_all = false;
>
> struct controller_engine_ctx ctrl_engine_ctx = {
> .lflow_cache = lflow_cache_create(),
> .if_mgr = if_status_mgr_create(),
> + .ovnsb_expected_cond_seqno = &ovnsb_expected_cond_seqno,
> + .sb_monitor_all = &sb_monitor_all,
> };
> struct if_status_mgr *if_mgr = ctrl_engine_ctx.if_mgr;
>
> @@ -7673,7 +7827,6 @@ main(int argc, char *argv[])
> /* Main loop. */
> int ovnsb_txn_status = 1;
> int ovs_txn_status = 1;
> - bool sb_monitor_all = false;
> struct tracked_acl_ids *tracked_acl_ids = NULL;
> while (!exit_args.exiting) {
> ovsrcu_quiesce_end();
> @@ -8085,11 +8238,17 @@ main(int argc, char *argv[])
> runtime_data ? &runtime_data->lbinding_data : NULL;
> stopwatch_start(IF_STATUS_MGR_UPDATE_STOPWATCH_NAME,
> time_msec());
> +
Unrelated change.
> if_status_mgr_update(if_mgr, binding_data, chassis,
> ovsrec_interface_table_get(
> ovs_idl_loop.idl),
> sbrec_port_binding_table_get(
> ovnsb_idl_loop.idl),
> + runtime_data ?
> + &runtime_data->local_datapaths
> + : NULL,
The proper way to indent, according to our coding guidelines, is:
runtime_data
? &runtime_data->local_datapaths
: NULL,
> + &dp_updated_data->
> + waiting_sb_cond_update,
> !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..f196213f0 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
Missing check.
> +
> +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
> + 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
> + test $flow_count -ge 1
> + else
> + false
> + fi
> +])
> +wait_for_ports_up
> +
> +OVN_CLEANUP([hv1])
> +AT_CLEANUP
> +])
> diff --git a/tests/ovn-inc-proc-graph-dump.at
> b/tests/ovn-inc-proc-graph-dump.at
> index 178310978..bd82f0015 100644
> --- a/tests/ovn-inc-proc-graph-dump.at
> +++ b/tests/ovn-inc-proc-graph-dump.at
> @@ -478,6 +478,10 @@ digraph "Incremental-Processing-Engine" {
> SB_acl_id [[style=filled, shape=box, fillcolor=white,
> label="SB_acl_id"]];
> acl_id [[style=filled, shape=box, fillcolor=white, label="acl_id"]];
> SB_acl_id -> acl_id [[label=""]];
> + sb_cond_seqno [[style=filled, shape=box, fillcolor=white,
> label="sb_cond_seqno"]];
> + datapaths_updated [[style=filled, shape=box, fillcolor=white,
> label="datapaths_updated"]];
> + sb_cond_seqno -> datapaths_updated
> [[label="datapaths_update_sb_cond_handler"]];
> + runtime_data -> datapaths_updated
> [[label="datapaths_updated_runtime_data_handler"]];
> controller_output [[style=filled, shape=box, fillcolor=white,
> label="controller_output"]];
> dns_cache -> controller_output [[label=""]];
> lflow_output -> controller_output
> [[label="controller_output_lflow_output_handler"]];
> @@ -487,6 +491,7 @@ digraph "Incremental-Processing-Engine" {
> route_exchange -> controller_output
> [[label="controller_output_route_exchange_handler"]];
> garp_rarp -> controller_output
> [[label="controller_output_garp_rarp_handler"]];
> acl_id -> controller_output
> [[label="controller_output_acl_id_handler"]];
> + datapaths_updated -> controller_output
> [[label="controlller_output_datapaths_updated_handler"]];
> }
> ])
> AT_CLEANUP
I went ahead and took care of the comments above and applied the patch
to main. I decided not to backport it though. The only user of
"ovn-installed" is ovn-kubernetes. And ovn-kubernetes uses
monitor-all=true so it doesn't hit this issue.
Regards,
Dumitru
_______________________________________________
dev mailing list
[email protected]
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
