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.maxim...@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 _______________________________________________ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev