[ovs-dev] [PATCH v2 21/21] ovn: Change strategy for tunnel keys.

Justin Pettit jpettit at nicira.com
Mon Aug 3 18:31:15 UTC 2015


> On Jul 28, 2015, at 8:44 AM, Ben Pfaff <blp at nicira.com> wrote:
> 

I missed this in my review yesterday on ovn-architure.7.xml:

> +        OpenFlow tables 32 through 47 implement the <code>output</code> action
> +        in the the logical ingress pipeline.  Specifically, table 32 handles

This introduces two "the"s.

Just some minor things on physical_run():

> +            /* Table 64, Priority 50.
> +             * =======================
>              *
> +             * Deliver the packet to the local vif. */
> ...
> +            ofctrl_add_flow(flow_table, 64, 100, &match, &ofpacts);

This seems to add the flow at priority 50, not 100.

> +    const struct sbrec_multicast_group *mc;
> +    SBREC_MULTICAST_GROUP_FOR_EACH (mc, ctx->ovnsb_idl) {
> +        struct sset remote_chassis = SSET_INITIALIZER(&remote_chassis);
> +        struct match match;

Since this block is adding more flows, it might be nice to add comments like all the other flows, since it's otherwise easy to miss them.

> +    /* Table 34, Priority 0.
> +     * =======================
> +     *
> +     * Resubmit packets that don't output to the ingress port to the logical
> +     * egress pipeline. */
> +    struct match match;
> +    match_init_catchall(&match);
> +    ofpbuf_clear(&ofpacts);
> +    put_resubmit(48, &ofpacts);
> +    ofctrl_add_flow(flow_table, 34, 0, &match, &ofpacts);

The rule definition doesn't match the flows written.  It took me a minute to realize this was because there was a flow at priority 100 that enforced the ingress and egress defined quite a bit earlier.  I think just referencing that other flow here would help.

I don't need to see a respin for this or the previous part of this review, so:

Acked-by: Justin Pettit <jpettit at nicira.com>

Thanks again for the code and thorough documentation!

--Justin





More information about the dev mailing list