[ovs-dev] [PATCH v2] ovn: Connect to remote lports through localnet port.
Han Zhou
zhouhan at gmail.com
Fri Feb 5 19:47:35 UTC 2016
On Fri, Feb 5, 2016 at 10:55 AM, Russell Bryant <russell at ovn.org> wrote:
>
> 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 <zhouhan at 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.
>
Thank you so much for the detailed background information!!
For the second use case, this patch also has some impact if we think about
the case where 2 logical routers are supposed to be connected to the same
external network. In that case we just need 1 lswitch instead of 2.
> 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.
Yes.
>
> > }
> > }
>
> 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(...)
> ...
> }
>
Good idea!
>
> > 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.
>
I think this indent makes it more clear that this line is not an arg of
CONTAINER_OF but an arg of hmap_first_with_hash(). But if it is the
convention to make it all aligned I will just follow.
> > + 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;
>
Sure :)
>
> > @@ -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?
>
This is to do the real job what table 33 was originally supposed to do,
e.g. load zone_id (or any new actions if added in the future) and then
continue the egress pipeline as if the the packet were sent out to the
localnet port directly.
> > } 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.
Yes, I will revise.
>
> 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.
Yes, I will revise.
>
> I wonder if we should say anything about the behavior of specifying a
> localnet port with inport/outport. That behavior may not be obvious.
I didn't mention because they are supposed to work the same way as the
normal lports. I will revise to make it explicit.
>
> > + </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
Thanks for the detailed review and I will send v3 soon!
--
Best regards,
Han
More information about the dev
mailing list