[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