[ovs-dev] [PATCH ovn v4 8/9] binding: Consider plugging of ports on CMS request.

Numan Siddique numans at ovn.org
Thu Sep 16 23:16:35 UTC 2021


On Fri, Sep 3, 2021 at 3:28 PM Frode Nordahl
<frode.nordahl at canonical.com> wrote:
>
> When OVN is linked with an appropriate plugging implementation,
> CMS can request OVN to plug individual lports into the local
> Open vSwitch instance.
>
> The port and instance record will be maintained during the lifetime
> of the lport and it will be removed on release of lport.
>
> Signed-off-by: Frode Nordahl <frode.nordahl at canonical.com>

Hi Frode,

I've a few comments with the approach taken in this patch.
This patch calls the plug_port APIs from binding.c.  This complicates
the already complicated binding.c code and I think plug_port() related stuff
can be handled separately.  I don't see a need for binding.c to be aware of
plugging feature.  For binding.c to claim a port, the ovs interface should
have external_ids:iface-id set to the port binding lport_name.

Below is how I would see this can be done:

1.  Add a new file called - controller/plug.c  which will handle port binding
    changes - plug_handle_port_binding_changes (and possibly ovs
    port/ovs interface changes).

2.  The function plug_handle_port_binding_changes() will iterate through the
     tracked port bindings and if the port binding has the required options set
    (i.e requested-chassis and plug-type) it will call plug_port APIs and create
    or delete OVS ports.  This function can  access the lbinding data
    stored in the runtime data.

3.   For recompute scenarios,  there can be a function - plug_run() which will
     iterate through all the port bindings and create/delete ovs ports.

4.  When the OVS ports are created / deleted in (2),  in the next run,
the binding module
     will claim or release the port binding.  binding.c module
wouldn't need to care
     if the port binding / ovs port is created by the plug module or
by some other
    mechanism.

5.  The functions plug_handle_port_binding_changes() can be either called by
     runtime_data_sb_port_binding_handler() (i.e runtime engine node)
     or another engine node for plug handling can be created which will handle
     port binding  changes (and possibly ovs port/interface changes).
     Similarly for full recompute,  either runtime_data_run() can call
plug_run()
     or a new engine run function can call it.  Having a separate engine node
     seems better but then it needs to have runtime data as input
     to access the lbinding information.

6.  If you think plug.c should handle ovs port/interface changes,  then you
     can add handlers for it too.


What do you think ? Would this proposal work ?   Let me know if
something is not clear
and I can try to explain better.

Thanks
Numan

