On Mon, Sep 11, 2023 at 9:21 PM Han Zhou <[email protected]> wrote:
>
> On Mon, Sep 11, 2023 at 9:01 AM <[email protected]> wrote:
> >
> > From: Numan Siddique <[email protected]>
> >
> > 'lb_data' engine node now also handles logical switch changes.
> > Its data maintains ls to lb related information. i.e if a
> > logical switch sw0 has lb1, lb2 and lb3 associated then
> > it stores this info in its data. And when a new load balancer
> > lb4 is associated to it, it stores this information in its
> > tracked data so that 'northd' engine node can handle it
> > accordingly. Tracked data will have information like:
> > changed ls -> {sw0 : {associated_lbs: [lb4]}
> >
> > The first handler 'northd_lb_data_handler_pre_od' is called before the
> > 'northd_nb_logical_switch_handler' handler and it just creates or
> > deletes the lb_datapaths hmap for the tracked lbs.
> >
> > The northd handler 'northd_lb_data_handler' updates the
> > ovn_lb_datapaths's 'nb_ls_map' bitmap accordingly.
> >
> > Eg. If the lb_data has the below tracked data:
> >
> > tracked_data = {'crupdated_lbs': [lb1, lb2],
> > 'deleted_lbs': [lb3],
> > 'crupdated_lb_groups': [lbg1, lbg2],
> > 'crupdated_ls_lbs': [{ls: sw0, assoc_lbs: [lb1],
> > {ls: sw1, assoc_lbs: [lb1, lb2]}
> >
> > The handler northd_lb_data_handler(), creates the
> > ovn_lb_datapaths object for lb1 and lb2 and deletes lb3 from
> > the ovn_lb_datapaths hmap. It does the same for the created or updated lb
> > groups lbg1 and lbg2 in the ovn_lbgrp_datapaths map. It also updates the
> > nb_ls_bitmap of lb1 for sw0 and sw1 and nb_ls_bitmap of lb2 for sw1.
> >
> > Reviewed-by: Ales Musil <[email protected]>
> > Acked-by: Mark Michelson <[email protected]>
> > Signed-off-by: Numan Siddique <[email protected]>
> > ---
> > lib/lb.c | 5 +-
> > northd/en-lb-data.c | 176 +++++++++++++++++++++++++++++++++++++++
> > northd/en-lb-data.h | 17 ++++
> > northd/en-lflow.c | 6 ++
> > northd/en-northd.c | 6 +-
> > northd/inc-proc-northd.c | 2 +
> > northd/northd.c | 83 +++++++++++++++---
> > northd/northd.h | 4 +-
> > tests/ovn-northd.at | 56 +++++++++----
> > 9 files changed, 322 insertions(+), 33 deletions(-)
> >
> > diff --git a/lib/lb.c b/lib/lb.c
> > index 6fd67e2218..e6c9dc2be2 100644
> > --- a/lib/lb.c
> > +++ b/lib/lb.c
> > @@ -1088,7 +1088,10 @@ ovn_lb_datapaths_add_ls(struct ovn_lb_datapaths
> *lb_dps, size_t n,
> > struct ovn_datapath **ods)
> > {
> > for (size_t i = 0; i < n; i++) {
> > - bitmap_set1(lb_dps->nb_ls_map, ods[i]->index);
> > + if (!bitmap_is_set(lb_dps->nb_ls_map, ods[i]->index)) {
> > + bitmap_set1(lb_dps->nb_ls_map, ods[i]->index);
> > + lb_dps->n_nb_ls++;
> > + }
> > }
> > }
> >
> > diff --git a/northd/en-lb-data.c b/northd/en-lb-data.c
> > index 8acd9c8cb2..02b1bfd7a4 100644
> > --- a/northd/en-lb-data.c
> > +++ b/northd/en-lb-data.c
> > @@ -39,6 +39,14 @@ static void lb_data_destroy(struct ed_type_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 void build_od_lb_map(const struct nbrec_logical_switch_table *,
> > + struct hmap *od_lb_map);
> > +static struct od_lb_data *find_od_lb_data(struct hmap *od_lb_map,
> > + const struct uuid *od_uuid);
> > +static void destroy_od_lb_data(struct od_lb_data *od_lb_data);
> > +static struct od_lb_data *create_od_lb_data(struct hmap *od_lb_map,
> > + const struct uuid *od_uuid);
> > +
> > static struct ovn_lb_group *create_lb_group(
> > const struct nbrec_load_balancer_group *, struct hmap *lbs,
> > struct hmap *lb_groups);
> > @@ -54,6 +62,7 @@ static struct crupdated_lbgrp *
> > struct tracked_lb_data *);
> > static void add_deleted_lbgrp_to_tracked_data(
> > struct ovn_lb_group *, struct tracked_lb_data *);
> > +static bool is_ls_lbs_changed(const struct nbrec_logical_switch *nbs);
> >
> > /* 'lb_data' engine node manages the NB load balancers and load balancer
> > * groups. For each NB LB, it creates 'struct ovn_northd_lb' and
> > @@ -80,9 +89,13 @@ en_lb_data_run(struct engine_node *node, void *data)
> > EN_OVSDB_GET(engine_get_input("NB_load_balancer", node));
> > const struct nbrec_load_balancer_group_table *nb_lbg_table =
> > EN_OVSDB_GET(engine_get_input("NB_load_balancer_group", node));
> > + const struct nbrec_logical_switch_table *nb_ls_table =
> > + EN_OVSDB_GET(engine_get_input("NB_logical_switch", node));
> >
> > lb_data->tracked = false;
> > build_lbs(nb_lb_table, nb_lbg_table, &lb_data->lbs,
> &lb_data->lbgrps);
> > + build_od_lb_map(nb_ls_table, &lb_data->ls_lb_map);
> > +
> > engine_set_node_state(node, EN_UPDATED);
> > }
> >
> > @@ -230,18 +243,104 @@ lb_data_load_balancer_group_handler(struct
> engine_node *node, void *data)
> > return true;
> > }
> >
> > +struct od_lb_data {
> > + struct hmap_node hmap_node;
> > + struct uuid od_uuid;
> > + struct uuidset *lbs;
> > + struct uuidset *lbgrps;
> > +};
> > +
> > +bool
> > +lb_data_logical_switch_handler(struct engine_node *node, void *data)
> > +{
> > + struct ed_type_lb_data *lb_data = (struct ed_type_lb_data *) data;
> > + const struct nbrec_logical_switch_table *nb_ls_table =
> > + EN_OVSDB_GET(engine_get_input("NB_logical_switch", node));
> > +
> > + struct tracked_lb_data *trk_lb_data = &lb_data->tracked_lb_data;
> > + lb_data->tracked = true;
> > +
> > + bool changed = false;
> > + const struct nbrec_logical_switch *nbs;
> > + NBREC_LOGICAL_SWITCH_TABLE_FOR_EACH_TRACKED (nbs, nb_ls_table) {
> > + if (nbrec_logical_switch_is_deleted(nbs)) {
> > + struct od_lb_data *od_lb_data =
> > + find_od_lb_data(&lb_data->ls_lb_map, &nbs->header_.uuid);
> > + if (od_lb_data) {
> > + hmap_remove(&lb_data->ls_lb_map, &od_lb_data->hmap_node);
> > + destroy_od_lb_data(od_lb_data);
> > + }
> > + } else {
> > + if (!is_ls_lbs_changed(nbs)) {
> > + continue;
> > + }
> > + struct crupdated_od_lb_data *codlb = xzalloc(sizeof *codlb);
> > + codlb->od_uuid = nbs->header_.uuid;
> > + uuidset_init(&codlb->assoc_lbs);
> > +
> > + struct od_lb_data *od_lb_data =
> > + find_od_lb_data(&lb_data->ls_lb_map, &nbs->header_.uuid);
> > + if (!od_lb_data) {
> > + od_lb_data = create_od_lb_data(&lb_data->ls_lb_map,
> > + &nbs->header_.uuid);
> > + }
> > +
> > + struct uuidset *pre_lb_uuids = od_lb_data->lbs;
> > + od_lb_data->lbs = xzalloc(sizeof *od_lb_data->lbs);
> > + uuidset_init(od_lb_data->lbs);
> > +
> > + for (size_t i = 0; i < nbs->n_load_balancer; i++) {
> > + const struct uuid *lb_uuid =
> > + &nbs->load_balancer[i]->header_.uuid;
> > + uuidset_insert(od_lb_data->lbs, lb_uuid);
> > +
> > + struct uuidset_node *unode = uuidset_find(pre_lb_uuids,
> > + lb_uuid);
> > +
> > + if (!unode || (nbrec_load_balancer_row_get_seqno(
> > + nbs->load_balancer[i], OVSDB_IDL_CHANGE_MODIFY)
> > 0)) {
> > + /* Add this lb to the tracked data. */
> > + uuidset_insert(&codlb->assoc_lbs, lb_uuid);
> > + changed = true;
> > + }
> > +
> > + if (unode) {
> > + uuidset_delete(pre_lb_uuids, unode);
> > + }
> > + }
> > +
> > + if (!uuidset_is_empty(pre_lb_uuids)) {
> > + trk_lb_data->has_dissassoc_lbs_from_od = true;
> > + changed = true;
> > + }
> > +
> > + uuidset_destroy(pre_lb_uuids);
> > + free(pre_lb_uuids);
> > +
> > + ovs_list_insert(&trk_lb_data->crupdated_ls_lbs,
> &codlb->list_node);
> > + }
> > + }
> > +
> > + if (changed) {
> > + engine_set_node_state(node, EN_UPDATED);
> > + }
> > + return true;
> > +}
> > +
> > /* static functions. */
> > static void
> > lb_data_init(struct ed_type_lb_data *lb_data)
> > {
> > hmap_init(&lb_data->lbs);
> > hmap_init(&lb_data->lbgrps);
> > + hmap_init(&lb_data->ls_lb_map);
> >
> > struct tracked_lb_data *trk_lb_data = &lb_data->tracked_lb_data;
> > hmap_init(&trk_lb_data->crupdated_lbs);
> > hmapx_init(&trk_lb_data->deleted_lbs);
> > hmap_init(&trk_lb_data->crupdated_lbgrps);
> > hmapx_init(&trk_lb_data->deleted_lbgrps);
> > + ovs_list_init(&trk_lb_data->crupdated_ls_lbs);
> > }
> >
> > static void
> > @@ -259,6 +358,12 @@ lb_data_destroy(struct ed_type_lb_data *lb_data)
> > }
> > hmap_destroy(&lb_data->lbgrps);
> >
> > + struct od_lb_data *od_lb_data;
> > + HMAP_FOR_EACH_POP (od_lb_data, hmap_node, &lb_data->ls_lb_map) {
> > + destroy_od_lb_data(od_lb_data);
> > + }
> > + hmap_destroy(&lb_data->ls_lb_map);
> > +
> > destroy_tracked_data(lb_data);
> > hmap_destroy(&lb_data->tracked_lb_data.crupdated_lbs);
> > hmapx_destroy(&lb_data->tracked_lb_data.deleted_lbs);
> > @@ -301,12 +406,67 @@ create_lb_group(const struct
> nbrec_load_balancer_group *nbrec_lb_group,
> > return lb_group;
> > }
> >
> > +static void
> > +build_od_lb_map(const struct nbrec_logical_switch_table *nbrec_ls_table,
> > + struct hmap *od_lb_map)
> > +{
> > + const struct nbrec_logical_switch *nbrec_ls;
> > + NBREC_LOGICAL_SWITCH_TABLE_FOR_EACH (nbrec_ls, nbrec_ls_table) {
> > + if (!nbrec_ls->n_load_balancer) {
> > + continue;
> > + }
> > +
> > + struct od_lb_data *od_lb_data =
> > + create_od_lb_data(od_lb_map, &nbrec_ls->header_.uuid);
> > + for (size_t i = 0; i < nbrec_ls->n_load_balancer; i++) {
> > + uuidset_insert(od_lb_data->lbs,
> > + &nbrec_ls->load_balancer[i]->header_.uuid);
> > + }
> > + }
> > +}
> > +
> > +static struct od_lb_data *
> > +create_od_lb_data(struct hmap *od_lb_map, const struct uuid *od_uuid)
> > +{
> > + struct od_lb_data *od_lb_data = xzalloc(sizeof *od_lb_data);
> > + od_lb_data->od_uuid = *od_uuid;
> > + od_lb_data->lbs = xzalloc(sizeof *od_lb_data->lbs);
> > + uuidset_init(od_lb_data->lbs);
> > +
> > + hmap_insert(od_lb_map, &od_lb_data->hmap_node,
> > + uuid_hash(&od_lb_data->od_uuid));
> > + return od_lb_data;
> > +}
> > +
> > +static struct od_lb_data *
> > +find_od_lb_data(struct hmap *od_lb_map, const struct uuid *od_uuid)
> > +{
> > + struct od_lb_data *od_lb_data;
> > + HMAP_FOR_EACH_WITH_HASH (od_lb_data, hmap_node, uuid_hash(od_uuid),
> > + od_lb_map) {
> > + if (uuid_equals(&od_lb_data->od_uuid, od_uuid)) {
> > + return od_lb_data;
> > + }
> > + }
> > +
> > + return NULL;
> > +}
> > +
> > +static void
> > +destroy_od_lb_data(struct od_lb_data *od_lb_data)
> > +{
> > + uuidset_destroy(od_lb_data->lbs);
> > + free(od_lb_data->lbs);
> > + free(od_lb_data);
> > +}
> > +
> > static void
> > destroy_tracked_data(struct ed_type_lb_data *lb_data)
> > {
> > lb_data->tracked = false;
> > lb_data->tracked_lb_data.has_health_checks = false;
> > lb_data->tracked_lb_data.has_dissassoc_lbs_from_lbgrps = false;
> > + lb_data->tracked_lb_data.has_dissassoc_lbs_from_od = false;
> >
> > struct hmapx_node *node;
> > HMAPX_FOR_EACH_SAFE (node, &lb_data->tracked_lb_data.deleted_lbs) {
> > @@ -331,6 +491,15 @@ destroy_tracked_data(struct ed_type_lb_data *lb_data)
> > hmapx_destroy(&crupdated_lbg->assoc_lbs);
> > free(crupdated_lbg);
> > }
> > +
> > + struct crupdated_od_lb_data *codlb;
> > + LIST_FOR_EACH_SAFE (codlb, list_node,
> > + &lb_data->tracked_lb_data.crupdated_ls_lbs) {
> > + ovs_list_remove(&codlb->list_node);
> > + uuidset_destroy(&codlb->assoc_lbs);
> > +
> > + free(codlb);
> > + }
> > }
> >
> > static void
> > @@ -376,3 +545,10 @@ add_deleted_lbgrp_to_tracked_data(struct
> ovn_lb_group *lbg,
> > {
> > hmapx_add(&tracked_lb_data->deleted_lbgrps, lbg);
> > }
> > +
> > +static bool
> > +is_ls_lbs_changed(const struct nbrec_logical_switch *nbs) {
> > + return ((nbrec_logical_switch_is_new(nbs) && nbs->n_load_balancer)
> > + || nbrec_logical_switch_is_updated(nbs,
> > + NBREC_LOGICAL_SWITCH_COL_LOAD_BALANCER));
> > +}
> > diff --git a/northd/en-lb-data.h b/northd/en-lb-data.h
> > index afb163dd9f..022b38523b 100644
> > --- a/northd/en-lb-data.h
> > +++ b/northd/en-lb-data.h
> > @@ -6,6 +6,7 @@
> > #include "openvswitch/hmap.h"
> > #include "include/openvswitch/list.h"
> > #include "lib/hmapx.h"
> > +#include "lib/uuidset.h"
> >
> > #include "lib/inc-proc-eng.h"
> >
> > @@ -27,6 +28,13 @@ struct crupdated_lbgrp {
> > struct hmapx assoc_lbs;
> > };
> >
> > +struct crupdated_od_lb_data {
> > + struct ovs_list list_node;
> > +
> > + struct uuid od_uuid;
> > + struct uuidset assoc_lbs;
> > +};
> > +
> > struct tracked_lb_data {
> > /* Both created and updated lbs. hmapx node is 'struct crupdated_lb
> *'. */
> > struct hmap crupdated_lbs;
> > @@ -41,12 +49,19 @@ struct tracked_lb_data {
> > /* Deleted lb_groups. hmapx node is 'struct ovn_lb_group *'. */
> > struct hmapx deleted_lbgrps;
> >
> > + /* List of logical switch <-> lb changes. List node is
> > + * 'struct crupdated_od_lb_data' */
> > + struct ovs_list crupdated_ls_lbs;
> > +
> > /* Indicates if any of the tracked lb has health checks enabled. */
> > bool has_health_checks;
> >
> > /* Indicates if any lb got disassociated from a lb group
> > * but not deleted. */
> > bool has_dissassoc_lbs_from_lbgrps;
> > +
> > + /* Indicates if a lb was disassociated from a logical switch. */
> > + bool has_dissassoc_lbs_from_od;
> > };
> >
> > /* struct which maintains the data of the engine node lb_data. */
> > @@ -56,6 +71,7 @@ struct ed_type_lb_data {
> >
> > /* hmap of load balancer groups. hmap node is 'struct ovn_lb_group
> *' */
> > struct hmap lbgrps;
> > + struct hmap ls_lb_map;
> >
> > /* tracked data*/
> > bool tracked;
> > @@ -69,5 +85,6 @@ void en_lb_data_clear_tracked_data(void *data);
> >
> > bool lb_data_load_balancer_handler(struct engine_node *, void *data);
> > bool lb_data_load_balancer_group_handler(struct engine_node *, void
> *data);
> > +bool lb_data_logical_switch_handler(struct engine_node *, void *data);
> >
> > #endif /* end of EN_NORTHD_LB_DATA_H */
> > diff --git a/northd/en-lflow.c b/northd/en-lflow.c
> > index ad8b62c735..2b84fef0ef 100644
> > --- a/northd/en-lflow.c
> > +++ b/northd/en-lflow.c
> > @@ -107,6 +107,12 @@ lflow_northd_handler(struct engine_node *node,
> > if (!northd_data->change_tracked) {
> > return false;
> > }
> > +
> > + /* Fall back to recompute if lb related data has changed. */
> > + if (northd_data->lb_changed) {
> > + return false;
> > + }
> > +
> > const struct engine_context *eng_ctx = engine_get_context();
> > struct lflow_data *lflow_data = data;
> >
> > diff --git a/northd/en-northd.c b/northd/en-northd.c
> > index 1704a18834..8487b003f7 100644
> > --- a/northd/en-northd.c
> > +++ b/northd/en-northd.c
> > @@ -1,4 +1,4 @@
> > -/*
> > + /*
> > * Licensed under the Apache License, Version 2.0 (the "License");
> > * you may not use this file except in compliance with the License.
> > * You may obtain a copy of the License at:
> > @@ -214,6 +214,10 @@ northd_lb_data_handler(struct engine_node *node,
> void *data)
> > return false;
> > }
> >
> > + /* Indicate the depedendant engine nodes that load balancer/group
> > + * related data has changed (including association to logical
> > + * switch/router). */
> > + nd->lb_changed = true;
> > engine_set_node_state(node, EN_UPDATED);
> > return true;
> > }
> > diff --git a/northd/inc-proc-northd.c b/northd/inc-proc-northd.c
> > index ccc6687207..303b58d43f 100644
> > --- a/northd/inc-proc-northd.c
> > +++ b/northd/inc-proc-northd.c
> > @@ -155,6 +155,8 @@ void inc_proc_northd_init(struct ovsdb_idl_loop *nb,
> > lb_data_load_balancer_handler);
> > engine_add_input(&en_lb_data, &en_nb_load_balancer_group,
> > lb_data_load_balancer_group_handler);
> > + engine_add_input(&en_lb_data, &en_nb_logical_switch,
> > + lb_data_logical_switch_handler);
> >
> > engine_add_input(&en_northd, &en_nb_logical_router, NULL);
> > engine_add_input(&en_northd, &en_nb_mirror, NULL);
> > diff --git a/northd/northd.c b/northd/northd.c
> > index e9cb906e2a..f767e0b39f 100644
> > --- a/northd/northd.c
> > +++ b/northd/northd.c
> > @@ -73,7 +73,7 @@ static bool install_ls_lb_from_router;
> > /* Use common zone for SNAT and DNAT if this option is set to "true". */
> > static bool use_common_zone = false;
> >
> > -/* MAC allocated for service monitor usage. Just one mac is allocated
> > +/* MAC allocated for service monitor usage. Just one mac is
> allocatedg5534
> > * for this purpose and ovn-controller's on each chassis will make use
> > * of this mac when sending out the packets to monitor the services
> > * defined in Service_Monitor Southbound table. Since these packets
> > @@ -852,7 +852,6 @@ ovn_datapath_destroy(struct hmap *datapaths, struct
> ovn_datapath *od)
> > free(od->l3dgw_ports);
> > destroy_mcast_info_for_datapath(od);
> > destroy_ports_for_datapath(od);
> > -
> > free(od);
> > }
> > }
> > @@ -4959,6 +4958,7 @@ destroy_northd_data_tracked_changes(struct
> northd_data *nd)
> > }
> >
> > nd->change_tracked = false;
> > + nd->lb_changed = false;
> > }
> >
> > /* Check if a changed LSP can be handled incrementally within the I-P
> engine
> > @@ -5074,6 +5074,7 @@ ls_port_create(struct ovsdb_idl_txn *ovnsb_txn,
> struct hmap *ls_ports,
> > * incrementally handled.
> > * Presently supports i-p for the below changes:
> > * - logical switch ports.
> > + * - load balancers.
> > */
> > static bool
> > ls_changes_can_be_handled(
> > @@ -5084,7 +5085,8 @@ ls_changes_can_be_handled(
> > for (col = 0; col < NBREC_LOGICAL_SWITCH_N_COLUMNS; col++) {
> > if (nbrec_logical_switch_is_updated(ls, col)) {
> > if (col == NBREC_LOGICAL_SWITCH_COL_ACLS ||
> > - col == NBREC_LOGICAL_SWITCH_COL_PORTS) {
> > + col == NBREC_LOGICAL_SWITCH_COL_PORTS ||
> > + col == NBREC_LOGICAL_SWITCH_COL_LOAD_BALANCER) {
> > continue;
> > }
> > return false;
> > @@ -5109,12 +5111,6 @@ ls_changes_can_be_handled(
> > return false;
> > }
> > }
> > - for (size_t i = 0; i < ls->n_load_balancer; i++) {
> > - if (nbrec_load_balancer_row_get_seqno(ls->load_balancer[i],
> > - OVSDB_IDL_CHANGE_MODIFY) > 0) {
> > - return false;
> > - }
> > - }
> > for (size_t i = 0; i < ls->n_load_balancer_group; i++) {
> > if
> (nbrec_load_balancer_group_row_get_seqno(ls->load_balancer_group[i],
> > OVSDB_IDL_CHANGE_MODIFY) > 0) {
> > @@ -5365,6 +5361,7 @@ northd_handle_ls_changes(struct ovsdb_idl_txn
> *ovnsb_idl_txn,
> > if (!ovs_list_is_empty(&nd->tracked_ls_changes.updated)) {
> > nd->change_tracked = true;
> > }
> > +
> > return true;
> >
> > fail:
> > @@ -5431,9 +5428,20 @@ northd_handle_sb_port_binding_changes(
> > return true;
> > }
> >
> > -/* Handler for lb_data engine changes. For every tracked lb_data
> > - * it creates or deletes the ovn_lb_datapaths/ovn_lb_group_datapaths
> > - * from the lb_datapaths hmap and lb_group_datapaths hmap. */
> > +/* Handler for lb_data engine changes. It does the following
> > + * For every tracked 'lb' and 'lb_group'
> > + * - it creates or deletes the ovn_lb_datapaths/ovn_lb_group_datapaths
> > + * from the lb_datapaths hmap and lb_group_datapaths hmap.
> > + *
> > + * - For any changes to a logical switch (in
> 'trk_lb_data->crupdated_ls_lbs')
> > + * due to association of a load balancer (eg. ovn-nbctl ls-lb-add sw0
> lb1),
> > + * the logical switch datapath is added to the load balancer
> (represented
> > + * by 'struct ovn_lb_datapaths') by calling ovn_lb_datapaths_add_ls().
> > + *
> > + * - For every 'lb' in the tracked data (trk_lb_data->crupdated_lbs) ,
> > + * it gets the associated logical switches and for each switch it
> > + * re-evaluates 'od->has_lb_vip' to reflect any changes to the lb
> vips.
> > + * */
> > bool
> > northd_handle_lb_data_changes(struct tracked_lb_data *trk_lb_data,
> > struct ovn_datapaths *ls_datapaths,
> > @@ -5459,8 +5467,15 @@ northd_handle_lb_data_changes(struct
> tracked_lb_data *trk_lb_data,
> > return false;
> > }
> >
> > + /* Fall back to recompute if any load balancer has been
> disassociated from
> > + * a logical switch or router. */
> > + if (trk_lb_data->has_dissassoc_lbs_from_od) {
> > + return false;
> > + }
> > +
> > struct ovn_lb_datapaths *lb_dps;
> > struct ovn_northd_lb *lb;
> > + struct ovn_datapath *od;
> > struct hmapx_node *hmapx_node;
> > HMAPX_FOR_EACH (hmapx_node, &trk_lb_data->deleted_lbs) {
> > lb = hmapx_node->data;
> > @@ -5468,10 +5483,22 @@ northd_handle_lb_data_changes(struct
> tracked_lb_data *trk_lb_data,
> >
> > lb_dps = ovn_lb_datapaths_find(lb_datapaths_map, lb_uuid);
> > ovs_assert(lb_dps);
> > +
> > + /* Re-evaluate 'od->has_lb_vip for od's associated with the
> > + * deleted lb. */
> > + size_t index;
> > + BITMAP_FOR_EACH_1 (index, ods_size(ls_datapaths),
> > + lb_dps->nb_ls_map) {
> > + od = ls_datapaths->array[index];
> > + init_lb_for_datapath(od);
> > + }
> > +
> > hmap_remove(lb_datapaths_map, &lb_dps->hmap_node);
> > ovn_lb_datapaths_destroy(lb_dps);
> > }
> >
> > + /* Create the 'lb_dps' if not already created for each
> > + * 'lb' in the trk_lb_data->crupdated_lbs. */
> > struct crupdated_lb *clb;
> > HMAP_FOR_EACH (clb, hmap_node, &trk_lb_data->crupdated_lbs) {
> > lb = clb->lb;
> > @@ -5504,6 +5531,37 @@ northd_handle_lb_data_changes(struct
> tracked_lb_data *trk_lb_data,
> > }
> > }
> >
> > + struct crupdated_od_lb_data *codlb;
> > + LIST_FOR_EACH (codlb, list_node, &trk_lb_data->crupdated_ls_lbs) {
> > + od = ovn_datapath_find(&ls_datapaths->datapaths,
> &codlb->od_uuid);
> > + ovs_assert(od);
> > +
> > + struct uuidset_node *uuidnode;
> > + UUIDSET_FOR_EACH (uuidnode, &codlb->assoc_lbs) {
> > + lb_dps = ovn_lb_datapaths_find(lb_datapaths_map,
> &uuidnode->uuid);
> > + ovs_assert(lb_dps);
> > + ovn_lb_datapaths_add_ls(lb_dps, 1, &od);
> > + }
> > +
> > + /* Re-evaluate 'od->has_lb_vip' */
> > + init_lb_for_datapath(od);
> > + }
> > +
> > + HMAP_FOR_EACH (clb, hmap_node, &trk_lb_data->crupdated_lbs) {
> > + lb = clb->lb;
> > + const struct uuid *lb_uuid = &lb->nlb->header_.uuid;
> > +
> > + lb_dps = ovn_lb_datapaths_find(lb_datapaths_map, lb_uuid);
> > + ovs_assert(lb_dps);
> > + size_t index;
> > + BITMAP_FOR_EACH_1 (index, ods_size(ls_datapaths),
> > + lb_dps->nb_ls_map) {
> > + od = ls_datapaths->array[index];
> > + /* Re-evaluate 'od->has_lb_vip' */
> > + init_lb_for_datapath(od);
>
> To continue the discussion of one of my comments in v6:
> > > > And I really didn't understand the last part. If there is any change
> of
> > > > the relationship between a LS and a LB, it should be reflected in
> > > > crupdated_ls_lbs, and handled in the first loop. So what's the
> purpose of
> > > > the second loop?
>
> > > I'm not very sure If I understood your comment. Please see the v7 and
> > > let me know if you still have some comments.
>
> So here the loop on trk_lb_data->crupdated_lbs is to re-evaluate
> 'od->has_lb_vip' for LS that have any LB association change, right?
Correct.
> But for my understanding the re-evaluation performed by the previous loop on
> trk_lb_data->crupdated_ls_lbs should have covered all such LS, and the
> current loop is unnecessary. Is there a case that the
> init_lb_for_datapath(od) would not be performed to a datapath by the
> previous loop but will be covered in this loop?
>
It is required. Lets say there are 2 lbs - lb1 and lb2
ovn-nbctl lb-add lb1 10.0.0.10:80 20.0.0.20:8080' will
ovn-nbctl lb-add lb2 10.0.0.20:80 20.0.0.30:8080
And lets say user runs the below command
ovn-nbctl ls-lb-add sw0 lb1
In this case the lb_data's tracked data will have
tracked_data = {'crupdated_lbs': [],
'deleted_lbs': [],
'crupdated_lb_groups': [],
'crupdated_ls_lbs': [{ls: sw0, assoc_lbs: [lb1]}
The handling in northd_handle_lb_data_changes() is straightforward and
od->has_lb_vip for sw0 is re-evaluated in the 'crupdated_ls_lbs' list iteration.
Now lets say the user runs the below command
----
ovn-nbctl ls-lb-add sw1 lb2 -- ovn-nbctl clear load_balancer lb1 vips
----
In this case the lb_data's tracked data will have
tracked_data = {'crupdated_lbs': ['lb1'],
'deleted_lbs': [],
'crupdated_lb_groups': [],
'crupdated_ls_lbs': [{ls: sw1, assoc_lbs: [lb2]}
For the command 'ovn-nbctl clear load_balancer lb1 vips', ovsdl idl
will only track the lb1 load balancer but not
the logical switches it references and hence 'crupdated_ls_lbs' will
not have sw0 in its entry.
But we do need to evaluate the od->has_lb_vip for all the logical switches
which references 'lb1' since the vip of lb1 is cleared now.
I hope this clarifies your doubt.
Thanks
Numan
> Thanks,
> Han
>
> > + }
> > + }
> > +
> > return true;
> > }
> >
> > @@ -16529,6 +16587,7 @@ bool lflow_handle_northd_ls_changes(struct
> ovsdb_idl_txn *ovnsb_txn,
> > struct hmap *lflows)
> > {
> > struct ls_change *ls_change;
> > +
> > LIST_FOR_EACH (ls_change, list_node, &ls_changes->updated) {
> > const struct sbrec_multicast_group *sbmc_flood =
> > mcast_group_lookup(lflow_input->sbrec_mcast_group_by_name_dp,
> > diff --git a/northd/northd.h b/northd/northd.h
> > index 2bb6910945..0d206a4e52 100644
> > --- a/northd/northd.h
> > +++ b/northd/northd.h
> > @@ -115,6 +115,8 @@ struct northd_data {
> > struct hmap svc_monitor_map;
> > bool change_tracked;
> > struct tracked_ls_changes tracked_ls_changes;
> > + bool lb_changed; /* Indicates if load balancers changed or
> association of
> > + * load balancer to logical switch/router changed.
> */
> > };
> >
> > struct lflow_data {
> > @@ -345,7 +347,7 @@ 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);
> > + struct hmap *lbgrp_datapaths_map);
> >
> > void build_bfd_table(struct ovsdb_idl_txn *ovnsb_txn,
> > const struct nbrec_bfd_table *,
> > diff --git a/tests/ovn-northd.at b/tests/ovn-northd.at
> > index 5c9da811fa..f0f8d1f503 100644
> > --- a/tests/ovn-northd.at
> > +++ b/tests/ovn-northd.at
> > @@ -10398,22 +10398,23 @@ ovn-nbctl lsp-set-type sw0-lr0 router
> > ovn-nbctl lsp-set-addresses sw0-lr0 00:00:00:00:ff:01
> > check as northd ovn-appctl -t NORTHD_TYPE inc-engine/clear-stats
> > ovn-nbctl --wait=sb lsp-set-options sw0-lr0 router-port=lr0-sw0
> > -check_engine_stats lb_data norecompute nocompute
> > +check_engine_stats lb_data norecompute compute
> > check_engine_stats northd recompute nocompute
> > check_engine_stats lflow recompute nocompute
> >
> > -# Associate lb1 to sw0. There should be a full recompute of northd
> engine node
> > +# Associate lb1 to sw0. There should be no 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 lb_data norecompute nocompute
> > -check_engine_stats northd recompute nocompute
> > +check_engine_stats lb_data norecompute compute
> > +check_engine_stats northd norecompute compute
> > check_engine_stats lflow recompute nocompute
> > +CHECK_NO_CHANGE_AFTER_RECOMPUTE
> >
> > # Modify the backend of the lb1 vip
> > check as northd ovn-appctl -t NORTHD_TYPE inc-engine/clear-stats
> > check ovn-nbctl --wait=sb set load_balancer lb1 vips:'"10.0.0.10:80"'='"
> 10.0.0.100:80"'
> > check_engine_stats lb_data norecompute compute
> > -check_engine_stats northd recompute nocompute
> > +check_engine_stats northd norecompute compute
> > check_engine_stats lflow recompute nocompute
> > CHECK_NO_CHANGE_AFTER_RECOMPUTE
> >
> > @@ -10421,7 +10422,7 @@ CHECK_NO_CHANGE_AFTER_RECOMPUTE
> > check as northd ovn-appctl -t NORTHD_TYPE inc-engine/clear-stats
> > check ovn-nbctl --wait=sb clear load_Balancer lb1 vips
> > check_engine_stats lb_data norecompute compute
> > -check_engine_stats northd recompute nocompute
> > +check_engine_stats northd norecompute compute
> > check_engine_stats lflow recompute nocompute
> > CHECK_NO_CHANGE_AFTER_RECOMPUTE
> >
> > @@ -10429,7 +10430,7 @@ CHECK_NO_CHANGE_AFTER_RECOMPUTE
> > 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 lb_data norecompute compute
> > -check_engine_stats northd recompute nocompute
> > +check_engine_stats northd norecompute compute
> > check_engine_stats lflow recompute nocompute
> > CHECK_NO_CHANGE_AFTER_RECOMPUTE
> >
> > @@ -10437,14 +10438,33 @@ CHECK_NO_CHANGE_AFTER_RECOMPUTE
> > check as northd ovn-appctl -t NORTHD_TYPE inc-engine/clear-stats
> > check ovn-nbctl --wait=sb lb-add lb1 10.0.0.20:80 10.0.0.30:8080
> > check_engine_stats lb_data norecompute compute
> > -check_engine_stats northd recompute nocompute
> > +check_engine_stats northd norecompute compute
> > check_engine_stats lflow recompute nocompute
> > CHECK_NO_CHANGE_AFTER_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 lb_data norecompute nocompute
> > +check_engine_stats lb_data norecompute compute
> > +check_engine_stats northd recompute nocompute
> > +check_engine_stats lflow recompute nocompute
> > +CHECK_NO_CHANGE_AFTER_RECOMPUTE
> > +
> > +# Associate lb1 to sw0 and also create a port sw0p1. This should not
> result in
> > +# full recompute of northd, but should rsult in full recompute of lflow
> node.
> > +check as northd ovn-appctl -t NORTHD_TYPE inc-engine/clear-stats
> > +check ovn-nbctl --wait=sb ls-lb-add sw0 lb1 -- lsp-add sw0 sw0p1
> > +check_engine_stats lb_data norecompute compute
> > +check_engine_stats northd norecompute compute
> > +check_engine_stats lflow recompute nocompute
> > +
> > +CHECK_NO_CHANGE_AFTER_RECOMPUTE
> > +check as northd ovn-appctl -t NORTHD_TYPE inc-engine/clear-stats
> > +
> > +# Disassociate lb1 from sw0. There should be a 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 lb_data norecompute compute
> > check_engine_stats northd recompute nocompute
> > check_engine_stats lflow recompute nocompute
> > CHECK_NO_CHANGE_AFTER_RECOMPUTE
> > @@ -10532,7 +10552,7 @@ CHECK_NO_CHANGE_AFTER_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 lb_data norecompute nocompute
> > +check_engine_stats lb_data norecompute compute
> > check_engine_stats northd recompute nocompute
> > check_engine_stats lflow recompute nocompute
> >
> > @@ -10577,7 +10597,7 @@ CHECK_NO_CHANGE_AFTER_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 lb_data norecompute nocompute
> > +check_engine_stats lb_data norecompute compute
> > check_engine_stats northd recompute nocompute
> > check_engine_stats lflow recompute nocompute
> >
> > @@ -10629,7 +10649,7 @@ check_engine_stats lflow recompute nocompute
> > # Add back lb group to logical switch and then delete it.
> > 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 lb_data norecompute nocompute
> > +check_engine_stats lb_data norecompute compute
> > check_engine_stats northd recompute nocompute
> > check_engine_stats lflow recompute nocompute
> >
> > @@ -10670,7 +10690,7 @@ check_engine_stats lflow recompute nocompute
> >
> > check as northd ovn-appctl -t NORTHD_TYPE inc-engine/clear-stats
> > check ovn-nbctl set logical_switch sw0 load_balancer_group=$lbg1_uuid
> > -check_engine_stats lb_data norecompute nocompute
> > +check_engine_stats lb_data norecompute compute
> > check_engine_stats northd recompute nocompute
> > check_engine_stats lflow recompute nocompute
> > CHECK_NO_CHANGE_AFTER_RECOMPUTE
> > @@ -10684,15 +10704,15 @@ CHECK_NO_CHANGE_AFTER_RECOMPUTE
> >
> > check as northd ovn-appctl -t NORTHD_TYPE inc-engine/clear-stats
> > check ovn-nbctl --wait=sb ls-lb-add sw0 lb2
> > -check_engine_stats lb_data norecompute nocompute
> > -check_engine_stats northd recompute nocompute
> > +check_engine_stats lb_data norecompute compute
> > +check_engine_stats northd norecompute compute
> > check_engine_stats lflow recompute nocompute
> > CHECK_NO_CHANGE_AFTER_RECOMPUTE
> >
> > check as northd ovn-appctl -t NORTHD_TYPE inc-engine/clear-stats
> > check ovn-nbctl --wait=sb ls-lb-add sw0 lb3
> > -check_engine_stats lb_data norecompute nocompute
> > -check_engine_stats northd recompute nocompute
> > +check_engine_stats lb_data norecompute compute
> > +check_engine_stats northd norecompute compute
> > check_engine_stats lflow recompute nocompute
> > CHECK_NO_CHANGE_AFTER_RECOMPUTE
> >
> > @@ -10706,7 +10726,7 @@ CHECK_NO_CHANGE_AFTER_RECOMPUTE
> >
> > check as northd ovn-appctl -t NORTHD_TYPE inc-engine/clear-stats
> > check ovn-nbctl --wait=sb ls-lb-del sw0 lb2
> > -check_engine_stats lb_data norecompute nocompute
> > +check_engine_stats lb_data norecompute compute
> > check_engine_stats northd recompute nocompute
> > check_engine_stats lflow recompute nocompute
> > CHECK_NO_CHANGE_AFTER_RECOMPUTE
> > --
> > 2.41.0
> >
> > _______________________________________________
> > 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
