[pve-devel] [PATCH pve-network 0/8] SDN Vnet blackbox testing

[pve-devel] [PATCH pve-network 0/8] SDN Vnet blackbox testing

[Stefan Lendl]

Add several tests for Vnets. State setup as well as testing results is
done only via the API to test on the API boundaries not not against the
internal state. Internal state is mocked to avoid requiring access to
system files or pmxcfs.

Tests validate the events of a nic joining a Vnet or a nic staring on a vnet
with different subnet configurations.
Further descriptions in the commit.

Stefan Lendl (8):
  refactor(sdn): extract cfs_read_file(datacenter.cfg)
  refactor(dnsmasq): extract systemctl_service function
  refactor(dnsmasq): extract ethers_file function
  refactor(dnsmasq): extract update_lease function
  refactor(controllers): extract read_etc_network_interfaces
  refactor(evpn): extract read_local_frr_config
  refactor(api): extract create_etc_interfaces_sdn_dir
  test(vnets): add test_vnets_blackbox

 src/PVE/API2/Network/SDN/Zones.pm             |   6 +-
 src/PVE/Network/SDN/Controllers.pm            |  16 +-
 src/PVE/Network/SDN/Controllers/EvpnPlugin.pm |  10 +-
 src/PVE/Network/SDN/Dhcp/Dnsmasq.pm           |  47 +-
 src/PVE/Network/SDN/Zones/EvpnPlugin.pm       |   3 +-
 src/PVE/Network/SDN/Zones/Plugin.pm           |   5 +
 src/PVE/Network/SDN/Zones/SimplePlugin.pm     |   2 +-
 src/test/Makefile                             |   5 +-
 src/test/run_test_vnets_blackbox.pl           | 797 ++++++++++++++++++
 9 files changed, 863 insertions(+), 28 deletions(-)
 create mode 100755 src/test/run_test_vnets_blackbox.pl

-- 
2.42.0

[pve-devel] [PATCH pve-network 1/8] refactor(sdn): extract cfs_read_file(datacenter.cfg)

[Stefan Lendl]

to datacenter_config() functions in Zones::Plugin.
This allows mocking the datacenter.cfg in tests.

Signed-off-by: Stefan Lendl <s.lendl@proxmox.com>
---
 src/PVE/Network/SDN/Zones/EvpnPlugin.pm   | 3 ++-
 src/PVE/Network/SDN/Zones/Plugin.pm       | 5 +++++
 src/PVE/Network/SDN/Zones/SimplePlugin.pm | 2 +-
 3 files changed, 8 insertions(+), 2 deletions(-)

diff --git a/src/PVE/Network/SDN/Zones/EvpnPlugin.pm b/src/PVE/Network/SDN/Zones/EvpnPlugin.pm
index 655a9f0..4843756 100644
--- a/src/PVE/Network/SDN/Zones/EvpnPlugin.pm
+++ b/src/PVE/Network/SDN/Zones/EvpnPlugin.pm
@@ -3,6 +3,7 @@ package PVE::Network::SDN::Zones::EvpnPlugin;
 use strict;
 use warnings;
 use PVE::Network::SDN::Zones::VxlanPlugin;
+use PVE::Network::SDN::Zones::Plugin;
 use PVE::Exception qw(raise raise_param_exc);
 use PVE::JSONSchema qw(get_standard_option);
 use PVE::Tools qw($IPV4RE);
@@ -294,7 +295,7 @@ sub on_update_hook {
     }
 
     if (!defined($zone_cfg->{ids}->{$zoneid}->{'mac'})) {
-	my $dc = PVE::Cluster::cfs_read_file('datacenter.cfg');
+        my $dc = PVE::Network::SDN::Zones::Plugin->datacenter_config();
 	$zone_cfg->{ids}->{$zoneid}->{'mac'} = PVE::Tools::random_ether_addr($dc->{mac_prefix});
     }
 }
diff --git a/src/PVE/Network/SDN/Zones/Plugin.pm b/src/PVE/Network/SDN/Zones/Plugin.pm
index b55b967..247d0b2 100644
--- a/src/PVE/Network/SDN/Zones/Plugin.pm
+++ b/src/PVE/Network/SDN/Zones/Plugin.pm
@@ -356,4 +356,9 @@ sub get_bridge_ifaces {
 
     return @bridge_ifaces;
 }
+
+sub datacenter_config {
+    return PVE::Cluster::cfs_read_file('datacenter.cfg');
+}
+
 1;
diff --git a/src/PVE/Network/SDN/Zones/SimplePlugin.pm b/src/PVE/Network/SDN/Zones/SimplePlugin.pm
index c996bf3..65e9ad4 100644
--- a/src/PVE/Network/SDN/Zones/SimplePlugin.pm
+++ b/src/PVE/Network/SDN/Zones/SimplePlugin.pm
@@ -139,7 +139,7 @@ sub vnet_update_hook {
     raise_param_exc({ tag => "vlan tag is not allowed on simple zone"}) if defined($tag);
 
     if (!defined($vnet->{mac})) {
-        my $dc = PVE::Cluster::cfs_read_file('datacenter.cfg');
+        my $dc = PVE::Network::SDN::Zones::Plugin::datacenter_config();
         $vnet->{mac} = PVE::Tools::random_ether_addr($dc->{mac_prefix});
     }
 }
-- 
2.42.0

[pve-devel] [PATCH pve-network 2/8] refactor(dnsmasq): extract systemctl_service function

[Stefan Lendl]

inside Dnsmasq.pm call systemctl services via a sub to allow mocking in
tests.

Signed-off-by: Stefan Lendl <s.lendl@proxmox.com>
---
 src/PVE/Network/SDN/Dhcp/Dnsmasq.pm | 18 ++++++++++++------
 1 file changed, 12 insertions(+), 6 deletions(-)

diff --git a/src/PVE/Network/SDN/Dhcp/Dnsmasq.pm b/src/PVE/Network/SDN/Dhcp/Dnsmasq.pm
index 2844943..f9f1c39 100644
--- a/src/PVE/Network/SDN/Dhcp/Dnsmasq.pm
+++ b/src/PVE/Network/SDN/Dhcp/Dnsmasq.pm
@@ -101,7 +101,7 @@ sub add_ip_mapping {
     }
 
     my $service_name = "dnsmasq\@$dhcpid";
-    PVE::Tools::run_command(['systemctl', 'reload', $service_name]) if $reload;
+    systemctl_service('reload', $service_name) if $reload;
 
     #update lease as ip could still be associated to an old removed mac
     my $bus = Net::DBus->system();
@@ -163,6 +163,12 @@ sub configure_vnet {
     );
 }
 
