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

Mark Michelson mmichels at redhat.com
Mon Oct 12 15:14:50 UTC 2020


On 10/9/20 1:23 PM, 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!
> 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.
> 
> - 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?

Thanks for the detailed feedback, Ilya.

Quite a few things you mentioned ran through my mind as well. For 
instance, when I was doing the IPAM refactor, I considered that it would 
fit better in its own file instead of being in ovn-northd.c. If I put 
IPAM code into lib/ then I could add some code to test-ovn.c to test the 
public portions of it.

I came to the conclusion that a likely endgame here is to move ipam code 
to northd/ipam.[hc]. I could then put a northd/test-ipam.c file in place 
and have it be the test entry point for IPAM testing. I could have this 
northd/test-ipam.c file use ovstest to run the IPAM testing. We would 
need to add something to only compile this test program when testing.

So then the questions remain:
1) Can the same treatment be applied to other code? In other words, can 
all code be separated into its own files? With IPAM it was pretty easy 
to identify how it could be separated from the rest of ovn-northd's 
logic. Will other parts separate as easily?
2) Even if code is separated, is there a possibility that there will be 
static functions in the separated code that we should test individually? 
If so, then having an external test file won't allow for those functions 
to be tested.

If it turns out that we want to test non-separable code or static 
functions, then having the unit test framework introduced here is a good 
way to perform those tests.

Back on IPAM specifically, an alternative to having northd/test-ipam.c 
would be to put unit tests directly into northd/ipam.[hc]. This way, it 
could test static functions if desired, and it wouldn't require 
conditional compilation of a separate test program.

I'm not familiar with the raft private code you referenced in ovsdb, so 
I can have a look at it to see how that might change the testing approach.

The last major portion of what you suggested is the idea of using 
command line arguments. I left the door open to this by having unit 
tests take a void * parameter. Currently, this is unused in all the 
tests I've introduced. However, it would be possible to have individual 
tests parse command line arguments so that they can have test cases 
added without requiring recompilation.

> 
> Best regards, Ilya Maximets.
> 



More information about the dev mailing list