[ovs-dev,RFC] localport type support

Submitted by Daniel Alvarez Sanchez on April 19, 2017, 3:35 p.m.

Details

Message ID 1492616134-9890-1-git-send-email-dalvarez@redhat.com
State New
Headers show

Commit Message

Daniel Alvarez Sanchez April 19, 2017, 3:35 p.m.
This patch introduces a new type of OVN ports called "localport".
These ports will be present in every hypervisor and may have the
same IP/MAC addresses. They are not bound to any chassis and traffic
to these ports will never go through a tunnel.

Its main use case is the metadata API support which relies on a local
agent running on every hypervisor and serving metadata to VM's locally.
This service is described in detail at [0].

TODO: write a test case for localport ports

[0] https://review.openstack.org/#/c/452811/

Signed-off-by: Daniel Alvarez <dalvarez@redhat.com>
---
 ovn/controller/binding.c  | 84 +++++++++++++++++++++++++++++++++++++++--------
 ovn/controller/physical.c |  6 ++++
 ovn/northd/ovn-northd.c   |  6 ++--
 3 files changed, 80 insertions(+), 16 deletions(-)

Comments

Russell Bryant April 19, 2017, 5:12 p.m.
Minor thing ... When you send an "[RFC]" patch, it's customary to
remove "[PATCH]".

Overall this looks like a great start.  The biggest things missing are
the test code and documentation.

I'd also be interested in other opinions about whether we should
explicitly add flows to drop traffic to a remote destination (more
comments about that inline).

On Wed, Apr 19, 2017 at 11:35 AM, Daniel Alvarez <dalvarez@redhat.com> wrote:
> This patch introduces a new type of OVN ports called "localport".
> These ports will be present in every hypervisor and may have the
> same IP/MAC addresses. They are not bound to any chassis and traffic
> to these ports will never go through a tunnel.

I believe in the current implementation, traffic that originates from
a "localport" with a remote destination *will* go over a tunnel.  Any
response to that traffic would go to the local instance of that
"localport", though so it would never work properly.

We could expand this patch to explicitly drop packets from a
"localport" with a remote destination.  I don't think it would be too
difficult.  One way would be to add a high priority flow to drop
packets in the table that sends packets to remote destinations that
matches on the input port for each localnet port.  I don't think the
traffic over a tunnel would be harmful, but it's wasteful since we
don't expect it to ever do something useful.

Another option is to just address this with documentation.  We only
expect a "localport" to generate traffic destined for a local
destination, typically in response to a request it received.

Perhaps if it's easy enough to add those flows, we should do it in
addition to clearly documenting the expected usage and that remote
destinations will never work.

>
> Its main use case is the metadata API support which relies on a local

I might clarify that it's the OpenStack metadata API we're talking about here.

> agent running on every hypervisor and serving metadata to VM's locally.
> This service is described in detail at [0].
>
> TODO: write a test case for localport ports

We'll also need documentation for this.  Searching for "localnet" will
give you some hints about places where we've documented that port
type.

