From: Numan Siddique <num...@ovn.org> For a given load balancer group 'A', northd engine data maintains a bitmap of datapaths associated to this lb group. So when lb group 'A' gets associated to a logical switch 's1', the bitmap index of 's1' is set in its bitmap.
In order to handle the load balancer group changes incrementally for a logical switch, we need to set and clear the bitmap bits accordingly. And this patch does it. Reviewed-by: Ales Musil <amu...@redhat.com> Acked-by: Mark Michelson <mmich...@redhat.com> Signed-off-by: Numan Siddique <num...@ovn.org> --- northd/en-lb-data.c | 102 ++++++++++++++++++++++++++++++++------------ northd/en-lb-data.h | 4 ++ northd/northd.c | 63 +++++++++++++++++++++++---- tests/ovn-northd.at | 30 +++++++++---- 4 files changed, 155 insertions(+), 44 deletions(-) diff --git a/northd/en-lb-data.c b/northd/en-lb-data.c index d854042feb..fd09e719d8 100644 --- a/northd/en-lb-data.c +++ b/northd/en-lb-data.c @@ -63,6 +63,7 @@ static struct crupdated_lbgrp * 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); +static bool is_ls_lbgrps_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 @@ -264,12 +265,15 @@ lb_data_logical_switch_handler(struct engine_node *node, void *data) hmapx_add(&trk_lb_data->deleted_od_lb_data, od_lb_data); } } else { - if (!is_ls_lbs_changed(nbs)) { + bool ls_lbs_changed = is_ls_lbs_changed(nbs); + bool ls_lbgrps_changed = is_ls_lbgrps_changed(nbs); + if (!ls_lbs_changed && !ls_lbgrps_changed) { continue; } struct crupdated_od_lb_data *codlb = xzalloc(sizeof *codlb); codlb->od_uuid = nbs->header_.uuid; uuidset_init(&codlb->assoc_lbs); + uuidset_init(&codlb->assoc_lbgrps); struct od_lb_data *od_lb_data = find_od_lb_data(&lb_data->ls_lb_map, &nbs->header_.uuid); @@ -278,38 +282,66 @@ lb_data_logical_switch_handler(struct engine_node *node, void *data) &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); + if (ls_lbs_changed) { + 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; + } - struct uuidset_node *unode = uuidset_find(pre_lb_uuids, - lb_uuid); + uuidset_destroy(pre_lb_uuids); + free(pre_lb_uuids); + } - 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 (ls_lbgrps_changed) { + struct uuidset *pre_lbgrp_uuids = od_lb_data->lbgrps; + od_lb_data->lbgrps = xzalloc(sizeof *od_lb_data->lbgrps); + uuidset_init(od_lb_data->lbgrps); + for (size_t i = 0; i < nbs->n_load_balancer_group; i++) { + const struct uuid *lbg_uuid = + &nbs->load_balancer_group[i]->header_.uuid; + uuidset_insert(od_lb_data->lbgrps, lbg_uuid); + + if (!uuidset_find_and_delete(pre_lbgrp_uuids, + lbg_uuid)) { + /* Add this lb group to the tracked data. */ + uuidset_insert(&codlb->assoc_lbgrps, lbg_uuid); + changed = true; + } } - if (unode) { - uuidset_delete(pre_lb_uuids, unode); + if (!uuidset_is_empty(pre_lbgrp_uuids)) { + trk_lb_data->has_dissassoc_lbgrps_from_od = true; + changed = true; } - } - if (!uuidset_is_empty(pre_lb_uuids)) { - trk_lb_data->has_dissassoc_lbs_from_od = true; - changed = true; + uuidset_destroy(pre_lbgrp_uuids); + free(pre_lbgrp_uuids); } - uuidset_destroy(pre_lb_uuids); - free(pre_lb_uuids); - ovs_list_insert(&trk_lb_data->crupdated_ls_lbs, &codlb->list_node); } } @@ -407,7 +439,7 @@ build_od_lb_map(const struct nbrec_logical_switch_table *nbrec_ls_table, { const struct nbrec_logical_switch *nbrec_ls; NBREC_LOGICAL_SWITCH_TABLE_FOR_EACH (nbrec_ls, nbrec_ls_table) { - if (!nbrec_ls->n_load_balancer) { + if (!nbrec_ls->n_load_balancer && !nbrec_ls->n_load_balancer_group) { continue; } @@ -417,6 +449,10 @@ build_od_lb_map(const struct nbrec_logical_switch_table *nbrec_ls_table, uuidset_insert(od_lb_data->lbs, &nbrec_ls->load_balancer[i]->header_.uuid); } + for (size_t i = 0; i < nbrec_ls->n_load_balancer_group; i++) { + uuidset_insert(od_lb_data->lbgrps, + &nbrec_ls->load_balancer_group[i]->header_.uuid); + } } } @@ -426,7 +462,9 @@ 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); + od_lb_data->lbgrps = xzalloc(sizeof *od_lb_data->lbgrps); uuidset_init(od_lb_data->lbs); + uuidset_init(od_lb_data->lbgrps); hmap_insert(od_lb_map, &od_lb_data->hmap_node, uuid_hash(&od_lb_data->od_uuid)); @@ -451,7 +489,9 @@ static void destroy_od_lb_data(struct od_lb_data *od_lb_data) { uuidset_destroy(od_lb_data->lbs); + uuidset_destroy(od_lb_data->lbgrps); free(od_lb_data->lbs); + free(od_lb_data->lbgrps); free(od_lb_data); } @@ -462,6 +502,7 @@ destroy_tracked_data(struct ed_type_lb_data *lb_data) 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; + lb_data->tracked_lb_data.has_dissassoc_lbgrps_from_od = false; struct hmapx_node *node; HMAPX_FOR_EACH_SAFE (node, &lb_data->tracked_lb_data.deleted_lbs) { @@ -492,7 +533,7 @@ destroy_tracked_data(struct ed_type_lb_data *lb_data) &lb_data->tracked_lb_data.crupdated_ls_lbs) { ovs_list_remove(&codlb->list_node); uuidset_destroy(&codlb->assoc_lbs); - + uuidset_destroy(&codlb->assoc_lbgrps); free(codlb); } @@ -552,3 +593,10 @@ is_ls_lbs_changed(const struct nbrec_logical_switch *nbs) { || nbrec_logical_switch_is_updated(nbs, NBREC_LOGICAL_SWITCH_COL_LOAD_BALANCER)); } + +static bool +is_ls_lbgrps_changed(const struct nbrec_logical_switch *nbs) { + return ((nbrec_logical_switch_is_new(nbs) && nbs->n_load_balancer_group) + || nbrec_logical_switch_is_updated(nbs, + NBREC_LOGICAL_SWITCH_COL_LOAD_BALANCER_GROUP)); +} diff --git a/northd/en-lb-data.h b/northd/en-lb-data.h index 6b2ba11cb8..ebfd547d77 100644 --- a/northd/en-lb-data.h +++ b/northd/en-lb-data.h @@ -33,6 +33,7 @@ struct crupdated_od_lb_data { struct uuid od_uuid; struct uuidset assoc_lbs; + struct uuidset assoc_lbgrps; }; struct tracked_lb_data { @@ -66,6 +67,9 @@ struct tracked_lb_data { /* Indicates if a lb was disassociated from a logical switch. */ bool has_dissassoc_lbs_from_od; + + /* Indicates if a lb group was disassociated from a logical switch. */ + bool has_dissassoc_lbgrps_from_od; }; /* Datapath (logical switch) to lb/lbgrp association data. */ diff --git a/northd/northd.c b/northd/northd.c index 492d9b4f88..d500f940b1 100644 --- a/northd/northd.c +++ b/northd/northd.c @@ -5075,6 +5075,7 @@ ls_port_create(struct ovsdb_idl_txn *ovnsb_txn, struct hmap *ls_ports, * Presently supports i-p for the below changes: * - logical switch ports. * - load balancers. + * - load balancer groups. */ static bool ls_changes_can_be_handled( @@ -5086,7 +5087,8 @@ ls_changes_can_be_handled( 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_LOAD_BALANCER) { + col == NBREC_LOGICAL_SWITCH_COL_LOAD_BALANCER || + col == NBREC_LOGICAL_SWITCH_COL_LOAD_BALANCER_GROUP) { continue; } return false; @@ -5111,12 +5113,6 @@ ls_changes_can_be_handled( 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) { - return false; - } - } for (size_t i = 0; i < ls->n_qos_rules; i++) { if (nbrec_qos_row_get_seqno(ls->qos_rules[i], OVSDB_IDL_CHANGE_MODIFY) > 0) { @@ -5304,7 +5300,11 @@ fail: /* Return true if changes are handled incrementally, false otherwise. * When there are any changes, try to track what's exactly changed and set * northd_data->change_tracked accordingly: change tracked - true, otherwise, - * false. */ + * false. + * + * Note: Changes to load balancer and load balancer groups associated with + * the logical switches are handled separately in the lb_data change handlers. + * */ bool northd_handle_ls_changes(struct ovsdb_idl_txn *ovnsb_idl_txn, const struct northd_input *ni, @@ -5479,6 +5479,12 @@ northd_handle_lb_data_changes(struct tracked_lb_data *trk_lb_data, return false; } + /* Fall back to recompute if any load balancer group has been disassociated + * from a logical switch or router. */ + if (trk_lb_data->has_dissassoc_lbgrps_from_od) { + return false; + } + struct ovn_lb_datapaths *lb_dps; struct ovn_northd_lb *lb; struct ovn_datapath *od; @@ -5549,6 +5555,22 @@ northd_handle_lb_data_changes(struct tracked_lb_data *trk_lb_data, ovn_lb_datapaths_add_ls(lb_dps, 1, &od); } + UUIDSET_FOR_EACH (uuidnode, &codlb->assoc_lbgrps) { + lbgrp_dps = ovn_lb_group_datapaths_find(lbgrp_datapaths_map, + &uuidnode->uuid); + ovs_assert(lbgrp_dps); + ovn_lb_group_datapaths_add_ls(lbgrp_dps, 1, &od); + + /* Associate all the lbs of the lbgrp to the datapath 'od' */ + for (size_t j = 0; j < lbgrp_dps->lb_group->n_lbs; j++) { + const struct uuid *lb_uuid + = &lbgrp_dps->lb_group->lbs[j]->nlb->header_.uuid; + lb_dps = ovn_lb_datapaths_find(lb_datapaths_map, lb_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); } @@ -5568,6 +5590,31 @@ northd_handle_lb_data_changes(struct tracked_lb_data *trk_lb_data, } } + HMAP_FOR_EACH (crupdated_lbgrp, hmap_node, + &trk_lb_data->crupdated_lbgrps) { + lbgrp = crupdated_lbgrp->lbgrp; + const struct uuid *lb_uuid = &lbgrp->uuid; + + lbgrp_dps = ovn_lb_group_datapaths_find(lbgrp_datapaths_map, + lb_uuid); + ovs_assert(lbgrp_dps); + + struct hmapx_node *hnode; + HMAPX_FOR_EACH (hnode, &crupdated_lbgrp->assoc_lbs) { + lb = hnode->data; + lb_uuid = &lb->nlb->header_.uuid; + lb_dps = ovn_lb_datapaths_find(lb_datapaths_map, lb_uuid); + ovs_assert(lb_dps); + for (size_t i = 0; i < lbgrp_dps->n_ls; i++) { + od = lbgrp_dps->ls[i]; + ovn_lb_datapaths_add_ls(lb_dps, 1, &od); + + /* Re-evaluate 'od->has_lb_vip' */ + init_lb_for_datapath(od); + } + } + } + return true; } diff --git a/tests/ovn-northd.at b/tests/ovn-northd.at index f0f8d1f503..34e10663d0 100644 --- a/tests/ovn-northd.at +++ b/tests/ovn-northd.at @@ -2191,6 +2191,18 @@ check ovn-nbctl --wait=sb sync AT_CHECK([ovn-sbctl lflow-list | grep "ls_out_pre_lb.*priority=100" | grep reg0 | sort], [0], [dnl ]) +# Now associate vip again to lb4 and then delete it. +check ovn-nbctl set load_balancer $lb4 vips:"10.0.0.13"="10.0.0.6" +check ovn-nbctl --wait=sb sync +AT_CHECK([ovn-sbctl lflow-list | grep "ls_out_pre_lb.*priority=100" | grep reg0 | sort], [0], [dnl + table=1 (ls_out_pre_lb ), priority=100 , match=(ip), action=(reg0[[2]] = 1; next;) +]) + +check ovn-nbctl lb-del $lb4 +check ovn-nbctl --wait=sb sync +AT_CHECK([ovn-sbctl lflow-list | grep "ls_out_pre_lb.*priority=100" | grep reg0 | sort], [0], [dnl +]) + AT_CLEANUP ]) @@ -10553,21 +10565,21 @@ 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 compute -check_engine_stats northd recompute nocompute +check_engine_stats northd norecompute compute check_engine_stats lflow recompute nocompute -# Update lb and this should result in recompute +# Update lb and this should not result in northd recompute check as northd ovn-appctl -t NORTHD_TYPE inc-engine/clear-stats check ovn-nbctl --wait=sb set load_balancer . options:bar=foo check_engine_stats lb_data norecompute compute -check_engine_stats northd recompute nocompute +check_engine_stats northd norecompute compute check_engine_stats lflow recompute nocompute # 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 @@ -10575,7 +10587,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 @@ -10583,7 +10595,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 @@ -10591,7 +10603,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.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 @@ -10650,7 +10662,7 @@ check_engine_stats lflow recompute nocompute 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 compute -check_engine_stats northd recompute nocompute +check_engine_stats northd norecompute compute check_engine_stats lflow recompute nocompute check as northd ovn-appctl -t NORTHD_TYPE inc-engine/clear-stats @@ -10691,7 +10703,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 compute -check_engine_stats northd recompute nocompute +check_engine_stats northd norecompute compute check_engine_stats lflow recompute nocompute CHECK_NO_CHANGE_AFTER_RECOMPUTE -- 2.41.0 _______________________________________________ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev