[ovs-dev] [PATCH] Adjust sFlow packet samplers to better reflect the underlying per-bridge sampling

[Jesse Gross]

OK, that sounds reasonable to me. Can you update the comments to
reflect that sFlow should be OK now - assuming that you think it is. I
see two: ofproto/ofproto-dpif.c:port_construct() and the one in
ofproto/tunnel.c about metadata for packet outs, which was intended to
be for sFlow.

On Thu, Apr 25, 2013 at 8:18 PM, Neil McKee <neil.mckee at inmon.com> wrote:
> Thank you for spending time thinking about this.
>
> There is no loss of information in changing from a per-interface sampling model to a per-bridge sampling model.
>
> If there is a difference it's only on the configuration side, and since the OVS sFlow config is already per-bridge, there is no conflict there.
>
> Hypothetically, if the datapath offered separate per-physical-port sampling probability and we always knew what physical port a packet came in/went out on, then we would have the choice of staying with the per-interface model and changing the config schema to expose it.  However that seems like a stretch -- and not worth the effort at all if you remember that the normal practice for sFlow monitoring is to configure the same sampling rate on every port of every switch in the LAN.
>
> Neil
>
>
>
> On Apr 25, 2013, at 5:06 PM, Jesse Gross <jesse at nicira.com> wrote:
>
>> OK, thanks for the explanation. I guess the part that I'm not entirely
>> sure about is what is the tradeoff in modeling this as being
>> per-bridge rather than per-port? It seems like like in the case of
>> per-bridge sampling you are still including the ifIndex with the
>> sample metadata so you don't lose any information. Is there any
>> disadvantage?
>>
>> On Wed, Apr 24, 2013 at 2:04 PM, Neil Mckee <neil.mckee at inmon.com> wrote:
>>> Actually it wasn't just tunnel ports.  Anything that didn't have an ifIndex was mapping to the same sFlow datasource.  Even if it was on another bridge.
>>>
>>> Besides, sFlow wouldn't want to model every tunnel as a separate datasource either -- even if they did have ifIndex numbers.  There might be thousands.  The idea is to have a small number of fairly-persistent datasources and report on tunnels by annotating individual packet samples (see below).
>>>
>>> I think the original thinking (years ago) was that we might ignore all samples that did not ingress on an ifindex-interface,  but that's not what is happening,  and it's not what we want to happen now anyway.   Now it's quite common to see ingress-sampled packets that we really want to report on where that ingress port doesn't have an ifIndex (GRE tunnels being just the most obvious example).    So if we drop the per-interface-sampler representation and adopt one that more accurately represents what is going on below then it falls out correctly,  and still conforms to the sFlow v5 standard.
>>>
>>> So looking at the code,  the hash-table that the ofproto-sflow module maintains to look up odp_port -> struct of_port becomes just a cache of the ifindex ports.  It is still needed so that the counter-push data-sources can retrieve the latest stats from the netdev,  and so that the packet samples can have their ingress ifindex filled in where applicable.   However if an ingress sample comes in for some other port and there is no lookup in the table then it's no longer a show-stopper.  It just means that the packet sample goes out with inputPort = 0 (meaning "ifindex unknown ").
>>>
>>> Ideally,  we would want to know the original ifIndex of the physical port where that tunnel first entered the hypervisor,   but that seems hard to know.
>>>
>>> Looking ahead, it would  be good to know the details of the tunnel too,  so we could annotate the packet-sample with another structure:
>>> http://sflow.org/sflow_tunnels.txt
>>>
>>> but that might be part of a broader discussion where we look at ways to expose more state to the ofproto-sflow module.  For example,  I am currently looking at how the bundle information might be exposed so that we can also fill in these structures:
>>> http://www.sflow.org/sflow_lag.txt
>>>
>>> And sFlow has standard structures for reporting on MPLS too:
>>> http://www.sflow.org/SFLOW-STRUCTS5.txt
>>>
>>> It seems like there is choice between (1) pushing the information down to ofproto-sflow on a state-change (as we do with that odp_port -> of_port lookup) or (2) providing callback hooks that allow ofproto-dpif-sflow to ask ofproto-dpif questions about tunnels, bundles and more.   Right now it looks to me like the latter would be better,  because it allows ofproto-dpif to completely control what is exposed and keep a tight grip on all it's internal pointers.  But maybe there are other architectural considerations?
>>>
>>> Neil
>>>
>>>
>>> On Apr 24, 2013, at 11:09 AM, Jesse Gross wrote:
>>>
>>>> On Wed, Apr 24, 2013 at 10:46 AM, Neil Mckee <neil.mckee at inmon.com> wrote:
>>>>> Adjust sFlow packet samplers to better reflect the underlying per-bridge
>>>>> sampling that is really happening,  rather than modeling it as per-interface
>>>>> sampling. This fixes an aliasing issue: tunnel traffic sampled on different
>>>>> bridges was all being reported against the same sFlow datasource.
>>>>>
>>>>> The sFlow counter samples remain unchanged,  reporting only on interfaces
>>>>> that have valid ifIndex numbers.
>>>>>
>>>>> See definition of SFlowDataSource in http://sflow.org/sflow_version_5.txt.
>>>>>
>>>>> Signed-off-by: Neil McKee <neil.mckee at inmon.com>
>>>>
>>>> It actually is possible to disambiguate between tunnel ports in order
>>>> to continue reporting on a per-port basis, have you considered doing
>>>> that?
>>>