@@ -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
@@ -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.) */
@@ -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;
@@ -773,7 +773,9 @@
<p>
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 <var>P</var>, 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.
</p>
+
+ <p>
+ 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.
+ </p>
</li>
</ol>
@@ -1039,8 +1039,9 @@ tcp.flags = RST;
<table name="Port_Binding" title="Physical-Logical Port Bindings">
<p>
- 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.)
</p>
<p>
@@ -1127,6 +1128,14 @@ tcp.flags = RST;
<dl>
<dt>(empty string)</dt>
<dd>VM (or VIF) interface.</dd>
+
+ <dt><code>patch</code></dt>
+ <dd>
+ 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.
+ </dd>
+
<dt><code>localnet</code></dt>
<dd>
A connection to a locally accessible network from each
@@ -1150,6 +1159,22 @@ tcp.flags = RST;
</column>
</group>
+ <group title="Patch Options">
+ <p>
+ These options apply to logical ports with <ref column="type"/> of
+ <code>patch</code>.
+ </p>
+
+ <column name="options" key="peer">
+ The <ref column="logical_port"/> in the <ref table="Port_Binding"/>
+ record for the other side of the patch. The named <ref
+ column="logical_port"/> must specify this <ref column="logical_port"/>
+ in its own <code>peer</code> option. That is, the two patch logical
+ ports must have reversed <ref column="logical_port"/> and
+ <code>peer</code> values.
+ </column>
+ </group>
+
<group title="Localnet Options">
<p>
These options apply to logical ports with <ref column="type"/> of
@@ -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
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 <blp@nicira.com> --- 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(-)