On Tue, Sep 12, 2023 at 8:38 AM Numan Siddique <[email protected]> wrote:
>
> 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.
>
Yes, thanks for explaining. I didn't realize that even without LS-LB
association changes there still can be an impact to od->has_lb_vip only
because of the LB update - I didn't think about the case where a LB has no
VIPs.
Regards,
Han
> 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
