[ovs-dev] [PATCH ovn v10 7/8] ovn-controller: Use the tracked runtime data changes for flow calculation.

Dumitru Ceara dceara at redhat.com
Tue Jun 9 10:48:20 UTC 2020


On 6/8/20 3:50 PM, numans at ovn.org wrote:
> From: Venkata Anil <anilvenkata at redhat.com>
> 
> This patch processes the logical flows of tracked datapaths
> and tracked logical ports. To handle the tracked logical port
> changes, reference of logical flows to port bindings is maintained.
> 
> Co-Authored-by: Numan Siddique <numans at ovn.org>
> Signed-off-by: Venkata Anil <anilvenkata at redhat.com>
> Signed-off-by: Numan Siddique <numans at ovn.org>
> ---
>  controller/lflow.c          |  88 ++++++++++++++++++++++++++-
>  controller/lflow.h          |  13 +++-
>  controller/ovn-controller.c | 116 +++++++++++++++++++-----------------
>  controller/physical.h       |   3 +-
>  tests/ovn-performance.at    |  12 ++--
>  5 files changed, 169 insertions(+), 63 deletions(-)
> 
> diff --git a/controller/lflow.c b/controller/lflow.c
> index 01214a3a6..46f84e5f8 100644
> --- a/controller/lflow.c
> +++ b/controller/lflow.c
> @@ -59,6 +59,10 @@ struct condition_aux {
>      struct ovsdb_idl_index *sbrec_port_binding_by_name;
>      const struct sbrec_chassis *chassis;
>      const struct sset *active_tunnels;
> +    const struct sbrec_logical_flow *lflow;
> +    /* Resource reference to store the port name referenced
> +     * in is_chassis_resident() to lhe logicl flow. */
> +    struct lflow_resource_ref *lfrr;
>  };
>  
>  static bool
> @@ -68,6 +72,8 @@ consider_logical_flow(const struct sbrec_logical_flow *lflow,
>                        struct controller_event_options *controller_event_opts,
>                        struct lflow_ctx_in *l_ctx_in,
>                        struct lflow_ctx_out *l_ctx_out);
> +static void lflow_resource_add(struct lflow_resource_ref *, enum ref_type,
> +                               const char *ref_name, const struct uuid *);
>  
>  static bool
>  lookup_port_cb(const void *aux_, const char *port_name, unsigned int *portp)
> @@ -120,6 +126,14 @@ is_chassis_resident_cb(const void *c_aux_, const char *port_name)
>      if (!pb) {
>          return false;
>      }
> +
> +    /* Store the port_name to lflow reference. */
> +    int64_t dp_id = pb->datapath->tunnel_key;
> +    char buf[16];
> +    snprintf(buf, sizeof(buf), "%"PRId64"_%"PRId64, dp_id, pb->tunnel_key);

Hi Numan, Anil,

We do this (or similar) in various places in lflow.c to build a unique
key from dp_id and pb->tunnel_key/port_id. Maybe it's good to refactor
it into a single function?

Also, should we add some compile time checks to make sure 16 chars are
enough?

