Re: [ovs-dev] [PATCH ovn v3 12/16] northd: Add lr_stateful handler for lflow engine node.
On Wed, Dec 13, 2023 at 8:54 AM Dumitru Ceara wrote:
>
> On 11/28/23 03:37, num...@ovn.org wrote:
> > From: Numan Siddique
> >
> > Signed-off-by: Numan Siddique
> > ---
> > northd/en-lflow.c| 29 +++
> > northd/en-lflow.h| 1 +
> > northd/en-lr-stateful.c | 39 -
> > northd/en-lr-stateful.h | 26 +++
> > northd/inc-proc-northd.c | 3 +-
> > northd/northd.c | 370 ++-
> > northd/northd.h | 10 ++
> > tests/ovn-northd.at | 48 ++---
> > 8 files changed, 336 insertions(+), 190 deletions(-)
> >
> > diff --git a/northd/en-lflow.c b/northd/en-lflow.c
> > index 13284b5556..09748f570b 100644
> > --- a/northd/en-lflow.c
> > +++ b/northd/en-lflow.c
> > @@ -164,6 +164,35 @@ lflow_port_group_handler(struct engine_node *node,
> > void *data OVS_UNUSED)
> > return true;
> > }
> >
> > +bool
> > +lflow_lr_stateful_handler(struct engine_node *node, void *data)
> > +{
> > +struct ed_type_lr_stateful *lr_sful_data =
>
> I don't really like the 'lr_sful_data' abbreviation. What about
> 'stateful_data', it's one character longer? Or even 'lr_stateful_data'?
>
> I now realize that this is used in a lot of places throughout
> the series. I don't want to generate a lot of extra work just to
> rename it so I'll leave it up to you.
>
In v4, I've changed 'sful' to 'stateful' where it is used.
> > +engine_get_input_data("lr_stateful", node);
> > +
> > +if (!lr_stateful_has_tracked_data(_sful_data->trk_data)
> > +|| lr_sful_data->trk_data.vip_nats_changed) {
> > +return false;
> > +}
> > +
> > +const struct engine_context *eng_ctx = engine_get_context();
> > +struct lflow_data *lflow_data = data;
> > +
> > +struct lflow_input lflow_input;
>
> Nit: I'd add an empty line here and remove the one above (to group
> all declarations).
Ack. Done in v4.
>
> > +lflow_get_input_data(node, _input);
> > +
> > +if (!lflow_handle_lr_stateful_changes(eng_ctx->ovnsb_idl_txn,
> > + _sful_data->trk_data,
> > + _input,
> > + lflow_data->lflow_table)) {
> > +return false;
> > +}
> > +
> > +engine_set_node_state(node, EN_UPDATED);
> > +
> > +return true;
> > +}
> > +
> > void *en_lflow_init(struct engine_node *node OVS_UNUSED,
> > struct engine_arg *arg OVS_UNUSED)
> > {
> > diff --git a/northd/en-lflow.h b/northd/en-lflow.h
> > index f7325c56b1..1d813a2a29 100644
> > --- a/northd/en-lflow.h
> > +++ b/northd/en-lflow.h
> > @@ -20,5 +20,6 @@ void *en_lflow_init(struct engine_node *node, struct
> > engine_arg *arg);
> > void en_lflow_cleanup(void *data);
> > bool lflow_northd_handler(struct engine_node *, void *data);
> > bool lflow_port_group_handler(struct engine_node *, void *data);
> > +bool lflow_lr_stateful_handler(struct engine_node *, void *data);
> >
> > #endif /* EN_LFLOW_H */
> > diff --git a/northd/en-lr-stateful.c b/northd/en-lr-stateful.c
> > index a54749ad93..8e025f057e 100644
> > --- a/northd/en-lr-stateful.c
> > +++ b/northd/en-lr-stateful.c
> > @@ -39,6 +39,7 @@
> > #include "lib/ovn-sb-idl.h"
> > #include "lib/ovn-util.h"
> > #include "lib/stopwatch-names.h"
> > +#include "lflow-mgr.h"
> > #include "northd.h"
> >
> > VLOG_DEFINE_THIS_MODULE(en_lr_stateful);
> > @@ -81,7 +82,7 @@ static void remove_lrouter_lb_reachable_ips(struct
> > lr_stateful_record *,
> > enum
> > lb_neighbor_responder_mode,
> > const struct sset *lb_ips_v4,
> > const struct sset *lb_ips_v6);
> > -static void lr_stateful_build_vip_nats(struct lr_stateful_record *);
> > +static bool lr_stateful_build_vip_nats(struct lr_stateful_record *);
> >
> > /* 'lr_stateful' engine node manages the NB logical router LB data.
> > */
> > @@ -110,6 +111,7 @@ en_lr_stateful_clear_tracked_data(void *data_)
> > struct ed_type_lr_stateful *data = (struct ed_type_lr_stateful *)
> > data_;
> >
> > hmapx_clear(>trk_data.crupdated);
> > +data->trk_data.vip_nats_changed = false;
> > }
> >
> > void
> > @@ -190,6 +192,10 @@ lr_stateful_lb_data_handler(struct engine_node *node,
> > void *data_)
> >
> > /* Add the lr_sful_rec rec to the tracking data. */
> > hmapx_add(>trk_data.crupdated, lr_sful_rec);
> > +
> > +if (!sset_is_empty(_sful_rec->vip_nats)) {
> > +data->trk_data.vip_nats_changed = true;
> > +}
> > continue;
> > }
> >
> > @@ -298,7 +304,9 @@ lr_stateful_lb_data_handler(struct engine_node *node,
> > void *data_)
> > * vip nats. */
> > HMAPX_FOR_EACH (hmapx_node, >trk_data.crupdated) {
> > lr_sful_rec = hmapx_node->data;
> > -
Re: [ovs-dev] [PATCH ovn v3 12/16] northd: Add lr_stateful handler for lflow engine node.
On Mon, Nov 27, 2023 at 6:38 PM wrote:
>
> From: Numan Siddique
>
> Signed-off-by: Numan Siddique
> ---
> northd/en-lflow.c| 29 +++
> northd/en-lflow.h| 1 +
> northd/en-lr-stateful.c | 39 -
> northd/en-lr-stateful.h | 26 +++
> northd/inc-proc-northd.c | 3 +-
> northd/northd.c | 370 ++-
> northd/northd.h | 10 ++
> tests/ovn-northd.at | 48 ++---
> 8 files changed, 336 insertions(+), 190 deletions(-)
>
> diff --git a/northd/en-lflow.c b/northd/en-lflow.c
> index 13284b5556..09748f570b 100644
> --- a/northd/en-lflow.c
> +++ b/northd/en-lflow.c
> @@ -164,6 +164,35 @@ lflow_port_group_handler(struct engine_node *node,
void *data OVS_UNUSED)
> return true;
> }
>
> +bool
> +lflow_lr_stateful_handler(struct engine_node *node, void *data)
> +{
> +struct ed_type_lr_stateful *lr_sful_data =
> +engine_get_input_data("lr_stateful", node);
> +
> +if (!lr_stateful_has_tracked_data(_sful_data->trk_data)
> +|| lr_sful_data->trk_data.vip_nats_changed) {
> +return false;
> +}
> +
> +const struct engine_context *eng_ctx = engine_get_context();
> +struct lflow_data *lflow_data = data;
> +
> +struct lflow_input lflow_input;
> +lflow_get_input_data(node, _input);
> +
> +if (!lflow_handle_lr_stateful_changes(eng_ctx->ovnsb_idl_txn,
> + _sful_data->trk_data,
> + _input,
> + lflow_data->lflow_table)) {
> +return false;
> +}
> +
> +engine_set_node_state(node, EN_UPDATED);
> +
> +return true;
> +}
> +
> void *en_lflow_init(struct engine_node *node OVS_UNUSED,
> struct engine_arg *arg OVS_UNUSED)
> {
> diff --git a/northd/en-lflow.h b/northd/en-lflow.h
> index f7325c56b1..1d813a2a29 100644
> --- a/northd/en-lflow.h
> +++ b/northd/en-lflow.h
> @@ -20,5 +20,6 @@ void *en_lflow_init(struct engine_node *node, struct
engine_arg *arg);
> void en_lflow_cleanup(void *data);
> bool lflow_northd_handler(struct engine_node *, void *data);
> bool lflow_port_group_handler(struct engine_node *, void *data);
> +bool lflow_lr_stateful_handler(struct engine_node *, void *data);
>
> #endif /* EN_LFLOW_H */
> diff --git a/northd/en-lr-stateful.c b/northd/en-lr-stateful.c
> index a54749ad93..8e025f057e 100644
> --- a/northd/en-lr-stateful.c
> +++ b/northd/en-lr-stateful.c
> @@ -39,6 +39,7 @@
> #include "lib/ovn-sb-idl.h"
> #include "lib/ovn-util.h"
> #include "lib/stopwatch-names.h"
> +#include "lflow-mgr.h"
> #include "northd.h"
>
> VLOG_DEFINE_THIS_MODULE(en_lr_stateful);
> @@ -81,7 +82,7 @@ static void remove_lrouter_lb_reachable_ips(struct
lr_stateful_record *,
> enum
lb_neighbor_responder_mode,
> const struct sset *lb_ips_v4,
> const struct sset
*lb_ips_v6);
> -static void lr_stateful_build_vip_nats(struct lr_stateful_record *);
> +static bool lr_stateful_build_vip_nats(struct lr_stateful_record *);
>
> /* 'lr_stateful' engine node manages the NB logical router LB data.
> */
> @@ -110,6 +111,7 @@ en_lr_stateful_clear_tracked_data(void *data_)
> struct ed_type_lr_stateful *data = (struct ed_type_lr_stateful *)
data_;
>
> hmapx_clear(>trk_data.crupdated);
> +data->trk_data.vip_nats_changed = false;
> }
>
> void
> @@ -190,6 +192,10 @@ lr_stateful_lb_data_handler(struct engine_node
*node, void *data_)
>
> /* Add the lr_sful_rec rec to the tracking data. */
> hmapx_add(>trk_data.crupdated, lr_sful_rec);
> +
> +if (!sset_is_empty(_sful_rec->vip_nats)) {
> +data->trk_data.vip_nats_changed = true;
> +}
> continue;
> }
>
> @@ -298,7 +304,9 @@ lr_stateful_lb_data_handler(struct engine_node *node,
void *data_)
> * vip nats. */
> HMAPX_FOR_EACH (hmapx_node, >trk_data.crupdated) {
> lr_sful_rec = hmapx_node->data;
> -lr_stateful_build_vip_nats(lr_sful_rec);
> +if (lr_stateful_build_vip_nats(lr_sful_rec)) {
> +data->trk_data.vip_nats_changed = true;
> +}
> lr_sful_rec->has_lb_vip = od_has_lb_vip(lr_sful_rec->od);
> }
>
> @@ -335,8 +343,13 @@ lr_stateful_lr_nat_handler(struct engine_node *node,
void *data_)
> lrnat_rec,
> input_data.lb_datapaths_map,
>
input_data.lbgrp_datapaths_map);
> +if (!sset_is_empty(_sful_rec->vip_nats)) {
> +data->trk_data.vip_nats_changed = true;
> +}
> } else {
> -lr_stateful_build_vip_nats(lr_sful_rec);
> +if (lr_stateful_build_vip_nats(lr_sful_rec)) {
> +
Re: [ovs-dev] [PATCH ovn v3 12/16] northd: Add lr_stateful handler for lflow engine node.
On 11/28/23 03:37, num...@ovn.org wrote:
> From: Numan Siddique
>
> Signed-off-by: Numan Siddique
> ---
> northd/en-lflow.c| 29 +++
> northd/en-lflow.h| 1 +
> northd/en-lr-stateful.c | 39 -
> northd/en-lr-stateful.h | 26 +++
> northd/inc-proc-northd.c | 3 +-
> northd/northd.c | 370 ++-
> northd/northd.h | 10 ++
> tests/ovn-northd.at | 48 ++---
> 8 files changed, 336 insertions(+), 190 deletions(-)
>
> diff --git a/northd/en-lflow.c b/northd/en-lflow.c
> index 13284b5556..09748f570b 100644
> --- a/northd/en-lflow.c
> +++ b/northd/en-lflow.c
> @@ -164,6 +164,35 @@ lflow_port_group_handler(struct engine_node *node, void
> *data OVS_UNUSED)
> return true;
> }
>
> +bool
> +lflow_lr_stateful_handler(struct engine_node *node, void *data)
> +{
> +struct ed_type_lr_stateful *lr_sful_data =
I don't really like the 'lr_sful_data' abbreviation. What about
'stateful_data', it's one character longer? Or even 'lr_stateful_data'?
I now realize that this is used in a lot of places throughout
the series. I don't want to generate a lot of extra work just to
rename it so I'll leave it up to you.
> +engine_get_input_data("lr_stateful", node);
> +
> +if (!lr_stateful_has_tracked_data(_sful_data->trk_data)
> +|| lr_sful_data->trk_data.vip_nats_changed) {
> +return false;
> +}
> +
> +const struct engine_context *eng_ctx = engine_get_context();
> +struct lflow_data *lflow_data = data;
> +
> +struct lflow_input lflow_input;
Nit: I'd add an empty line here and remove the one above (to group
all declarations).
> +lflow_get_input_data(node, _input);
> +
> +if (!lflow_handle_lr_stateful_changes(eng_ctx->ovnsb_idl_txn,
> + _sful_data->trk_data,
> + _input,
> + lflow_data->lflow_table)) {
> +return false;
> +}
> +
> +engine_set_node_state(node, EN_UPDATED);
> +
> +return true;
> +}
> +
> void *en_lflow_init(struct engine_node *node OVS_UNUSED,
> struct engine_arg *arg OVS_UNUSED)
> {
> diff --git a/northd/en-lflow.h b/northd/en-lflow.h
> index f7325c56b1..1d813a2a29 100644
> --- a/northd/en-lflow.h
> +++ b/northd/en-lflow.h
> @@ -20,5 +20,6 @@ void *en_lflow_init(struct engine_node *node, struct
> engine_arg *arg);
> void en_lflow_cleanup(void *data);
> bool lflow_northd_handler(struct engine_node *, void *data);
> bool lflow_port_group_handler(struct engine_node *, void *data);
> +bool lflow_lr_stateful_handler(struct engine_node *, void *data);
>
> #endif /* EN_LFLOW_H */
> diff --git a/northd/en-lr-stateful.c b/northd/en-lr-stateful.c
> index a54749ad93..8e025f057e 100644
> --- a/northd/en-lr-stateful.c
> +++ b/northd/en-lr-stateful.c
> @@ -39,6 +39,7 @@
> #include "lib/ovn-sb-idl.h"
> #include "lib/ovn-util.h"
> #include "lib/stopwatch-names.h"
> +#include "lflow-mgr.h"
> #include "northd.h"
>
> VLOG_DEFINE_THIS_MODULE(en_lr_stateful);
> @@ -81,7 +82,7 @@ static void remove_lrouter_lb_reachable_ips(struct
> lr_stateful_record *,
> enum lb_neighbor_responder_mode,
> const struct sset *lb_ips_v4,
> const struct sset *lb_ips_v6);
> -static void lr_stateful_build_vip_nats(struct lr_stateful_record *);
> +static bool lr_stateful_build_vip_nats(struct lr_stateful_record *);
>
> /* 'lr_stateful' engine node manages the NB logical router LB data.
> */
> @@ -110,6 +111,7 @@ en_lr_stateful_clear_tracked_data(void *data_)
> struct ed_type_lr_stateful *data = (struct ed_type_lr_stateful *) data_;
>
> hmapx_clear(>trk_data.crupdated);
> +data->trk_data.vip_nats_changed = false;
> }
>
> void
> @@ -190,6 +192,10 @@ lr_stateful_lb_data_handler(struct engine_node *node,
> void *data_)
>
> /* Add the lr_sful_rec rec to the tracking data. */
> hmapx_add(>trk_data.crupdated, lr_sful_rec);
> +
> +if (!sset_is_empty(_sful_rec->vip_nats)) {
> +data->trk_data.vip_nats_changed = true;
> +}
> continue;
> }
>
> @@ -298,7 +304,9 @@ lr_stateful_lb_data_handler(struct engine_node *node,
> void *data_)
> * vip nats. */
> HMAPX_FOR_EACH (hmapx_node, >trk_data.crupdated) {
> lr_sful_rec = hmapx_node->data;
> -lr_stateful_build_vip_nats(lr_sful_rec);
> +if (lr_stateful_build_vip_nats(lr_sful_rec)) {
> +data->trk_data.vip_nats_changed = true;
> +}
> lr_sful_rec->has_lb_vip = od_has_lb_vip(lr_sful_rec->od);
> }
>
> @@ -335,8 +343,13 @@ lr_stateful_lr_nat_handler(struct engine_node *node,
> void *data_)
> lrnat_rec,
>
