| Message ID | 20230707055430.961782-1-numans@ovn.org |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | northd: I-P for load balancer and lb groups | expand |
| Context | Check | Description |
|---|---|---|
| ovsrobot/apply-robot | success | apply and check: success |
| ovsrobot/github-robot-_Build_and_Test | success | github build: passed |
| ovsrobot/github-robot-_ovn-kubernetes | success | github build: passed |
On Fri, Jul 7, 2023 at 11:25 AM <numans@ovn.org> wrote: > > From: Numan Siddique <numans@ovn.org> > > For every 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. > > Signed-off-by: Numan Siddique <numans@ovn.org> FYI to the reviewers. I've an issue with this patch. When a load balancer group is destroyed, then ovn-northd is not clearing up the nb_ls_map of load balancers associated with the load balancer group. This results in the load balancer logical flows still intact until northd engine recompute is triggered. I'll fix this issue in v3. Thanks Numan > --- > lib/lb.c | 45 +++++++++++++++--- > lib/lb.h | 33 ++++++-------- > northd/northd.c | 109 ++++++++++++++++++++++++++++++++++++++------ > tests/ovn-northd.at | 6 +-- > 4 files changed, 150 insertions(+), 43 deletions(-) > > diff --git a/lib/lb.c b/lib/lb.c > index b2b6473c1d..a0426cc42e 100644 > --- a/lib/lb.c > +++ b/lib/lb.c > @@ -1100,7 +1100,7 @@ ovn_lb_datapaths_add_ls(struct ovn_lb_datapaths *lb_dps, size_t n, > > void > ovn_lb_datapaths_remove_ls(struct ovn_lb_datapaths *lb_dps, size_t n, > - struct ovn_datapath **ods) > + struct ovn_datapath **ods) > { > for (size_t i = 0; i < n; i++) { > if (bitmap_is_set(lb_dps->nb_ls_map, ods[i]->index)) { > @@ -1112,14 +1112,14 @@ ovn_lb_datapaths_remove_ls(struct ovn_lb_datapaths *lb_dps, size_t n, > > struct ovn_lb_group_datapaths * > ovn_lb_group_datapaths_create(const struct ovn_lb_group *lb_group, > - size_t max_ls_datapaths, > - size_t max_lr_datapaths) > + size_t n_ls_datapaths, > + size_t n_lr_datapaths) > { > struct ovn_lb_group_datapaths *lb_group_dps = > xzalloc(sizeof *lb_group_dps); > lb_group_dps->lb_group = lb_group; > - lb_group_dps->ls = xmalloc(max_ls_datapaths * sizeof *lb_group_dps->ls); > - lb_group_dps->lr = xmalloc(max_lr_datapaths * sizeof *lb_group_dps->lr); > + lb_group_dps->nb_ls_map = bitmap_allocate(n_ls_datapaths); > + lb_group_dps->nb_lr_map = bitmap_allocate(n_lr_datapaths); > > return lb_group_dps; > } > @@ -1127,8 +1127,8 @@ ovn_lb_group_datapaths_create(const struct ovn_lb_group *lb_group, > void > ovn_lb_group_datapaths_destroy(struct ovn_lb_group_datapaths *lb_group_dps) > { > - free(lb_group_dps->ls); > - free(lb_group_dps->lr); > + bitmap_free(lb_group_dps->nb_lr_map); > + bitmap_free(lb_group_dps->nb_ls_map); > free(lb_group_dps); > } > > @@ -1146,3 +1146,34 @@ ovn_lb_group_datapaths_find(const struct hmap *lb_group_dps_map, > } > return NULL; > } > + > +void > +ovn_lb_group_datapaths_add_ls(struct ovn_lb_group_datapaths *lbg_dps, size_t n, > + struct ovn_datapath **ods) > +{ > + for (size_t i = 0; i < n; i++) { > + if (!bitmap_is_set(lbg_dps->nb_ls_map, ods[i]->index)) { > + bitmap_set1(lbg_dps->nb_ls_map, ods[i]->index); > + lbg_dps->n_nb_ls++; > + } > + } > +} > + > +void > +ovn_lb_group_datapaths_remove_ls(struct ovn_lb_group_datapaths *lbg_dps, > + size_t n, struct ovn_datapath **ods) > +{ > + for (size_t i = 0; i < n; i++) { > + if (bitmap_is_set(lbg_dps->nb_ls_map, ods[i]->index)) { > + bitmap_set0(lbg_dps->nb_ls_map, ods[i]->index); > + lbg_dps->n_nb_ls--; > + } > + } > +} > + > +void > +ovn_lb_group_datapaths_add_lr(struct ovn_lb_group_datapaths *lbg_dps, > + struct ovn_datapath *lr) > +{ > + bitmap_set1(lbg_dps->nb_lr_map, lr->index); > +} > diff --git a/lib/lb.h b/lib/lb.h > index ac33333a7f..08723e8ef3 100644 > --- a/lib/lb.h > +++ b/lib/lb.h > @@ -19,6 +19,7 @@ > > #include <sys/types.h> > #include <netinet/in.h> > +#include "lib/bitmap.h" > #include "lib/smap.h" > #include "openvswitch/hmap.h" > #include "ovn-util.h" > @@ -179,6 +180,9 @@ void ovn_lb_datapaths_add_lr(struct ovn_lb_datapaths *, size_t n, > struct ovn_datapath **); > void ovn_lb_datapaths_add_ls(struct ovn_lb_datapaths *, size_t n, > struct ovn_datapath **); > +void ovn_lb_datapaths_set_ls(struct ovn_lb_datapaths *, size_t n, > + struct ovn_datapath **); > + > void ovn_lb_datapaths_remove_ls(struct ovn_lb_datapaths *, size_t n, > struct ovn_datapath **); > > @@ -188,10 +192,11 @@ struct ovn_lb_group_datapaths { > const struct ovn_lb_group *lb_group; > > /* Datapaths to which 'lb_group' is applied. */ > - size_t n_ls; > - struct ovn_datapath **ls; > - size_t n_lr; > - struct ovn_datapath **lr; > + size_t n_nb_ls; > + unsigned long *nb_ls_map; > + > + size_t n_nb_lr; > + unsigned long *nb_lr_map; > }; > > struct ovn_lb_group_datapaths *ovn_lb_group_datapaths_create( > @@ -202,21 +207,13 @@ void ovn_lb_group_datapaths_destroy(struct ovn_lb_group_datapaths *); > struct ovn_lb_group_datapaths *ovn_lb_group_datapaths_find( > const struct hmap *lb_group_dps, const struct uuid *); > > -static inline void > -ovn_lb_group_datapaths_add_ls(struct ovn_lb_group_datapaths *lbg_dps, size_t n, > - struct ovn_datapath **ods) > -{ > - memcpy(&lbg_dps->ls[lbg_dps->n_ls], ods, n * sizeof *ods); > - lbg_dps->n_ls += n; > -} > - > -static inline void > -ovn_lb_group_datapaths_add_lr(struct ovn_lb_group_datapaths *lbg_dps, > - struct ovn_datapath *lr) > -{ > - lbg_dps->lr[lbg_dps->n_lr++] = lr; > -} > +void ovn_lb_group_datapaths_add_ls(struct ovn_lb_group_datapaths *, size_t n, > + struct ovn_datapath **); > +void ovn_lb_group_datapaths_remove_ls(struct ovn_lb_group_datapaths *, > + size_t n, struct ovn_datapath **); > > +void ovn_lb_group_datapaths_add_lr(struct ovn_lb_group_datapaths *, > + struct ovn_datapath *lr); > struct ovn_controller_lb { > struct hmap_node hmap_node; > > diff --git a/northd/northd.c b/northd/northd.c > index bf02190f7c..20a58afa6b 100644 > --- a/northd/northd.c > +++ b/northd/northd.c > @@ -4102,7 +4102,8 @@ associate_ls_lbs(struct ovn_datapath *ls_od, struct hmap *lb_datapaths_map) > > static void > associate_ls_lb_groups(struct ovn_datapath *ls_od, > - struct hmap *lb_group_datapaths_map) > + struct hmap *lb_group_datapaths_map, > + struct hmap *lb_datapaths_map) > { > const struct nbrec_load_balancer_group *nbrec_lb_group; > struct ovn_lb_group_datapaths *lb_group_dps; > @@ -4120,6 +4121,15 @@ associate_ls_lb_groups(struct ovn_datapath *ls_od, > ovs_assert(lb_group_dps); > ovn_lb_group_datapaths_add_ls(lb_group_dps, 1, &ls_od); > ls_od->lb_group_uuids[i] = *lb_group_uuid; > + > + struct ovn_lb_datapaths *lb_dps; > + for (size_t j = 0; j < lb_group_dps->lb_group->n_lbs; j++) { > + const struct uuid *lb_uuid = > + &lb_group_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, &ls_od); > + } > } > } > > @@ -4159,7 +4169,7 @@ build_lb_datapaths(const struct hmap *lbs, const struct hmap *lb_groups, > continue; > } > associate_ls_lbs(od, lb_datapaths_map); > - associate_ls_lb_groups(od, lb_group_datapaths_map); > + associate_ls_lb_groups(od, lb_group_datapaths_map, lb_datapaths_map); > } > > HMAP_FOR_EACH (od, key_node, &lr_datapaths->datapaths) { > @@ -4219,10 +4229,12 @@ build_lb_datapaths(const struct hmap *lbs, const struct hmap *lb_groups, > &lb_group_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, lb_group_dps->n_ls, > - lb_group_dps->ls); > - ovn_lb_datapaths_add_lr(lb_dps, lb_group_dps->n_lr, > - lb_group_dps->lr); > + size_t index; > + BITMAP_FOR_EACH_1 (index, ods_size(lr_datapaths), > + lb_group_dps->nb_lr_map) { > + od = lr_datapaths->array[index]; > + ovn_lb_datapaths_add_lr(lb_dps, 1, &od); > + } > } > } > } > @@ -4401,8 +4413,9 @@ build_lswitch_lbs_from_lrouter(struct ovn_datapaths *lr_datapaths, > > struct ovn_lb_group_datapaths *lb_group_dps; > HMAP_FOR_EACH (lb_group_dps, hmap_node, lb_group_dps_map) { > - for (size_t i = 0; i < lb_group_dps->n_lr; i++) { > - struct ovn_datapath *od = lb_group_dps->lr[i]; > + BITMAP_FOR_EACH_1 (index, ods_size(lr_datapaths), > + lb_group_dps->nb_lr_map) { > + struct ovn_datapath *od = lr_datapaths->array[index]; > ovn_lb_group_datapaths_add_ls(lb_group_dps, od->n_ls_peers, > od->ls_peers); > for (size_t j = 0; j < lb_group_dps->lb_group->n_lbs; j++) { > @@ -5072,7 +5085,8 @@ check_unsupported_inc_proc_for_ls_changes( > for (col = 0; col < NBREC_LOGICAL_SWITCH_N_COLUMNS; col++) { > if (nbrec_logical_switch_is_updated(ls, col)) { > if (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 true; > @@ -5103,12 +5117,6 @@ check_unsupported_inc_proc_for_ls_changes( > return true; > } > } > - 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 true; > - } > - } > 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) { > @@ -5338,6 +5346,68 @@ ls_check_and_handle_lb_changes(const struct nbrec_logical_switch *changed_ls, > return true; > } > > +static bool > +ls_check_and_handle_lb_group_changes( > + const struct nbrec_logical_switch *changed_ls, > + struct hmap *lb_group_datapaths_map, > + struct hmap *lb_datapaths_map, > + struct ovn_datapath *od, > + struct ls_change *ls_change, > + bool *updated) > +{ > + bool lbg_modified = false; > + /* Check if lb_groups changed or not */ > + if (!nbrec_logical_switch_is_updated(changed_ls, > + NBREC_LOGICAL_SWITCH_COL_LOAD_BALANCER_GROUP)) { > + /* load_balancer_group column of logical switch hasn't changed, but > + * its possible that a load balancer group may have changed (like > + * a load balancer added or removed from the group). */ > + for (size_t i = 0; i < changed_ls->n_load_balancer_group; i++) { > + if (nbrec_load_balancer_group_row_get_seqno( > + changed_ls->load_balancer_group[i], > + OVSDB_IDL_CHANGE_MODIFY) > 0) { > + lbg_modified = true; > + break; > + } > + } > + } else { > + /* load_balancer_group column of logical switch has changed. */ > + lbg_modified = true; > + } > + > + if (!lbg_modified) { > + /* Nothing changed */ > + return true; > + } > + > + /* Disassociate load balancer group (and its load balancers) from > + * the datapath and rebuild the association again. */ > + struct ovn_lb_group_datapaths *lbg_dps; > + for (size_t i = 0; i < od->n_lb_group_uuids; i++) { > + lbg_dps = ovn_lb_group_datapaths_find(lb_group_datapaths_map, > + &od->lb_group_uuids[i]); > + if (lbg_dps) { > + ovn_lb_group_datapaths_remove_ls(lbg_dps, 1, &od); > + > + struct ovn_lb_datapaths *lb_dps; > + for (size_t j = 0; j < lbg_dps->lb_group->n_lbs; j++) { > + const struct uuid *lb_uuid = > + &lbg_dps->lb_group->lbs[j]->nlb->header_.uuid; > + lb_dps = ovn_lb_datapaths_find(lb_datapaths_map, lb_uuid); > + if (lb_dps) { > + ovn_lb_datapaths_remove_ls(lb_dps, 1, &od); > + } > + } > + } > + } > + > + associate_ls_lb_groups(od, lb_group_datapaths_map, lb_datapaths_map); > + ls_change->lbs_changed = true; > + *updated = true; > + /* Re-evaluate 'od->has_lb_vip' */ > + init_lb_for_datapath(od); > + return true; > +} > > /* Return true if changes are handled incrementally, false otherwise. > * When there are any changes, try to track what's exactly changed and set > @@ -5396,6 +5466,15 @@ northd_handle_ls_changes(struct ovsdb_idl_txn *ovnsb_idl_txn, > goto fail; > } > > + if (!ls_check_and_handle_lb_group_changes(changed_ls, > + &nd->lb_group_datapaths_map, > + &nd->lb_datapaths_map, > + od, ls_change, &updated)) { > + destroy_tracked_ls_change(ls_change); > + free(ls_change); > + goto fail; > + } > + > if (updated) { > ovs_list_push_back(&nd->tracked_ls_changes.updated, > &ls_change->list_node); > diff --git a/tests/ovn-northd.at b/tests/ovn-northd.at > index d9f3917a3e..d3a5851e35 100644 > --- a/tests/ovn-northd.at > +++ b/tests/ovn-northd.at > @@ -9783,16 +9783,16 @@ check_engine_stats norecompute recompute > > check as northd ovn-appctl -t NORTHD_TYPE inc-engine/clear-stats > check ovn-nbctl add logical_switch sw0 load_balancer_group $lbg1_uuid > -check_engine_stats recompute recompute > +check_engine_stats norecompute recompute > > # Update lb and this should result in recompute > check as northd ovn-appctl -t NORTHD_TYPE inc-engine/clear-stats > check ovn-nbctl --wait=sb set load_balancer . options:bar=foo > -check_engine_stats recompute recompute > +check_engine_stats norecompute recompute > > check as northd ovn-appctl -t NORTHD_TYPE inc-engine/clear-stats > check ovn-nbctl clear logical_switch sw0 load_balancer_group > -check_engine_stats recompute recompute > +check_engine_stats norecompute recompute > > check as northd ovn-appctl -t NORTHD_TYPE inc-engine/clear-stats > check ovn-nbctl add logical_router lr0 load_balancer_group $lbg1_uuid > -- > 2.40.1 > > _______________________________________________ > dev mailing list > dev@openvswitch.org > https://mail.openvswitch.org/mailman/listinfo/ovs-dev >
On Fri, Jul 7, 2023 at 1:55 PM <numans@ovn.org> wrote: > > From: Numan Siddique <numans@ovn.org> > > For every 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. > > Signed-off-by: Numan Siddique <numans@ovn.org> > --- > lib/lb.c | 45 +++++++++++++++--- > lib/lb.h | 33 ++++++-------- > northd/northd.c | 109 ++++++++++++++++++++++++++++++++++++++------ > tests/ovn-northd.at | 6 +-- > 4 files changed, 150 insertions(+), 43 deletions(-) > > diff --git a/lib/lb.c b/lib/lb.c > index b2b6473c1d..a0426cc42e 100644 > --- a/lib/lb.c > +++ b/lib/lb.c > @@ -1100,7 +1100,7 @@ ovn_lb_datapaths_add_ls(struct ovn_lb_datapaths *lb_dps, size_t n, > > void > ovn_lb_datapaths_remove_ls(struct ovn_lb_datapaths *lb_dps, size_t n, > - struct ovn_datapath **ods) > + struct ovn_datapath **ods) > { > for (size_t i = 0; i < n; i++) { > if (bitmap_is_set(lb_dps->nb_ls_map, ods[i]->index)) { > @@ -1112,14 +1112,14 @@ ovn_lb_datapaths_remove_ls(struct ovn_lb_datapaths *lb_dps, size_t n, > > struct ovn_lb_group_datapaths * > ovn_lb_group_datapaths_create(const struct ovn_lb_group *lb_group, > - size_t max_ls_datapaths, > - size_t max_lr_datapaths) > + size_t n_ls_datapaths, > + size_t n_lr_datapaths) > { > struct ovn_lb_group_datapaths *lb_group_dps = > xzalloc(sizeof *lb_group_dps); > lb_group_dps->lb_group = lb_group; > - lb_group_dps->ls = xmalloc(max_ls_datapaths * sizeof *lb_group_dps->ls); > - lb_group_dps->lr = xmalloc(max_lr_datapaths * sizeof *lb_group_dps->lr); > + lb_group_dps->nb_ls_map = bitmap_allocate(n_ls_datapaths); > + lb_group_dps->nb_lr_map = bitmap_allocate(n_lr_datapaths); > > return lb_group_dps; > } > @@ -1127,8 +1127,8 @@ ovn_lb_group_datapaths_create(const struct ovn_lb_group *lb_group, > void > ovn_lb_group_datapaths_destroy(struct ovn_lb_group_datapaths *lb_group_dps) > { > - free(lb_group_dps->ls); > - free(lb_group_dps->lr); > + bitmap_free(lb_group_dps->nb_lr_map); > + bitmap_free(lb_group_dps->nb_ls_map); > free(lb_group_dps); > } > > @@ -1146,3 +1146,34 @@ ovn_lb_group_datapaths_find(const struct hmap *lb_group_dps_map, > } > return NULL; > } > + > +void > +ovn_lb_group_datapaths_add_ls(struct ovn_lb_group_datapaths *lbg_dps, size_t n, > + struct ovn_datapath **ods) > +{ > + for (size_t i = 0; i < n; i++) { > + if (!bitmap_is_set(lbg_dps->nb_ls_map, ods[i]->index)) { > + bitmap_set1(lbg_dps->nb_ls_map, ods[i]->index); > + lbg_dps->n_nb_ls++; > + } > + } > +} > + > +void > +ovn_lb_group_datapaths_remove_ls(struct ovn_lb_group_datapaths *lbg_dps, > + size_t n, struct ovn_datapath **ods) > +{ > + for (size_t i = 0; i < n; i++) { > + if (bitmap_is_set(lbg_dps->nb_ls_map, ods[i]->index)) { > + bitmap_set0(lbg_dps->nb_ls_map, ods[i]->index); > + lbg_dps->n_nb_ls--; > + } > + } > +} > + > +void > +ovn_lb_group_datapaths_add_lr(struct ovn_lb_group_datapaths *lbg_dps, > + struct ovn_datapath *lr) > +{ > + bitmap_set1(lbg_dps->nb_lr_map, lr->index); > +} > diff --git a/lib/lb.h b/lib/lb.h > index ac33333a7f..08723e8ef3 100644 > --- a/lib/lb.h > +++ b/lib/lb.h > @@ -19,6 +19,7 @@ > > #include <sys/types.h> > #include <netinet/in.h> > +#include "lib/bitmap.h" > #include "lib/smap.h" > #include "openvswitch/hmap.h" > #include "ovn-util.h" > @@ -179,6 +180,9 @@ void ovn_lb_datapaths_add_lr(struct ovn_lb_datapaths *, size_t n, > struct ovn_datapath **); > void ovn_lb_datapaths_add_ls(struct ovn_lb_datapaths *, size_t n, > struct ovn_datapath **); > +void ovn_lb_datapaths_set_ls(struct ovn_lb_datapaths *, size_t n, > + struct ovn_datapath **); > + > void ovn_lb_datapaths_remove_ls(struct ovn_lb_datapaths *, size_t n, > struct ovn_datapath **); > > @@ -188,10 +192,11 @@ struct ovn_lb_group_datapaths { > const struct ovn_lb_group *lb_group; > > /* Datapaths to which 'lb_group' is applied. */ > - size_t n_ls; > - struct ovn_datapath **ls; > - size_t n_lr; > - struct ovn_datapath **lr; > + size_t n_nb_ls; > + unsigned long *nb_ls_map; > + > + size_t n_nb_lr; > + unsigned long *nb_lr_map; > }; > > struct ovn_lb_group_datapaths *ovn_lb_group_datapaths_create( > @@ -202,21 +207,13 @@ void ovn_lb_group_datapaths_destroy(struct ovn_lb_group_datapaths *); > struct ovn_lb_group_datapaths *ovn_lb_group_datapaths_find( > const struct hmap *lb_group_dps, const struct uuid *); > > -static inline void > -ovn_lb_group_datapaths_add_ls(struct ovn_lb_group_datapaths *lbg_dps, size_t n, > - struct ovn_datapath **ods) > -{ > - memcpy(&lbg_dps->ls[lbg_dps->n_ls], ods, n * sizeof *ods); > - lbg_dps->n_ls += n; > -} > - This was O(1), but this patch changed it to O(n). Maybe we need some scale test data to see the impact of recompute performance until we are confident that recompute won't be triggered in most cases. I remember this "memcpy" approach was for a significant optimization from Ilya. cc @Ilya Maximets <i.maximets@ovn.org> in case he has more comments. Thanks, Han > -static inline void > -ovn_lb_group_datapaths_add_lr(struct ovn_lb_group_datapaths *lbg_dps, > - struct ovn_datapath *lr) > -{ > - lbg_dps->lr[lbg_dps->n_lr++] = lr; > -} > +void ovn_lb_group_datapaths_add_ls(struct ovn_lb_group_datapaths *, size_t n, > + struct ovn_datapath **); > +void ovn_lb_group_datapaths_remove_ls(struct ovn_lb_group_datapaths *, > + size_t n, struct ovn_datapath **); > > +void ovn_lb_group_datapaths_add_lr(struct ovn_lb_group_datapaths *, > + struct ovn_datapath *lr); > struct ovn_controller_lb { > struct hmap_node hmap_node; > > diff --git a/northd/northd.c b/northd/northd.c > index bf02190f7c..20a58afa6b 100644 > --- a/northd/northd.c > +++ b/northd/northd.c > @@ -4102,7 +4102,8 @@ associate_ls_lbs(struct ovn_datapath *ls_od, struct hmap *lb_datapaths_map) > > static void > associate_ls_lb_groups(struct ovn_datapath *ls_od, > - struct hmap *lb_group_datapaths_map) > + struct hmap *lb_group_datapaths_map, > + struct hmap *lb_datapaths_map) > { > const struct nbrec_load_balancer_group *nbrec_lb_group; > struct ovn_lb_group_datapaths *lb_group_dps; > @@ -4120,6 +4121,15 @@ associate_ls_lb_groups(struct ovn_datapath *ls_od, > ovs_assert(lb_group_dps); > ovn_lb_group_datapaths_add_ls(lb_group_dps, 1, &ls_od); > ls_od->lb_group_uuids[i] = *lb_group_uuid; > + > + struct ovn_lb_datapaths *lb_dps; > + for (size_t j = 0; j < lb_group_dps->lb_group->n_lbs; j++) { > + const struct uuid *lb_uuid = > + &lb_group_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, &ls_od); > + } > } > } > > @@ -4159,7 +4169,7 @@ build_lb_datapaths(const struct hmap *lbs, const struct hmap *lb_groups, > continue; > } > associate_ls_lbs(od, lb_datapaths_map); > - associate_ls_lb_groups(od, lb_group_datapaths_map); > + associate_ls_lb_groups(od, lb_group_datapaths_map, lb_datapaths_map); > } > > HMAP_FOR_EACH (od, key_node, &lr_datapaths->datapaths) { > @@ -4219,10 +4229,12 @@ build_lb_datapaths(const struct hmap *lbs, const struct hmap *lb_groups, > &lb_group_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, lb_group_dps->n_ls, > - lb_group_dps->ls); > - ovn_lb_datapaths_add_lr(lb_dps, lb_group_dps->n_lr, > - lb_group_dps->lr); > + size_t index; > + BITMAP_FOR_EACH_1 (index, ods_size(lr_datapaths), > + lb_group_dps->nb_lr_map) { > + od = lr_datapaths->array[index]; > + ovn_lb_datapaths_add_lr(lb_dps, 1, &od); > + } > } > } > } > @@ -4401,8 +4413,9 @@ build_lswitch_lbs_from_lrouter(struct ovn_datapaths *lr_datapaths, > > struct ovn_lb_group_datapaths *lb_group_dps; > HMAP_FOR_EACH (lb_group_dps, hmap_node, lb_group_dps_map) { > - for (size_t i = 0; i < lb_group_dps->n_lr; i++) { > - struct ovn_datapath *od = lb_group_dps->lr[i]; > + BITMAP_FOR_EACH_1 (index, ods_size(lr_datapaths), > + lb_group_dps->nb_lr_map) { > + struct ovn_datapath *od = lr_datapaths->array[index]; > ovn_lb_group_datapaths_add_ls(lb_group_dps, od->n_ls_peers, > od->ls_peers); > for (size_t j = 0; j < lb_group_dps->lb_group->n_lbs; j++) { > @@ -5072,7 +5085,8 @@ check_unsupported_inc_proc_for_ls_changes( > for (col = 0; col < NBREC_LOGICAL_SWITCH_N_COLUMNS; col++) { > if (nbrec_logical_switch_is_updated(ls, col)) { > if (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 true; > @@ -5103,12 +5117,6 @@ check_unsupported_inc_proc_for_ls_changes( > return true; > } > } > - 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 true; > - } > - } > 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) { > @@ -5338,6 +5346,68 @@ ls_check_and_handle_lb_changes(const struct nbrec_logical_switch *changed_ls, > return true; > } > > +static bool > +ls_check_and_handle_lb_group_changes( > + const struct nbrec_logical_switch *changed_ls, > + struct hmap *lb_group_datapaths_map, > + struct hmap *lb_datapaths_map, > + struct ovn_datapath *od, > + struct ls_change *ls_change, > + bool *updated) > +{ > + bool lbg_modified = false; > + /* Check if lb_groups changed or not */ > + if (!nbrec_logical_switch_is_updated(changed_ls, > + NBREC_LOGICAL_SWITCH_COL_LOAD_BALANCER_GROUP)) { > + /* load_balancer_group column of logical switch hasn't changed, but > + * its possible that a load balancer group may have changed (like > + * a load balancer added or removed from the group). */ > + for (size_t i = 0; i < changed_ls->n_load_balancer_group; i++) { > + if (nbrec_load_balancer_group_row_get_seqno( > + changed_ls->load_balancer_group[i], > + OVSDB_IDL_CHANGE_MODIFY) > 0) { > + lbg_modified = true; > + break; > + } > + } > + } else { > + /* load_balancer_group column of logical switch has changed. */ > + lbg_modified = true; > + } > + > + if (!lbg_modified) { > + /* Nothing changed */ > + return true; > + } > + > + /* Disassociate load balancer group (and its load balancers) from > + * the datapath and rebuild the association again. */ > + struct ovn_lb_group_datapaths *lbg_dps; > + for (size_t i = 0; i < od->n_lb_group_uuids; i++) { > + lbg_dps = ovn_lb_group_datapaths_find(lb_group_datapaths_map, > + &od->lb_group_uuids[i]); > + if (lbg_dps) { > + ovn_lb_group_datapaths_remove_ls(lbg_dps, 1, &od); > + > + struct ovn_lb_datapaths *lb_dps; > + for (size_t j = 0; j < lbg_dps->lb_group->n_lbs; j++) { > + const struct uuid *lb_uuid = > + &lbg_dps->lb_group->lbs[j]->nlb->header_.uuid; > + lb_dps = ovn_lb_datapaths_find(lb_datapaths_map, lb_uuid); > + if (lb_dps) { > + ovn_lb_datapaths_remove_ls(lb_dps, 1, &od); > + } > + } > + } > + } > + > + associate_ls_lb_groups(od, lb_group_datapaths_map, lb_datapaths_map); > + ls_change->lbs_changed = true; > + *updated = true; > + /* Re-evaluate 'od->has_lb_vip' */ > + init_lb_for_datapath(od); > + return true; > +} > > /* Return true if changes are handled incrementally, false otherwise. > * When there are any changes, try to track what's exactly changed and set > @@ -5396,6 +5466,15 @@ northd_handle_ls_changes(struct ovsdb_idl_txn *ovnsb_idl_txn, > goto fail; > } > > + if (!ls_check_and_handle_lb_group_changes(changed_ls, > + &nd->lb_group_datapaths_map, > + &nd->lb_datapaths_map, > + od, ls_change, &updated)) { > + destroy_tracked_ls_change(ls_change); > + free(ls_change); > + goto fail; > + } > + > if (updated) { > ovs_list_push_back(&nd->tracked_ls_changes.updated, > &ls_change->list_node); > diff --git a/tests/ovn-northd.at b/tests/ovn-northd.at > index d9f3917a3e..d3a5851e35 100644 > --- a/tests/ovn-northd.at > +++ b/tests/ovn-northd.at > @@ -9783,16 +9783,16 @@ check_engine_stats norecompute recompute > > check as northd ovn-appctl -t NORTHD_TYPE inc-engine/clear-stats > check ovn-nbctl add logical_switch sw0 load_balancer_group $lbg1_uuid > -check_engine_stats recompute recompute > +check_engine_stats norecompute recompute > > # Update lb and this should result in recompute > check as northd ovn-appctl -t NORTHD_TYPE inc-engine/clear-stats > check ovn-nbctl --wait=sb set load_balancer . options:bar=foo > -check_engine_stats recompute recompute > +check_engine_stats norecompute recompute > > check as northd ovn-appctl -t NORTHD_TYPE inc-engine/clear-stats > check ovn-nbctl clear logical_switch sw0 load_balancer_group > -check_engine_stats recompute recompute > +check_engine_stats norecompute recompute > > check as northd ovn-appctl -t NORTHD_TYPE inc-engine/clear-stats > check ovn-nbctl add logical_router lr0 load_balancer_group $lbg1_uuid > -- > 2.40.1 > > _______________________________________________ > dev mailing list > dev@openvswitch.org > https://mail.openvswitch.org/mailman/listinfo/ovs-dev
On 7/18/23 05:32, Han Zhou wrote: > > > On Fri, Jul 7, 2023 at 1:55 PM <numans@ovn.org <mailto:numans@ovn.org>> wrote: >> >> From: Numan Siddique <numans@ovn.org <mailto:numans@ovn.org>> >> >> For every 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. >> >> Signed-off-by: Numan Siddique <numans@ovn.org <mailto:numans@ovn.org>> >> --- >> lib/lb.c | 45 +++++++++++++++--- >> lib/lb.h | 33 ++++++-------- >> northd/northd.c | 109 ++++++++++++++++++++++++++++++++++++++------ >> tests/ovn-northd.at <http://ovn-northd.at> | 6 +-- >> 4 files changed, 150 insertions(+), 43 deletions(-) >> >> diff --git a/lib/lb.c b/lib/lb.c >> index b2b6473c1d..a0426cc42e 100644 >> --- a/lib/lb.c >> +++ b/lib/lb.c >> @@ -1100,7 +1100,7 @@ ovn_lb_datapaths_add_ls(struct ovn_lb_datapaths *lb_dps, size_t n, >> >> void >> ovn_lb_datapaths_remove_ls(struct ovn_lb_datapaths *lb_dps, size_t n, >> - struct ovn_datapath **ods) >> + struct ovn_datapath **ods) >> { >> for (size_t i = 0; i < n; i++) { >> if (bitmap_is_set(lb_dps->nb_ls_map, ods[i]->index)) { >> @@ -1112,14 +1112,14 @@ ovn_lb_datapaths_remove_ls(struct ovn_lb_datapaths *lb_dps, size_t n, >> >> struct ovn_lb_group_datapaths * >> ovn_lb_group_datapaths_create(const struct ovn_lb_group *lb_group, >> - size_t max_ls_datapaths, >> - size_t max_lr_datapaths) >> + size_t n_ls_datapaths, >> + size_t n_lr_datapaths) >> { >> struct ovn_lb_group_datapaths *lb_group_dps = >> xzalloc(sizeof *lb_group_dps); >> lb_group_dps->lb_group = lb_group; >> - lb_group_dps->ls = xmalloc(max_ls_datapaths * sizeof *lb_group_dps->ls); >> - lb_group_dps->lr = xmalloc(max_lr_datapaths * sizeof *lb_group_dps->lr); >> + lb_group_dps->nb_ls_map = bitmap_allocate(n_ls_datapaths); >> + lb_group_dps->nb_lr_map = bitmap_allocate(n_lr_datapaths); >> >> return lb_group_dps; >> } >> @@ -1127,8 +1127,8 @@ ovn_lb_group_datapaths_create(const struct ovn_lb_group *lb_group, >> void >> ovn_lb_group_datapaths_destroy(struct ovn_lb_group_datapaths *lb_group_dps) >> { >> - free(lb_group_dps->ls); >> - free(lb_group_dps->lr); >> + bitmap_free(lb_group_dps->nb_lr_map); >> + bitmap_free(lb_group_dps->nb_ls_map); >> free(lb_group_dps); >> } >> >> @@ -1146,3 +1146,34 @@ ovn_lb_group_datapaths_find(const struct hmap *lb_group_dps_map, >> } >> return NULL; >> } >> + >> +void >> +ovn_lb_group_datapaths_add_ls(struct ovn_lb_group_datapaths *lbg_dps, size_t n, >> + struct ovn_datapath **ods) >> +{ >> + for (size_t i = 0; i < n; i++) { >> + if (!bitmap_is_set(lbg_dps->nb_ls_map, ods[i]->index)) { >> + bitmap_set1(lbg_dps->nb_ls_map, ods[i]->index); >> + lbg_dps->n_nb_ls++; >> + } >> + } >> +} >> + >> +void >> +ovn_lb_group_datapaths_remove_ls(struct ovn_lb_group_datapaths *lbg_dps, >> + size_t n, struct ovn_datapath **ods) >> +{ >> + for (size_t i = 0; i < n; i++) { >> + if (bitmap_is_set(lbg_dps->nb_ls_map, ods[i]->index)) { >> + bitmap_set0(lbg_dps->nb_ls_map, ods[i]->index); >> + lbg_dps->n_nb_ls--; >> + } >> + } >> +} >> + >> +void >> +ovn_lb_group_datapaths_add_lr(struct ovn_lb_group_datapaths *lbg_dps, >> + struct ovn_datapath *lr) >> +{ >> + bitmap_set1(lbg_dps->nb_lr_map, lr->index); >> +} >> diff --git a/lib/lb.h b/lib/lb.h >> index ac33333a7f..08723e8ef3 100644 >> --- a/lib/lb.h >> +++ b/lib/lb.h >> @@ -19,6 +19,7 @@ >> >> #include <sys/types.h> >> #include <netinet/in.h> >> +#include "lib/bitmap.h" >> #include "lib/smap.h" >> #include "openvswitch/hmap.h" >> #include "ovn-util.h" >> @@ -179,6 +180,9 @@ void ovn_lb_datapaths_add_lr(struct ovn_lb_datapaths *, size_t n, >> struct ovn_datapath **); >> void ovn_lb_datapaths_add_ls(struct ovn_lb_datapaths *, size_t n, >> struct ovn_datapath **); >> +void ovn_lb_datapaths_set_ls(struct ovn_lb_datapaths *, size_t n, >> + struct ovn_datapath **); >> + >> void ovn_lb_datapaths_remove_ls(struct ovn_lb_datapaths *, size_t n, >> struct ovn_datapath **); >> >> @@ -188,10 +192,11 @@ struct ovn_lb_group_datapaths { >> const struct ovn_lb_group *lb_group; >> >> /* Datapaths to which 'lb_group' is applied. */ >> - size_t n_ls; >> - struct ovn_datapath **ls; >> - size_t n_lr; >> - struct ovn_datapath **lr; >> + size_t n_nb_ls; >> + unsigned long *nb_ls_map; >> + >> + size_t n_nb_lr; >> + unsigned long *nb_lr_map; >> }; >> >> struct ovn_lb_group_datapaths *ovn_lb_group_datapaths_create( >> @@ -202,21 +207,13 @@ void ovn_lb_group_datapaths_destroy(struct ovn_lb_group_datapaths *); >> struct ovn_lb_group_datapaths *ovn_lb_group_datapaths_find( >> const struct hmap *lb_group_dps, const struct uuid *); >> >> -static inline void >> -ovn_lb_group_datapaths_add_ls(struct ovn_lb_group_datapaths *lbg_dps, size_t n, >> - struct ovn_datapath **ods) >> -{ >> - memcpy(&lbg_dps->ls[lbg_dps->n_ls], ods, n * sizeof *ods); >> - lbg_dps->n_ls += n; >> -} >> - > > This was O(1), but this patch changed it to O(n). Maybe we need some scale test data > to see the impact of recompute performance until we are confident that recompute > won't be triggered in most cases. I remember this "memcpy" approach was for a significant > optimization from Ilya. cc @Ilya Maximets <mailto:i.maximets@ovn.org> in case he has > more comments. Thanks, Han, for pointing out. It is hard to tell what the impact would be without testing as there are way too many factors, as usual with performance testing. So, I fired up 500 node density-heavy test with ovn-heater. Results are not great. I see average long poll intervals on northd went up to 6.6 seconds from previous 2.6 seconds. 95%% went up to 9.8 seconds with the patch set applied from 3.9 seconds on current main. Here is an example iteration log during the test: 2023-07-18T12:34:37.147Z|01264|inc_proc_eng|INFO|node: northd, handler for input NB_logical_switch took 2479ms 2023-07-18T12:34:40.633Z|01265|inc_proc_eng|INFO|node: northd, handler for input NB_logical_router took 3486ms 2023-07-18T12:34:41.976Z|01266|inc_proc_eng|INFO|node: lflow, recompute (failed handler for input northd) took 962ms 2023-07-18T12:34:41.977Z|01267|timeval|WARN|Unreasonably long 7362ms poll interval (6604ms user, 381ms system) We can see that northd node is processed incrementally, but very slow. In comparison, an average iteration looks like this on the current main: 2023-07-15T19:55:02.234Z|00456|inc_proc_eng|INFO|node: northd, recompute (missing handler for input NB_load_balancer) took 1829ms 2023-07-15T19:55:03.237Z|00457|inc_proc_eng|INFO|node: lflow, recompute (failed handler for input northd) took 979ms 2023-07-15T19:55:03.239Z|00458|timeval|WARN|Unreasonably long 2868ms poll interval (2314ms user, 130ms system) We can see that a full recompute of the northd node on current main is much faster than either of the I-P handlers. perf somewhat points to the functions above to be hot spots, but the datapath lookup dominates the chart: 29.39% ovn-northd [.] ovn_lb_datapaths_find 8.38% ovn-northd [.] ovn_lb_datapaths_add_lr 6.72% ovn-northd [.] ovn_lb_datapaths_remove_ls 6.40% ovn-northd [.] ovn_lb_datapaths_add_ls 6.19% ovn-northd [.] ovn_lb_datapaths_remove_lr 3.86% ovn-northd [.] northd_handle_lr_changes The ovn-installed latency in the test went up from average 3.3 seconds to 6.4 seconds. All in all, incremental processing implemented in this patch set appears to be 2-3 times slower than a full recompute on current main. Best regards, Ilya Maximets.
On 7/18/23 15:17, Ilya Maximets wrote: > On 7/18/23 05:32, Han Zhou wrote: >> >> >> On Fri, Jul 7, 2023 at 1:55 PM <numans@ovn.org <mailto:numans@ovn.org>> wrote: >>> >>> From: Numan Siddique <numans@ovn.org <mailto:numans@ovn.org>> >>> >>> For every 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. >>> >>> Signed-off-by: Numan Siddique <numans@ovn.org <mailto:numans@ovn.org>> >>> --- >>> lib/lb.c | 45 +++++++++++++++--- >>> lib/lb.h | 33 ++++++-------- >>> northd/northd.c | 109 ++++++++++++++++++++++++++++++++++++++------ >>> tests/ovn-northd.at <http://ovn-northd.at> | 6 +-- >>> 4 files changed, 150 insertions(+), 43 deletions(-) >>> >>> diff --git a/lib/lb.c b/lib/lb.c >>> index b2b6473c1d..a0426cc42e 100644 >>> --- a/lib/lb.c >>> +++ b/lib/lb.c >>> @@ -1100,7 +1100,7 @@ ovn_lb_datapaths_add_ls(struct ovn_lb_datapaths *lb_dps, size_t n, >>> >>> void >>> ovn_lb_datapaths_remove_ls(struct ovn_lb_datapaths *lb_dps, size_t n, >>> - struct ovn_datapath **ods) >>> + struct ovn_datapath **ods) >>> { >>> for (size_t i = 0; i < n; i++) { >>> if (bitmap_is_set(lb_dps->nb_ls_map, ods[i]->index)) { >>> @@ -1112,14 +1112,14 @@ ovn_lb_datapaths_remove_ls(struct ovn_lb_datapaths *lb_dps, size_t n, >>> >>> struct ovn_lb_group_datapaths * >>> ovn_lb_group_datapaths_create(const struct ovn_lb_group *lb_group, >>> - size_t max_ls_datapaths, >>> - size_t max_lr_datapaths) >>> + size_t n_ls_datapaths, >>> + size_t n_lr_datapaths) >>> { >>> struct ovn_lb_group_datapaths *lb_group_dps = >>> xzalloc(sizeof *lb_group_dps); >>> lb_group_dps->lb_group = lb_group; >>> - lb_group_dps->ls = xmalloc(max_ls_datapaths * sizeof *lb_group_dps->ls); >>> - lb_group_dps->lr = xmalloc(max_lr_datapaths * sizeof *lb_group_dps->lr); >>> + lb_group_dps->nb_ls_map = bitmap_allocate(n_ls_datapaths); >>> + lb_group_dps->nb_lr_map = bitmap_allocate(n_lr_datapaths); >>> >>> return lb_group_dps; >>> } >>> @@ -1127,8 +1127,8 @@ ovn_lb_group_datapaths_create(const struct ovn_lb_group *lb_group, >>> void >>> ovn_lb_group_datapaths_destroy(struct ovn_lb_group_datapaths *lb_group_dps) >>> { >>> - free(lb_group_dps->ls); >>> - free(lb_group_dps->lr); >>> + bitmap_free(lb_group_dps->nb_lr_map); >>> + bitmap_free(lb_group_dps->nb_ls_map); >>> free(lb_group_dps); >>> } >>> >>> @@ -1146,3 +1146,34 @@ ovn_lb_group_datapaths_find(const struct hmap *lb_group_dps_map, >>> } >>> return NULL; >>> } >>> + >>> +void >>> +ovn_lb_group_datapaths_add_ls(struct ovn_lb_group_datapaths *lbg_dps, size_t n, >>> + struct ovn_datapath **ods) >>> +{ >>> + for (size_t i = 0; i < n; i++) { >>> + if (!bitmap_is_set(lbg_dps->nb_ls_map, ods[i]->index)) { >>> + bitmap_set1(lbg_dps->nb_ls_map, ods[i]->index); >>> + lbg_dps->n_nb_ls++; >>> + } >>> + } >>> +} >>> + >>> +void >>> +ovn_lb_group_datapaths_remove_ls(struct ovn_lb_group_datapaths *lbg_dps, >>> + size_t n, struct ovn_datapath **ods) >>> +{ >>> + for (size_t i = 0; i < n; i++) { >>> + if (bitmap_is_set(lbg_dps->nb_ls_map, ods[i]->index)) { >>> + bitmap_set0(lbg_dps->nb_ls_map, ods[i]->index); >>> + lbg_dps->n_nb_ls--; >>> + } >>> + } >>> +} >>> + >>> +void >>> +ovn_lb_group_datapaths_add_lr(struct ovn_lb_group_datapaths *lbg_dps, >>> + struct ovn_datapath *lr) >>> +{ >>> + bitmap_set1(lbg_dps->nb_lr_map, lr->index); >>> +} >>> diff --git a/lib/lb.h b/lib/lb.h >>> index ac33333a7f..08723e8ef3 100644 >>> --- a/lib/lb.h >>> +++ b/lib/lb.h >>> @@ -19,6 +19,7 @@ >>> >>> #include <sys/types.h> >>> #include <netinet/in.h> >>> +#include "lib/bitmap.h" >>> #include "lib/smap.h" >>> #include "openvswitch/hmap.h" >>> #include "ovn-util.h" >>> @@ -179,6 +180,9 @@ void ovn_lb_datapaths_add_lr(struct ovn_lb_datapaths *, size_t n, >>> struct ovn_datapath **); >>> void ovn_lb_datapaths_add_ls(struct ovn_lb_datapaths *, size_t n, >>> struct ovn_datapath **); >>> +void ovn_lb_datapaths_set_ls(struct ovn_lb_datapaths *, size_t n, >>> + struct ovn_datapath **); >>> + >>> void ovn_lb_datapaths_remove_ls(struct ovn_lb_datapaths *, size_t n, >>> struct ovn_datapath **); >>> >>> @@ -188,10 +192,11 @@ struct ovn_lb_group_datapaths { >>> const struct ovn_lb_group *lb_group; >>> >>> /* Datapaths to which 'lb_group' is applied. */ >>> - size_t n_ls; >>> - struct ovn_datapath **ls; >>> - size_t n_lr; >>> - struct ovn_datapath **lr; >>> + size_t n_nb_ls; >>> + unsigned long *nb_ls_map; >>> + >>> + size_t n_nb_lr; >>> + unsigned long *nb_lr_map; >>> }; >>> >>> struct ovn_lb_group_datapaths *ovn_lb_group_datapaths_create( >>> @@ -202,21 +207,13 @@ void ovn_lb_group_datapaths_destroy(struct ovn_lb_group_datapaths *); >>> struct ovn_lb_group_datapaths *ovn_lb_group_datapaths_find( >>> const struct hmap *lb_group_dps, const struct uuid *); >>> >>> -static inline void >>> -ovn_lb_group_datapaths_add_ls(struct ovn_lb_group_datapaths *lbg_dps, size_t n, >>> - struct ovn_datapath **ods) >>> -{ >>> - memcpy(&lbg_dps->ls[lbg_dps->n_ls], ods, n * sizeof *ods); >>> - lbg_dps->n_ls += n; >>> -} >>> - >> >> This was O(1), but this patch changed it to O(n). Maybe we need some scale test data >> to see the impact of recompute performance until we are confident that recompute >> won't be triggered in most cases. I remember this "memcpy" approach was for a significant >> optimization from Ilya. cc @Ilya Maximets <mailto:i.maximets@ovn.org> in case he has >> more comments. > > Thanks, Han, for pointing out. It is hard to tell what the impact would be > without testing as there are way too many factors, as usual with performance > testing. So, I fired up 500 node density-heavy test with ovn-heater. > > Results are not great. I see average long poll intervals on northd went up > to 6.6 seconds from previous 2.6 seconds. 95%% went up to 9.8 seconds with > the patch set applied from 3.9 seconds on current main. > > Here is an example iteration log during the test: > > 2023-07-18T12:34:37.147Z|01264|inc_proc_eng|INFO|node: northd, handler for input NB_logical_switch took 2479ms > 2023-07-18T12:34:40.633Z|01265|inc_proc_eng|INFO|node: northd, handler for input NB_logical_router took 3486ms > 2023-07-18T12:34:41.976Z|01266|inc_proc_eng|INFO|node: lflow, recompute (failed handler for input northd) took 962ms > 2023-07-18T12:34:41.977Z|01267|timeval|WARN|Unreasonably long 7362ms poll interval (6604ms user, 381ms system) > > We can see that northd node is processed incrementally, but very slow. > In comparison, an average iteration looks like this on the current main: > > 2023-07-15T19:55:02.234Z|00456|inc_proc_eng|INFO|node: northd, recompute (missing handler for input NB_load_balancer) took 1829ms > 2023-07-15T19:55:03.237Z|00457|inc_proc_eng|INFO|node: lflow, recompute (failed handler for input northd) took 979ms > 2023-07-15T19:55:03.239Z|00458|timeval|WARN|Unreasonably long 2868ms poll interval (2314ms user, 130ms system) > > We can see that a full recompute of the northd node on current main is much > faster than either of the I-P handlers. > > perf somewhat points to the functions above to be hot spots, but the > datapath lookup dominates the chart: > > 29.39% ovn-northd [.] ovn_lb_datapaths_find > 8.38% ovn-northd [.] ovn_lb_datapaths_add_lr > 6.72% ovn-northd [.] ovn_lb_datapaths_remove_ls > 6.40% ovn-northd [.] ovn_lb_datapaths_add_ls > 6.19% ovn-northd [.] ovn_lb_datapaths_remove_lr > 3.86% ovn-northd [.] northd_handle_lr_changes > > The ovn-installed latency in the test went up from average 3.3 seconds to > 6.4 seconds. > > All in all, incremental processing implemented in this patch set appears to > be 2-3 times slower than a full recompute on current main. I also checked the recompute time now. And it looks like a full recompute after applying the patch set is about 2x slower than on a current main. Interestingly enough, it means that with a patch set applied, the full recompute is a bit faster than I-P iterations. > > Best regards, Ilya Maximets
On Tue, Jul 18, 2023 at 9:39 PM Ilya Maximets <i.maximets@ovn.org> wrote: > > On 7/18/23 15:17, Ilya Maximets wrote: > > On 7/18/23 05:32, Han Zhou wrote: > >> > >> > >> On Fri, Jul 7, 2023 at 1:55 PM <numans@ovn.org <mailto:numans@ovn.org>> wrote: > >>> > >>> From: Numan Siddique <numans@ovn.org <mailto:numans@ovn.org>> > >>> > >>> For every 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. > >>> > >>> Signed-off-by: Numan Siddique <numans@ovn.org <mailto:numans@ovn.org>> > >>> --- > >>> lib/lb.c | 45 +++++++++++++++--- > >>> lib/lb.h | 33 ++++++-------- > >>> northd/northd.c | 109 ++++++++++++++++++++++++++++++++++++++------ > >>> tests/ovn-northd.at <http://ovn-northd.at> | 6 +-- > >>> 4 files changed, 150 insertions(+), 43 deletions(-) > >>> > >>> diff --git a/lib/lb.c b/lib/lb.c > >>> index b2b6473c1d..a0426cc42e 100644 > >>> --- a/lib/lb.c > >>> +++ b/lib/lb.c > >>> @@ -1100,7 +1100,7 @@ ovn_lb_datapaths_add_ls(struct ovn_lb_datapaths *lb_dps, size_t n, > >>> > >>> void > >>> ovn_lb_datapaths_remove_ls(struct ovn_lb_datapaths *lb_dps, size_t n, > >>> - struct ovn_datapath **ods) > >>> + struct ovn_datapath **ods) > >>> { > >>> for (size_t i = 0; i < n; i++) { > >>> if (bitmap_is_set(lb_dps->nb_ls_map, ods[i]->index)) { > >>> @@ -1112,14 +1112,14 @@ ovn_lb_datapaths_remove_ls(struct ovn_lb_datapaths *lb_dps, size_t n, > >>> > >>> struct ovn_lb_group_datapaths * > >>> ovn_lb_group_datapaths_create(const struct ovn_lb_group *lb_group, > >>> - size_t max_ls_datapaths, > >>> - size_t max_lr_datapaths) > >>> + size_t n_ls_datapaths, > >>> + size_t n_lr_datapaths) > >>> { > >>> struct ovn_lb_group_datapaths *lb_group_dps = > >>> xzalloc(sizeof *lb_group_dps); > >>> lb_group_dps->lb_group = lb_group; > >>> - lb_group_dps->ls = xmalloc(max_ls_datapaths * sizeof *lb_group_dps->ls); > >>> - lb_group_dps->lr = xmalloc(max_lr_datapaths * sizeof *lb_group_dps->lr); > >>> + lb_group_dps->nb_ls_map = bitmap_allocate(n_ls_datapaths); > >>> + lb_group_dps->nb_lr_map = bitmap_allocate(n_lr_datapaths); > >>> > >>> return lb_group_dps; > >>> } > >>> @@ -1127,8 +1127,8 @@ ovn_lb_group_datapaths_create(const struct ovn_lb_group *lb_group, > >>> void > >>> ovn_lb_group_datapaths_destroy(struct ovn_lb_group_datapaths *lb_group_dps) > >>> { > >>> - free(lb_group_dps->ls); > >>> - free(lb_group_dps->lr); > >>> + bitmap_free(lb_group_dps->nb_lr_map); > >>> + bitmap_free(lb_group_dps->nb_ls_map); > >>> free(lb_group_dps); > >>> } > >>> > >>> @@ -1146,3 +1146,34 @@ ovn_lb_group_datapaths_find(const struct hmap *lb_group_dps_map, > >>> } > >>> return NULL; > >>> } > >>> + > >>> +void > >>> +ovn_lb_group_datapaths_add_ls(struct ovn_lb_group_datapaths *lbg_dps, size_t n, > >>> + struct ovn_datapath **ods) > >>> +{ > >>> + for (size_t i = 0; i < n; i++) { > >>> + if (!bitmap_is_set(lbg_dps->nb_ls_map, ods[i]->index)) { > >>> + bitmap_set1(lbg_dps->nb_ls_map, ods[i]->index); > >>> + lbg_dps->n_nb_ls++; > >>> + } > >>> + } > >>> +} > >>> + > >>> +void > >>> +ovn_lb_group_datapaths_remove_ls(struct ovn_lb_group_datapaths *lbg_dps, > >>> + size_t n, struct ovn_datapath **ods) > >>> +{ > >>> + for (size_t i = 0; i < n; i++) { > >>> + if (bitmap_is_set(lbg_dps->nb_ls_map, ods[i]->index)) { > >>> + bitmap_set0(lbg_dps->nb_ls_map, ods[i]->index); > >>> + lbg_dps->n_nb_ls--; > >>> + } > >>> + } > >>> +} > >>> + > >>> +void > >>> +ovn_lb_group_datapaths_add_lr(struct ovn_lb_group_datapaths *lbg_dps, > >>> + struct ovn_datapath *lr) > >>> +{ > >>> + bitmap_set1(lbg_dps->nb_lr_map, lr->index); > >>> +} > >>> diff --git a/lib/lb.h b/lib/lb.h > >>> index ac33333a7f..08723e8ef3 100644 > >>> --- a/lib/lb.h > >>> +++ b/lib/lb.h > >>> @@ -19,6 +19,7 @@ > >>> > >>> #include <sys/types.h> > >>> #include <netinet/in.h> > >>> +#include "lib/bitmap.h" > >>> #include "lib/smap.h" > >>> #include "openvswitch/hmap.h" > >>> #include "ovn-util.h" > >>> @@ -179,6 +180,9 @@ void ovn_lb_datapaths_add_lr(struct ovn_lb_datapaths *, size_t n, > >>> struct ovn_datapath **); > >>> void ovn_lb_datapaths_add_ls(struct ovn_lb_datapaths *, size_t n, > >>> struct ovn_datapath **); > >>> +void ovn_lb_datapaths_set_ls(struct ovn_lb_datapaths *, size_t n, > >>> + struct ovn_datapath **); > >>> + > >>> void ovn_lb_datapaths_remove_ls(struct ovn_lb_datapaths *, size_t n, > >>> struct ovn_datapath **); > >>> > >>> @@ -188,10 +192,11 @@ struct ovn_lb_group_datapaths { > >>> const struct ovn_lb_group *lb_group; > >>> > >>> /* Datapaths to which 'lb_group' is applied. */ > >>> - size_t n_ls; > >>> - struct ovn_datapath **ls; > >>> - size_t n_lr; > >>> - struct ovn_datapath **lr; > >>> + size_t n_nb_ls; > >>> + unsigned long *nb_ls_map; > >>> + > >>> + size_t n_nb_lr; > >>> + unsigned long *nb_lr_map; > >>> }; > >>> > >>> struct ovn_lb_group_datapaths *ovn_lb_group_datapaths_create( > >>> @@ -202,21 +207,13 @@ void ovn_lb_group_datapaths_destroy(struct ovn_lb_group_datapaths *); > >>> struct ovn_lb_group_datapaths *ovn_lb_group_datapaths_find( > >>> const struct hmap *lb_group_dps, const struct uuid *); > >>> > >>> -static inline void > >>> -ovn_lb_group_datapaths_add_ls(struct ovn_lb_group_datapaths *lbg_dps, size_t n, > >>> - struct ovn_datapath **ods) > >>> -{ > >>> - memcpy(&lbg_dps->ls[lbg_dps->n_ls], ods, n * sizeof *ods); > >>> - lbg_dps->n_ls += n; > >>> -} > >>> - > >> > >> This was O(1), but this patch changed it to O(n). Maybe we need some scale test data > >> to see the impact of recompute performance until we are confident that recompute > >> won't be triggered in most cases. I remember this "memcpy" approach was for a significant > >> optimization from Ilya. cc @Ilya Maximets <mailto:i.maximets@ovn.org> in case he has > >> more comments. > > > > Thanks, Han, for pointing out. It is hard to tell what the impact would be > > without testing as there are way too many factors, as usual with performance > > testing. So, I fired up 500 node density-heavy test with ovn-heater. > > > > Results are not great. I see average long poll intervals on northd went up > > to 6.6 seconds from previous 2.6 seconds. 95%% went up to 9.8 seconds with > > the patch set applied from 3.9 seconds on current main. > > > > Here is an example iteration log during the test: > > > > 2023-07-18T12:34:37.147Z|01264|inc_proc_eng|INFO|node: northd, handler for input NB_logical_switch took 2479ms > > 2023-07-18T12:34:40.633Z|01265|inc_proc_eng|INFO|node: northd, handler for input NB_logical_router took 3486ms > > 2023-07-18T12:34:41.976Z|01266|inc_proc_eng|INFO|node: lflow, recompute (failed handler for input northd) took 962ms > > 2023-07-18T12:34:41.977Z|01267|timeval|WARN|Unreasonably long 7362ms poll interval (6604ms user, 381ms system) > > > > We can see that northd node is processed incrementally, but very slow. > > In comparison, an average iteration looks like this on the current main: > > > > 2023-07-15T19:55:02.234Z|00456|inc_proc_eng|INFO|node: northd, recompute (missing handler for input NB_load_balancer) took 1829ms > > 2023-07-15T19:55:03.237Z|00457|inc_proc_eng|INFO|node: lflow, recompute (failed handler for input northd) took 979ms > > 2023-07-15T19:55:03.239Z|00458|timeval|WARN|Unreasonably long 2868ms poll interval (2314ms user, 130ms system) > > > > We can see that a full recompute of the northd node on current main is much > > faster than either of the I-P handlers. > > > > perf somewhat points to the functions above to be hot spots, but the > > datapath lookup dominates the chart: > > > > 29.39% ovn-northd [.] ovn_lb_datapaths_find > > 8.38% ovn-northd [.] ovn_lb_datapaths_add_lr > > 6.72% ovn-northd [.] ovn_lb_datapaths_remove_ls > > 6.40% ovn-northd [.] ovn_lb_datapaths_add_ls > > 6.19% ovn-northd [.] ovn_lb_datapaths_remove_lr > > 3.86% ovn-northd [.] northd_handle_lr_changes > > > > The ovn-installed latency in the test went up from average 3.3 seconds to > > 6.4 seconds. > > > > All in all, incremental processing implemented in this patch set appears to > > be 2-3 times slower than a full recompute on current main. > > I also checked the recompute time now. And it looks like a full recompute > after applying the patch set is about 2x slower than on a current main. > Interestingly enough, it means that with a patch set applied, the full > recompute is a bit faster than I-P iterations. Oops. Thanks Han and Ilya for the comments and Ilya for running a scale test. I'll address all these and in the next version will also include the scale test results. Numan > > > > > Best regards, Ilya Maximets > _______________________________________________ > dev mailing list > dev@openvswitch.org > https://mail.openvswitch.org/mailman/listinfo/ovs-dev
diff --git a/lib/lb.c b/lib/lb.c index b2b6473c1d..a0426cc42e 100644 --- a/lib/lb.c +++ b/lib/lb.c @@ -1100,7 +1100,7 @@ ovn_lb_datapaths_add_ls(struct ovn_lb_datapaths *lb_dps, size_t n, void ovn_lb_datapaths_remove_ls(struct ovn_lb_datapaths *lb_dps, size_t n, - struct ovn_datapath **ods) + struct ovn_datapath **ods) { for (size_t i = 0; i < n; i++) { if (bitmap_is_set(lb_dps->nb_ls_map, ods[i]->index)) { @@ -1112,14 +1112,14 @@ ovn_lb_datapaths_remove_ls(struct ovn_lb_datapaths *lb_dps, size_t n, struct ovn_lb_group_datapaths * ovn_lb_group_datapaths_create(const struct ovn_lb_group *lb_group, - size_t max_ls_datapaths, - size_t max_lr_datapaths) + size_t n_ls_datapaths, + size_t n_lr_datapaths) { struct ovn_lb_group_datapaths *lb_group_dps = xzalloc(sizeof *lb_group_dps); lb_group_dps->lb_group = lb_group; - lb_group_dps->ls = xmalloc(max_ls_datapaths * sizeof *lb_group_dps->ls); - lb_group_dps->lr = xmalloc(max_lr_datapaths * sizeof *lb_group_dps->lr); + lb_group_dps->nb_ls_map = bitmap_allocate(n_ls_datapaths); + lb_group_dps->nb_lr_map = bitmap_allocate(n_lr_datapaths); return lb_group_dps; } @@ -1127,8 +1127,8 @@ ovn_lb_group_datapaths_create(const struct ovn_lb_group *lb_group, void ovn_lb_group_datapaths_destroy(struct ovn_lb_group_datapaths *lb_group_dps) { - free(lb_group_dps->ls); - free(lb_group_dps->lr); + bitmap_free(lb_group_dps->nb_lr_map); + bitmap_free(lb_group_dps->nb_ls_map); free(lb_group_dps); } @@ -1146,3 +1146,34 @@ ovn_lb_group_datapaths_find(const struct hmap *lb_group_dps_map, } return NULL; } + +void +ovn_lb_group_datapaths_add_ls(struct ovn_lb_group_datapaths *lbg_dps, size_t n, + struct ovn_datapath **ods) +{ + for (size_t i = 0; i < n; i++) { + if (!bitmap_is_set(lbg_dps->nb_ls_map, ods[i]->index)) { + bitmap_set1(lbg_dps->nb_ls_map, ods[i]->index); + lbg_dps->n_nb_ls++; + } + } +} + +void +ovn_lb_group_datapaths_remove_ls(struct ovn_lb_group_datapaths *lbg_dps, + size_t n, struct ovn_datapath **ods) +{ + for (size_t i = 0; i < n; i++) { + if (bitmap_is_set(lbg_dps->nb_ls_map, ods[i]->index)) { + bitmap_set0(lbg_dps->nb_ls_map, ods[i]->index); + lbg_dps->n_nb_ls--; + } + } +} + +void +ovn_lb_group_datapaths_add_lr(struct ovn_lb_group_datapaths *lbg_dps, + struct ovn_datapath *lr) +{ + bitmap_set1(lbg_dps->nb_lr_map, lr->index); +} diff --git a/lib/lb.h b/lib/lb.h index ac33333a7f..08723e8ef3 100644 --- a/lib/lb.h +++ b/lib/lb.h @@ -19,6 +19,7 @@ #include <sys/types.h> #include <netinet/in.h> +#include "lib/bitmap.h" #include "lib/smap.h" #include "openvswitch/hmap.h" #include "ovn-util.h" @@ -179,6 +180,9 @@ void ovn_lb_datapaths_add_lr(struct ovn_lb_datapaths *, size_t n, struct ovn_datapath **); void ovn_lb_datapaths_add_ls(struct ovn_lb_datapaths *, size_t n, struct ovn_datapath **); +void ovn_lb_datapaths_set_ls(struct ovn_lb_datapaths *, size_t n, + struct ovn_datapath **); + void ovn_lb_datapaths_remove_ls(struct ovn_lb_datapaths *, size_t n, struct ovn_datapath **); @@ -188,10 +192,11 @@ struct ovn_lb_group_datapaths { const struct ovn_lb_group *lb_group; /* Datapaths to which 'lb_group' is applied. */ - size_t n_ls; - struct ovn_datapath **ls; - size_t n_lr; - struct ovn_datapath **lr; + size_t n_nb_ls; + unsigned long *nb_ls_map; + + size_t n_nb_lr; + unsigned long *nb_lr_map; }; struct ovn_lb_group_datapaths *ovn_lb_group_datapaths_create( @@ -202,21 +207,13 @@ void ovn_lb_group_datapaths_destroy(struct ovn_lb_group_datapaths *); struct ovn_lb_group_datapaths *ovn_lb_group_datapaths_find( const struct hmap *lb_group_dps, const struct uuid *); -static inline void -ovn_lb_group_datapaths_add_ls(struct ovn_lb_group_datapaths *lbg_dps, size_t n, - struct ovn_datapath **ods) -{ - memcpy(&lbg_dps->ls[lbg_dps->n_ls], ods, n * sizeof *ods); - lbg_dps->n_ls += n; -} - -static inline void -ovn_lb_group_datapaths_add_lr(struct ovn_lb_group_datapaths *lbg_dps, - struct ovn_datapath *lr) -{ - lbg_dps->lr[lbg_dps->n_lr++] = lr; -} +void ovn_lb_group_datapaths_add_ls(struct ovn_lb_group_datapaths *, size_t n, + struct ovn_datapath **); +void ovn_lb_group_datapaths_remove_ls(struct ovn_lb_group_datapaths *, + size_t n, struct ovn_datapath **); +void ovn_lb_group_datapaths_add_lr(struct ovn_lb_group_datapaths *, + struct ovn_datapath *lr); struct ovn_controller_lb { struct hmap_node hmap_node; diff --git a/northd/northd.c b/northd/northd.c index bf02190f7c..20a58afa6b 100644 --- a/northd/northd.c +++ b/northd/northd.c @@ -4102,7 +4102,8 @@ associate_ls_lbs(struct ovn_datapath *ls_od, struct hmap *lb_datapaths_map) static void associate_ls_lb_groups(struct ovn_datapath *ls_od, - struct hmap *lb_group_datapaths_map) + struct hmap *lb_group_datapaths_map, + struct hmap *lb_datapaths_map) { const struct nbrec_load_balancer_group *nbrec_lb_group; struct ovn_lb_group_datapaths *lb_group_dps; @@ -4120,6 +4121,15 @@ associate_ls_lb_groups(struct ovn_datapath *ls_od, ovs_assert(lb_group_dps); ovn_lb_group_datapaths_add_ls(lb_group_dps, 1, &ls_od); ls_od->lb_group_uuids[i] = *lb_group_uuid; + + struct ovn_lb_datapaths *lb_dps; + for (size_t j = 0; j < lb_group_dps->lb_group->n_lbs; j++) { + const struct uuid *lb_uuid = + &lb_group_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, &ls_od); + } } } @@ -4159,7 +4169,7 @@ build_lb_datapaths(const struct hmap *lbs, const struct hmap *lb_groups, continue; } associate_ls_lbs(od, lb_datapaths_map); - associate_ls_lb_groups(od, lb_group_datapaths_map); + associate_ls_lb_groups(od, lb_group_datapaths_map, lb_datapaths_map); } HMAP_FOR_EACH (od, key_node, &lr_datapaths->datapaths) { @@ -4219,10 +4229,12 @@ build_lb_datapaths(const struct hmap *lbs, const struct hmap *lb_groups, &lb_group_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, lb_group_dps->n_ls, - lb_group_dps->ls); - ovn_lb_datapaths_add_lr(lb_dps, lb_group_dps->n_lr, - lb_group_dps->lr); + size_t index; + BITMAP_FOR_EACH_1 (index, ods_size(lr_datapaths), + lb_group_dps->nb_lr_map) { + od = lr_datapaths->array[index]; + ovn_lb_datapaths_add_lr(lb_dps, 1, &od); + } } } } @@ -4401,8 +4413,9 @@ build_lswitch_lbs_from_lrouter(struct ovn_datapaths *lr_datapaths, struct ovn_lb_group_datapaths *lb_group_dps; HMAP_FOR_EACH (lb_group_dps, hmap_node, lb_group_dps_map) { - for (size_t i = 0; i < lb_group_dps->n_lr; i++) { - struct ovn_datapath *od = lb_group_dps->lr[i]; + BITMAP_FOR_EACH_1 (index, ods_size(lr_datapaths), + lb_group_dps->nb_lr_map) { + struct ovn_datapath *od = lr_datapaths->array[index]; ovn_lb_group_datapaths_add_ls(lb_group_dps, od->n_ls_peers, od->ls_peers); for (size_t j = 0; j < lb_group_dps->lb_group->n_lbs; j++) { @@ -5072,7 +5085,8 @@ check_unsupported_inc_proc_for_ls_changes( for (col = 0; col < NBREC_LOGICAL_SWITCH_N_COLUMNS; col++) { if (nbrec_logical_switch_is_updated(ls, col)) { if (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 true; @@ -5103,12 +5117,6 @@ check_unsupported_inc_proc_for_ls_changes( return true; } } - 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 true; - } - } 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) { @@ -5338,6 +5346,68 @@ ls_check_and_handle_lb_changes(const struct nbrec_logical_switch *changed_ls, return true; } +static bool +ls_check_and_handle_lb_group_changes( + const struct nbrec_logical_switch *changed_ls, + struct hmap *lb_group_datapaths_map, + struct hmap *lb_datapaths_map, + struct ovn_datapath *od, + struct ls_change *ls_change, + bool *updated) +{ + bool lbg_modified = false; + /* Check if lb_groups changed or not */ + if (!nbrec_logical_switch_is_updated(changed_ls, + NBREC_LOGICAL_SWITCH_COL_LOAD_BALANCER_GROUP)) { + /* load_balancer_group column of logical switch hasn't changed, but + * its possible that a load balancer group may have changed (like + * a load balancer added or removed from the group). */ + for (size_t i = 0; i < changed_ls->n_load_balancer_group; i++) { + if (nbrec_load_balancer_group_row_get_seqno( + changed_ls->load_balancer_group[i], + OVSDB_IDL_CHANGE_MODIFY) > 0) { + lbg_modified = true; + break; + } + } + } else { + /* load_balancer_group column of logical switch has changed. */ + lbg_modified = true; + } + + if (!lbg_modified) { + /* Nothing changed */ + return true; + } + + /* Disassociate load balancer group (and its load balancers) from + * the datapath and rebuild the association again. */ + struct ovn_lb_group_datapaths *lbg_dps; + for (size_t i = 0; i < od->n_lb_group_uuids; i++) { + lbg_dps = ovn_lb_group_datapaths_find(lb_group_datapaths_map, + &od->lb_group_uuids[i]); + if (lbg_dps) { + ovn_lb_group_datapaths_remove_ls(lbg_dps, 1, &od); + + struct ovn_lb_datapaths *lb_dps; + for (size_t j = 0; j < lbg_dps->lb_group->n_lbs; j++) { + const struct uuid *lb_uuid = + &lbg_dps->lb_group->lbs[j]->nlb->header_.uuid; + lb_dps = ovn_lb_datapaths_find(lb_datapaths_map, lb_uuid); + if (lb_dps) { + ovn_lb_datapaths_remove_ls(lb_dps, 1, &od); + } + } + } + } + + associate_ls_lb_groups(od, lb_group_datapaths_map, lb_datapaths_map); + ls_change->lbs_changed = true; + *updated = true; + /* Re-evaluate 'od->has_lb_vip' */ + init_lb_for_datapath(od); + return true; +} /* Return true if changes are handled incrementally, false otherwise. * When there are any changes, try to track what's exactly changed and set @@ -5396,6 +5466,15 @@ northd_handle_ls_changes(struct ovsdb_idl_txn *ovnsb_idl_txn, goto fail; } + if (!ls_check_and_handle_lb_group_changes(changed_ls, + &nd->lb_group_datapaths_map, + &nd->lb_datapaths_map, + od, ls_change, &updated)) { + destroy_tracked_ls_change(ls_change); + free(ls_change); + goto fail; + } + if (updated) { ovs_list_push_back(&nd->tracked_ls_changes.updated, &ls_change->list_node); diff --git a/tests/ovn-northd.at b/tests/ovn-northd.at index d9f3917a3e..d3a5851e35 100644 --- a/tests/ovn-northd.at +++ b/tests/ovn-northd.at @@ -9783,16 +9783,16 @@ check_engine_stats norecompute recompute check as northd ovn-appctl -t NORTHD_TYPE inc-engine/clear-stats check ovn-nbctl add logical_switch sw0 load_balancer_group $lbg1_uuid -check_engine_stats recompute recompute +check_engine_stats norecompute recompute # Update lb and this should result in recompute check as northd ovn-appctl -t NORTHD_TYPE inc-engine/clear-stats check ovn-nbctl --wait=sb set load_balancer . options:bar=foo -check_engine_stats recompute recompute +check_engine_stats norecompute recompute check as northd ovn-appctl -t NORTHD_TYPE inc-engine/clear-stats check ovn-nbctl clear logical_switch sw0 load_balancer_group -check_engine_stats recompute recompute +check_engine_stats norecompute recompute check as northd ovn-appctl -t NORTHD_TYPE inc-engine/clear-stats check ovn-nbctl add logical_router lr0 load_balancer_group $lbg1_uuid