| Message ID | 1653105300-28434-1-git-send-email-lic121@chinatelecom.cn |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | [ovs-dev,v4] dpif-netdev: ct maxconns config persistence | expand |
| Context | Check | Description |
|---|---|---|
| ovsrobot/apply-robot | success | apply and check: success |
| ovsrobot/intel-ovs-compilation | fail | test: fail |
| ovsrobot/github-robot-_Build_and_Test | fail | github build: failed |
lic121 <lic121@chinatelecom.cn> writes: > Max allowed userspace dp conntrack entries is configurable with > 'ovs-appctl dpctl/ct-set-maxconns' command. In real scenarios, > this configuration is expected to survive from host reboot, from > ovs service restart. > > Signed-off-by: lic121 <lic121@chinatelecom.cn> > --- Acked-by: Aaron Conole <aconole@redhat.com>
On 5/21/22 05:55, lic121 wrote: > Max allowed userspace dp conntrack entries is configurable with > 'ovs-appctl dpctl/ct-set-maxconns' command. In real scenarios, > this configuration is expected to survive from host reboot, from > ovs service restart. > > Signed-off-by: lic121 <lic121@chinatelecom.cn> > --- > > Notes: > v4: > - add '\n' for warning msg > v3: > - add a warning to dpctl_ct_set_maxconns > - add NEWS entry > v2: > - rename "ct-maxconns" to "userspace-ct-maxconns" > > NEWS | 5 +++++ > lib/dpctl.c | 3 +++ > lib/dpctl.man | 4 +++- > lib/dpif-netdev.c | 11 +++++++++++ > tests/system-traffic.at | 10 ++++++++++ > vswitchd/vswitch.xml | 7 +++++++ > 6 files changed, 39 insertions(+), 1 deletion(-) Hi. Sorry for the late reply. The ct configuration in general seems to be a complete mess at the moment and I agree that this kind of configuration should be persistent, i.e. stored in the database. I'm not sure about the exact implementation though. First of all, It doesn't seem great to have some options configured via database and some still via dpctl. I think, it's better to move all options at once and deprecate all the old APIs to avoid confusion in the future. Secondly, while most of the options are strictly for userspace, some are not, e.g. ct-set-limits. And dpctl provides a unified interface for all of them, so we should probably have unified database configuration knobs as well. Userspace-only options will need to have a note in the documentation and also trigger a warning in the log if configured for a system datapath type. Suggesting a schema change like this: diff --git a/vswitchd/vswitch.ovsschema b/vswitchd/vswitch.ovsschema index 4873cfde7..4dad41d4c 100644 --- a/vswitchd/vswitch.ovsschema +++ b/vswitchd/vswitch.ovsschema @@ -656,6 +656,28 @@ "value": {"type": "uuid", "refTable": "CT_Zone"}, "min": 0, "max": "unlimited"}}, + "ct_maxconns": { + "type": {"key": {"type": "integer", + "minInteger": 0, + "maxInteger" : 4294967295}, + "min": 0, "max": 1}}, + "ct_tcp_seq_check": { + "type": {"key": {"type": "boolean"}, + "min": 0, "max": 1}}, + "ct_ipf_enabled": { + "type": {"key": {"type": "string", "enum": ["set", ["v4", "v6"]]}, + "value": "boolean", + "min": 0, "max": 2}}, + "ct_ipf_min_frag_size": { + "type": {"key": {"type": "string", "enum": ["set", ["v4", "v6"]]}, + "value": {"type": "integer", + "minInteger": 400, "maxInteger": 5000}, + "min": 0, "max": 2}}, + "ct_ipf_max_nfrag": { + "type": {"key": {"type": "integer", + "minInteger": 0, + "maxInteger" : 4294967295}, + "min": 0, "max": 1}}, "capabilities": { "type": {"key": "string", "value": "string", "min": 0, "max": "unlimited"}}, @@ -668,6 +690,10 @@ "type": {"key": {"type": "uuid", "refTable": "CT_Timeout_Policy"}, "min": 0, "max": 1}}, + "limit": {"key": {"type": "integer", + "minInteger": 0, + "maxInteger" : 4294967295}, + "min": 0, "max": 1}}, "external_ids": { "type": {"key": "string", "value": "string", "min": 0, "max": "unlimited"}}}}, --- All the configs are optional, but if they are specified, datapath_reconfigure() should take the values and update the corresponding configuration via ofproto_ct_* API calls. Failures to do so, either due to no support from the underlying datapath or for other reasons should be logged at WARN level. Corresponding ovs-vsctl commands should be added to set these values. All the dpctl commands should be deprecated, at least 'set' ones. We may also deprecate all the 'get' commands as well and replace all of them with a single 'show'-type of a command, i.e. conntrack/show or something like that. What do you think? Aaron, Paolo? One caveat here is that entries in the 'Datapath' table are not created automatically, which is also weird. Though, should not be very hard to create them once all the datapath types are registered, maybe inside the same datapath_reconfigure() function. Best regards, Ilya Maximets.
Ilya Maximets <i.maximets@ovn.org> writes: > On 5/21/22 05:55, lic121 wrote: >> Max allowed userspace dp conntrack entries is configurable with >> 'ovs-appctl dpctl/ct-set-maxconns' command. In real scenarios, >> this configuration is expected to survive from host reboot, from >> ovs service restart. >> >> Signed-off-by: lic121 <lic121@chinatelecom.cn> >> --- >> >> Notes: >> v4: >> - add '\n' for warning msg >> v3: >> - add a warning to dpctl_ct_set_maxconns >> - add NEWS entry >> v2: >> - rename "ct-maxconns" to "userspace-ct-maxconns" >> >> NEWS | 5 +++++ >> lib/dpctl.c | 3 +++ >> lib/dpctl.man | 4 +++- >> lib/dpif-netdev.c | 11 +++++++++++ >> tests/system-traffic.at | 10 ++++++++++ >> vswitchd/vswitch.xml | 7 +++++++ >> 6 files changed, 39 insertions(+), 1 deletion(-) > > Hi. Sorry for the late reply. > > The ct configuration in general seems to be a complete mess at the > moment and I agree that this kind of configuration should be > persistent, i.e. stored in the database. I'm not sure about the > exact implementation though. > > First of all, It doesn't seem great to have some options configured > via database and some still via dpctl. I think, it's better to move > all options at once and deprecate all the old APIs to avoid confusion > in the future. +1 > Secondly, while most of the options are strictly for userspace, some > are not, e.g. ct-set-limits. And dpctl provides a unified interface > for all of them, so we should probably have unified database > configuration knobs as well. Userspace-only options will need to > have a note in the documentation and also trigger a warning in the > log if configured for a system datapath type. I'm not sure about that part. I do like having the knobs explicitly per-datapath because it will probably never make sense to have OVS configure the netlink datapath (no matter the OS). I guess it is a bit of a bike shed at the moment, but I want to get this right. Warnings and "read the docs" are an okay alternative, but I do prefer specific knobs for, ex. max-conns, because the vswitchd is carrying its own CT implementation. > Suggesting a schema change like this: > > diff --git a/vswitchd/vswitch.ovsschema b/vswitchd/vswitch.ovsschema > index 4873cfde7..4dad41d4c 100644 > --- a/vswitchd/vswitch.ovsschema > +++ b/vswitchd/vswitch.ovsschema > @@ -656,6 +656,28 @@ > "value": {"type": "uuid", > "refTable": "CT_Zone"}, > "min": 0, "max": "unlimited"}}, > + "ct_maxconns": { > + "type": {"key": {"type": "integer", > + "minInteger": 0, > + "maxInteger" : 4294967295}, > + "min": 0, "max": 1}}, > + "ct_tcp_seq_check": { > + "type": {"key": {"type": "boolean"}, > + "min": 0, "max": 1}}, > + "ct_ipf_enabled": { > + "type": {"key": {"type": "string", "enum": ["set", ["v4", "v6"]]}, > + "value": "boolean", > + "min": 0, "max": 2}}, > + "ct_ipf_min_frag_size": { > + "type": {"key": {"type": "string", "enum": ["set", ["v4", "v6"]]}, > + "value": {"type": "integer", > + "minInteger": 400, "maxInteger": 5000}, > + "min": 0, "max": 2}}, > + "ct_ipf_max_nfrag": { > + "type": {"key": {"type": "integer", > + "minInteger": 0, > + "maxInteger" : 4294967295}, > + "min": 0, "max": 1}}, > "capabilities": { > "type": {"key": "string", "value": "string", > "min": 0, "max": "unlimited"}}, > @@ -668,6 +690,10 @@ > "type": {"key": {"type": "uuid", > "refTable": "CT_Timeout_Policy"}, > "min": 0, "max": 1}}, > + "limit": {"key": {"type": "integer", > + "minInteger": 0, > + "maxInteger" : 4294967295}, > + "min": 0, "max": 1}}, > "external_ids": { > "type": {"key": "string", "value": "string", > "min": 0, "max": "unlimited"}}}}, > --- > > All the configs are optional, but if they are specified, datapath_reconfigure() > should take the values and update the corresponding configuration via > ofproto_ct_* API calls. Failures to do so, either due to no support from the > underlying datapath or for other reasons should be logged at WARN level. > Corresponding ovs-vsctl commands should be added to set these values. > > All the dpctl commands should be deprecated, at least 'set' ones. We may > also deprecate all the 'get' commands as well and replace all of them with > a single 'show'-type of a command, i.e. conntrack/show or something like that. > > What do you think? Aaron, Paolo? It makes things "elegant" from a programmers perpective, but I don't know if it is really intuitive from a user perspective. > One caveat here is that entries in the 'Datapath' table are not created > automatically, which is also weird. Though, should not be very hard to create > them once all the datapath types are registered, maybe inside the same > datapath_reconfigure() function. > > Best regards, Ilya Maximets.
diff --git a/NEWS b/NEWS index eece0d0..e75b2af 100644 --- a/NEWS +++ b/NEWS @@ -29,6 +29,11 @@ Post-v2.17.0 - Windows: * Conntrack support for TCPv6, UDPv6, ICMPv6, FTPv6. * IPv6 Geneve tunnel support. + - Userspace datapath: + * 'ovs-appctl dpctl/ct-set-maxconns' is deprecated for lack of persistence + capabilitiy. + * New configuration knob 'other_config:userspace-ct-maxconns' to set + maximum number of connection tracker entries for userspace datapath. v2.17.0 - 17 Feb 2022 diff --git a/lib/dpctl.c b/lib/dpctl.c index 29041fa..ff64691 100644 --- a/lib/dpctl.c +++ b/lib/dpctl.c @@ -1990,6 +1990,9 @@ dpctl_ct_set_maxconns(int argc, const char *argv[], struct dpif *dpif; int error = opt_dpif_open(argc, argv, dpctl_p, 3, &dpif); if (!error) { + dpctl_print(dpctl_p, + "Warning: dpctl/ct-set-maxconns is deprecated by " + "other_config:userspace-ct-maxconns\n"); uint32_t maxconns; if (ovs_scan(argv[argc - 1], "%"SCNu32, &maxconns)) { error = ct_dpif_set_maxconns(dpif, maxconns); diff --git a/lib/dpctl.man b/lib/dpctl.man index c100d0a..4f08a3f 100644 --- a/lib/dpctl.man +++ b/lib/dpctl.man @@ -343,7 +343,9 @@ system due to connection tracking or simply limiting connection tracking. If the number of connections is already over the new maximum limit request then the new maximum limit will be enforced when the number of connections decreases to that limit, which normally happens -due to connection expiry. Only supported for userspace datapath. +due to connection expiry. Only supported for userspace datapath. This +command is deprecated by ovsdb cfg other_config:userspace-ct-maxconns +because of persistence capability. . .TP \*(DX\fBct\-get\-maxconns\fR [\fIdp\fR] diff --git a/lib/dpif-netdev.c b/lib/dpif-netdev.c index 21277b2..bfbe6db 100644 --- a/lib/dpif-netdev.c +++ b/lib/dpif-netdev.c @@ -4828,6 +4828,17 @@ dpif_netdev_set_config(struct dpif *dpif, const struct smap *other_config) } } + uint32_t ct_maxconns, cur_maxconns; + ct_maxconns = smap_get_int(other_config, "userspace-ct-maxconns", + UINT32_MAX); + /* Leave runtime value as it is when cfg is removed. */ + if (ct_maxconns < UINT32_MAX) { + conntrack_get_maxconns(dp->conntrack, &cur_maxconns); + if (ct_maxconns != cur_maxconns) { + conntrack_set_maxconns(dp->conntrack, ct_maxconns); + } + } + bool smc_enable = smap_get_bool(other_config, "smc-enable", false); bool cur_smc; atomic_read_relaxed(&dp->smc_enable_db, &cur_smc); diff --git a/tests/system-traffic.at b/tests/system-traffic.at index 1d20366..cb1eb16 100644 --- a/tests/system-traffic.at +++ b/tests/system-traffic.at @@ -2305,6 +2305,16 @@ AT_CHECK([ovs-appctl dpctl/ct-get-maxconns], [], [dnl 10 ]) +AT_CHECK([ovs-vsctl set Open_vswitch . other_config:userspace-ct-maxconns=20], [0]) +AT_CHECK([ovs-appctl dpctl/ct-get-maxconns], [], [dnl +20 +]) + +AT_CHECK([ovs-vsctl remove Open_vswitch . other_config userspace-ct-maxconns], [0]) +AT_CHECK([ovs-appctl dpctl/ct-get-maxconns], [], [dnl +20 +]) + OVS_TRAFFIC_VSWITCHD_STOP AT_CLEANUP diff --git a/vswitchd/vswitch.xml b/vswitchd/vswitch.xml index cc1dd77..f2324be 100644 --- a/vswitchd/vswitch.xml +++ b/vswitchd/vswitch.xml @@ -183,6 +183,13 @@ </p> </column> + <column name="other_config" key="userspace-ct-maxconns" + type='{"type": "integer", "minInteger": 0}'> + The maximum number of connection tracker entries allowed in the + userspace datapath. This deprecates "ovs-appctl dpctl/ct-set-maxconns" + command. + </column> + <column name="other_config" key="max-idle" type='{"type": "integer", "minInteger": 500}'> <p>
Max allowed userspace dp conntrack entries is configurable with 'ovs-appctl dpctl/ct-set-maxconns' command. In real scenarios, this configuration is expected to survive from host reboot, from ovs service restart. Signed-off-by: lic121 <lic121@chinatelecom.cn> --- Notes: v4: - add '\n' for warning msg v3: - add a warning to dpctl_ct_set_maxconns - add NEWS entry v2: - rename "ct-maxconns" to "userspace-ct-maxconns" NEWS | 5 +++++ lib/dpctl.c | 3 +++ lib/dpctl.man | 4 +++- lib/dpif-netdev.c | 11 +++++++++++ tests/system-traffic.at | 10 ++++++++++ vswitchd/vswitch.xml | 7 +++++++ 6 files changed, 39 insertions(+), 1 deletion(-)