On 6/24/24 19:38, Adrián Moreno wrote: > On Mon, Jun 24, 2024 at 04:15:50PM GMT, Ilya Maximets wrote: >> On 6/24/24 15:14, Adrián Moreno wrote: >>> On Mon, Jun 24, 2024 at 02:03:00PM GMT, Ilya Maximets wrote: >>>> On 6/5/24 22:23, Adrian Moreno wrote: >>>>> Test simultaneous IPFIX and local sampling including slow-path. >>>>> >>>>> Signed-off-by: Adrian Moreno <[email protected]> >>>>> --- >>>>> tests/system-common-macros.at | 4 ++ >>>>> tests/system-traffic.at | 105 ++++++++++++++++++++++++++++++++++ >>>>> 2 files changed, 109 insertions(+) >>>>> >>>> >>>> Not a full review, but see a few comments below. >>>> >>>>> diff --git a/tests/system-common-macros.at b/tests/system-common-macros.at >>>>> index 2a68cd664..22b8885e4 100644 >>>>> --- a/tests/system-common-macros.at >>>>> +++ b/tests/system-common-macros.at >>>>> @@ -378,3 +378,7 @@ m4_define([OVS_CHECK_GITHUB_ACTION], >>>>> # OVS_CHECK_DROP_ACTION() >>>>> m4_define([OVS_CHECK_DROP_ACTION], >>>>> [AT_SKIP_IF([! grep -q "Datapath supports explicit drop action" >>>>> ovs-vswitchd.log])]) >>>>> + >>>>> +# OVS_CHECK_EMIT_SAMPLE() >>>>> +m4_define([OVS_CHECK_EMIT_SAMPLE], >>>>> + [AT_SKIP_IF([! grep -q "Datapath supports emit_sample" >>>>> ovs-vswitchd.log])]) >>>>> diff --git a/tests/system-traffic.at b/tests/system-traffic.at >>>>> index bd7647cbe..babc56b56 100644 >>>>> --- a/tests/system-traffic.at >>>>> +++ b/tests/system-traffic.at >>>>> @@ -8977,3 +8977,108 @@ OVS_WAIT_UNTIL([cat p2.pcap | grep -E "0x0050: >>>>> *0000 *0000 *5002 *2000 *b85e *00 >>>>> >>>>> OVS_TRAFFIC_VSWITCHD_STOP >>>>> AT_CLEANUP >>>>> + >>>>> +AT_SETUP([emit_sample]) >>>> >>>> Other tests have a test group name at the beginning. >>>> You're also adding the test into NSH section. Add a new section, e.g., >>>> 'sampling' for this with AT_BANNER. >>>> >>> >>> Thanks. Will do. >>> >>>>> +OVS_TRAFFIC_VSWITCHD_START() >>>>> +OVS_CHECK_EMIT_SAMPLE() >>>>> + >>>>> +ADD_NAMESPACES(at_ns0, at_ns1) >>>>> +NS_CHECK_EXEC([at_ns0], [sysctl -w net.ipv6.conf.all.disable_ipv6=1], >>>>> [0], [ignore]) >>>>> +NS_CHECK_EXEC([at_ns1], [sysctl -w net.ipv6.conf.all.disable_ipv6=1], >>>>> [0], [ignore]) >>>> >>>> This looks strange, don't disable IPv6. Appropriate datapath actions >>>> should not forward IPv6 traffic, if that is a desired behavior. >>>> >>> >>> I found tests cleaner if automatic ipv6 traffic is not sent by ports. >>> But sure, I can deal with that with OpenFlow. >> >> Would be good to have two separate tests. One for IPv4 and one for IPv6. >> >>> >>>> Though, I guess, it would be nice to test capturing both IPv4 and IPv6. >>>> >>>>> + >>>>> +ADD_VETH(p0, at_ns0, br0, "10.1.1.1/24", "e4:11:22:33:44:55") >>>>> +ADD_VETH(p1, at_ns1, br0, "10.1.1.2/24", "e4:11:22:33:44:66") >>>>> + >>>>> +NS_CHECK_EXEC([at_ns0], [arp -s 10.1.1.2 e4:11:22:33:44:66]) >>>>> +NS_CHECK_EXEC([at_ns1], [arp -s 10.1.1.1 e4:11:22:33:44:55]) >>>>> + >>>>> + >>>>> +AT_CHECK([ovs-appctl vlog/set dpif:dbg dpif_netdev:dbg >>>>> ofproto_dpif_upcall:dbg]) >>>>> + >>>>> +AT_CHECK([ovs-vsctl -- --id=@br0 get Bridge br0 \ >>>>> + -- --id=@ipfix create IPFIX >>>>> targets=\"127.0.0.1:4739\" \ >>>>> + -- create Flow_Sample_Collector_Set id=1 bridge=@br0 >>>>> ipfix=@ipfix, local_sample_group=10 \ >>>>> + -- create Flow_Sample_Collector_Set id=2 bridge=@br0 >>>>> ipfix=@ipfix, local_sample_group=12], >>>>> + [0], [ignore]) >>>> >>>> Why are we adding IPFIX? This should work without IPFIX. >>>> >>> >>> It is intentional. I want to test both features work simulteaneously. >> >> Tests should be simple. At least, we should have at least one very simple >> test, so if it fails we know that there is an issue in the basic logic. >> >> It's valuable to test a combination of the two, but it should be a separate >> test. So, 3 tests so far: IPv4, IPv6, psample+ipfix. Maybe the 4th separate >> test for the slow path. >> > > Ack. > >>> >>>> Having both together can be a separate test. >>>> >>>>> + >>>>> +AT_DATA([flows.txt], [dnl >>>>> +in_port=ovs-p0,ip >>>>> actions=sample(probability=65535,collector_set_id=1,obs_domain_id=1431655765,obs_point_id=1717986918),output(port=ovs-p1,max_len=100) >>>>> +in_port=ovs-p1,ip >>>>> actions=sample(probability=65535,collector_set_id=2,obs_domain_id=2290649224,obs_point_id=2576980377),output(port=ovs-p0,max_len=100) >>>> >>>> It should be possible to wrap these lines a little with dnl. >>>> >>>> Also, please make IDs non-symmetric, so we can't check that the >>>> byte order is correct. >>>> >>> >>> Originally, I thought just concatenating the values in native endianness >>> as a kind of replacement for the user_action_cookie (hence the symmetric >>> values). But I'm having second thoughts. I plan to change it in the next >>> version and store it in "network" order. I will change the test to >>> verify it accordingly. >> >> I'm not sure I get that, but just in case: OpenFlow messages themselves >> should be encoded with a network byte order. When printed to the user, >> they should be printed in a host byte order. Netlink messages typically >> have values in the host byte order, but there are some weird netlink >> messages that expect network byte order. In any case, the psample test >> utility should print out values in the host byte order. >> > > If we keep the cookie in host order, "ovs-appctl dump/flows" output will > be different depending on the host the test is run, that's why I chose a > symmetric value. If the host order is what is typically used in netlink, > my original thinking prevails. In that case I'll use a symmetric value > to check the datapath flow and a non-symmetric one to test the output of > "ovstest psample".
Let's put it this way to avoid confusion: If I see obs_domain_id=0x01020304 and obs_point_id=0x05060708 in the OpenFlow rule, then I want to see cookie=0x0102030405060708 in the datapath flow. And then I want the test-psample to print out obs_domain_id=0x01020304 and obs_point_id=0x05060708 back. i.e. obs_domain_id=16909060, obs_point_id=84281096 => cookie=0x0102030405060708. So, yes, since cookie is a few numbers squashed together, i.e. basically a byte array and not a number on its own, it makes sense that bytes are put into the cookie in the network order. This byte order of the cookie components is worth adding to the documentation. Best regards, Ilya Maximets. _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
