On Tue, Jul 26, 2016 at 6:46 AM, <[email protected]> wrote:
> From: Babu Shanmugam <[email protected]>
>
> Commit 263064a (Convert binding_run to incremental processing.) removed
> the usage
> of all_lports from binding_run, but it is infact used in the context of
> the caller,
> especially by update_ct_zones().
>
> Without this change, update_ct_zones operates on an empty set always.
>
> Signed-off-by: Babu Shanmugam <[email protected]>
>
Ouch. This is a really bad regression. If I understand correctly, we're
not setting a ct zone ID for any logical ports. All are just using the
default zone of 0.
We should think about a good way to test OVN's use of conntrack zones to
ensure that entries end up in separate zones for separate ports. A good
test for that may require userspace conntrack support, though.
Another test we could do now would be looking at the flows in table 0 and
ensuring that the input flow for each port has a different conntrack zone
ID assigned. That feels like kind of a hack, though.
---
> ovn/controller/binding.c | 4 +++-
> ovn/controller/binding.h | 3 ++-
> ovn/controller/ovn-controller.c | 2 +-
> 3 files changed, 6 insertions(+), 3 deletions(-)
>
> diff --git a/ovn/controller/binding.c b/ovn/controller/binding.c
> index e83c1d5..7bc6fb4 100644
> --- a/ovn/controller/binding.c
> +++ b/ovn/controller/binding.c
> @@ -230,7 +230,8 @@ consider_local_datapath(struct controller_ctx *ctx,
>
> void
> binding_run(struct controller_ctx *ctx, const struct ovsrec_bridge
> *br_int,
> - const char *chassis_id, struct hmap *local_datapaths)
> + const char *chassis_id, struct hmap *local_datapaths,
> + struct sset *all_lports)
> {
> const struct sbrec_chassis *chassis_rec;
> const struct sbrec_port_binding *binding_rec;
> @@ -292,6 +293,7 @@ binding_run(struct controller_ctx *ctx, const struct
> ovsrec_bridge *br_int,
> }
> }
>
> + sset_clone(all_lports, &local_ids);
>
I don't think this is quite sufficient. It's missing, at least:
- the IDs of sub-ports
- localnet ports
The old handling of building up all_lports ensure those got added.
> shash_destroy(&lport_to_iface);
> }
>
> diff --git a/ovn/controller/binding.h b/ovn/controller/binding.h
> index 8753d44..fbd16c8 100644
> --- a/ovn/controller/binding.h
> +++ b/ovn/controller/binding.h
> @@ -29,7 +29,8 @@ struct sset;
> void binding_register_ovs_idl(struct ovsdb_idl *);
> void binding_reset_processing(void);
> void binding_run(struct controller_ctx *, const struct ovsrec_bridge
> *br_int,
> - const char *chassis_id, struct hmap *local_datapaths);
> + const char *chassis_id, struct hmap *local_datapaths,
> + struct sset *all_lports);
> bool binding_cleanup(struct controller_ctx *, const char *chassis_id);
>
> #endif /* ovn/binding.h */
> diff --git a/ovn/controller/ovn-controller.c
> b/ovn/controller/ovn-controller.c
> index 4d9490a..6a6bb93 100644
> --- a/ovn/controller/ovn-controller.c
> +++ b/ovn/controller/ovn-controller.c
> @@ -425,7 +425,7 @@ main(int argc, char *argv[])
> if (chassis_id) {
> chassis_run(&ctx, chassis_id);
> encaps_run(&ctx, br_int, chassis_id);
> - binding_run(&ctx, br_int, chassis_id, &local_datapaths);
> + binding_run(&ctx, br_int, chassis_id, &local_datapaths,
> &all_lports);
> }
>
> if (br_int && chassis_id) {
> --
> 2.5.5
>
> _______________________________________________
> dev mailing list
> [email protected]
> http://openvswitch.org/mailman/listinfo/dev
>
--
Russell Bryant
_______________________________________________
dev mailing list
[email protected]
http://openvswitch.org/mailman/listinfo/dev
