| Message ID | 1564097054-72663-1-git-send-email-yihung.wei@gmail.com |
|---|---|
| Headers | show |
| Series | Support zone-based conntrack timeout policy | expand |
I did my first round of review by just reading through the code. I plan to test it next week. btw, can you update NEWS? Thanks William On Thu, Jul 25, 2019 at 04:24:02PM -0700, Yi-Hung Wei wrote: > This patch series enables zone-based conntrack timeout policy support in OVS. > Timeout policy is a set of timeout attributes that can be associated with a > connection when it is committed. Then, the connection tracking system will > expire a connection based on its connection state. For example, one use > case would be to extend the timeout of TCP connection in the established > state to avoid re-connect overhead. Or use case is to shorten the connection > timeout so that the system can reclaim resources faster. > The idea of zone-based conntrack timeout policy is to group connections > with similar characteristics in a conntrack zone, and assign timeout policy > to the conntrack zone. Therefore, all the connections in that zone will share > the same timeout policy. > > For zone-based timeout policy configuration, the association of conntrack > zone and conntrack timeout policy is defined per datapath in vswitch ovsdb > schema. User can program the database through ovs-vsctl or using ovsdb > protocol directly. Once the zone-based timeout policy configuration is > in the database, vswitchd will read those configuration and orgaznie it > in internal datapath strcture, and push the timeout policy into datapath. > Currenlty, only the kernel datapath supports customized timeout policy. > > When a packet is committed to connection tracking system, during flow > translation in ofproto-dpif-xlate, vsiwtchd will lookup the internal > data structure to figure out which timeout policy to associate with > the connection. If timeout policy is not specified to the committed > zone, it defaults to the timeout policy in the default zone (zone 0). > If the timeout policy is not specified in the default zone, it defaults > to the system default timeouts. > > Here are some more details about each patch > * p01, p04, p06: Some utility functions. > * p02: Introduce ovsdb schema for ct timeout policy. > * p03: ovs-vsctl commands to configure zone-based ct timeout policy. > * p05: dpif interface to support ct timeout policy. > * p07: dpif-netlink implementation to support ct timeout policy. > * p08: Consume ct timeout policy configuration from ovsdb server, > keep it in interal data structure, and push configuration to > datapath. > * p09-10: Kernel datapath support for the new ct action attribute. > * p11: Translate timeout policy in ofproto-dpif-xlate > * p12: System traffic test > > Travis CI: > * https://travis-ci.org/YiHungWei/ovs/builds/563768546 > > Appveyor CI: > * https://ci.appveyor.com/project/YiHungWei/ovs/builds/26250549 > > > Ben Pfaff (1): > simap: Add utility function to help compare two simaps. > > Justin Pettit (1): > vswitchd: Add datapath, CT_Zone, and CT_Timeout_Policy tables. > > William Tu (1): > ovs-vsctl: Add datapath and CT zone commands. > > Yi-Hung Wei (9): > ct-dpif: Export ct_dpif_format_ipproto() > ct-dpif: Add conntrack timeout policy support in dpif layer > ct-dpif: Add timeout policy related utility functions. > dpif-netlink: Add conntrack timeout policy support > datapath-config: Consume datapath, CT_Zone, and CT_Timeout_Policy > tables > datapath: compat: Backport nf_conntrack_timeout support > datapath: Add support for conntrack timeout policy > ofproto-dpif-xlate: Translate timeout policy in ct action > system-traffic: Add zone-based conntrack timeout policy test > > acinclude.m4 | 7 + > datapath-windows/include/OvsDpInterfaceCtExt.h | 114 ++++++ > datapath-windows/ovsext/Netlink/NetlinkProto.h | 1 + > datapath/conntrack.c | 30 +- > datapath/linux/Modules.mk | 2 + > datapath/linux/compat/include/linux/openvswitch.h | 4 + > .../include/net/netfilter/nf_conntrack_timeout.h | 34 ++ > datapath/linux/compat/nf_conntrack_timeout.c | 102 +++++ > include/windows/automake.mk | 1 + > .../windows/linux/netfilter/nfnetlink_cttimeout.h | 0 > lib/automake.mk | 2 + > lib/ct-dpif.c | 117 +++++- > lib/ct-dpif.h | 60 +++ > lib/datapath-config.c | 409 +++++++++++++++++++ > lib/datapath-config.h | 27 ++ > lib/dpif-netdev.c | 11 + > lib/dpif-netlink.c | 436 +++++++++++++++++++++ > lib/dpif-netlink.h | 2 +- > lib/dpif-provider.h | 48 +++ > lib/netlink-conntrack.c | 363 +++++++++++++++++ > lib/netlink-conntrack.h | 29 ++ > lib/netlink-protocol.h | 1 + > lib/odp-util.c | 29 +- > lib/simap.c | 15 +- > lib/simap.h | 1 + > ofproto/ofproto-dpif-xlate.c | 23 ++ > tests/odp.at | 1 + > tests/ovs-vsctl.at | 20 +- > tests/system-kmod-macros.at | 9 + > tests/system-traffic.at | 65 +++ > tests/system-userspace-macros.at | 10 + > utilities/ovs-vsctl.8.in | 29 ++ > utilities/ovs-vsctl.c | 245 ++++++++++++ > vswitchd/bridge.c | 3 + > vswitchd/vswitch.ovsschema | 44 ++- > vswitchd/vswitch.xml | 254 +++++++++--- > 36 files changed, 2488 insertions(+), 60 deletions(-) > create mode 100644 datapath/linux/compat/include/net/netfilter/nf_conntrack_timeout.h > create mode 100644 datapath/linux/compat/nf_conntrack_timeout.c > create mode 100644 include/windows/linux/netfilter/nfnetlink_cttimeout.h > create mode 100644 lib/datapath-config.c > create mode 100644 lib/datapath-config.h > > -- > 2.7.4 > > _______________________________________________ > dev mailing list > dev@openvswitch.org > https://mail.openvswitch.org/mailman/listinfo/ovs-dev
On 26.07.2019 2:24, Yi-Hung Wei wrote: > This patch series enables zone-based conntrack timeout policy support in OVS. > Timeout policy is a set of timeout attributes that can be associated with a > connection when it is committed. Then, the connection tracking system will > expire a connection based on its connection state. For example, one use > case would be to extend the timeout of TCP connection in the established > state to avoid re-connect overhead. Or use case is to shorten the connection > timeout so that the system can reclaim resources faster. > The idea of zone-based conntrack timeout policy is to group connections > with similar characteristics in a conntrack zone, and assign timeout policy > to the conntrack zone. Therefore, all the connections in that zone will share > the same timeout policy. > > For zone-based timeout policy configuration, the association of conntrack > zone and conntrack timeout policy is defined per datapath in vswitch ovsdb > schema. User can program the database through ovs-vsctl or using ovsdb > protocol directly. Once the zone-based timeout policy configuration is > in the database, vswitchd will read those configuration and orgaznie it > in internal datapath strcture, and push the timeout policy into datapath. > Currenlty, only the kernel datapath supports customized timeout policy. 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. * 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. Best regards, Ilya Maximets.
On 29.07.2019 12:21, Ilya Maximets wrote: > On 26.07.2019 2:24, Yi-Hung Wei wrote: >> This patch series enables zone-based conntrack timeout policy support in OVS. >> Timeout policy is a set of timeout attributes that can be associated with a >> connection when it is committed. Then, the connection tracking system will >> expire a connection based on its connection state. For example, one use >> case would be to extend the timeout of TCP connection in the established >> state to avoid re-connect overhead. Or use case is to shorten the connection >> timeout so that the system can reclaim resources faster. >> The idea of zone-based conntrack timeout policy is to group connections >> with similar characteristics in a conntrack zone, and assign timeout policy >> to the conntrack zone. Therefore, all the connections in that zone will share >> the same timeout policy. >> >> For zone-based timeout policy configuration, the association of conntrack >> zone and conntrack timeout policy is defined per datapath in vswitch ovsdb >> schema. User can program the database through ovs-vsctl or using ovsdb >> protocol directly. Once the zone-based timeout policy configuration is >> in the database, vswitchd will read those configuration and orgaznie it >> in internal datapath strcture, and push the timeout policy into datapath. >> Currenlty, only the kernel datapath supports customized timeout policy. > > 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. > > * 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. Another option is a single 'CT_Zones' table with 'id' and 'timeouts' columns. Where 'timeouts' is a map {string --> integer}. So, something like this will work: ovs-vsctl create CT_Zones 1 -- set CT_Zones 1 timeouts:icmp_first=60 timeouts:icmp_reply=60 > 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. > > > Best regards, Ilya Maximets. >
On Fri, Jul 26, 2019 at 2:19 PM William Tu <u9012063@gmail.com> wrote: > > > I did my first round of review by just reading through the code. > I plan to test it next week. > > btw, can you update NEWS? > > Thanks > William Thanks William for review. I will address your comments and add NEWS in v2. Thank, -Yi-Hung
Hi Ilya, Thanks for your comment. On Mon, Jul 29, 2019 at 2:22 AM Ilya Maximets <i.maximets@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
On Mon, Jul 29, 2019 at 11:53 AM Yi-Hung Wei <yihung.wei@gmail.com> wrote: > Hi Ilya, > > Thanks for your comment. > > On Mon, Jul 29, 2019 at 2:22 AM Ilya Maximets <i.maximets@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 >
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@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
On Wed, Jul 31, 2019 at 1:25 AM Ilya Maximets <i.maximets@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@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 >
> On Jul 31, 2019, at 1:25 AM, Ilya Maximets <i.maximets@samsung.com> wrote: > > On 29.07.2019 21:53, Yi-Hung Wei wrote: >> >> 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: The reason we arranged it like this is that we wanted to be able to allow per-flow timeout policies later. The idea is that in the Bridge or Datapath table we could have a column that goes from integer to timeout policy row. Those integers could then be used in OpenFlow ct(commit) actions. This would allow a hierarchy of timeout policies from flow -> zone -> system. --Justin
On 31.07.2019 22:49, Justin Pettit wrote: > >> On Jul 31, 2019, at 1:25 AM, Ilya Maximets <i.maximets@samsung.com> wrote: >> >> On 29.07.2019 21:53, Yi-Hung Wei wrote: >>> >>> 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: > > The reason we arranged it like this is that we wanted to be able to allow per-flow timeout policies later. The idea is that in the Bridge or Datapath table we could have a column that goes from integer to timeout policy row. Those integers could then be used in OpenFlow ct(commit) actions. This would allow a hierarchy of timeout policies from flow -> zone -> system. Thanks for explanation. So, you're going to have some timeout policies that will not be assigned to any zone, but will be used by the flow rules directly. I'm not a conntrack expert, so I will not insist on simplification. But I'm curious, do you have some real-world use-cases for such a complex 3-level configuration? For me, it looks like most of the users will just configure a few zones and it'll be enough. Also, it might be not so easy to properly configure everything since flow will not control values stored in policy. It's hard for me to imagine why we can't just create enough number of different zones and use them than needed. Is there a hard limit on number of zones? I see that in kernel 'zone' is u16, so it seems fairly large. Best regards, Ilya Maximets.
This patch series enables zone-based conntrack timeout policy support in OVS. Timeout policy is a set of timeout attributes that can be associated with a connection when it is committed. Then, the connection tracking system will expire a connection based on its connection state. For example, one use case would be to extend the timeout of TCP connection in the established state to avoid re-connect overhead. Or use case is to shorten the connection timeout so that the system can reclaim resources faster. The idea of zone-based conntrack timeout policy is to group connections with similar characteristics in a conntrack zone, and assign timeout policy to the conntrack zone. Therefore, all the connections in that zone will share the same timeout policy. For zone-based timeout policy configuration, the association of conntrack zone and conntrack timeout policy is defined per datapath in vswitch ovsdb schema. User can program the database through ovs-vsctl or using ovsdb protocol directly. Once the zone-based timeout policy configuration is in the database, vswitchd will read those configuration and orgaznie it in internal datapath strcture, and push the timeout policy into datapath. Currenlty, only the kernel datapath supports customized timeout policy. When a packet is committed to connection tracking system, during flow translation in ofproto-dpif-xlate, vsiwtchd will lookup the internal data structure to figure out which timeout policy to associate with the connection. If timeout policy is not specified to the committed zone, it defaults to the timeout policy in the default zone (zone 0). If the timeout policy is not specified in the default zone, it defaults to the system default timeouts. Here are some more details about each patch * p01, p04, p06: Some utility functions. * p02: Introduce ovsdb schema for ct timeout policy. * p03: ovs-vsctl commands to configure zone-based ct timeout policy. * p05: dpif interface to support ct timeout policy. * p07: dpif-netlink implementation to support ct timeout policy. * p08: Consume ct timeout policy configuration from ovsdb server, keep it in interal data structure, and push configuration to datapath. * p09-10: Kernel datapath support for the new ct action attribute. * p11: Translate timeout policy in ofproto-dpif-xlate * p12: System traffic test Travis CI: * https://travis-ci.org/YiHungWei/ovs/builds/563768546 Appveyor CI: * https://ci.appveyor.com/project/YiHungWei/ovs/builds/26250549 Ben Pfaff (1): simap: Add utility function to help compare two simaps. Justin Pettit (1): vswitchd: Add datapath, CT_Zone, and CT_Timeout_Policy tables. William Tu (1): ovs-vsctl: Add datapath and CT zone commands. Yi-Hung Wei (9): ct-dpif: Export ct_dpif_format_ipproto() ct-dpif: Add conntrack timeout policy support in dpif layer ct-dpif: Add timeout policy related utility functions. dpif-netlink: Add conntrack timeout policy support datapath-config: Consume datapath, CT_Zone, and CT_Timeout_Policy tables datapath: compat: Backport nf_conntrack_timeout support datapath: Add support for conntrack timeout policy ofproto-dpif-xlate: Translate timeout policy in ct action system-traffic: Add zone-based conntrack timeout policy test acinclude.m4 | 7 + datapath-windows/include/OvsDpInterfaceCtExt.h | 114 ++++++ datapath-windows/ovsext/Netlink/NetlinkProto.h | 1 + datapath/conntrack.c | 30 +- datapath/linux/Modules.mk | 2 + datapath/linux/compat/include/linux/openvswitch.h | 4 + .../include/net/netfilter/nf_conntrack_timeout.h | 34 ++ datapath/linux/compat/nf_conntrack_timeout.c | 102 +++++ include/windows/automake.mk | 1 + .../windows/linux/netfilter/nfnetlink_cttimeout.h | 0 lib/automake.mk | 2 + lib/ct-dpif.c | 117 +++++- lib/ct-dpif.h | 60 +++ lib/datapath-config.c | 409 +++++++++++++++++++ lib/datapath-config.h | 27 ++ lib/dpif-netdev.c | 11 + lib/dpif-netlink.c | 436 +++++++++++++++++++++ lib/dpif-netlink.h | 2 +- lib/dpif-provider.h | 48 +++ lib/netlink-conntrack.c | 363 +++++++++++++++++ lib/netlink-conntrack.h | 29 ++ lib/netlink-protocol.h | 1 + lib/odp-util.c | 29 +- lib/simap.c | 15 +- lib/simap.h | 1 + ofproto/ofproto-dpif-xlate.c | 23 ++ tests/odp.at | 1 + tests/ovs-vsctl.at | 20 +- tests/system-kmod-macros.at | 9 + tests/system-traffic.at | 65 +++ tests/system-userspace-macros.at | 10 + utilities/ovs-vsctl.8.in | 29 ++ utilities/ovs-vsctl.c | 245 ++++++++++++ vswitchd/bridge.c | 3 + vswitchd/vswitch.ovsschema | 44 ++- vswitchd/vswitch.xml | 254 +++++++++--- 36 files changed, 2488 insertions(+), 60 deletions(-) create mode 100644 datapath/linux/compat/include/net/netfilter/nf_conntrack_timeout.h create mode 100644 datapath/linux/compat/nf_conntrack_timeout.c create mode 100644 include/windows/linux/netfilter/nfnetlink_cttimeout.h create mode 100644 lib/datapath-config.c create mode 100644 lib/datapath-config.h