| Message ID | 20211124005147.26066-1-dceara@redhat.com |
|---|---|
| State | Accepted |
| Headers | show |
| Series | [ovs-dev] northd: I-P: fix BFD dependency. | expand |
| Context | Check | Description |
|---|---|---|
| ovsrobot/apply-robot | success | apply and check: success |
| ovsrobot/github-robot-_Build_and_Test | success | github build: passed |
| ovsrobot/github-robot-_ovn-kubernetes | success | github build: passed |
On 11/24/21 01:51, Dumitru Ceara 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@redhat.com> > --- Successful CI run: https://mail.openvswitch.org/pipermail/ovs-build/2021-November/018639.html
On Tue, Nov 23, 2021 at 7:52 PM Dumitru Ceara <dceara@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@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@openvswitch.org > https://mail.openvswitch.org/mailman/listinfo/ovs-dev >
On 11/24/21 17:15, Numan Siddique wrote: > On Tue, Nov 23, 2021 at 7:52 PM Dumitru Ceara <dceara@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@redhat.com> > > Hi Dumitru, > Hi Numan, > Thanks for fixing this issue. > Thanks for reviewing this. > 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 ? > Sounds like this would be the ideal way to implement this, yes. > I'm not too sure about how easy it is ? I'm inclined to accept this I don't see a very clean way of implementing it though. > patch and then revisit this > as a follow up or once we branch out of 21.12. I think this would be the safest way to go for now. > > Let me know your thoughts. > > Thanks > Numan > Thanks, Dumitru
On Wed, Nov 24, 2021 at 11:19 AM Dumitru Ceara <dceara@redhat.com> wrote: > > On 11/24/21 17:15, Numan Siddique wrote: > > On Tue, Nov 23, 2021 at 7:52 PM Dumitru Ceara <dceara@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@redhat.com> > > > > Hi Dumitru, > > > > Hi Numan, > > > Thanks for fixing this issue. > > > > Thanks for reviewing this. > > > 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 ? > > > > Sounds like this would be the ideal way to implement this, yes. > > > I'm not too sure about how easy it is ? I'm inclined to accept this > > I don't see a very clean way of implementing it though. > > > patch and then revisit this > > as a follow up or once we branch out of 21.12. > > I think this would be the safest way to go for now. Sounds good. I applied this patch to the main branch. Numan > > > > > Let me know your thoughts. > > > > Thanks > > Numan > > > > Thanks, > Dumitru > > _______________________________________________ > dev mailing list > dev@openvswitch.org > https://mail.openvswitch.org/mailman/listinfo/ovs-dev >
On 11/24/21 17:21, Numan Siddique wrote: > On Wed, Nov 24, 2021 at 11:19 AM Dumitru Ceara <dceara@redhat.com> wrote: >> >> On 11/24/21 17:15, Numan Siddique wrote: >>> On Tue, Nov 23, 2021 at 7:52 PM Dumitru Ceara <dceara@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@redhat.com> >>> >>> Hi Dumitru, >>> >> >> Hi Numan, >> >>> Thanks for fixing this issue. >>> >> >> Thanks for reviewing this. >> >>> 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 ? >>> >> >> Sounds like this would be the ideal way to implement this, yes. >> >>> I'm not too sure about how easy it is ? I'm inclined to accept this >> >> I don't see a very clean way of implementing it though. >> >>> patch and then revisit this >>> as a follow up or once we branch out of 21.12. >> >> I think this would be the safest way to go for now. > > Sounds good. I applied this patch to the main branch. > Thanks!
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 ])
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@redhat.com> --- 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(-)