On Sat, Jul 8, 2023 at 3:57 AM Mark Michelson <mmich...@redhat.com> wrote:
>
> Hi Numan,
>
> I have one small nit below.
>
> On 7/7/23 01:53, num...@ovn.org wrote:
> > From: Numan Siddique <num...@ovn.org>
> >
> > Any changes to load balancers and load balancer groups
> > are handled incrementally in the newly added 'northd_lb_data'
> > engine node. 'northd_lb_data' is input to 'northd' node
> > and the handler - northd_northd_lb_data_handler in 'northd'
> > node handles the changes.
> >
> > If a load balancer or load balancer group is associated to
> > a logical switch or router then 'northd' node falls back
> > to full recompute.
> >
> > Signed-off-by: Numan Siddique <num...@ovn.org>
> > ---
> > lib/lb.c | 85 ++++++++++++----
> > lib/lb.h | 9 ++
> > northd/en-northd-lb-data.c | 202 +++++++++++++++++++++++++++++++++++--
> > northd/en-northd-lb-data.h | 37 +++++++
> > northd/en-northd.c | 24 +++++
> > northd/en-northd.h | 1 +
> > northd/inc-proc-northd.c | 13 ++-
> > northd/northd.c | 77 ++++++++++++++
> > northd/northd.h | 7 ++
> > tests/ovn-northd.at | 123 ++++++++++++++++++++++
> > 10 files changed, 545 insertions(+), 33 deletions(-)
> >
> > diff --git a/lib/lb.c b/lib/lb.c
> > index 429dbf15af..a51fe225fa 100644
> > --- a/lib/lb.c
> > +++ b/lib/lb.c
> > @@ -606,13 +606,13 @@ ovn_lb_get_health_check(const struct
nbrec_load_balancer *nbrec_lb,
> > return NULL;
> > }
> >
> > -struct ovn_northd_lb *
> > -ovn_northd_lb_create(const struct nbrec_load_balancer *nbrec_lb)
> > +static void
> > +ovn_northd_lb_init(struct ovn_northd_lb *lb,
> > + const struct nbrec_load_balancer *nbrec_lb)
> > {
> > bool template = smap_get_bool(&nbrec_lb->options, "template",
false);
> > bool is_udp = nullable_string_is_equal(nbrec_lb->protocol, "udp");
> > bool is_sctp = nullable_string_is_equal(nbrec_lb->protocol,
"sctp");
> > - struct ovn_northd_lb *lb = xzalloc(sizeof *lb);
> > int address_family = !strcmp(smap_get_def(&nbrec_lb->options,
> > "address-family",
"ipv4"),
> > "ipv4")
> > @@ -668,6 +668,10 @@ ovn_northd_lb_create(const struct
nbrec_load_balancer *nbrec_lb)
> > "reject", false);
> > ovn_northd_lb_vip_init(lb_vip_nb, lb_vip, nbrec_lb,
> > node->key, node->value, template);
> > + if (lb_vip_nb->lb_health_check) {
> > + lb->health_checks = true;
> > + }
> > +
> > if (IN6_IS_ADDR_V4MAPPED(&lb_vip->vip)) {
> > sset_add(&lb->ips_v4, lb_vip->vip_str);
> > } else {
> > @@ -713,6 +717,13 @@ ovn_northd_lb_create(const struct
nbrec_load_balancer *nbrec_lb)
> > ds_chomp(&sel_fields, ',');
> > lb->selection_fields = ds_steal_cstr(&sel_fields);
> > }
> > +}
> > +
> > +struct ovn_northd_lb *
> > +ovn_northd_lb_create(const struct nbrec_load_balancer *nbrec_lb)
> > +{
> > + struct ovn_northd_lb *lb = xzalloc(sizeof *lb);
> > + ovn_northd_lb_init(lb, nbrec_lb);
> > return lb;
> > }
> >
> > @@ -738,8 +749,8 @@ ovn_northd_lb_get_vips(const struct ovn_northd_lb
*lb)
> > return &lb->nlb->vips;
> > }
> >
> > -void
> > -ovn_northd_lb_destroy(struct ovn_northd_lb *lb)
> > +static void
> > +ovn_northd_lb_cleanup(struct ovn_northd_lb *lb)
> > {
> > for (size_t i = 0; i < lb->n_vips; i++) {
> > ovn_lb_vip_destroy(&lb->vips[i]);
> > @@ -747,26 +758,36 @@ ovn_northd_lb_destroy(struct ovn_northd_lb *lb)
> > }
> > free(lb->vips);
> > free(lb->vips_nb);
> > + lb->vips = NULL;
> > + lb->vips_nb = NULL;
> > smap_destroy(&lb->template_vips);
> > sset_destroy(&lb->ips_v4);
> > sset_destroy(&lb->ips_v6);
> > free(lb->selection_fields);
> > + lb->selection_fields = NULL;
> > + lb->health_checks = false;
> > +}
> > +
> > +void
> > +ovn_northd_lb_destroy(struct ovn_northd_lb *lb)
> > +{
> > + ovn_northd_lb_cleanup(lb);
> > free(lb);
> > }
> >
> > -/* Constructs a new 'struct ovn_lb_group' object from the Nb LB Group
record
> > - * and a hash map of all existing 'struct ovn_northd_lb' objects.
Space will
> > - * be allocated for 'max_ls_datapaths' logical switches and
'max_lr_datapaths'
> > - * logical routers to which this LB Group is applied. Can be filled
later
> > - * with ovn_lb_group_add_ls() and ovn_lb_group_add_lr() respectively.
*/
> > -struct ovn_lb_group *
> > -ovn_lb_group_create(const struct nbrec_load_balancer_group
*nbrec_lb_group,
> > - const struct hmap *lbs)
> > +void
> > +ovn_northd_lb_reinit(struct ovn_northd_lb *lb,
> > + const struct nbrec_load_balancer *nbrec_lb)
> > {
> > - struct ovn_lb_group *lb_group;
> > + ovn_northd_lb_cleanup(lb);
> > + ovn_northd_lb_init(lb, nbrec_lb);
> > +}
> >
> > - lb_group = xzalloc(sizeof *lb_group);
> > - lb_group->uuid = nbrec_lb_group->header_.uuid;
> > +static void
> > +ovn_lb_group_init(struct ovn_lb_group *lb_group,
> > + const struct nbrec_load_balancer_group
*nbrec_lb_group,
> > + const struct hmap *lbs)
> > +{
> > lb_group->n_lbs = nbrec_lb_group->n_load_balancer;
> > lb_group->lbs = xmalloc(lb_group->n_lbs * sizeof *lb_group->lbs);
> > lb_group->lb_ips = ovn_lb_ip_set_create();
> > @@ -776,10 +797,30 @@ ovn_lb_group_create(const struct
nbrec_load_balancer_group *nbrec_lb_group,
> > &nbrec_lb_group->load_balancer[i]->header_.uuid;
> > lb_group->lbs[i] = ovn_northd_lb_find(lbs, lb_uuid);
> > }
> > +}
> >
> > +/* Constructs a new 'struct ovn_lb_group' object from the Nb LB Group
record
> > + * and a hash map of all existing 'struct ovn_northd_lb' objects.
Space will
> > + * be allocated for 'max_ls_datapaths' logical switches and
'max_lr_datapaths'
> > + * logical routers to which this LB Group is applied. Can be filled
later
> > + * with ovn_lb_group_add_ls() and ovn_lb_group_add_lr() respectively.
*/
> > +struct ovn_lb_group *
> > +ovn_lb_group_create(const struct nbrec_load_balancer_group
*nbrec_lb_group,
> > + const struct hmap *lbs)
> > +{
> > + struct ovn_lb_group *lb_group = xzalloc(sizeof *lb_group);
> > + lb_group->uuid = nbrec_lb_group->header_.uuid;
> > + ovn_lb_group_init(lb_group, nbrec_lb_group, lbs);
> > return lb_group;
> > }
> >
> > +static void
> > +ovn_lb_group_cleanup(struct ovn_lb_group *lb_group)
> > +{
> > + ovn_lb_ip_set_destroy(lb_group->lb_ips);
> > + free(lb_group->lbs);
> > +}
> > +
> > void
> > ovn_lb_group_destroy(struct ovn_lb_group *lb_group)
> > {
> > @@ -787,11 +828,19 @@ ovn_lb_group_destroy(struct ovn_lb_group
*lb_group)
> > return;
> > }
> >
> > - ovn_lb_ip_set_destroy(lb_group->lb_ips);
> > - free(lb_group->lbs);
> > + ovn_lb_group_cleanup(lb_group);
> > free(lb_group);
> > }
> >
> > +void
> > +ovn_lb_group_reinit(struct ovn_lb_group *lb_group,
> > + const struct nbrec_load_balancer_group
*nbrec_lb_group,
> > + const struct hmap *lbs)
> > +{
> > + ovn_lb_group_cleanup(lb_group);
> > + ovn_lb_group_init(lb_group, nbrec_lb_group, lbs);
> > +}
> > +
> > struct ovn_lb_group *
> > ovn_lb_group_find(const struct hmap *lb_groups, const struct uuid
*uuid)
> > {
> > diff --git a/lib/lb.h b/lib/lb.h
> > index 0339050cba..74905c73b7 100644
> > --- a/lib/lb.h
> > +++ b/lib/lb.h
> > @@ -77,6 +77,9 @@ struct ovn_northd_lb {
> >
> > struct sset ips_v4;
> > struct sset ips_v6;
> > +
> > + /* Indicates if the load balancer has health checks configured. */
> > + bool health_checks;
> > };
> >
> > struct ovn_lb_vip {
> > @@ -130,6 +133,8 @@ struct ovn_northd_lb *ovn_northd_lb_find(const
struct hmap *,
> > const struct uuid *);
> > const struct smap *ovn_northd_lb_get_vips(const struct ovn_northd_lb
*);
> > void ovn_northd_lb_destroy(struct ovn_northd_lb *);
> > +void ovn_northd_lb_reinit(struct ovn_northd_lb *,
> > + const struct nbrec_load_balancer *);
> >
> > void build_lrouter_lb_ips(struct ovn_lb_ip_set *,
> > const struct ovn_northd_lb *);
> > @@ -148,6 +153,10 @@ struct ovn_lb_group *ovn_lb_group_create(
> > void ovn_lb_group_destroy(struct ovn_lb_group *lb_group);
> > struct ovn_lb_group *ovn_lb_group_find(const struct hmap *lb_groups,
> > const struct uuid *);
> > +void ovn_lb_group_reinit(
> > + struct ovn_lb_group *lb_group,
> > + const struct nbrec_load_balancer_group *,
> > + const struct hmap *lbs);
> >
> > struct ovn_lb_datapaths {
> > struct hmap_node hmap_node;
> > diff --git a/northd/en-northd-lb-data.c b/northd/en-northd-lb-data.c
> > index d46c3c27ed..e52535b4a9 100644
> > --- a/northd/en-northd-lb-data.c
> > +++ b/northd/en-northd-lb-data.c
> > @@ -37,6 +37,15 @@ static void northd_lb_data_destroy(struct
northd_lb_data *);
> > static void build_lbs(const struct nbrec_load_balancer_table *,
> > const struct nbrec_load_balancer_group_table *,
> > struct hmap *lbs, struct hmap *lb_groups);
> > +static struct ovn_lb_group *create_lb_group(
> > + const struct nbrec_load_balancer_group *, struct hmap *lbs,
> > + struct hmap *lb_groups);
> > +static void destroy_tracked_data(struct northd_lb_data *);
> > +static void add_lb_to_tracked_data(struct ovn_northd_lb *,
> > + struct ovs_list *tracked_list,
> > + bool health_checks);
> > +static void add_lb_group_to_tracked_data(struct ovn_lb_group *,
> > + struct ovs_list
*tracked_list);
> >
> > void *
> > en_northd_lb_data_init(struct engine_node *node OVS_UNUSED,
> > @@ -61,6 +70,7 @@ en_northd_lb_data_run(struct engine_node *node, void
*data)
> > const struct nbrec_load_balancer_group_table *nb_lbg_table =
> > EN_OVSDB_GET(engine_get_input("NB_load_balancer_group",
node));
> >
> > + lb_data->tracked = false;
> > build_lbs(nb_lb_table, nb_lbg_table, &lb_data->lbs,
&lb_data->lb_groups);
> > engine_set_node_state(node, EN_UPDATED);
> > }
> > @@ -72,12 +82,122 @@ en_northd_lb_data_cleanup(void *data)
> > northd_lb_data_destroy(lb_data);
> > }
> >
> > +void
> > +en_northd_lb_data_clear_tracked_data(void *data)
> > +{
> > + struct northd_lb_data *lb_data = (struct northd_lb_data *) data;
> > + destroy_tracked_data(lb_data);
> > +}
> > +
> > +
> > +/* Handler functions. */
> > +bool
> > +northd_lb_data_load_balancer_handler(struct engine_node *node,
> > + void *data OVS_UNUSED)
> > +{
> > + const struct nbrec_load_balancer_table *nb_lb_table =
> > + EN_OVSDB_GET(engine_get_input("NB_load_balancer", node));
> > +
> > + struct northd_lb_data *lb_data = (struct northd_lb_data *) data;
> > +
> > + lb_data->tracked = true;
> > + struct tracked_lb_data *trk_lb_data = &lb_data->tracked_lb_data;
> > +
> > + const struct nbrec_load_balancer *tracked_lb;
> > + NBREC_LOAD_BALANCER_TABLE_FOR_EACH_TRACKED (tracked_lb,
nb_lb_table) {
> > + struct ovn_northd_lb *lb;
> > + if (nbrec_load_balancer_is_new(tracked_lb)) {
> > + /* New load balancer. */
> > + lb = ovn_northd_lb_create(tracked_lb);
> > + hmap_insert(&lb_data->lbs, &lb->hmap_node,
> > + uuid_hash(&tracked_lb->header_.uuid));
> > + add_lb_to_tracked_data(
> > + lb, &trk_lb_data->tracked_updated_lbs.updated,
> > + lb->health_checks);
> > + } else if (nbrec_load_balancer_is_deleted(tracked_lb)) {
> > + lb = ovn_northd_lb_find(&lb_data->lbs,
> > + &tracked_lb->header_.uuid);
> > + ovs_assert(lb);
> > + hmap_remove(&lb_data->lbs, &lb->hmap_node);
> > + add_lb_to_tracked_data(
> > + lb, &trk_lb_data->tracked_deleted_lbs.updated,
> > + lb->health_checks);
> > + } else {
> > + /* Load balancer updated. */
> > + lb = ovn_northd_lb_find(&lb_data->lbs,
> > + &tracked_lb->header_.uuid);
> > + ovs_assert(lb);
> > + bool health_checks = lb->health_checks;
> > + ovn_northd_lb_reinit(lb, tracked_lb);
> > + health_checks |= lb->health_checks;
> > + add_lb_to_tracked_data(
> > + lb, &trk_lb_data->tracked_updated_lbs.updated,
health_checks);
> > + }
> > + }
> > +
> > + engine_set_node_state(node, EN_UPDATED);
> > + return true;
> > +}
> > +
> > +bool
> > +northd_lb_data_load_balancer_group_handler(struct engine_node *node,
> > + void *data OVS_UNUSED)
> > +{
> > + struct northd_lb_data *lb_data = (struct northd_lb_data *) data;
> > + const struct nbrec_load_balancer_group_table *nb_lbg_table =
> > + EN_OVSDB_GET(engine_get_input("NB_load_balancer_group", node));
> > +
> > + lb_data->tracked = true;
> > + struct tracked_lb_data *trk_lb_data = &lb_data->tracked_lb_data;
> > + const struct nbrec_load_balancer_group *tracked_lb_group;
> > + NBREC_LOAD_BALANCER_GROUP_TABLE_FOR_EACH_TRACKED (tracked_lb_group,
> > + nb_lbg_table) {
> > + if (nbrec_load_balancer_group_is_new(tracked_lb_group)) {
> > + struct ovn_lb_group *lb_group =
> > + create_lb_group(tracked_lb_group, &lb_data->lbs,
> > + &lb_data->lb_groups);
> > + add_lb_group_to_tracked_data(
> > + lb_group,
&trk_lb_data->tracked_updated_lb_groups.updated);
> > + } else if
(nbrec_load_balancer_group_is_deleted(tracked_lb_group)) {
> > + struct ovn_lb_group *lb_group;
> > + lb_group = ovn_lb_group_find(&lb_data->lb_groups,
> > +
&tracked_lb_group->header_.uuid);
> > + ovs_assert(lb_group);
> > + hmap_remove(&lb_data->lb_groups, &lb_group->hmap_node);
> > + add_lb_group_to_tracked_data(
> > + lb_group,
&trk_lb_data->tracked_deleted_lb_groups.updated);
> > + } else if
(nbrec_load_balancer_group_is_updated(tracked_lb_group,
> > +
NBREC_LOAD_BALANCER_GROUP_COL_LOAD_BALANCER)) {
> > +
> > + struct ovn_lb_group *lb_group;
> > + lb_group = ovn_lb_group_find(&lb_data->lb_groups,
> > +
&tracked_lb_group->header_.uuid);
> > + ovs_assert(lb_group);
> > + ovn_lb_group_reinit(lb_group, tracked_lb_group,
&lb_data->lbs);
> > + for (size_t i = 0; i < lb_group->n_lbs; i++) {
> > + build_lrouter_lb_ips(lb_group->lb_ips,
lb_group->lbs[i]);
> > + }
> > + add_lb_group_to_tracked_data(
> > + lb_group,
&trk_lb_data->tracked_updated_lb_groups.updated);
> > + }
> > + }
> > +
> > + engine_set_node_state(node, EN_UPDATED);
> > + return true;
> > +}
> > +
> > /* static functions. */
> > static void
> > northd_lb_data_init(struct northd_lb_data *lb_data)
> > {
> > hmap_init(&lb_data->lbs);
> > hmap_init(&lb_data->lb_groups);
> > +
> > + struct tracked_lb_data *trk_lb_data = &lb_data->tracked_lb_data;
> > + ovs_list_init(&trk_lb_data->tracked_updated_lbs.updated);
> > + ovs_list_init(&trk_lb_data->tracked_deleted_lbs.updated);
> > + ovs_list_init(&trk_lb_data->tracked_updated_lb_groups.updated);
> > + ovs_list_init(&trk_lb_data->tracked_deleted_lb_groups.updated);
> > }
> >
> > static void
> > @@ -94,6 +214,8 @@ northd_lb_data_destroy(struct northd_lb_data
*lb_data)
> > ovn_lb_group_destroy(lb_group);
> > }
> > hmap_destroy(&lb_data->lb_groups);
> > +
> > + destroy_tracked_data(lb_data);
> > }
> >
> > static void
> > @@ -101,12 +223,9 @@ build_lbs(const struct nbrec_load_balancer_table
*nbrec_load_balancer_table,
> > const struct nbrec_load_balancer_group_table
*nbrec_lb_group_table,
> > struct hmap *lbs, struct hmap *lb_groups)
> > {
> > - struct ovn_lb_group *lb_group;
> > - struct ovn_northd_lb *lb_nb;
> > -
> > const struct nbrec_load_balancer *nbrec_lb;
> > NBREC_LOAD_BALANCER_TABLE_FOR_EACH (nbrec_lb,
nbrec_load_balancer_table) {
> > - lb_nb = ovn_northd_lb_create(nbrec_lb);
> > + struct ovn_northd_lb *lb_nb = ovn_northd_lb_create(nbrec_lb);
> > hmap_insert(lbs, &lb_nb->hmap_node,
> > uuid_hash(&nbrec_lb->header_.uuid));
> > }
> > @@ -114,13 +233,76 @@ build_lbs(const struct nbrec_load_balancer_table
*nbrec_load_balancer_table,
> > const struct nbrec_load_balancer_group *nbrec_lb_group;
> > NBREC_LOAD_BALANCER_GROUP_TABLE_FOR_EACH (nbrec_lb_group,
> > nbrec_lb_group_table) {
> > - lb_group = ovn_lb_group_create(nbrec_lb_group, lbs);
> > + create_lb_group(nbrec_lb_group, lbs, lb_groups);
> > + }
> > +}
> >
> > - for (size_t i = 0; i < lb_group->n_lbs; i++) {
> > - build_lrouter_lb_ips(lb_group->lb_ips, lb_group->lbs[i]);
> > - }
> > +static struct ovn_lb_group *
> > +create_lb_group(const struct nbrec_load_balancer_group *nbrec_lb_group,
> > + struct hmap *lbs, struct hmap *lb_groups)
> > +{
> > + struct ovn_lb_group *lb_group =
ovn_lb_group_create(nbrec_lb_group, lbs);
> > +
> > + for (size_t i = 0; i < lb_group->n_lbs; i++) {
> > + build_lrouter_lb_ips(lb_group->lb_ips, lb_group->lbs[i]);
> > + }
> > +
> > + hmap_insert(lb_groups, &lb_group->hmap_node,
> > + uuid_hash(&lb_group->uuid));
> > +
> > + return lb_group;
> > +}
> > +
> > +static void
> > +destroy_tracked_data(struct northd_lb_data *lb_data)
> > +{
> > + lb_data->tracked = false;
> > +
> > + struct tracked_lb_data *trk_lb_data = &lb_data->tracked_lb_data;
> > + struct tracked_lb *tracked_lb;
> > + LIST_FOR_EACH_SAFE (tracked_lb, list_node,
> > + &trk_lb_data->tracked_updated_lbs.updated) {
> > + ovs_list_remove(&tracked_lb->list_node);
> > + free(tracked_lb);
> > + }
> > +
> > + LIST_FOR_EACH_SAFE (tracked_lb, list_node,
> > + &trk_lb_data->tracked_deleted_lbs.updated) {
> > + ovs_list_remove(&tracked_lb->list_node);
> > + ovn_northd_lb_destroy(tracked_lb->lb);
> > + free(tracked_lb);
> > + }
> > +
> > + struct tracked_lb_group *tracked_lb_group;
> > + LIST_FOR_EACH_SAFE (tracked_lb_group, list_node,
> > +
&trk_lb_data->tracked_updated_lb_groups.updated) {
> > + ovs_list_remove(&tracked_lb_group->list_node);
> > + free(tracked_lb_group);
> > + }
> >
> > - hmap_insert(lb_groups, &lb_group->hmap_node,
> > - uuid_hash(&lb_group->uuid));
> > + LIST_FOR_EACH_SAFE (tracked_lb_group, list_node,
> > +
&trk_lb_data->tracked_deleted_lb_groups.updated) {
> > + ovs_list_remove(&tracked_lb_group->list_node);
> > + ovn_lb_group_destroy(tracked_lb_group->lb_group);
> > + free(tracked_lb_group);
> > }
> > }
> > +
> > +static void
> > +add_lb_to_tracked_data(struct ovn_northd_lb *lb, struct ovs_list
*tracked_list,
> > + bool health_checks)
> > +{
> > + struct tracked_lb *t_lb = xzalloc(sizeof *t_lb);
> > + t_lb->lb = lb;
> > + t_lb->health_checks = health_checks;
> > + ovs_list_push_back(tracked_list, &t_lb->list_node);
> > +}
> > +
> > +static void
> > +add_lb_group_to_tracked_data(struct ovn_lb_group *lb_group,
> > + struct ovs_list *tracked_list)
> > +{
> > + struct tracked_lb_group *t_lb_group = xzalloc(sizeof *t_lb_group);
> > + t_lb_group->lb_group = lb_group;
> > + ovs_list_push_back(tracked_list, &t_lb_group->list_node);
> > +}
> > diff --git a/northd/en-northd-lb-data.h b/northd/en-northd-lb-data.h
> > index eb297e376d..bb07fcf686 100644
> > --- a/northd/en-northd-lb-data.h
> > +++ b/northd/en-northd-lb-data.h
> > @@ -4,16 +4,53 @@
> > #include <config.h>
> >
> > #include "openvswitch/hmap.h"
> > +#include "include/openvswitch/list.h"
> >
> > #include "lib/inc-proc-eng.h"
> >
> > +struct ovn_northd_lb;
> > +struct ovn_lb_group;
> > +
> > +struct tracked_lb {
> > + struct ovs_list list_node;
> > + struct ovn_northd_lb *lb;
> > + bool health_checks;
> > +};
> > +
> > +struct tracked_lb_group {
> > + struct ovs_list list_node;
> > + struct ovn_lb_group *lb_group;
> > +};
> > +
> > +struct tracked_lb_changes {
> > + struct ovs_list updated; /* contains list of 'struct tracked_lb '
or
> > + list of 'struct tracked_lb_group' */
> > +};
> > +
> > +struct tracked_lb_data {
> > + struct tracked_lb_changes tracked_updated_lbs;
> > + struct tracked_lb_changes tracked_deleted_lbs;
> > + struct tracked_lb_changes tracked_updated_lb_groups;
> > + struct tracked_lb_changes tracked_deleted_lb_groups;
> > +};
>
> The typing here is a bit odd. The problem is that struct
> tracked_lb_changes is being used for two distinct types of lists. I
> think you should do one of two things here.
>
> 1) Get rid of the trakced_lb_changes struct and use ovs_list in-line.
>
> struct tracked_lb_data {
> struct ovs_list tracked_updated_lbs;
> struct ovs_list tracked_deleted_lbs;
> struct ovs_list tracked_updated_lb_groups;
> struct ovs_list tracked_deleted_lb_groups;
> };
>
> 2) Make a separate type for tracked lbs and tracked lb groups.
>
> struct tracked_lb_changes {
> struct ovs_list updated;
> };
>
> struct tracked_lb_group_changes {
> struct ovs_list updated;
> };
>
> struct tracked_lb_data {
> struct tracked_lb_changes tracked_updated_lbs;
> struct tracked_lb_changes tracked_deleted_lbs;
> struct tracked_lb_group_changes tracked_updated_lb_groups;
> struct tracked_lb_group_changes tracked_deleted_lb_groups;
> };
>
> I think option (2) is my preference.
I prefer option (1), because the names of the fields in struct
tracked_lb_data already tells the purpose, and the only member "update" in
the struct tracked_lb_changes is unnecessary. In addition, the name
"update" is confusing, because it is in fact used for both "updated"
(added/updated) and "deleted".
>
> > +
> > struct northd_lb_data {
> > struct hmap lbs;
> > struct hmap lb_groups;
> > +
> > + /* tracked data*/
> > + bool tracked;
> > + struct tracked_lb_data tracked_lb_data;
> > };
> >
> > void *en_northd_lb_data_init(struct engine_node *, struct engine_arg
*);
> > void en_northd_lb_data_run(struct engine_node *, void *data);
> > void en_northd_lb_data_cleanup(void *data);
> > +void en_northd_lb_data_clear_tracked_data(void *data);
> > +
> > +bool northd_lb_data_load_balancer_handler(struct engine_node *,
> > + void *data);
> > +bool northd_lb_data_load_balancer_group_handler(struct engine_node *,
> > + void *data);
> >
> > #endif /* end of EN_NORTHD_LB_DATA_H */
> > diff --git a/northd/en-northd.c b/northd/en-northd.c
> > index cc7d838451..b3c03c54bd 100644
> > --- a/northd/en-northd.c
> > +++ b/northd/en-northd.c
> > @@ -206,6 +206,30 @@ northd_sb_port_binding_handler(struct engine_node
*node,
> > return true;
> > }
> >
> > +bool
> > +northd_northd_lb_data_handler(struct engine_node *node, void *data)
> > +{
> > + struct northd_lb_data *lb_data =
> > + engine_get_input_data("northd_lb_data", node);
> > +
> > + if (!lb_data->tracked) {
> > + return false;
> > + }
> > +
> > + struct northd_data *nd = data;
> > +
> > + if (!northd_handle_lb_data_changes(&lb_data->tracked_lb_data,
> > + &nd->ls_datapaths,
> > + &nd->lr_datapaths,
> > + &nd->lb_datapaths_map,
> > + &nd->lb_group_datapaths_map)) {
> > + return false;
> > + }
> > +
> > + engine_set_node_state(node, EN_UPDATED);
> > + return true;
> > +}
> > +
> > void
> > *en_northd_init(struct engine_node *node OVS_UNUSED,
> > struct engine_arg *arg OVS_UNUSED)
> > diff --git a/northd/en-northd.h b/northd/en-northd.h
> > index 20cc77f108..5674f4390c 100644
> > --- a/northd/en-northd.h
> > +++ b/northd/en-northd.h
> > @@ -17,5 +17,6 @@ void en_northd_clear_tracked_data(void *data);
> > bool northd_nb_nb_global_handler(struct engine_node *, void *data
OVS_UNUSED);
> > bool northd_nb_logical_switch_handler(struct engine_node *, void
*data);
> > bool northd_sb_port_binding_handler(struct engine_node *, void *data);
> > +bool northd_northd_lb_data_handler(struct engine_node *, void *data);
> >
> > #endif /* EN_NORTHD_H */
> > diff --git a/northd/inc-proc-northd.c b/northd/inc-proc-northd.c
> > index b2e884962f..b444a488db 100644
> > --- a/northd/inc-proc-northd.c
> > +++ b/northd/inc-proc-northd.c
> > @@ -141,13 +141,18 @@ static ENGINE_NODE(sync_to_sb_addr_set,
"sync_to_sb_addr_set");
> > static ENGINE_NODE(fdb_aging, "fdb_aging");
> > static ENGINE_NODE(fdb_aging_waker, "fdb_aging_waker");
> > static ENGINE_NODE(sync_to_sb_lb, "sync_to_sb_lb");
> > -static ENGINE_NODE(northd_lb_data, "northd_lb_data");
> > +static ENGINE_NODE_WITH_CLEAR_TRACK_DATA(northd_lb_data,
"northd_lb_data");
> >
> > void inc_proc_northd_init(struct ovsdb_idl_loop *nb,
> > struct ovsdb_idl_loop *sb)
> > {
> > /* Define relationships between nodes where first argument is
dependent
> > * on the second argument */
> > + engine_add_input(&en_northd_lb_data, &en_nb_load_balancer,
> > + northd_lb_data_load_balancer_handler);
> > + engine_add_input(&en_northd_lb_data, &en_nb_load_balancer_group,
> > + northd_lb_data_load_balancer_group_handler);
> > +
Maybe it is not a big deal, but I think it is better to move the
dependencies that have handlers after the ones that don't, so that any
changes that will trigger recompute are handled first, to avoid wasting
time handling other changes and then fallback to recompute.
With these minor comments addressed:
Acked-by: Han Zhou <hz...@ovn.org>
> > engine_add_input(&en_northd, &en_nb_port_group, NULL);
> > engine_add_input(&en_northd, &en_nb_acl, NULL);
> > engine_add_input(&en_northd, &en_nb_logical_router, NULL);
> > @@ -171,6 +176,8 @@ void inc_proc_northd_init(struct ovsdb_idl_loop *nb,
> > engine_add_input(&en_northd, &en_sb_static_mac_binding, NULL);
> > engine_add_input(&en_northd, &en_sb_chassis_template_var, NULL);
> >
> > + engine_add_input(&en_northd, &en_northd_lb_data,
> > + northd_northd_lb_data_handler);
> > engine_add_input(&en_northd, &en_sb_port_binding,
> > northd_sb_port_binding_handler);
> > engine_add_input(&en_northd, &en_nb_nb_global,
> > @@ -178,10 +185,6 @@ void inc_proc_northd_init(struct ovsdb_idl_loop
*nb,
> > engine_add_input(&en_northd, &en_nb_logical_switch,
> > northd_nb_logical_switch_handler);
> >
> > - engine_add_input(&en_northd_lb_data, &en_nb_load_balancer, NULL);
> > - engine_add_input(&en_northd_lb_data, &en_nb_load_balancer_group,
NULL);
> > - engine_add_input(&en_northd, &en_northd_lb_data, NULL);
> > -
> > 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);
> > diff --git a/northd/northd.c b/northd/northd.c
> > index 890186b29c..f27f0de49b 100644
> > --- a/northd/northd.c
> > +++ b/northd/northd.c
> > @@ -40,6 +40,7 @@
> > #include "lib/lb.h"
> > #include "memory.h"
> > #include "northd.h"
> > +#include "en-northd-lb-data.h"
> > #include "lib/ovn-parallel-hmap.h"
> > #include "ovn/actions.h"
> > #include "ovn/features.h"
> > @@ -5323,6 +5324,82 @@ northd_handle_sb_port_binding_changes(
> > return true;
> > }
> >
> > +bool
> > +northd_handle_lb_data_changes(struct tracked_lb_data *trk_lb_data,
> > + struct ovn_datapaths *ls_datapaths,
> > + struct ovn_datapaths *lr_datapaths,
> > + struct hmap *lb_datapaths_map,
> > + struct hmap *lb_group_datapaths_map)
> > +{
> > + struct tracked_lb *trk_lb;
> > +
> > + struct ovn_lb_datapaths *lb_dps;
> > + LIST_FOR_EACH (trk_lb, list_node,
> > + &trk_lb_data->tracked_deleted_lbs.updated) {
> > + if (trk_lb->health_checks) {
> > + /* Fall back to recompute if the deleted load balancer has
> > + * health checks configured. */
> > + return false;
> > + }
> > +
> > + const struct uuid *lb_uuid =
> > + &trk_lb->lb->nlb->header_.uuid;
> > +
> > + lb_dps = ovn_lb_datapaths_find(lb_datapaths_map, lb_uuid);
> > + ovs_assert(lb_dps);
> > + hmap_remove(lb_datapaths_map, &lb_dps->hmap_node);
> > + ovn_lb_datapaths_destroy(lb_dps);
> > + }
> > +
> > + LIST_FOR_EACH (trk_lb, list_node,
> > + &trk_lb_data->tracked_updated_lbs.updated) {
> > + if (trk_lb->health_checks) {
> > + /* Fall back to recompute if the created/updated load
balancer has
> > + * health checks configured. */
> > + return false;
> > + }
> > +
> > + const struct uuid *lb_uuid =
> > + &trk_lb->lb->nlb->header_.uuid;
> > + lb_dps = ovn_lb_datapaths_find(lb_datapaths_map, lb_uuid);
> > + if (!lb_dps) {
> > + lb_dps = ovn_lb_datapaths_create(trk_lb->lb,
> > + ods_size(ls_datapaths),
> > + ods_size(lr_datapaths));
> > + hmap_insert(lb_datapaths_map, &lb_dps->hmap_node,
> > + uuid_hash(lb_uuid));
> > + }
> > + }
> > +
> > + struct ovn_lb_group_datapaths *lb_group_dps;
> > + struct tracked_lb_group *trk_lb_group;
> > + LIST_FOR_EACH_SAFE (trk_lb_group, list_node,
> > +
&trk_lb_data->tracked_deleted_lb_groups.updated) {
> > + const struct uuid *lb_uuid = &trk_lb_group->lb_group->uuid;
> > + lb_group_dps =
ovn_lb_group_datapaths_find(lb_group_datapaths_map,
> > + lb_uuid);
> > + ovs_assert(lb_group_dps);
> > + hmap_remove(lb_group_datapaths_map, &lb_group_dps->hmap_node);
> > + ovn_lb_group_datapaths_destroy(lb_group_dps);
> > + }
> > +
> > + LIST_FOR_EACH_SAFE (trk_lb_group, list_node,
> > +
&trk_lb_data->tracked_updated_lb_groups.updated) {
> > + const struct uuid *lb_uuid = &trk_lb_group->lb_group->uuid;
> > + lb_group_dps =
ovn_lb_group_datapaths_find(lb_group_datapaths_map,
> > + lb_uuid);
> > + if (!lb_group_dps) {
> > + lb_group_dps = ovn_lb_group_datapaths_create(
> > + trk_lb_group->lb_group, ods_size(ls_datapaths),
> > + ods_size(lr_datapaths));
> > + hmap_insert(lb_group_datapaths_map,
&lb_group_dps->hmap_node,
> > + uuid_hash(lb_uuid));
> > + }
> > + }
> > +
> > + return true;
> > +}
> > +
> > struct multicast_group {
> > const char *name;
> > uint16_t key; /*
OVN_MIN_MULTICAST...OVN_MAX_MULTICAST. */
> > diff --git a/northd/northd.h b/northd/northd.h
> > index 7d92028c7d..7d17921fa2 100644
> > --- a/northd/northd.h
> > +++ b/northd/northd.h
> > @@ -347,6 +347,13 @@ bool lflow_handle_northd_ls_changes(struct
ovsdb_idl_txn *ovnsb_txn,
> > bool northd_handle_sb_port_binding_changes(
> > const struct sbrec_port_binding_table *, struct hmap *ls_ports);
> >
> > +struct tracked_lb_data;
> > +bool northd_handle_lb_data_changes(struct tracked_lb_data *,
> > + struct ovn_datapaths *ls_datapaths,
> > + struct ovn_datapaths *lr_datapaths,
> > + struct hmap *lb_datapaths_map,
> > + struct hmap
*lb_group_datapaths_map);
> > +
> > void build_bfd_table(struct ovsdb_idl_txn *ovnsb_txn,
> > const struct nbrec_bfd_table *,
> > const struct sbrec_bfd_table *,
> > diff --git a/tests/ovn-northd.at b/tests/ovn-northd.at
> > index e79d33b2ae..dd0bd8b36a 100644
> > --- a/tests/ovn-northd.at
> > +++ b/tests/ovn-northd.at
> > @@ -9681,3 +9681,126 @@ AT_CHECK([grep "lr_in_gw_redirect" R1flows |sed
s'/table=../table=??/' |sort], [
> >
> > AT_CLEANUP
> > ])
> > +
> > +OVN_FOR_EACH_NORTHD_NO_HV([
> > +AT_SETUP([Load balancer incremental processing])
> > +ovn_start
> > +
> > +check_engine_stats() {
> > + northd_comp=$1
> > + lflow_comp=$2
> > +
> > + AT_CHECK([as northd ovn-appctl -t NORTHD_TYPE inc-engine/show-stats
northd_lb_data recompute], [0], [0
> > +])
> > +
> > + northd_recompute_ct=$(as northd ovn-appctl -t NORTHD_TYPE
inc-engine/show-stats northd recompute)
> > + if [[ "$northd_comp" == "norecompute" ]]; then
> > + check test "$northd_recompute_ct" -eq "0"
> > + else
> > + check test "$northd_recompute_ct" -ne "0"
> > + fi
> > +
> > + lflow_recompute_ct=$(as northd ovn-appctl -t NORTHD_TYPE
inc-engine/show-stats lflow recompute)
> > + if [[ "$lflow_comp" == "norecompute" ]]; then
> > + check test "$lflow_recompute_ct" -eq "0"
> > + else
> > + check test "$lflow_recompute_ct" -ne "0"
> > + fi
> > +}
> > +
> > +# Test I-P for load balancers.
> > +# Presently ovn-northd handles I-P for NB LBs in northd_lb_data engine
node
> > +# only.
> > +check as northd ovn-appctl -t NORTHD_TYPE inc-engine/clear-stats
> > +check ovn-nbctl --wait=sb lb-add lb1 10.0.0.10:80 10.0.0.3:80
> > +check_engine_stats norecompute recompute
> > +
> > +check ovn-nbctl --wait=sb set load_balancer .
ip_port_mappings:10.0.0.3=sw0-p1:10.0.0.2
> > +check_engine_stats norecompute recompute
> > +
> > +check ovn-nbctl --wait=sb set load_balancer . options:foo=bar
> > +check_engine_stats norecompute recompute
> > +
> > +check ovn-nbctl --wait=sb -- lb-add lb2 20.0.0.10:80 20.0.0.20:80 --
lb-add lb3 30.0.0.10:80 30.0.0.20:80
> > +check_engine_stats norecompute recompute
> > +
> > +check ovn-nbctl --wait=sb -- lb-del lb2 -- lb-del lb3
> > +check_engine_stats norecompute recompute
> > +
> > +AT_CHECK([ovn-nbctl --wait=sb \
> > + -- --id=@hc create Load_Balancer_Health_Check
vip="10.0.0.10\:80" \
> > + options:failure_count=100 \
> > + -- add Load_Balancer . health_check @hc | uuidfilt], [0],
[<0>
> > +])
> > +check_engine_stats recompute recompute
> > +
> > +# Any change to load balancer health check should also result in full
recompute
> > +# of northd node (but not northd_lb_data node)
> > +check as northd ovn-appctl -t NORTHD_TYPE inc-engine/clear-stats
> > +check ovn-nbctl --wait=sb set load_balancer_health_check .
options:foo=bar1
> > +check_engine_stats recompute recompute
> > +
> > +# Delete the health check from the load balancer. northd engine node
should do a full recompute.
> > +check as northd ovn-appctl -t NORTHD_TYPE inc-engine/clear-stats
> > +check ovn-nbctl --wait=sb clear Load_Balancer . health_check
> > +check_engine_stats recompute recompute
> > +
> > +check as northd ovn-appctl -t NORTHD_TYPE inc-engine/clear-stats
> > +check ovn-nbctl ls-add sw0
> > +check ovn-nbctl --wait=sb lr-add lr0
> > +check_engine_stats recompute recompute
> > +
> > +# Associate lb1 to sw0. There should be a full recompute of northd
engine node
> > +check as northd ovn-appctl -t NORTHD_TYPE inc-engine/clear-stats
> > +check ovn-nbctl --wait=sb ls-lb-add sw0 lb1
> > +check_engine_stats recompute recompute
> > +
> > +# Disassociate lb1 from sw0. There should be a full recompute of
northd engine node.
> > +check as northd ovn-appctl -t NORTHD_TYPE inc-engine/clear-stats
> > +check ovn-nbctl --wait=sb ls-lb-del sw0 lb1
> > +check_engine_stats recompute recompute
> > +
> > +# Test load balancer group now
> > +check as northd ovn-appctl -t NORTHD_TYPE inc-engine/clear-stats
> > +lbg1_uuid=$(ovn-nbctl create load_balancer_group name=lbg1)
> > +check_engine_stats norecompute recompute
> > +
> > +lb1_uuid=$(fetch_column nb:Load_Balancer _uuid)
> > +
> > +# Add lb to the lbg1 group
> > +check as northd ovn-appctl -t NORTHD_TYPE inc-engine/clear-stats
> > +check ovn-nbctl add load_balancer_group . load_Balancer $lb1_uuid
> > +check_engine_stats norecompute recompute
> > +
> > +check as northd ovn-appctl -t NORTHD_TYPE inc-engine/clear-stats
> > +check ovn-nbctl clear load_balancer_group . load_Balancer
> > +check_engine_stats norecompute recompute
> > +
> > +# Add back lb to the lbg1 group
> > +check as northd ovn-appctl -t NORTHD_TYPE inc-engine/clear-stats
> > +check ovn-nbctl add load_balancer_group . load_Balancer $lb1_uuid
> > +check_engine_stats norecompute recompute
> > +
> > +check as northd ovn-appctl -t NORTHD_TYPE inc-engine/clear-stats
> > +check ovn-nbctl add logical_switch sw0 load_balancer_group $lbg1_uuid
> > +check_engine_stats recompute recompute
> > +
> > +# Update lb and this should result in recompute
> > +check as northd ovn-appctl -t NORTHD_TYPE inc-engine/clear-stats
> > +check ovn-nbctl --wait=sb set load_balancer . options:bar=foo
> > +check_engine_stats recompute recompute
> > +
> > +check as northd ovn-appctl -t NORTHD_TYPE inc-engine/clear-stats
> > +check ovn-nbctl clear logical_switch sw0 load_balancer_group
> > +check_engine_stats recompute recompute
> > +
> > +check as northd ovn-appctl -t NORTHD_TYPE inc-engine/clear-stats
> > +check ovn-nbctl add logical_router lr0 load_balancer_group $lbg1_uuid
> > +check_engine_stats recompute recompute
> > +
> > +check as northd ovn-appctl -t NORTHD_TYPE inc-engine/clear-stats
> > +check ovn-nbctl clear logical_router lr0 load_balancer_group
> > +check_engine_stats recompute recompute
> > +
> > +AT_CLEANUP
> > +])
>
>
> _______________________________________________
> 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
