On 1/26/24 06:27, Numan Siddique wrote:
> On Wed, Jan 17, 2024 at 10:17 AM Dumitru Ceara <dce...@redhat.com> wrote:
>>
>> On 1/11/24 16:30, num...@ovn.org wrote:
>>> From: Numan Siddique <num...@ovn.org>
>>>
>>> This new engine now maintains the load balancer and ACL data of a
>>> logical switch which was earlier part of northd engine node data.
>>> The main inputs to this engine are:
>>> - northd node
>>> - Port group node
>>>
>>> A record for each logical switch is maintained in the 'ls_statefuls'
>>> hmap table and this record stores the below data which was earlier
>>> part of 'struct ovn_datapath'.
>>>
>>> - bool has_stateful_acl;
>>> - bool has_lb_vip;
>>> - bool has_acls;
>>> - uint64_t max_acl_tier;
>>>
>>> This engine node becomes an input to 'lflow' node.
>>>
>>> If a logical switch is configured with
>>> - DHCP for all or some of its ports
>>> - stateful ACLs and/or load balancers
>>>
>>> ovn-northd was adding flows to commit the DHCP response generated by
>>> ovn-controller to conntrack. With this patch we don't commit the
>>> DHCP response to conntrack anymore for the following reasons:
>>> 1. There is no need to actually commit the response.
>>> 2. Since stateful information is moved to 'ls_stateful' node, it
>>> becomes ineffecient to access ls_stateful's data
>>> ('has_lb_vip' and 'has_acls') in build_dhcpv4_options_flows().
>>>
>>> Signed-off-by: Numan Siddique <num...@ovn.org>
>>> ---
>>
>> Hi Numan,
>>
>> Overall this looks correct to me. I only had a few minor comments,
>> please see below.
>>
>> If you address them feel free to add my ack:
>>
>> Acked-by: Dumitru Ceara <dce...@redhat.com>
>>
>> Thanks,
>> Dumitru
>>
>>> lib/stopwatch-names.h | 1 +
>>> northd/automake.mk | 2 +
>>> northd/en-lflow.c | 4 +
>>> northd/en-lr-stateful.c | 6 +-
>>> northd/en-lr-stateful.h | 2 +
>>> northd/en-ls-stateful.c | 440 +++++++++++++++++++++++++++++++++++++++
>>> northd/en-ls-stateful.h | 81 +++++++
>>> northd/en-northd.c | 2 +-
>>> northd/en-port-group.h | 3 +
>>> northd/inc-proc-northd.c | 7 +
>>> northd/northd.c | 335 +++++++++++++++--------------
>>> northd/northd.h | 29 ++-
>>> northd/ovn-northd.c | 1 +
>>> 13 files changed, 753 insertions(+), 160 deletions(-)
>>> create mode 100644 northd/en-ls-stateful.c
>>> create mode 100644 northd/en-ls-stateful.h
>>>
>>> diff --git a/lib/stopwatch-names.h b/lib/stopwatch-names.h
>>> index e5e41fbfd8..c865f125be 100644
>>> --- a/lib/stopwatch-names.h
>>> +++ b/lib/stopwatch-names.h
>>> @@ -31,5 +31,6 @@
>>> #define SYNC_METERS_RUN_STOPWATCH_NAME "sync_meters_run"
>>> #define LR_NAT_RUN_STOPWATCH_NAME "lr_nat_run"
>>> #define LR_STATEFUL_RUN_STOPWATCH_NAME "lr_stateful"
>>> +#define LS_STATEFUL_RUN_STOPWATCH_NAME "ls_stateful"
>>>
>>> #endif
>>> diff --git a/northd/automake.mk b/northd/automake.mk
>>> index b886356c9c..a178541759 100644
>>> --- a/northd/automake.mk
>>> +++ b/northd/automake.mk
>>> @@ -28,6 +28,8 @@ northd_ovn_northd_SOURCES = \
>>> northd/en-lr-nat.h \
>>> northd/en-lr-stateful.c \
>>> northd/en-lr-stateful.h \
>>> + northd/en-ls-stateful.c \
>>> + northd/en-ls-stateful.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 a3a0d62f15..eb6b2a8666 100644
>>> --- a/northd/en-lflow.c
>>> +++ b/northd/en-lflow.c
>>> @@ -21,6 +21,7 @@
>>> #include "en-lflow.h"
>>> #include "en-lr-nat.h"
>>> #include "en-lr-stateful.h"
>>> +#include "en-ls-stateful.h"
>>> #include "en-northd.h"
>>> #include "en-meters.h"
>>>
>>> @@ -44,6 +45,8 @@ lflow_get_input_data(struct engine_node *node,
>>> engine_get_input_data("sync_meters", node);
>>> struct ed_type_lr_stateful *lr_stateful_data =
>>> engine_get_input_data("lr_stateful", node);
>>> + struct ed_type_ls_stateful *ls_stateful_data =
>>> + engine_get_input_data("ls_stateful", node);
>>>
>>> lflow_input->nbrec_bfd_table =
>>> EN_OVSDB_GET(engine_get_input("NB_bfd", node));
>>> @@ -67,6 +70,7 @@ lflow_get_input_data(struct engine_node *node,
>>> lflow_input->lr_ports = &northd_data->lr_ports;
>>> lflow_input->ls_port_groups = &pg_data->ls_port_groups;
>>> lflow_input->lr_stateful_table = &lr_stateful_data->table;
>>> + lflow_input->ls_stateful_table = &ls_stateful_data->table;
>>> lflow_input->meter_groups = &sync_meters_data->meter_groups;
>>> lflow_input->lb_datapaths_map = &northd_data->lb_datapaths_map;
>>> lflow_input->svc_monitor_map = &northd_data->svc_monitor_map;
>>> diff --git a/northd/en-lr-stateful.c b/northd/en-lr-stateful.c
>>> index dedfdf6747..4287aaddc9 100644
>>> --- a/northd/en-lr-stateful.c
>>> +++ b/northd/en-lr-stateful.c
>>> @@ -295,7 +295,9 @@ lr_stateful_lb_data_handler(struct engine_node *node,
>>> void *data_)
>>> /* For all the modified lr_stateful records (re)build the
>>> * vip nats. */
>>> HMAPX_FOR_EACH (hmapx_node, &data->trk_data.crupdated) {
>>> - lr_stateful_build_vip_nats(hmapx_node->data);
>>> + struct lr_stateful_record *lr_stateful_rec = hmapx_node->data;
>>> + lr_stateful_build_vip_nats(lr_stateful_rec);
>>> + lr_stateful_rec->has_lb_vip =
>>> od_has_lb_vip(lr_stateful_rec->od);
>>> }
>>>
>>> engine_set_node_state(node, EN_UPDATED);
>>> @@ -510,6 +512,8 @@ lr_stateful_record_init(struct lr_stateful_record
>>> *lr_stateful_rec,
>>> if (nbr->n_nat) {
>>> lr_stateful_build_vip_nats(lr_stateful_rec);
>>> }
>>> +
>>> + lr_stateful_rec->has_lb_vip = od_has_lb_vip(lr_stateful_rec->od);
>>> }
>>>
>>> static struct lr_stateful_input
>>> diff --git a/northd/en-lr-stateful.h b/northd/en-lr-stateful.h
>>> index 74029c9a6c..5d647d8863 100644
>>> --- a/northd/en-lr-stateful.h
>>> +++ b/northd/en-lr-stateful.h
>>> @@ -49,6 +49,8 @@ struct lr_stateful_record {
>>> const struct ovn_datapath *od;
>>> const struct lr_nat_record *lrnat_rec;
>>>
>>> + bool has_lb_vip;
>>> +
>>> /* Load Balancer vIPs relevant for this datapath. */
>>> struct ovn_lb_ip_set *lb_ips;
>>>
>>> diff --git a/northd/en-ls-stateful.c b/northd/en-ls-stateful.c
>>> new file mode 100644
>>> index 0000000000..5fa305bc29
>>> --- /dev/null
>>> +++ b/northd/en-ls-stateful.c
>>> @@ -0,0 +1,440 @@
>>> +/*
>>> + * Copyright (c) 2023, Red Hat, Inc.
>>
>> 2024
>
> Ack
>
>>
>>> + *
>>> + * Licensed under the Apache License, Version 2.0 (the "License");
>>> + * you may not use this file except in compliance with the License.
>>> + * You may obtain a copy of the License at:
>>> + *
>>> + * http://www.apache.org/licenses/LICENSE-2.0
>>> + *
>>> + * Unless required by applicable law or agreed to in writing, software
>>> + * distributed under the License is distributed on an "AS IS" BASIS,
>>> + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
>>> + * See the License for the specific language governing permissions and
>>> + * limitations under the License.
>>> + */
>>> +
>>> +#include <config.h>
>>> +
>>> +#include <getopt.h>
>>> +#include <stdlib.h>
>>> +#include <stdio.h>
>>> +
>>> +/* OVS includes */
>>> +#include "include/openvswitch/hmap.h"
>>> +#include "lib/bitmap.h"
>>> +#include "lib/socket-util.h"
>>> +#include "lib/uuidset.h"
>>> +#include "openvswitch/util.h"
>>> +#include "openvswitch/vlog.h"
>>> +#include "stopwatch.h"
>>> +
>>> +/* OVN includes */
>>> +#include "en-lb-data.h"
>>> +#include "en-ls-stateful.h"
>>> +#include "en-port-group.h"
>>> +#include "lib/inc-proc-eng.h"
>>> +#include "lib/lb.h"
>>> +#include "lib/ovn-nb-idl.h"
>>> +#include "lib/ovn-sb-idl.h"
>>> +#include "lib/ovn-util.h"
>>> +#include "lib/stopwatch-names.h"
>>> +#include "northd.h"
>>> +
>>> +VLOG_DEFINE_THIS_MODULE(en_ls_stateful);
>>> +
>>> +/* Static function declarations. */
>>> +static void ls_stateful_table_init(struct ls_stateful_table *);
>>> +static void ls_stateful_table_clear(struct ls_stateful_table *);
>>> +static void ls_stateful_table_destroy(struct ls_stateful_table *);
>>> +static struct ls_stateful_record *ls_stateful_table_find_(
>>> + const struct ls_stateful_table *, const struct nbrec_logical_switch *);
>>> +static void ls_stateful_table_build(struct ls_stateful_table *,
>>> + const struct ovn_datapaths
>>> *ls_datapaths,
>>> + const struct ls_port_group_table *);
>>> +
>>> +static struct ls_stateful_input ls_stateful_get_input_data(
>>> + struct engine_node *);
>>> +
>>> +static struct ls_stateful_record *ls_stateful_record_create(
>>> + struct ls_stateful_table *,
>>> + const struct ovn_datapath *,
>>> + const struct ls_port_group_table *);
>>> +static void ls_stateful_record_destroy(struct ls_stateful_record *);
>>> +static void ls_stateful_record_init(
>>> + struct ls_stateful_record *,
>>> + const struct ovn_datapath *,
>>> + const struct ls_port_group *,
>>> + const struct ls_port_group_table *);
>>> +static void ls_stateful_record_reinit(
>>> + struct ls_stateful_record *,
>>> + const struct ls_port_group *,
>>> + const struct ls_port_group_table *);
>>> +static bool ls_has_lb_vip(const struct ovn_datapath *);
>>> +static void ls_stateful_record_set_acl_flags(
>>> + struct ls_stateful_record *, const struct ovn_datapath *,
>>> + const struct ls_port_group *, const struct ls_port_group_table *);
>>> +static bool ls_stateful_record_set_acl_flags_(struct ls_stateful_record *,
>>> + struct nbrec_acl **,
>>> + size_t n_acls);
>>> +static struct ls_stateful_input ls_stateful_get_input_data(
>>> + struct engine_node *);
>>> +
>>> +struct ls_stateful_input {
>>> + const struct ls_port_group_table *ls_port_groups;
>>> + const struct ovn_datapaths *ls_datapaths;
>>> +};
>>> +
>>> +/* public functions. */
>>> +const struct ls_stateful_record *
>>> +ls_stateful_table_find(const struct ls_stateful_table *table,
>>> + const struct nbrec_logical_switch *nbs)
>>> +{
>>> + return ls_stateful_table_find_(table, nbs);
>>> +}
>>> +
>>> +void *
>>> +en_ls_stateful_init(struct engine_node *node OVS_UNUSED,
>>> + struct engine_arg *arg OVS_UNUSED)
>>> +{
>>> + struct ed_type_ls_stateful *data = xzalloc(sizeof *data);
>>> + ls_stateful_table_init(&data->table);
>>> + hmapx_init(&data->trk_data.crupdated);
>>> + return data;
>>> +}
>>> +
>>> +void
>>> +en_ls_stateful_cleanup(void *data_)
>>> +{
>>> + struct ed_type_ls_stateful *data = data_;
>>> + ls_stateful_table_destroy(&data->table);
>>> + hmapx_destroy(&data->trk_data.crupdated);
>>> +}
>>> +
>>> +void
>>> +en_ls_stateful_clear_tracked_data(void *data_)
>>> +{
>>> + struct ed_type_ls_stateful *data = data_;
>>> + hmapx_clear(&data->trk_data.crupdated);
>>> +}
>>> +
>>> +void
>>> +en_ls_stateful_run(struct engine_node *node, void *data_)
>>> +{
>>> + struct ls_stateful_input input_data = ls_stateful_get_input_data(node);
>>> + struct ed_type_ls_stateful *data = data_;
>>> +
>>> + stopwatch_start(LS_STATEFUL_RUN_STOPWATCH_NAME, time_msec());
>>> +
>>> + ls_stateful_table_clear(&data->table);
>>> + ls_stateful_table_build(&data->table, input_data.ls_datapaths,
>>> + input_data.ls_port_groups);
>>> +
>>> + stopwatch_stop(LS_STATEFUL_RUN_STOPWATCH_NAME, time_msec());
>>> + engine_set_node_state(node, EN_UPDATED);
>>> +}
>>> +
>>> +/* Handler functions. */
>>> +bool
>>> +ls_stateful_northd_handler(struct engine_node *node, void *data_)
>>> +{
>>> + struct northd_data *northd_data = engine_get_input_data("northd",
>>> node);
>>> + if (!northd_has_tracked_data(&northd_data->trk_data)) {
>>> + return false;
>>> + }
>>> +
>>> + if (!northd_has_ls_lbs_in_tracked_data(&northd_data->trk_data) &&
>>> + !northd_has_ls_acls_in_tracked_data(&northd_data->trk_data)) {
>>> + return true;
>>> + }
>>> +
>>> + struct northd_tracked_data *nd_changes = &northd_data->trk_data;
>>> + struct ls_stateful_input input_data = ls_stateful_get_input_data(node);
>>> + struct ed_type_ls_stateful *data = data_;
>>> + struct hmapx_node *hmapx_node;
>>> +
>>> + struct hmapx changed_stateful_od =
>>> HMAPX_INITIALIZER(&changed_stateful_od);
>>> + HMAPX_FOR_EACH (hmapx_node, &nd_changes->ls_with_changed_lbs) {
>>> + hmapx_add(&changed_stateful_od, hmapx_node->data);
>>> + }
>>> +
>>> + HMAPX_FOR_EACH (hmapx_node, &nd_changes->ls_with_changed_acls) {
>>> + hmapx_add(&changed_stateful_od, hmapx_node->data);
>>> + }
>>> +
>>> + HMAPX_FOR_EACH (hmapx_node, &changed_stateful_od) {
>>> + const struct ovn_datapath *od = hmapx_node->data;
>>> +
>>> + struct ls_stateful_record *ls_stateful_rec =
>>> ls_stateful_table_find_(
>>> + &data->table, od->nbs);
>>> + ovs_assert(ls_stateful_rec);
>>> + ls_stateful_record_reinit(ls_stateful_rec, NULL,
>>> + input_data.ls_port_groups);
>>> +
>>> + /* Add the ls_stateful_rec to the tracking data. */
>>> + hmapx_add(&data->trk_data.crupdated, ls_stateful_rec);
>>> + }
>>> +
>>> + if (ls_stateful_has_tracked_data(&data->trk_data)) {
>>> + engine_set_node_state(node, EN_UPDATED);
>>> + }
>>> +
>>> + hmapx_destroy(&changed_stateful_od);
>>> +
>>> + return true;
>>> +}
>>> +
>>> +bool
>>> +ls_stateful_port_group_handler(struct engine_node *node, void *data_)
>>> +{
>>> + struct port_group_data *pg_data =
>>> + engine_get_input_data("port_group", node);
>>> +
>>> + if (pg_data->ls_port_groups_sets_changed) {
>>> + return false;
>>> + }
>>> +
>>> + /* port_group engine node doesn't provide the tracking data yet.
>>> + * Loop through all the ls port groups and update the ls_stateful_rec.
>>> + * This is still better than returning false. */
>>> + struct ls_stateful_input input_data = ls_stateful_get_input_data(node);
>>> + struct ed_type_ls_stateful *data = data_;
>>> + const struct ls_port_group *ls_pg;
>>> +
>>> + LS_PORT_GROUP_TABLE_FOR_EACH (ls_pg, input_data.ls_port_groups) {
>>> + struct ls_stateful_record *ls_stateful_rec =
>>> + ls_stateful_table_find_(&data->table, ls_pg->nbs);
>>> + ovs_assert(ls_stateful_rec);
>>> +
>>> + bool had_stateful_acl = ls_stateful_rec->has_stateful_acl;
>>> + uint64_t max_acl_tier = ls_stateful_rec->max_acl_tier;
>>> + bool had_acls = ls_stateful_rec->has_acls;
>>> + bool modified = false;
>>> +
>>> + ls_stateful_record_reinit(ls_stateful_rec, ls_pg,
>>> + input_data.ls_port_groups);
>>> +
>>> + if ((had_stateful_acl != ls_stateful_rec->has_stateful_acl)
>>> + || (had_acls != ls_stateful_rec->has_acls)
>>> + || max_acl_tier != ls_stateful_rec->max_acl_tier) {
>>> + modified = true;
>>> + }
>>> +
>>> + if (modified) {
>>> + /* Add the ls_stateful_rec to the tracking data. */
>>> + hmapx_add(&data->trk_data.crupdated, ls_stateful_rec);
>>> + }
>>> + }
>>> +
>>> + if (ls_stateful_has_tracked_data(&data->trk_data)) {
>>> + engine_set_node_state(node, EN_UPDATED);
>>> + }
>>> + return true;
>>> +}
>>> +
>>> +/* static functions. */
>>> +static void
>>> +ls_stateful_table_init(struct ls_stateful_table *table)
>>> +{
>>> + *table = (struct ls_stateful_table) {
>>> + .entries = HMAP_INITIALIZER(&table->entries),
>>> + };
>>> +}
>>> +
>>> +static void
>>> +ls_stateful_table_destroy(struct ls_stateful_table *table)
>>> +{
>>> + ls_stateful_table_clear(table);
>>> + hmap_destroy(&table->entries);
>>> +}
>>> +
>>> +static void
>>> +ls_stateful_table_clear(struct ls_stateful_table *table)
>>> +{
>>> + struct ls_stateful_record *ls_stateful_rec;
>>> + HMAP_FOR_EACH_POP (ls_stateful_rec, key_node, &table->entries) {
>>> + ls_stateful_record_destroy(ls_stateful_rec);
>>> + }
>>> +}
>>> +
>>> +static void
>>> +ls_stateful_table_build(struct ls_stateful_table *table,
>>> + const struct ovn_datapaths *ls_datapaths,
>>> + const struct ls_port_group_table *ls_pgs)
>>> +{
>>> + const struct ovn_datapath *od;
>>> + HMAP_FOR_EACH (od, key_node, &ls_datapaths->datapaths) {
>>> + ls_stateful_record_create(table, od, ls_pgs);
>>> + }
>>> +}
>>> +
>>> +struct ls_stateful_record *
>>> +ls_stateful_table_find_(const struct ls_stateful_table *table,
>>> + const struct nbrec_logical_switch *nbs)
>>> +{
>>> + struct ls_stateful_record *rec;
>>> +
>>> + HMAP_FOR_EACH_WITH_HASH (rec, key_node,
>>> + uuid_hash(&nbs->header_.uuid),
>>> &table->entries) {
>>> + if (nbs == rec->od->nbs) {
>>> + return rec;
>>> + }
>>> + }
>>> + return NULL;
>>> +}
>>> +
>>> +static struct ls_stateful_record *
>>> +ls_stateful_record_create(struct ls_stateful_table *table,
>>> + const struct ovn_datapath *od,
>>> + const struct ls_port_group_table *ls_pgs)
>>> +{
>>> + struct ls_stateful_record *ls_stateful_rec =
>>> + xzalloc(sizeof *ls_stateful_rec);
>>> + ls_stateful_rec->od = od;
>>> + ls_stateful_record_init(ls_stateful_rec, od, NULL, ls_pgs);
>>> +
>>> + hmap_insert(&table->entries, &ls_stateful_rec->key_node,
>>> + uuid_hash(&ls_stateful_rec->od->nbs->header_.uuid));
>>> +
>>> + return ls_stateful_rec;
>>> +}
>>> +
>>> +static void
>>> +ls_stateful_record_destroy(struct ls_stateful_record *ls_stateful_rec)
>>> +{
>>> + free(ls_stateful_rec);
>>> +}
>>> +
>>> +static void
>>> +ls_stateful_record_init(struct ls_stateful_record *ls_stateful_rec,
>>> + const struct ovn_datapath *od,
>>> + const struct ls_port_group *ls_pg,
>>> + const struct ls_port_group_table *ls_pgs)
>>> +{
>>> + ls_stateful_rec->has_lb_vip = ls_has_lb_vip(od);
>>> + ls_stateful_record_set_acl_flags(ls_stateful_rec, od, ls_pg, ls_pgs);
>>> +}
>>> +
>>> +static void
>>> +ls_stateful_record_reinit(struct ls_stateful_record *ls_stateful_rec,
>>> + const struct ls_port_group *ls_pg,
>>> + const struct ls_port_group_table *ls_pgs)
>>> +{
>>> + ls_stateful_record_init(ls_stateful_rec, ls_stateful_rec->od,
>>> + ls_pg, ls_pgs);
>>> +}
>>> +
>>> +static bool
>>> +lb_has_vip(const struct nbrec_load_balancer *lb)
>>> +{
>>> + return !smap_is_empty(&lb->vips);
>>> +}
>>> +
>>> +static bool
>>> +lb_group_has_vip(const struct nbrec_load_balancer_group *lb_group)
>>> +{
>>> + for (size_t i = 0; i < lb_group->n_load_balancer; i++) {
>>> + if (lb_has_vip(lb_group->load_balancer[i])) {
>>> + return true;
>>> + }
>>> + }
>>> + return false;
>>> +}
>>> +
>>> +static bool
>>> +ls_has_lb_vip(const struct ovn_datapath *od)
>>> +{
>>> + for (size_t i = 0; i < od->nbs->n_load_balancer; i++) {
>>> + if (lb_has_vip(od->nbs->load_balancer[i])) {
>>> + return true;
>>> + }
>>> + }
>>> +
>>> + for (size_t i = 0; i < od->nbs->n_load_balancer_group; i++) {
>>> + if (lb_group_has_vip(od->nbs->load_balancer_group[i])) {
>>> + return true;
>>> + }
>>> + }
>>> + return false;
>>> +}
>>> +
>>> +static void
>>> +ls_stateful_record_set_acl_flags(struct ls_stateful_record
>>> *ls_stateful_rec,
>>> + const struct ovn_datapath *od,
>>> + const struct ls_port_group *ls_pg,
>>> + const struct ls_port_group_table *ls_pgs)
>>> +{
>>> + ls_stateful_rec->has_stateful_acl = false;
>>> + ls_stateful_rec->max_acl_tier = 0;
>>> + ls_stateful_rec->has_acls = false;
>>> +
>>> + if (ls_stateful_record_set_acl_flags_(ls_stateful_rec, od->nbs->acls,
>>> + od->nbs->n_acls)) {
>>> + return;
>>> + }
>>> +
>>> + if (!ls_pg) {
>>> + ls_pg = ls_port_group_table_find(ls_pgs, od->nbs);
>>> + }
>>> +
>>> + if (!ls_pg) {
>>> + return;
>>> + }
>>> +
>>> + const struct ls_port_group_record *ls_pg_rec;
>>> + HMAP_FOR_EACH (ls_pg_rec, key_node, &ls_pg->nb_pgs) {
>>> + if (ls_stateful_record_set_acl_flags_(ls_stateful_rec,
>>> + ls_pg_rec->nb_pg->acls,
>>> + ls_pg_rec->nb_pg->n_acls)) {
>>> + return;
>>> + }
>>> + }
>>> +}
>>> +
>>> +static bool
>>> +ls_stateful_record_set_acl_flags_(struct ls_stateful_record
>>> *ls_stateful_rec,
>>> + struct nbrec_acl **acls,
>>> + size_t n_acls)
>>> +{
>>> + /* A true return indicates that there are no possible ACL flags
>>> + * left to set on ls_stateful record. A false return indicates that
>>> + * further ACLs should be explored in case more flags need to be
>>> + * set on ls_stateful record.
>>> + */
>>> + if (!n_acls) {
>>> + return false;
>>> + }
>>> +
>>> + ls_stateful_rec->has_acls = true;
>>> + for (size_t i = 0; i < n_acls; i++) {
>>> + const struct nbrec_acl *acl = acls[i];
>>> + if (acl->tier > ls_stateful_rec->max_acl_tier) {
>>> + ls_stateful_rec->max_acl_tier = acl->tier;
>>> + }
>>> + if (!ls_stateful_rec->has_stateful_acl
>>> + && !strcmp(acl->action, "allow-related")) {
>>> + ls_stateful_rec->has_stateful_acl = true;
>>> + }
>>> + if (ls_stateful_rec->has_stateful_acl &&
>>> + ls_stateful_rec->max_acl_tier ==
>>> + nbrec_acl_col_tier.type.value.integer.max) {
>>> + return true;
>>> + }
>>> + }
>>> +
>>> + return false;
>>> +}
>>> +
>>> +static struct ls_stateful_input
>>> +ls_stateful_get_input_data(struct engine_node *node)
>>> +{
>>> + const struct northd_data *northd_data =
>>> + engine_get_input_data("northd", node);
>>> + const struct port_group_data *pg_data =
>>> + engine_get_input_data("port_group", node);
>>> +
>>> + return (struct ls_stateful_input) {
>>> + .ls_port_groups = &pg_data->ls_port_groups,
>>> + .ls_datapaths = &northd_data->ls_datapaths,
>>> + };
>>> +}
>>> diff --git a/northd/en-ls-stateful.h b/northd/en-ls-stateful.h
>>> new file mode 100644
>>> index 0000000000..cba53e1f29
>>> --- /dev/null
>>> +++ b/northd/en-ls-stateful.h
>>> @@ -0,0 +1,81 @@
>>> +/*
>>> + * Copyright (c) 2023, Red Hat, Inc.
>>
>> 2024
>
> Ack
>
>>
>>> + *
>>> + * Licensed under the Apache License, Version 2.0 (the "License");
>>> + * you may not use this file except in compliance with the License.
>>> + * You may obtain a copy of the License at:
>>> + *
>>> + * http://www.apache.org/licenses/LICENSE-2.0
>>> + *
>>> + * Unless required by applicable law or agreed to in writing, software
>>> + * distributed under the License is distributed on an "AS IS" BASIS,
>>> + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
>>> + * See the License for the specific language governing permissions and
>>> + * limitations under the License.
>>> + */
>>> +#ifndef EN_LS_STATEFUL_H
>>> +#define EN_LS_STATEFUL_H 1
>>> +
>>> +#include <stdint.h>
>>> +
>>> +/* OVS includes. */
>>> +#include "lib/hmapx.h"
>>> +#include "openvswitch/hmap.h"
>>> +#include "sset.h"
>>> +
>>> +/* OVN includes. */
>>> +#include "lib/inc-proc-eng.h"
>>> +#include "lib/lb.h"
>>> +#include "lib/ovn-nb-idl.h"
>>> +#include "lib/ovn-sb-idl.h"
>>> +#include "lib/ovn-util.h"
>>> +#include "lib/stopwatch-names.h"
>>> +
>>> +struct ls_stateful_record {
>>> + struct hmap_node key_node;
>>> +
>>> + const struct ovn_datapath *od;
>>> + bool has_stateful_acl;
>>> + bool has_lb_vip;
>>> + bool has_acls;
>>> + uint64_t max_acl_tier;
>>> +};
>>> +
>>> +struct ls_stateful_table {
>>> + struct hmap entries;
>>> +};
>>> +
>>> +#define LS_STATEFUL_TABLE_FOR_EACH(LS_STATEFUL_REC, TABLE) \
>>> + HMAP_FOR_EACH (LS_STATEFUL_REC, key_node, &(TABLE)->entries)
>>> +
>>> +#define LS_STATEFUL_TABLE_FOR_EACH_IN_P(LS_STATEFUL_REC, JOBID, TABLE) \
>>> + HMAP_FOR_EACH_IN_PARALLEL (LS_STATEFUL_REC, key_node, JOBID, \
>>> + &(TABLE)->entries)
>>> +
>>> +struct ls_stateful_tracked_data {
>>> + /* Created or updated logical switch with LB and ACL data. */
>>> + struct hmapx crupdated; /* Stores 'struct ls_stateful_record'. */
>>> +};
>>> +
>>> +struct ed_type_ls_stateful {
>>> + struct ls_stateful_table table;
>>> + struct ls_stateful_tracked_data trk_data;
>>> +};
>>> +
>>> +void *en_ls_stateful_init(struct engine_node *, struct engine_arg *);
>>> +void en_ls_stateful_cleanup(void *data);
>>> +void en_ls_stateful_clear_tracked_data(void *data);
>>> +void en_ls_stateful_run(struct engine_node *, void *data);
>>> +
>>> +bool ls_stateful_northd_handler(struct engine_node *, void *data);
>>> +bool ls_stateful_port_group_handler(struct engine_node *, void *data);
>>> +
>>> +const struct ls_stateful_record *ls_stateful_table_find(
>>> + const struct ls_stateful_table *, const struct nbrec_logical_switch *);
>>> +
>>> +static inline bool
>>> +ls_stateful_has_tracked_data(struct ls_stateful_tracked_data *trk_data) {
>>> + return !hmapx_is_empty(&trk_data->crupdated);
>>> +}
>>> +
>>> +#endif /* EN_LS_STATEFUL_H */
>>> diff --git a/northd/en-northd.c b/northd/en-northd.c
>>> index 546397f3dc..5143603f39 100644
>>> --- a/northd/en-northd.c
>>> +++ b/northd/en-northd.c
>>> @@ -171,7 +171,7 @@ northd_nb_logical_switch_handler(struct engine_node
>>> *node,
>>> return false;
>>> }
>>>
>>> - if (northd_has_lsps_in_tracked_data(&nd->trk_data)) {
>>> + if (northd_has_tracked_data(&nd->trk_data)) {
>>> engine_set_node_state(node, EN_UPDATED);
>>> }
>>>
>>> diff --git a/northd/en-port-group.h b/northd/en-port-group.h
>>> index 3b28a23694..54014062ce 100644
>>> --- a/northd/en-port-group.h
>>> +++ b/northd/en-port-group.h
>>> @@ -48,6 +48,9 @@ struct ls_port_group_record {
>>> struct sset ports; /* Subset of 'nb_pg' ports in this record.
>>> */
>>> };
>>>
>>> +#define LS_PORT_GROUP_TABLE_FOR_EACH(LS_PG, TABLE) \
>>> + HMAP_FOR_EACH (LS_PG, key_node, &(TABLE)->entries)
>>> +
>>> void ls_port_group_table_init(struct ls_port_group_table *);
>>> void ls_port_group_table_clear(struct ls_port_group_table *);
>>> void ls_port_group_table_destroy(struct ls_port_group_table *);
>>> diff --git a/northd/inc-proc-northd.c b/northd/inc-proc-northd.c
>>> index adb38dde78..9ce4279ee8 100644
>>> --- a/northd/inc-proc-northd.c
>>> +++ b/northd/inc-proc-northd.c
>>> @@ -33,6 +33,7 @@
>>> #include "en-lb-data.h"
>>> #include "en-lr-stateful.h"
>>> #include "en-lr-nat.h"
>>> +#include "en-ls-stateful.h"
>>> #include "en-northd.h"
>>> #include "en-lflow.h"
>>> #include "en-northd-output.h"
>>> @@ -150,6 +151,7 @@ static ENGINE_NODE(sync_to_sb_pb, "sync_to_sb_pb");
>>> static ENGINE_NODE_WITH_CLEAR_TRACK_DATA(lb_data, "lb_data");
>>> static ENGINE_NODE_WITH_CLEAR_TRACK_DATA(lr_nat, "lr_nat");
>>> static ENGINE_NODE_WITH_CLEAR_TRACK_DATA(lr_stateful, "lr_stateful");
>>> +static ENGINE_NODE_WITH_CLEAR_TRACK_DATA(ls_stateful, "ls_stateful");
>>>
>>> void inc_proc_northd_init(struct ovsdb_idl_loop *nb,
>>> struct ovsdb_idl_loop *sb)
>>> @@ -200,6 +202,10 @@ void inc_proc_northd_init(struct ovsdb_idl_loop *nb,
>>> engine_add_input(&en_lr_stateful, &en_lb_data,
>>> lr_stateful_lb_data_handler);
>>>
>>> + engine_add_input(&en_ls_stateful, &en_northd,
>>> ls_stateful_northd_handler);
>>> + engine_add_input(&en_ls_stateful, &en_port_group,
>>> + ls_stateful_port_group_handler);
>>> +
>>> engine_add_input(&en_mac_binding_aging, &en_nb_nb_global, NULL);
>>> engine_add_input(&en_mac_binding_aging, &en_sb_mac_binding, NULL);
>>> engine_add_input(&en_mac_binding_aging, &en_northd, NULL);
>>> @@ -222,6 +228,7 @@ void inc_proc_northd_init(struct ovsdb_idl_loop *nb,
>>> engine_add_input(&en_lflow, &en_sb_multicast_group, NULL);
>>> engine_add_input(&en_lflow, &en_sb_igmp_group, NULL);
>>> engine_add_input(&en_lflow, &en_lr_stateful, NULL);
>>> + engine_add_input(&en_lflow, &en_ls_stateful, NULL);
>>> engine_add_input(&en_lflow, &en_northd, lflow_northd_handler);
>>> engine_add_input(&en_lflow, &en_port_group, lflow_port_group_handler);
>>>
>>> diff --git a/northd/northd.c b/northd/northd.c
>>> index f56a7c5ea4..68f2b0bab4 100644
>>> --- a/northd/northd.c
>>> +++ b/northd/northd.c
>>> @@ -46,6 +46,7 @@
>>> #include "en-lb-data.h"
>>> #include "en-lr-nat.h"
>>> #include "en-lr-stateful.h"
>>> +#include "en-ls-stateful.h"
>>> #include "lib/ovn-parallel-hmap.h"
>>> #include "ovn/actions.h"
>>> #include "ovn/features.h"
>>> @@ -576,7 +577,7 @@ lb_group_has_vip(const struct nbrec_load_balancer_group
>>> *lb_group)
>>> }
>>>
>>> static bool
>>> -ls_has_lb_vip(struct ovn_datapath *od)
>>> +ls_has_lb_vip(const struct ovn_datapath *od)
>>> {
>>> for (size_t i = 0; i < od->nbs->n_load_balancer; i++) {
>>> if (lb_has_vip(od->nbs->load_balancer[i])) {
>>> @@ -593,7 +594,7 @@ ls_has_lb_vip(struct ovn_datapath *od)
>>> }
>>>
>>> static bool
>>> -lr_has_lb_vip(struct ovn_datapath *od)
>>> +lr_has_lb_vip(const struct ovn_datapath *od)
>>> {
>>> for (size_t i = 0; i < od->nbr->n_load_balancer; i++) {
>>> if (lb_has_vip(od->nbr->load_balancer[i])) {
>>> @@ -609,13 +610,13 @@ lr_has_lb_vip(struct ovn_datapath *od)
>>> return false;
>>> }
>>>
>>> -static void
>>> -init_lb_for_datapath(struct ovn_datapath *od)
>>> +bool
>>> +od_has_lb_vip(const struct ovn_datapath *od)
>>> {
>>> if (od->nbs) {
>>> - od->has_lb_vip = ls_has_lb_vip(od);
>>> + return ls_has_lb_vip(od);
>>> } else {
>>> - od->has_lb_vip = lr_has_lb_vip(od);
>>> + return lr_has_lb_vip(od);
>>> }
>>> }
>>>
>>> @@ -1065,7 +1066,6 @@ join_datapaths(const struct
>>> nbrec_logical_switch_table *nbrec_ls_table,
>>>
>>> init_ipam_info_for_datapath(od);
>>> init_mcast_info_for_datapath(od);
>>> - init_lb_for_datapath(od);
>>> }
>>>
>>> const struct nbrec_logical_router *nbr;
>>> @@ -1096,7 +1096,6 @@ join_datapaths(const struct
>>> nbrec_logical_switch_table *nbrec_ls_table,
>>> ovs_list_push_back(nb_only, &od->list);
>>> }
>>> init_mcast_info_for_datapath(od);
>>> - init_lb_for_datapath(od);
>>> if (smap_get(&od->nbr->options, "chassis")) {
>>> od->is_gw_router = true;
>>> }
>>> @@ -2583,7 +2582,8 @@ get_nat_addresses(const struct ovn_port *op, size_t
>>> *n, bool routable_only,
>>> size_t n_nats = 0;
>>> struct eth_addr mac;
>>> if (!op || !op->nbrp || !op->od || !op->od->nbr
>>> - || (!op->od->nbr->n_nat && !op->od->has_lb_vip)
>>> + || (!op->od->nbr->n_nat && (!lr_stateful_rec
>>> + || !lr_stateful_rec->has_lb_vip))
>>
>> IMO this is very hard to read. What about a simple inline function in
>> en-lr-stateful.h to do this instead? E.g.:
>>
>> static inline bool
>> lr_stateful_rec_has_lb_vip(const struct lr_stateful_record* lr_rec)
>> {
>> return lr_rec && lr_rec->has_lb_vip;
>> }
>
> Ack
>
>
>>
>>> || !eth_addr_from_string(op->nbrp->mac, &mac)) {
>>> *n = n_nats;
>>> return NULL;
>>> @@ -3830,7 +3830,7 @@ build_lrouter_lbs_check(const struct ovn_datapaths
>>> *lr_datapaths)
>>> HMAP_FOR_EACH (od, key_node, &lr_datapaths->datapaths) {
>>> ovs_assert(od->nbr);
>>>
>>> - if (od->has_lb_vip && od->n_l3dgw_ports > 1
>>> + if (od_has_lb_vip(od) && od->n_l3dgw_ports > 1
>>
>> This walks all LBs applied to the router again. It's probably fine to
>> just query the lr_stateful_rec associated to this router (it's O(1) to
>> do that).
>
> build_lrouter_lbs_check() is part of the northd engine and hence we
> can't use the lr_stateful_rec here
> as en_lr_stateful is not the input node to en_northd engine node. Its
> actually the other way.
>
Ah, right, I see now.
> This function only prints warning log. Shall we move this instead to
> lr_stateful.c ?
Yes, that'd be great.
>
> I'll address all other comments and add your Acked-by. Let me know if
> that's not fine as I can't address the above comment.
>
It's fine if we move the check to en-lr-stateful.c.
Thanks,
Dumitru
_______________________________________________
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
