Re: [ovs-dev] [PATCH ovn v2 05/18] northd: Add a new engine 'lr-nat' to manage lr NAT data.

2023-12-08 Thread Dumitru Ceara
On 12/6/23 04:03, Numan Siddique wrote:
> On Thu, Nov 23, 2023 at 9:04 AM Dumitru Ceara  wrote:
>>
>> On 11/17/23 23:05, Numan Siddique wrote:
>>> On Wed, Nov 15, 2023 at 1:27 AM Han Zhou  wrote:

 On Thu, Oct 26, 2023 at 11:15 AM  wrote:
>
> From: Numan Siddique 
>
> This new engine now maintains the NAT related data for each
> logical router which was earlier maintained by the northd
> engine node in the 'struct ovn_datapath'.  Main inputs to
> this engine node are:
>- northd
>- NB logical router
>
> A record for each logical router is maintained in the 'lr_nats'
> hmap table and this record
>   - stores the ovn_nat's

 It seems the sentence is incomplete.

>
> Handlers are also added to handle the changes to both these
> inputs.  This engine node becomes an input to 'lflow' node.
> This essentially decouples the lr NAT data from the northd
> engine node.
>
> Signed-off-by: Numan Siddique 
> ---
>  lib/ovn-util.c   |   6 +-
>  lib/ovn-util.h   |   2 +-
>  lib/stopwatch-names.h|   1 +
>  northd/automake.mk   |   2 +
>  northd/en-lflow.c|   5 +
>  northd/en-lr-nat.c   | 498 +
>  northd/en-lr-nat.h   | 134 ++
>  northd/en-sync-sb.c  |  11 +-
>  northd/inc-proc-northd.c |   9 +
>  northd/northd.c  | 514 ++-
>  northd/northd.h  |  32 ++-
>  tests/ovn-northd.at  |  18 ++
>  12 files changed, 877 insertions(+), 355 deletions(-)
>  create mode 100644 northd/en-lr-nat.c
>  create mode 100644 northd/en-lr-nat.h
>>
>> I would call these en-nat.{hc}, I think.  There's no NAT for switches.
> 
> 
> In v3 I haven't renamed it.  I can rename it in the next version once
> most of the review comments
> are addressed  (it's a bit of a pain to rename and fix all the compiler 
> errors).
> 
> But in this case I still prefer lr-nat (and the structures -
> lr_nat_record, ... ) because,
>  -  This engine node maintains the data for the NATs associated
> with a logical router.  A router
>  can have 0 or more NATs
>  - We have a NB NAT table which is for each NAT entry, whereas
> this engine node is for NATs associated with a router
> and it could confuse and not convey the relationship between
> router and NATs.
>  -  This patch creates a 'lr_nat_record' entry for every logical
> router even if it doesn't have any NATs.
> 
> 

OK, this seems reasonable.

>>
>
> diff --git a/lib/ovn-util.c b/lib/ovn-util.c
> index 33105202f2..05e635a6b4 100644
> --- a/lib/ovn-util.c
> +++ b/lib/ovn-util.c
> @@ -395,7 +395,7 @@ extract_sbrec_binding_first_mac(const struct
 sbrec_port_binding *binding,
>  }
>
>  bool
> -lport_addresses_is_empty(struct lport_addresses *laddrs)
> +lport_addresses_is_empty(const struct lport_addresses *laddrs)
>  {
>  return !laddrs->n_ipv4_addrs && !laddrs->n_ipv6_addrs;
>  }
> @@ -405,6 +405,10 @@ destroy_lport_addresses(struct lport_addresses
 *laddrs)
