On Fri, Sep 17, 2021 at 12:41 PM Frode Nordahl
<frode.nord...@canonical.com> wrote:
>
> On Fri, Sep 17, 2021 at 1:16 AM Numan Siddique <num...@ovn.org> wrote:
> >
> > On Fri, Sep 3, 2021 at 3:28 PM Frode Nordahl
> > <frode.nord...@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.nord...@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.
>
> Numan, thank you so much for the review and for providing such a
> detailed and complete counter proposal, this is most appreciated.
>
> I agree with you completely and have started working on such an approach.
Great. Thanks.
>
> This does mean I will most likely miss the 21.09 release with this
> series, which is unfortunate for me, but that's ok. Better to get it
> into a great shape for the next one than taking risks or cutting
> corners for this one.
If the patches look good before the official release, then I've no objection
to backporting to the newly created branch.
Thanks
Numan
>
> Cheers!
>
> --
> Frode Nordahl
>
> > 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
> > > d...@openvswitch.org
> > > https://mail.openvswitch.org/mailman/listinfo/ovs-dev
> > >
> _______________________________________________
> dev mailing list
> d...@openvswitch.org
> https://mail.openvswitch.org/mailman/listinfo/ovs-dev
>
_______________________________________________
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
