On Fri, Jul 14, 2023 at 6:54 PM Numan Siddique <[email protected]> wrote: > > On Wed, Jul 12, 2023 at 9:26 PM Han Zhou <[email protected]> wrote: > > > > On Sat, Jul 8, 2023 at 3:57 AM Mark Michelson <[email protected]> wrote: > > > > > > Hi Numan, > > > > > > I have one small nit below. > > > > > > On 7/7/23 01:53, [email protected] wrote: > > > > From: Numan Siddique <[email protected]> > > > > > > > > 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 <[email protected]> > > > > --- > > > > 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". > > How about the below one ? > > ----------------------------------------- > struct tracked_lb_data { > /* Both created and updated lbs. Contains hmapx of 'struct ovn_northd_lb' */ > struct hmapx crupdated_lbs; > struct hmapx deleted_lbs; > > /* Both created and updated lb groups. Contains hmapx of > * 'struct ovn_lb_group' */ > struct hmapx crupdated_lb_groups; /* Both created and updated lb groups. */ > struct hmapx deleted_lb_groups; > > bool has_health_checks; > }; > ----------------------------------------- > > I just googled a proper word for create and update and came across > "upsert" and "crupdate" in the database context. > I prefer the latter. So went with it. > > Since northd engine will be handling both the created and updated > lbs/lb groups, I think having just one is sufficient. > > Also I'm changing from a list to hmapx because in handling load > balancer groups I want to also update the "crupdated_lbs" > with the load balancers in the load balancer groups as this is > required later when handling lb group changes for lflow_engine. > > > Please let me know if there are any comments/objections. > > Thanks > Numan > > > > > > > > > > > > > + > > > > 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.
I didn't get this comment. The above handlers are for the "lb_data" engine node. Since "lb_data" engine node is an input to "northd" engine node, "lb_data" engine node handlers will be evaluated first anyway. Correct me if I'm wrong. Thanks Numan > > > > With these minor comments addressed: > > Acked-by: Han Zhou <[email protected]> > > > > > > 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 > > > [email protected] > > > https://mail.openvswitch.org/mailman/listinfo/ovs-dev > > > > > _______________________________________________ > > dev mailing list > > [email protected] > > https://mail.openvswitch.org/mailman/listinfo/ovs-dev _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
