[ovs-dev] [PATCH 00/12] Support zone-based conntrack timeout policy

[Ilya Maximets]

On 26.07.2019 2:24, Yi-Hung Wei wrote:
> This patch series enables zone-based conntrack timeout policy support in OVS.
> Timeout policy is a set of timeout attributes that can be associated with a
> connection when it is committed.  Then, the connection tracking system will
> expire a connection based on its connection state.  For example, one use
> case would be to extend the timeout of TCP connection in the established
> state to avoid re-connect overhead. Or use case is to shorten the connection
> timeout so that the system can reclaim resources faster.
> The idea of zone-based conntrack timeout policy is to group connections
> with similar characteristics in a conntrack zone, and assign timeout policy
> to the conntrack zone. Therefore, all the connections in that zone will share
> the same timeout policy.
> 
> For zone-based timeout policy configuration, the association of conntrack
> zone and conntrack timeout policy is defined per datapath in vswitch ovsdb
> schema.  User can program the database through ovs-vsctl or using ovsdb
> protocol directly.  Once the zone-based timeout policy configuration is
> in the database, vswitchd will read those configuration and orgaznie it
> in internal datapath strcture, and push the timeout policy into datapath.
> Currenlty, only the kernel datapath supports customized timeout policy.

Hi everyone,

My 2 cents for the feature design:

>From the user's perspective:

* 'add-dp'/'del-dp' commands looks very strange.
  "I didn't add datapath into ovsdb, why it exists and switches packets?"
  "I deleted the datapath from the OVS, why it still exists and switches packets?"

  If you're implementing the configuration like this, 'datapath' should
  own the bridges and interfaces, i.e. datapath should be created manually
  on 'add-dp' and automatically on adding the first bridge on that datapath.
  All the bridges and interfaces must be deleted/destroyed on 'del-dp'.

  Or you need to rename your tables and commands to not look like this.

>From the developer's perspective:

* Right now 'ofproto-dpif' is the only module that manages datapath interfaces
  and it knows that (there are specific comments in the code). 'dpif's has
  no reference counts and it's slightly unsafe to manage them outside of
  'ofproto-dpif'.
  You're adding the side module that allowed to open dpif (and it's not able
  to delete it, that is the possible cause if issues) and use it without
  noticing any other modules. This breaks the hierarchical structure of OVS.

* Right now most of the datapath configuration is done via 'other_config'
  and corresponding dpif_set_config() callback. Since you're introducing
  datapath-config module, it should take care of all of this staff. And this
  will require significant rework of the current datapath configuration scheme.

* 'reconfigure_datapath' is an ambiguous name.

  Solution for above issues might be not introducing the new modules at all.
  Everything could be handled like we're handling meters, but with OVSDB as the
  configuration source. On configuration change bridge layer will call ofproto
  layer that will pass configuration to ofproto-dpif and, finally, dpif layer.
  Inside 'struct dpif' in dpif.c module you could track all the configuration
  and pass all the required changes to the dpif-provider via callbacks.
  This way everything will work fine without breaking current OVS hierarchy.

* DB scheme looks just overcomplicated. 3 additional tables which references
  others just to store a string to integer map.
  I think that it might be much easier to create a single 'CT_Zones' table
  with all the required columns:
      'id', 'tcp_syn_sent', 'tcp_syn_recv', ..., 'icmp_reply'.
  This is not a big deal since you need to describe every single field in
  vswitch.xml anyway. This will also allow you to check support of particular
  field on the stage of adding value to the database.
  If you really need to distinguish zones by the datapath type (which is not
  obvious), you may add 'datapath_type' column, just like we have in a 'Bridge'
  table.


Best regards, Ilya Maximets.