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

Yi-Hung Wei yihung.wei at gmail.com
Mon Jul 29 18:53:29 UTC 2019


Hi Ilya,

Thanks for your comment.

On Mon, Jul 29, 2019 at 2:22 AM Ilya Maximets <i.maximets at samsung.com> wrote:
>
> 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.

Thanks for your suggestion about the datapath-config part. I think it
makes sense to implement what datapath-config does inside dpif.c
rather than introduce a new module.  I will make proper change for
that in v2.


>
> * 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.

As for the database schema, we intend to make CT_Zone table references
to CT_Timeout_Policy table because some other zone-based feature can
be configured through ovsdb later on. For example, we can have a new
column in CT_Zone table that stores 'limit' as an integer to support
the zone limit feature (limiting number of connection in a zone).  It
is currently configured through dpctl commands.

I understand your concern on the complication that introduced by the
datapath table.  Let me think about it more carefully and go back to
you later.

Thanks,

-Yi-Hung


More information about the dev mailing list