[ovs-dev] [PATCH] test suite : add sFlow test

[Neil Mckee]

On reflection,  it's better if the sflow-test.c file just has the same Apache license as the other files in the tests directory.  So I think the header should just be:

/*                                                                                                                               
 * Copyright (c) 2011, 2012 Nicira, Inc.
 *  Copyright (c) 2013 InMon Corp. 
 *
 * Licensed under the Apache License, Version 2.0 (the "License"); 
 * you may not use this file except in compliance with the License.
 * You may obtain a copy of the License at:
 *
 *     http://www.apache.org/licenses/LICENSE-2.0
 *
 * Unless required by applicable law or agreed to in writing, software
 * distributed under the License is distributed on an "AS IS" BASIS,
 * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
 * See the License for the specific language governing permissions and
 * limitations under the License.
 */

You don't need me sign off again,  right?  You can submit the revised patch yourself?

Neil



On Mar 31, 2013, at 9:48 PM, Neil McKee wrote:

> That answers all my questions.  I was indeed in the 1.10 branch.
> 
> (And please reformat the C code.  It shares snippets with the sFlow decoder in Ganglia,  but not enough to matter.)
> 
> Neil
> 
> On Mar 31, 2013, at 8:31 PM, Ben Pfaff <blp at nicira.com> wrote:
> 
>> I'll do an update to the patch tomorrow but here's an early response.
>> 
>> On Fri, Mar 29, 2013 at 08:39:42PM -0700, Neil Mckee wrote:
>>> 
>>> On Mar 29, 2013, at 4:16 PM, Ben Pfaff wrote:
>>> 
>>>> On Wed, Mar 27, 2013 at 11:02:21PM -0700, Neil Mckee wrote:
>>>>> This patch adds an sFlow test to the test suite (in branch 1.10).  To make that work properly I added netdev_dummy_get_ifindex() so that a dummy netdev can return a dummy ifindex when asked.   Is there anywhere in OVS that assumes that a netdev_dummy cannot make up a dummy ifindex?  If so, I guess this behavior could be off by default and turned on just for this test.
>>>>> 
>>>>> I have only tested this on a Fedora 17 OS.
>>>>> 
>>>>> Signed-off-by: Neil McKee <neil.mckee at inmon.com>
>>>>> 
>>>>> ---
>>>> 
>>>> Thanks for the test!
>>>> 
>>>> I noticed that the new test-sflow.c has the Apache 2 boilerplate
>>>> license text in it, which is fine, but the copyright notice should
>>>> probably mention InMon in place of or in addition to Nicira, since a
>>>> lot of the code in test-sflow.c is not written by Nicira.  Neil, can
>>>> you give me a correct copyright notice for that file?
>>> 
>>> How about we add the same copyright notice that appears at the top of
>>> all the lib/sflow* source files (just updating the year to 2013)?
>> 
>> OK, that will do, thanks.
>> 
>>>> Running this, I had some trouble with ifindexes.  As written, the
>>>> ifindex that each dummy device receives depends on the order in which
>>>> they are created.  That, in turn, depends on hash order in the
>>>> database and other factors.  I decided to fix it by making the ifindex
>>>> a configurable property of the dummy devices, applying the following
>>>> incremental patch.  Does that make sense?
>>> 
>>> Yes.  Much better.  I had started to do the same thing but baulked at
>>> the number of lines I was adding.
>> 
>> Thanks.
>> 
>> I think it's OK for the tests to be big.
>> 
>>>> Even after I applied those changes, the test still didn't pass for me.
>>>> When I looked more closely, some of the expected output didn't
>>>> entirely make sense.  For example, my reading of the sflow spec says
>>>> that samplePool should more or less count upward, which is what I
>>>> actually saw in the test output, but the expected output provided in
>>>> the patch shows all the samplePool values as 0.  Some of the other
>>>> expected output needed to be adjusted too.
>>> 
>>> I wonder why your samplePool numbers are incrementing and mine are not?
>>> The samplePool is supplied in ofproto-dpif-sflow.c:dpif-sflow-received():
>>> 
>>>  fs.sample_pool = stats.rx_packets;
>>> 
>>> So I was assuming that since netdev-dummy doesn't seem to increment
>>> any of if's interface-counter stats then that would account for
>>> rx_packets being always 0.  Does this vary depending on the presence
>>> or absence of the kernel module, or something like that?
>> 
>> No, the tests don't ever use the kernel module (they will run whether
>> or not you compile it), so that does not affect them.
>> 
>> I think the difference is probably the OVS version.  The current OVS
>> "master" version of netdev-dummy properly maintains counters, the
>> versions on branch-1.10 and earlier do not.  I guess that you must have
>> developed against branch-1.10 or an earlier version.
>> 
>>> I was thinking that we should probably also change "grep HEADER" to
>>> something like "egrep 'HEADER|ERROR' to make certain than any
>>> exception flagged in the sflow-test.c code is certain to get through
>>> and break the test.  Same thing for "grep IFCOUNTERS".  I hesitated
>>> because I don't know how portable that egrep construct is.
>> 
>> It should be portable enough as long as we use $EGREP instead of plain
>> egrep, since the Autoconf manual says:
>> 
>> `egrep'
>>    Posix 1003.1-2001 no longer requires `egrep', but many hosts do
>>    not yet support the Posix replacement `grep -E'.  Also, some
>>    traditional implementations do not work on long input lines.  To
>>    work around these problems, invoke `AC_PROG_EGREP' and then use
>>    `$EGREP'.
>> 
>>> Just two minor questions left:
>>> 
>>> 1) I wanted to craft a test packet with an odd-numbered length between
>>> 64 and 128 bytes, and another with > 128 bytes.  Where should I look
>>> for documentation on that packet-construction language?  Can I specify
>>> something like ipv6-over-vxlan-over-ipv4-over-mpls?  I'm not concerned
>>> about this for now, just thinking about future enhancements to the
>>> test.
>> 
>> The language isn't really documented, but it doesn't support anything
>> that complicated anyway.
>> 
>> However, you have another alternative: you can fully specify an Ethernet
>> frame as a series of hex digits with optional spaces.  If you do it that
>> way, then you can makes the packets as sophisticated as you like.
>> 
>>> 2) Are you really going to let me away with 2-character indentation in C?
>> 
>> I stopped reading that file for style when I realized that it wasn't
>> really OVS code, it was code that must be cut-and-pasted in from InMon
>> sflow code, probably sflowtool.  At that point, I didn't want to ask for
>> divergence from whatever upstream source it comes from, because that
>> will just make life harder for everyone later if we ever need to update
>> it.
>> 
>> (I know that not complaining about style is out of character for me.)
>> 
>> Thanks,
>> 
>> Ben.