[ovs-dev] [PATCH ovn v3 7/7] binding: Consider plugging of ports on CMS request.
Han Zhou
zhouhan at gmail.com
Fri Aug 27 19:32:36 UTC 2021
On Fri, Aug 27, 2021 at 2:17 AM Frode Nordahl <frode.nordahl at canonical.com>
wrote:
>
> On Fri, Aug 27, 2021 at 9:29 AM Han Zhou <zhouhan at gmail.com> wrote:
> >
> >
> >
> > On Thu, Aug 19, 2021 at 4:09 AM 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>
> > > ---
> > > controller/binding.c | 269
+++++++++++++++++++++++++++++++++++++++-
> > > tests/ovn-controller.at | 31 +++++
> > > tests/ovn-macros.at | 2 +-
> > > 3 files changed, 296 insertions(+), 6 deletions(-)
> > >
> > > diff --git a/controller/binding.c b/controller/binding.c
> > > index 52eb47b79..7499fd9a2 100644
> > > --- a/controller/binding.c
> > > +++ b/controller/binding.c
> > > @@ -23,6 +23,7 @@
> > > #include "lib/netdev.h"
> > > #include "lib/vswitch-idl.h"
> > > #include "openvswitch/hmap.h"
> > > +#include "openvswitch/uuid.h"
> > > #include "openvswitch/vlog.h"
> > >
> > > /* OVN includes. */
> > > @@ -35,7 +36,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 +48,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 +76,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);
> > > @@ -1046,6 +1054,18 @@ is_binding_lport_this_chassis(struct
binding_lport *b_lport,
> > > b_lport->pb->chassis == chassis);
> > > }
> > >
> > > +static bool
> > > +chassis_uses_dpdk(const struct ovsrec_open_vswitch_table *ovs_table,
> > > + const struct ovsrec_bridge *br_int)
> > > +{
> > > + const struct ovsrec_open_vswitch *cfg;
> > > +
> > > + cfg = ovsrec_open_vswitch_table_first(ovs_table);
> > > +
> > > + return (cfg && cfg->dpdk_initialized &&
> > > + !strcmp(br_int->datapath_type, "netdev"));
> > > +}
> > > +
> > > static bool
> > > can_bind_on_this_chassis(const struct sbrec_chassis *chassis_rec,
> > > const char *requested_chassis)
> > > @@ -1055,6 +1075,15 @@ can_bind_on_this_chassis(const struct
sbrec_chassis *chassis_rec,
> > > || !strcmp(requested_chassis, chassis_rec->hostname);
> > > }
> > >
> > > +static bool
> > > +can_plug_on_this_chassis(const struct sbrec_chassis *chassis_rec,
> > > + const struct sbrec_port_binding *pb)
> > > +{
> > > + return (pb && chassis_rec && pb->plugged_by
> > > + && uuid_equals(&chassis_rec->header_.uuid,
> > > + &pb->plugged_by->header_.uuid));
> >
> > Could it be just chassis_rec == pb->plugged_by?
>
> Ah, indeed, thx!
>
> > > +}
> > > +
> > > /* Returns 'true' if the 'lbinding' has binding lports of type
LP_CONTAINER,
> > > * 'false' otherwise. */
> > > static bool
> > > @@ -1088,6 +1117,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) {
> > > + struct plug *plug;
> > > + if (plug_open(plug_type, &plug)) {
> > > + 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,
> > > + .use_dpdk =
chassis_uses_dpdk(b_ctx_in->ovs_table,
> > > + 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, const char *vif_chassis,
> > > @@ -1138,15 +1212,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);
> >
> > Is this supposed to revert the plug? I wonder why would this be needed.
The can_bind and can_plug should be consistent, since they come from the
same NB configuration. (this leads to the necessity of avoiding the
redundant information in SB)
>
> I have added a call to consider_unplug_lport whenever a VIF type lport
> is released, this is to make sure that if we have plugged it we should
> also unplug it when the lport is no longer for this chassis. I will
> revisit if it is needed.
>
> > > + }
> > > + 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 *plug,
> > > + const struct smap *iface_external_ids,
> > > + const struct sbrec_port_binding *pb,
> > > + struct binding_ctx_in *b_ctx_in)
> > > +{
> > > + bool ret = true;
> > > + struct plug_port_ctx_in plug_ctx_in = {
> > > + .op_type = PLUG_OP_CREATE,
> > > + .use_dpdk = chassis_uses_dpdk(b_ctx_in->ovs_table,
> > > + 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 *plug,
> > > + const struct smap *iface_external_ids,
> > > + const struct sbrec_port_binding *pb,
> > > + struct binding_ctx_in *b_ctx_in,
> > > + struct local_binding *lbinding)
> > > +{
> > > + bool ret = true;
> > > + struct plug_port_ctx_in plug_ctx_in = {
> > > + .op_type = PLUG_OP_CREATE,
> > > + .use_dpdk = chassis_uses_dpdk(b_ctx_in->ovs_table,
> > > + 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_class_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
> >
> > The return bool seems unused. For the consider_vif_lport(), returning
false means when it is called by incremental processing handlers it cannot
handle the changes incrementally, which means the incremental processing
engine needs recompute in the next round. I think same principle should
apply here. Otherwise, if for example ovs_idl_txn is NULL, then the
interface is not plugged, and it may not be triggered soon. If it returns
false out to the incremental handler, it will force recompute which would
make sure it is plugged.
>
> Thank you for the detail on how the return is used by the incremental
> engine, I will revisit how it is used in the added functions and make
> sure they comply to with this contract and also make sure to consider
> returns from both consider_plug_lport and consider_vif_lport_ before
> returning from consider_vif_lport().
>
> > > +consider_plug_lport(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)
> > > + {
> > > + return false;
> > > + }
> > > + bool ret = true;
> > > +
> > > + if (can_plug_on_this_chassis(b_ctx_in->chassis_rec, pb)) {
> > > + static struct vlog_rate_limit rl = VLOG_RATE_LIMIT_INIT(5,
1);
> > > + const char *plug_type = smap_get(&pb->options, "plug-type");
> > > + if (!plug_type) {
> > > + /* This should never happen, but better safe than sorry
*/
> > > + VLOG_WARN_RL(&rl,
> > > + "Possible database inconsistency detected: "
> > > + "Port_Binding->plugged_by points at this
chassis, "
> > > + "but Port_Binding->options:plug-type not
set. "
> > > + "lport %s",
> > > + pb->logical_port);
> > > + return false;
> > > + }
> > > +
> > > + struct plug *plug;
> > > + if (plug_open(plug_type, &plug)) {
> > > + VLOG_WARN_RL(&rl,
> > > + "Unable to open plug provider for
plug-type: '%s' "
> > > + "lport %s",
> > > + plug_type, pb->logical_port);
> > > + return false;
> > > + }
> > > + 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));
> > > + ret = false;
> > > + goto out;
> > > + }
> > > + 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);
> > > + }
> > > + }
> > > +
> > > +out:
> > > + return ret;
> > > +}
> > > +
> > > static bool
> > > consider_vif_lport(const struct sbrec_port_binding *pb,
> > > struct binding_ctx_in *b_ctx_in,
> > > @@ -1170,6 +1413,11 @@ consider_vif_lport(const struct
sbrec_port_binding *pb,
> > > b_lport = local_binding_add_lport(binding_lports, lbinding,
pb, LP_VIF);
> > > }
> > >
> > > + /* 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. */
> > > + consider_plug_lport(pb, b_ctx_in, lbinding);
> > > return consider_vif_lport_(pb, can_bind, vif_chassis, b_ctx_in,
> > > b_ctx_out, b_lport, qos_map);
> > > }
> > > @@ -1563,6 +1811,10 @@ binding_run(struct binding_ctx_in *b_ctx_in,
struct binding_ctx_out *b_ctx_out)
> > > const struct sbrec_port_binding *pb;
> > > };
> > >
> > > + /* For any open plug provider instances, perform periodic
non-blocking
> > > + * maintenance */
> > > + plug_run_instances();
> >
> > I am not clear about the purpose of this function. Plugging and
unplugging interfaces are performed in consider_plug/unplug_lport, so what
else is needed? Is it supposed to iterate the local VIFs and see if some
interfaces should be unplugged? If so, should it be handled directly with a
loop here and call the consider_unplug_lport()? I just don't see a need for
the callback "plug_run()".
>
> As you have pointed out earlier I omitted to add the documentation, so
> this being unclear is my fault :)
>
> The plugging provider may have a need for maintaining its own data
> structures for lookup.
>
> A concrete example is for the representor plugin. I am going to do a
> dump of devlink port data on plugin registration and also open a
> non-blocking netlink socket to monitor for any changes. Then at each
> call to plug_run() the plugin will check if there is any data
> available, and update the lookup map accordingly. For the current
> legacy SR-IOV workflow this may appear overkill as each card can only
> support a limited number of VFs, but I'm anticipating a possible
> future where we will see much higher densities with mediated devices.
>
> Even with the legacy SR-IOV workflow we need to be able to act on
> system runtime changes, the ovn-controller can start before the VFs
> are created, and the system administrator may also change the number
> of VFs at runtime.
>
Thanks! This is a good example and explains why plug_run() is needed.
However, calling it in binding_run() is not enough to fulfil this purpose.
binding_run() is called only at full-recompute of ovn-controller. To react
to the changes specific to a provider, we need to define a new input to the
I-P engine, and implement change handlers by calling some callbacks of the
plugging provider.
> Thanks!
>
> --
> Frode Nordahl
>
> > Thanks,
> > Han
> >
> > > +
> > > /* Run through each binding record to see if it is resident on
this
> > > * chassis and update the binding accordingly. This includes
both
> > > * directly connected logical ports and children of those ports
> > > @@ -2097,8 +2349,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) {
> > > @@ -2676,6 +2931,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 e8550a5dc..c7f61457b 100644
> > > --- a/tests/ovn-controller.at
> > > +++ b/tests/ovn-controller.at
> > > @@ -692,3 +692,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 b5a01b2b5..f26e84f3d 100644
> > > --- a/tests/ovn-macros.at
> > > +++ b/tests/ovn-macros.at
> > > @@ -325,7 +325,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