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. 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