diff mbox series

[ovs-dev,RFC,v3,3/3] Add ipam unit tests

Message ID 20201102142828.3444732-4-mmichels@redhat.com
State Superseded
Headers show
Series Unit Testing in OVN | expand

Commit Message

Mark Michelson Nov. 2, 2020, 2:28 p.m. UTC 
This adds unit tests for IPAM IPv6 initialization, IPv4 initialization,
and IPv4 address retrieval. It also adds testsuite tests corresponding
to these unit tests.

The IPv6 initialization and IPv4 initialization tests make use of the
new unit test framework. They use ovn-appctl to get access to internal
functions in northd/ipam.c. They require ENABLE_UNIT_TESTS to be
defined, otherwise the internal unit test code will not be compiled in.

The IPv4 address retrieval test makes use of the pre-existing ovstest
utility.

Signed-off-by: Mark Michelson <mmichels@redhat.com>
---
 northd/ipam.c           |  56 ++++++++++
 northd/ovn-northd.c     |   3 +
 tests/automake.mk       |   8 +-
 tests/ovn-unit-tests.at | 237 ++++++++++++++++++++++++++++++++++++++++
 tests/testsuite.at      |   1 +
 5 files changed, 303 insertions(+), 2 deletions(-)
 create mode 100644 tests/ovn-unit-tests.at

Comments

Mark Gray Nov. 12, 2020, 1:54 p.m. UTC | #1 
On 02/11/2020 14:28, Mark Michelson wrote:
> This adds unit tests for IPAM IPv6 initialization, IPv4 initialization,
> and IPv4 address retrieval. It also adds testsuite tests corresponding
> to these unit tests.
> 
> The IPv6 initialization and IPv4 initialization tests make use of the
> new unit test framework. They use ovn-appctl to get access to internal
> functions in northd/ipam.c. They require ENABLE_UNIT_TESTS to be
> defined, otherwise the internal unit test code will not be compiled in.
> 
> The IPv4 address retrieval test makes use of the pre-existing ovstest
> utility.
> 
> Signed-off-by: Mark Michelson <mmichels@redhat.com>
> ---
>  northd/ipam.c           |  56 ++++++++++
>  northd/ovn-northd.c     |   3 +
>  tests/automake.mk       |   8 +-
>  tests/ovn-unit-tests.at | 237 ++++++++++++++++++++++++++++++++++++++++
>  tests/testsuite.at      |   1 +
>  5 files changed, 303 insertions(+), 2 deletions(-)
>  create mode 100644 tests/ovn-unit-tests.at
> 
> diff --git a/northd/ipam.c b/northd/ipam.c
> index e5383c46f..fb367700b 100644
> --- a/northd/ipam.c
> +++ b/northd/ipam.c
> @@ -326,3 +326,59 @@ ipam_get_unused_mac(ovs_be32 ip)
>  
>      return mac64;
>  }
> +
> +#ifdef ENABLE_UNIT_TESTS
> +


Thanks for looking at this further. IMHO I have a preference to not add
test code in production code. Admittedly, because this is just adding
text at the end of the file, it should not cause any problems at this
stage. However, this makes it too easy to add this #ifdef anywhere in
the file to do things like mocking out objects and this could start
getting messy as the test code paths and production code paths could
start to deviate. There are other patterns (like #include "file.c" and
others suggested by others on this list) that can get around the problem
of testing static functions in the file.

