From patchwork Sat Oct 10 04:20:32 2015 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Ben Pfaff X-Patchwork-Id: 528533 Return-Path: X-Original-To: incoming@patchwork.ozlabs.org Delivered-To: patchwork-incoming@bilbo.ozlabs.org Received: from archives.nicira.com (unknown [IPv6:2600:3c00::f03c:91ff:fe6e:bdf7]) by ozlabs.org (Postfix) with ESMTP id 23648140E42 for ; Sat, 10 Oct 2015 15:20:50 +1100 (AEDT) Received: from archives.nicira.com (localhost [127.0.0.1]) by archives.nicira.com (Postfix) with ESMTP id 57D5310B57; Fri, 9 Oct 2015 21:20:49 -0700 (PDT) X-Original-To: dev@openvswitch.org Delivered-To: dev@openvswitch.org Received: from mx1e4.cudamail.com (mx1.cudamail.com [69.90.118.67]) by archives.nicira.com (Postfix) with ESMTPS id 354AE10B55 for ; Fri, 9 Oct 2015 21:20:47 -0700 (PDT) Received: from bar2.cudamail.com (unknown [192.168.21.12]) by mx1e4.cudamail.com (Postfix) with ESMTPS id ABBAD1E0524 for ; Fri, 9 Oct 2015 22:20:46 -0600 (MDT) X-ASG-Debug-ID: 1444450846-03dc537fe2ef2660001-byXFYA Received: from mx1-pf1.cudamail.com ([192.168.24.1]) by bar2.cudamail.com with ESMTP id d3HZIASNyuBwmDvw (version=TLSv1 cipher=DHE-RSA-AES256-SHA bits=256 verify=NO) for ; Fri, 09 Oct 2015 22:20:46 -0600 (MDT) X-Barracuda-Envelope-From: blp@nicira.com X-Barracuda-RBL-Trusted-Forwarder: 192.168.24.1 Received: from unknown (HELO mail-pa0-f42.google.com) (209.85.220.42) by mx1-pf1.cudamail.com with ESMTPS (RC4-SHA encrypted); 10 Oct 2015 04:20:45 -0000 Received-SPF: unknown (mx1-pf1.cudamail.com: Multiple SPF records returned) X-Barracuda-RBL-Trusted-Forwarder: 209.85.220.42 Received: by pacex6 with SMTP id ex6so104245460pac.0 for ; Fri, 09 Oct 2015 21:20:45 -0700 (PDT) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20130820; h=x-gm-message-state:from:to:cc:subject:date:message-id:in-reply-to :references; bh=KYjNsPl0pSb2+DxbUR6HEvJixR7FVNlyHhvN+mv4AZk=; b=EdGAyIe+NjKUBcg1MffUmS4VwMAnoru7e28RPIw/ob0b6Cqk1kVlnxmrfgOcA1ZGDf C0H/+MSYJ+4PzWy2BU7Fz4ad4ioFwyQhViKvndMoWLjzDMVsmu8x703SN02pdD0o0O02 fD27IyNZ/fG6S0F+AtlgYbsdE60+THz9l/VS3oom3VrIr/3HRmyAfh0U0N72wXuDhY1u OuCuXslokXUMiFPPd2JES0gB1UH8aeNsLGJhVEK1SLoCyMbdl7iuiUeMLYSl3//9xZEO iqo7uii5icHXhbwebXIl5VhOFqVGk5ytiEtRkFdXYoZXmY3Jp5f478xNRIF9hSL5hXd+ 9QMw== X-Gm-Message-State: ALoCoQm2cJ7vDFaza2eyuO0yD4ZXxH7toAG5Td5X4Yx/AEILO0rP+aF8N4YXqm1IaQr8+z5W0RwP X-Received: by 10.68.220.166 with SMTP id px6mr19339554pbc.99.1444450845354; Fri, 09 Oct 2015 21:20:45 -0700 (PDT) Received: from sigabrt.gateway.sonic.net (173-228-112-112.dsl.dynamic.fusionbroadband.com. [173.228.112.112]) by smtp.gmail.com with ESMTPSA id o3sm5365390pap.37.2015.10.09.21.20.43 (version=TLS1_2 cipher=ECDHE-RSA-AES128-SHA bits=128/128); Fri, 09 Oct 2015 21:20:44 -0700 (PDT) X-CudaMail-Envelope-Sender: blp@nicira.com X-Barracuda-Apparent-Source-IP: 173.228.112.112 From: Ben Pfaff To: dev@openvswitch.org X-CudaMail-Whitelist-To: dev@openvswitch.org X-CudaMail-MID: CM-E1-1008110424 X-CudaMail-DTE: 100915 X-CudaMail-Originating-IP: 209.85.220.42 Date: Fri, 9 Oct 2015 21:20:32 -0700 X-ASG-Orig-Subj: [##CM-E1-1008110424##][PATCH 13/23] ovn: Implement logical patch ports. Message-Id: <1444450838-12150-2-git-send-email-blp@nicira.com> X-Mailer: git-send-email 2.1.3 In-Reply-To: <1444450838-12150-1-git-send-email-blp@nicira.com> References: <1444450838-12150-1-git-send-email-blp@nicira.com> X-Barracuda-Connect: UNKNOWN[192.168.24.1] X-Barracuda-Start-Time: 1444450846 X-Barracuda-Encrypted: DHE-RSA-AES256-SHA X-Barracuda-URL: https://web.cudamail.com:443/cgi-mod/mark.cgi X-ASG-Whitelist: Header =?UTF-8?B?eFwtY3VkYW1haWxcLXdoaXRlbGlzdFwtdG8=?= X-Virus-Scanned: by bsmtpd at cudamail.com X-Barracuda-BRTS-Status: 1 Cc: Ben Pfaff Subject: [ovs-dev] [PATCH 13/23] ovn: Implement logical patch ports. X-BeenThere: dev@openvswitch.org X-Mailman-Version: 2.1.16 Precedence: list List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , MIME-Version: 1.0 Errors-To: dev-bounces@openvswitch.org Sender: "dev" This implementation is suboptimal for several reasons. First, it creates an OVS port for every OVN logical patch port, not just for the ones that are actually useful on this hypervisor. Second, it's wasteful to create an OVS patch port per OVN logical patch port, when really there's no benefit to them beyond a way to identify how a packet ingressed into a logical datapath. There are two obvious ways to improve the situation here, by modifying OVS: 1. Add a way to configure in OVS which fields are preserved on a hop across an OVS patch port. If MFF_LOG_DATAPATH and MFF_LOG_INPORT were preserved, then only a single pair of OVS patch ports would be required regardless of the number of OVN logical patch ports. 2. Add a new OpenFlow extension action modeled on "resubmit" that also saves and restores the packet data and metadata (the inability to do this is the only reason that "resubmit" can't be used already). Or add OpenFlow extension actions to otherwise save and restore packet data and metadata. We should probably choose one of those in the medium to long term, but I don't know which one. Signed-off-by: Ben Pfaff Acked-by: Justin Pettit --- ovn/TODO | 37 +++++++++++---------------- ovn/controller/patch.c | 63 ++++++++++++++++++++++++++++++++++++++++------ ovn/controller/physical.c | 15 +++++++++-- ovn/ovn-architecture.7.xml | 12 ++++++++- ovn/ovn-sb.xml | 29 +++++++++++++++++++-- tests/ovn-controller.at | 40 ++++++++++++++++++++++++++++- 6 files changed, 160 insertions(+), 36 deletions(-) diff --git a/ovn/TODO b/ovn/TODO index b29df75..c914c10 100644 --- a/ovn/TODO +++ b/ovn/TODO @@ -18,30 +18,21 @@ one router to another, this doesn't seem to matter (just put more than one connection between them), but for connections between a router and a switch it might matter because a switch has only one router port. -** OVN_SB schema +*** Logical router port names in ACLs + +Currently the ACL table documents that the logical router port is +always named "ROUTER". This can't work directly using logical patch +ports to connect a logical switch to its logical router, because every +row in the Logical_Port table must have a unique name. This probably +means that we should change the convention for the ACL table so that +the logical router port name is unique; for example, we could change +the Logical_Router_Port table to require the 'name' column to be +unique, and then use that name in the ACL table. -*** Logical datapath interconnection - -There needs to be a way in the OVN_Southbound database to express -connections between logical datapaths, so that packets can pass from a -logical switch to its logical router (and vice versa) and from one -logical router to another. - -One way to do this would be to introduce logical patch ports, closely -modeled on the "physical" patch ports that OVS has had for ages. Each -logical patch port would consist of two rows in the Port_Binding table -(one in each logical datapath), with type "patch" and an option "peer" -that names the other logical port in the pair. - -If we do it this way then we'll need to figure out one odd special -case. Currently the ACL table documents that the logical router port -is always named "ROUTER". This can't be done directly with this patch -port technique, because every row in the Logical_Port table must have -a unique name. This probably means that we should change the -convention for the ACL table so that the logical router port name is -unique; for example, we could change the Logical_Router_Port table to -require the 'name' column to be unique, and then use that name in the -ACL table. +Another alternative would be to add a way to have aliases for logical +ports, but I'm not sure that's a rathole we really want to go down. + +** OVN_SB schema *** Allow output to ingress port diff --git a/ovn/controller/patch.c b/ovn/controller/patch.c index 90c72ff..f25709c 100644 --- a/ovn/controller/patch.c +++ b/ovn/controller/patch.c @@ -50,7 +50,7 @@ match_patch_port(const struct ovsrec_port *port, const char *peer) static void create_patch_port(struct controller_ctx *ctx, - const char *network, + const char *key, const char *value, const struct ovsrec_bridge *src, const char *src_name, const struct ovsrec_bridge *dst, const char *dst_name, struct shash *existing_ports) @@ -78,7 +78,7 @@ create_patch_port(struct controller_ctx *ctx, port = ovsrec_port_insert(ctx->ovs_idl_txn); ovsrec_port_set_name(port, src_name); ovsrec_port_set_interfaces(port, &iface, 1); - const struct smap ids = SMAP_CONST1(&ids, "ovn-localnet-port", network); + const struct smap ids = SMAP_CONST1(&ids, key, value); ovsrec_port_set_external_ids(port, &ids); struct ovsrec_port **ports; @@ -93,13 +93,13 @@ create_patch_port(struct controller_ctx *ctx, static void create_patch_ports(struct controller_ctx *ctx, - const char *network, + const char *key, const char *value, const struct ovsrec_bridge *b1, const char *name1, const struct ovsrec_bridge *b2, const char *name2, struct shash *existing_ports) { - create_patch_port(ctx, network, b1, name1, b2, name2, existing_ports); - create_patch_port(ctx, network, b2, name2, b1, name1, existing_ports); + create_patch_port(ctx, key, value, b1, name1, b2, name2, existing_ports); + create_patch_port(ctx, key, value, b2, name2, b1, name1, existing_ports); } static void @@ -172,7 +172,7 @@ parse_bridge_mappings(struct controller_ctx *ctx, char *br_int_name = patch_port_name(br_int, ovs_bridge); char *ovs_bridge_name = patch_port_name(ovs_bridge, br_int); - create_patch_ports(ctx, network, + create_patch_ports(ctx, "ovn-localnet-port", network, br_int, br_int_name, ovs_bridge, ovs_bridge_name, existing_ports); @@ -182,6 +182,53 @@ parse_bridge_mappings(struct controller_ctx *ctx, free(start); } +/* Add one OVS patch port for each OVN logical patch port. + * + * This is suboptimal for several reasons. First, it creates an OVS port for + * every OVN logical patch port, not just for the ones that are actually useful + * on this hypervisor. Second, it's wasteful to create an OVS patch port per + * OVN logical patch port, when really there's no benefit to them beyond a way + * to identify how a packet ingressed into a logical datapath. + * + * There are two obvious ways to improve the situation here, by modifying + * OVS: + * + * 1. Add a way to configure in OVS which fields are preserved on a hop + * across an OVS patch port. If MFF_LOG_DATAPATH and MFF_LOG_INPORT + * were preserved, then only a single pair of OVS patch ports would be + * required regardless of the number of OVN logical patch ports. + * + * 2. Add a new OpenFlow extension action modeled on "resubmit" that also + * saves and restores the packet data and metadata (the inability to do + * this is the only reason that "resubmit" can't be used already). Or + * add OpenFlow extension actions to otherwise save and restore packet + * data and metadata. + */ +static void +add_logical_patch_ports(struct controller_ctx *ctx, + const struct ovsrec_bridge *br_int, + struct shash *existing_ports) +{ + const struct sbrec_port_binding *binding; + SBREC_PORT_BINDING_FOR_EACH (binding, ctx->ovnsb_idl) { + if (!strcmp(binding->type, "patch")) { + const char *local = binding->logical_port; + const char *peer = smap_get(&binding->options, "peer"); + if (!peer) { + continue; + } + + char *src_name = xasprintf("patch-%s-to-%s", local, peer); + char *dst_name = xasprintf("patch-%s-to-%s", peer, local); + create_patch_port(ctx, "ovn-logical-patch-port", local, + br_int, src_name, br_int, dst_name, + existing_ports); + free(dst_name); + free(src_name); + } + } +} + void patch_run(struct controller_ctx *ctx, const struct ovsrec_bridge *br_int) { @@ -193,13 +240,15 @@ patch_run(struct controller_ctx *ctx, const struct ovsrec_bridge *br_int) struct shash existing_ports = SHASH_INITIALIZER(&existing_ports); const struct ovsrec_port *port; OVSREC_PORT_FOR_EACH (port, ctx->ovs_idl) { - if (smap_get(&port->external_ids, "ovn-localnet-port")) { + if (smap_get(&port->external_ids, "ovn-localnet-port") || + smap_get(&port->external_ids, "ovn-logical-patch-port")) { shash_add(&existing_ports, port->name, port); } } /* Add any patch ports that should exist but don't. */ parse_bridge_mappings(ctx, br_int, &existing_ports); + add_logical_patch_ports(ctx, br_int, &existing_ports); /* Delete any patch ports that do exist but shouldn't. (Any that both * should and do exist were removed above.) */ diff --git a/ovn/controller/physical.c b/ovn/controller/physical.c index a1f7fe5..2cf7d8c 100644 --- a/ovn/controller/physical.c +++ b/ovn/controller/physical.c @@ -156,6 +156,8 @@ physical_run(struct controller_ctx *ctx, enum mf_field_id mff_ovn_geneve, const char *localnet = smap_get(&port_rec->external_ids, "ovn-localnet-port"); + const char *logpatch = smap_get(&port_rec->external_ids, + "ovn-logical-patch-port"); for (int j = 0; j < port_rec->n_interfaces; j++) { const struct ovsrec_interface *iface_rec = port_rec->interfaces[j]; @@ -169,10 +171,16 @@ physical_run(struct controller_ctx *ctx, enum mf_field_id mff_ovn_geneve, continue; } - /* Record as patch to local net, chassis, or local logical port. */ - if (!strcmp(iface_rec->type, "patch") && localnet) { + /* Record as patch to local net, logical patch port, chassis, or + * local logical port. */ + bool is_patch = !strcmp(iface_rec->type, "patch"); + if (is_patch && localnet) { simap_put(&localnet_to_ofport, localnet, ofport); break; + } else if (is_patch && logpatch) { + /* Logical patch ports can be handled just like VIFs. */ + simap_put(&localvif_to_ofport, logpatch, ofport); + break; } else if (chassis_id) { enum chassis_tunnel_type tunnel_type; if (!strcmp(iface_rec->type, "geneve")) { @@ -242,6 +250,8 @@ physical_run(struct controller_ctx *ctx, enum mf_field_id mff_ovn_geneve, * and accessible via a VLAN, 'tag' is the VLAN ID; otherwise * 'tag' is 0. * + * The same logic handles logical patch ports. + * * - If the port is on a remote chassis, the OpenFlow port for a * tunnel to the VIF's remote chassis. 'tun' identifies that * tunnel. @@ -252,6 +262,7 @@ physical_run(struct controller_ctx *ctx, enum mf_field_id mff_ovn_geneve, * * The "localnet" port may be configured with a VLAN ID. If so, * 'tag' will be set to that VLAN ID; otherwise 'tag' is 0. + * */ int tag = 0; diff --git a/ovn/ovn-architecture.7.xml b/ovn/ovn-architecture.7.xml index a7ff674..364ccce 100644 --- a/ovn/ovn-architecture.7.xml +++ b/ovn/ovn-architecture.7.xml @@ -773,7 +773,9 @@

