On Fri, Oct 16, 2015 at 05:20:09PM -0700, Justin Pettit wrote:
>
> > On Oct 9, 2015, at 9:21 PM, Ben Pfaff <b...@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 <jpet...@nicira.com>
Thanks! I'll push this in a minute.
_______________________________________________
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