However, I question the need to do this at all. If we had a need to
achieve some kind of code coverage target (in which we have to make sure
we test X% of all code paths) then we probably would need some method
like this in order to allow this. I think the main problem is the
difficultly testing large files (e.g northd.c, ovn-controller.c). If we
try to find a way to test these private static functions then we are
just covering up the problem that these large files need to be broken up
into something more modular. Therefore, I think we should be splitting
out logical functionality (like you did with ipam.c), defining a
"public" interface, testing against that interface which follows the
pattern in OVS. If we can't achieve our goals with this, then maybe we
should look at methods like this.
Numan Siddique Nov. 16, 2020, 10:57 a.m. UTC | #2 
On Thu, Nov 12, 2020 at 7:24 PM Mark Gray <mark.d.gray@redhat.com> wrote:
>
> On 02/11/2020 14:28, Mark Michelson wrote:
> > This adds unit tests for IPAM IPv6 initialization, IPv4 initialization,
> > and IPv4 address retrieval. It also adds testsuite tests corresponding
> > to these unit tests.
> >
> > The IPv6 initialization and IPv4 initialization tests make use of the
> > new unit test framework. They use ovn-appctl to get access to internal
> > functions in northd/ipam.c. They require ENABLE_UNIT_TESTS to be
> > defined, otherwise the internal unit test code will not be compiled in.
> >
> > The IPv4 address retrieval test makes use of the pre-existing ovstest
> > utility.
> >
> > Signed-off-by: Mark Michelson <mmichels@redhat.com>
> > ---
> >  northd/ipam.c           |  56 ++++++++++
> >  northd/ovn-northd.c     |   3 +
> >  tests/automake.mk       |   8 +-
> >  tests/ovn-unit-tests.at | 237 ++++++++++++++++++++++++++++++++++++++++
> >  tests/testsuite.at      |   1 +
> >  5 files changed, 303 insertions(+), 2 deletions(-)
> >  create mode 100644 tests/ovn-unit-tests.at
> >
> > diff --git a/northd/ipam.c b/northd/ipam.c
> > index e5383c46f..fb367700b 100644
> > --- a/northd/ipam.c
> > +++ b/northd/ipam.c
> > @@ -326,3 +326,59 @@ ipam_get_unused_mac(ovs_be32 ip)
> >
> >      return mac64;
> >  }
> > +
> > +#ifdef ENABLE_UNIT_TESTS
> > +
>
>
> Thanks for looking at this further. IMHO I have a preference to not add
> test code in production code. Admittedly, because this is just adding
> text at the end of the file, it should not cause any problems at this
> stage. However, this makes it too easy to add this #ifdef anywhere in
> the file to do things like mocking out objects and this could start
> getting messy as the test code paths and production code paths could
> start to deviate. There are other patterns (like #include "file.c" and
> others suggested by others on this list) that can get around the problem
> of testing static functions in the file.
>
> However, I question the need to do this at all. If we had a need to
> achieve some kind of code coverage target (in which we have to make sure
> we test X% of all code paths) then we probably would need some method
> like this in order to allow this. I think the main problem is the
> difficultly testing large files (e.g northd.c, ovn-controller.c). If we
> try to find a way to test these private static functions then we are
> just covering up the problem that these large files need to be broken up
> into something more modular. Therefore, I think we should be splitting
> out logical functionality (like you did with ipam.c), defining a
> "public" interface, testing against that interface which follows the
> pattern in OVS. If we can't achieve our goals with this, then maybe we
> should look at methods like this.

I would agree with Mark G.

Thanks
Numan

>
> _______________________________________________
> dev mailing list
> dev@openvswitch.org
> https://mail.openvswitch.org/mailman/listinfo/ovs-dev
>
diff mbox series

Patch

diff --git a/northd/ipam.c b/northd/ipam.c
index e5383c46f..fb367700b 100644
--- a/northd/ipam.c
+++ b/northd/ipam.c
@@ -326,3 +326,59 @@  ipam_get_unused_mac(ovs_be32 ip)
 
     return mac64;
 }
