On 6/30/23 03:38, Thomas Bachman wrote: > Hi, > > It wasn't clear to me from the responses whether there was an action item - > is something needed from the submitte? )Or is the patch under consideration?
Hi. I replied to the patch now. The patch itself has extra spacing between lines, so it wasn't recognized as a patch by any of our automated systems (like patchwork or our CI robot) and fell through the cracks in the end. Sorry about that. It needs to be re-sent with correct formatting. Best regards, Ilya Maximets. > > cheers, > > -Thomas > > > On Wed, May 24, 2023 at 9:08 AM Adrian Moreno <[email protected] > <mailto:[email protected]>> 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: [email protected] <mailto:[email protected]> > <[email protected] <mailto:[email protected]>> > >> 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 > [email protected] <mailto:[email protected]> > https://mail.openvswitch.org/mailman/listinfo/ovs-dev > <https://mail.openvswitch.org/mailman/listinfo/ovs-dev> > _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
