cheers,
-Thomas
On Wed, May 24, 2023 at 9:08 AM Adrian Moreno <amore...@redhat.com
<mailto:amore...@redhat.com>> wrote:
On 5/23/23 22:26, Ilya Maximets wrote:
> On 5/23/23 20:51, Sayali Naval (sanaval) via dev wrote:
>> Does anyone have any insights on the below?
>> ________________________________
>> From: Sayali Naval (sanaval)
>> Sent: Friday, May 12, 2023 11:10 AM
>> To: d...@openvswitch.org <mailto:d...@openvswitch.org> <d...@openvswitch.org
<mailto:d...@openvswitch.org>>
>> Subject: [ovs-dev] ovs-vsctl Bridge IPFIX enable_input_sampling,
enable_ouput_sampling and enable_tunnel_sampling returning unexpected values
>>
>>
>> As per the Open vSwitch Manual
(http://www.openvswitch.org/support/dist-docs/ovs-vsctl.8.txt
<http://www.openvswitch.org/support/dist-docs/ovs-vsctl.8.txt>) the Bridge IPFIX
parameters can be passed as follows:
>>
>> ovs-vsctl -- set Bridge br0 ipfix=@i \
>> -- --id=@i create IPFIX targets=\"192.168.0.34:4739
<http://192.168.0.34:4739>\" obs_do‐
>> main_id=123 obs_point_id=456
cache_active_timeout=60
>> cache_max_flows=13 \
>> other_config:enable-input-sampling=false \
>> other_config:enable-output-sampling=false \
>> other_config:enable-tunnel-sampling=true
>>
>> where the default values are:
>>
>> enable_input_sampling: true
>> enable_output_sampling: true
>> enable_tunnel_sampling: false
>>
I think wording is very confusing. In ovs-vsctl(8), before the example you
provide, it says:
"
Configure bridge br0 to send one IPFIX flow record per packet
sample to UDP port 4739 on host 192.168.0.34, with Observation Domain
ID 123 and Observation Point ID 456, a flow cache active timeout of 1
minute (60 seconds), maximum flow cache size of 13 flows, and flows
sampled on output port with tunnel info(sampling on input and output
port is enabled by default if not disabled) :
"
It doesn't explicitly say what's the default value for
"other_config:enable-tunnel-sampling". On the other hand, the
ovs-vswitchd.conf.db(5) says:
"
other_config : enable-input-sampling: optional string, either true or false
By default, Open vSwitch samples and reports flows at bridge port
input in
IPFIX flow records. Set this column to false to disable input sampling.
other_config : enable-output-sampling: optional string, either true or
false
By default, Open vSwitch samples and reports flows at bridge port
output in
IPFIX flow records. Set this column to false to disable output sampling.
"
One might interpret this as 'you can only set this to "false", since
"true" is
the default value'. If it was possible to forbid the user to put "true" in
the
field, maybe the current implementation would be reasonable. But it's not.
Now for the tunnel-sampling:
"
other_config : enable-tunnel-sampling: optional string, either true or
false
Set to true to enable sampling and reporting tunnel header 7-tuples in
IPFIX flow records. Tunnel sampling is enabled by default.
"
So the default value is true after all!
>> But in the existing code
https://github.com/openvswitch/ovs/blob/master/vswitchd/bridge.c#L1560-L1567
<https://github.com/openvswitch/ovs/blob/master/vswitchd/bridge.c#L1560-L1567>, these
3 parameters take up unexpected values in some scenarios.
>>
>> Consider the following case for enable_input_sampling and
enable_output_sampling:
>>
>> be_opts.enable_input_sampling =
!smap_get_bool(&be_cfg->other_config,
>> "enable-input-sampling",
false);
>>
>> be_opts.enable_output_sampling =
!smap_get_bool(&be_cfg->other_config,
>>
"enable-output-sampling", false);
>>
>> Here, the function smap_get_bool is being used with a negation.
>>
>> smap_get_bool is defined as below:
>> (https://github.com/openvswitch/ovs/blob/master/lib/smap.c#L220-L232
<https://github.com/openvswitch/ovs/blob/master/lib/smap.c#L220-L232>)
>>
>> /* Gets the value associated with 'key' in 'smap' and converts it to a
boolean.
>> * If 'key' is not in 'smap', or its value is neither "true" nor
"false",
>> * returns 'def'. */
>> bool
>> smap_get_bool(const struct smap *smap, const char *key, bool def)
>> {
>> const char *value = smap_get_def(smap, key, "");
>>
>> if (def) {
>> return strcasecmp("false", value) != 0;
>> } else {
>> return !strcasecmp("true", value);
>> }
>> }
>>
>> This returns expected values for the default case (since the above code
will negate “false” we get from smap_get bool function and return the value “true”)
but unexpected values for the case where the sampling value is passed through the CLI.
>>
>> For example, if we pass "true" for other_config:enable-input-sampling
in the CLI, the above code will negate the “true” value we get from the smap_bool function and
return the value “false”. Same would be the case for enable_output_sampling.
>>
>> My proposed solution is as below:
>>
>> be_opts.enable_input_sampling =
smap_get_bool(&be_cfg->other_config,
>> "enable-input-sampling",
true);
>>
>> be_opts.enable_output_sampling =
smap_get_bool(&be_cfg->other_config,
>>
"enable-output-sampling", true);
>>
>> Here, by removing the negation on the smap_get bool function, we make
sure the CLI value is returned as expected and by changing the 3rd parameter to
“true”, we return the default value “true” as expected.
>
> Yeah, the negation is very strange. I'd expect the code be implemented
> the way you're suggesting. Not sure why it was done this way in the
> first place.
>
+1 this implementation makes more sense.
>>
>> Now, consider the following case for enable_tunnel_sampling:
>>
>> be_opts.enable_tunnel_sampling =
smap_get_bool(&be_cfg->other_config,
>> "enable-tunnel-sampling",
true);
>>
>> Here again the smap_get_bool function is being used (without a
negation).
>>
>> This returns unexpected value for the default case (since expected
value is false and smap_get_bool function will return true) and expected value for
the case where the sampling value is passed through the CLI.
>>
Note the default value is true according to ovs-vswitchd.conf.db(5) so I
think
this implementation is OK.
>> My proposed solution is as below:
>>
>> be_opts.enable_tunnel_sampling =
smap_get_bool(&be_cfg->other_config,
>> "enable-tunnel-sampling",
false);
>>
>> Here, by changing the 3rd parameter to “false”, we return the default
value “false” as expected.
>
> Yeah, this is also weird. The option is documented in
vswitchd/vswitch.xml
> as disabled by default, while in fact it is enabled by default.
> It's been this way for 9 years already. So, it might make sense to keep
it
> enabled, but fix the documentation instead.
>
> Adrian, do you have any thoughts on all that?
>
>>
>> I am happy to discuss more if there are any additional thoughts around
this.
>> If this looks like a good fix, I can go ahead and send out a PR for
review.
>
> FWIW, it's better to send patches to the mailing list instead as most of
> our automation is based on the mailing list workflow.
>
> Best regards, Ilya Maximets.
>
>> Thanks & Regards,
>>
>> Sayali Naval
>
--
Adrián Moreno
_______________________________________________
dev mailing list
d...@openvswitch.org <mailto:d...@openvswitch.org>
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
<https://mail.openvswitch.org/mailman/listinfo/ovs-dev>
