> On Jul 26, 2016, at 6:13 PM, Russell Bryant <russ...@ovn.org> wrote:
>
>> On Tue, Jul 26, 2016 at 6:46 AM, <bscha...@redhat.com> wrote:
>>
>> From: Babu Shanmugam <bscha...@redhat.com>
>>
>> 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 <bscha...@redhat.com>
>
> 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.
I have added a couple of ovn tests in system-ovn.at that leverage kernel module
and conntrack. A basic test for firewall can be added.
>
> 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
>> dev@openvswitch.org
>> http://openvswitch.org/mailman/listinfo/dev
>
>
>
> --
> Russell Bryant
> _______________________________________________
> dev mailing list
> dev@openvswitch.org
> http://openvswitch.org/mailman/listinfo/dev
_______________________________________________
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