+
+#ifdef ENABLE_UNIT_TESTS
+
+static void
+test_init_ipam_ipv6_prefix(struct ovs_cmdl_context *ctx)
+{
+    const char *prefix = ctx->argc > 1 ? ctx->argv[1] : NULL;
+    struct ipam_info ipam;
+    struct ds *output = ctx->pvt;
+
+    init_ipam_ipv6_prefix(prefix, &ipam);
+    ds_put_format(output, "ipv6_prefix_set: %s\n",
+                  ipam.ipv6_prefix_set ? "true" : "false");
+    if (ipam.ipv6_prefix_set) {
+        char ipv6[INET6_ADDRSTRLEN];
+        inet_ntop(AF_INET6, &ipam.ipv6_prefix,
+                  ipv6, sizeof ipv6);
+        ds_put_format(output, "ipv6_prefix: %s\n", ipv6);
+    }
+}
+
+UNIT_TEST_DEFINE(init_ipam_ipv6_prefix, 0, 1, test_init_ipam_ipv6_prefix);
+
+static void
+test_init_ipam_ipv4(struct ovs_cmdl_context *ctx)
+{
+    const char *subnet = ctx->argv[1];
+    const char *exclude_ips = ctx->argc > 2 ? ctx->argv[2] : NULL;
+    struct ipam_info info;
+
+    init_ipam_ipv4(subnet, exclude_ips, &info);
+    struct ds *output = ctx->pvt;
+
+    ds_put_format(output, "start_ipv4: " IP_FMT "\n",
+                  IP_ARGS(htonl(info.start_ipv4)));
+    ds_put_format(output, "total_ipv4s: %" PRIuSIZE "\n", info.total_ipv4s);
+
+    ds_put_cstr(output, "allocated_ipv4s: ");
+    if (info.allocated_ipv4s) {
+        int start = 0;
+        int end = info.total_ipv4s;
+        for (size_t bit = bitmap_scan(info.allocated_ipv4s, true, start, end);
+             bit != end;
+             bit = bitmap_scan(info.allocated_ipv4s, true, bit + 1, end)) {
+            ds_put_format(output, IP_FMT " ",
+                          IP_ARGS((htonl(info.start_ipv4 + bit))));
+        }
+    }
+    ds_chomp(output, ' ');
+    ds_put_char(output, '\n');
+    destroy_ipam_info(&info);
+}
+
+UNIT_TEST_DEFINE(init_ipam_ipv4, 1, 2, test_init_ipam_ipv4);
+
+#endif /* ENABLE_UNIT_TESTS */
diff --git a/northd/ovn-northd.c b/northd/ovn-northd.c
index 0fb7d0969..41c5df49e 100644
--- a/northd/ovn-northd.c
+++ b/northd/ovn-northd.c
@@ -36,6 +36,7 @@ 
 #include "lib/ovn-nb-idl.h"
 #include "lib/ovn-sb-idl.h"
 #include "lib/ovn-util.h"
+#include "lib/unit-test.h"
 #include "ovn/actions.h"
 #include "ovn/logical-fields.h"
 #include "packets.h"
@@ -12742,6 +12743,8 @@  main(int argc, char *argv[])
                              cluster_state_reset_cmd,
                              &reset_ovnnb_idl_min_index);
 
+    register_unixctl_unit_test();
+
     daemonize_complete();
 
     /* We want to detect (almost) all changes to the ovn-nb db. */
diff --git a/tests/automake.mk b/tests/automake.mk
index 26b6d11b4..782af79b9 100644
--- a/tests/automake.mk
+++ b/tests/automake.mk
@@ -30,7 +30,8 @@  TESTSUITE_AT = \
 	tests/ovn-controller-vtep.at \
 	tests/ovn-ic.at \
 	tests/ovn-macros.at \
-	tests/ovn-performance.at
+	tests/ovn-performance.at \
+	tests/ovn-unit-tests.at
 
 SYSTEM_KMOD_TESTSUITE_AT = \
 	tests/system-common-macros.at \
@@ -200,7 +201,10 @@  noinst_PROGRAMS += tests/ovstest
 tests_ovstest_SOURCES = \
 	tests/ovstest.c \
 	tests/ovstest.h \
-	tests/test-ovn.c
+	tests/test-ovn.c \
+	northd/test-ipam.c \
+	northd/ipam.c \
+	northd/ipmam.h
 
 tests_ovstest_LDADD = $(OVS_LIBDIR)/daemon.lo \
     $(OVS_LIBDIR)/libopenvswitch.la lib/libovn.la
