On 5/13/24 13:10, Adrian Moreno wrote:
>
>
> On 5/13/24 12:44 PM, Eelco Chaudron wrote:
>>
>>
>> On 13 May 2024, at 9:01, Adrian Moreno wrote:
>>
>>> On 5/10/24 12:06 PM, Eelco Chaudron wrote:
>>>> On 24 Apr 2024, at 21:53, Adrian Moreno wrote:
>>>>
>>>>> This simple program reads from psample and prints the packets to stdout.
>>>>> It's useful for quickly collecting sampled packets.
>>>>
>>>> See some comments below, did not review the actual sample application in
>>>> detail.
>>>>
>>>> //Eelco
>>>>
>>>>> Signed-off-by: Adrian Moreno <amore...@redhat.com>
>>>>> ---
>>>>> Documentation/automake.mk | 1 +
>>>>> Documentation/conf.py | 2 +
>>>>> Documentation/ref/index.rst | 1 +
>>>>> Documentation/ref/ovs-psample.8.rst | 47 ++++
>>>>> include/linux/automake.mk | 1 +
>>>>> include/linux/psample.h | 64 ++++++
>>>>> rhel/openvswitch-fedora.spec.in | 2 +
>>>>> rhel/openvswitch.spec.in | 1 +
>>>>> utilities/automake.mk | 9 +
>>>>> utilities/ovs-psample.c | 330 ++++++++++++++++++++++++++++
>>>>> 10 files changed, 458 insertions(+)
>>>>> create mode 100644 Documentation/ref/ovs-psample.8.rst
>>>>> create mode 100644 include/linux/psample.h
>>>>> create mode 100644 utilities/ovs-psample.c
>>>>>
>>>>> diff --git a/Documentation/automake.mk b/Documentation/automake.mk
>>>>> index 47d2e336a..c22facfd6 100644
>>>>> --- a/Documentation/automake.mk
>>>>> +++ b/Documentation/automake.mk
>>>>> @@ -165,6 +165,7 @@ RST_MANPAGES = \
>>>>> ovs-l3ping.8.rst \
>>>>> ovs-parse-backtrace.8.rst \
>>>>> ovs-pki.8.rst \
>>>>> + ovs-psample.8.rst \
>>>>> ovs-tcpdump.8.rst \
>>>>> ovs-tcpundump.1.rst \
>>>>> ovs-test.8.rst \
>>>>> diff --git a/Documentation/conf.py b/Documentation/conf.py
>>>>> index 15785605a..75efed2fc 100644
>>>>> --- a/Documentation/conf.py
>>>>> +++ b/Documentation/conf.py
>>>>> @@ -134,6 +134,8 @@ _man_pages = [
>>>>> u'convert "tcpdump -xx" output to hex strings'),
>>>>> ('ovs-test.8',
>>>>> u'Check Linux drivers for performance, vlan and L3 tunneling
>>>>> problems'),
>>>>> + ('ovs-psample.8',
>>>>> + u'Print packets sampled by psample'),
>>>>> ('ovs-vlan-test.8',
>>>>> u'Check Linux drivers for problems with vlan traffic'),
>>>>> ('ovsdb-server.7',
>>>>> diff --git a/Documentation/ref/index.rst b/Documentation/ref/index.rst
>>>>> index 03ada932f..d1076435a 100644
>>>>> --- a/Documentation/ref/index.rst
>>>>> +++ b/Documentation/ref/index.rst
>>>>> @@ -46,6 +46,7 @@ time:
>>>>> ovs-pki.8
>>>>> ovs-sim.1
>>>>> ovs-parse-backtrace.8
>>>>> + ovs-psample.8
>>>>> ovs-tcpdump.8
>>>>> ovs-tcpundump.1
>>>>> ovs-test.8
>>>>> diff --git a/Documentation/ref/ovs-psample.8.rst
>>>>> b/Documentation/ref/ovs-psample.8.rst
>>>>> new file mode 100644
>>>>> index 000000000..c849c83d8
>>>>> --- /dev/null
>>>>> +++ b/Documentation/ref/ovs-psample.8.rst
>>>>> @@ -0,0 +1,47 @@
>>>>> +==========
>>>>> +ovs-sample
>>>>
>>>> Interesting, here you call it all ovs-sample here, which is like ;)
>>>
>>> Yes, at the begining I called it ovs-sample as I thought to make some
>>> generic tool, but since it ended up being very psample-specific I added the
>>> "p" (and missed this one).
>>>
>>>>
>>>> You could add options like —local-kernel --local-userspace (--ipfix,
>>>> --sflow) and it will eventually work on each datapath.
>>
>>>
>>> You mean implementing an IPFIX and sFlow collector?
>>>
>> ?
>>>> If you want to keep this a separate psample utility, I would not have ovs
>>>> in the name, as it's rather general and not OVS specific. Maybe just
>>>> psample/psample_mon, as we also have nlmon.
>>>>
>>>
>>> Well, the way the cookie is decoded into observation_domain_id and
>>> observation_point_id is ovs-specific.
>>>
>>> In fact, for the next iteration of the series I will remove the filtering
>>> API since its getting removed from the kernel series as well and add some
>>> kind of BPF code that does the filtering. Also I was considering looking
>>> into the OVSDB to ensure that we filter on groups configured in it or else
>>> we could wrongly interpet a sampled packet that comes from some other
>>> subsystem.
>>
>> I would prefer to have this as an ova-sample application, which can be
>> extended with other sample methods as we see fit. So when we added userspace
>> support we can add it here. If we find someone crazy enough to do a simple
>> IPFIX and/or sFlow collector it can be added too.
>>
>> So my request would be to remove the (p) from ovs-psample, and have a switch
>> to select --psample (the only supported (MANDATORY) option for now).
>>
>
> Oh, ok. Now I undestand, thank you. We want to leave room for the userspace
> implementation. Will do.
FYI, we do have sflow and netflow basic collectors in tests/test-sflow.c and
tests/test-netflow.c.
And I'm not convinced we should maintain an actual collector for any of these.
It's not OVS'es job as a project. Like we're not shipping nlmon, we probably
shouldn't ship psample or any other collector. But we can and should have basic
implementations for testing purposes in tests/test-psample.c for example.
Best regards, Ilya Maximets.
_______________________________________________
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
