Re: [ovs-dev] [PATCH ovn] northd: I-P: fix BFD dependency.
On 11/24/21 17:21, Numan Siddique wrote:
> On Wed, Nov 24, 2021 at 11:19 AM Dumitru Ceara wrote:
>>
>> On 11/24/21 17:15, Numan Siddique wrote:
>>> On Tue, Nov 23, 2021 at 7:52 PM 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
>>>
>>> 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!
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH ovn] northd: I-P: fix BFD dependency.
On Wed, Nov 24, 2021 at 11:19 AM Dumitru Ceara wrote:
>
> On 11/24/21 17:15, Numan Siddique wrote:
> > On Tue, Nov 23, 2021 at 7:52 PM 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
> >
> > 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
> d...@openvswitch.org
> https://mail.openvswitch.org/mailman/listinfo/ovs-dev
>
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH ovn] northd: I-P: fix BFD dependency.
On 11/24/21 17:15, Numan Siddique wrote:
> On Tue, Nov 23, 2021 at 7:52 PM 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
>
> 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
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH ovn] northd: I-P: fix BFD dependency.
On Tue, Nov 23, 2021 at 7:52 PM 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
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(_input, eng_ctx->ovnsb_idl_txn,
> +_data->bfd_connections,
> +_data->ports);
> build_lflows(_input, eng_ctx->ovnsb_idl_txn);
> +bfd_cleanup_connections(_input, _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(_northd, _nb_gateway_chassis, NULL);
> engine_add_input(_northd, _nb_ha_chassis_group, NULL);
> engine_add_input(_northd, _nb_ha_chassis, NULL);
> -engine_add_input(_northd, _nb_bfd, NULL);
>
> engine_add_input(_northd, _sb_sb_global, NULL);
> engine_add_input(_northd, _sb_chassis, NULL);
> @@ -206,8 +205,9 @@ void inc_proc_northd_init(struct ovsdb_idl_loop *nb,
> engine_add_input(_northd, _sb_ip_multicast, NULL);
> engine_add_input(_northd, _sb_service_monitor, NULL);
> engine_add_input(_northd, _sb_load_balancer, NULL);
> -
Re: [ovs-dev] [PATCH ovn] northd: I-P: fix BFD dependency.
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
> ---
Successful CI run:
https://mail.openvswitch.org/pipermail/ovs-build/2021-November/018639.html
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
[ovs-dev] [PATCH ovn] northd: I-P: fix BFD dependency.
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
---
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(_input, eng_ctx->ovnsb_idl_txn,
+_data->bfd_connections,
+_data->ports);
build_lflows(_input, eng_ctx->ovnsb_idl_txn);
+bfd_cleanup_connections(_input, _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(_northd, _nb_gateway_chassis, NULL);
engine_add_input(_northd, _nb_ha_chassis_group, NULL);
engine_add_input(_northd, _nb_ha_chassis, NULL);
-engine_add_input(_northd, _nb_bfd, NULL);
engine_add_input(_northd, _sb_sb_global, NULL);
engine_add_input(_northd, _sb_chassis, NULL);
@@ -206,8 +205,9 @@ void inc_proc_northd_init(struct ovsdb_idl_loop *nb,
engine_add_input(_northd, _sb_ip_multicast, NULL);
engine_add_input(_northd, _sb_service_monitor, NULL);
engine_add_input(_northd, _sb_load_balancer, NULL);
-engine_add_input(_northd, _sb_bfd, NULL);
engine_add_input(_northd, _sb_fdb, NULL);
+engine_add_input(_lflow, _nb_bfd, NULL);
+engine_add_input(_lflow, _sb_bfd, NULL);
engine_add_input(_lflow, _sb_logical_flow, NULL);
engine_add_input(_lflow, _sb_multicast_group, NULL);
engine_add_input(_lflow, _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