> ---
>  controller/binding.c    | 247 ++++++++++++++++++++++++++++++++++++++--
>  tests/ovn-controller.at |  31 +++++
>  tests/ovn-macros.at     |   2 +-
>  3 files changed, 272 insertions(+), 8 deletions(-)
>
> diff --git a/controller/binding.c b/controller/binding.c
> index 938e1d81d..ae67ee3fc 100644
> --- a/controller/binding.c
> +++ b/controller/binding.c
> @@ -35,7 +35,9 @@
>  #include "local_data.h"
>  #include "lport.h"
>  #include "ovn-controller.h"
> +#include "ovsport.h"
>  #include "patch.h"
> +#include "plug.h"
>
>  VLOG_DEFINE_THIS_MODULE(binding);
>
> @@ -45,6 +47,8 @@ VLOG_DEFINE_THIS_MODULE(binding);
>   */
>  #define OVN_INSTALLED_EXT_ID "ovn-installed"
>
> +#define OVN_PLUGGED_EXT_ID "ovn-plugged"
> +
>  #define OVN_QOS_TYPE "linux-htb"
>
>  struct qos_queue {
> @@ -71,10 +75,13 @@ binding_register_ovs_idl(struct ovsdb_idl *ovs_idl)
>
>      ovsdb_idl_add_table(ovs_idl, &ovsrec_table_interface);
>      ovsdb_idl_track_add_column(ovs_idl, &ovsrec_interface_col_name);
> +    ovsdb_idl_track_add_column(ovs_idl, &ovsrec_interface_col_type);
>      ovsdb_idl_track_add_column(ovs_idl, &ovsrec_interface_col_external_ids);
>      ovsdb_idl_track_add_column(ovs_idl, &ovsrec_interface_col_bfd);
>      ovsdb_idl_track_add_column(ovs_idl, &ovsrec_interface_col_bfd_status);
>      ovsdb_idl_track_add_column(ovs_idl, &ovsrec_interface_col_status);
> +    ovsdb_idl_track_add_column(ovs_idl, &ovsrec_interface_col_options);
> +    ovsdb_idl_track_add_column(ovs_idl, &ovsrec_interface_col_mtu_request);
>
>      ovsdb_idl_add_table(ovs_idl, &ovsrec_table_qos);
>      ovsdb_idl_add_column(ovs_idl, &ovsrec_qos_col_type);
> @@ -1090,6 +1097,51 @@ release_binding_lport(const struct sbrec_chassis *chassis_rec,
>      return true;
>  }
>
> +static void
> +consider_unplug_lport(const struct sbrec_port_binding *pb,
> +                      struct binding_ctx_in *b_ctx_in,
> +                      struct local_binding *lbinding)
> +{
> +    const char *plug_type = NULL;
> +    if (lbinding && lbinding->iface) {
> +        plug_type = smap_get(&lbinding->iface->external_ids,
> +                             OVN_PLUGGED_EXT_ID);
> +    }
> +
> +    if (plug_type) {
> +        const struct ovsrec_port *port = ovsport_lookup_by_interface(
> +                b_ctx_in->ovsrec_port_by_interfaces,
> +                (struct ovsrec_interface *) lbinding->iface);
> +        if (port) {
> +            const struct plug_class *plug;
> +            if (!(plug = plug_get_provider(plug_type))) {
> +                static struct vlog_rate_limit rl = VLOG_RATE_LIMIT_INIT(5, 1);
> +                VLOG_WARN_RL(&rl,
> +                             "Unable to open plug provider for "
> +                             "plug-type: '%s' lport %s",
> +                             plug_type, pb->logical_port);
> +                return;
> +            }
> +            const struct plug_port_ctx_in plug_ctx_in = {
> +                    .op_type = PLUG_OP_REMOVE,
> +                    .ovs_table = b_ctx_in->ovs_table,
> +                    .br_int = b_ctx_in->br_int,
> +                    .lport_name = (const char *)pb->logical_port,
> +                    .lport_options = (const struct smap *)&pb->options,
> +                    .iface_name = lbinding->iface->name,
> +                    .iface_type = lbinding->iface->type,
> +                    .iface_options = &lbinding->iface->options,
> +            };
> +            plug_port_prepare(plug, &plug_ctx_in, NULL);
> +            VLOG_INFO("Unplugging port %s from %s for lport %s on this "
> +                      "chassis.",
> +                      port->name, b_ctx_in->br_int->name, pb->logical_port);
> +            ovsport_remove(b_ctx_in->br_int, port);
> +            plug_port_finish(plug, &plug_ctx_in, NULL);
> +        }
> +    }
> +}
> +
>  static bool
>  consider_vif_lport_(const struct sbrec_port_binding *pb,
>                      bool can_bind,
> @@ -1141,15 +1193,184 @@ consider_vif_lport_(const struct sbrec_port_binding *pb,
>      if (pb->chassis == b_ctx_in->chassis_rec) {
>          /* Release the lport if there is no lbinding. */
>          if (!lbinding_set || !can_bind) {
> -            return release_lport(pb, !b_ctx_in->ovnsb_idl_txn,
> -                                 b_ctx_out->tracked_dp_bindings,
> -                                 b_ctx_out->if_mgr);
> +            bool handled = release_lport(pb, !b_ctx_in->ovnsb_idl_txn,
> +                                         b_ctx_out->tracked_dp_bindings,
> +                                         b_ctx_out->if_mgr);
> +            if (handled && b_lport && b_lport->lbinding) {
> +                consider_unplug_lport(pb, b_ctx_in, b_lport->lbinding);
> +            }
> +            return handled;
>          }
>      }
>
>      return true;
>  }
>
> +static int64_t
> +get_plug_mtu_request(const struct smap *lport_options)
> +{
> +    return smap_get_int(lport_options, "plug-mtu-request", 0);
> +}
> +
> +static bool
> +consider_plug_lport_create__(const struct plug_class *plug,
> +                             const struct smap *iface_external_ids,
> +                             const struct sbrec_port_binding *pb,
> +                             struct binding_ctx_in *b_ctx_in)
> +{
> +    if (!b_ctx_in->chassis_rec || !b_ctx_in->br_int || !b_ctx_in->ovs_idl_txn)
> +    {
> +        /* Some of our prerequisites are not available, ask for a recompute. */
> +        return false;
> +    }
> +
> +    bool ret = true;
> +    struct plug_port_ctx_in plug_ctx_in = {
> +            .op_type = PLUG_OP_CREATE,
> +            .ovs_table = b_ctx_in->ovs_table,
> +            .br_int = b_ctx_in->br_int,
> +            .lport_name = (const char *)pb->logical_port,
> +            .lport_options = (const struct smap *)&pb->options,
> +    };
> +    struct plug_port_ctx_out plug_ctx_out;
> +
> +    if (!plug_port_prepare(plug, &plug_ctx_in, &plug_ctx_out)) {
> +        static struct vlog_rate_limit rl = VLOG_RATE_LIMIT_INIT(5, 1);
> +        VLOG_INFO_RL(&rl,
> +                     "Not plugging lport %s on direction from plugging "
> +                     "library.",
> +                     pb->logical_port);
> +        ret = false;
> +        goto out;
> +    }
> +
> +    VLOG_INFO("Plugging port %s into %s for lport %s on this "
> +              "chassis.",
> +              plug_ctx_out.name, b_ctx_in->br_int->name,
> +              pb->logical_port);
> +    ovsport_create(b_ctx_in->ovs_idl_txn, b_ctx_in->br_int,
> +                   plug_ctx_out.name, plug_ctx_out.type,
> +                   NULL, iface_external_ids,
> +                   plug_ctx_out.iface_options,
> +                   get_plug_mtu_request(&pb->options));
> +
> +    plug_port_finish(plug, &plug_ctx_in, &plug_ctx_out);
> +
> +out:
> +    plug_port_ctx_destroy(plug, &plug_ctx_in, &plug_ctx_out);
> +
> +    return ret;
> +}
> +
> +static bool
> +consider_plug_lport_update__(const struct plug_class *plug,
> +                             const struct smap *iface_external_ids,
> +                             const struct sbrec_port_binding *pb,
> +                             struct binding_ctx_in *b_ctx_in,
> +                             struct local_binding *lbinding)
> +{
> +    if (!b_ctx_in->chassis_rec || !b_ctx_in->br_int || !b_ctx_in->ovs_idl_txn)
> +    {
> +        /* Some of our prerequisites are not available, ask for a recompute. */
> +        return false;
> +    }
> +
> +    bool ret = true;
> +    struct plug_port_ctx_in plug_ctx_in = {
> +            .op_type = PLUG_OP_CREATE,
> +            .ovs_table = b_ctx_in->ovs_table,
> +            .br_int = b_ctx_in->br_int,
> +            .lport_name = (const char *)pb->logical_port,
> +            .lport_options = (const struct smap *)&pb->options,
> +            .iface_name = lbinding->iface->name,
> +            .iface_type = lbinding->iface->type,
> +            .iface_options = &lbinding->iface->options,
> +    };
> +    struct plug_port_ctx_out plug_ctx_out;
> +
> +    if (!plug_port_prepare(plug, &plug_ctx_in, &plug_ctx_out)) {
> +        static struct vlog_rate_limit rl = VLOG_RATE_LIMIT_INIT(5, 1);
> +        VLOG_INFO_RL(&rl,
> +                     "Not updating lport %s on direction from plugging "
> +                     "library.",
> +                     pb->logical_port);
> +        ret = false;
> +        goto out;
> +    }
> +
> +    if (strcmp(lbinding->iface->name, plug_ctx_out.name)) {
> +        VLOG_WARN("Attempt of incompatible change to existing "
> +                  "port detected, please recreate port: %s",
> +                   pb->logical_port);
> +        ret = false;
> +        goto out;
> +    }
> +    VLOG_DBG("updating iface for: %s", pb->logical_port);
> +    ovsport_update_iface(lbinding->iface, plug_ctx_out.type,
> +                         iface_external_ids,
> +                         NULL,
> +                         plug_ctx_out.iface_options,
> +                         plug_get_maintained_iface_options(plug),
> +                         get_plug_mtu_request(&pb->options));
> +
> +    plug_port_finish(plug, &plug_ctx_in, &plug_ctx_out);
> +out:
> +    plug_port_ctx_destroy(plug, &plug_ctx_in, &plug_ctx_out);
> +
> +    return ret;
> +}
> +
> +static bool
> +consider_plug_lport(const struct sbrec_port_binding *pb,
> +                    struct binding_ctx_in *b_ctx_in,
> +                    struct local_binding *lbinding)
> +{
> +    bool ret = true;
> +    if (can_bind_on_this_chassis(b_ctx_in->chassis_rec, pb)) {
> +        const char *plug_type = smap_get(&pb->options, "plug-type");
> +        if (!plug_type) {
> +            /* Nothing for us to do and we don't need a recompute. */
> +            return true;
> +        }
> +
> +        static struct vlog_rate_limit rl = VLOG_RATE_LIMIT_INIT(5, 1);
> +        const struct plug_class *plug;
> +        if (!(plug = plug_get_provider(plug_type))) {
> +            VLOG_WARN_RL(&rl,
> +                         "Unable to open plug provider for plug-type: '%s' "
> +                         "lport %s",
> +                         plug_type, pb->logical_port);
> +            /* While we are unable to handle this, asking for a recompute will
> +             * not change that fact. */
> +            return true;
> +        }
> +        const struct smap iface_external_ids = SMAP_CONST2(
> +                &iface_external_ids,
> +                OVN_PLUGGED_EXT_ID, plug_type,
> +                "iface-id", pb->logical_port);
> +        if (lbinding && lbinding->iface) {
> +            if (!smap_get(&lbinding->iface->external_ids,
> +                          OVN_PLUGGED_EXT_ID))
> +            {
> +                VLOG_WARN_RL(&rl,
> +                             "CMS requested plugging of lport %s, but a port "
> +                             "that is not maintained by OVN already exsist "
> +                             "in local vSwitch: "UUID_FMT,
> +                             pb->logical_port,
> +                             UUID_ARGS(&lbinding->iface->header_.uuid));
> +                return false;
> +            }
> +            ret = consider_plug_lport_update__(plug, &iface_external_ids, pb,
> +                                               b_ctx_in, lbinding);
> +        } else {
> +            ret = consider_plug_lport_create__(plug, &iface_external_ids, pb,
> +                                               b_ctx_in);
> +        }
> +    }
> +
> +    return ret;
> +}
> +
>  static bool
>  consider_vif_lport(const struct sbrec_port_binding *pb,
>                     struct binding_ctx_in *b_ctx_in,
> @@ -1187,8 +1408,13 @@ consider_vif_lport(const struct sbrec_port_binding *pb,
>          }
>      }
>
> -    return consider_vif_lport_(pb, can_bind, b_ctx_in, b_ctx_out,
> -                               b_lport, qos_map);
> +    /* Consider if we should create or update local port/interface record for
> +     * this lport.  Note that a newly created port/interface will get its flows
> +     * installed on the next loop iteration as we won't wait for OVS vSwitchd
> +     * to configure and assign a ofport to the interface. */
> +    return consider_plug_lport(pb, b_ctx_in, lbinding)
> +           & consider_vif_lport_(pb, can_bind, b_ctx_in, b_ctx_out,
> +                                 b_lport, qos_map);
>  }
>
>  static bool
> @@ -2111,8 +2337,11 @@ handle_deleted_vif_lport(const struct sbrec_port_binding *pb,
>          lbinding = b_lport->lbinding;
>          bound = is_binding_lport_this_chassis(b_lport, b_ctx_in->chassis_rec);
>
> -         /* Remove b_lport from local_binding. */
> -         binding_lport_delete(binding_lports, b_lport);
> +        if (b_lport->lbinding) {
> +            consider_unplug_lport(pb, b_ctx_in, b_lport->lbinding);
> +        }
> +        /* Remove b_lport from local_binding. */
> +        binding_lport_delete(binding_lports, b_lport);
>      }
>
>      if (bound && lbinding && lport_type == LP_VIF) {
> @@ -2690,6 +2919,10 @@ local_binding_handle_stale_binding_lports(struct local_binding *lbinding,
>              handled = release_binding_lport(b_ctx_in->chassis_rec, b_lport,
>                                              !b_ctx_in->ovnsb_idl_txn,
>                                              b_ctx_out);
> +            if (handled && b_lport && b_lport->lbinding) {
> +                consider_unplug_lport(b_lport->pb, b_ctx_in,
> +                                      b_lport->lbinding);
> +            }
>              binding_lport_delete(&b_ctx_out->lbinding_data->lports,
>                                   b_lport);
>          }
> diff --git a/tests/ovn-controller.at b/tests/ovn-controller.at
> index 4ae218ed6..a38884374 100644
> --- a/tests/ovn-controller.at
> +++ b/tests/ovn-controller.at
> @@ -723,3 +723,34 @@ AT_CHECK([as hv1 ovs-ofctl dump-flows br-int | grep controller | grep userdata=0
>  OVN_CLEANUP([hv1])
>  AT_CLEANUP
>  ])
> +
> +OVN_FOR_EACH_NORTHD([
> +AT_SETUP([ovn-controller - plugging])
> +AT_KEYWORDS([plugging])
> +
> +ovn_start
> +
> +net_add n1
> +sim_add hv1
> +ovs-vsctl add-br br-phys
> +ovn_attach n1 br-phys 192.168.0.1
> +
> +check ovn-nbctl ls-add lsw0
> +check ovn-nbctl lsp-add lsw0 lsp1
> +check ovn-nbctl lsp-set-addresses lsp1 "f0:00:00:00:00:01 172.16.0.100"
> +check ovn-nbctl --wait=sb set Logical_Switch_Port lsp1 \
> +    options:requested-chassis=hv1 \
> +    options:plug-type=dummy \
> +    options:plug-mtu-request=42
> +
> +wait_column "true" Port_Binding up logical_port=lsp1
> +
> +as hv1
> +
> +AT_CHECK([as hv1 ovs-vsctl find interface name=lsp1 options:plug-dummy-option=value | grep -q "options.*value"])
> +AT_CHECK([as hv1 ovs-vsctl find interface name=lsp1 mtu_request=42 | grep -q "mtu_request.*42"])
> +
> +OVN_CLEANUP([hv1])
> +AT_CLEANUP
> +])
> +
> diff --git a/tests/ovn-macros.at b/tests/ovn-macros.at
> index f06f2e68e..b3c2d72fa 100644
> --- a/tests/ovn-macros.at
> +++ b/tests/ovn-macros.at
> @@ -327,7 +327,7 @@ ovn_az_attach() {
>          -- --may-exist add-br br-int \
>          -- set bridge br-int fail-mode=secure other-config:disable-in-band=true \
>          || return 1
> -    start_daemon ovn-controller || return 1
> +    start_daemon ovn-controller --enable-dummy-plug || return 1
>  }
>
>  # ovn_attach NETWORK BRIDGE IP [MASKLEN]
> --
> 2.32.0
>
> _______________________________________________
> dev mailing list
> dev at openvswitch.org
> https://mail.openvswitch.org/mailman/listinfo/ovs-dev
>


More information about the dev mailing list