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

Ilya Maximets i.maximets at samsung.com
Wed Jul 31 08:25:35 UTC 2019


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": {
---

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