| Message ID | 20210903192748.1408062-9-frode.nordahl@canonical.com |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | Introduce infrastructure for plugging providers | expand |
| Context | Check | Description |
|---|---|---|
| ovsrobot/apply-robot | success | apply and check: success |
On Fri, Sep 3, 2021 at 3:28 PM Frode Nordahl <frode.nordahl@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@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@openvswitch.org > https://mail.openvswitch.org/mailman/listinfo/ovs-dev >
On Fri, Sep 17, 2021 at 1:16 AM Numan Siddique <numans@ovn.org> wrote: > > On Fri, Sep 3, 2021 at 3:28 PM Frode Nordahl > <frode.nordahl@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@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. 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. Cheers!
On Fri, Sep 17, 2021 at 12:41 PM Frode Nordahl <frode.nordahl@canonical.com> wrote: > > On Fri, Sep 17, 2021 at 1:16 AM Numan Siddique <numans@ovn.org> wrote: > > > > On Fri, Sep 3, 2021 at 3:28 PM Frode Nordahl > > <frode.nordahl@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@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 > > > dev@openvswitch.org > > > https://mail.openvswitch.org/mailman/listinfo/ovs-dev > > > > _______________________________________________ > dev mailing list > dev@openvswitch.org > https://mail.openvswitch.org/mailman/listinfo/ovs-dev >
On Fri, Sep 17, 2021 at 9:22 PM Numan Siddique <numans@ovn.org> wrote: > > On Fri, Sep 17, 2021 at 12:41 PM Frode Nordahl > <frode.nordahl@canonical.com> wrote: > > > > On Fri, Sep 17, 2021 at 1:16 AM Numan Siddique <numans@ovn.org> wrote: > > > > > > On Fri, Sep 3, 2021 at 3:28 PM Frode Nordahl > > > <frode.nordahl@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@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. Great. I'll have something up for review tomorrow or early next week, and we'll see where we get to. Thanks!
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]
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@canonical.com> --- controller/binding.c | 247 ++++++++++++++++++++++++++++++++++++++-- tests/ovn-controller.at | 31 +++++ tests/ovn-macros.at | 2 +- 3 files changed, 272 insertions(+), 8 deletions(-)