[ovs-dev] [PATCH] ovn-controller: Fix table 33 flows for containers.

[Russell Bryant]

On 09/28/2015 09:54 AM, Gurucharan Shetty wrote:
> When containers are inside a VM, the broadcast flow
> generated in table 33 was faulty. This was because
> 'lport_to_ofport' did not include logical ports
> associated with containers.
> 
> Signed-off-by: Gurucharan Shetty <gshetty at nicira.com>
> ---
>  ovn/controller/physical.c | 27 ++++++++++++++++++++-------
>  1 file changed, 20 insertions(+), 7 deletions(-)
> 
> diff --git a/ovn/controller/physical.c b/ovn/controller/physical.c
> index bdb02da..c9bc9de 100644
> --- a/ovn/controller/physical.c
> +++ b/ovn/controller/physical.c
> @@ -221,9 +221,21 @@ physical_run(struct controller_ctx *ctx, enum mf_field_id mff_ovn_geneve,
>       * of datapaths with at least one local port binding. */
>      struct hmap local_datapaths = HMAP_INITIALIZER(&local_datapaths);
>  
> +    const struct sbrec_port_binding *binding;
> +    /* Populate 'lport_to_ofport' with containers behind local vif. */
> +    SBREC_PORT_BINDING_FOR_EACH (binding, ctx->ovnsb_idl) {
> +        if (binding->parent_port) {
> +            ofp_port_t ofport;
> +            ofport = u16_to_ofp(simap_get(&lport_to_ofport,
> +                                          binding->parent_port));
> +            if (ofport) {
> +                simap_put(&lport_to_ofport, binding->logical_port, ofport);
> +            }
> +        }
> +    }
> +
>      /* Set up flows in table 0 for physical-to-logical translation and in table
>       * 64 for logical-to-physical translation. */
> -    const struct sbrec_port_binding *binding;
>      SBREC_PORT_BINDING_FOR_EACH (binding, ctx->ovnsb_idl) {
>          /* Find the OpenFlow port for the logical port, as 'ofport'.  This is
>           * one of:
> @@ -252,15 +264,16 @@ physical_run(struct controller_ctx *ctx, enum mf_field_id mff_ovn_geneve,
>                  continue;
>              }
>              ofport = u16_to_ofp(simap_get(&localnet_to_ofport, network));
> -        } else if (binding->parent_port) {
> -            ofport = u16_to_ofp(simap_get(&lport_to_ofport,
> -                                          binding->parent_port));
> -            if (ofport && binding->tag) {
> -                tag = *binding->tag;
> -            }
>          } else {
>              ofport = u16_to_ofp(simap_get(&lport_to_ofport,
>                                            binding->logical_port));
> +            if (ofport && binding->parent_port) {
> +                if (binding->tag) {
> +                    tag = *binding->tag;
> +                } else {
> +                    continue;
> +                }
> +            }
>          }
>  
>          const struct chassis_tunnel *tun = NULL;
> 

>From first read of the patch, it looks like this doesn't change
anything.  The above code still seems to provide the same result.  The
important part is actually in code not changed.  There is code later
that uses whether or not the port is in  lport_to_ofport to determine if
it's local or not.

>             if (simap_contains(&lport_to_ofport, port->logical_port)) {
>                 put_load(port->tunnel_key, MFF_LOG_OUTPORT, 0, 32, &ofpacts);
>                 put_resubmit(OFTABLE_DROP_LOOPBACK, &ofpacts);

I think I'd actually prefer just fixing that code to handle the
parent_port case.  It prevents having to add another full iteration of
port bindings, which could have many thousands of entries.

I think this would fix it, but I haven't tested it.


> diff --git a/ovn/controller/physical.c b/ovn/controller/physical.c
> index bdb02da..5cc700e 100644
> --- a/ovn/controller/physical.c
> +++ b/ovn/controller/physical.c
> @@ -471,7 +471,8 @@ physical_run(struct controller_ctx *ctx, enum mf_field_id mff_ovn_geneve,
>                  continue;
>              }
>  
> -            if (simap_contains(&lport_to_ofport, port->logical_port)) {
> +            if (simap_contains(&lport_to_ofport,
> +                    port->parent_port ? port->parent_port : port->logical_port)) {
>                  put_load(port->tunnel_key, MFF_LOG_OUTPORT, 0, 32, &ofpacts);
>                  put_resubmit(OFTABLE_DROP_LOOPBACK, &ofpacts);
>              } else if (port->chassis) {

-- 
Russell Bryant