[ovs-dev] [PATCH ovn v12 2/7] ovn-controller: I-P for ct zone and OVS interface changes in flow output stage.

Numan Siddique numans at ovn.org
Fri Jun 19 08:18:59 UTC 2020


On Fri, Jun 19, 2020 at 1:39 PM Numan Siddique <numans at ovn.org> wrote:

>
>
> On Fri, Jun 19, 2020 at 1:11 PM Dumitru Ceara <dceara at redhat.com> wrote:
>
>> On 6/11/20 2:43 PM, numans at ovn.org wrote:
>> > From: Numan Siddique <numans at ovn.org>
>> >
>> > This patch handles ct zone changes and OVS interface changes
>> incrementally
>> > in the flow output stage.
>> >
>> > Any changes to ct zone can be handled by running physical_run() instead
>> of running
>> > flow_output_run(). And any changes to OVS interfaces can be either
>> handled
>> > incrementally (for OVS interfaces representing VIFs) or just running
>> > physical_run() (for tunnel and other types of interfaces).
>> >
>> > To better handle this, a new engine node 'physical_flow_changes' is
>> added which
>> > handles changes to ct zone and OVS interfaces.
>> >
>> > Signed-off-by: Numan Siddique <numans at ovn.org>
>>
>> Hi Numan,
>>
>> I have a few comments on this patch. Please see below.
>>
>> > ---
>> >  controller/binding.c        |  23 +-----
>> >  controller/binding.h        |  24 ++++++-
>> >  controller/ovn-controller.c | 136 +++++++++++++++++++++++++++++++++++-
>> >  controller/physical.c       |  51 ++++++++++++++
>> >  controller/physical.h       |   5 +-
>> >  5 files changed, 214 insertions(+), 25 deletions(-)
>> >
>> > diff --git a/controller/binding.c b/controller/binding.c
>> > index 06ecb93fb..67fd2777f 100644
>> > --- a/controller/binding.c
>> > +++ b/controller/binding.c
>> > @@ -535,7 +535,7 @@ remove_local_lport_ids(struct binding_ctx_out
>> *b_ctx,
>> >   * 'struct local_binding' is used. A shash of these local bindings is
>> >   * maintained with the 'external_ids:iface-id' as the key to the shash.
>> >   *
>> > - * struct local_binding has 3 main fields:
>> > + * struct local_binding (defined in binding.h) has 3 main fields:
>> >   *    - type
>> >   *    - OVS interface row object
>> >   *    - Port_Binding row object
>> > @@ -586,21 +586,6 @@ remove_local_lport_ids(struct binding_ctx_out
>> *b_ctx,
>> >   *   - For each 'virtual' Port Binding (of type BT_VIRTUAL) provided
>> its parent
>> >   *     is bound to this chassis.
>> >   */
>> > -enum local_binding_type {
>> > -    BT_VIF,
>> > -    BT_CONTAINER,
>> > -    BT_VIRTUAL
>> > -};
>> > -
>> > -struct local_binding {
>> > -    char *name;
>> > -    enum local_binding_type type;
>> > -    const struct ovsrec_interface *iface;
>> > -    const struct sbrec_port_binding *pb;
>> > -
>> > -    /* shash of 'struct local_binding' representing children. */
>> > -    struct shash children;
>> > -};
>> >
>> >  static struct local_binding *
>> >  local_binding_create(const char *name, const struct ovsrec_interface
>> *iface,
>> > @@ -622,12 +607,6 @@ local_binding_add(struct shash *local_bindings,
>> struct local_binding *lbinding)
>> >      shash_add(local_bindings, lbinding->name, lbinding);
>> >  }
>> >
>> > -static struct local_binding *
>> > -local_binding_find(struct shash *local_bindings, const char *name)
>> > -{
>> > -    return shash_find_data(local_bindings, name);
>> > -}
>> > -
>> >  static void
>> >  local_binding_destroy(struct local_binding *lbinding)
>> >  {
>> > diff --git a/controller/binding.h b/controller/binding.h
>> > index 9214a7479..161766d2f 100644
>> > --- a/controller/binding.h
>> > +++ b/controller/binding.h
>> > @@ -18,6 +18,7 @@
>> >  #define OVN_BINDING_H 1
>> >
>> >  #include <stdbool.h>
>> > +#include "openvswitch/shash.h"
>> >
>> >  struct hmap;
>> >  struct ovsdb_idl;
>> > @@ -32,7 +33,6 @@ struct sbrec_chassis;
>> >  struct sbrec_port_binding_table;
>> >  struct sset;
>> >  struct sbrec_port_binding;
>> > -struct shash;
>> >
>> >  struct binding_ctx_in {
>> >      struct ovsdb_idl_txn *ovnsb_idl_txn;
>> > @@ -77,6 +77,28 @@ struct binding_ctx_out {
>> >      struct smap *local_iface_ids;
>> >  };
>> >
>> > +enum local_binding_type {
>> > +    BT_VIF,
>> > +    BT_CONTAINER,
>> > +    BT_VIRTUAL
>> > +};
>> > +
>> > +struct local_binding {
>> > +    char *name;
>> > +    enum local_binding_type type;
>> > +    const struct ovsrec_interface *iface;
>> > +    const struct sbrec_port_binding *pb;
>> > +
>> > +    /* shash of 'struct local_binding' representing children. */
>> > +    struct shash children;
>> > +};
>> > +
>> > +static inline struct local_binding *
>> > +local_binding_find(struct shash *local_bindings, const char *name)
>> > +{
>> > +    return shash_find_data(local_bindings, name);
>> > +}
>> > +
>> >  void binding_register_ovs_idl(struct ovsdb_idl *);
>> >  void binding_run(struct binding_ctx_in *, struct binding_ctx_out *);
>> >  bool binding_cleanup(struct ovsdb_idl_txn *ovnsb_idl_txn,
>> > diff --git a/controller/ovn-controller.c b/controller/ovn-controller.c
>> > index bf7214e45..6e66705da 100644
>> > --- a/controller/ovn-controller.c
>> > +++ b/controller/ovn-controller.c
>> > @@ -1368,6 +1368,10 @@ static void init_physical_ctx(struct engine_node
>> *node,
>> >
>> >      ovs_assert(br_int && chassis);
>> >
>> > +    struct ovsrec_interface_table *iface_table =
>> > +        (struct ovsrec_interface_table *)EN_OVSDB_GET(
>> > +            engine_get_input("OVS_interface", node));
>> > +
>> >      struct ed_type_ct_zones *ct_zones_data =
>> >          engine_get_input_data("ct_zones", node);
>> >      struct simap *ct_zones = &ct_zones_data->current;
>> > @@ -1377,12 +1381,14 @@ static void init_physical_ctx(struct
>> engine_node *node,
>> >      p_ctx->mc_group_table = multicast_group_table;
>> >      p_ctx->br_int = br_int;
>> >      p_ctx->chassis_table = chassis_table;
>> > +    p_ctx->iface_table = iface_table;
>> >      p_ctx->chassis = chassis;
>> >      p_ctx->active_tunnels = &rt_data->active_tunnels;
>> >      p_ctx->local_datapaths = &rt_data->local_datapaths;
>> >      p_ctx->local_lports = &rt_data->local_lports;
>> >      p_ctx->ct_zones = ct_zones;
>> >      p_ctx->mff_ovn_geneve = ed_mff_ovn_geneve->mff_ovn_geneve;
>> > +    p_ctx->local_bindings = &rt_data->local_bindings;
>> >  }
>> >
>> >  static void init_lflow_ctx(struct engine_node *node,
>> > @@ -1790,6 +1796,118 @@ flow_output_port_groups_handler(struct
>> engine_node *node, void *data)
>> >      return _flow_output_resource_ref_handler(node, data,
>> REF_TYPE_PORTGROUP);
>> >  }
>> >
>> > +/* Engine node en_physical_flow_changes indicates whether
>> > + * there is a need to
>> > + *   - recompute only physical flows or
>> > + *   - we can incrementally process the physical flows.
>> > + *
>> > + * en_physical_flow_changes is an input to flow_output engine node.
>> > + * If the engine node 'en_physical_flow_changes' gets updated during
>> > + * engine run, it means the handler for this -
>> > + * flow_output_physical_flow_changes_handler() will either
>> > + *    - recompute the physical flows by calling 'physical_run() or
>> > + *    - incrementlly process some of the changes for physical flow
>> > + *      calculation. Right now we handle OVS interfaces changes
>> > + *      for physical flow computation.
>> > + *
>> > + * When ever a port binding happens, the follow up
>> > + * activity is the zone id allocation for that port binding.
>> > + * With this intermediate engine node, we avoid full recomputation.
>> > + * Instead we do physical flow computation (either full recomputation
>> > + * by calling physical_run() or handling the changes incrementally.
>> > + *
>> > + * Hence this is an intermediate engine node to indicate the
>> > + * flow_output engine to recomputes/compute the physical flows.
>> > + *
>> > + * TODO 1. Ideally this engine node should recompute/compute the
>> physical
>> > + *      flows instead of relegating it to the flow_output node.
>> > + *      But this requires splitting the flow_output node to
>> > + *      logical_flow_output and physical_flow_output.
>> > + *
>> > + * TODO 2. We can further optimise the en_ct_zone changes to
>> > + *         compute the phsyical flows for changed zone ids.
>> > + *
>> > + * TODO 3: physical.c has a global simap -localvif_to_ofport which
>> stores the
>> > + *         local OVS interfaces and the ofport numbers. Ideally this
>> should be
>> > + *         part of the engine data.
>>
>> Nit: inconsistent indentation with TODO 1 above.
>>
>
> Ack. I'll fix it.
>
>
>
>>
>> > + */
>> > +struct ed_type_pfc_data {
>> > +    /* Both these variables are tracked and set in each engine run. */
>> > +    bool recompute_physical_flows;
>> > +    bool ovs_ifaces_changed;
>> > +};
>> > +
>> > +static void
>> > +en_physical_flow_changes_clear_tracked_data(void *data_)
>> > +{
>> > +    struct ed_type_pfc_data *data = data_;
>> > +    data->recompute_physical_flows = false;
>> > +    data->ovs_ifaces_changed = false;
>> > +}
>> > +
>> > +static void *
>> > +en_physical_flow_changes_init(struct engine_node *node OVS_UNUSED,
>> > +                              struct engine_arg *arg OVS_UNUSED)
>> > +{
>> > +    struct ed_type_pfc_data *data = xzalloc(sizeof *data);
>> > +    return data;
>> > +}
>> > +
>> > +static void
>> > +en_physical_flow_changes_cleanup(void *data OVS_UNUSED)
>> > +{
>> > +}
>> > +
>> > +/* Indicate to the flow_output engine that we need to recompute
>> physical
>> > + * flows. */
>> > +static void
>> > +en_physical_flow_changes_run(struct engine_node *node, void *data)
>> > +{
>> > +    struct ed_type_pfc_data *pfc_tdata = data;
>> > +    pfc_tdata->recompute_physical_flows = true;
>> > +    engine_set_node_state(node, EN_UPDATED);
>> > +}
>> > +
>> > +/* There are OVS interface changes. Indicate to the flow_output engine
>> > + * to handle these OVS interface changes for physical flow
>> computations. */
>> > +static bool
>> > +physical_flow_changes_ovs_iface_handler(struct engine_node *node, void
>> *data)
>> > +{
>> > +    struct ed_type_pfc_data *pfc_tdata = data;
>> > +    pfc_tdata->ovs_ifaces_changed = true;
>> > +    engine_set_node_state(node, EN_UPDATED);
>> > +    return true;
>> > +}
>> > +
>> > +static bool
>> > +flow_output_physical_flow_changes_handler(struct engine_node *node,
>> void *data)
>>
>> Nit: I think this function should be moved above, in the section where
>> we define all the flow_output_*_handler() input handlers, e.g., after
>> flow_output_port_groups_handler().
>>
>
> If we move it there, we also need to move the "struct  ed_type_pfc_data"
> definition above
> this function. That's why I put it at the end.
>

I'll move the entire physical_changes engine block above the flow output
engine block
so that the code is organized properly.

Thanks
Numan


>
>
>> > +{
>> > +    struct ed_type_runtime_data *rt_data =
>> > +        engine_get_input_data("runtime_data", node);
>> > +
>> > +    struct ed_type_flow_output *fo = data;
>> > +    struct physical_ctx p_ctx;
>> > +    init_physical_ctx(node, rt_data, &p_ctx);
>> > +
>> > +    engine_set_node_state(node, EN_UPDATED);
>> > +    struct ed_type_pfc_data *pfc_data =
>> > +        engine_get_input_data("physical_flow_changes", node);
>> > +
>> > +    if (pfc_data->recompute_physical_flows) {
>> > +        /* This indicates that we need to recompute the physical
>> flows. */
>> > +        physical_run(&p_ctx, &fo->flow_table);
>> > +        return true;
>> > +    }
>> > +
>> > +    if (pfc_data->ovs_ifaces_changed) {
>> > +        /* There are OVS interface changes. Try to handle them
>> > +         * incrementally. */
>> > +        return physical_handle_ovs_iface_changes(&p_ctx,
>> &fo->flow_table);
>> > +    }
>> > +
>> > +    return true;
>> > +}
>> > +
>> >  struct ovn_controller_exit_args {
>> >      bool *exiting;
>> >      bool *restart;
>> > @@ -1914,6 +2032,8 @@ main(int argc, char *argv[])
>> >      ENGINE_NODE(runtime_data, "runtime_data");
>> >      ENGINE_NODE(mff_ovn_geneve, "mff_ovn_geneve");
>> >      ENGINE_NODE(ofctrl_is_connected, "ofctrl_is_connected");
>> > +    ENGINE_NODE_WITH_CLEAR_TRACK_DATA(physical_flow_changes,
>> > +                                      "physical_flow_changes");
>> >      ENGINE_NODE(flow_output, "flow_output");
>> >      ENGINE_NODE(addr_sets, "addr_sets");
>> >      ENGINE_NODE(port_groups, "port_groups");
>> > @@ -1933,13 +2053,27 @@ main(int argc, char *argv[])
>> >      engine_add_input(&en_port_groups, &en_sb_port_group,
>> >                       port_groups_sb_port_group_handler);
>> >
>> > +    /* Engine node physical_flow_changes indicates whether
>> > +     * we can recompute only physical flows or we can
>> > +     * incrementally process the physical flows.
>> > +     */
>> > +    engine_add_input(&en_physical_flow_changes, &en_ct_zones,
>> > +                     NULL);
>> > +    engine_add_input(&en_physical_flow_changes, &en_ovs_interface,
>> > +                     physical_flow_changes_ovs_iface_handler);
>> > +
>> >      engine_add_input(&en_flow_output, &en_addr_sets,
>> >                       flow_output_addr_sets_handler);
>> >      engine_add_input(&en_flow_output, &en_port_groups,
>> >                       flow_output_port_groups_handler);
>> >      engine_add_input(&en_flow_output, &en_runtime_data, NULL);
>> > -    engine_add_input(&en_flow_output, &en_ct_zones, NULL);
>> >      engine_add_input(&en_flow_output, &en_mff_ovn_geneve, NULL);
>> > +    engine_add_input(&en_flow_output, &en_physical_flow_changes,
>> > +                     flow_output_physical_flow_changes_handler);
>> > +
>> > +    /* We need this input nodes for only data. Hence the noop handler.
>> */
>> > +    engine_add_input(&en_flow_output, &en_ct_zones,
>> engine_noop_handler);
>> > +    engine_add_input(&en_flow_output, &en_ovs_interface,
>> engine_noop_handler);
>> >
>> >      engine_add_input(&en_flow_output, &en_ovs_open_vswitch, NULL);
>> >      engine_add_input(&en_flow_output, &en_ovs_bridge, NULL);
>> > diff --git a/controller/physical.c b/controller/physical.c
>> > index f06313b9d..240ec158e 100644
>> > --- a/controller/physical.c
>> > +++ b/controller/physical.c
>> > @@ -1780,6 +1780,57 @@ physical_run(struct physical_ctx *p_ctx,
>> >      simap_destroy(&new_tunnel_to_ofport);
>> >  }
>> >
>> > +bool
>> > +physical_handle_ovs_iface_changes(struct physical_ctx *p_ctx,
>> > +                                  struct ovn_desired_flow_table
>> *flow_table)
>> > +{
>> > +    const struct ovsrec_interface *iface_rec;
>> > +    OVSREC_INTERFACE_TABLE_FOR_EACH_TRACKED (iface_rec,
>> p_ctx->iface_table) {
>> > +        if (!strcmp(iface_rec->type, "geneve") ||
>> > +            !strcmp(iface_rec->type, "patch")) {
>> > +            return false;
>>
>> What about iface types "vxlan", "stt"? Shouldn't those be treated in the
>> same way as "geneve"?
>>
>
> Thanks. I think we should add that check too.
>
> I'll add it
>
> Thanks
> Numan
>
>
>>
>> Thanks,
>> Dumitru
>>
>> > +        }
>> > +    }
>> > +
>> > +    struct ofpbuf ofpacts;
>> > +    ofpbuf_init(&ofpacts, 0);
>> > +
>> > +    OVSREC_INTERFACE_TABLE_FOR_EACH_TRACKED (iface_rec,
>> p_ctx->iface_table) {
>> > +        const char *iface_id = smap_get(&iface_rec->external_ids,
>> "iface-id");
>> > +        if (!iface_id) {
>> > +            continue;
>> > +        }
>> > +
>> > +        const struct local_binding *lb =
>> > +            local_binding_find(p_ctx->local_bindings, iface_id);
>> > +
>> > +        if (!lb || !lb->pb) {
>> > +            continue;
>> > +        }
>> > +
>> > +        int64_t ofport = iface_rec->n_ofport ? *iface_rec->ofport : 0;
>> > +        if (ovsrec_interface_is_deleted(iface_rec)) {
>> > +            ofctrl_remove_flows(flow_table, &lb->pb->header_.uuid);
>> > +            simap_find_and_delete(&localvif_to_ofport, iface_id);
>> > +        } else {
>> > +            if (!ovsrec_interface_is_new(iface_rec)) {
>> > +                ofctrl_remove_flows(flow_table, &lb->pb->header_.uuid);
>> > +            }
>> > +
>> > +            simap_put(&localvif_to_ofport, iface_id, ofport);
>> > +            consider_port_binding(p_ctx->sbrec_port_binding_by_name,
>> > +                                  p_ctx->mff_ovn_geneve,
>> p_ctx->ct_zones,
>> > +                                  p_ctx->active_tunnels,
>> > +                                  p_ctx->local_datapaths,
>> > +                                  lb->pb, p_ctx->chassis,
>> > +                                  flow_table, &ofpacts);
>> > +        }
>> > +    }
>> > +
>> > +    ofpbuf_uninit(&ofpacts);
>> > +    return true;
>> > +}
>> > +
>> >  bool
>> >  get_tunnel_ofport(const char *chassis_name, char *encap_ip, ofp_port_t
>> *ofport)
>> >  {
>> > diff --git a/controller/physical.h b/controller/physical.h
>> > index dadf84ea1..9ca34436a 100644
>> > --- a/controller/physical.h
>> > +++ b/controller/physical.h
>> > @@ -49,11 +49,13 @@ struct physical_ctx {
>> >      const struct ovsrec_bridge *br_int;
>> >      const struct sbrec_chassis_table *chassis_table;
>> >      const struct sbrec_chassis *chassis;
>> > +    const struct ovsrec_interface_table *iface_table;
>> >      const struct sset *active_tunnels;
>> >      struct hmap *local_datapaths;
>> >      struct sset *local_lports;
>> >      const struct simap *ct_zones;
>> >      enum mf_field_id mff_ovn_geneve;
>> > +    struct shash *local_bindings;
>> >  };
>> >
>> >  void physical_register_ovs_idl(struct ovsdb_idl *);
>> > @@ -63,7 +65,8 @@ void physical_handle_port_binding_changes(struct
>> physical_ctx *,
>> >                                            struct
>> ovn_desired_flow_table *);
>> >  void physical_handle_mc_group_changes(struct physical_ctx *,
>> >                                        struct ovn_desired_flow_table *);
>> > -
>> > +bool physical_handle_ovs_iface_changes(struct physical_ctx *,
>> > +                                       struct ovn_desired_flow_table
>> *);
>> >  bool get_tunnel_ofport(const char *chassis_name, char *encap_ip,
>> >                         ofp_port_t *ofport);
>> >  #endif /* controller/physical.h */
>> >
>>
>> _______________________________________________
>> dev mailing list
>> dev at openvswitch.org
>> https://mail.openvswitch.org/mailman/listinfo/ovs-dev
>>
>>


More information about the dev mailing list