| Message ID | 1447797606-1887-1-git-send-email-russell@ovn.org |
|---|---|
| State | Accepted |
| Headers | show |
On Tue, Nov 17, 2015 at 02:00:06PM -0800, Russell Bryant wrote: > The physical input flows for child logical ports (for the > container-in-a-VM use case, for example) did not set a conntrack zone > ID. The previous code only allocated a zone ID for local VIFs and > missed doing it for child ports. > > Signed-off-by: Russell Bryant <russell@ovn.org> Justin, are you the right person to review this? I haven't done much with conntrack.
Thanks for fixing this. It might be nice to include a comment such as the following since it's not super obvious from a quick look what's being added: /* Add child logical port to the set of all local ports. */ Acked-by: Justin Pettit <jpettit@ovn.org> I'd suggest cherry-picking this to "branch-2.5", too. --Justin > On Nov 17, 2015, at 2:00 PM, Russell Bryant <russell@ovn.org> wrote: > > The physical input flows for child logical ports (for the > container-in-a-VM use case, for example) did not set a conntrack zone > ID. The previous code only allocated a zone ID for local VIFs and > missed doing it for child ports. > > Signed-off-by: Russell Bryant <russell@ovn.org> > --- > ovn/controller/binding.c | 7 ++++++- > 1 file changed, 6 insertions(+), 1 deletion(-) > > diff --git a/ovn/controller/binding.c b/ovn/controller/binding.c > index 7f31b31..89dca98 100644 > --- a/ovn/controller/binding.c > +++ b/ovn/controller/binding.c > @@ -144,7 +144,6 @@ binding_run(struct controller_ctx *ctx, const struct ovsrec_bridge *br_int, > /* We have no integration bridge, therefore no local logical ports. > * We'll remove our chassis from all port binding records below. */ > } > - update_ct_zones(&lports, ct_zones, ct_zone_bitmap); > sset_clone(&all_lports, &lports); > > ovsdb_idl_txn_add_comment( > @@ -155,6 +154,9 @@ binding_run(struct controller_ctx *ctx, const struct ovsrec_bridge *br_int, > if (sset_find_and_delete(&lports, binding_rec->logical_port) || > (binding_rec->parent_port && binding_rec->parent_port[0] && > sset_contains(&all_lports, binding_rec->parent_port))) { > + if (binding_rec->parent_port && binding_rec->parent_port[0]) { > + sset_add(&all_lports, binding_rec->logical_port); > + } > if (binding_rec->chassis == chassis_rec) { > continue; > } > @@ -173,6 +175,9 @@ binding_run(struct controller_ctx *ctx, const struct ovsrec_bridge *br_int, > SSET_FOR_EACH (name, &lports) { > VLOG_DBG("No port binding record for lport %s", name); > } > + > + update_ct_zones(&all_lports, ct_zones, ct_zone_bitmap); > + > sset_destroy(&lports); > sset_destroy(&all_lports); > } > -- > 2.5.0 >
On 12/10/2015 08:51 PM, Justin Pettit wrote: > Thanks for fixing this. It might be nice to include a comment such as the following since it's not super obvious from a quick look what's being added: > > /* Add child logical port to the set of all local ports. */ > > Acked-by: Justin Pettit <jpettit@ovn.org> > > I'd suggest cherry-picking this to "branch-2.5", too. Thanks for the review. I added your suggested comment, your ACK, and then pushed this to master and branch-2.5.
diff --git a/ovn/controller/binding.c b/ovn/controller/binding.c index 7f31b31..89dca98 100644 --- a/ovn/controller/binding.c +++ b/ovn/controller/binding.c @@ -144,7 +144,6 @@ binding_run(struct controller_ctx *ctx, const struct ovsrec_bridge *br_int, /* We have no integration bridge, therefore no local logical ports. * We'll remove our chassis from all port binding records below. */ } - update_ct_zones(&lports, ct_zones, ct_zone_bitmap); sset_clone(&all_lports, &lports); ovsdb_idl_txn_add_comment( @@ -155,6 +154,9 @@ binding_run(struct controller_ctx *ctx, const struct ovsrec_bridge *br_int, if (sset_find_and_delete(&lports, binding_rec->logical_port) || (binding_rec->parent_port && binding_rec->parent_port[0] && sset_contains(&all_lports, binding_rec->parent_port))) { + if (binding_rec->parent_port && binding_rec->parent_port[0]) { + sset_add(&all_lports, binding_rec->logical_port); + } if (binding_rec->chassis == chassis_rec) { continue; } @@ -173,6 +175,9 @@ binding_run(struct controller_ctx *ctx, const struct ovsrec_bridge *br_int, SSET_FOR_EACH (name, &lports) { VLOG_DBG("No port binding record for lport %s", name); } + + update_ct_zones(&all_lports, ct_zones, ct_zone_bitmap); + sset_destroy(&lports); sset_destroy(&all_lports); }
The physical input flows for child logical ports (for the container-in-a-VM use case, for example) did not set a conntrack zone ID. The previous code only allocated a zone ID for local VIFs and missed doing it for child ports. Signed-off-by: Russell Bryant <russell@ovn.org> --- ovn/controller/binding.c | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-)