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.
>
> 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
