[ovs-dev] [PATCH ovn] northd: I-P: fix BFD dependency.
Numan Siddique
numans at ovn.org
Wed Nov 24 16:15:49 UTC 2021
On Tue, Nov 23, 2021 at 7:52 PM Dumitru Ceara <dceara at redhat.com> wrote:
>
> BFD entries are updated and their ->ref field is set to 'true' when
> static routes are parsed, within build_lflows(), in the 'en_lflow' I-P
> node. Therefore we cannot clean up BFD entries in 'en_northd' which is
> an input of 'en_lflow'.
>
> To fix this we now move all BFD handling in the 'en_lflow' node.
>
> This fixes the CI failures that we've recently started hitting when
> running the ovn-kubernetes jobs, for example:
> https://github.com/ovn-org/ovn/runs/4294661249?check_suite_focus=true#step:10:9154
>
> Fixes: 1fa6612ffebf ("northd: Add lflow node")
> Signed-off-by: Dumitru Ceara <dceara at redhat.com>
Hi Dumitru,
Thanks for fixing this issue.
In my opinion en_northd engine node should handle the DB syncs and
en_lflow engine node
should handle the flow generation. I think it would be better to move
the syncing of
NB bfd->state from en_lflow engine node (ie frombuild_lflows()) to
en_northd engine node.
This would mean en_northd engine node should iterate the logical
static routes and set the
status. Which would mean we need to move out the function
parsed_routes_add() from
build_static_route_flows_for_lrouter().
What do you think ? Would you agree with this ?
I'm not too sure about how easy it is ? I'm inclined to accept this
patch and then revisit this
as a follow up or once we branch out of 21.12.
Let me know your thoughts.
Thanks
Numan
> northd/en-lflow.c | 8 ++++++++
> northd/en-northd.c | 4 ----
> northd/inc-proc-northd.c | 4 ++--
> northd/northd.c | 11 ++++-------
> northd/northd.h | 11 +++++++++--
> tests/ovn-northd.at | 8 ++++++++
> 6 files changed, 31 insertions(+), 15 deletions(-)
>
> diff --git a/northd/en-lflow.c b/northd/en-lflow.c
> index 5bef35cf6..ffbdaf4e8 100644
> --- a/northd/en-lflow.c
> +++ b/northd/en-lflow.c
> @@ -38,6 +38,10 @@ void en_lflow_run(struct engine_node *node, void *data OVS_UNUSED)
>
> struct northd_data *northd_data = engine_get_input_data("northd", node);
>
> + lflow_input.nbrec_bfd_table =
> + EN_OVSDB_GET(engine_get_input("NB_bfd", node));
> + lflow_input.sbrec_bfd_table =
> + EN_OVSDB_GET(engine_get_input("SB_bfd", node));
> lflow_input.sbrec_logical_flow_table =
> EN_OVSDB_GET(engine_get_input("SB_logical_flow", node));
> lflow_input.sbrec_multicast_group_table =
> @@ -60,7 +64,11 @@ void en_lflow_run(struct engine_node *node, void *data OVS_UNUSED)
> northd_data->ovn_internal_version_changed;
>
> stopwatch_start(BUILD_LFLOWS_STOPWATCH_NAME, time_msec());
> + build_bfd_table(&lflow_input, eng_ctx->ovnsb_idl_txn,
> + &northd_data->bfd_connections,
> + &northd_data->ports);
> build_lflows(&lflow_input, eng_ctx->ovnsb_idl_txn);
> + bfd_cleanup_connections(&lflow_input, &northd_data->bfd_connections);
> stopwatch_stop(BUILD_LFLOWS_STOPWATCH_NAME, time_msec());
>
> engine_set_node_state(node, EN_UPDATED);
> diff --git a/northd/en-northd.c b/northd/en-northd.c
> index 0523560f8..79da7e1c4 100644
> --- a/northd/en-northd.c
> +++ b/northd/en-northd.c
> @@ -66,8 +66,6 @@ void en_northd_run(struct engine_node *node, void *data)
> EN_OVSDB_GET(engine_get_input("NB_load_balancer", node));
> input_data.nbrec_port_group_table =
> EN_OVSDB_GET(engine_get_input("NB_port_group", node));
> - input_data.nbrec_bfd_table =
> - EN_OVSDB_GET(engine_get_input("NB_bfd", node));
> input_data.nbrec_address_set_table =
> EN_OVSDB_GET(engine_get_input("NB_address_set", node));
> input_data.nbrec_meter_table =
> @@ -93,8 +91,6 @@ void en_northd_run(struct engine_node *node, void *data)
> EN_OVSDB_GET(engine_get_input("SB_load_balancer", node));
> input_data.sbrec_service_monitor_table =
> EN_OVSDB_GET(engine_get_input("SB_service_monitor", node));
> - input_data.sbrec_bfd_table =
> - EN_OVSDB_GET(engine_get_input("SB_bfd", node));
> input_data.sbrec_address_set_table =
> EN_OVSDB_GET(engine_get_input("SB_address_set", node));
> input_data.sbrec_port_group_table =
> diff --git a/northd/inc-proc-northd.c b/northd/inc-proc-northd.c
> index 8e7428dda..c02c5a44a 100644
> --- a/northd/inc-proc-northd.c
> +++ b/northd/inc-proc-northd.c
> @@ -178,7 +178,6 @@ void inc_proc_northd_init(struct ovsdb_idl_loop *nb,
> engine_add_input(&en_northd, &en_nb_gateway_chassis, NULL);
> engine_add_input(&en_northd, &en_nb_ha_chassis_group, NULL);
> engine_add_input(&en_northd, &en_nb_ha_chassis, NULL);
> - engine_add_input(&en_northd, &en_nb_bfd, NULL);
>
> engine_add_input(&en_northd, &en_sb_sb_global, NULL);
> engine_add_input(&en_northd, &en_sb_chassis, NULL);
> @@ -206,8 +205,9 @@ void inc_proc_northd_init(struct ovsdb_idl_loop *nb,
> engine_add_input(&en_northd, &en_sb_ip_multicast, NULL);
> engine_add_input(&en_northd, &en_sb_service_monitor, NULL);
> engine_add_input(&en_northd, &en_sb_load_balancer, NULL);
> - engine_add_input(&en_northd, &en_sb_bfd, NULL);
> engine_add_input(&en_northd, &en_sb_fdb, NULL);
> + engine_add_input(&en_lflow, &en_nb_bfd, NULL);
> + engine_add_input(&en_lflow, &en_sb_bfd, NULL);
> engine_add_input(&en_lflow, &en_sb_logical_flow, NULL);
> engine_add_input(&en_lflow, &en_sb_multicast_group, NULL);
> engine_add_input(&en_lflow, &en_sb_igmp_group, NULL);
> diff --git a/northd/northd.c b/northd/northd.c
> index 2b7dd5980..c57026081 100644
> --- a/northd/northd.c
> +++ b/northd/northd.c
> @@ -8348,8 +8348,8 @@ bfd_port_lookup(const struct hmap *bfd_map, const char *logical_port,
> return NULL;
> }
>
> -static void
> -bfd_cleanup_connections(struct northd_input *input_data,
> +void
> +bfd_cleanup_connections(struct lflow_input *input_data,
> struct hmap *bfd_map)
> {
> const struct nbrec_bfd *nb_bt;
> @@ -8432,8 +8432,8 @@ static int bfd_get_unused_port(unsigned long *bfd_src_ports)
> return port + BFD_UDP_SRC_PORT_START;
> }
>
> -static void
> -build_bfd_table(struct northd_input *input_data,
> +void
> +build_bfd_table(struct lflow_input *input_data,
> struct ovsdb_idl_txn *ovnsb_txn,
> struct hmap *bfd_connections, struct hmap *ports)
> {
> @@ -14935,8 +14935,6 @@ ovnnb_db_run(struct northd_input *input_data,
> build_lrouter_groups(&data->ports, &data->lr_list);
> build_ip_mcast(input_data, ovnsb_txn, &data->datapaths);
> build_meter_groups(input_data, &data->meter_groups);
> - build_bfd_table(input_data,
> - ovnsb_txn, &data->bfd_connections, &data->ports);
> stopwatch_stop(BUILD_LFLOWS_CTX_STOPWATCH_NAME, time_msec());
> stopwatch_start(CLEAR_LFLOWS_CTX_STOPWATCH_NAME, time_msec());
> ovn_update_ipv6_prefix(&data->ports);
> @@ -14946,7 +14944,6 @@ ovnnb_db_run(struct northd_input *input_data,
> sync_meters(input_data, ovnsb_txn, &data->meter_groups);
> sync_dns_entries(input_data, ovnsb_txn, &data->datapaths);
> cleanup_stale_fdp_entries(input_data, &data->datapaths);
> - bfd_cleanup_connections(input_data, &data->bfd_connections);
> stopwatch_stop(CLEAR_LFLOWS_CTX_STOPWATCH_NAME, time_msec());
> }
>
> diff --git a/northd/northd.h b/northd/northd.h
> index 07cf66f71..ebcb40de7 100644
> --- a/northd/northd.h
> +++ b/northd/northd.h
> @@ -25,7 +25,6 @@ struct northd_input {
> const struct nbrec_logical_router_table *nbrec_logical_router;
> const struct nbrec_load_balancer_table *nbrec_load_balancer_table;
> const struct nbrec_port_group_table *nbrec_port_group_table;
> - const struct nbrec_bfd_table *nbrec_bfd_table;
> const struct nbrec_address_set_table *nbrec_address_set_table;
> const struct nbrec_meter_table *nbrec_meter_table;
> const struct nbrec_acl_table *nbrec_acl_table;
> @@ -40,7 +39,6 @@ struct northd_input {
> const struct sbrec_fdb_table *sbrec_fdb_table;
> const struct sbrec_load_balancer_table *sbrec_load_balancer_table;
> const struct sbrec_service_monitor_table *sbrec_service_monitor_table;
> - const struct sbrec_bfd_table *sbrec_bfd_table;
> const struct sbrec_address_set_table *sbrec_address_set_table;
> const struct sbrec_port_group_table *sbrec_port_group_table;
> const struct sbrec_meter_table *sbrec_meter_table;
> @@ -68,7 +66,11 @@ struct northd_data {
> };
>
> struct lflow_input {
> + /* Northbound table references */
> + const struct nbrec_bfd_table *nbrec_bfd_table;
> +
> /* Southbound table references */
> + const struct sbrec_bfd_table *sbrec_bfd_table;
> const struct sbrec_logical_flow_table *sbrec_logical_flow_table;
> const struct sbrec_multicast_group_table *sbrec_multicast_group_table;
> const struct sbrec_igmp_group_table *sbrec_igmp_group_table;
> @@ -95,5 +97,10 @@ void northd_indices_create(struct northd_data *data,
> struct ovsdb_idl *ovnsb_idl);
> void build_lflows(struct lflow_input *input_data,
> struct ovsdb_idl_txn *ovnsb_txn);
> +void build_bfd_table(struct lflow_input *input_data,
> + struct ovsdb_idl_txn *ovnsb_txn,
> + struct hmap *bfd_connections, struct hmap *ports);
> +void bfd_cleanup_connections(struct lflow_input *input_data,
> + struct hmap *bfd_map);
>
> #endif /* NORTHD_H */
> diff --git a/tests/ovn-northd.at b/tests/ovn-northd.at
> index 316e5a535..670efd496 100644
> --- a/tests/ovn-northd.at
> +++ b/tests/ovn-northd.at
> @@ -3219,6 +3219,14 @@ wait_column admin_down bfd status logical_port=r0-sw1
> ovn-nbctl destroy bfd $uuid
> wait_row_count bfd 5
>
> +# Simulate BFD up in Southbound for an automatically created entry.
> +# This entry is referenced so the state in the Northbound should also
> +# become "up".
> +wait_column down nb:bfd status logical_port=r0-sw2
> +bfd2_uuid=$(fetch_column bfd _uuid logical_port=r0-sw2)
> +check ovn-sbctl set bfd $bfd2_uuid status=up
> +wait_column up nb:bfd status logical_port=r0-sw2
> +
> AT_CLEANUP
> ])
>
> --
> 2.27.0
>
> _______________________________________________
> dev mailing list
> dev at openvswitch.org
> https://mail.openvswitch.org/mailman/listinfo/ovs-dev
>
More information about the dev
mailing list