[ovs-dev] [PATCH 23/23] ovn: Implement basic logical L3 routing.

Ben Pfaff blp at nicira.com
Sat Oct 17 06:50:20 UTC 2015


On Fri, Oct 16, 2015 at 05:20:09PM -0700, Justin Pettit wrote:
> 
> > On Oct 9, 2015, at 9:21 PM, Ben Pfaff <blp at nicira.com> wrote:
> > 
> > ---
> > ovn/TODO                |   6 -
> 
> From ovn/TODO:
> 
> -=-=-=-=-=-=-=-=-
> ** IP to MAC binding
> 
> Somehow it has to be possible for an L3 logical router to map from an
> IP address to an Ethernet address.  This can happen statically or
> dynamically.  Probably both cases need to be supported eventually.
> -=-=-=-=-=-=-=-=-
> 
> With this patch, I think this part of TODO could be updated, since
> static is supported.

OK, done.

> > /* The 'key' comes from nb->header_.uuid or sb->external_ids:logical-switch. */
> > struct ovn_datapath {
> >     struct hmap_node key_node;  /* Index on 'key'. */
> > -    struct uuid key;            /* nb->header_.uuid. */
> > +    struct uuid key;            /* (nbs/nbr)->header_.uuid. */
> 
> I think the comment describing this structure also needs an update.

Done.

> > -        if (!smap_get_uuid(&sb->external_ids, "logical-switch", &key)) {
> > +        if (!smap_get_uuid(&sb->external_ids, "logical-switch", &key) &&
> > +            !smap_get_uuid(&sb->external_ids, "logical-router", &key)) {
> >             ovsdb_idl_txn_add_comment(ctx->ovnsb_txn,
> >                                       "deleting Datapath_Binding "UUID_FMT" that "
> >                                       "lacks external-ids:logical-switch",
> 
> I think this comment can use an update to include "logical-router".
> There's a warning a few lines down with a similar issue.

Done.

> > struct ovn_port {
> >     struct hmap_node key_node;  /* Index on 'key'. */
> > -    const char *key;            /* nb->name and sb->logical_port */
> > +    char *key;                  /* nb->name and sb->logical_port */
> 
> I think this can be either "nbs" or "nbr", not "nb".

Right.  Fixed.

> > +    /* Logical router port data. */
> > +    ovs_be32 ip, mask;          /* 192.168.10.123/24. */
> > +    ovs_be32 network;           /* 192.168.10.0. */
> > +    ovs_be32 bcast;             /* 192.168.10.255. */
> 
> It's probably obvious, but it may be worth pointing out that these are
> example values if the system were supplied an IP/mask of
> "192.168.10.123/24".

I think this is obvious enough.

> > static struct ovn_port *
> > ovn_port_create(struct hmap *ports, const char *key,
> > -                const struct nbrec_logical_port *nb,
> > +                const struct nbrec_logical_port *nbs,
> > +                const struct nbrec_logical_router_port *nbr,
> 
> Do you think it's worth renaming the table Logical_Port to
> Logical_Switch_Port?

We can talk about it.  I don't want to do it in this patch.

> > +    ovn_lflow_add(lflows, od, S_ROUTER_IN_IP_ROUTING,
> > +                  count_1bits(ntohl(mask)), match, ds_cstr(&actions));
> 
> It might be worth mentioning the priorities are based on the netmask
> to achieve LPM.

OK, I added a comment.

> > 
> > +        /* ARP reply.  These flows reply to ARP requests for the router's own
> > +         * IP address. */
> > +        match = xasprintf(
> > +            "inport == %s && arp.tha == "ETH_ADDR_FMT" && arp.op == 1",
> > +            op->json_key, ETH_ADDR_ARGS(op->mac));
> 
> As Han mentioned, this should be the "arp.tpa" instead of "tha".

Fixed.

> > +        char *actions = xasprintf(
> > +            "eth.dst = eth.src; "
> > +            "eth.src = "ETH_ADDR_FMT"; "
> 
> Shouldn't we be setting "eth.type"?

No, eth.type isn't modifiable.  We're not changing the type of the
packet anyway (we're sending an ARP reply to an ARP request).

> > +            "inport = \"\"; /* Allow sending out inport. */ "
> 
> I assume you tested this, and ovn-controller handles this properly.

It's a pleasant assumption, but wrong.

I'll send a series in a minute to fix the bugs and add a test.

> > +    <group title="OVN_Northbound Relationship">
> > +      <p>
> > +        Each row in <ref table="Datapath_Binding"/> is associated with some
> > +        logical datapath.  <code>ovn-northd</code> uses these key to track the
> > +        association of a logical datapath with concepts in the <ref
> > +        db="OVN_Northbound"/> database.
> 
> s/key/keys/

Thanks, fixed.

> This is awesome!  Can't wait to try it all together!
> 
> Acked-by: Justin Pettit <jpettit at nicira.com>

Thanks!  I'll push this in a minute.



More information about the dev mailing list