On 02/03/2016 07:03 PM, Han Zhou wrote:
> Before this patch, inter-chassis communication between VIFs of same
> lswitch will always go through tunnel, which end up of modeling a
> single physical network with many lswitches and pairs of lports, and
> complexity in CMS like OpenStack neutron to manage the lswitches and
> lports.
>
> With this patch, inter-chassis communication can go through physical
> networks via localnet port with a 1:1 mapping between lswitches and
> physical networks. The pipeline becomes:
>
> Ingress -> Egress (local) -> Ingress (remote) -> Egress
>
> The original tunneling mechanism will still be used if there is no
> localnet port configured on the lswitch.
>
> Signed-off-by: Han Zhou <zhou...@gmail.com>
I'll start by saying that I like this approach. This is what I
originally wanted to implement but couldn't get it work out elegantly
enough for whatever reason. OVN takes on some additional complexity
with this, but I think it's worth it. For that reason, I'd also like to
see Ben sign off on this as well before we merge it, since he reviewed
the original localnet patches and has also worked on the code.
For everyone's benefit, there are two major use cases for localnet ports
that I see. The first one (and only one completely usable today) is
where you want to use OVN to manage port connectivity to existing
networks in your data center. Prior to this patch, the way to do that
was to create a logical switch for every port we wanted to connect to
the network. For N VMs, we had to create N logical switches and (N * 2)
logical ports.
LP-1 (VM-1) <--> LS-1 <--> localnet port 1, to physical network
LP-2 (VM-2) <--> LS-2 <--> localnet port 2, to physical network
...
LP-N (VM-N) <--> LS-N <--> localnet port N, to physical network
With this patch, things are greatly simplified for the CMS (OpenStack
Neutron in our case). We can now model with N logical ports with 1
logical switch and a single localnet port.
LP-1 (VM-1) <--> +------+
LP-2 (VM-2) <--> | LS-1 | <--> localnet port 1, to physical network
... | |
LP-N (VM-N) <--> +------+
For 100 VMs, the old method results in 1400 logical flows in a very
simple setup with no ACLs defined. This new approach results in only
311 logical flows for the same effective configuration.
In one way, this patch simplifies things greatly. The complexity that
OVN takes on here is an expanded definition of what a logical switch is
in OVN.
The second main use case for localnet ports is for modeling the most
common way of using the existing OVS integration in OpenStack. This use
case isn't fully supported yet, though. We would typically create
logical routers between tenant managed overlay networks and a network
that connects to a physical network for external connectivity. The
Floating IP proposal on the list right now makes use of this use case.
I don't think this patch affects that use case, though.
Some minor implementation comments inline.
> ---
>
> Notes:
> v1->v2: rebase on master, and more updates on documents
>
> ovn/controller/binding.c | 10 +++---
> ovn/controller/ovn-controller.c | 18 +++++------
> ovn/controller/ovn-controller.h | 10 ++++++
> ovn/controller/patch.c | 17 ++++++++--
> ovn/controller/physical.c | 70
> ++++++++++++++++++++++++++++++++++++-----
> ovn/controller/physical.h | 3 +-
> ovn/ovn-nb.xml | 11 +++++--
> ovn/ovn-sb.xml | 5 ++-
> 8 files changed, 112 insertions(+), 32 deletions(-)
>
> diff --git a/ovn/controller/binding.c b/ovn/controller/binding.c
> index ce9cccf..af221f1 100644
> --- a/ovn/controller/binding.c
> +++ b/ovn/controller/binding.c
> @@ -122,13 +122,15 @@ static void
> add_local_datapath(struct hmap *local_datapaths,
> const struct sbrec_port_binding *binding_rec)
> {
> - struct hmap_node *ld;
> - ld = hmap_first_with_hash(local_datapaths,
> - binding_rec->datapath->tunnel_key);
> + struct local_datapath *ld;
> + ld = CONTAINER_OF(hmap_first_with_hash(local_datapaths,
> + binding_rec->datapath->tunnel_key),
> + struct local_datapath, hmap_node);
> if (!ld) {
> ld = xmalloc(sizeof *ld);
> - hmap_insert(local_datapaths, ld,
> + hmap_insert(local_datapaths, &ld->hmap_node,
> binding_rec->datapath->tunnel_key);
> + ld->localnet_port = NULL;
I'd probably just use xzalloc() instead.
> }
> }
What about this to save having to use CONTAINER_OF() ?
It's not functionally different, I just find it more readable.
{
if (hmap_first_with_hash(...)) {
return;
}
struct local_datapath *ld;
ld = xmalloc(...);
hmap_insert(...)
...
}
> diff --git a/ovn/controller/patch.c b/ovn/controller/patch.c
> index 1f981b7..7b3b656 100644
> --- a/ovn/controller/patch.c
> +++ b/ovn/controller/patch.c
> @@ -180,15 +180,26 @@ add_bridge_mappings(struct controller_ctx *ctx,
> continue;
> }
>
> - struct hmap_node *ld;
> - ld = hmap_first_with_hash(local_datapaths,
> - binding->datapath->tunnel_key);
> + struct local_datapath *ld;
> + ld = CONTAINER_OF(hmap_first_with_hash(local_datapaths,
> + binding->datapath->tunnel_key),
nit: the indentation is a little weird. I think I would align this at
the same level as the next line.
> + struct local_datapath, hmap_node);
> if (!ld) {
> /* This localnet port is on a datapath with no
> * logical ports bound to this chassis, so there's no need
> * to create patch ports for it. */
> continue;
> }
> + if (ld->localnet_port) {
> + static struct vlog_rate_limit rl = VLOG_RATE_LIMIT_INIT(5, 1);
> + VLOG_WARN_RL(&rl, "localnet port '%s' already set for datapath "
> + "'%ld', skipping the new port '%s'.",
> + ld->localnet_port->logical_port,
> + binding->datapath->tunnel_key,
> + binding->logical_port);
> + continue;
> + }
> + ld->localnet_port = binding;
>
> const char *network = smap_get(&binding->options, "network_name");
> if (!network) {
> diff --git a/ovn/controller/physical.c b/ovn/controller/physical.c
> index 8b12769..a781deb 100644
> --- a/ovn/controller/physical.c
> +++ b/ovn/controller/physical.c
> @@ -135,10 +135,23 @@ put_stack(enum mf_field_id field, struct ofpact_stack
> *stack)
> stack->subfield.n_bits = stack->subfield.field->n_bits;
> }
>
> +static const struct sbrec_port_binding*
> +get_localnet_port(struct hmap *local_datapaths, int64_t tunnel_key)
> +{
> + struct local_datapath *ld;
> + ld = CONTAINER_OF(hmap_first_with_hash(local_datapaths, tunnel_key),
> + struct local_datapath, hmap_node);
> + if (!ld) {
> + return NULL;
> + }
> + return ld->localnet_port;
or you could use:
return ld ? ld->localnet_port : NULL;
> @@ -395,7 +422,32 @@ physical_run(struct controller_ctx *ctx, enum
> mf_field_id mff_ovn_geneve,
> }
> ofctrl_add_flow(flow_table, OFTABLE_LOG_TO_PHY, 100,
> &match, &ofpacts);
> + } else if (!tun) {
> + /* Remote port connected by localnet port */
> + /* Table 33, priority 100.
> + * =======================
> + *
> + * Implements switching to localnet port. Each flow matches a
> + * logical output port on remote hypervisor, switch the output
> port
> + * to connected localnet port and resubmits to same table.
> + */
> +
> + match_init_catchall(&match);
> + ofpbuf_clear(&ofpacts);
> +
> + /* Match MFF_LOG_DATAPATH, MFF_LOG_OUTPORT. */
> + match_set_metadata(&match,
> htonll(binding->datapath->tunnel_key));
> + match_set_reg(&match, MFF_LOG_OUTPORT - MFF_REG0,
> + binding->tunnel_key);
> +
> + put_load(localnet_port->tunnel_key, MFF_LOG_OUTPORT, 0, 32,
> &ofpacts);
> +
> + /* Resubmit to table 33. */
> + put_resubmit(OFTABLE_LOCAL_OUTPUT, &ofpacts);
> + ofctrl_add_flow(flow_table, OFTABLE_LOCAL_OUTPUT, 100, &match,
> + &ofpacts);
What's the value in resubmitting back to table 33 instead of moving on
to table 34?
> } else {
> + /* Remote port connected by tunnel */
> /* Table 32, priority 100.
> * =======================
> *
> diff --git a/ovn/ovn-nb.xml b/ovn/ovn-nb.xml
> index 4e414ce..2bb7893 100644
> --- a/ovn/ovn-nb.xml
> +++ b/ovn/ovn-nb.xml
> @@ -116,9 +116,8 @@
> <dd>
> A connection to a locally accessible network from each
> <code>ovn-controller</code> instance. A logical switch can only
> - have a single <code>localnet</code> port attached and at most one
> - regular logical port. This is used to model direct connectivity
> to
> - an existing network.
> + have a single <code>localnet</code> port attached. This is used
> + to model direct connectivity to an existing network.
> </dd>
>
> <dt><code>vtep</code></dt>
> @@ -393,6 +392,12 @@
> Note that you can not create an ACL matching on a port with
> type=router.
> </p>
> +
> + <p>
> + Note that for lswitches with localnet port the <code>inport</code>
> + works only if the target port is located on the same chassis as
> + the <code>inport</code>.
I believe inport will always work for "from-lport" ACLs. It will only
work for "to-lport" ACLs in the case you listed, though.
It may also be worth clarifying that we're talking about packets between
2 logical ports on this logical switch and that you can not match on the
source logical port with inport in "to-lport" ACLs.
I wonder if we should say anything about the behavior of specifying a
localnet port with inport/outport. That behavior may not be obvious.
> + </p>
> </column>
>
> <column name="action">
> diff --git a/ovn/ovn-sb.xml b/ovn/ovn-sb.xml
> index e674f3a..4d2ec93 100644
> --- a/ovn/ovn-sb.xml
> +++ b/ovn/ovn-sb.xml
> @@ -1217,9 +1217,8 @@ tcp.flags = RST;
> <dd>
> A connection to a locally accessible network from each
> <code>ovn-controller</code> instance. A logical switch can only
> - have a single <code>localnet</code> port attached and at most one
> - regular logical port. This is used to model direct connectivity
> to
> - an existing network.
> + have a single <code>localnet</code> port attached. This is used
> + to model direct connectivity to an existing network.
> </dd>
>
> <dt><code>vtep</code></dt>
>
--
Russell Bryant
_______________________________________________
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