>
> [0] https://review.openstack.org/#/c/452811/
>
> Signed-off-by: Daniel Alvarez <dalvarez@redhat.com>
> ---
>  ovn/controller/binding.c  | 84 +++++++++++++++++++++++++++++++++++++++--------
>  ovn/controller/physical.c |  6 ++++
>  ovn/northd/ovn-northd.c   |  6 ++--
>  3 files changed, 80 insertions(+), 16 deletions(-)
>
> diff --git a/ovn/controller/binding.c b/ovn/controller/binding.c
> index 95e9deb..1eff3d4 100644
> --- a/ovn/controller/binding.c
> +++ b/ovn/controller/binding.c
> @@ -352,6 +352,43 @@ setup_qos(const char *egress_iface, struct hmap *queue_map)
>      hmap_destroy(&consistent_queues);
>      netdev_close(netdev_phy);
>  }
> +static bool
> +set_ovsport_external_ids(struct controller_ctx *ctx,
> +                         const struct ovsrec_bridge *bridge,
> +                         const struct ovsrec_interface *iface_rec,
> +                         const char *key,
> +                         const char *value)
> +{
> +    if (!ctx->ovs_idl_txn) {
> +        return false;
> +    }
> +
> +    /* Find the port with this interface and add the key/value pair to its
> +     * external_ids. Assume 1-1 relationship between port and interface. */

I don't think the following code makes the 1-1 relationship assumption.

> +    int i;
> +    for (i = 0; i < bridge->n_ports; i++) {
> +        const struct ovsrec_port *port_rec = bridge->ports[i];
> +        int j;
> +
> +        if (!strcmp(port_rec->name, bridge->name)) {
> +            continue;
> +        }
> +
> +        for (j = 0; j < port_rec->n_interfaces; j++) {
> +            if (port_rec->interfaces[j] == iface_rec) {
> +                struct smap new_ids;
> +                smap_clone(&new_ids, &port_rec->external_ids);
> +                smap_replace(&new_ids, key, value);
> +                ovsrec_port_verify_external_ids(port_rec);
> +                ovsrec_port_set_external_ids(port_rec, &new_ids);
> +                smap_destroy(&new_ids);
> +                return true;
> +            }
> +        }
> +    }
> +    return false;
> +}
> +
>
>  static void
>  consider_local_datapath(struct controller_ctx *ctx,
> @@ -359,6 +396,7 @@ consider_local_datapath(struct controller_ctx *ctx,
>                          const struct lport_index *lports,
>                          const struct sbrec_chassis *chassis_rec,
>                          const struct sbrec_port_binding *binding_rec,
> +                        const struct ovsrec_bridge *bridge,
>                          struct hmap *qos_map,
>                          struct hmap *local_datapaths,
>                          struct shash *lport_to_iface,
> @@ -368,19 +406,37 @@ consider_local_datapath(struct controller_ctx *ctx,
>          = shash_find_data(lport_to_iface, binding_rec->logical_port);
>
>      bool our_chassis = false;
> -    if (iface_rec
> -        || (binding_rec->parent_port && binding_rec->parent_port[0] &&
> -            sset_contains(local_lports, binding_rec->parent_port))) {
> -        if (binding_rec->parent_port && binding_rec->parent_port[0]) {
> -            /* Add child logical port to the set of all local ports. */
> -            sset_add(local_lports, binding_rec->logical_port);
> -        }
> -        add_local_datapath(ldatapaths, lports, binding_rec->datapath,
> -                           false, local_datapaths);
> -        if (iface_rec && qos_map && ctx->ovs_idl_txn) {
> -            get_qos_params(binding_rec, qos_map);
> +
> +    if (iface_rec && !strcmp(binding_rec->type, "localport")) {
> +        /* Make sure localport external_id is present, otherwise set it
> +            * for both interface and port. This should only happen the first
> +            * time. */

I would add some more explanation about why the code is setting it on
both the interface and port.  Something like ...

We set the value on both the interface and port because it's most
convenient to check for the value here on the interface.  Later, we
need to read this value in physical.c, and expect to read it from the
port there.

> +        if (!smap_get(&iface_rec->external_ids, "ovn-localport-port")) {
> +            if (set_ovsport_external_ids(ctx, bridge, iface_rec,
> +                                         "ovn-localport-port",
> +                                         binding_rec->logical_port)) {
> +                struct smap new_ids;
> +                smap_clone(&new_ids, &iface_rec->external_ids);
> +                smap_replace(&new_ids, "ovn-localport-port",
> +                             binding_rec->logical_port);
> +                ovsrec_interface_verify_external_ids(iface_rec);
> +                ovsrec_interface_set_external_ids(iface_rec, &new_ids);
> +                smap_destroy(&new_ids);
> +            }
>          }
> -        our_chassis = true;
> +    } else if (iface_rec
> +               || (binding_rec->parent_port && binding_rec->parent_port[0] &&
> +                   sset_contains(local_lports, binding_rec->parent_port))) {
> +               if (binding_rec->parent_port && binding_rec->parent_port[0]) {
> +                   /* Add child logical port to the set of all local ports. */
> +                   sset_add(local_lports, binding_rec->logical_port);
> +               }
> +               add_local_datapath(ldatapaths, lports, binding_rec->datapath,
> +                                  false, local_datapaths);
> +               if (iface_rec && qos_map && ctx->ovs_idl_txn) {
> +                   get_qos_params(binding_rec, qos_map);
> +               }
> +               our_chassis = true;
>      } else if (!strcmp(binding_rec->type, "l2gateway")) {
>          const char *chassis_id = smap_get(&binding_rec->options,
>                                            "l2gateway-chassis");
> @@ -468,8 +524,8 @@ binding_run(struct controller_ctx *ctx, const struct ovsrec_bridge *br_int,
>          consider_local_datapath(ctx, ldatapaths, lports,
>                                  chassis_rec, binding_rec,
>                                  sset_is_empty(&egress_ifaces) ? NULL :
> -                                &qos_map, local_datapaths, &lport_to_iface,
> -                                local_lports);
> +                                br_int, &qos_map, local_datapaths,
> +                                &lport_to_iface, local_lports);
>
>      }
>
> diff --git a/ovn/controller/physical.c b/ovn/controller/physical.c
> index 0f1aa63..187bff5 100644
> --- a/ovn/controller/physical.c
> +++ b/ovn/controller/physical.c
> @@ -784,6 +784,8 @@ physical_run(struct controller_ctx *ctx, enum mf_field_id mff_ovn_geneve,
>                                          "ovn-localnet-port");
>          const char *l2gateway = smap_get(&port_rec->external_ids,
>                                          "ovn-l2gateway-port");
> +        const char *localport = smap_get(&port_rec->external_ids,
> +                                         "ovn-localport-port");
>
>          for (int j = 0; j < port_rec->n_interfaces; j++) {
>              const struct ovsrec_interface *iface_rec = port_rec->interfaces[j];
> @@ -808,6 +810,10 @@ physical_run(struct controller_ctx *ctx, enum mf_field_id mff_ovn_geneve,
>                  /* L2 gateway patch ports can be handled just like VIFs. */
>                  simap_put(&new_localvif_to_ofport, l2gateway, ofport);
>                  break;
> +            } else if (localport) {
> +                /* localport ports can be handled just like VIFs. */
> +                simap_put(&new_localvif_to_ofport, localport, ofport);
> +                break;
>              } else if (chassis_id) {
>                  enum chassis_tunnel_type tunnel_type;
>                  if (!strcmp(iface_rec->type, "geneve")) {
> diff --git a/ovn/northd/ovn-northd.c b/ovn/northd/ovn-northd.c
> index 5a2e5ab..09f55c8 100644
> --- a/ovn/northd/ovn-northd.c
> +++ b/ovn/northd/ovn-northd.c
> @@ -3166,9 +3166,11 @@ build_lswitch_flows(struct hmap *datapaths, struct hmap *ports,
>          /*
>           * Add ARP/ND reply flows if either the
>           *  - port is up or
> -         *  - port type is router
> +         *  - port type is router or
> +         *  - port type is localport
>           */
> -        if (!lsp_is_up(op->nbsp) && strcmp(op->nbsp->type, "router")) {
> +        if (!lsp_is_up(op->nbsp) && strcmp(op->nbsp->type, "router") &&
> +            strcmp(op->nbsp->type, "localport")) {
>              continue;
>          }
>
> --
> 1.8.3.1
>
> _______________________________________________
> dev mailing list
> dev@openvswitch.org
> https://mail.openvswitch.org/mailman/listinfo/ovs-dev

Patch hide | download patch | download mbox

diff --git a/ovn/controller/binding.c b/ovn/controller/binding.c
index 95e9deb..1eff3d4 100644
--- a/ovn/controller/binding.c
+++ b/ovn/controller/binding.c
@@ -352,6 +352,43 @@  setup_qos(const char *egress_iface, struct hmap *queue_map)
     hmap_destroy(&consistent_queues);
     netdev_close(netdev_phy);
 }
+static bool
+set_ovsport_external_ids(struct controller_ctx *ctx,
+                         const struct ovsrec_bridge *bridge,
+                         const struct ovsrec_interface *iface_rec,
+                         const char *key,
+                         const char *value)
+{
+    if (!ctx->ovs_idl_txn) {
+        return false;
+    }
+
+    /* Find the port with this interface and add the key/value pair to its
+     * external_ids. Assume 1-1 relationship between port and interface. */
+    int i;
+    for (i = 0; i < bridge->n_ports; i++) {
+        const struct ovsrec_port *port_rec = bridge->ports[i];
+        int j;
+
+        if (!strcmp(port_rec->name, bridge->name)) {
+            continue;
+        }
+
+        for (j = 0; j < port_rec->n_interfaces; j++) {
+            if (port_rec->interfaces[j] == iface_rec) {
+                struct smap new_ids;
+                smap_clone(&new_ids, &port_rec->external_ids);
+                smap_replace(&new_ids, key, value);
+                ovsrec_port_verify_external_ids(port_rec);
+                ovsrec_port_set_external_ids(port_rec, &new_ids);
+                smap_destroy(&new_ids);
+                return true;
+            }
+        }
+    }
+    return false;
+}
+
 
 static void
 consider_local_datapath(struct controller_ctx *ctx,
@@ -359,6 +396,7 @@  consider_local_datapath(struct controller_ctx *ctx,
                         const struct lport_index *lports,
                         const struct sbrec_chassis *chassis_rec,
                         const struct sbrec_port_binding *binding_rec,
+                        const struct ovsrec_bridge *bridge,
                         struct hmap *qos_map,
                         struct hmap *local_datapaths,
                         struct shash *lport_to_iface,
@@ -368,19 +406,37 @@  consider_local_datapath(struct controller_ctx *ctx,
         = shash_find_data(lport_to_iface, binding_rec->logical_port);
 
     bool our_chassis = false;
-    if (iface_rec
-        || (binding_rec->parent_port && binding_rec->parent_port[0] &&
-            sset_contains(local_lports, binding_rec->parent_port))) {
-        if (binding_rec->parent_port && binding_rec->parent_port[0]) {
-            /* Add child logical port to the set of all local ports. */
-            sset_add(local_lports, binding_rec->logical_port);
-        }
-        add_local_datapath(ldatapaths, lports, binding_rec->datapath,
-                           false, local_datapaths);
-        if (iface_rec && qos_map && ctx->ovs_idl_txn) {
-            get_qos_params(binding_rec, qos_map);
+
+    if (iface_rec && !strcmp(binding_rec->type, "localport")) {
+        /* Make sure localport external_id is present, otherwise set it
+            * for both interface and port. This should only happen the first
+            * time. */
+        if (!smap_get(&iface_rec->external_ids, "ovn-localport-port")) {
+            if (set_ovsport_external_ids(ctx, bridge, iface_rec,
+                                         "ovn-localport-port",
+                                         binding_rec->logical_port)) {
+                struct smap new_ids;
+                smap_clone(&new_ids, &iface_rec->external_ids);
+                smap_replace(&new_ids, "ovn-localport-port",
+                             binding_rec->logical_port);
+                ovsrec_interface_verify_external_ids(iface_rec);
+                ovsrec_interface_set_external_ids(iface_rec, &new_ids);
+                smap_destroy(&new_ids);
+            }
         }
-        our_chassis = true;
+    } else if (iface_rec
+               || (binding_rec->parent_port && binding_rec->parent_port[0] &&
+                   sset_contains(local_lports, binding_rec->parent_port))) {
+               if (binding_rec->parent_port && binding_rec->parent_port[0]) {
+                   /* Add child logical port to the set of all local ports. */
+                   sset_add(local_lports, binding_rec->logical_port);
+               }
+               add_local_datapath(ldatapaths, lports, binding_rec->datapath,
+                                  false, local_datapaths);
+               if (iface_rec && qos_map && ctx->ovs_idl_txn) {
+                   get_qos_params(binding_rec, qos_map);
+               }
+               our_chassis = true;
     } else if (!strcmp(binding_rec->type, "l2gateway")) {
         const char *chassis_id = smap_get(&binding_rec->options,
                                           "l2gateway-chassis");
@@ -468,8 +524,8 @@  binding_run(struct controller_ctx *ctx, const struct ovsrec_bridge *br_int,
         consider_local_datapath(ctx, ldatapaths, lports,
                                 chassis_rec, binding_rec,
                                 sset_is_empty(&egress_ifaces) ? NULL :
-                                &qos_map, local_datapaths, &lport_to_iface,
-                                local_lports);
+                                br_int, &qos_map, local_datapaths,
+                                &lport_to_iface, local_lports);
 
     }
 
diff --git a/ovn/controller/physical.c b/ovn/controller/physical.c
index 0f1aa63..187bff5 100644
--- a/ovn/controller/physical.c
+++ b/ovn/controller/physical.c
@@ -784,6 +784,8 @@  physical_run(struct controller_ctx *ctx, enum mf_field_id mff_ovn_geneve,
                                         "ovn-localnet-port");
         const char *l2gateway = smap_get(&port_rec->external_ids,
                                         "ovn-l2gateway-port");
+        const char *localport = smap_get(&port_rec->external_ids,
+                                         "ovn-localport-port");
 
         for (int j = 0; j < port_rec->n_interfaces; j++) {
             const struct ovsrec_interface *iface_rec = port_rec->interfaces[j];
@@ -808,6 +810,10 @@  physical_run(struct controller_ctx *ctx, enum mf_field_id mff_ovn_geneve,
                 /* L2 gateway patch ports can be handled just like VIFs. */
                 simap_put(&new_localvif_to_ofport, l2gateway, ofport);
                 break;
+            } else if (localport) {
+                /* localport ports can be handled just like VIFs. */
+                simap_put(&new_localvif_to_ofport, localport, ofport);
+                break;
             } else if (chassis_id) {
                 enum chassis_tunnel_type tunnel_type;
                 if (!strcmp(iface_rec->type, "geneve")) {
diff --git a/ovn/northd/ovn-northd.c b/ovn/northd/ovn-northd.c
index 5a2e5ab..09f55c8 100644
--- a/ovn/northd/ovn-northd.c
+++ b/ovn/northd/ovn-northd.c
@@ -3166,9 +3166,11 @@  build_lswitch_flows(struct hmap *datapaths, struct hmap *ports,
         /*
          * Add ARP/ND reply flows if either the
          *  - port is up or
-         *  - port type is router
+         *  - port type is router or
+         *  - port type is localport
          */
-        if (!lsp_is_up(op->nbsp) && strcmp(op->nbsp->type, "router")) {
+        if (!lsp_is_up(op->nbsp) && strcmp(op->nbsp->type, "router") &&
+            strcmp(op->nbsp->type, "localport")) {
             continue;
         }