[ovs-dev] [PATCH ovn v3] controller: Avoid unnecessary load balancer flow processing.
Mark Michelson
mmichels at redhat.com
Mon Jul 12 15:19:26 UTC 2021
Since this addressed Han's findings in v1 and I had already ACKed it, I
pushed this change to the main branch. Thanks, Dumitru and Han!
On 7/12/21 10:14 AM, Dumitru Ceara wrote:
> Whenever a Load_Balancer is updated, e.g., a VIP is added, the following
> sequence of events happens:
>
> 1. The Southbound Load_Balancer record is updated.
> 2. The Southbound Datapath_Binding records on which the Load_Balancer is
> applied are updated.
> 3. Southbound ovsdb-server sends updates about the Load_Balancer and
> Datapath_Binding records to ovn-controller.
> 4. The IDL layer in ovn-controller processes the updates at #3, but
> because of the SB schema references between tables [0] all logical
> flows referencing the updated Datapath_Binding are marked as
> "updated". The same is true for Logical_DP_Group records
> referencing the Datapath_Binding, and also for all logical flows
> pointing to the new "updated" datapath groups.
> 5. ovn-controller ends up recomputing (removing/readding) all flows for
> all these tracked updates.
>
> From the SB Schema:
> "Datapath_Binding": {
> "columns": {
> [...]
> "load_balancers": {"type": {"key": {"type": "uuid",
> "refTable": "Load_Balancer",
> "refType": "weak"},
> "min": 0,
> "max": "unlimited"}},
> [...]
> "Load_Balancer": {
> "columns": {
> "datapaths": {
> [...]
> "type": {"key": {"type": "uuid",
> "refTable": "Datapath_Binding"},
> "min": 0, "max": "unlimited"}},
> [...]
> "Logical_DP_Group": {
> "columns": {
> "datapaths":
> {"type": {"key": {"type": "uuid",
> "refTable": "Datapath_Binding",
> "refType": "weak"},
> "min": 0, "max": "unlimited"}}},
> [...]
> "Logical_Flow": {
> "columns": {
> "logical_datapath":
> {"type": {"key": {"type": "uuid",
> "refTable": "Datapath_Binding"},
> "min": 0, "max": 1}},
> "logical_dp_group":
> {"type": {"key": {"type": "uuid",
> "refTable": "Logical_DP_Group"},
>
> In order to avoid this unnecessary Logical_Flow notification storm we
> now remove the explicit reference from Datapath_Binding to
> Load_Balancer and instead store raw UUIDs.
>
> This means that on the ovn-controller side we need to perform a
> Load_Balancer table lookup by UUID whenever a new datapath is added,
> but that doesn't happen too often and the cost of the lookup is
> negligible compared to the huge cost of processing the unnecessary
> logical flow updates.
>
> This change is backwards compatible because the contents stored in the
> database are not changed, just that the schema constraints are relaxed a
> bit.
>
> Some performance measurements, on a scale test deployment simulating an
> ovn-kubernetes deployment with 120 nodes and a large load balancer
> with 16K VIPs associated to each node's logical switch, the event
> processing loop time in ovn-controller, when adding a new VIP, is
> reduced from ~39 seconds to ~8 seconds.
>
> There's no need to change the northd DDlog implementation.
>
> Reported-at: https://bugzilla.redhat.com/show_bug.cgi?id=1978605
> Acked-by: Mark Michelson <mmichels at redhat.com>
> Signed-off-by: Dumitru Ceara <dceara at redhat.com>
> ---
> v3: Update SB schema version.
> v2: Address Han's comments and add Mark's ack.
> ---
> controller/lflow.c | 6 ++++--
> northd/ovn-northd.c | 14 ++++++--------
> ovn-sb.ovsschema | 8 +++-----
> 3 files changed, 13 insertions(+), 15 deletions(-)
>
> diff --git a/controller/lflow.c b/controller/lflow.c
> index 60aa011ff..c58c4f25c 100644
> --- a/controller/lflow.c
> +++ b/controller/lflow.c
> @@ -1744,8 +1744,10 @@ lflow_processing_end:
> /* Add load balancer hairpin flows if the datapath has any load balancers
> * associated. */
> for (size_t i = 0; i < dp->n_load_balancers; i++) {
> - consider_lb_hairpin_flows(dp->load_balancers[i],
> - l_ctx_in->local_datapaths,
> + const struct sbrec_load_balancer *lb =
> + sbrec_load_balancer_table_get_for_uuid(l_ctx_in->lb_table,
> + &dp->load_balancers[i]);
> + consider_lb_hairpin_flows(lb, l_ctx_in->local_datapaths,
> l_ctx_out->flow_table);
> }
>
> diff --git a/northd/ovn-northd.c b/northd/ovn-northd.c
> index 562dc62b2..999c3f482 100644
> --- a/northd/ovn-northd.c
> +++ b/northd/ovn-northd.c
> @@ -3635,19 +3635,17 @@ build_ovn_lbs(struct northd_context *ctx, struct hmap *datapaths,
> continue;
> }
>
> - const struct sbrec_load_balancer **sbrec_lbs =
> - xmalloc(od->nbs->n_load_balancer * sizeof *sbrec_lbs);
> + struct uuid *lb_uuids =
> + xmalloc(od->nbs->n_load_balancer * sizeof *lb_uuids);
> for (size_t i = 0; i < od->nbs->n_load_balancer; i++) {
> const struct uuid *lb_uuid =
> &od->nbs->load_balancer[i]->header_.uuid;
> lb = ovn_northd_lb_find(lbs, lb_uuid);
> - sbrec_lbs[i] = lb->slb;
> + lb_uuids[i] = lb->slb->header_.uuid;
> }
> -
> - sbrec_datapath_binding_set_load_balancers(
> - od->sb, (struct sbrec_load_balancer **)sbrec_lbs,
> - od->nbs->n_load_balancer);
> - free(sbrec_lbs);
> + sbrec_datapath_binding_set_load_balancers(od->sb, lb_uuids,
> + od->nbs->n_load_balancer);
> + free(lb_uuids);
> }
> }
>
> diff --git a/ovn-sb.ovsschema b/ovn-sb.ovsschema
> index bbf60781d..73ef33467 100644
> --- a/ovn-sb.ovsschema
> +++ b/ovn-sb.ovsschema
> @@ -1,7 +1,7 @@
> {
> "name": "OVN_Southbound",
> - "version": "20.18.0",
> - "cksum": "1816525029 26536",
> + "version": "20.19.0",
> + "cksum": "4027775051 26386",
> "tables": {
> "SB_Global": {
> "columns": {
> @@ -166,9 +166,7 @@
> "type": {"key": {"type": "integer",
> "minInteger": 1,
> "maxInteger": 16777215}}},
> - "load_balancers": {"type": {"key": {"type": "uuid",
> - "refTable": "Load_Balancer",
> - "refType": "weak"},
> + "load_balancers": {"type": {"key": {"type": "uuid"},
> "min": 0,
> "max": "unlimited"}},
> "external_ids": {
>
More information about the dev
mailing list