On Tue, Apr 28, 2015 at 05:20:13PM -0700, Justin Pettit wrote:
>
> > On Apr 24, 2015, at 3:34 PM, Ben Pfaff <b...@nicira.com> wrote:
> >
> > When packets travel among nodes in OVN over tunnels, a tunnel key value is
> > needed to convey the logical port to which the packet is destined. This
> > commit adds a tunnel_key column to the Bindings table and adds code to
> > ovn-northd to assign a unique tunnel_key value to each logical port.
> >
> > Signed-off-by: Ben Pfaff <b...@nicira.com>
>
> This patch (and the implementation that follows) goes the route of
> having each logical port have a unique tunnel key. I had envisioned a
> different approach. I'll briefly write it down here, but then it's
> probably worth discussing it more fully in a different forum (or at
> least a different thread). I was thinking that we'd use a 24-bit VNI
> that would identify a logical switch. In Geneve, it would be in the
> VNI field, and in STT, it would be in the lower 24 bits of the 64 bit
> key. The local OVS would receive packets from a particular VNI, and
> then look at the DMAC to figure out which logical ports should receive
> it.
>
> For port security, we would add a logical port that would uniquely
> identify a port within a particular VNI (ie, the value is only unique
> for a particular VNI). For this, the logical port would only be used
> to identify the ingress port, since the receiving OVS would still be
> responsible for local replication and enforcing policies. We would
> store the logical port in a TLV extension in Geneve and in the next
> (let's say) 16 bits of the STT header.
>
> Okay, now that I've written that down, I'll review the code as you've
> implemented it. We can have a follow-up discussion on the relative
> merits of each approach.
I have some thoughts on the above, but I also would like to discuss it
separately. Do you want to repost the above as a new thread? Or do you
want to discuss some other way?
> > + /* Choose unique tunnel_key for the logical port. */
> > + static uint16_t next_tunnel_key = 1;
> > + while (tunnel_key_in_use(&tk_hmap, next_tunnel_key)) {
> > + next_tunnel_key++;
> > + }
> > + sbrec_bindings_set_tunnel_key(binding, next_tunnel_key++);
>
> Were you trying to avoid a tunnel key of zero? If so, I think we'll
> hit that on wrap-around. Also, if we have 64K logical ports, I think
> this will get stuck in a permanent loop with no way to get out, since
> it's the only thing that ever deletes a tunnel_key. Finally, should
> you add the new tunnel to tk_hmap so that if we're anywhere near full
> that we don't assign the same tunnel key twice?
Yes, I was trying to avoid a tunnel key of 0 (the schema prohibits 0; I
use it in the pipeline as a special value), and I didn't expect that
we'd get anywhere near 64k ports (initially) so I didn't worry about
that case. Anyway, it's better to avoid the problem, and not too hard.
I've appended an incremental diff to this email.
> > + struct binding_hash_node *hash_node;
> > + HMAP_FOR_EACH (hash_node, lp_node, &lp_hmap) {
> > + hmap_remove(&lp_hmap, &hash_node->lp_node);
>
> Should you be using HMAP_FOR_EACH_SAFE()?
That's necessary if the loop actually modifies or frees the hmap_node or
the structure that contains it (which is the usual case), but this loop
doesn't do that.
> > + "tunnel_key": {
> > + "type": {"key": {"type": "integer",
> > + "minInteger": 1,
> > + "maxInteger": 65535}}},
>
> If we decide to go with a unique id for each logical port, I do worry
> that 64K is too small for the total number of logical ports supported
> by OVN.
Yes, I do think that we need to scope it within a logical switch.
--8<--------------------------cut here-------------------------->8--
diff --git a/ovn/northd/ovn-northd.c b/ovn/northd/ovn-northd.c
index 09a98cc..0b5becf 100644
--- a/ovn/northd/ovn-northd.c
+++ b/ovn/northd/ovn-northd.c
@@ -464,6 +464,25 @@ tunnel_key_in_use(const struct hmap *tk_hmap, uint16_t
tunnel_key)
return false;
}
+/* Chooses and returns a positive tunnel key that is not already in use in
+ * 'tk_hmap'. Returns 0 if all tunnel keys are in use. */
+static uint16_t
+choose_tunnel_key(const struct hmap *tk_hmap)
+{
+ static uint16_t prev;
+
+ for (uint16_t key = prev + 1; key != prev; key++) {
+ if (!tunnel_key_in_use(tk_hmap, key)) {
+ prev = key;
+ return key;
+ }
+ }
+
+ static struct vlog_rate_limit rl = VLOG_RATE_LIMIT_INIT(1, 1);
+ VLOG_WARN_RL(&rl, "all tunnel keys exhausted");
+ return 0;
+}
+
/*
* When a change has occurred in the OVN_Northbound database, we go through and
* make sure that the contents of the Bindings table in the OVN_Southbound
@@ -545,6 +564,11 @@ set_bindings(struct northd_context *ctx)
} else {
/* There is no binding for this logical port, so create one. */
+ uint16_t tunnel_key = choose_tunnel_key(&tk_hmap);
+ if (!tunnel_key) {
+ continue;
+ }
+
binding = sbrec_bindings_insert(ctx->ovnsb_txn);
sbrec_bindings_set_logical_port(binding, lport->name);
sbrec_bindings_set_mac(binding,
@@ -554,14 +578,16 @@ set_bindings(struct northd_context *ctx)
sbrec_bindings_set_tag(binding, lport->tag, lport->n_tag);
}
- /* Choose unique tunnel_key for the logical port. */
- static uint16_t next_tunnel_key = 1;
- while (tunnel_key_in_use(&tk_hmap, next_tunnel_key)) {
- next_tunnel_key++;
- }
- sbrec_bindings_set_tunnel_key(binding, next_tunnel_key++);
-
+ sbrec_bindings_set_tunnel_key(binding, tunnel_key);
sbrec_bindings_set_logical_datapath(binding, logical_datapath);
+
+ /* Add the tunnel key to the tk_hmap so that we don't try to use it
+ * for another port. (We don't want it in the lp_hmap because that
+ * would just get the Bindings record deleted later.) */
+ struct binding_hash_node *hash_node = xzalloc(sizeof *hash_node);
+ hash_node->binding = binding;
+ hmap_insert(&tk_hmap, &hash_node->tk_node,
+ hash_int(binding->tunnel_key, 0));
}
}
diff --git a/ovn/ovn-sb.xml b/ovn/ovn-sb.xml
index 5eabf32..4fb7529 100644
--- a/ovn/ovn-sb.xml
+++ b/ovn/ovn-sb.xml
@@ -666,7 +666,16 @@
</column>
<column name="tunnel_key">
- A number that represents the logical port in tunnel IDs.
+ <p>
+ A number that represents the logical port in the IDs carried within
+ tunnel protocol packets. (This avoids wasting space for a whole UUID
+ in tunneled packets. It allows OVN to support encapsulations that
+ cannot fit an entire UUID in their tunnel keys.)
+ </p>
+
+ <p>
+ Tunnel ID 0 is reserved for internal use within OVN.
+ </p>
</column>
<column name="parent_port">
_______________________________________________
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
