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.
_______________________________________________
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