Flows in table 33 resemble those in table 32 but for logical ports that - reside locally rather than remotely. For unicast logical output ports + reside locally rather than remotely. (This includes logical patch + ports, which do not have a physical location and effectively reside on + every hypervisor.) For unicast logical output ports on the local hypervisor, the actions just resubmit to table 34. For multicast output ports that include one or more logical ports on the local hypervisor, for each such logical port P, the actions @@ -814,6 +816,14 @@ is a container nested with a VM, then before sending the packet the actions push on a VLAN header with an appropriate VLAN ID.

+ +

+ If the logical egress port is a logical patch port, then table 64 + outputs to an OVS patch port that represents the logical patch port. + The packet re-enters the OpenFlow flow table from the OVS patch port's + peer in table 0, which identifies the logical datapath and logical + input port based on the OVS patch port's OpenFlow port number. +

diff --git a/ovn/ovn-sb.xml b/ovn/ovn-sb.xml index 3cc96d4..bd116c3 100644 --- a/ovn/ovn-sb.xml +++ b/ovn/ovn-sb.xml @@ -1039,8 +1039,9 @@ tcp.flags = RST;

- Each row in this table identifies the physical location of a logical - port. + Most rows in this table identify the physical location of a logical port. + (The exceptions are logical patch ports, which do not have any physical + location.)

