| Message ID | 1443448497-14226-1-git-send-email-gshetty@nicira.com |
|---|---|
| State | Superseded |
| Headers | show |
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) {
> 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
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
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;
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(-)