On Fri, Aug 18, 2023 at 10:58 AM <num...@ovn.org> wrote:
> From: Numan Siddique <num...@ovn.org>
>
> '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]}
>
> In the 'northd' engne node, an additional handler for 'lb_data'
> is added after the 'nbrec_logical_switch' changes. With this patch
> we now have 2 handlers for 'lb_data' in 'northd' engine node.
>
> ----
> engine_add_input(&en_northd, &en_lb_data, northd_lb_data_handler_pre_od);
> engine_add_input(&en_northd, &en_nb_logical_switch,
> northd_nb_logical_switch_handler);
> engine_add_input(&en_northd, &en_lb_data, northd_lb_data_handler_post_od);
> ----
>
> 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 second handler 'northd_lb_data_handler_post_od' is called after
> the 'northd_nb_logical_switch_handler' handler and it updates the
> ovn_lb_datapaths's 'nb_ls_map' bitmap.
>
> 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]}
>
> then the first handler northd_lb_data_handler_pre_od(), creates the
> ovn_lb_datapaths object for lb1 and lb2 and deletes lb3 from
> the ovn_lb_datapaths hmap. Similarly for the created or updated lb
> groups lbg1 and lbg2.
>
> The second handler northd_lb_data_handler_post_od() updates the
> nb_ls_bitmap of lb1 for sw0 and sw1 and nb_ls_bitmap of lb2 for sw1.
>
> This second handler is added mainly so that the actual logical switch
> handler northd_nb_logical_switch_handler() has done modifying the
> 'od' struct.
>
> Signed-off-by: Numan Siddique <num...@ovn.org>
> ---
> 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 | 40 +++++++--
> northd/en-northd.h | 3 +-
> northd/inc-proc-northd.c | 5 +-
> northd/northd.c | 104 +++++++++++++++++++----
> northd/northd.h | 15 ++--
> tests/ovn-northd.at | 56 +++++++++----
> 10 files changed, 380 insertions(+), 47 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 23f2cb1021..e0c4db1422 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_lb_group *
> struct tracked_lb_data *);
> static void add_deleted_lb_group_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->lb_groups);
> + 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->lb_groups);
> + 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_lb_groups);
> hmapx_init(&trk_lb_data->deleted_lb_groups);
> + 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->lb_groups);
>
> + 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_lb_grops = 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_lb_group_to_tracked_data(struct
> ovn_lb_group *lbg,
> {
> hmapx_add(&tracked_lb_data->deleted_lb_groups, 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 2e9a024620..b2f86322a2 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_lb_group {
> 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_lb_groups;
>
> + /* 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_lb_grops;
> +
> + /* 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 lb_groups;
> + 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 db1bcbccd6..77e2eff056 100644
> --- a/northd/en-lflow.c
> +++ b/northd/en-lflow.c
> @@ -102,6 +102,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 d59ef062df..9d1838a1a4 100644
> --- a/northd/en-northd.c
> +++ b/northd/en-northd.c
> @@ -207,7 +207,7 @@ northd_sb_port_binding_handler(struct engine_node
> *node,
> }
>
> bool
> -northd_lb_data_handler(struct engine_node *node, void *data)
> +northd_lb_data_handler_pre_od(struct engine_node *node, void *data)
> {
> struct ed_type_lb_data *lb_data = engine_get_input_data("lb_data",
> node);
>
> @@ -230,19 +230,47 @@ northd_lb_data_handler(struct engine_node *node,
> void *data)
>
> /* Fall back to recompute if load balancer groups are deleted. */
> if (!hmapx_is_empty(&lb_data->tracked_lb_data.deleted_lb_groups)) {
> + }
> +
> + if (lb_data->tracked_lb_data.has_dissassoc_lbs_from_od) {
> 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)) {
> + if (!northd_handle_lb_data_changes_pre_od(&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;
> +}
> +
> +bool
> +northd_lb_data_handler_post_od(struct engine_node *node, void *data)
> +{
> + struct ed_type_lb_data *lb_data =
> + engine_get_input_data("lb_data", node);
> +
> + ovs_assert(lb_data->tracked);
> + ovs_assert(!lb_data->tracked_lb_data.has_dissassoc_lbs_from_od);
> +
> + struct northd_data *nd = data;
> +
> + if (!northd_handle_lb_data_changes_post_od(&lb_data->tracked_lb_data,
> + &nd->ls_datapaths,
> + &nd->lb_datapaths_map)) {
> 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/en-northd.h b/northd/en-northd.h
> index 3c77b64bb2..5926e7a9d3 100644
> --- a/northd/en-northd.h
> +++ b/northd/en-northd.h
> @@ -17,6 +17,7 @@ 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_lb_data_handler(struct engine_node *, void *data);
> +bool northd_lb_data_handler_pre_od(struct engine_node *, void *data);
> +bool northd_lb_data_handler_post_od(struct engine_node *, void *data);
>
> #endif /* EN_NORTHD_H */
> diff --git a/northd/inc-proc-northd.c b/northd/inc-proc-northd.c
> index 75d059645f..402c94e88c 100644
> --- a/northd/inc-proc-northd.c
> +++ b/northd/inc-proc-northd.c
> @@ -152,6 +152,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_port_group, NULL);
> engine_add_input(&en_northd, &en_nb_acl, NULL);
> @@ -180,9 +182,10 @@ void inc_proc_northd_init(struct ovsdb_idl_loop *nb,
> northd_sb_port_binding_handler);
> engine_add_input(&en_northd, &en_nb_nb_global,
> northd_nb_nb_global_handler);
> - engine_add_input(&en_northd, &en_lb_data, northd_lb_data_handler);
> + engine_add_input(&en_northd, &en_lb_data,
> northd_lb_data_handler_pre_od);
> engine_add_input(&en_northd, &en_nb_logical_switch,
> northd_nb_logical_switch_handler);
> + engine_add_input(&en_northd, &en_lb_data,
> northd_lb_data_handler_post_od);
>
> engine_add_input(&en_mac_binding_aging, &en_nb_nb_global, NULL);
> engine_add_input(&en_mac_binding_aging, &en_sb_mac_binding, NULL);
> diff --git a/northd/northd.c b/northd/northd.c
> index 4cc9ef8c8d..6e8efbd496 100644
> --- a/northd/northd.c
> +++ b/northd/northd.c
> @@ -853,7 +853,6 @@ ovn_datapath_destroy(struct hmap *datapaths, struct
> ovn_datapath *od)
> ovn_ls_port_group_destroy(&od->nb_pgs);
> destroy_mcast_info_for_datapath(od);
> destroy_ports_for_datapath(od);
> -
> free(od);
> }
> }
> @@ -4919,6 +4918,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
> @@ -5034,6 +5034,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(
> @@ -5042,8 +5043,11 @@ ls_changes_can_be_handled(
> /* Check if the columns are changed in this row. */
> enum nbrec_logical_switch_column_id col;
> for (col = 0; col < NBREC_LOGICAL_SWITCH_N_COLUMNS; col++) {
> - if (nbrec_logical_switch_is_updated(ls, col) &&
> - col != NBREC_LOGICAL_SWITCH_COL_PORTS) {
> + if (nbrec_logical_switch_is_updated(ls, col)) {
> + if (col == NBREC_LOGICAL_SWITCH_COL_PORTS ||
> + col == NBREC_LOGICAL_SWITCH_COL_LOAD_BALANCER) {
> + continue;
> + }
> return false;
> }
> }
> @@ -5072,12 +5076,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) {
> @@ -5357,6 +5355,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:
> @@ -5425,16 +5424,21 @@ northd_handle_sb_port_binding_changes(
>
> /* 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. */
> + * from the lb_datapaths hmap and lb_group_datapaths hmap.
> + *
> + * This handler is called if there are any lb_data changes but before
> + * processing the logical switch changes.
> + * */
> 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)
> +northd_handle_lb_data_changes_pre_od(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 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;
> @@ -5442,6 +5446,16 @@ 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);
> }
> @@ -5481,6 +5495,65 @@ northd_handle_lb_data_changes(struct
> tracked_lb_data *trk_lb_data,
> return true;
> }
>
> +/* This handler is called if there are any lb_data changes but afer
> processing
> + * the logical switch changes.
> + *
> + * For every tracked data it does the following:
> + * For any changes to a created or updated logical switch 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 created or updated load balancer in the tracked data,
> + * it gets the associated logical switches and for each switch it
> + * re-evaluates 'od->has_lb_vip'. Since this handler is called after
> + * handling any logical switch changes. */
> +bool
> +northd_handle_lb_data_changes_post_od(struct tracked_lb_data *trk_lb_data,
> + struct ovn_datapaths *ls_datapaths,
> + struct hmap *lb_datapaths_map)
> +{
> + ovs_assert(!trk_lb_data->has_health_checks);
> +
> + struct ovn_northd_lb *lb;
> + struct ovn_lb_datapaths *lb_dps;
> + struct ovn_datapath *od;
> + 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);
> + }
> +
> + struct crupdated_lb *clb;
> + 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);
> + }
> + }
> +
> + return true;
> +}
> +
> struct multicast_group {
> const char *name;
> uint16_t key; /* OVN_MIN_MULTICAST...OVN_MAX_MULTICAST.
> */
> @@ -16667,6 +16740,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 7d17921fa2..0ed7215356 100644
> --- a/northd/northd.h
> +++ b/northd/northd.h
> @@ -120,6 +120,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 {
> @@ -348,11 +350,14 @@ 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);
> +bool northd_handle_lb_data_changes_pre_od(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);
> +bool northd_handle_lb_data_changes_post_od(struct tracked_lb_data *,
> + struct ovn_datapaths
> *ls_datapaths,
> + struct hmap *lb_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 213fd51d77..9e9b26ce09 100644
> --- a/tests/ovn-northd.at
> +++ b/tests/ovn-northd.at
> @@ -9861,22 +9861,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
>
> @@ -9884,7 +9885,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
>
> @@ -9892,7 +9893,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
>
> @@ -9900,14 +9901,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 result
> in
> +# full recompute of northd and lflow engine 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 recompute 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
> @@ -9995,7 +10015,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
>
> @@ -10040,7 +10060,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
>
> @@ -10092,7 +10112,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
>
> @@ -10133,7 +10153,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
> @@ -10147,15 +10167,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
>
> @@ -10169,7 +10189,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.40.1
>
> _______________________________________________
> dev mailing list
> d...@openvswitch.org
> https://mail.openvswitch.org/mailman/listinfo/ovs-dev
>
>
Looks good to me, thanks.
Reviewed-by: Ales Musil <amu...@redhat.com>
--
Ales Musil
Senior Software Engineer - OVN Core
Red Hat EMEA <https://www.redhat.com>
amu...@redhat.com IM: amusil
<https://red.ht/sig>
_______________________________________________
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
