[ovs-dev] [PATCH v6 ovn] ovn-northd: introduce new allow-stateless ACL verb

[Dumitru Ceara]

On 4/27/21 1:53 AM, Ihar Hrachyshka wrote:
> For allow-stateless ACLs, bypass connection tracking by avoiding
> setting ct hints for matching traffic. Avoid sending all traffic to ct
> when a stateful ACL is present.
> 
> ===
> 
> Reusing an existing 'allow' verb for stateless matching would have its
> drawbacks, specifically, when it comes to backwards incompatibility of
> the new behavior with existing environments. When using "allow" ACLs
> in mixed allow/allow-related environment, we still commit "allow"
> traffic to conntrack. This unnecessarily hits performance when mixed
> ACL action types were used for the same datapath. This is why we
> introduce a new action verb to describe stateless behavior.
> 
> Another complexity to consider is the fact that with stateless
> matching, one would not be able to rely on 'related' magic that
> guarantees that reply traffic is passed through. Instead, the user
> would have to accurately define matching rules both for request and
> reply directions of a protocol session. Specifically, when allowing
> ICMP for a specific peer host, one has to define 'allow-stateless'
> rules that would match against ip.dst for request direction and ip.src
> for reply direction. Other protocols and scenarios will require their
> own fine grained matching approaches implemented by the user.
> 
> ===
> 
> For performance measurements, ovn-fake-multinode environment and qperf
> were used. Performance measured between two virtual nodes, two ports
> that belong to different LSs connected via router. Using qperf,
> performance was measured for UDP, TCP, SCTP protocols (using
> <proto>_lat and <proto>_bw tests). The qperf version used:
> 0.4.9-16.fc31.x86_64.  Each test scenario was executed five times and
> averages compared.
> 
> Tests were executed with `allow-stateless` rules for the tested
> protocol and `allow-related` for another protocol set for both ports,
> both directions, e.g. for TCP scenario, the following ACLs were
> defined:
> 
> ovn-nbctl acl-add sw0 to-lport 100 tcp allow-stateless
> ovn-nbctl acl-add sw0 from-lport 100 tcp allow-stateless
> ovn-nbctl acl-add sw1 to-lport 100 tcp allow-stateless
> ovn-nbctl acl-add sw1 from-lport 100 tcp allow-stateless
> 
> ovn-nbctl acl-add sw0 to-lport 100 sctp allow-related
> ovn-nbctl acl-add sw0 from-lport 100 sctp allow-related
> ovn-nbctl acl-add sw1 to-lport 100 sctp allow-related
> ovn-nbctl acl-add sw1 from-lport 100 sctp allow-related
> 
> In this particular environment, improvement was seen in send_bw,
> latency, and msg_rate measurements, where applicable, for all three
> protocols under test.
> 
> for UDP, send_bw: 293.6 MB/sec => 313.2 MB/sec (+6.68%)
>          latency: 16 us => 14.08 us (-12%)
>          msg_rate: 62.56 K/sec => 71.06 K/sec (+13.59%)
> 
> for TCP, latency: 18.6 us => 14.88 us (-20%)
>          msg_rate: 53.8 K/sec => 67.28 K/sec (+25.06%)
> 
> for SCTP, latency: 21.98 us => 19.42 us (-11.65%)
>           msg_rate: 45.58 K/sec => 51.54 K/sec (+13.08%)
> 
> Interestingly, some performance improvement was also seen for the same
> scenarios with no ACLs set at all, albeit significantly more
> negligible.
> 
> for UDP, send_bw: 320.0 MB/sec => 338.6 MB/sec (+5.81%)
>          latency: 13.74 us => 12.88 us (-6.68%)
>          msg_rate: 73.02 K/sec => 77.84 K/sec (+6.6%)
> 
> for TCP, latency: 15.62 us => 14.26 us (-9.54%)
>          msg_rate: 64.02 K/sec => 70.26 K/sec (+9.75%)
> 
> for SCTP, latency: 19.56 us => 18.16 us (-7.16%)
>           msg_rate: 51.16 K/sec => 55.12 K/sec (+7.74%)
> 
> Comparable numbers can be captured with iperf. It may be useful to run
> more tests in a more elaborate (bare metal) environment.
> 
> ===
> 
> The patch takes inspiration from a now abandoned patch:
> 
> "ovn-northd: Support mixing stateless/stateful ACLs with
> Stateless_Filter." by Dumitru Ceara.
> 
> The original patch assumed CMS doesn't require full flexibility of
> matching rules for stateless matching (for example, to be used by
> OpenShift). But other CMS interfaces may require the same
> customizability for stateless as well as stateful matching, like in
> OpenStack Neutron API. Which is why this patch reuses existing ACL
> object type to describe stateless rules.
> 
> Signed-off-by: Ihar Hrachyshka <ihrachys at redhat.com>
> 
> ---

A minor nit below (sorry I missed it in the previous iteration),
otherwise my ack stands:

Acked-by: Dumitru Ceara <dceara at redhat.com>

[...]

> diff --git a/ovn-nb.ovsschema b/ovn-nb.ovsschema
> index 29019809c..58932db14 100644
> --- a/ovn-nb.ovsschema
> +++ b/ovn-nb.ovsschema
> @@ -1,7 +1,7 @@
>  {
>      "name": "OVN_Northbound",
>      "version": "5.31.0",

We should bump the version to 5.32.0 because there were changes to the
schema in a backwards compatible way:

https://github.com/openvswitch/ovs/blob/master/Documentation/ref/ovsdb.7.rst#schemas