On Mon, Jun 24, 2024 at 07:57:28PM GMT, Ilya Maximets wrote: > 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 <amore...@redhat.com> > >>>>> --- > >>>>> 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. >
Exactly. That's what I was trying to say, sorry if I was not clear. The current patch uses host order, so in your example, the cookie that will be seen is: 0x0403020108070605. My original rationale was "it's just like passing a struct, i.e: a replacement for "struct user_action_cookie". This would make more sense if we were documenting the struct layout. However, we are "concatenating numbers", so I agree with you: network order makes more sense here. > This byte order of the cookie components is worth adding to the documentation. > +1 > Best regards, Ilya Maximets. > _______________________________________________ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