diff --git a/tests/ovn-unit-tests.at b/tests/ovn-unit-tests.at
new file mode 100644
index 000000000..50fc53745
--- /dev/null
+++ b/tests/ovn-unit-tests.at
@@ -0,0 +1,237 @@ 
+AT_BANNER([OVN unit tests])
+
+AT_SETUP([ovn -- unit test -- init_ipam_ipv4])
+AT_SKIP_IF([test "$ENABLE_UNIT_TESTS" = no])
+ovn_start
+
+# Valid subnet, no exclude IPs
+AT_CHECK([ovn-appctl -t northd/ovn-northd unit-test init_ipam_ipv4 192.168.0.0/29], [0], [dnl
+start_ipv4: 192.168.0.1
+total_ipv4s: 7
+allocated_ipv4s: 192.168.0.1
+])
+
+# Valid subnet, single exclude IP
+AT_CHECK([ovn-appctl -t northd/ovn-northd unit-test init_ipam_ipv4 192.168.0.0/29 192.168.0.3], [0], [dnl
+start_ipv4: 192.168.0.1
+total_ipv4s: 7
+allocated_ipv4s: 192.168.0.1 192.168.0.3
+])
+
+# Valid subnet, two exclude IPs
+AT_CHECK([ovn-appctl -t northd/ovn-northd unit-test init_ipam_ipv4 192.168.0.0/29 "192.168.0.3 192.168.0.5"], [0], [dnl
+start_ipv4: 192.168.0.1
+total_ipv4s: 7
+allocated_ipv4s: 192.168.0.1 192.168.0.3 192.168.0.5
+])
+
+# Valid subnet, range of exclude IPs
+AT_CHECK([ovn-appctl -t northd/ovn-northd unit-test init_ipam_ipv4 192.168.0.0/29 "192.168.0.3..192.168.0.5"], [0], [dnl
+start_ipv4: 192.168.0.1
+total_ipv4s: 7
+allocated_ipv4s: 192.168.0.1 192.168.0.3 192.168.0.4 192.168.0.5
+])
+
+# Valid subnet, exclude IP outside of subnet
+# Excluded IP should be ignored.
+AT_CHECK([ovn-appctl -t northd/ovn-northd unit-test init_ipam_ipv4 192.168.0.0/29 192.168.0.9], [0], [dnl
+start_ipv4: 192.168.0.1
+total_ipv4s: 7
+allocated_ipv4s: 192.168.0.1
+])
+
+# Valid subnet, range of exclude IPs starts in subnet but ends outside
+# Excluded IPs inside the subnet should be allocated
+AT_CHECK([ovn-appctl -t northd/ovn-northd unit-test init_ipam_ipv4 192.168.0.0/29 "192.168.0.5..192.168.0.11"], [0], [dnl
+start_ipv4: 192.168.0.1
+total_ipv4s: 7
+allocated_ipv4s: 192.168.0.1 192.168.0.5 192.168.0.6 192.168.0.7
+])
+
+# Valid subnet, range of exclude IPs starts outside subnet but ends inside
+# Excluded IPs inside the subnet should be allocated
+AT_CHECK([ovn-appctl -t northd/ovn-northd unit-test init_ipam_ipv4 192.168.0.8/29 "192.168.0.5..192.168.0.11"], [0], [dnl
+start_ipv4: 192.168.0.9
+total_ipv4s: 7
+allocated_ipv4s: 192.168.0.9 192.168.0.10 192.168.0.11
+])
+
+# Valid subnet, range of exclude IPs starts before and ends after the subnet
+# Entire subnet should be allocated
+# XXX Should excluding every address in a subnet be an invalid configuration?
+AT_CHECK([ovn-appctl -t northd/ovn-northd unit-test init_ipam_ipv4 192.168.0.8/29 "192.168.0.5..192.168.0.18"], [0], [dnl
+start_ipv4: 192.168.0.9
+total_ipv4s: 7
+allocated_ipv4s: 192.168.0.9 192.168.0.10 192.168.0.11 192.168.0.12 192.168.0.13 192.168.0.14 192.168.0.15
+])
+
+# Valid subnet, inverted exclude range
+# Exclude range should be ignored
+AT_CHECK([ovn-appctl -t northd/ovn-northd unit-test init_ipam_ipv4 192.168.0.0/29 "192.168.0.5..192.168.0.2"], [0], [dnl
+start_ipv4: 192.168.0.1
+total_ipv4s: 7
+allocated_ipv4s: 192.168.0.1
+])
+
+# XXX At this point, I wanted to insert some tests where I put in invalid
+# IP addresses like 400.500.600.700 to ensure that the start_ipv4 was set
+# to "0.0.0.0". However, ovs_scan_ip_masked() does no validation of the
+# IP address. So long as the given IP address follows the format of
+# xxx.xxx.xxx.xxx, it's seen as valid. In the specific case of
+# "400.500.600.700", it ends up setting the start_ipv4 to
+# "144.244.88.185". This result is probably system-dependent.
+
+# Invalid subnet: Bad mask
+AT_CHECK([ovn-appctl -t northd/ovn-northd unit-test init_ipam_ipv4 192.168.0.0/-69], [0], [dnl
+start_ipv4: 0.0.0.0
+total_ipv4s: 0
+allocated_ipv4s:
+])
+
+AT_CLEANUP
+
+AT_SETUP([ovn -- unit test -- init_ipam_ipv6_prefix])
+AT_SKIP_IF([test "$ENABLE_UNIT_TESTS" = no])
+ovn_start
+
+# No prefix set
+AT_CHECK([ovn-appctl -t northd/ovn-northd unit-test init_ipam_ipv6_prefix], [0], [dnl
+ipv6_prefix_set: false
+])
+
+# Good prefix with no mask
+AT_CHECK([ovn-appctl -t northd/ovn-northd unit-test init_ipam_ipv6_prefix aef0::], [0], [dnl
+ipv6_prefix_set: true
+ipv6_prefix: aef0::
+])
+
+# Good prefix with good mask
+AT_CHECK([ovn-appctl -t northd/ovn-northd unit-test init_ipam_ipv6_prefix aef0::/64], [0], [dnl
+ipv6_prefix_set: true
+ipv6_prefix: aef0::
+])
+
+# Bad prefix with no mask
+AT_CHECK([ovn-appctl -t northd/ovn-northd unit-test init_ipam_ipv6_prefix aef20::], [0], [dnl
+ipv6_prefix_set: false
+])
+
+# Good prefix with nonsense mask.
+AT_CHECK([ovn-appctl -t northd/ovn-northd unit-test init_ipam_ipv6_prefix aef0::/900], [0], [dnl
+ipv6_prefix_set: false
+])
+
+# Good prefix with a non-/64 mask.
+AT_CHECK([ovn-appctl -t northd/ovn-northd unit-test init_ipam_ipv6_prefix aef0::/32], [0], [dnl
+ipv6_prefix_set: false
+])
+
+# Bad prefix and a non-/64 mask.
+AT_CHECK([ovn-appctl -t northd/ovn-northd unit-test init_ipam_ipv6_prefix aef20::/32], [0], [dnl
+ipv6_prefix_set: false
+])
+
+# Overspecify the IPv6 address.
+# We should "round down" to the /64 network address.
+AT_CHECK([ovn-appctl -t northd/ovn-northd unit-test init_ipam_ipv6_prefix aef0::2323], [0], [dnl
+ipv6_prefix_set: true
+ipv6_prefix: aef0::
+])
+
+# Overspecify the IPv6 address, and specify a mask.
+# We should "round down" to the /64 network address.
+AT_CHECK([ovn-appctl -t northd/ovn-northd unit-test init_ipam_ipv6_prefix aef0::2323/64], [0], [dnl
+ipv6_prefix_set: true
+ipv6_prefix: aef0::
+])
+
+AT_CLEANUP
+
+AT_SETUP([ovn -- unit test -- ipam_get_unused_ip])
+AT_SKIP_IF([test "$ENABLE_UNIT_TESTS" = no])
+ovn_start
+
+# Ensure first address returned by IPAM is .2, since .1 is reserved for the
+# connected router
+AT_CHECK([ovstest test-ipam ipam_get_unused_ip 192.168.0.0/29 1], [0], [dnl
+192.168.0.2
+])
+
+# Ensure that we only grab IPs within the requested subnet
+# Ignore stderr so that the warning about address space being
+# exhausted does not cause the test to fail
+AT_CHECK([ovstest test-ipam ipam_get_unused_ip 192.168.0.0/29 6], [0], [dnl
+192.168.0.2
+192.168.0.3
+192.168.0.4
+192.168.0.5
+192.168.0.6
+0.0.0.0
+],[ignore])
+
+# Set up an exclude IP and ensure it does not get selected
+AT_CHECK([ovstest test-ipam ipam_get_unused_ip 192.168.0.0/29 4 192.168.0.3], [0], [dnl
+192.168.0.2
+192.168.0.4
+192.168.0.5
+192.168.0.6
+])
+
+# Set up an exclude IP range and ensure none gets selected
+AT_CHECK([ovstest test-ipam ipam_get_unused_ip 192.168.0.0/29 2 192.168.0.3..192.168.0.5], [0], [dnl
+192.168.0.2
+192.168.0.6
+])
+
+# Set up an exclude range from outside the subnet. Ensure it is ignored.
+AT_CHECK([ovstest test-ipam ipam_get_unused_ip 192.168.0.0/29 5 192.168.1.3..192.168.1.5], [0], [dnl
+192.168.0.2
+192.168.0.3
+192.168.0.4
+192.168.0.5
+192.168.0.6
+],[ignore])
+
+# Set up an exclude range from outside the subnet. Ensure we cannot assign
+# addresses outside the subnet
+AT_CHECK([ovstest test-ipam ipam_get_unused_ip 192.168.0.0/29 6 192.168.1.3..192.168.1.5], [0], [dnl
+192.168.0.2
+192.168.0.3
+192.168.0.4
+192.168.0.5
+192.168.0.6
+0.0.0.0
+],[ignore])
+
+# Set up an exclude range that starts before the subnet but ends in the subnet.
+# The overlapping part should be excluded
+AT_CHECK([ovstest test-ipam ipam_get_unused_ip 192.168.0.8/29 2 192.168.0.2..192.168.0.12], [0], [dnl
+192.168.0.13
+192.168.0.14
+],[ignore])
+
+# Set up an exclude range that starts in the subnet but ends after the subnet.
+# The overlapping part should be excluded.
+AT_CHECK([ovstest test-ipam ipam_get_unused_ip 192.168.0.0/29 3 192.168.0.4..192.168.0.9], [0], [dnl
+192.168.0.2
+192.168.0.3
+0.0.0.0
+],[ignore])
+
+# Set up an exclude range that starts before the subnet and ends after the subnet.
+# The entire range should be excluded.
+AT_CHECK([ovstest test-ipam ipam_get_unused_ip 192.168.0.8/29 1 192.168.0.2..192.168.0.18], [0], [dnl
+0.0.0.0
+],[ignore])
+
+# Configure the subnet using a starting IP that is not the network address of the
+# subnet. Ensure that we "round it down" to the proper subnet starting point.
+AT_CHECK([ovstest test-ipam ipam_get_unused_ip 192.168.0.4/29 5], [0], [dnl
+192.168.0.2
+192.168.0.3
+192.168.0.4
+192.168.0.5
+192.168.0.6
+])
+
+AT_CLEANUP
diff --git a/tests/testsuite.at b/tests/testsuite.at
index 1985923d5..5e60bad73 100644
--- a/tests/testsuite.at
+++ b/tests/testsuite.at
@@ -21,6 +21,7 @@  m4_include([tests/ovsdb-macros.at])
 m4_include([tests/ofproto-macros.at])
 m4_include([tests/ovn-macros.at])
 
+m4_include([tests/ovn-unit-tests.at])
 m4_include([tests/ovn.at])
 m4_include([tests/ovn-performance.at])
 m4_include([tests/ovn-northd.at])