[ovs-dev] [RFC PATCH v1 1/3] OVN ACL: Replace the usage of ct_label with ct_mark

[Ankur Sharma]

Hi Ben,

Thanks a lot for the review.
Sure, I will add comments in logical-fields.c explaining the reason for retaining ct_label.blocked.
I will rename ct_mark.blocked to ct.blocked as well.

V2 will have all these changes.

Thanks again for review.

Regards,
Ankur

-----Original Message-----
From: Ben Pfaff <blp at ovn.org> 
Sent: Tuesday, February 5, 2019 1:21 PM
To: Ankur Sharma <ankur.sharma at nutanix.com>
Cc: ovs-dev at openvswitch.org
Subject: Re: [ovs-dev] [RFC PATCH v1 1/3] OVN ACL: Replace the usage of ct_label with ct_mark

On Fri, Jan 11, 2019 at 12:16:35AM +0000, Ankur Sharma wrote:
> OVN ACL implementation used ct_label to indicate if a previosuly 
> allowed connection shoudl not be allowed anymore and vice versa.
> 
> However, ct_label is a 128 bit value and we should rather leverage on 
> ct_mark which is a 32 bit value.
> 
> Using ct_mark for this purpose, allows us to use ct_label for storing 
> other values like, identifier for corresponidng OVN ACL/Security group etc.
> 
> signed-off-by: Ankur Sharma <ankur.sharma at nutanix.com>

Thanks for the patch.

I think that the idea here is to retain the existing ct_label.blocked for compatibility with older ovn-northd, so that during an upgrade the older logical flows continue to work.  That is a good idea.  I think that there should be a comment in logical-fields.c explaining why ct_label.blocked is still there.  Then, someday in the future, when we think that upgrades from such an old version is no longer important, we will have an idea why it is there and that we can now to remove it.

I find myself wondering, though, why we have ct_label.blocked at all.
In some other cases where ovn-northd uses a bit specifically, it has a macro for it, e.g. REGBIT_CONNTRACK_COMMIT.  Another option would be to have better abstraction, i.e. to name the bit "ct.blocked" instead of ct_mark.blocked or ct_label.blocked.

Thanks,

Ben.