> +    lflow_resource_add(c_aux->lfrr, REF_TYPE_PORTBINDING, buf,
> +                       &c_aux->lflow->header_.uuid);
> +
>      if (strcmp(pb->type, "chassisredirect")) {
>          /* for non-chassisredirect ports */
>          return pb->chassis && pb->chassis == c_aux->chassis;
> @@ -258,7 +272,7 @@ lflow_resource_destroy_lflow(struct lflow_resource_ref *lfrr,
>  /* Adds the logical flows from the Logical_Flow table to flow tables. */
>  static void
>  add_logical_flows(struct lflow_ctx_in *l_ctx_in,
> -                  struct lflow_ctx_out *l_ctx_out)
> +                        struct lflow_ctx_out *l_ctx_out)

Nit: indentation.

>  {
>      const struct sbrec_logical_flow *lflow;
>  
> @@ -594,6 +608,8 @@ consider_logical_flow(const struct sbrec_logical_flow *lflow,
>          .sbrec_port_binding_by_name = l_ctx_in->sbrec_port_binding_by_name,
>          .chassis = l_ctx_in->chassis,
>          .active_tunnels = l_ctx_in->active_tunnels,
> +        .lflow = lflow,
> +        .lfrr = l_ctx_out->lfrr
>      };
>      expr = expr_simplify(expr, is_chassis_resident_cb, &cond_aux);
>      expr = expr_normalize(expr);
> @@ -649,6 +665,8 @@ consider_logical_flow(const struct sbrec_logical_flow *lflow,
>                  int64_t dp_id = lflow->logical_datapath->tunnel_key;
>                  char buf[16];
>                  snprintf(buf, sizeof(buf), "%"PRId64"_%"PRId64, dp_id, port_id);
> +                lflow_resource_add(l_ctx_out->lfrr, REF_TYPE_PORTBINDING, buf,
> +                                   &lflow->header_.uuid);
>                  if (!sset_contains(l_ctx_in->local_lport_ids, buf)) {
>                      VLOG_DBG("lflow "UUID_FMT
>                               " port %s in match is not local, skip",
> @@ -847,3 +865,71 @@ lflow_destroy(void)
>      expr_symtab_destroy(&symtab);
>      shash_destroy(&symtab);
>  }
> +
> +bool
> +lflow_add_flows_for_datapath(const struct sbrec_datapath_binding *dp,
> +                             struct lflow_ctx_in *l_ctx_in,
> +                             struct lflow_ctx_out *l_ctx_out)
> +{
> +    bool handled = true;
> +    struct hmap dhcp_opts = HMAP_INITIALIZER(&dhcp_opts);
> +    struct hmap dhcpv6_opts = HMAP_INITIALIZER(&dhcpv6_opts);
> +    const struct sbrec_dhcp_options *dhcp_opt_row;
> +    SBREC_DHCP_OPTIONS_TABLE_FOR_EACH (dhcp_opt_row,
> +                                       l_ctx_in->dhcp_options_table) {
> +        dhcp_opt_add(&dhcp_opts, dhcp_opt_row->name, dhcp_opt_row->code,
> +                     dhcp_opt_row->type);
> +    }
> +
> +
> +    const struct sbrec_dhcpv6_options *dhcpv6_opt_row;
> +    SBREC_DHCPV6_OPTIONS_TABLE_FOR_EACH (dhcpv6_opt_row,
> +                                         l_ctx_in->dhcpv6_options_table) {
> +       dhcp_opt_add(&dhcpv6_opts, dhcpv6_opt_row->name, dhcpv6_opt_row->code,
> +                    dhcpv6_opt_row->type);
> +    }
> +
> +    struct hmap nd_ra_opts = HMAP_INITIALIZER(&nd_ra_opts);
> +    nd_ra_opts_init(&nd_ra_opts);
> +
> +    struct controller_event_options controller_event_opts;
> +    controller_event_opts_init(&controller_event_opts);
> +
> +    struct sbrec_logical_flow *lf_row = sbrec_logical_flow_index_init_row(
> +        l_ctx_in->sbrec_logical_flow_by_logical_datapath);
> +    sbrec_logical_flow_index_set_logical_datapath(lf_row, dp);
> +
> +    const struct sbrec_logical_flow *lflow;
> +    SBREC_LOGICAL_FLOW_FOR_EACH_EQUAL (
> +        lflow, lf_row, l_ctx_in->sbrec_logical_flow_by_logical_datapath) {
> +        /* Remove the lflow from flow_table if present before processing it. */
> +        ofctrl_remove_flows(l_ctx_out->flow_table, &lflow->header_.uuid);
> +
> +        if (!consider_logical_flow(lflow, &dhcp_opts, &dhcpv6_opts,
> +                                   &nd_ra_opts, &controller_event_opts,
> +                                   l_ctx_in, l_ctx_out)) {
> +            handled = false;
> +            break;
> +        }
> +    }
> +
> +    dhcp_opts_destroy(&dhcp_opts);
> +    dhcp_opts_destroy(&dhcpv6_opts);
> +    nd_ra_opts_destroy(&nd_ra_opts);
> +    controller_event_opts_destroy(&controller_event_opts);
> +    return handled;
> +}
> +
> +bool
> +lflow_handle_flows_for_lport(const struct sbrec_port_binding *pb,
> +                             struct lflow_ctx_in *l_ctx_in,
> +                             struct lflow_ctx_out *l_ctx_out)

I think this function should be in ovn-controller.c where we do all the
other calls to lflow_handle_changed_ref() for address sets and port groups.

> +{
> +    int64_t dp_id = pb->datapath->tunnel_key;
> +    char pb_ref_name[16];
> +    snprintf(pb_ref_name, sizeof(pb_ref_name), "%"PRId64"_%"PRId64,
> +             dp_id, pb->tunnel_key);
> +    bool changed = true;
> +    return lflow_handle_changed_ref(REF_TYPE_PORTBINDING, pb_ref_name,
> +                                    l_ctx_in, l_ctx_out, &changed);
> +}
> diff --git a/controller/lflow.h b/controller/lflow.h
> index f02f709d7..c8ed5b686 100644
> --- a/controller/lflow.h
> +++ b/controller/lflow.h
> @@ -48,6 +48,9 @@ struct sbrec_dhcp_options_table;
>  struct sbrec_dhcpv6_options_table;
>  struct sbrec_logical_flow_table;
>  struct sbrec_mac_binding_table;
> +struct sbrec_port_binding_table;

We never use sbrec_port_binding_table, I guess we can remove it.

> +struct sbrec_datapath_binding;
> +struct sbrec_port_binding;
>  struct simap;
>  struct sset;
>  struct uuid;
> @@ -72,7 +75,8 @@ struct uuid;
>  
>  enum ref_type {
>      REF_TYPE_ADDRSET,
> -    REF_TYPE_PORTGROUP
> +    REF_TYPE_PORTGROUP,
> +    REF_TYPE_PORTBINDING
>  };
>  
>  /* Maintains the relationship for a pair of named resource and
> @@ -117,6 +121,7 @@ void lflow_resource_clear(struct lflow_resource_ref *);
>  
>  struct lflow_ctx_in {
>      struct ovsdb_idl_index *sbrec_multicast_group_by_name_datapath;
> +    struct ovsdb_idl_index *sbrec_logical_flow_by_logical_datapath;
>      struct ovsdb_idl_index *sbrec_port_binding_by_name;
>      const struct sbrec_dhcp_options_table *dhcp_options_table;
>      const struct sbrec_dhcpv6_options_table *dhcpv6_options_table;
> @@ -157,4 +162,10 @@ void lflow_destroy(void);
>  
>  void lflow_expr_destroy(struct hmap *lflow_expr_cache);
>  
> +bool lflow_add_flows_for_datapath(const struct sbrec_datapath_binding *,
> +                                  struct lflow_ctx_in *,
> +                                  struct lflow_ctx_out *);
> +bool lflow_handle_flows_for_lport(const struct sbrec_port_binding *,
> +                                  struct lflow_ctx_in *,
> +                                  struct lflow_ctx_out *);
>  #endif /* controller/lflow.h */
> diff --git a/controller/ovn-controller.c b/controller/ovn-controller.c
> index 98652866c..2347ec669 100644
> --- a/controller/ovn-controller.c
> +++ b/controller/ovn-controller.c
> @@ -1502,6 +1502,11 @@ static void init_lflow_ctx(struct engine_node *node,
>                  engine_get_input("SB_port_binding", node),
>                  "name");
>  
> +    struct ovsdb_idl_index *sbrec_logical_flow_by_dp =
> +        engine_ovsdb_node_get_index(
> +                engine_get_input("SB_logical_flow", node),
> +                "logical_datapath");
> +
>      struct ovsdb_idl_index *sbrec_mc_group_by_name_dp =
>          engine_ovsdb_node_get_index(
>                  engine_get_input("SB_multicast_group", node),
> @@ -1549,6 +1554,8 @@ static void init_lflow_ctx(struct engine_node *node,
>  
>      l_ctx_in->sbrec_multicast_group_by_name_datapath =
>          sbrec_mc_group_by_name_dp;
> +    l_ctx_in->sbrec_logical_flow_by_logical_datapath =
> +        sbrec_logical_flow_by_dp;
>      l_ctx_in->sbrec_port_binding_by_name = sbrec_port_binding_by_name;
>      l_ctx_in->dhcp_options_table  = dhcp_table;
>      l_ctx_in->dhcpv6_options_table = dhcpv6_table;
> @@ -1703,7 +1710,8 @@ flow_output_sb_mac_binding_handler(struct engine_node *node, void *data)
>  }
>  
>  static bool
> -flow_output_sb_port_binding_handler(struct engine_node *node, void *data)
> +flow_output_sb_port_binding_handler(struct engine_node *node,
> +                                    void *data)
>  {
>      struct ed_type_runtime_data *rt_data =
>          engine_get_input_data("runtime_data", node);
> @@ -1711,56 +1719,13 @@ flow_output_sb_port_binding_handler(struct engine_node *node, void *data)
>      struct ed_type_flow_output *fo = data;
>      struct ovn_desired_flow_table *flow_table = &fo->flow_table;
>  
> -    /* XXX: now we handle port-binding changes for physical flow processing
> -     * only, but port-binding change can have impact to logical flow
> -     * processing, too, in below circumstances:
> -     *
> -     *  - When a port-binding for a lport is inserted/deleted but the lflow
> -     *    using that lport doesn't change.
> -     *
> -     *    This can happen only when the lport name is used by ACL match
> -     *    condition, which is specified by user. Even in that case, if the port
> -     *    is actually bound on the current chassis it will trigger recompute on
> -     *    that chassis since ovs interface would be updated. So the only
> -     *    situation this would have real impact is when user defines an ACL
> -     *    that includes lport that is not on current chassis, and there is a
> -     *    port-binding creation/deletion related to that lport.e.g.: an ACL is
> -     *    defined:
> -     *
> -     *    to-lport 1000 'outport=="A" && inport=="B"' allow-related
> -     *
> -     *    If "A" is on current chassis, but "B" is lport that hasn't been
> -     *    created yet. When a lport "B" is created and bound on another
> -     *    chassis, the ACL will not take effect on the current chassis until a
> -     *    recompute is triggered later. This case doesn't seem to be a problem
> -     *    for real world use cases because usually lport is created before
> -     *    being referenced by name in ACLs.
> -     *
> -     *  - When is_chassis_resident(<lport>) is used in lflow. In this case the
> -     *    port binding is not a regular VIF. It can be either "patch" or
> -     *    "external", with ha-chassis-group assigned.  In current
> -     *    "runtime_data" handling, port-binding changes for these types always
> -     *    trigger recomputing. So it is fine even if we do not handle it here.
> -     *    (due to the ovsdb tracking support for referenced table changes,
> -     *    ha-chassis-group changes will appear as port-binding change).
> -     *
> -     *  - When a mac-binding doesn't change but the port-binding related to
> -     *    that mac-binding is deleted. In this case the neighbor flow generated
> -     *    for the mac-binding should be deleted. This would not cause any real
> -     *    issue for now, since the port-binding related to mac-binding is
> -     *    always logical router port, and any change to logical router port
> -     *    would just trigger recompute.
> -     *
> -     * Although there is no correctness issue so far (except the unusual ACL
> -     * use case, which doesn't seem to be a real problem), it might be better
> -     * to handle this more gracefully, without the need to consider these
> -     * tricky scenarios.  One approach is to maintain a mapping between lport
> -     * names and the lflows that uses them, and reprocess the related lflows
> -     * when related port-bindings change.
> -     */
>      struct physical_ctx p_ctx;
>      init_physical_ctx(node, rt_data, &p_ctx);
>  
> +    /* We handle port-binding changes for physical flow processing
> +     * only. flow_output runtime data handler takes care of processing
> +     * logical flows for any port binding changes.
> +     */
>      physical_handle_port_binding_changes(&p_ctx, flow_table);
>  
>      engine_set_node_state(node, EN_UPDATED);
> @@ -1848,6 +1813,8 @@ _flow_output_resource_ref_handler(struct engine_node *node, void *data,
>              updated = &pg_data->updated;
>              deleted = &pg_data->deleted;
>              break;
> +
> +        case REF_TYPE_PORTBINDING:

I think a comment here would be helpful; something to specify that
port_binding related lflow updates are handled in
lflow_handle_flows_for_lport().

>          default:
>              OVS_NOT_REACHED();
>      }
> @@ -1909,17 +1876,53 @@ flow_output_runtime_data_handler(struct engine_node *node,
>          return false;
>      }
>  
> -    if (!hmap_is_empty(&rt_data->tracked_dp_bindings)) {
> -        /* We are not yet handling the tracked datapath binding
> -         * changes. Return false to trigger full recompute. */
> -        return false;
> +    struct hmap *tracked_dp_bindings = &rt_data->tracked_dp_bindings;
> +    if (hmap_is_empty(tracked_dp_bindings)) {
> +        if (rt_data->local_lports_changed) {
> +            engine_set_node_state(node, EN_UPDATED);
> +        }
> +        return true;
> +    }
> +
> +    struct lflow_ctx_in l_ctx_in;
> +    struct lflow_ctx_out l_ctx_out;
> +    struct ed_type_flow_output *fo = data;
> +    init_lflow_ctx(node, rt_data, fo, &l_ctx_in, &l_ctx_out);
> +
> +    struct physical_ctx p_ctx;
> +    init_physical_ctx(node, rt_data, &p_ctx);
> +
> +    bool handled = true;

I think we don't need the handled variable here. We can just return
false where needed and true at the end.

> +    struct tracked_binding_datapath *tdp;
> +    HMAP_FOR_EACH (tdp, node, tracked_dp_bindings) {
> +        if (tdp->is_new || !datapath_is_switch(tdp->dp)) {

Why do we always have to add flows for logical routers? A comment
explaining that might be enough.

> +            handled = lflow_add_flows_for_datapath(tdp->dp, &l_ctx_in,
> +                                                   &l_ctx_out);
> +            if (!handled) {
> +                break;
> +            }

This could be:

if (!lflow_add_flows_for_datapath(tdp->dp, &l_ctx_in, &l_ctx_out)) {
    return false;
}

> +        } else {
> +            struct shash_node *shash_node;
> +            SHASH_FOR_EACH (shash_node, &tdp->lports) {
> +                struct tracked_binding_lport *lport = shash_node->data;
> +                if (!lflow_handle_flows_for_lport(lport->pb, &l_ctx_in,
> +                                                  &l_ctx_out)) {
> +                    handled = false;
> +                    break;

Similar here.

> +                }
> +            }
> +
> +            if (!handled) {
> +                break;
> +            }

This would not be needed then.

> +        }
>      }
>  
> -    if (rt_data->local_lports_changed) {
> +    if (handled) {
>          engine_set_node_state(node, EN_UPDATED);
>      }

Here we could skip the if instead:

engine_set_node_state(node, EN_UPDATED);
return true;

>  
> -    return true;
> +    return handled;
>  }
>  
>  static bool
> @@ -2095,6 +2098,9 @@ main(int argc, char *argv[])
>          = chassis_index_create(ovnsb_idl_loop.idl);
>      struct ovsdb_idl_index *sbrec_multicast_group_by_name_datapath
>          = mcast_group_index_create(ovnsb_idl_loop.idl);
> +    struct ovsdb_idl_index *sbrec_logical_flow_by_logical_datapath
> +        = ovsdb_idl_index_create1(ovnsb_idl_loop.idl,
> +                                  &sbrec_logical_flow_col_logical_datapath);
>      struct ovsdb_idl_index *sbrec_port_binding_by_name
>          = ovsdb_idl_index_create1(ovnsb_idl_loop.idl,
>                                    &sbrec_port_binding_col_logical_port);
> @@ -2255,6 +2261,8 @@ main(int argc, char *argv[])
>      engine_ovsdb_node_add_index(&en_sb_chassis, "name", sbrec_chassis_by_name);
>      engine_ovsdb_node_add_index(&en_sb_multicast_group, "name_datapath",
>                                  sbrec_multicast_group_by_name_datapath);
> +    engine_ovsdb_node_add_index(&en_sb_logical_flow, "logical_datapath",
> +                                sbrec_logical_flow_by_logical_datapath);
>      engine_ovsdb_node_add_index(&en_sb_port_binding, "name",
>                                  sbrec_port_binding_by_name);
>      engine_ovsdb_node_add_index(&en_sb_port_binding, "key",
> diff --git a/controller/physical.h b/controller/physical.h

It looks to me like we don't need the changes in physical.h.

> index 9ca34436a..481f03901 100644
> --- a/controller/physical.h
> +++ b/controller/physical.h
> @@ -33,6 +33,7 @@ struct ovsrec_bridge;
>  struct simap;
>  struct sbrec_multicast_group_table;
>  struct sbrec_port_binding_table;
> +struct sbrec_port_binding;
>  struct sset;
>  
>  /* OVN Geneve option information.
> @@ -61,7 +62,7 @@ struct physical_ctx {
>  void physical_register_ovs_idl(struct ovsdb_idl *);
>  void physical_run(struct physical_ctx *,
>                    struct ovn_desired_flow_table *);
> -void physical_handle_port_binding_changes(struct physical_ctx *,
> +void physical_handle_port_binding_changes(struct physical_ctx *p_ctx,
>                                            struct ovn_desired_flow_table *);
>  void physical_handle_mc_group_changes(struct physical_ctx *,
>                                        struct ovn_desired_flow_table *);
> diff --git a/tests/ovn-performance.at b/tests/ovn-performance.at
> index 08acd3bae..a12757e18 100644
> --- a/tests/ovn-performance.at
> +++ b/tests/ovn-performance.at
> @@ -274,7 +274,7 @@ for i in 1 2; do
>      )
>  
>      # Add router port to $ls
> -    OVN_CONTROLLER_EXPECT_HIT(
> +    OVN_CONTROLLER_EXPECT_NO_HIT(
>          [hv1 hv2], [lflow_run],
>          [ovn-nbctl --wait=hv lrp-add lr1 $lrp 02:00:00:00:0$i:01 10.0.$i.1/24]
>      )
> @@ -282,7 +282,7 @@ for i in 1 2; do
>          [hv1 hv2], [lflow_run],
>          [ovn-nbctl --wait=hv lsp-add $ls $lsp]
>      )
> -    OVN_CONTROLLER_EXPECT_HIT(
> +    OVN_CONTROLLER_EXPECT_NO_HIT(
>          [hv1 hv2], [lflow_run],
>          [ovn-nbctl --wait=hv lsp-set-type $lsp router]
>      )
> @@ -353,8 +353,8 @@ for i in 1 2; do
>      )
>  
>      # Bind port $lp and wait for it to come up
> -    OVN_CONTROLLER_EXPECT_HIT_COND(
> -        [hv$i hv$j], [lflow_run], [>0 =0],
> +    OVN_CONTROLLER_EXPECT_NO_HIT(
> +        [hv$i hv$j], [lflow_run],
>          [as hv$i ovs-vsctl add-port br-int $vif -- set Interface $vif external-ids:iface-id=$lp &&
>           ovn-nbctl wait-until Logical_Switch_Port $lp 'up=true' &&
>           ovn-nbctl --wait=hv sync]
> @@ -404,8 +404,8 @@ for i in 1 2; do
>      lp=lp$i
>  
>      # Delete port $lp
> -    OVN_CONTROLLER_EXPECT_HIT_COND(
> -        [hv$i hv$j], [lflow_run], [>0 =0],
> +    OVN_CONTROLLER_EXPECT_NO_HIT(
> +        [hv$i hv$j], [lflow_run],
>          [ovn-nbctl --wait=hv lsp-del $lp]
>      )
>  done
> 



More information about the dev mailing list