+sub systemctl_service {
+    my ($action, $service) = @_;
+
+    PVE::Tools::run_command(['systemctl', $action, $service]);
+}
+
 sub before_configure {
     my ($class, $dhcpid) = @_;
 
@@ -250,9 +256,9 @@ sub after_configure {
 
     my $service_name = "dnsmasq\@$dhcpid";
 
-    PVE::Tools::run_command(['systemctl', 'reload', 'dbus']);
-    PVE::Tools::run_command(['systemctl', 'enable', $service_name]);
-    PVE::Tools::run_command(['systemctl', 'restart', $service_name]);
+    systemctl_service('reload', 'dbus');
+    systemctl_service('enable', $service_name);
+    systemctl_service('restart', $service_name);
 }
 
 sub before_regenerate {
@@ -260,8 +266,8 @@ sub before_regenerate {
 
     return if !assert_dnsmasq_installed($noerr);
 
-    PVE::Tools::run_command(['systemctl', 'stop', "dnsmasq@*"]);
-    PVE::Tools::run_command(['systemctl', 'disable', 'dnsmasq@']);
+    systemctl_service('stop', "dnsmasq@*");
+    systemctl_service('disable', 'dnsmasq@');
 }
 
 sub after_regenerate {
-- 
2.42.0

[pve-devel] [PATCH pve-network 3/8] refactor(dnsmasq): extract ethers_file function

[Stefan Lendl]

That generates the ethers path so that it can be mocked.

Signed-off-by: Stefan Lendl <s.lendl@proxmox.com>
---
 src/PVE/Network/SDN/Dhcp/Dnsmasq.pm | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/src/PVE/Network/SDN/Dhcp/Dnsmasq.pm b/src/PVE/Network/SDN/Dhcp/Dnsmasq.pm
index f9f1c39..5a227ba 100644
--- a/src/PVE/Network/SDN/Dhcp/Dnsmasq.pm
+++ b/src/PVE/Network/SDN/Dhcp/Dnsmasq.pm
@@ -33,10 +33,15 @@ my sub assert_dnsmasq_installed {
     return 1;
 }
 
+sub ethers_file {
+    my ($dhcpid) = @_;
+    return "$DNSMASQ_CONFIG_ROOT/$dhcpid/ethers";
+}
+
 sub add_ip_mapping {
     my ($class, $dhcpid, $macdb, $mac, $ip4, $ip6) = @_;
 
-    my $ethers_file = "$DNSMASQ_CONFIG_ROOT/$dhcpid/ethers";
+    my $ethers_file = ethers_file($dhcpid);
     my $ethers_tmp_file = "$ethers_file.tmp";
 
     my $reload = undef;
-- 
2.42.0

[pve-devel] [PATCH pve-network 4/8] refactor(dnsmasq): extract update_lease function

[Stefan Lendl]

that calls the dbus service.
can be mocked.

Signed-off-by: Stefan Lendl <s.lendl@proxmox.com>
---
 src/PVE/Network/SDN/Dhcp/Dnsmasq.pm | 22 ++++++++++++----------
 1 file changed, 12 insertions(+), 10 deletions(-)

diff --git a/src/PVE/Network/SDN/Dhcp/Dnsmasq.pm b/src/PVE/Network/SDN/Dhcp/Dnsmasq.pm
index 5a227ba..c14f5d7 100644
--- a/src/PVE/Network/SDN/Dhcp/Dnsmasq.pm
+++ b/src/PVE/Network/SDN/Dhcp/Dnsmasq.pm
@@ -38,6 +38,17 @@ sub ethers_file {
     return "$DNSMASQ_CONFIG_ROOT/$dhcpid/ethers";
 }
 
+sub update_lease {
+    my ($dhcpid, $ip4, $mac) = @_;
+    #update lease as ip could still be associated to an old removed mac
+    my $bus = Net::DBus->system();
+    my $dnsmasq = $bus->get_service("uk.org.thekelleys.dnsmasq.$dhcpid");
+    my $manager = $dnsmasq->get_object("/uk/org/thekelleys/dnsmasq","uk.org.thekelleys.dnsmasq.$dhcpid");
+
+    my @hostname = unpack("C*", "*");
+    $manager->AddDhcpLease($ip4, $mac, \@hostname, undef, 0, 0, 0) if $ip4;
+}
+
 sub add_ip_mapping {
     my ($class, $dhcpid, $macdb, $mac, $ip4, $ip6) = @_;
 
@@ -107,16 +118,7 @@ sub add_ip_mapping {
 
     my $service_name = "dnsmasq\@$dhcpid";
     systemctl_service('reload', $service_name) if $reload;
-
-    #update lease as ip could still be associated to an old removed mac
-    my $bus = Net::DBus->system();
-    my $dnsmasq = $bus->get_service("uk.org.thekelleys.dnsmasq.$dhcpid");
-    my $manager = $dnsmasq->get_object("/uk/org/thekelleys/dnsmasq","uk.org.thekelleys.dnsmasq.$dhcpid");
-
-    my @hostname = unpack("C*", "*");
-    $manager->AddDhcpLease($ip4, $mac, \@hostname, undef, 0, 0, 0) if $ip4;
-#    $manager->AddDhcpLease($ip6, $mac, \@hostname, undef, 0, 0, 0) if $ip6;
-
+    update_lease($dhcpid, $ip4, $mac);
 }
 
 sub configure_subnet {
-- 
2.42.0

[pve-devel] [PATCH pve-network 5/8] refactor(controllers): extract read_etc_network_interfaces

[Stefan Lendl]

to allow mocking local fs access.

Signed-off-by: Stefan Lendl <s.lendl@proxmox.com>
---
 src/PVE/Network/SDN/Controllers.pm | 16 +++++++++++-----
 1 file changed, 11 insertions(+), 5 deletions(-)

diff --git a/src/PVE/Network/SDN/Controllers.pm b/src/PVE/Network/SDN/Controllers.pm
index 167d3ea..fd7ad54 100644
--- a/src/PVE/Network/SDN/Controllers.pm
+++ b/src/PVE/Network/SDN/Controllers.pm
@@ -70,6 +70,16 @@ sub complete_sdn_controller {
     return  $cmdname eq 'add' ? [] : [ PVE::Network::SDN::sdn_controllers_ids($cfg) ];
 }
 
+sub read_etc_network_interfaces {
+    # read main config for physical interfaces
+    my $current_config_file = "/etc/network/interfaces";
+    my $fh = IO::File->new($current_config_file) or die "failed to open $current_config_file - $!\n";
+    my $interfaces_config = PVE::INotify::read_etc_network_interfaces($current_config_file, $fh);
+    $fh->close();
+
+    return $interfaces_config;
+}
+
 sub generate_controller_config {
 
     my $cfg = PVE::Network::SDN::running_config();
@@ -79,11 +89,7 @@ sub generate_controller_config {
 
     return if !$vnet_cfg && !$zone_cfg && !$controller_cfg;
 
-    # read main config for physical interfaces
-    my $current_config_file = "/etc/network/interfaces";
-    my $fh = IO::File->new($current_config_file) or die "failed to open $current_config_file - $!\n";
-    my $interfaces_config = PVE::INotify::read_etc_network_interfaces($current_config_file, $fh);
-    $fh->close();
+    my $interfaces_config = read_etc_network_interfaces();
 
     # check uplinks
     my $uplinks = {};
-- 
2.42.0

[pve-devel] [PATCH pve-network 6/8] refactor(evpn): extract read_local_frr_config

[Stefan Lendl]

to allow mocking local fs access.

Signed-off-by: Stefan Lendl <s.lendl@proxmox.com>
---
 src/PVE/Network/SDN/Controllers/EvpnPlugin.pm | 10 ++++++++--
 1 file changed, 8 insertions(+), 2 deletions(-)

diff --git a/src/PVE/Network/SDN/Controllers/EvpnPlugin.pm b/src/PVE/Network/SDN/Controllers/EvpnPlugin.pm
index f320139..fc297f9 100644
--- a/src/PVE/Network/SDN/Controllers/EvpnPlugin.pm
+++ b/src/PVE/Network/SDN/Controllers/EvpnPlugin.pm
@@ -461,6 +461,12 @@ sub generate_frr_list {
     }
 }
 
+sub read_local_frr_config {
+    if (-e "/etc/frr/frr.conf.local") {
+	return file_get_contents("/etc/frr/frr.conf.local");
+    }
+};
+
 sub generate_controller_rawconfig {
     my ($class, $plugin_config, $config) = @_;
 
@@ -474,8 +480,8 @@ sub generate_controller_rawconfig {
     push @{$final_config}, "service integrated-vtysh-config";
     push @{$final_config}, "!";
 
-    if (-e "/etc/frr/frr.conf.local") {
-	my $local_conf = file_get_contents("/etc/frr/frr.conf.local");
+    my $local_conf = read_local_frr_config();
+    if ($local_conf) {
 	parse_merge_frr_local_config($config, $local_conf);
     }
 
-- 
2.42.0

[pve-devel] [PATCH pve-network 7/8] refactor(api): extract create_etc_interfaces_sdn_dir

[Stefan Lendl]

can be mocked in tests to prevent system fs access in tests

Signed-off-by: Stefan Lendl <s.lendl@proxmox.com>
---
 src/PVE/API2/Network/SDN/Zones.pm | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/src/PVE/API2/Network/SDN/Zones.pm b/src/PVE/API2/Network/SDN/Zones.pm
index b09c9ad..35e2f7f 100644
--- a/src/PVE/API2/Network/SDN/Zones.pm
+++ b/src/PVE/API2/Network/SDN/Zones.pm
@@ -186,6 +186,10 @@ __PACKAGE__->register_method ({
 	return &$api_sdn_zones_config($cfg, $param->{zone});
     }});
 
+sub create_etc_interfaces_sdn_dir {
+    mkdir("/etc/pve/sdn");
+}
+
 __PACKAGE__->register_method ({
     name => 'create',
     protected => 1,
@@ -207,7 +211,7 @@ __PACKAGE__->register_method ({
 	my $opts = $plugin->check_config($id, $param, 1, 1);
 
 	PVE::Cluster::check_cfs_quorum();
-	mkdir("/etc/pve/sdn");
+	create_etc_interfaces_sdn_dir();
 
 	PVE::Network::SDN::lock_sdn_config(sub {
 	    my $zone_cfg = PVE::Network::SDN::Zones::config();
-- 
2.42.0

[pve-devel] [PATCH pve-network 8/8] test(vnets): add test_vnets_blackbox

[Stefan Lendl]

Add several tests for Vnets. State setup as well as testing results is
done only via the API to test on the API boundaries not not against the
internal state. Internal state is mocked to avoid requiring access to
system files or pmxcfs.

Mocking is done by reading and writing to a hash that holds the entire
state of SDN. The state is reset after every test run.

Testing is done via helper functions: nic_join and nic_start.
When a nic joins a Vnet, currently it always - and only - calls
add_next_free_cidr(). The same is true if a nic starts on Vnet, which
only calles add_dhcp_mapping.

These test functions homogenize the parameter list in contrast to the
current calls to the current functions.  These functions should move to
Vnets.pm to be called from QemuServer and LXC!

The run_test function takes a function pointer and passes the rest of
the arguments to the test functions after resetting the test state.
This allows fine-grained parameterization per-test directly in the code
instead of separated files that require the entire state to be passed
in.

The tests setup the SDN by creating a simple zone and a simple vnet. The
nic_join and nic_start function is called with different subnet
configuration wiht and without a dhcp-range configured and with or
without an already present IP in the IPAM.

Several of the tests fail and uncovers bugs, that shall be fixed in
subsequent commits.

Signed-off-by: Stefan Lendl <s.lendl@proxmox.com>
---
 src/test/Makefile                   |   5 +-
 src/test/run_test_vnets_blackbox.pl | 797 ++++++++++++++++++++++++++++
 2 files changed, 801 insertions(+), 1 deletion(-)
 create mode 100755 src/test/run_test_vnets_blackbox.pl

diff --git a/src/test/Makefile b/src/test/Makefile
index eb59d5f..5a937a4 100644
--- a/src/test/Makefile
+++ b/src/test/Makefile
@@ -1,6 +1,6 @@
 all: test
 
-test: test_zones test_ipams test_dns test_subnets
+test: test_zones test_ipams test_dns test_subnets test_vnets_blackbox
 
 test_zones: run_test_zones.pl
 	./run_test_zones.pl
@@ -14,4 +14,7 @@ test_dns: run_test_dns.pl
 test_subnets: run_test_subnets.pl
 	./run_test_subnets.pl
 
+test_vnets_blackbox: run_test_vnets_blackbox.pl
+	./run_test_vnets_blackbox.pl
+
 clean:
diff --git a/src/test/run_test_vnets_blackbox.pl b/src/test/run_test_vnets_blackbox.pl
new file mode 100755
index 0000000..2fd5bd7
--- /dev/null
+++ b/src/test/run_test_vnets_blackbox.pl
@@ -0,0 +1,797 @@
+#!/usr/bin/perl
+
+use strict;
+use warnings;
+
+use lib qw(..);
+use File::Slurp;
+use List::Util qw(first all);
+use NetAddr::IP qw(:lower);
+
+use Test::More;
+use Test::MockModule;
+
+use PVE::Tools qw(extract_param file_set_contents);
+
+use PVE::Network::SDN;
+use PVE::Network::SDN::Zones;
+use PVE::Network::SDN::Zones::Plugin;
+use PVE::Network::SDN::Controllers;
+use PVE::Network::SDN::Dns;
+use PVE::Network::SDN::Vnets;
+
+use PVE::RESTEnvironment;
+
+use PVE::API2::Network::SDN::Zones;
+use PVE::API2::Network::SDN::Subnets;
+use PVE::API2::Network::SDN::Vnets;
+use PVE::API2::Network::SDN::Ipams;
+
+use Data::Dumper qw(Dumper);
+$Data::Dumper::Sortkeys = 1;
+$Data::Dumper::Indent = 1;
+
+my $TMP_ETHERS_FILE = "/tmp/ethers";
+
+my $test_state = undef;
+sub clear_test_state {
+    $test_state = {
+	locks => {},
+	datacenter_config => {},
+	subnets_config => {},
+	controller_config => {},
+	dns_config => {},
+	zones_config => {},
+	vnets_config => {},
+	macdb => {},
+	ipamdb => {},
+	ipam_config => {
+	    'ids' => {
+		'pve' => {
+		    'type' => 'pve'
+		},
+	    }
+	},
+    };
+    PVE::Tools::file_set_contents($TMP_ETHERS_FILE, "\n");
+}
+clear_test_state();
+
+my $mocked_cfs_lock_file = sub {
+    my ($filename, $timeout, $code, @param) = @_;
+
+    die "$filename already locked\n" if ($test_state->{locks}->{$filename});
+
+    $test_state->{locks}->{$filename} = 1;
+
+    my $res = eval { $code->(@param); };
+
+    delete $test_state->{locks}->{$filename};
+
+    return $res;
+};
+
+sub read_sdn_config {
+    my ($file) = @_;
+    # Read structure back in again
+    open my $in, '<', $file or die $!;
+    my $sdn_config;
+    {
+	local $/;    # slurp mode
+	$sdn_config = eval <$in>;
+    }
+    close $in;
+    return $sdn_config;
+}
+
+my $mocked_pve_sdn;
+$mocked_pve_sdn = Test::MockModule->new('PVE::Network::SDN');
+$mocked_pve_sdn->mock(
+    cfs_lock_file => $mocked_cfs_lock_file,
+);
+
+my $mocked_pve_tools = Test::MockModule->new('PVE::Tools');
+$mocked_pve_tools->mock(
+    lock_file => $mocked_cfs_lock_file,
+);
+
+
+# my $test_state->{zones_config} = {};
+my $mocked_sdn_zones;
+$mocked_sdn_zones = Test::MockModule->new('PVE::Network::SDN::Zones');
+$mocked_sdn_zones->mock(
+    config => sub {
+	# print "mocked zones\n";
+	# warn Dumper($test_state).' ';
+	# print Dumper($test_state->{zones_config});
+	return $test_state->{zones_config};
+    },
+    write_config => sub {
+	my ($cfg) = @_;
+	# print "mocked write_config zones\n";
+	# print Dumper($cfg);
+	$test_state->{zones_config} = $cfg;
+    },
+);
+
+# my $datacenter_config = undef;
+my $mocked_sdn_zones_super_plugin;
+$mocked_sdn_zones_super_plugin = Test::MockModule->new('PVE::Network::SDN::Zones::Plugin');
+$mocked_sdn_zones_super_plugin->mock(
+    datacenter_config => sub {
+	# print "mocked datacenter_config\n";
+	return $test_state->{datacenter_config};
+    },
+);
+
+
+# my $test_state->{vnets_config} = {};
+my $mocked_sdn_vnets;
+$mocked_sdn_vnets = Test::MockModule->new('PVE::Network::SDN::Vnets');
+$mocked_sdn_vnets->mock(
+    config => sub {
+	# print "mocked vnets\n";
+	# warn Dumper($test_state->{vnets_config}).' ';
+	return $test_state->{vnets_config};
+    },
+    write_config => sub {
+	my ($cfg) = @_;
+	# print "mocked vnets write_config\n";
+	# warn Dumper($cfg, $test_state->{vnets_config}).' ';
+	$test_state->{vnets_config} = $cfg;
+    },
+    cfs_lock_file => $mocked_cfs_lock_file,
+);
+
+
+# my $test_state->{subnets_config} = undef;
+my $mocked_sdn_subnets;
+$mocked_sdn_subnets = Test::MockModule->new('PVE::Network::SDN::Subnets');
+$mocked_sdn_subnets->mock(
+    config => sub {
+	# print "mocked subnet\n";
+	return $test_state->{subnets_config};
+    },
+    write_config => sub {
+	my ($cfg) = @_;
+	$test_state->{subnets_config} = $cfg;
+    },
+    cfs_lock_file => $mocked_cfs_lock_file,
+    # verify_dns_zone => sub {
+    # 	return;
+    # },
+    # add_dns_record => sub {
+    # 	return;
+    # }
+);
+
+# my $test_state->{controller_config} = undef;
+my $mocked_sdn_controller;
+$mocked_sdn_controller = Test::MockModule->new('PVE::Network::SDN::Controllers');
+$mocked_sdn_controller->mock(
+    config => sub {
+	# print "mocked controller\n";
+	return $test_state->{controller_config};
+    },
+    write_config => sub {
+	my ($cfg) = @_;
+	$test_state->{controller_config} = $cfg;
+    },
+    cfs_lock_file => $mocked_cfs_lock_file,
+);
+
+
+my $mocked_sdn_dns;
+$mocked_sdn_dns = Test::MockModule->new('PVE::Network::SDN::Dns');
+$mocked_sdn_dns->mock(
+    config => sub {
+	# print "mocked dns\n";
+	return $test_state->{dns_config};
+    },
+    write_config => sub {
+	my ($cfg) = @_;
+	$test_state->{dns_config} = $cfg;
+    },
+    cfs_lock_file => $mocked_cfs_lock_file,
+);
+
+
+my $mocked_sdn_ipams;
+$mocked_sdn_ipams = Test::MockModule->new('PVE::Network::SDN::Ipams');
+$mocked_sdn_ipams->mock(
+    config => sub {
+	# print "mocked ipam config\n";
+	return $test_state->{ipam_config};
+    },
+    write_config => sub {
+	my ($cfg) = @_;
+	$test_state->{ipam_config} = $cfg;
+    },
+    read_macdb => sub {
+	return $test_state->{macdb};
+    },
+    write_macdb => sub {
+	my ($cfg) = @_;
+	# print "write mocked macsdb\n";
+	$test_state->{macdb} = $cfg;
+    },
+    cfs_lock_file => $mocked_cfs_lock_file,
+);
+
+# my $test_state->{ipamdb} = {};
+my $ipam_plugin = PVE::Network::SDN::Ipams::Plugin->lookup("pve"); # FIXME hardcoded to pve
+my $mocked_ipam_plugin = Test::MockModule->new($ipam_plugin);
+$mocked_ipam_plugin->mock(
+    read_db => sub {
+	# print "mocked ipamdb\n";
+	return $test_state->{ipamdb};
+    },
+    write_db => sub {
+	my ($cfg) = @_;
+	# print "mocked write ipamdb\n";
+	$test_state->{ipamdb} = $cfg;
+    },
+    cfs_lock_file => $mocked_cfs_lock_file,
+);
+
+my $mocked_sdn_dhcp_dnsmasq = Test::MockModule->new('PVE::Network::SDN::Dhcp::Dnsmasq');
+$mocked_sdn_dhcp_dnsmasq->mock(
+    ethers_file => sub {
+	return "/tmp/ethers";
+    },
+    update_lease => sub {
+	my ($dhcpid, $ip4, $mac) = @_;
+    },
+    assert_dnsmasq_installed => sub {
+	return 1;
+    },
+    before_configure => sub {
+	my ($class, $dhcpid) = @_;
+    },
+    systemctl_service => sub {
+	my ($action, $service) = @_;
+	# print "mocked `systemctl $action $service`\n";
+    },
+);
+
+my $mocked_api_zones = Test::MockModule->new('PVE::API2::Network::SDN::Zones');
+$mocked_api_zones->mock(
+    'create_etc_interfaces_sdn_dir', sub {},
+);
+
+my $rpcenv = PVE::RESTEnvironment->init('priv');
+$rpcenv->init_request();
+$rpcenv->set_language("en_US.UTF-8");
+$rpcenv->set_user('root@pam');
+
+my $mocked_rpc_env_obj = Test::MockModule->new('PVE::RESTEnvironment');
+$mocked_rpc_env_obj->mock(
+    check_any => sub {
+	my ($self, $user, $path, $privs, $noerr) = @_;
+	# print "mocked check_any\n";
+	return 1;
+    },
+);
+
+# ------- TEST FUNCTIONS --------------
+
+sub nic_join {
+    my ($vnetid, $mac, $hostname, $vmid) = @_;
+    return PVE::Network::SDN::Vnets::add_next_free_cidr($vnetid, $hostname, $mac, "$vmid", undef, 1);
+}
+
+sub nic_leave {
+    my ($vnetid, $mac, $hostname) = @_;
+    return PVE::Network::SDN::Vnets::del_ips_from_mac($vnetid, $mac, $hostname);
+}
+
+sub nic_start {
+    my ($vnetid, $mac, $vmid, $hostname) = @_;
+    return PVE::Network::SDN::Vnets::add_dhcp_mapping($vnetid, $mac, $vmid, $hostname);
+}
+
+
+# ---- API HELPER FUNCTIONS FOR THE TESTS -----
+
+my $t_invalid;
+sub get_zone {
+    my ($id) = @_;
+    return eval { PVE::API2::Network::SDN::Zones->read({zone => $id}); };
+}
+# verify get_zone actually fails if invalid
+# FIXME re-add
+# my $t_invalid = get_zone("invalid");
+# die("getting an invalid zone must fail") if (!$@);
+# fail("getting an invalid zone must fail") if (defined $t_invalid);
+
+sub create_zone {
+    my ($params) = @_;
+    my $zoneid = $params->{zone};
+    # die if failed!
+    eval { PVE::API2::Network::SDN::Zones->create($params); };
+    die("creating zone failed: $@") if ($@);
+
+    my $zone = get_zone($zoneid);
+    die ("test setup: zone ($zoneid) not defined") if (!defined $zone);
+    return $zone;
+}
+
+sub get_vnet {
+    my ($id) = @_;
+    return eval { PVE::API2::Network::SDN::Vnets->read({vnet => $id}); };
+}
+# verify get_vnet
+$t_invalid = get_vnet("invalid");
+die("getting an invalid vnet must fail") if (!$@);
+fail("getting an invalid vnet must fail") if (defined $t_invalid);
+
+sub create_vnet {
+    my ($params) = @_;
+    my $vnetid = $params->{vnet};
+    PVE::API2::Network::SDN::Vnets->create($params);
+
+    my $vnet = get_vnet($vnetid);
+    die ("test setup: vnet ($vnetid) not defined") if (!defined $vnet);
+    return $vnet;
+}
+
+sub get_subnet {
+    my ($id) = @_;
+    return eval { PVE::API2::Network::SDN::Subnets->read({subnet => $id}); };
+}
+# verify get_subnet
+$t_invalid = get_subnet("invalid");
+die("getting an invalid subnet must fail") if (!$@);
+fail("getting an invalid subnet must fail") if (defined $t_invalid);
+
+sub create_subnet {
+    my ($params) = @_;
+    PVE::API2::Network::SDN::Subnets->create($params);
+
+    # warn Dumper($params).' ';
+    # FIXME get_subnet  > HOW DO I GET THE ID?
+}
+
+sub get_ipam_entries {
+    return PVE::API2::Network::SDN::Ipams->ipamindex({ipam => "pve"});
+}
+
+sub create_ip {
+    my ($param) = @_;
+    return PVE::API2::Network::SDN::Ips->ipcreate($param);
+}
+
+sub run_test {
+    my $test = shift;
+    clear_test_state();
+    $test->(@_);
+}
+
+sub get_ips_from_mac {
+    my ($mac) = @_;
+    my $ipam_entries = get_ipam_entries();
+    return grep { $_->{mac} eq $mac if defined $_->{mac} } $ipam_entries->@* if $ipam_entries;
+}
+
+sub get_ip4 {
+    my $ip4 = first { Net::IP::ip_is_ipv4($_->{ip}) } @_;
+    return $ip4->{ip} if defined $ip4;
+}
+
+sub get_ip6 {
+    my $ip6 = first { Net::IP::ip_is_ipv6($_->{ip}) } @_;
+    return $ip6->{ip} if defined $ip6;
+}
+
+
+# -------------- ACTUAL TESTS  -----------------------
+
+sub test_create_vnet_with_gateway {
+    my $test_name = (split(/::/,(caller(0))[3]))[-1];
+    my $zoneid = "TESTZONE";
+    my $vnetid = "testvnet";
+
+    my $zone = create_zone({
+	type => "simple",
+	dhcp => "dnsmasq",
+	ipam => "pve",
+	zone => $zoneid,
+    });
+
+    my $vnet = create_vnet({
+	type => "vnet",
+	zone => $zoneid,
+	vnet => $vnetid,
+    });
+
+    create_subnet({
+	type => "subnet",
+	vnet => $vnetid,
+	subnet => "10.0.0.0/24",
+	gateway => "10.0.0.1",
+	'dhcp-range' => ["start-address=10.0.0.100,end-address=10.0.0.200"],
+    });
+
+    my ($p) = first { $_->{gateway} == 1 } get_ipam_entries()->@*;
+    ok ($p, "$test_name: Gateway IP was created in IPAM");
+}
+run_test(\&test_create_vnet_with_gateway);
+
+
+sub test_without_subnet {
+    my $test_name = (split(/::/,(caller(0))[3]))[-1];
+
+    my $zoneid = "TESTZONE";
+    my $vnetid = "testvnet";
+
+    my $zone = create_zone({
+	type => "simple",
+	dhcp => "dnsmasq",
+	ipam => "pve",
+	zone => $zoneid,
+    });
+
+    my $vnet = create_vnet({
+	type => "vnet",
+	zone => $zoneid,
+	vnet => $vnetid,
+    });
+
+    my $hostname = "testhostname";
+    my $mac = "da:65:8f:18:9b:6f";
+    my $vmid = "999";
+
+    eval {
+	nic_join($vnetid, $mac, $hostname, $vmid);
+    };
+
+    if ($@) {
+	fail("$test_name: $@");
+	return;
+    }
+
+    my @ips = grep { $_->{mac} eq $mac } get_ipam_entries()->@*;
+    my $num_ips = scalar @ips;
+    is ($num_ips, 0, "$test_name: No IP allocated in IPAM");
+}
+run_test(\&test_without_subnet);
+
+
+sub test_nic_join {
+    my ($test_name, $subnets) = @_;
+
+    die "$test_name: we're expecting an array of subnets" if !$subnets;
+    my $num_subnets = scalar $subnets->@*;
+    die "$test_name: we're expecting an array of subnets. $num_subnets elements found" if ($num_subnets < 1);
+    my $num_dhcp_ranges = scalar grep { $_->{'dhcp-range'} } $subnets->@*;
+
+    my $zoneid = "TESTZONE";
+    my $vnetid = "testvnet";
+
+    my $zone = create_zone({
+	type => "simple",
+	dhcp => "dnsmasq",
+	ipam => "pve",
+	zone => $zoneid,
+    });
+
+    my $vnet = create_vnet({
+	type => "vnet",
+	zone => $zoneid,
+	vnet => $vnetid,
+    });
+
+    foreach my $subnet ($subnets->@*) {
+	$subnet->{type} = "subnet";
+	$subnet->{vnet} = $vnetid;
+	create_subnet($subnet);
+    };
+
+    my $hostname = "testhostname";
+    my $mac = "da:65:8f:18:9b:6f";
+    my $vmid = "999";
+
+    eval {
+	nic_join($vnetid, $mac, $hostname, $vmid);
+    };
+
+    if ($@) {
+	fail("$test_name: $@");
+	return;
+    }
+
+    my @ips = get_ips_from_mac($mac);
+    my $num_ips = scalar @ips;
+    is ($num_ips, $num_dhcp_ranges, "$test_name: Expecting $num_dhcp_ranges IPs, found $num_ips");
+    ok ((all { ($_->{vnet} eq $vnetid && $_->{zone} eq $zoneid) } @ips),
+	"$test_name: all IPs in correct vnet and zone"
+    );
+}
+
+run_test(
+    \&test_nic_join,
+    "nic_join IPv4 with dhcp",
+    [{
+	subnet => "10.0.0.0/24",
+	gateway => "10.0.0.1",
+	'dhcp-range' => ["start-address=10.0.0.100,end-address=10.0.0.200"],
+    },
+]);
+
+run_test(
+    \&test_nic_join,
+    "nic_join IPv4 no dhcp",
+    [{
+	subnet => "10.0.0.0/24",
+	gateway => "10.0.0.1",
+    },
+]);
+
+run_test(
+    \&test_nic_join,
+    "nic_join IPv6 with dhcp",
+    [{
+	subnet => "8888::/64",
+	gateway => "8888::1",
+	'dhcp-range' => ["start-address=8888::100,end-address=8888::200"],
+    },
+]);
+
+run_test(
+    \&test_nic_join,
+    "nic_join IPv6 no dhcp",
+    [{
+	subnet => "8888::/64",
+	gateway => "8888::1",
+    },
+]);
+
+run_test(
+    \&test_nic_join,
+    "nic_join IPv4+6 with dhcp",
+    [{
+	subnet => "10.0.0.0/24",
+	gateway => "10.0.0.1",
+	'dhcp-range' => ["start-address=10.0.0.100,end-address=10.0.0.200"],
+    }, {
+	subnet => "8888::/64",
+	gateway => "8888::1",
+	'dhcp-range' => ["start-address=8888::100,end-address=8888::200"],
+    },
+]);
+
+run_test(
+    \&test_nic_join,
+    "nic_join IPv4+6 no dhcp",
+    [{
+	subnet => "10.0.0.0/24",
+	gateway => "10.0.0.1",
+    }, {
+	subnet => "8888::/64",
+	gateway => "8888::1",
+    },
+]);
+
+run_test(
+    \&test_nic_join,
+    "nic_join IPv4 no DHCP, IPv6 with DHCP",
+    [{
+	subnet => "10.0.0.0/24",
+	gateway => "10.0.0.1",
+    }, {
+	subnet => "8888::/64",
+	gateway => "8888::1",
+	'dhcp-range' => ["start-address=8888::100,end-address=8888::200"],
+    },
+]);
+
+run_test(
+    \&test_nic_join,
+    "nic_join IPv4 with DHCP, IPv6 no DHCP",
+    [{
+	subnet => "10.0.0.0/24",
+	gateway => "10.0.0.1",
+	'dhcp-range' => ["start-address=10.0.0.100,end-address=10.0.0.200"],
+    }, {
+	subnet => "8888::/64",
+	gateway => "8888::1",
+    },
+]);
+
+
+# -------------- nic_start
+sub test_nic_start {
+    my ($test_name, $subnets, $current_ip4, $current_ip6) = @_;
+
+    die "$test_name: we're expecting an array of subnets" if !$subnets;
+    my $num_subnets = scalar $subnets->@*;
+    die "$test_name: we're expecting an array of subnets. $num_subnets elements found" if ($num_subnets < 1);
+
+    # my $num_expected_ips = 0;
+    # $num_expected_ips++ if $current_ip4;
+    # $num_expected_ips++ if $current_ip6;
+    # print "expecting ips: $num_expected_ips\n";
+
+    my $num_expected_ips = scalar grep { $_->{'dhcp-range'} } $subnets->@*;
+    if (defined $current_ip4 && defined $current_ip6) {
+        # if we already have an IP for a subnet without a dhcp-range (special case for a single test)
+	$num_expected_ips = 2;
+    }
+
+    my $zoneid = "TESTZONE";
+    my $vnetid = "testvnet";
+
+    my $zone = create_zone({
+	type => "simple",
+	dhcp => "dnsmasq",
+	ipam => "pve",
+	zone => $zoneid,
+    });
+
+    my $vnet = create_vnet({
+	type => "vnet",
+	zone => $zoneid,
+	vnet => $vnetid,
+    });
+
+    foreach my $subnet ($subnets->@*) {
+	$subnet->{type} = "subnet";
+	$subnet->{vnet} = $vnetid;
+	create_subnet($subnet);
+    };
+
+    my $hostname = "testhostname";
+    my $mac = "da:65:8f:18:9b:6f";
+    my $vmid = "999";
+
+    if ($current_ip4) {
+	create_ip({
+	    zone => $zoneid,
+	    vnet => $vnetid,
+	    mac => $mac,
+	    ip => $current_ip4,
+	});
+    }
+
+    if ($current_ip6) {
+	create_ip({
+	    zone => $zoneid,
+	    vnet => $vnetid,
+	    mac => $mac,
+	    ip => $current_ip6,
+	});
+    }
+    my @current_ips = get_ips_from_mac($mac);
+    is ( get_ip4(@current_ips), $current_ip4, "$test_name: setup current IPv4: $current_ip4" ) if defined $current_ip4;
+    is ( get_ip6(@current_ips), $current_ip6, "$test_name: setup current IPv6: $current_ip6" ) if defined $current_ip6;
+
+    eval {
+	nic_start($vnetid, $mac, $hostname, $vmid);
+    };
+
+    if ($@) {
+	fail("$test_name: $@");
+	return;
+    }
+
+    my @ips = get_ips_from_mac($mac);
+    my $num_ips = scalar @ips;
+    is ($num_ips, $num_expected_ips, "$test_name: Expecting $num_expected_ips IPs, found $num_ips");
+    ok ((all { ($_->{vnet} eq $vnetid && $_->{zone} eq $zoneid) } @ips),
+	"$test_name: all IPs in correct vnet and zone"
+    );
+
+    is ( get_ip4(@ips), $current_ip4, "$test_name: still current IPv4: $current_ip4" ) if $current_ip4;
+    is ( get_ip6(@ips), $current_ip6, "$test_name: still current IPv6: $current_ip6" ) if $current_ip6;
+}
+
+run_test(
+    \&test_nic_start,
+    "nic_start no IP, IPv4 subnet with dhcp",
+    [{
+	subnet => "10.0.0.0/24",
+	gateway => "10.0.0.1",
+	'dhcp-range' => ["start-address=10.0.0.100,end-address=10.0.0.200"],
+    },
+]);
+
+run_test(
+    \&test_nic_start,
+    "nic_start already IP, IPv4 subnet with dhcp",
+    [{
+	subnet => "10.0.0.0/24",
+	gateway => "10.0.0.1",
+	'dhcp-range' => ["start-address=10.0.0.100,end-address=10.0.0.200"],
+    }],
+    "10.0.0.99"
+);
+
+run_test(
+    \&test_nic_start,
+    "nic_start already IP, IPv6 subnet with dhcp",
+    [{
+	subnet => "8888::/64",
+	gateway => "8888::1",
+	'dhcp-range' => ["start-address=8888::100,end-address=8888::200"],
+    }],
+    undef,
+    "8888::99"
+);
+
+run_test(
+    \&test_nic_start,
+    "nic_start IP, IPv4+6 subnet with dhcp",
+    [{
+	subnet => "10.0.0.0/24",
+	gateway => "10.0.0.1",
+	'dhcp-range' => ["start-address=10.0.0.100,end-address=10.0.0.200"],
+    }, {
+	subnet => "8888::/64",
+	gateway => "8888::1",
+	'dhcp-range' => ["start-address=8888::100,end-address=8888::200"],
+    },
+]);
+
+run_test(
+    \&test_nic_start,
+    "nic_start already IPv4, IPv4+6 subnet with dhcp",
+    [{
+	subnet => "10.0.0.0/24",
+	gateway => "10.0.0.1",
+	'dhcp-range' => ["start-address=10.0.0.100,end-address=10.0.0.200"],
+    }, {
+	subnet => "8888::/64",
+	gateway => "8888::1",
+	'dhcp-range' => ["start-address=8888::100,end-address=8888::200"],
+    }],
+    "10.0.0.99"
+);
+
+run_test(
+    \&test_nic_start,
+    "nic_start already IPv6, IPv4+6 subnet with dhcp",
+    [{
+	subnet => "10.0.0.0/24",
+	gateway => "10.0.0.1",
+	'dhcp-range' => ["start-address=10.0.0.100,end-address=10.0.0.200"],
+    }, {
+	subnet => "8888::/64",
+	gateway => "8888::1",
+	'dhcp-range' => ["start-address=8888::100,end-address=8888::200"],
+    }],
+    undef,
+    "8888::99"
+);
+
+run_test(
+    \&test_nic_start,
+    "nic_start already IPv4+6, IPv4+6 subnets with dhcp",
+    [{
+	subnet => "10.0.0.0/24",
+	gateway => "10.0.0.1",
+	'dhcp-range' => ["start-address=10.0.0.100,end-address=10.0.0.200"],
+    }, {
+	subnet => "8888::/64",
+	gateway => "8888::1",
+	'dhcp-range' => ["start-address=8888::100,end-address=8888::200"],
+    }],
+    "10.0.0.99",
+    "8888::99"
+);
+
+run_test(
+    \&test_nic_start,
+    "nic_start already IPv4+6, only IPv4 subnet with dhcp",
+    [{
+	subnet => "10.0.0.0/24",
+	gateway => "10.0.0.1",
+	'dhcp-range' => ["start-address=10.0.0.100,end-address=10.0.0.200"],
+    }, {
+	subnet => "8888::/64",
+	gateway => "8888::1",
+    }],
+    "10.0.0.99",
+    "8888::99"
+);
+
+done_testing();
-- 
2.42.0

Re: [pve-devel] [PATCH pve-network 8/8] test(vnets): add test_vnets_blackbox

[Max Carrara]

On Wed Jan 3, 2024 at 4:37 PM CET, Stefan Lendl wrote:
> Add several tests for Vnets. State setup as well as testing results is
> done only via the API to test on the API boundaries not not against the
> internal state. Internal state is mocked to avoid requiring access to
> system files or pmxcfs.
>
> Mocking is done by reading and writing to a hash that holds the entire
> state of SDN. The state is reset after every test run.
>
> Testing is done via helper functions: nic_join and nic_start.
> When a nic joins a Vnet, currently it always - and only - calls
> add_next_free_cidr(). The same is true if a nic starts on Vnet, which
> only calles add_dhcp_mapping.
>
> These test functions homogenize the parameter list in contrast to the
> current calls to the current functions.  These functions should move to
> Vnets.pm to be called from QemuServer and LXC!
>
> The run_test function takes a function pointer and passes the rest of
> the arguments to the test functions after resetting the test state.
> This allows fine-grained parameterization per-test directly in the code
> instead of separated files that require the entire state to be passed
> in.
>
> The tests setup the SDN by creating a simple zone and a simple vnet. The
> nic_join and nic_start function is called with different subnet
> configuration wiht and without a dhcp-range configured and with or
> without an already present IP in the IPAM.

I really like where this is going! Now that I've read through this
patch, it's become clear why you factored so many calls to commands etc.
into their own `sub`s.

Since you mentioned that this is more of an RFC off-list, I get why
there are a bunch of lines that are commented out at the moment. Those
obviously shouldn't be committed later on.

>
> Several of the tests fail and uncovers bugs, that shall be fixed in
> subsequent commits.

Would be nice to perhaps also have those in the final series though ;)

Another thing that stood out to me is that some cases could be
declarative, e.g. the cases for `test_nic_join` and `test_nic_start`
could be declared in an array for each. You could then just loop over
the cases - that makes it easier to `plan` those cases later on.

That being said, you could perhaps structure the whole script so that
you call a `sub` named e.g. `setup` where you - well - set up all the
required logic and perform checks for the necessary pre-conditions, then
another `sub` that runs the tests (and optionally one that cleans things
up if necessary).

Though, please note that this is not a strict necessity from my side,
just something I wanted to mention! I like the way you've written your
tests a lot, it's just that I personally tend to prefer a more
declarative approach. So, it's okay if you just leave the structure as
it is right now, if you prefer it that way.

There are some more comments inline that give a little more context
regarding the above, but otherwise, LGTM - pretty good to see more
testing to be done!

>
> Signed-off-by: Stefan Lendl <s.lendl@proxmox.com>
> ---
>  src/test/Makefile                   |   5 +-
>  src/test/run_test_vnets_blackbox.pl | 797 ++++++++++++++++++++++++++++
>  2 files changed, 801 insertions(+), 1 deletion(-)
>  create mode 100755 src/test/run_test_vnets_blackbox.pl
>
> diff --git a/src/test/Makefile b/src/test/Makefile
> index eb59d5f..5a937a4 100644
> --- a/src/test/Makefile
> +++ b/src/test/Makefile
> @@ -1,6 +1,6 @@
>  all: test
>  
> -test: test_zones test_ipams test_dns test_subnets
> +test: test_zones test_ipams test_dns test_subnets test_vnets_blackbox
>  
>  test_zones: run_test_zones.pl
>  	./run_test_zones.pl
> @@ -14,4 +14,7 @@ test_dns: run_test_dns.pl
>  test_subnets: run_test_subnets.pl
>  	./run_test_subnets.pl
>  
> +test_vnets_blackbox: run_test_vnets_blackbox.pl
> +	./run_test_vnets_blackbox.pl
> +
>  clean:
> diff --git a/src/test/run_test_vnets_blackbox.pl b/src/test/run_test_vnets_blackbox.pl
> new file mode 100755
> index 0000000..2fd5bd7
> --- /dev/null
> +++ b/src/test/run_test_vnets_blackbox.pl
> @@ -0,0 +1,797 @@
> +#!/usr/bin/perl
> +
> +use strict;
> +use warnings;
> +
> +use lib qw(..);
> +use File::Slurp;
> +use List::Util qw(first all);
> +use NetAddr::IP qw(:lower);
> +
> +use Test::More;
> +use Test::MockModule;
> +
> +use PVE::Tools qw(extract_param file_set_contents);
> +
> +use PVE::Network::SDN;
> +use PVE::Network::SDN::Zones;
> +use PVE::Network::SDN::Zones::Plugin;
> +use PVE::Network::SDN::Controllers;
> +use PVE::Network::SDN::Dns;
> +use PVE::Network::SDN::Vnets;
> +
> +use PVE::RESTEnvironment;
> +
> +use PVE::API2::Network::SDN::Zones;
> +use PVE::API2::Network::SDN::Subnets;
> +use PVE::API2::Network::SDN::Vnets;
> +use PVE::API2::Network::SDN::Ipams;
> +
> +use Data::Dumper qw(Dumper);
> +$Data::Dumper::Sortkeys = 1;
> +$Data::Dumper::Indent = 1;
> +
> +my $TMP_ETHERS_FILE = "/tmp/ethers";
> +
> +my $test_state = undef;
> +sub clear_test_state {
> +    $test_state = {
> +	locks => {},
> +	datacenter_config => {},
> +	subnets_config => {},
> +	controller_config => {},
> +	dns_config => {},
> +	zones_config => {},
> +	vnets_config => {},
> +	macdb => {},
> +	ipamdb => {},
> +	ipam_config => {
> +	    'ids' => {
> +		'pve' => {
> +		    'type' => 'pve'
> +		},
> +	    }
> +	},
> +    };
> +    PVE::Tools::file_set_contents($TMP_ETHERS_FILE, "\n");
> +}
> +clear_test_state();
> +
> +my $mocked_cfs_lock_file = sub {
> +    my ($filename, $timeout, $code, @param) = @_;
> +
> +    die "$filename already locked\n" if ($test_state->{locks}->{$filename});
> +
> +    $test_state->{locks}->{$filename} = 1;
> +
> +    my $res = eval { $code->(@param); };
> +
> +    delete $test_state->{locks}->{$filename};
> +
> +    return $res;
> +};
> +
> +sub read_sdn_config {
> +    my ($file) = @_;
> +    # Read structure back in again
> +    open my $in, '<', $file or die $!;
> +    my $sdn_config;
> +    {
> +	local $/;    # slurp mode
> +	$sdn_config = eval <$in>;

I was about to say that perlcritic complains about the above line, but
then noticed that we're using the same sub in other tests as well.
However, it's not used in this file at all, so it should probably be
removed for now (and be added when it's needed).

> +    }
> +    close $in;
> +    return $sdn_config;
> +}
> +
> +my $mocked_pve_sdn;
> +$mocked_pve_sdn = Test::MockModule->new('PVE::Network::SDN');
> +$mocked_pve_sdn->mock(
> +    cfs_lock_file => $mocked_cfs_lock_file,
> +);
> +
> +my $mocked_pve_tools = Test::MockModule->new('PVE::Tools');
> +$mocked_pve_tools->mock(
> +    lock_file => $mocked_cfs_lock_file,
> +);
> +
> +
> +# my $test_state->{zones_config} = {};
> +my $mocked_sdn_zones;
> +$mocked_sdn_zones = Test::MockModule->new('PVE::Network::SDN::Zones');
> +$mocked_sdn_zones->mock(
> +    config => sub {
> +	# print "mocked zones\n";
> +	# warn Dumper($test_state).' ';
> +	# print Dumper($test_state->{zones_config});
> +	return $test_state->{zones_config};
> +    },
> +    write_config => sub {
> +	my ($cfg) = @_;
> +	# print "mocked write_config zones\n";
> +	# print Dumper($cfg);
> +	$test_state->{zones_config} = $cfg;
> +    },
> +);
> +
> +# my $datacenter_config = undef;
> +my $mocked_sdn_zones_super_plugin;
> +$mocked_sdn_zones_super_plugin = Test::MockModule->new('PVE::Network::SDN::Zones::Plugin');
> +$mocked_sdn_zones_super_plugin->mock(
> +    datacenter_config => sub {
> +	# print "mocked datacenter_config\n";
> +	return $test_state->{datacenter_config};
> +    },
> +);
> +
> +
> +# my $test_state->{vnets_config} = {};
> +my $mocked_sdn_vnets;
> +$mocked_sdn_vnets = Test::MockModule->new('PVE::Network::SDN::Vnets');
> +$mocked_sdn_vnets->mock(
> +    config => sub {
> +	# print "mocked vnets\n";
> +	# warn Dumper($test_state->{vnets_config}).' ';
> +	return $test_state->{vnets_config};
> +    },
> +    write_config => sub {
> +	my ($cfg) = @_;
> +	# print "mocked vnets write_config\n";
> +	# warn Dumper($cfg, $test_state->{vnets_config}).' ';
> +	$test_state->{vnets_config} = $cfg;
> +    },
> +    cfs_lock_file => $mocked_cfs_lock_file,
> +);
> +
> +
> +# my $test_state->{subnets_config} = undef;
> +my $mocked_sdn_subnets;
> +$mocked_sdn_subnets = Test::MockModule->new('PVE::Network::SDN::Subnets');
> +$mocked_sdn_subnets->mock(
> +    config => sub {
> +	# print "mocked subnet\n";
> +	return $test_state->{subnets_config};
> +    },
> +    write_config => sub {
> +	my ($cfg) = @_;
> +	$test_state->{subnets_config} = $cfg;
> +    },
> +    cfs_lock_file => $mocked_cfs_lock_file,
> +    # verify_dns_zone => sub {
> +    # 	return;
> +    # },
> +    # add_dns_record => sub {
> +    # 	return;
> +    # }
> +);
> +
> +# my $test_state->{controller_config} = undef;
> +my $mocked_sdn_controller;
> +$mocked_sdn_controller = Test::MockModule->new('PVE::Network::SDN::Controllers');
> +$mocked_sdn_controller->mock(
> +    config => sub {
> +	# print "mocked controller\n";
> +	return $test_state->{controller_config};
> +    },
> +    write_config => sub {
> +	my ($cfg) = @_;
> +	$test_state->{controller_config} = $cfg;
> +    },
> +    cfs_lock_file => $mocked_cfs_lock_file,
> +);
> +
> +
> +my $mocked_sdn_dns;
> +$mocked_sdn_dns = Test::MockModule->new('PVE::Network::SDN::Dns');
> +$mocked_sdn_dns->mock(
> +    config => sub {
> +	# print "mocked dns\n";
> +	return $test_state->{dns_config};
> +    },
> +    write_config => sub {
> +	my ($cfg) = @_;
> +	$test_state->{dns_config} = $cfg;
> +    },
> +    cfs_lock_file => $mocked_cfs_lock_file,
> +);
> +
> +
> +my $mocked_sdn_ipams;
> +$mocked_sdn_ipams = Test::MockModule->new('PVE::Network::SDN::Ipams');
> +$mocked_sdn_ipams->mock(
> +    config => sub {
> +	# print "mocked ipam config\n";
> +	return $test_state->{ipam_config};
> +    },
> +    write_config => sub {
> +	my ($cfg) = @_;
> +	$test_state->{ipam_config} = $cfg;
> +    },
> +    read_macdb => sub {
> +	return $test_state->{macdb};
> +    },
> +    write_macdb => sub {
> +	my ($cfg) = @_;
> +	# print "write mocked macsdb\n";
> +	$test_state->{macdb} = $cfg;
> +    },
> +    cfs_lock_file => $mocked_cfs_lock_file,
> +);
> +
> +# my $test_state->{ipamdb} = {};
> +my $ipam_plugin = PVE::Network::SDN::Ipams::Plugin->lookup("pve"); # FIXME hardcoded to pve
> +my $mocked_ipam_plugin = Test::MockModule->new($ipam_plugin);
> +$mocked_ipam_plugin->mock(
> +    read_db => sub {
> +	# print "mocked ipamdb\n";
> +	return $test_state->{ipamdb};
> +    },
> +    write_db => sub {
> +	my ($cfg) = @_;
> +	# print "mocked write ipamdb\n";
> +	$test_state->{ipamdb} = $cfg;
> +    },
> +    cfs_lock_file => $mocked_cfs_lock_file,
> +);
> +
> +my $mocked_sdn_dhcp_dnsmasq = Test::MockModule->new('PVE::Network::SDN::Dhcp::Dnsmasq');
> +$mocked_sdn_dhcp_dnsmasq->mock(
> +    ethers_file => sub {
> +	return "/tmp/ethers";
> +    },
> +    update_lease => sub {
> +	my ($dhcpid, $ip4, $mac) = @_;
> +    },
> +    assert_dnsmasq_installed => sub {
> +	return 1;
> +    },
> +    before_configure => sub {
> +	my ($class, $dhcpid) = @_;
> +    },
> +    systemctl_service => sub {
> +	my ($action, $service) = @_;
> +	# print "mocked `systemctl $action $service`\n";
> +    },
> +);
> +
> +my $mocked_api_zones = Test::MockModule->new('PVE::API2::Network::SDN::Zones');
> +$mocked_api_zones->mock(
> +    'create_etc_interfaces_sdn_dir', sub {},
> +);
> +
> +my $rpcenv = PVE::RESTEnvironment->init('priv');
> +$rpcenv->init_request();
> +$rpcenv->set_language("en_US.UTF-8");
> +$rpcenv->set_user('root@pam');

Regarding my comment about structure further above, setting up the
rpcenv here is something that I personally would prefer to have in a
`sub setup { ... }` or similar, just so it's easier to track what's
being set up and where.

> +
> +my $mocked_rpc_env_obj = Test::MockModule->new('PVE::RESTEnvironment');
> +$mocked_rpc_env_obj->mock(
> +    check_any => sub {
> +	my ($self, $user, $path, $privs, $noerr) = @_;
> +	# print "mocked check_any\n";
> +	return 1;
> +    },
> +);
> +
> +# ------- TEST FUNCTIONS --------------
> +
> +sub nic_join {
> +    my ($vnetid, $mac, $hostname, $vmid) = @_;
> +    return PVE::Network::SDN::Vnets::add_next_free_cidr($vnetid, $hostname, $mac, "$vmid", undef, 1);
> +}
> +
> +sub nic_leave {
> +    my ($vnetid, $mac, $hostname) = @_;
> +    return PVE::Network::SDN::Vnets::del_ips_from_mac($vnetid, $mac, $hostname);
> +}
> +
> +sub nic_start {
> +    my ($vnetid, $mac, $vmid, $hostname) = @_;
> +    return PVE::Network::SDN::Vnets::add_dhcp_mapping($vnetid, $mac, $vmid, $hostname);
> +}
> +
> +
> +# ---- API HELPER FUNCTIONS FOR THE TESTS -----
> +
> +my $t_invalid;
> +sub get_zone {
> +    my ($id) = @_;
> +    return eval { PVE::API2::Network::SDN::Zones->read({zone => $id}); };
> +}
> +# verify get_zone actually fails if invalid
> +# FIXME re-add
> +# my $t_invalid = get_zone("invalid");
> +# die("getting an invalid zone must fail") if (!$@);
> +# fail("getting an invalid zone must fail") if (defined $t_invalid);
> +
> +sub create_zone {
> +    my ($params) = @_;
> +    my $zoneid = $params->{zone};
> +    # die if failed!
> +    eval { PVE::API2::Network::SDN::Zones->create($params); };
> +    die("creating zone failed: $@") if ($@);
> +
> +    my $zone = get_zone($zoneid);
> +    die ("test setup: zone ($zoneid) not defined") if (!defined $zone);
> +    return $zone;
> +}
> +
> +sub get_vnet {
> +    my ($id) = @_;
> +    return eval { PVE::API2::Network::SDN::Vnets->read({vnet => $id}); };
> +}
> +# verify get_vnet
> +$t_invalid = get_vnet("invalid");
> +die("getting an invalid vnet must fail") if (!$@);
> +fail("getting an invalid vnet must fail") if (defined $t_invalid);
> +
> +sub create_vnet {
> +    my ($params) = @_;
> +    my $vnetid = $params->{vnet};
> +    PVE::API2::Network::SDN::Vnets->create($params);
> +
> +    my $vnet = get_vnet($vnetid);
> +    die ("test setup: vnet ($vnetid) not defined") if (!defined $vnet);
> +    return $vnet;
> +}
> +
> +sub get_subnet {
> +    my ($id) = @_;
> +    return eval { PVE::API2::Network::SDN::Subnets->read({subnet => $id}); };
> +}
> +# verify get_subnet
> +$t_invalid = get_subnet("invalid");
> +die("getting an invalid subnet must fail") if (!$@);
> +fail("getting an invalid subnet must fail") if (defined $t_invalid);
> +
> +sub create_subnet {
> +    my ($params) = @_;
> +    PVE::API2::Network::SDN::Subnets->create($params);
> +
> +    # warn Dumper($params).' ';
> +    # FIXME get_subnet  > HOW DO I GET THE ID?
> +}
> +
> +sub get_ipam_entries {
> +    return PVE::API2::Network::SDN::Ipams->ipamindex({ipam => "pve"});
> +}
> +
> +sub create_ip {
> +    my ($param) = @_;
> +    return PVE::API2::Network::SDN::Ips->ipcreate($param);
> +}
> +
> +sub run_test {
> +    my $test = shift;
> +    clear_test_state();
> +    $test->(@_);
> +}
> +
> +sub get_ips_from_mac {
> +    my ($mac) = @_;
> +    my $ipam_entries = get_ipam_entries();
> +    return grep { $_->{mac} eq $mac if defined $_->{mac} } $ipam_entries->@* if $ipam_entries;
> +}
> +
> +sub get_ip4 {
> +    my $ip4 = first { Net::IP::ip_is_ipv4($_->{ip}) } @_;
> +    return $ip4->{ip} if defined $ip4;
> +}
> +
> +sub get_ip6 {
> +    my $ip6 = first { Net::IP::ip_is_ipv6($_->{ip}) } @_;
> +    return $ip6->{ip} if defined $ip6;
> +}
> +
> +
> +# -------------- ACTUAL TESTS  -----------------------
> +
> +sub test_create_vnet_with_gateway {
> +    my $test_name = (split(/::/,(caller(0))[3]))[-1];
> +    my $zoneid = "TESTZONE";
> +    my $vnetid = "testvnet";
> +
> +    my $zone = create_zone({
> +	type => "simple",
> +	dhcp => "dnsmasq",
> +	ipam => "pve",
> +	zone => $zoneid,
> +    });
> +
> +    my $vnet = create_vnet({
> +	type => "vnet",
> +	zone => $zoneid,
> +	vnet => $vnetid,
> +    });
> +
> +    create_subnet({
> +	type => "subnet",
> +	vnet => $vnetid,
> +	subnet => "10.0.0.0/24",
> +	gateway => "10.0.0.1",
> +	'dhcp-range' => ["start-address=10.0.0.100,end-address=10.0.0.200"],
> +    });
> +
> +    my ($p) = first { $_->{gateway} == 1 } get_ipam_entries()->@*;
> +    ok ($p, "$test_name: Gateway IP was created in IPAM");
> +}
> +run_test(\&test_create_vnet_with_gateway);
> +
> +
> +sub test_without_subnet {
> +    my $test_name = (split(/::/,(caller(0))[3]))[-1];
> +
> +    my $zoneid = "TESTZONE";
> +    my $vnetid = "testvnet";
> +
> +    my $zone = create_zone({
> +	type => "simple",
> +	dhcp => "dnsmasq",
> +	ipam => "pve",
> +	zone => $zoneid,
> +    });
> +
> +    my $vnet = create_vnet({
> +	type => "vnet",
> +	zone => $zoneid,
> +	vnet => $vnetid,
> +    });
> +
> +    my $hostname = "testhostname";
> +    my $mac = "da:65:8f:18:9b:6f";
> +    my $vmid = "999";
> +
> +    eval {
> +	nic_join($vnetid, $mac, $hostname, $vmid);
> +    };
> +
> +    if ($@) {
> +	fail("$test_name: $@");
> +	return;
> +    }
> +
> +    my @ips = grep { $_->{mac} eq $mac } get_ipam_entries()->@*;
> +    my $num_ips = scalar @ips;
> +    is ($num_ips, 0, "$test_name: No IP allocated in IPAM");
> +}
> +run_test(\&test_without_subnet);
> +
> +
> +sub test_nic_join {
> +    my ($test_name, $subnets) = @_;
> +
> +    die "$test_name: we're expecting an array of subnets" if !$subnets;
> +    my $num_subnets = scalar $subnets->@*;
> +    die "$test_name: we're expecting an array of subnets. $num_subnets elements found" if ($num_subnets < 1);
> +    my $num_dhcp_ranges = scalar grep { $_->{'dhcp-range'} } $subnets->@*;
> +
> +    my $zoneid = "TESTZONE";
> +    my $vnetid = "testvnet";
> +
> +    my $zone = create_zone({
> +	type => "simple",
> +	dhcp => "dnsmasq",
> +	ipam => "pve",
> +	zone => $zoneid,
> +    });
> +
> +    my $vnet = create_vnet({
> +	type => "vnet",
> +	zone => $zoneid,
> +	vnet => $vnetid,
> +    });
> +
> +    foreach my $subnet ($subnets->@*) {
> +	$subnet->{type} = "subnet";
> +	$subnet->{vnet} = $vnetid;
> +	create_subnet($subnet);
> +    };
> +
> +    my $hostname = "testhostname";
> +    my $mac = "da:65:8f:18:9b:6f";
> +    my $vmid = "999";
> +
> +    eval {
> +	nic_join($vnetid, $mac, $hostname, $vmid);
> +    };
> +
> +    if ($@) {
> +	fail("$test_name: $@");
> +	return;
> +    }
> +
> +    my @ips = get_ips_from_mac($mac);
> +    my $num_ips = scalar @ips;
> +    is ($num_ips, $num_dhcp_ranges, "$test_name: Expecting $num_dhcp_ranges IPs, found $num_ips");
> +    ok ((all { ($_->{vnet} eq $vnetid && $_->{zone} eq $zoneid) } @ips),
> +	"$test_name: all IPs in correct vnet and zone"
> +    );
> +}
> +
> +run_test(
> +    \&test_nic_join,
> +    "nic_join IPv4 with dhcp",
> +    [{
> +	subnet => "10.0.0.0/24",
> +	gateway => "10.0.0.1",
> +	'dhcp-range' => ["start-address=10.0.0.100,end-address=10.0.0.200"],
> +    },
> +]);

These kinds of tests are what I meant above - the case above and the
ones following below could be put into an array of hashes like so:

my $nic_join_cases = [
    {
        desc = 'nic_join IPv4 with DHCP',
	subnets = [ 
	    {
	        subnet => '10.0.0.0/24',
		gateway => '10.0.0.1',
		'dhcp-range' => [
                    'start-address=10.0.0.100,end-address=10.0.0.200',
		] 
	    },
	],
    },
    // ...
];

Just to give an example.

> +
> +run_test(
> +    \&test_nic_join,
> +    "nic_join IPv4 no dhcp",
> +    [{
> +	subnet => "10.0.0.0/24",
> +	gateway => "10.0.0.1",
> +    },
> +]);
> +
> +run_test(
> +    \&test_nic_join,
> +    "nic_join IPv6 with dhcp",
> +    [{
> +	subnet => "8888::/64",
> +	gateway => "8888::1",
> +	'dhcp-range' => ["start-address=8888::100,end-address=8888::200"],
> +    },
> +]);
> +
> +run_test(
> +    \&test_nic_join,
> +    "nic_join IPv6 no dhcp",
> +    [{
> +	subnet => "8888::/64",
> +	gateway => "8888::1",
> +    },
> +]);
> +
> +run_test(
> +    \&test_nic_join,
> +    "nic_join IPv4+6 with dhcp",
> +    [{
> +	subnet => "10.0.0.0/24",
> +	gateway => "10.0.0.1",
> +	'dhcp-range' => ["start-address=10.0.0.100,end-address=10.0.0.200"],
> +    }, {
> +	subnet => "8888::/64",
> +	gateway => "8888::1",
> +	'dhcp-range' => ["start-address=8888::100,end-address=8888::200"],
> +    },
> +]);
> +
> +run_test(
> +    \&test_nic_join,
> +    "nic_join IPv4+6 no dhcp",
> +    [{
> +	subnet => "10.0.0.0/24",
> +	gateway => "10.0.0.1",
> +    }, {
> +	subnet => "8888::/64",
> +	gateway => "8888::1",
> +    },
> +]);
> +
> +run_test(
> +    \&test_nic_join,
> +    "nic_join IPv4 no DHCP, IPv6 with DHCP",
> +    [{
> +	subnet => "10.0.0.0/24",
> +	gateway => "10.0.0.1",
> +    }, {
> +	subnet => "8888::/64",
> +	gateway => "8888::1",
> +	'dhcp-range' => ["start-address=8888::100,end-address=8888::200"],
> +    },
> +]);
> +
> +run_test(
> +    \&test_nic_join,
> +    "nic_join IPv4 with DHCP, IPv6 no DHCP",
> +    [{
> +	subnet => "10.0.0.0/24",
> +	gateway => "10.0.0.1",
> +	'dhcp-range' => ["start-address=10.0.0.100,end-address=10.0.0.200"],
> +    }, {
> +	subnet => "8888::/64",
> +	gateway => "8888::1",
> +    },
> +]);
> +
> +
> +# -------------- nic_start
> +sub test_nic_start {
> +    my ($test_name, $subnets, $current_ip4, $current_ip6) = @_;
> +
> +    die "$test_name: we're expecting an array of subnets" if !$subnets;
> +    my $num_subnets = scalar $subnets->@*;
> +    die "$test_name: we're expecting an array of subnets. $num_subnets elements found" if ($num_subnets < 1);
> +
> +    # my $num_expected_ips = 0;
> +    # $num_expected_ips++ if $current_ip4;
> +    # $num_expected_ips++ if $current_ip6;
> +    # print "expecting ips: $num_expected_ips\n";
> +
> +    my $num_expected_ips = scalar grep { $_->{'dhcp-range'} } $subnets->@*;
> +    if (defined $current_ip4 && defined $current_ip6) {
> +        # if we already have an IP for a subnet without a dhcp-range (special case for a single test)
> +	$num_expected_ips = 2;
> +    }
> +
> +    my $zoneid = "TESTZONE";
> +    my $vnetid = "testvnet";
> +
> +    my $zone = create_zone({
> +	type => "simple",
> +	dhcp => "dnsmasq",
> +	ipam => "pve",
> +	zone => $zoneid,
> +    });
> +
> +    my $vnet = create_vnet({
> +	type => "vnet",
> +	zone => $zoneid,
> +	vnet => $vnetid,
> +    });
> +
> +    foreach my $subnet ($subnets->@*) {
> +	$subnet->{type} = "subnet";
> +	$subnet->{vnet} = $vnetid;
> +	create_subnet($subnet);
> +    };
> +
> +    my $hostname = "testhostname";
> +    my $mac = "da:65:8f:18:9b:6f";
> +    my $vmid = "999";
> +
> +    if ($current_ip4) {
> +	create_ip({
> +	    zone => $zoneid,
> +	    vnet => $vnetid,
> +	    mac => $mac,
> +	    ip => $current_ip4,
> +	});
> +    }
> +
> +    if ($current_ip6) {
> +	create_ip({
> +	    zone => $zoneid,
> +	    vnet => $vnetid,
> +	    mac => $mac,
> +	    ip => $current_ip6,
> +	});
> +    }
> +    my @current_ips = get_ips_from_mac($mac);
> +    is ( get_ip4(@current_ips), $current_ip4, "$test_name: setup current IPv4: $current_ip4" ) if defined $current_ip4;
> +    is ( get_ip6(@current_ips), $current_ip6, "$test_name: setup current IPv6: $current_ip6" ) if defined $current_ip6;
> +
> +    eval {
> +	nic_start($vnetid, $mac, $hostname, $vmid);
> +    };
> +
> +    if ($@) {
> +	fail("$test_name: $@");
> +	return;
> +    }
> +
> +    my @ips = get_ips_from_mac($mac);
> +    my $num_ips = scalar @ips;
> +    is ($num_ips, $num_expected_ips, "$test_name: Expecting $num_expected_ips IPs, found $num_ips");
> +    ok ((all { ($_->{vnet} eq $vnetid && $_->{zone} eq $zoneid) } @ips),
> +	"$test_name: all IPs in correct vnet and zone"
> +    );
> +
> +    is ( get_ip4(@ips), $current_ip4, "$test_name: still current IPv4: $current_ip4" ) if $current_ip4;
> +    is ( get_ip6(@ips), $current_ip6, "$test_name: still current IPv6: $current_ip6" ) if $current_ip6;
> +}
> +
> +run_test(
> +    \&test_nic_start,
> +    "nic_start no IP, IPv4 subnet with dhcp",
> +    [{
> +	subnet => "10.0.0.0/24",
> +	gateway => "10.0.0.1",
> +	'dhcp-range' => ["start-address=10.0.0.100,end-address=10.0.0.200"],
> +    },
> +]);
> +
> +run_test(
> +    \&test_nic_start,
> +    "nic_start already IP, IPv4 subnet with dhcp",
> +    [{
> +	subnet => "10.0.0.0/24",
> +	gateway => "10.0.0.1",
> +	'dhcp-range' => ["start-address=10.0.0.100,end-address=10.0.0.200"],
> +    }],
> +    "10.0.0.99"
> +);
> +
> +run_test(
> +    \&test_nic_start,
> +    "nic_start already IP, IPv6 subnet with dhcp",
> +    [{
> +	subnet => "8888::/64",
> +	gateway => "8888::1",
> +	'dhcp-range' => ["start-address=8888::100,end-address=8888::200"],
> +    }],
> +    undef,
> +    "8888::99"
> +);
> +
> +run_test(
> +    \&test_nic_start,
> +    "nic_start IP, IPv4+6 subnet with dhcp",
> +    [{
> +	subnet => "10.0.0.0/24",
> +	gateway => "10.0.0.1",
> +	'dhcp-range' => ["start-address=10.0.0.100,end-address=10.0.0.200"],
> +    }, {
> +	subnet => "8888::/64",
> +	gateway => "8888::1",
> +	'dhcp-range' => ["start-address=8888::100,end-address=8888::200"],
> +    },
> +]);
> +
> +run_test(
> +    \&test_nic_start,
> +    "nic_start already IPv4, IPv4+6 subnet with dhcp",
> +    [{
> +	subnet => "10.0.0.0/24",
> +	gateway => "10.0.0.1",
> +	'dhcp-range' => ["start-address=10.0.0.100,end-address=10.0.0.200"],
> +    }, {
> +	subnet => "8888::/64",
> +	gateway => "8888::1",
> +	'dhcp-range' => ["start-address=8888::100,end-address=8888::200"],
> +    }],
> +    "10.0.0.99"
> +);
> +
> +run_test(
> +    \&test_nic_start,
> +    "nic_start already IPv6, IPv4+6 subnet with dhcp",
> +    [{
> +	subnet => "10.0.0.0/24",
> +	gateway => "10.0.0.1",
> +	'dhcp-range' => ["start-address=10.0.0.100,end-address=10.0.0.200"],
> +    }, {
> +	subnet => "8888::/64",
> +	gateway => "8888::1",
> +	'dhcp-range' => ["start-address=8888::100,end-address=8888::200"],
> +    }],
> +    undef,
> +    "8888::99"
> +);
> +
> +run_test(
> +    \&test_nic_start,
> +    "nic_start already IPv4+6, IPv4+6 subnets with dhcp",
> +    [{
> +	subnet => "10.0.0.0/24",
> +	gateway => "10.0.0.1",
> +	'dhcp-range' => ["start-address=10.0.0.100,end-address=10.0.0.200"],
> +    }, {
> +	subnet => "8888::/64",
> +	gateway => "8888::1",
> +	'dhcp-range' => ["start-address=8888::100,end-address=8888::200"],
> +    }],
> +    "10.0.0.99",
> +    "8888::99"
> +);
> +
> +run_test(
> +    \&test_nic_start,
> +    "nic_start already IPv4+6, only IPv4 subnet with dhcp",
> +    [{
> +	subnet => "10.0.0.0/24",
> +	gateway => "10.0.0.1",
> +	'dhcp-range' => ["start-address=10.0.0.100,end-address=10.0.0.200"],
> +    }, {
> +	subnet => "8888::/64",
> +	gateway => "8888::1",
> +    }],
> +    "10.0.0.99",
> +    "8888::99"
> +);
> +
> +done_testing();

Re: [pve-devel] [PATCH pve-network 8/8] test(vnets): add test_vnets_blackbox

[Stefan Lendl]

"Max Carrara" <m.carrara@proxmox.com> writes:

> On Wed Jan 3, 2024 at 4:37 PM CET, Stefan Lendl wrote:
>> Add several tests for Vnets. State setup as well as testing results is
>> done only via the API to test on the API boundaries not not against the
>> internal state. Internal state is mocked to avoid requiring access to
>> system files or pmxcfs.
>>
>> Mocking is done by reading and writing to a hash that holds the entire
>> state of SDN. The state is reset after every test run.
>>
>> Testing is done via helper functions: nic_join and nic_start.
>> When a nic joins a Vnet, currently it always - and only - calls
>> add_next_free_cidr(). The same is true if a nic starts on Vnet, which
>> only calles add_dhcp_mapping.
>>
>> These test functions homogenize the parameter list in contrast to the
>> current calls to the current functions.  These functions should move to
>> Vnets.pm to be called from QemuServer and LXC!
>>
>> The run_test function takes a function pointer and passes the rest of
>> the arguments to the test functions after resetting the test state.
>> This allows fine-grained parameterization per-test directly in the code
>> instead of separated files that require the entire state to be passed
>> in.
>>
>> The tests setup the SDN by creating a simple zone and a simple vnet. The
>> nic_join and nic_start function is called with different subnet
>> configuration wiht and without a dhcp-range configured and with or
>> without an already present IP in the IPAM.
>
> I really like where this is going! Now that I've read through this
> patch, it's become clear why you factored so many calls to commands etc.
> into their own `sub`s.
>
> Since you mentioned that this is more of an RFC off-list, I get why
> there are a bunch of lines that are commented out at the moment. Those
> obviously shouldn't be committed later on.
>
>>
>> Several of the tests fail and uncovers bugs, that shall be fixed in
>> subsequent commits.
>
> Would be nice to perhaps also have those in the final series though ;)

Yes agreed will uncomment them in a follow-up.

>
> Another thing that stood out to me is that some cases could be
> declarative, e.g. the cases for `test_nic_join` and `test_nic_start`
> could be declared in an array for each. You could then just loop over
> the cases - that makes it easier to `plan` those cases later on.
>

I totally agree it would be nice to have it like that.
I tried to get it there but found unrolling the calls to be more
readable and making the test sub body simpler not requiring to loop in
the test or a setup sub.

If this approach would be refactored further with some Perl-magic™ this
would be nice but I chose this deliberatly for simplicity and readability.

> That being said, you could perhaps structure the whole script so that
> you call a `sub` named e.g. `setup` where you - well - set up all the
> required logic and perform checks for the necessary pre-conditions, then
> another `sub` that runs the tests (and optionally one that cleans things
> up if necessary).

Yes, agreed as well. Also tried that but chose a "simpler" version for
the first iteration.

I found that it is sometimes simpler to have dedicated test functions
for example if you have a dhcp-range instead of if-casing whether a a
property is present in the hash.

I will re-consider a dedicated setup sub for a follow-up.

>
> Though, please note that this is not a strict necessity from my side,
> just something I wanted to mention! I like the way you've written your
> tests a lot, it's just that I personally tend to prefer a more
> declarative approach. So, it's okay if you just leave the structure as
> it is right now, if you prefer it that way.
>
> There are some more comments inline that give a little more context
> regarding the above, but otherwise, LGTM - pretty good to see more
> testing to be done!
>

Thanks for the review. 

Re: [pve-devel] [PATCH pve-network 8/8] test(vnets): add test_vnets_blackbox

[Max Carrara]

On Mon Mar 18, 2024 at 3:14 PM CET, Stefan Lendl wrote:
> "Max Carrara" <m.carrara@proxmox.com> writes:
>
> > On Wed Jan 3, 2024 at 4:37 PM CET, Stefan Lendl wrote:
> >> Add several tests for Vnets. State setup as well as testing results is
> >> done only via the API to test on the API boundaries not not against the
> >> internal state. Internal state is mocked to avoid requiring access to
> >> system files or pmxcfs.
> >>
> >> Mocking is done by reading and writing to a hash that holds the entire
> >> state of SDN. The state is reset after every test run.
> >>
> >> Testing is done via helper functions: nic_join and nic_start.
> >> When a nic joins a Vnet, currently it always - and only - calls
> >> add_next_free_cidr(). The same is true if a nic starts on Vnet, which
> >> only calles add_dhcp_mapping.
> >>
> >> These test functions homogenize the parameter list in contrast to the
> >> current calls to the current functions.  These functions should move to
> >> Vnets.pm to be called from QemuServer and LXC!
> >>
> >> The run_test function takes a function pointer and passes the rest of
> >> the arguments to the test functions after resetting the test state.
> >> This allows fine-grained parameterization per-test directly in the code
> >> instead of separated files that require the entire state to be passed
> >> in.
> >>
> >> The tests setup the SDN by creating a simple zone and a simple vnet. The
> >> nic_join and nic_start function is called with different subnet
> >> configuration wiht and without a dhcp-range configured and with or
> >> without an already present IP in the IPAM.
> >
> > I really like where this is going! Now that I've read through this
> > patch, it's become clear why you factored so many calls to commands etc.
> > into their own `sub`s.
> >
> > Since you mentioned that this is more of an RFC off-list, I get why
> > there are a bunch of lines that are commented out at the moment. Those
> > obviously shouldn't be committed later on.
> >
> >>
> >> Several of the tests fail and uncovers bugs, that shall be fixed in
> >> subsequent commits.
> >
> > Would be nice to perhaps also have those in the final series though ;)
>
> Yes agreed will uncomment them in a follow-up.
>
> >
> > Another thing that stood out to me is that some cases could be
> > declarative, e.g. the cases for `test_nic_join` and `test_nic_start`
> > could be declared in an array for each. You could then just loop over
> > the cases - that makes it easier to `plan` those cases later on.
> >
>
> I totally agree it would be nice to have it like that.
> I tried to get it there but found unrolling the calls to be more
> readable and making the test sub body simpler not requiring to loop in
> the test or a setup sub.
>
> If this approach would be refactored further with some Perl-magic™ this
> would be nice but I chose this deliberatly for simplicity and readability.

Very fair! Then it's best to just keep it as it is, in that case.

>
> > That being said, you could perhaps structure the whole script so that
> > you call a `sub` named e.g. `setup` where you - well - set up all the
> > required logic and perform checks for the necessary pre-conditions, then
> > another `sub` that runs the tests (and optionally one that cleans things
> > up if necessary).
>
> Yes, agreed as well. Also tried that but chose a "simpler" version for
> the first iteration.
>
> I found that it is sometimes simpler to have dedicated test functions
> for example if you have a dhcp-range instead of if-casing whether a a
> property is present in the hash.

I agree completely - it was more of an idea / nice-to-have. FWIW, if you
want to go down that route, you can of course also save references to
the `sub`s being run in the array of cases. Alternatively, it's also
totally fine to have multiple arrays of cases for each specific
function.

That being said, if you just want to keep it as it is, that's also
totally fine by me. Should the file grow, such a small refactor most
likely won't cost that much time.

>
> I will re-consider a dedicated setup sub for a follow-up.
>
> >
> > Though, please note that this is not a strict necessity from my side,
> > just something I wanted to mention! I like the way you've written your
> > tests a lot, it's just that I personally tend to prefer a more
> > declarative approach. So, it's okay if you just leave the structure as
> > it is right now, if you prefer it that way.
> >
> > There are some more comments inline that give a little more context
> > regarding the above, but otherwise, LGTM - pretty good to see more
> > testing to be done!
> >
>
> Thanks for the review. 

You're welcome!

Re: [pve-devel] [PATCH pve-network 0/8] SDN Vnet blackbox testing

[Max Carrara]

On Wed Jan 3, 2024 at 4:37 PM CET, Stefan Lendl wrote:
> Add several tests for Vnets. State setup as well as testing results is
> done only via the API to test on the API boundaries not not against the
> internal state. Internal state is mocked to avoid requiring access to
> system files or pmxcfs.
>
> Tests validate the events of a nic joining a Vnet or a nic staring on a vnet
> with different subnet configurations.
> Further descriptions in the commit.

I really like this! I'm always a fan of more testing being done.

There are some things I mentioned in patch 8, but I overall like this
series a lot.

>
> Stefan Lendl (8):
>   refactor(sdn): extract cfs_read_file(datacenter.cfg)
>   refactor(dnsmasq): extract systemctl_service function
>   refactor(dnsmasq): extract ethers_file function
>   refactor(dnsmasq): extract update_lease function
>   refactor(controllers): extract read_etc_network_interfaces
>   refactor(evpn): extract read_local_frr_config
>   refactor(api): extract create_etc_interfaces_sdn_dir

The naming here could be a little different though; I think you can just
skip the `refactor()` part, we don't really use that anywhere AFAIK.

>   test(vnets): add test_vnets_blackbox

Also, for this message you could just use "test: vnets: ..." instead.

See also: https://pve.proxmox.com/wiki/Developer_Documentation#Commits_and_Commit_Messages

>
>  src/PVE/API2/Network/SDN/Zones.pm             |   6 +-
>  src/PVE/Network/SDN/Controllers.pm            |  16 +-
>  src/PVE/Network/SDN/Controllers/EvpnPlugin.pm |  10 +-
>  src/PVE/Network/SDN/Dhcp/Dnsmasq.pm           |  47 +-
>  src/PVE/Network/SDN/Zones/EvpnPlugin.pm       |   3 +-
>  src/PVE/Network/SDN/Zones/Plugin.pm           |   5 +
>  src/PVE/Network/SDN/Zones/SimplePlugin.pm     |   2 +-
>  src/test/Makefile                             |   5 +-
>  src/test/run_test_vnets_blackbox.pl           | 797 ++++++++++++++++++
>  9 files changed, 863 insertions(+), 28 deletions(-)
>  create mode 100755 src/test/run_test_vnets_blackbox.pl

Re: [pve-devel] [PATCH pve-network 0/8] SDN Vnet blackbox testing

[Stefan Lendl]

sent v2 incorporating the changes and adding additional tests.