OVS commit 1cc618c32524 ("ovsdb-idl: Fix atomicity of writes that
don't change a column's value.") [0] explains why writes that don't
change a column's value cannot be optimized out early if the column is
read/write.In northd, most tables have change tracking enabled, making all their columns read/write. That means that a write to a column (even if it doesn't change the value) will add the row to the current transaction. Validation is eventually performed before sending the transaction to the server but if there are lots of such records this becomes costly. Profiling what happens in northd when running with a NB database taken from an ovn-k8s-like scale test (16K load balancers applied to 120 logical switches and routers) we notice that ovn-northd was always writing to the SB.Load_Balancer columns even if nothing changed. This commit changes that behavior and only writes to the SB.Load_Balancer columns if needed. Without this change, in our test server, with ovn-northd running against NB database mentioned above, processing loop intervals were ~13 seconds. With this change applied, loop intervals go down to ~7 seconds. [0] https://github.com/openvswitch/ovs/commit/1cc618c32524 Reported-at: https://bugzilla.redhat.com/show_bug.cgi?id=2069623 Signed-off-by: Dumitru Ceara <[email protected]> --- v2: - Check that NB/SB LB proto is not NULL before trying to access it. --- northd/northd.c | 106 ++++++++++++++++++++++++++++++++++++++---------- 1 file changed, 84 insertions(+), 22 deletions(-) diff --git a/northd/northd.c b/northd/northd.c index 87d9b2867a..744c960f6d 100644 --- a/northd/northd.c +++ b/northd/northd.c @@ -3951,6 +3951,57 @@ build_lb_port_related_data(struct hmap *datapaths, struct hmap *ports, build_lb_svcs(input_data, ovnsb_txn, ports, lbs); } +static bool +sb_lb_needs_update(const struct ovn_northd_lb *lb, + const struct sbrec_load_balancer *slb) +{ + if (strcmp(lb->nlb->name, slb->name)) { + return true; + } + + if (!smap_equal(&lb->nlb->vips, &slb->vips)) { + return true; + } + + if ((lb->nlb->protocol && !slb->protocol) + || (!lb->nlb->protocol && slb->protocol)) { + return true; + } + + if (lb->nlb->protocol && slb->protocol + && strcmp(lb->nlb->protocol, slb->protocol)) { + return true; + } + + if (!smap_get_bool(&slb->options, "hairpin_orig_tuple", false)) { + return true; + } + + if (strcmp(smap_get_def(&lb->nlb->options, "hairpin_snat_ip", ""), + smap_get_def(&slb->options, "hairpin_snat_ip", ""))) { + return true; + } + + if (lb->n_nb_ls != slb->n_datapaths) { + return true; + } + + struct hmapx nb_datapaths = HMAPX_INITIALIZER(&nb_datapaths); + for (size_t i = 0; i < lb->n_nb_ls; i++) { + hmapx_add(&nb_datapaths, CONST_CAST(void *, lb->nb_ls[i]->sb)); + } + + bool stale = false; + for (size_t i = 0; i < slb->n_datapaths; i++) { + if (!hmapx_contains(&nb_datapaths, slb->datapaths[i])) { + stale = true; + break; + } + } + hmapx_destroy(&nb_datapaths); + return stale; +} + /* Syncs relevant load balancers (applied to logical switches) to the * Southbound database. */ @@ -3997,21 +4048,8 @@ sync_lbs(struct northd_input *input_data, struct ovsdb_idl_txn *ovnsb_txn, continue; } - /* Store the fact that northd provides the original (destination IP + - * transport port) tuple. - */ - struct smap options; - smap_clone(&options, &lb->nlb->options); - smap_replace(&options, "hairpin_orig_tuple", "true"); - - struct sbrec_datapath_binding **lb_dps = - xmalloc(lb->n_nb_ls * sizeof *lb_dps); - for (size_t i = 0; i < lb->n_nb_ls; i++) { - lb_dps[i] = CONST_CAST(struct sbrec_datapath_binding *, - lb->nb_ls[i]->sb); - } - - if (!lb->slb) { + sbrec_lb = lb->slb; + if (!sbrec_lb) { sbrec_lb = sbrec_load_balancer_insert(ovnsb_txn); lb->slb = sbrec_lb; char *lb_id = xasprintf( @@ -4021,13 +4059,37 @@ sync_lbs(struct northd_input *input_data, struct ovsdb_idl_txn *ovnsb_txn, sbrec_load_balancer_set_external_ids(sbrec_lb, &external_ids); free(lb_id); } - sbrec_load_balancer_set_name(lb->slb, lb->nlb->name); - sbrec_load_balancer_set_vips(lb->slb, &lb->nlb->vips); - sbrec_load_balancer_set_protocol(lb->slb, lb->nlb->protocol); - sbrec_load_balancer_set_datapaths(lb->slb, lb_dps, lb->n_nb_ls); - sbrec_load_balancer_set_options(lb->slb, &options); - smap_destroy(&options); - free(lb_dps); + + /* Only update SB.Load_Balancer columns if needed. */ + if (!lb->slb || sb_lb_needs_update(lb, sbrec_lb)) { + lb->slb = sbrec_lb; + sbrec_load_balancer_set_name(lb->slb, lb->nlb->name); + sbrec_load_balancer_set_vips(lb->slb, &lb->nlb->vips); + sbrec_load_balancer_set_protocol(lb->slb, lb->nlb->protocol); + + struct sbrec_datapath_binding **lb_dps = + xmalloc(lb->n_nb_ls * sizeof *lb_dps); + for (size_t i = 0; i < lb->n_nb_ls; i++) { + lb_dps[i] = CONST_CAST(struct sbrec_datapath_binding *, + lb->nb_ls[i]->sb); + } + sbrec_load_balancer_set_datapaths(lb->slb, lb_dps, lb->n_nb_ls); + free(lb_dps); + + /* Store the fact that northd provides the original (destination + * IP + transport port) tuple. + */ + struct smap options; + smap_clone(&options, &lb->nlb->options); + smap_add(&options, "hairpin_orig_tuple", "true"); + const char *hairpin_snat_ip = + smap_get(&lb->nlb->options, "hairpin_snat_ip"); + if (hairpin_snat_ip) { + smap_add(&options, "hairpin_snat_ip", hairpin_snat_ip); + } + sbrec_load_balancer_set_options(lb->slb, &options); + smap_destroy(&options); + } } /* Datapath_Binding.load_balancers is not used anymore, it's still in the -- 2.27.0 _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
