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

Darrell Ball dlu998 at gmail.com
Wed Jul 31 14:59:43 UTC 2019


On Wed, Jul 31, 2019 at 1:25 AM Ilya Maximets <i.maximets at samsung.com>
wrote:

> On 29.07.2019 21:53, Yi-Hung Wei 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.
> >>
> >> 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.
>
> At least, since each zone could have only one timeout policy it's easy to
> just
> inline CT_Timeout_Policy into CT_Zone like this:
>
> diff --git a/vswitchd/vswitch.ovsschema b/vswitchd/vswitch.ovsschema
> index 17aed1fc3..8ee860f19 100644
> --- a/vswitchd/vswitch.ovsschema
> +++ b/vswitchd/vswitch.ovsschema
> @@ -649,15 +649,6 @@
>           "type": {"key": "string", "value": "string",
>                    "min": 0, "max": "unlimited"}}}},
>     "CT_Zone": {
> -     "columns": {
> -       "timeout_policy": {
> -         "type": {"key": {"type": "uuid",
> -                          "refTable": "CT_Timeout_Policy"},
> -                  "min": 0, "max": 1}},
> -       "external_ids": {
> -         "type": {"key": "string", "value": "string",
> -                  "min": 0, "max": "unlimited"}}}},
> -   "CT_Timeout_Policy": {
>       "columns": {
>         "timeouts": {
>           "type": {"key": "string",
> @@ -667,8 +658,7 @@
>                    "min": 0, "max": "unlimited"}},
>         "external_ids": {
>           "type": {"key": "string", "value": "string",
> -                  "min": 0, "max": "unlimited"}}},
> -       "indexes": [["timeouts"]]},
> +                  "min": 0, "max": "unlimited"}}}},
>     "SSL": {
>       "columns": {
>         "private_key": {
> ---
>

There could be many policies associated with a CT_ZONE
Each of these policies could have many policy attributes themselves.

If we were to flatten all policies to their policy attributes, as you
suggest,
it would still be functionally correct.
Grouping the policy attributes associated with each policy is just a way to
express which policy attributes (PA) belong to which policy (P).

PAw, PAx -> P1

PAy, PAz -> P2


> If you will need additional column like 'conn_limit', just add a new
> column.
> All the timeouts will be grouped in 'timeouts' simap and it'll be obvious
> that
> 'conn_limit' is not a part of timeout policy.
>
> Best regards, Ilya Maximets.
>
> >
> > 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