>  {
>  free(laddrs->ipv4_addrs);
>  free(laddrs->ipv6_addrs);
> +laddrs->ipv4_addrs = NULL;
> +laddrs->ipv6_addrs = NULL;
> +laddrs->n_ipv4_addrs = 0;
> +laddrs->n_ipv6_addrs = 0;
>  }
>
>  /* Returns a string of the IP address of 'laddrs' that overlaps with
 'ip_s'.
> diff --git a/lib/ovn-util.h b/lib/ovn-util.h
> index bff50dbde9..5805415885 100644
> --- a/lib/ovn-util.h
> +++ b/lib/ovn-util.h
> @@ -112,7 +112,7 @@ bool extract_sbrec_binding_first_mac(const struct
 sbrec_port_binding *binding,
>  bool extract_lrp_networks__(char *mac, char **networks, size_t
 n_networks,
>  struct lport_addresses *laddrs);
>
> -bool lport_addresses_is_empty(struct lport_addresses *);
> +bool lport_addresses_is_empty(const struct lport_addresses *);
>  void destroy_lport_addresses(struct lport_addresses *);
>  const char *find_lport_address(const struct lport_addresses *laddrs,
> const char *ip_s);
> diff --git a/lib/stopwatch-names.h b/lib/stopwatch-names.h
> index 3452cc71cf..0a16da211e 100644
> --- a/lib/stopwatch-names.h
> +++ b/lib/stopwatch-names.h
> @@ -32,5 +32,6 @@
>  #define LFLOWS_TO_SB_STOPWATCH_NAME "lflows_to_sb"
>  #define PORT_GROUP_RUN_STOPWATCH_NAME "port_group_run"
>  #define SYNC_METERS_RUN_STOPWATCH_NAME "sync_meters_run"
> +#define LR_NAT_RUN_STOPWATCH_NAME "lr_nat_run"
>
>  #endif
> diff --git a/northd/automake.mk b/northd/automake.mk
> index cf622fc3c9..ae367a2a8b 100644
> --- a/northd/automake.mk
> +++ b/northd/automake.mk
> @@ -24,6 +24,8 @@ northd_ovn_northd_SOURCES = \
> 

Re: [ovs-dev] [PATCH ovn v2 05/18] northd: Add a new engine 'lr-nat' to manage lr NAT data.

2023-12-05 Thread Numan Siddique
On Thu, Nov 23, 2023 at 9:04 AM Dumitru Ceara  wrote:
>
> On 11/17/23 23:05, Numan Siddique wrote:
> > On Wed, Nov 15, 2023 at 1:27 AM Han Zhou  wrote:
> >>
> >> On Thu, Oct 26, 2023 at 11:15 AM  wrote:
> >>>
> >>> From: Numan Siddique 
> >>>
> >>> This new engine now maintains the NAT related data for each
> >>> logical router which was earlier maintained by the northd
> >>> engine node in the 'struct ovn_datapath'.  Main inputs to
> >>> this engine node are:
> >>>- northd
> >>>- NB logical router
> >>>
> >>> A record for each logical router is maintained in the 'lr_nats'
> >>> hmap table and this record
> >>>   - stores the ovn_nat's
> >>
> >> It seems the sentence is incomplete.
> >>
> >>>
> >>> Handlers are also added to handle the changes to both these
> >>> inputs.  This engine node becomes an input to 'lflow' node.
> >>> This essentially decouples the lr NAT data from the northd
> >>> engine node.
> >>>
> >>> Signed-off-by: Numan Siddique 
> >>> ---
> >>>  lib/ovn-util.c   |   6 +-
> >>>  lib/ovn-util.h   |   2 +-
> >>>  lib/stopwatch-names.h|   1 +
> >>>  northd/automake.mk   |   2 +
> >>>  northd/en-lflow.c|   5 +
> >>>  northd/en-lr-nat.c   | 498 +
> >>>  northd/en-lr-nat.h   | 134 ++
> >>>  northd/en-sync-sb.c  |  11 +-
> >>>  northd/inc-proc-northd.c |   9 +
> >>>  northd/northd.c  | 514 ++-
> >>>  northd/northd.h  |  32 ++-
> >>>  tests/ovn-northd.at  |  18 ++
> >>>  12 files changed, 877 insertions(+), 355 deletions(-)
> >>>  create mode 100644 northd/en-lr-nat.c
> >>>  create mode 100644 northd/en-lr-nat.h
>
> I would call these en-nat.{hc}, I think.  There's no NAT for switches.


In v3 I haven't renamed it.  I can rename it in the next version once
most of the review comments
are addressed  (it's a bit of a pain to rename and fix all the compiler errors).

But in this case I still prefer lr-nat (and the structures -
lr_nat_record, ... ) because,
 -  This engine node maintains the data for the NATs associated
with a logical router.  A router
 can have 0 or more NATs
 - We have a NB NAT table which is for each NAT entry, whereas
this engine node is for NATs associated with a router
and it could confuse and not convey the relationship between
router and NATs.
 -  This patch creates a 'lr_nat_record' entry for every logical
router even if it doesn't have any NATs.


>
> >>>
> >>> diff --git a/lib/ovn-util.c b/lib/ovn-util.c
> >>> index 33105202f2..05e635a6b4 100644
> >>> --- a/lib/ovn-util.c
> >>> +++ b/lib/ovn-util.c
> >>> @@ -395,7 +395,7 @@ extract_sbrec_binding_first_mac(const struct
> >> sbrec_port_binding *binding,
> >>>  }
> >>>
> >>>  bool
> >>> -lport_addresses_is_empty(struct lport_addresses *laddrs)
> >>> +lport_addresses_is_empty(const struct lport_addresses *laddrs)
> >>>  {
> >>>  return !laddrs->n_ipv4_addrs && !laddrs->n_ipv6_addrs;
> >>>  }
> >>> @@ -405,6 +405,10 @@ destroy_lport_addresses(struct lport_addresses
> >> *laddrs)
> >>>  {
> >>>  free(laddrs->ipv4_addrs);
> >>>  free(laddrs->ipv6_addrs);
> >>> +laddrs->ipv4_addrs = NULL;
> >>> +laddrs->ipv6_addrs = NULL;
> >>> +laddrs->n_ipv4_addrs = 0;
> >>> +laddrs->n_ipv6_addrs = 0;
> >>>  }
> >>>
> >>>  /* Returns a string of the IP address of 'laddrs' that overlaps with
> >> 'ip_s'.
> >>> diff --git a/lib/ovn-util.h b/lib/ovn-util.h
> >>> index bff50dbde9..5805415885 100644
> >>> --- a/lib/ovn-util.h
> >>> +++ b/lib/ovn-util.h
> >>> @@ -112,7 +112,7 @@ bool extract_sbrec_binding_first_mac(const struct
> >> sbrec_port_binding *binding,
> >>>  bool extract_lrp_networks__(char *mac, char **networks, size_t
> >> n_networks,
> >>>  struct lport_addresses *laddrs);
> >>>
> >>> -bool lport_addresses_is_empty(struct lport_addresses *);
> >>> +bool lport_addresses_is_empty(const struct lport_addresses *);
> >>>  void destroy_lport_addresses(struct lport_addresses *);
> >>>  const char *find_lport_address(const struct lport_addresses *laddrs,
> >>> const char *ip_s);
> >>> diff --git a/lib/stopwatch-names.h b/lib/stopwatch-names.h
> >>> index 3452cc71cf..0a16da211e 100644
> >>> --- a/lib/stopwatch-names.h
> >>> +++ b/lib/stopwatch-names.h
> >>> @@ -32,5 +32,6 @@
> >>>  #define LFLOWS_TO_SB_STOPWATCH_NAME "lflows_to_sb"
> >>>  #define PORT_GROUP_RUN_STOPWATCH_NAME "port_group_run"
> >>>  #define SYNC_METERS_RUN_STOPWATCH_NAME "sync_meters_run"
> >>> +#define LR_NAT_RUN_STOPWATCH_NAME "lr_nat_run"
> >>>
> >>>  #endif
> >>> diff --git a/northd/automake.mk b/northd/automake.mk
> >>> index cf622fc3c9..ae367a2a8b 100644
> >>> --- a/northd/automake.mk
> >>> +++ b/northd/automake.mk
> >>> @@ -24,6 +24,8 @@ northd_ovn_northd_SOURCES = \
> >>> northd/en-sync-from-sb.h \
> >>> northd/en-lb-data.c \
> >>> northd/en-lb-data.h \
> >>> +   northd/en-lr-nat.c 

Re: [ovs-dev] [PATCH ovn v2 05/18] northd: Add a new engine 'lr-nat' to manage lr NAT data.

2023-11-23 Thread Dumitru Ceara
On 11/17/23 23:05, Numan Siddique wrote:
> On Wed, Nov 15, 2023 at 1:27 AM Han Zhou  wrote:
>>
>> On Thu, Oct 26, 2023 at 11:15 AM  wrote:
>>>
>>> From: Numan Siddique 
>>>
>>> This new engine now maintains the NAT related data for each
>>> logical router which was earlier maintained by the northd
>>> engine node in the 'struct ovn_datapath'.  Main inputs to
>>> this engine node are:
>>>- northd
>>>- NB logical router
>>>
>>> A record for each logical router is maintained in the 'lr_nats'
>>> hmap table and this record
>>>   - stores the ovn_nat's
>>
>> It seems the sentence is incomplete.
>>
>>>
>>> Handlers are also added to handle the changes to both these
>>> inputs.  This engine node becomes an input to 'lflow' node.
>>> This essentially decouples the lr NAT data from the northd
>>> engine node.
>>>
>>> Signed-off-by: Numan Siddique 
>>> ---
>>>  lib/ovn-util.c   |   6 +-
>>>  lib/ovn-util.h   |   2 +-
>>>  lib/stopwatch-names.h|   1 +
>>>  northd/automake.mk   |   2 +
>>>  northd/en-lflow.c|   5 +
>>>  northd/en-lr-nat.c   | 498 +
>>>  northd/en-lr-nat.h   | 134 ++
>>>  northd/en-sync-sb.c  |  11 +-
>>>  northd/inc-proc-northd.c |   9 +
>>>  northd/northd.c  | 514 ++-
>>>  northd/northd.h  |  32 ++-
>>>  tests/ovn-northd.at  |  18 ++
>>>  12 files changed, 877 insertions(+), 355 deletions(-)
>>>  create mode 100644 northd/en-lr-nat.c
>>>  create mode 100644 northd/en-lr-nat.h

I would call these en-nat.{hc}, I think.  There's no NAT for switches.

>>>
>>> diff --git a/lib/ovn-util.c b/lib/ovn-util.c
>>> index 33105202f2..05e635a6b4 100644
>>> --- a/lib/ovn-util.c
>>> +++ b/lib/ovn-util.c
>>> @@ -395,7 +395,7 @@ extract_sbrec_binding_first_mac(const struct
>> sbrec_port_binding *binding,
>>>  }
>>>
>>>  bool
>>> -lport_addresses_is_empty(struct lport_addresses *laddrs)
>>> +lport_addresses_is_empty(const struct lport_addresses *laddrs)
>>>  {
>>>  return !laddrs->n_ipv4_addrs && !laddrs->n_ipv6_addrs;
>>>  }
>>> @@ -405,6 +405,10 @@ destroy_lport_addresses(struct lport_addresses
>> *laddrs)
>>>  {
>>>  free(laddrs->ipv4_addrs);
>>>  free(laddrs->ipv6_addrs);
>>> +laddrs->ipv4_addrs = NULL;
>>> +laddrs->ipv6_addrs = NULL;
>>> +laddrs->n_ipv4_addrs = 0;
>>> +laddrs->n_ipv6_addrs = 0;
>>>  }
>>>
>>>  /* Returns a string of the IP address of 'laddrs' that overlaps with
>> 'ip_s'.
>>> diff --git a/lib/ovn-util.h b/lib/ovn-util.h
>>> index bff50dbde9..5805415885 100644
>>> --- a/lib/ovn-util.h
>>> +++ b/lib/ovn-util.h
>>> @@ -112,7 +112,7 @@ bool extract_sbrec_binding_first_mac(const struct
>> sbrec_port_binding *binding,
>>>  bool extract_lrp_networks__(char *mac, char **networks, size_t
>> n_networks,
>>>  struct lport_addresses *laddrs);
>>>
>>> -bool lport_addresses_is_empty(struct lport_addresses *);
>>> +bool lport_addresses_is_empty(const struct lport_addresses *);
>>>  void destroy_lport_addresses(struct lport_addresses *);
>>>  const char *find_lport_address(const struct lport_addresses *laddrs,
>>> const char *ip_s);
>>> diff --git a/lib/stopwatch-names.h b/lib/stopwatch-names.h
>>> index 3452cc71cf..0a16da211e 100644
>>> --- a/lib/stopwatch-names.h
>>> +++ b/lib/stopwatch-names.h
>>> @@ -32,5 +32,6 @@
>>>  #define LFLOWS_TO_SB_STOPWATCH_NAME "lflows_to_sb"
>>>  #define PORT_GROUP_RUN_STOPWATCH_NAME "port_group_run"
>>>  #define SYNC_METERS_RUN_STOPWATCH_NAME "sync_meters_run"
>>> +#define LR_NAT_RUN_STOPWATCH_NAME "lr_nat_run"
>>>
>>>  #endif
>>> diff --git a/northd/automake.mk b/northd/automake.mk
>>> index cf622fc3c9..ae367a2a8b 100644
>>> --- a/northd/automake.mk
>>> +++ b/northd/automake.mk
>>> @@ -24,6 +24,8 @@ northd_ovn_northd_SOURCES = \
>>> northd/en-sync-from-sb.h \
>>> northd/en-lb-data.c \
>>> northd/en-lb-data.h \
>>> +   northd/en-lr-nat.c \
>>> +   northd/en-lr-nat.h \
>>> northd/inc-proc-northd.c \
>>> northd/inc-proc-northd.h \
>>> northd/ipam.c \
>>> diff --git a/northd/en-lflow.c b/northd/en-lflow.c
>>> index 96d03b7ada..22f398d419 100644
>>> --- a/northd/en-lflow.c
>>> +++ b/northd/en-lflow.c
>>> @@ -19,6 +19,7 @@
>>>  #include 
>>>
>>>  #include "en-lflow.h"
>>> +#include "en-lr-nat.h"
>>>  #include "en-northd.h"
>>>  #include "en-meters.h"
>>>
>>> @@ -40,6 +41,9 @@ lflow_get_input_data(struct engine_node *node,
>>>  engine_get_input_data("port_group", node);
>>>  struct sync_meters_data *sync_meters_data =
>>>  engine_get_input_data("sync_meters", node);
>>> +struct ed_type_lr_nat_data *lr_nat_data =
>>> +engine_get_input_data("lr_nat", node);
>>> +
>>>  lflow_input->nbrec_bfd_table =
>>>  EN_OVSDB_GET(engine_get_input("NB_bfd", node));
>>>  lflow_input->sbrec_bfd_table =
>>> @@ -61,6 +65,7 @@ lflow_get_input_data(struct engine_node 

Re: [ovs-dev] [PATCH ovn v2 05/18] northd: Add a new engine 'lr-nat' to manage lr NAT data.

2023-11-17 Thread Numan Siddique
On Wed, Nov 15, 2023 at 1:27 AM Han Zhou  wrote:
>
> On Thu, Oct 26, 2023 at 11:15 AM  wrote:
> >
> > From: Numan Siddique 
> >
> > This new engine now maintains the NAT related data for each
> > logical router which was earlier maintained by the northd
> > engine node in the 'struct ovn_datapath'.  Main inputs to
> > this engine node are:
> >- northd
> >- NB logical router
> >
> > A record for each logical router is maintained in the 'lr_nats'
> > hmap table and this record
> >   - stores the ovn_nat's
>
> It seems the sentence is incomplete.
>
> >
> > Handlers are also added to handle the changes to both these
> > inputs.  This engine node becomes an input to 'lflow' node.
> > This essentially decouples the lr NAT data from the northd
> > engine node.
> >
> > Signed-off-by: Numan Siddique 
> > ---
> >  lib/ovn-util.c   |   6 +-
> >  lib/ovn-util.h   |   2 +-
> >  lib/stopwatch-names.h|   1 +
> >  northd/automake.mk   |   2 +
> >  northd/en-lflow.c|   5 +
> >  northd/en-lr-nat.c   | 498 +
> >  northd/en-lr-nat.h   | 134 ++
> >  northd/en-sync-sb.c  |  11 +-
> >  northd/inc-proc-northd.c |   9 +
> >  northd/northd.c  | 514 ++-
> >  northd/northd.h  |  32 ++-
> >  tests/ovn-northd.at  |  18 ++
> >  12 files changed, 877 insertions(+), 355 deletions(-)
> >  create mode 100644 northd/en-lr-nat.c
> >  create mode 100644 northd/en-lr-nat.h
> >
> > diff --git a/lib/ovn-util.c b/lib/ovn-util.c
> > index 33105202f2..05e635a6b4 100644
> > --- a/lib/ovn-util.c
> > +++ b/lib/ovn-util.c
> > @@ -395,7 +395,7 @@ extract_sbrec_binding_first_mac(const struct
> sbrec_port_binding *binding,
> >  }
> >
> >  bool
> > -lport_addresses_is_empty(struct lport_addresses *laddrs)
> > +lport_addresses_is_empty(const struct lport_addresses *laddrs)
> >  {
> >  return !laddrs->n_ipv4_addrs && !laddrs->n_ipv6_addrs;
> >  }
> > @@ -405,6 +405,10 @@ destroy_lport_addresses(struct lport_addresses
> *laddrs)
> >  {
> >  free(laddrs->ipv4_addrs);
> >  free(laddrs->ipv6_addrs);
> > +laddrs->ipv4_addrs = NULL;
> > +laddrs->ipv6_addrs = NULL;
> > +laddrs->n_ipv4_addrs = 0;
> > +laddrs->n_ipv6_addrs = 0;
> >  }
> >
> >  /* Returns a string of the IP address of 'laddrs' that overlaps with
> 'ip_s'.
> > diff --git a/lib/ovn-util.h b/lib/ovn-util.h
> > index bff50dbde9..5805415885 100644
> > --- a/lib/ovn-util.h
> > +++ b/lib/ovn-util.h
> > @@ -112,7 +112,7 @@ bool extract_sbrec_binding_first_mac(const struct
> sbrec_port_binding *binding,
> >  bool extract_lrp_networks__(char *mac, char **networks, size_t
> n_networks,
> >  struct lport_addresses *laddrs);
> >
> > -bool lport_addresses_is_empty(struct lport_addresses *);
> > +bool lport_addresses_is_empty(const struct lport_addresses *);
> >  void destroy_lport_addresses(struct lport_addresses *);
> >  const char *find_lport_address(const struct lport_addresses *laddrs,
> > const char *ip_s);
> > diff --git a/lib/stopwatch-names.h b/lib/stopwatch-names.h
> > index 3452cc71cf..0a16da211e 100644
> > --- a/lib/stopwatch-names.h
> > +++ b/lib/stopwatch-names.h
> > @@ -32,5 +32,6 @@
> >  #define LFLOWS_TO_SB_STOPWATCH_NAME "lflows_to_sb"
> >  #define PORT_GROUP_RUN_STOPWATCH_NAME "port_group_run"
> >  #define SYNC_METERS_RUN_STOPWATCH_NAME "sync_meters_run"
> > +#define LR_NAT_RUN_STOPWATCH_NAME "lr_nat_run"
> >
> >  #endif
> > diff --git a/northd/automake.mk b/northd/automake.mk
> > index cf622fc3c9..ae367a2a8b 100644
> > --- a/northd/automake.mk
> > +++ b/northd/automake.mk
> > @@ -24,6 +24,8 @@ northd_ovn_northd_SOURCES = \
> > northd/en-sync-from-sb.h \
> > northd/en-lb-data.c \
> > northd/en-lb-data.h \
> > +   northd/en-lr-nat.c \
> > +   northd/en-lr-nat.h \
> > northd/inc-proc-northd.c \
> > northd/inc-proc-northd.h \
> > northd/ipam.c \
> > diff --git a/northd/en-lflow.c b/northd/en-lflow.c
> > index 96d03b7ada..22f398d419 100644
> > --- a/northd/en-lflow.c
> > +++ b/northd/en-lflow.c
> > @@ -19,6 +19,7 @@
> >  #include 
> >
> >  #include "en-lflow.h"
> > +#include "en-lr-nat.h"
> >  #include "en-northd.h"
> >  #include "en-meters.h"
> >
> > @@ -40,6 +41,9 @@ lflow_get_input_data(struct engine_node *node,
> >  engine_get_input_data("port_group", node);
> >  struct sync_meters_data *sync_meters_data =
> >  engine_get_input_data("sync_meters", node);
> > +struct ed_type_lr_nat_data *lr_nat_data =
> > +engine_get_input_data("lr_nat", node);
> > +
> >  lflow_input->nbrec_bfd_table =
> >  EN_OVSDB_GET(engine_get_input("NB_bfd", node));
> >  lflow_input->sbrec_bfd_table =
> > @@ -61,6 +65,7 @@ lflow_get_input_data(struct engine_node *node,
> >  lflow_input->ls_ports = _data->ls_ports;
> >  lflow_input->lr_ports = _data->lr_ports;
> >  

Re: [ovs-dev] [PATCH ovn v2 05/18] northd: Add a new engine 'lr-nat' to manage lr NAT data.

2023-11-14 Thread Han Zhou
On Thu, Oct 26, 2023 at 11:15 AM  wrote:
>
> From: Numan Siddique 
>
> This new engine now maintains the NAT related data for each
> logical router which was earlier maintained by the northd
> engine node in the 'struct ovn_datapath'.  Main inputs to
> this engine node are:
>- northd
>- NB logical router
>
> A record for each logical router is maintained in the 'lr_nats'
> hmap table and this record
>   - stores the ovn_nat's

It seems the sentence is incomplete.

>
> Handlers are also added to handle the changes to both these
> inputs.  This engine node becomes an input to 'lflow' node.
> This essentially decouples the lr NAT data from the northd
> engine node.
>
> Signed-off-by: Numan Siddique 
> ---
>  lib/ovn-util.c   |   6 +-
>  lib/ovn-util.h   |   2 +-
>  lib/stopwatch-names.h|   1 +
>  northd/automake.mk   |   2 +
>  northd/en-lflow.c|   5 +
>  northd/en-lr-nat.c   | 498 +
>  northd/en-lr-nat.h   | 134 ++
>  northd/en-sync-sb.c  |  11 +-
>  northd/inc-proc-northd.c |   9 +
>  northd/northd.c  | 514 ++-
>  northd/northd.h  |  32 ++-
>  tests/ovn-northd.at  |  18 ++
>  12 files changed, 877 insertions(+), 355 deletions(-)
>  create mode 100644 northd/en-lr-nat.c
>  create mode 100644 northd/en-lr-nat.h
>
> diff --git a/lib/ovn-util.c b/lib/ovn-util.c
> index 33105202f2..05e635a6b4 100644
> --- a/lib/ovn-util.c
> +++ b/lib/ovn-util.c
> @@ -395,7 +395,7 @@ extract_sbrec_binding_first_mac(const struct
sbrec_port_binding *binding,
>  }
>
>  bool
> -lport_addresses_is_empty(struct lport_addresses *laddrs)
> +lport_addresses_is_empty(const struct lport_addresses *laddrs)
>  {
>  return !laddrs->n_ipv4_addrs && !laddrs->n_ipv6_addrs;
>  }
> @@ -405,6 +405,10 @@ destroy_lport_addresses(struct lport_addresses
*laddrs)
>  {
>  free(laddrs->ipv4_addrs);
>  free(laddrs->ipv6_addrs);
> +laddrs->ipv4_addrs = NULL;
> +laddrs->ipv6_addrs = NULL;
> +laddrs->n_ipv4_addrs = 0;
> +laddrs->n_ipv6_addrs = 0;
>  }
>
>  /* Returns a string of the IP address of 'laddrs' that overlaps with
'ip_s'.
> diff --git a/lib/ovn-util.h b/lib/ovn-util.h
> index bff50dbde9..5805415885 100644
> --- a/lib/ovn-util.h
> +++ b/lib/ovn-util.h
> @@ -112,7 +112,7 @@ bool extract_sbrec_binding_first_mac(const struct
sbrec_port_binding *binding,
>  bool extract_lrp_networks__(char *mac, char **networks, size_t
n_networks,
>  struct lport_addresses *laddrs);
>
> -bool lport_addresses_is_empty(struct lport_addresses *);
> +bool lport_addresses_is_empty(const struct lport_addresses *);
>  void destroy_lport_addresses(struct lport_addresses *);
>  const char *find_lport_address(const struct lport_addresses *laddrs,
> const char *ip_s);
> diff --git a/lib/stopwatch-names.h b/lib/stopwatch-names.h
> index 3452cc71cf..0a16da211e 100644
> --- a/lib/stopwatch-names.h
> +++ b/lib/stopwatch-names.h
> @@ -32,5 +32,6 @@
>  #define LFLOWS_TO_SB_STOPWATCH_NAME "lflows_to_sb"
>  #define PORT_GROUP_RUN_STOPWATCH_NAME "port_group_run"
>  #define SYNC_METERS_RUN_STOPWATCH_NAME "sync_meters_run"
> +#define LR_NAT_RUN_STOPWATCH_NAME "lr_nat_run"
>
>  #endif
> diff --git a/northd/automake.mk b/northd/automake.mk
> index cf622fc3c9..ae367a2a8b 100644
> --- a/northd/automake.mk
> +++ b/northd/automake.mk
> @@ -24,6 +24,8 @@ northd_ovn_northd_SOURCES = \
> northd/en-sync-from-sb.h \
> northd/en-lb-data.c \
> northd/en-lb-data.h \
> +   northd/en-lr-nat.c \
> +   northd/en-lr-nat.h \
> northd/inc-proc-northd.c \
> northd/inc-proc-northd.h \
> northd/ipam.c \
> diff --git a/northd/en-lflow.c b/northd/en-lflow.c
> index 96d03b7ada..22f398d419 100644
> --- a/northd/en-lflow.c
> +++ b/northd/en-lflow.c
> @@ -19,6 +19,7 @@
>  #include 
>
>  #include "en-lflow.h"
> +#include "en-lr-nat.h"
>  #include "en-northd.h"
>  #include "en-meters.h"
>
> @@ -40,6 +41,9 @@ lflow_get_input_data(struct engine_node *node,
>  engine_get_input_data("port_group", node);
>  struct sync_meters_data *sync_meters_data =
>  engine_get_input_data("sync_meters", node);
> +struct ed_type_lr_nat_data *lr_nat_data =
> +engine_get_input_data("lr_nat", node);
> +
>  lflow_input->nbrec_bfd_table =
>  EN_OVSDB_GET(engine_get_input("NB_bfd", node));
>  lflow_input->sbrec_bfd_table =
> @@ -61,6 +65,7 @@ lflow_get_input_data(struct engine_node *node,
>  lflow_input->ls_ports = _data->ls_ports;
>  lflow_input->lr_ports = _data->lr_ports;
>  lflow_input->ls_port_groups = _data->ls_port_groups;
> +lflow_input->lr_nats = _nat_data->lr_nats;
>  lflow_input->meter_groups = _meters_data->meter_groups;
>  lflow_input->lb_datapaths_map = _data->lb_datapaths_map;
>  lflow_input->svc_monitor_map = _data->svc_monitor_map;
> diff --git a/northd/en-lr-nat.c 

[ovs-dev] [PATCH ovn v2 05/18] northd: Add a new engine 'lr-nat' to manage lr NAT data.

2023-10-26 Thread numans
From: Numan Siddique 

This new engine now maintains the NAT related data for each
logical router which was earlier maintained by the northd
engine node in the 'struct ovn_datapath'.  Main inputs to
this engine node are:
   - northd
   - NB logical router

A record for each logical router is maintained in the 'lr_nats'
hmap table and this record
  - stores the ovn_nat's

Handlers are also added to handle the changes to both these
inputs.  This engine node becomes an input to 'lflow' node.
This essentially decouples the lr NAT data from the northd
engine node.

Signed-off-by: Numan Siddique 
---
 lib/ovn-util.c   |   6 +-
 lib/ovn-util.h   |   2 +-
 lib/stopwatch-names.h|   1 +
 northd/automake.mk   |   2 +
 northd/en-lflow.c|   5 +
 northd/en-lr-nat.c   | 498 +
 northd/en-lr-nat.h   | 134 ++
 northd/en-sync-sb.c  |  11 +-
 northd/inc-proc-northd.c |   9 +
 northd/northd.c  | 514 ++-
 northd/northd.h  |  32 ++-
 tests/ovn-northd.at  |  18 ++
 12 files changed, 877 insertions(+), 355 deletions(-)
 create mode 100644 northd/en-lr-nat.c
 create mode 100644 northd/en-lr-nat.h

diff --git a/lib/ovn-util.c b/lib/ovn-util.c
index 33105202f2..05e635a6b4 100644
--- a/lib/ovn-util.c
+++ b/lib/ovn-util.c
@@ -395,7 +395,7 @@ extract_sbrec_binding_first_mac(const struct 
sbrec_port_binding *binding,
 }
 
 bool
-lport_addresses_is_empty(struct lport_addresses *laddrs)
+lport_addresses_is_empty(const struct lport_addresses *laddrs)
 {
 return !laddrs->n_ipv4_addrs && !laddrs->n_ipv6_addrs;
 }
@@ -405,6 +405,10 @@ destroy_lport_addresses(struct lport_addresses *laddrs)
 {
 free(laddrs->ipv4_addrs);
 free(laddrs->ipv6_addrs);
+laddrs->ipv4_addrs = NULL;
+laddrs->ipv6_addrs = NULL;
+laddrs->n_ipv4_addrs = 0;
+laddrs->n_ipv6_addrs = 0;
 }
 
 /* Returns a string of the IP address of 'laddrs' that overlaps with 'ip_s'.
diff --git a/lib/ovn-util.h b/lib/ovn-util.h
index bff50dbde9..5805415885 100644
--- a/lib/ovn-util.h
+++ b/lib/ovn-util.h
@@ -112,7 +112,7 @@ bool extract_sbrec_binding_first_mac(const struct 
sbrec_port_binding *binding,
 bool extract_lrp_networks__(char *mac, char **networks, size_t n_networks,
 struct lport_addresses *laddrs);
 
-bool lport_addresses_is_empty(struct lport_addresses *);
+bool lport_addresses_is_empty(const struct lport_addresses *);
 void destroy_lport_addresses(struct lport_addresses *);
 const char *find_lport_address(const struct lport_addresses *laddrs,
const char *ip_s);
diff --git a/lib/stopwatch-names.h b/lib/stopwatch-names.h
index 3452cc71cf..0a16da211e 100644
--- a/lib/stopwatch-names.h
+++ b/lib/stopwatch-names.h
@@ -32,5 +32,6 @@
 #define LFLOWS_TO_SB_STOPWATCH_NAME "lflows_to_sb"
 #define PORT_GROUP_RUN_STOPWATCH_NAME "port_group_run"
 #define SYNC_METERS_RUN_STOPWATCH_NAME "sync_meters_run"
+#define LR_NAT_RUN_STOPWATCH_NAME "lr_nat_run"
 
 #endif
diff --git a/northd/automake.mk b/northd/automake.mk
index cf622fc3c9..ae367a2a8b 100644
--- a/northd/automake.mk
+++ b/northd/automake.mk
@@ -24,6 +24,8 @@ northd_ovn_northd_SOURCES = \
northd/en-sync-from-sb.h \
northd/en-lb-data.c \
northd/en-lb-data.h \
+   northd/en-lr-nat.c \
+   northd/en-lr-nat.h \
northd/inc-proc-northd.c \
northd/inc-proc-northd.h \
northd/ipam.c \
diff --git a/northd/en-lflow.c b/northd/en-lflow.c
index 96d03b7ada..22f398d419 100644
--- a/northd/en-lflow.c
+++ b/northd/en-lflow.c
@@ -19,6 +19,7 @@
 #include 
 
 #include "en-lflow.h"
+#include "en-lr-nat.h"
 #include "en-northd.h"
 #include "en-meters.h"
 
@@ -40,6 +41,9 @@ lflow_get_input_data(struct engine_node *node,
 engine_get_input_data("port_group", node);
 struct sync_meters_data *sync_meters_data =
 engine_get_input_data("sync_meters", node);
+struct ed_type_lr_nat_data *lr_nat_data =
+engine_get_input_data("lr_nat", node);
+
 lflow_input->nbrec_bfd_table =
 EN_OVSDB_GET(engine_get_input("NB_bfd", node));
 lflow_input->sbrec_bfd_table =
@@ -61,6 +65,7 @@ lflow_get_input_data(struct engine_node *node,
 lflow_input->ls_ports = _data->ls_ports;
 lflow_input->lr_ports = _data->lr_ports;
 lflow_input->ls_port_groups = _data->ls_port_groups;
+lflow_input->lr_nats = _nat_data->lr_nats;
 lflow_input->meter_groups = _meters_data->meter_groups;
 lflow_input->lb_datapaths_map = _data->lb_datapaths_map;
 lflow_input->svc_monitor_map = _data->svc_monitor_map;
diff --git a/northd/en-lr-nat.c b/northd/en-lr-nat.c
new file mode 100644
index 00..0d332d300b
--- /dev/null
+++ b/northd/en-lr-nat.c
@@ -0,0 +1,498 @@
+/*
+ * Copyright (c) 2023, Red Hat, Inc.
+ *
+ * Licensed under the Apache License, Version 2.0 (the "License");
+ * you may not use this file except in compliance with the License.
+ * You