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

Darrell Ball dlu998 at gmail.com
Mon Jul 29 20:20:12 UTC 2019


On Mon, Jul 29, 2019 at 11:53 AM Yi-Hung Wei <yihung.wei at gmail.com> wrote:

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

I agree that a "datapath" should be auto-created when bridge of that type
is created.
This came up in internal e-mails and I lost track of it.

It is a bit strange to call this column 'datapath'; 'datapath-config' might
be a bit better and reflects
what it is really is. I think renaming would clear things up considerably
and avoid confusion.



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