@@ -1127,6 +1128,14 @@ tcp.flags = RST;

(empty string)
VM (or VIF) interface.
+ +
patch
+
+ One of a pair of logical ports that act as if connected by a patch + cable. Useful for connecting two logical datapaths, e.g. to connect + a logical router to a logical switch or to another logical router. +
+
localnet
A connection to a locally accessible network from each @@ -1150,6 +1159,22 @@ tcp.flags = RST; + +

+ These options apply to logical ports with of + patch. +

+ + + The in the + record for the other side of the patch. The named must specify this + in its own peer option. That is, the two patch logical + ports must have reversed and + peer values. + + +

These options apply to logical ports with of diff --git a/tests/ovn-controller.at b/tests/ovn-controller.at index 44206c1..773b3a7 100644 --- a/tests/ovn-controller.at +++ b/tests/ovn-controller.at @@ -64,8 +64,46 @@ check_patches \ 'br-eth1 patch-br-eth1-to-br-int patch-br-int-to-br-eth1' \ 'br-int patch-br-int-to-br-eth1 patch-br-eth1-to-br-int' -# Delete the mapping and the patch ports should go away. +# Add logical patch ports. +AT_CHECK([ovn-sbctl \ + -- --id=@dp1 create Datapath_Binding tunnel_key=1 \ + -- --id=@dp2 create Datapath_Binding tunnel_key=2 \ + -- create Port_Binding datapath=@dp1 logical_port=foo tunnel_key=1 type=patch options:peer=bar \ + -- create Port_Binding datapath=@dp2 logical_port=bar tunnel_key=2 type=patch options:peer=foo \ +| ${PERL} $srcdir/uuidfilt.pl], [0], [<0> +<1> +<2> +<3> +]) +check_patches \ + 'br-eth2 patch-br-eth2-to-br-int patch-br-int-to-br-eth2' \ + 'br-int patch-br-int-to-br-eth2 patch-br-eth2-to-br-int' \ + 'br-eth1 patch-br-eth1-to-br-int patch-br-int-to-br-eth1' \ + 'br-int patch-br-int-to-br-eth1 patch-br-eth1-to-br-int' \ + 'br-int patch-foo-to-bar patch-bar-to-foo' \ + 'br-int patch-bar-to-foo patch-foo-to-bar' + +# Delete the mapping and the ovn-bridge-mapping patch ports should go away; +# the ones from the logical patch port remain. AT_CHECK([ovs-vsctl remove Open_vSwitch . external-ids ovn-bridge-mappings]) +check_patches \ + 'br-int patch-foo-to-bar patch-bar-to-foo' \ + 'br-int patch-bar-to-foo patch-foo-to-bar' + +# Change name of logical patch port, check that the OVS patch ports +# get updated. +AT_CHECK([ovn-sbctl \ + -- set Port_Binding foo logical_port=quux options:peer=baz \ + -- set Port_Binding bar logical_port=baz options:peer=quux]) +check_patches \ + 'br-int patch-quux-to-baz patch-baz-to-quux' \ + 'br-int patch-baz-to-quux patch-quux-to-baz' + +# Change the logical patch ports to VIFs and verify that the OVS patch +# ports disappear. +AT_CHECK([ovn-sbctl \ + -- set Port_Binding quux type='""' options='{}' \ + -- set Port_Binding baz type='""' options='{}']) check_patches AT_CLEANUP