[ovs-dev] [RFC PATCH ovn 0/3] Unit Testing in OVN

Mark Gray mark.d.gray at redhat.com
Mon Oct 12 16:49:08 UTC 2020


On 12/10/2020 16:13, Ilya Maximets wrote:
> On 10/12/20 4:57 PM, Mark Gray wrote:
>> On 09/10/2020 18:23, Ilya Maximets wrote:
>>> On 10/9/20 5:29 PM, Mark Michelson wrote:
>>>> OVN has had a test framework for as long as I've been working on the
>>>> project. The test framework is designed for performing functional tests
>>>> of OVN. That is, with the entirety of OVN up and running, we can provide
>>>> configuration and test data and ensure that OVN does with that data what
>>>> we expect. This is 100% a good thing and has helped us to detect lots of
>>>> bugs before they can actually be merged in.
>>>>
>>>> What's missing, though, are smaller-scale unit tests. As an example, if
>>>> I wanted to test ovn-northd's IPAM code, I would need to start up
>>>> ovn-northd, create a logical switch, configure that logical switch to
>>>> use IPAM, and then create logical switch ports to exercise the IPAM
>>>> code. This can be overkill if my only goal is to ensure that IPAM's
>>>> algorithm for selecting the next IP address is correct.
>>>>
>>>> This patch series proposes a unit test framework for OVN.
>>>>
>>>> If you want to run the unit tests, you can do so in a couple of ways.
>>>>
>>>> 1) Within the testsuite.
>>>>    ./configure --with-ovs-source=/path/to/ovs --enable-unit-tests
>>>>    make check TESTSUITEFLAGS="-k unit"
>>>>
>>>> 2) One-off from the command line
>>>>    ./configure --with-ovs-source=/path/to/ovs --enable-unit-tests
>>>>    make sandbox
>>>>    ovn-appctl -t ovn-northd unit-test <test_name>
>>>>
>>>> Some notes on this patch series
>>>> 1) Patch 1 is the most important one in the series. This is an RFC
>>>> because I'm trying to find out if the unit test framework itself is
>>>> good. The refactoring in patch 2 and the unit tests added in patch 3 are
>>>> meant to illustrate examples of the framework. They do not necessarily
>>>> need to be merged as-is. Feel free to comment on them if you'd like,
>>>> though.
>>>> 2) Ideally, new unit tests could be added to the testsuite via a script.
>>>> They've been added manually in this patch series.
>>>> 3) This patch series only adds unit test capabilities to ovn-northd.
>>>> However, the patch series we actually merge should add unit test
>>>> capabilities to ovn-controller as well.
>>>
>>> That is an interesting approach, thanks for working n this!
>>
>> +1
>>
>>> This is very similar to ovstest framework (tests/ovstest.{c,h}), however
>>> there are few differences:
>>>
>>> - ovstest is a separate executable and tests are implemented as a separate
>>>   source files.  So, usage is 'ovstest mytest' instead of
>>>   'ovn-appctl -t ovn-northd mytest'.  Upside of ovstest is that it doesn't
>>>   require test code being integrated to the main executable.  Downside
>>>   is that it requires functions being in a separate library that could
>>>   be included.  Not sure if this is a downside though, as it forces
>>>   developers to avoid huge source files with static functions.
>>>
>>
>> I don't think this is a downside. If the code was broken down into
>> smaller modules, we could do exactly what you are suggesting Ilya. If we
>> enable the ability to test code like this, it may reduce the motivation
>> for modularization of the code :)
>>
>> However, if we did go down this path, the other way that I have seen
>> this done (while minimizing the amount test code leaking into production
>> code) is to include .c files in your test code.
>>
>> #include "northd/ovn-northd.c"
>>
>> That way the test code can access private functions and structure in
>> the code under test.
> 
> That's nasty. :)  Anyway, you can not just include ovn-northd.c because
> it contains main() function.  You need to get rid of it somehow at first.

I did say "minimizing" :D - you would have to do something with main()
(#ifdef, or maybe there is a smart way to change the entry point of the
executable through the compiler). Everything is nasty to enable this in
C if the code is not modular but I think the preference should be to not
modify the production code. Maybe Numan's suggestion is worth looking at?

The other issue we will have is mocking out components so we could
inject defined responses to certain function calls to, for example,
force error conditions.

> 
>>
>>
>>> - current implementation of ovn unit test framework doesn't allow passing
>>>   datasets via cmdline, while each unit test with ovstest could be called
>>>   with different cmdline arguments. This makes it easier to test, as you
>>>   don't need to rebuild binaries under test.
>>>
>>> - since ovn unit test framework implements tests inside main source files
>>>   these files will, ideally, grow significantly.  ovn-northd is already
>>>   13K lines of code, so this, probably, is not a very good thing.
>>>
>>> You also did a good job decoupling init_ipam_info out of northd stuff,
>>> so it basically self-sufficient now.  What my suggestion here is to take
>>> one more step forward and move this function to its own library, e.g.
>>> move all the ipam related code that could be decoupled to lib/ipam.c
>>> and lib/ipam.h.  This might be a good thing to do not only for unit
>>> testing purposes, but just as a generic refactoring step that will
>>> reduce code complexity and increase readability and maintainability.
>>> Also, this will open a road to write separate testing module like
>>> tests/test-ipam.c and integrate it into ovstest-like testing framework.
>>> Same could be done for all other logically separable parts of northd.
>>> Even actual logic of a northd itself could be split in parts, e.g.
>>> northd/ovs-northd-something.{c,h}.  In this case all the northd-specific
>>> datastructures/functions needed in different modules could be moved to
>>> northd/ovs-northd-private.{c,h}.  OVS uses this approach in many places,
>>> e.g.  ovsdb/raft.{c,h}, ovsdb/raft-rpc.{c,h}, ovsdb/raft-private.{c,h}.
>>> In this example rpc moved outside of main raft logic code and all required
>>> common code moved to raft-private module.
>>>
>>> What do you think?
>>>
>>> Best regards, Ilya Maximets.
>>> _______________________________________________
>>> dev mailing list
>>> dev at openvswitch.org
>>> https://mail.openvswitch.org/mailman/listinfo/ovs-dev
>>>
>>
> 



More information about the dev mailing list