Ilya Maximets <[email protected]> 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 <[email protected]>
>> ---
>>
>> 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.
_______________________________________________
dev mailing list
[email protected]
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
