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

Message ID 1443448497-14226-1-git-send-email-gshetty@nicira.com
State Superseded
Headers show

Commit Message

Gurucharan Shetty Sept. 28, 2015, 1:54 p.m. UTC
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@nicira.com>
---
 ovn/controller/physical.c | 27 ++++++++++++++++++++-------
 1 file changed, 20 insertions(+), 7 deletions(-)

Comments

Russell Bryant Sept. 28, 2015, 6:47 p.m. UTC | #1
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@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) {
Gurucharan Shetty Sept. 28, 2015, 7:28 p.m. UTC | #2
> 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.

Right. I took the approach to prevent future bugs that may add code based on
 the name 'lport_to_ofport' which can indicate that the simap has all
the lport to ofport mappings. Instead it only has local vm backed
logical ports.

So presumably, I can simply change the name to 'vif_to_ofport'.


>
>>             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.

Yes, it should fix it. I am fine with either approach.

>
>
>> 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
Gurucharan Shetty Sept. 28, 2015, 7:31 p.m. UTC | #3
I will send a v2.

On Mon, Sep 28, 2015 at 12:28 PM, Gurucharan Shetty <shettyg@nicira.com> wrote:
>> 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.
>
> Right. I took the approach to prevent future bugs that may add code based on
>  the name 'lport_to_ofport' which can indicate that the simap has all
> the lport to ofport mappings. Instead it only has local vm backed
> logical ports.
>
> So presumably, I can simply change the name to 'vif_to_ofport'.
>
>
>>
>>>             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.
>
> Yes, it should fix it. I am fine with either approach.
>
>>
>>
>>> 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

Patch
diff mbox

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;