[pve-devel] [PATCH v2 pve-network 0/5] SDN tests in sbuild

[pve-devel] [PATCH v2 pve-network 0/5] SDN tests in sbuild

[Stefan Lendl]

Extract and mock functions that otherwise access system files which is not
possible in a clean sbuild environment.
Namely /etc/network/interfaces as well as /etc/frr/frr.config.local
Disabling DNS tests

Changes v1 -> v2:
* Disabled DNS tests because they fail


Stefan Lendl (5):
  refactor(controllers): extract read_etc_network_interfaces
  refactor(evpn): extract read_local_frr_config
  tests: mocking more functions to avoid system access
  tests: disable failing DNS tests
  tests: run tests in sbuild

 src/Makefile                                  |  2 +-
 src/PVE/Network/SDN/Controllers.pm            | 16 ++++++---
 src/PVE/Network/SDN/Controllers/EvpnPlugin.pm | 10 ++++--
 src/test/Makefile                             |  2 +-
 src/test/run_test_zones.pl                    | 36 ++++++++++++++++++-
 5 files changed, 56 insertions(+), 10 deletions(-)

-- 
2.43.0

[pve-devel] [PATCH v2 pve-network 1/5] 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.43.0

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

[Max Carrara]

Should just be "controllers: [...]" as per our commit guidelines [0],
but this can honestly be changed while applying.

Otherwise LGTM!

[0]: https://pve.proxmox.com/wiki/Developer_Documentation#Commits_and_Commit_Messages

On Thu Mar 7, 2024 at 4:13 PM CET, Stefan Lendl wrote:
> 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 = {};

[pve-devel] [PATCH v2 pve-network 2/5] 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 c2fdf88..836a689 100644
--- a/src/PVE/Network/SDN/Controllers/EvpnPlugin.pm
+++ b/src/PVE/Network/SDN/Controllers/EvpnPlugin.pm
@@ -487,6 +487,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) = @_;
 
@@ -500,8 +506,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.43.0

Re: [pve-devel] [PATCH v2 pve-network 2/5] refactor(evpn): extract read_local_frr_config

[Max Carrara]

Should just be "evpn: [...]" as per our commit guidelines [0], but this
can honestly be changed while applying.

Otherwise LGTM!

[0]: https://pve.proxmox.com/wiki/Developer_Documentation#Commits_and_Commit_Messages

On Thu Mar 7, 2024 at 4:13 PM CET, Stefan Lendl wrote:
> 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 c2fdf88..836a689 100644
> --- a/src/PVE/Network/SDN/Controllers/EvpnPlugin.pm
> +++ b/src/PVE/Network/SDN/Controllers/EvpnPlugin.pm
> @@ -487,6 +487,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) = @_;
>  
> @@ -500,8 +506,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);
>      }
>  

[pve-devel] [PATCH v2 pve-network 3/5] tests: mocking more functions to avoid system access

[Stefan Lendl]

previously extracted functions are now mocked in the zone tests

Signed-off-by: Stefan Lendl <s.lendl@proxmox.com>
---
 src/test/run_test_zones.pl | 36 +++++++++++++++++++++++++++++++++++-
 1 file changed, 35 insertions(+), 1 deletion(-)

diff --git a/src/test/run_test_zones.pl b/src/test/run_test_zones.pl
index ce8edc2..2d9be88 100755
--- a/src/test/run_test_zones.pl
+++ b/src/test/run_test_zones.pl
@@ -14,6 +14,10 @@ use PVE::Network::SDN::Zones;
 use PVE::Network::SDN::Controllers;
 use PVE::INotify;
 
+use Data::Dumper qw(Dumper);
+$Data::Dumper::Sortkeys = 1;
+$Data::Dumper::Indent = 1;
+
 sub read_sdn_config {
     my ($file) = @_;
 
@@ -29,7 +33,6 @@ sub read_sdn_config {
     return $sdn_config;
 }
 
-
 my @tests = grep { -d } glob './zones/*/*';
 
 foreach my $test (@tests) {
@@ -47,8 +50,20 @@ foreach my $test (@tests) {
 	    return 'localhost';
 	},
 	read_file => sub {
+	    # HACK this assumes we are always calling PVE::INotify::read_file('interfaces');
 	    return $interfaces_config;
 	},
+	read_etc_network_interfaces => sub {
+	    return $interfaces_config;
+	},
+    );
+
+    my $mocked_pve_sdn_controllers;
+    $mocked_pve_sdn_controllers = Test::MockModule->new('PVE::Network::SDN::Controllers');
+    $mocked_pve_sdn_controllers->mock(
+	read_etc_network_interfaces => sub {
+	    return $interfaces_config;
+	}
     );
 
     my $pve_sdn_subnets;
@@ -88,6 +103,25 @@ foreach my $test (@tests) {
 	},
     );
 
+    my ($first_plugin) = %{$sdn_config->{controllers}->{ids}} if defined $sdn_config->{controllers};
+    if ($first_plugin) {
+	my $controller_plugin = PVE::Network::SDN::Controllers::Plugin->lookup(
+	    $sdn_config->{controllers}->{ids}->{$first_plugin}->{type}
+	);
+	my $mocked_controller_plugin = Test::MockModule->new($controller_plugin);
+	$mocked_controller_plugin->mock(
+	    write_controller_config => sub {
+		return;
+	    },
+	    reload_controller => sub {
+		return;
+	    },
+	    read_local_frr_config => sub {
+		return;
+	    },
+	);
+    }
+
     my $name = $test;
     my $expected = read_file("./$test/expected_sdn_interfaces");
 
-- 
2.43.0

Re: [pve-devel] [PATCH v2 pve-network 3/5] tests: mocking more functions to avoid system access

[Max Carrara]

On Thu Mar 7, 2024 at 4:13 PM CET, Stefan Lendl wrote:
> previously extracted functions are now mocked in the zone tests
>
> Signed-off-by: Stefan Lendl <s.lendl@proxmox.com>
> ---
>  src/test/run_test_zones.pl | 36 +++++++++++++++++++++++++++++++++++-
>  1 file changed, 35 insertions(+), 1 deletion(-)
>
> diff --git a/src/test/run_test_zones.pl b/src/test/run_test_zones.pl
> index ce8edc2..2d9be88 100755
> --- a/src/test/run_test_zones.pl
> +++ b/src/test/run_test_zones.pl
> @@ -14,6 +14,10 @@ use PVE::Network::SDN::Zones;
>  use PVE::Network::SDN::Controllers;
>  use PVE::INotify;
>  
> +use Data::Dumper qw(Dumper);
> +$Data::Dumper::Sortkeys = 1;
> +$Data::Dumper::Indent = 1;
> +
>  sub read_sdn_config {
>      my ($file) = @_;
>  
> @@ -29,7 +33,6 @@ sub read_sdn_config {
>      return $sdn_config;
>  }
>  
> -

Somehow, this deletion makes this patch fail to apply. As in, the
rejected hunk is literally the following:

---

diff a/src/test/run_test_zones.pl b/src/test/run_test_zones.pl	(rejected hunks)
@@ -29,7 +33,6 @@ sub read_sdn_config {
     return $sdn_config;
 }
 
-
 my @tests = grep { -d } glob './zones/*/*';
 
 foreach my $test (@tests) {

---

... I don't know what happened here, but this can still just be applied
otherwise - in my case, I resolved this via the following:

* `git am --reject`
* `git add src/test/run_test_zones.pl`
* `git am --continue`

Then it just applies. When in doubt, one may also check
`src/test/run_test_zones.pl.rej` to see what was rejected - should be
just the hunk I pasted above.

IMHO this can also just be done when applying the series; while
*somewhat* inconvenient, I don't really see this as a blocker or
anything.

So, LGTM!

>  my @tests = grep { -d } glob './zones/*/*';
>  
>  foreach my $test (@tests) {
> @@ -47,8 +50,20 @@ foreach my $test (@tests) {
>  	    return 'localhost';
>  	},
>  	read_file => sub {
> +	    # HACK this assumes we are always calling PVE::INotify::read_file('interfaces');
>  	    return $interfaces_config;
>  	},
> +	read_etc_network_interfaces => sub {
> +	    return $interfaces_config;
> +	},
> +    );
> +
> +    my $mocked_pve_sdn_controllers;
> +    $mocked_pve_sdn_controllers = Test::MockModule->new('PVE::Network::SDN::Controllers');
> +    $mocked_pve_sdn_controllers->mock(
> +	read_etc_network_interfaces => sub {
> +	    return $interfaces_config;
> +	}
>      );
>  
>      my $pve_sdn_subnets;
> @@ -88,6 +103,25 @@ foreach my $test (@tests) {
>  	},
>      );
>  
> +    my ($first_plugin) = %{$sdn_config->{controllers}->{ids}} if defined $sdn_config->{controllers};
> +    if ($first_plugin) {
> +	my $controller_plugin = PVE::Network::SDN::Controllers::Plugin->lookup(
> +	    $sdn_config->{controllers}->{ids}->{$first_plugin}->{type}
> +	);
> +	my $mocked_controller_plugin = Test::MockModule->new($controller_plugin);
> +	$mocked_controller_plugin->mock(
> +	    write_controller_config => sub {
> +		return;
> +	    },
> +	    reload_controller => sub {
> +		return;
> +	    },
> +	    read_local_frr_config => sub {
> +		return;
> +	    },
> +	);
> +    }
> +
>      my $name = $test;
>      my $expected = read_file("./$test/expected_sdn_interfaces");
>  

[pve-devel] [PATCH v2 pve-network 4/5] tests: disable failing DNS tests

[Stefan Lendl]

Signed-off-by: Stefan Lendl <s.lendl@proxmox.com>
---
 src/test/Makefile | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/src/test/Makefile b/src/test/Makefile
index eb59d5f..db70c89 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_subnets
 
 test_zones: run_test_zones.pl
 	./run_test_zones.pl
-- 
2.43.0

Re: [pve-devel] [PATCH v2 pve-network 4/5] tests: disable failing DNS tests

[Max Carrara]

On Thu Mar 7, 2024 at 4:13 PM CET, Stefan Lendl wrote:
> Signed-off-by: Stefan Lendl <s.lendl@proxmox.com>
> ---
>  src/test/Makefile | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/src/test/Makefile b/src/test/Makefile
> index eb59d5f..db70c89 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_subnets

Interestingly enough, these don't fail for me at all... do you know what
tests failed for you in the first place, perhaps? All of them?

Otherwise, I'd suggest just dropping this patch if the tests run fine
now - no need to disable them after all then.

>  
>  test_zones: run_test_zones.pl
>  	./run_test_zones.pl

[pve-devel] [PATCH v2 pve-network 5/5] tests: run tests in sbuild

[Stefan Lendl]

Signed-off-by: Stefan Lendl <s.lendl@proxmox.com>
---
 src/Makefile | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/src/Makefile b/src/Makefile
index c9dee4c..c4056b4 100644
--- a/src/Makefile
+++ b/src/Makefile
@@ -10,7 +10,7 @@ clean:
 
 .PHONY: test
 test:
-	[ -e /run/lock/sbuild ] || $(MAKE) -C $@
+	$(MAKE) -C $@
 
 .PHONY: install
 install:
-- 
2.43.0

Re: [pve-devel] [PATCH v2 pve-network 0/5] SDN tests in sbuild

[Max Carrara]

On Thu Mar 7, 2024 at 4:13 PM CET, Stefan Lendl wrote:
> Extract and mock functions that otherwise access system files which is not
> possible in a clean sbuild environment.
> Namely /etc/network/interfaces as well as /etc/frr/frr.config.local
> Disabling DNS tests
>
> Changes v1 -> v2:
> * Disabled DNS tests because they fail
>

Looked through it all and ran the tests manually and also indirectly via
builds - looks pretty good to me! Always fond of making things more
testable! :)

There are some minor comments inline - but one important thing: The DNS
tests are passing for me. Maybe double-check if they pass for you now
too; if that's the case, patch #4 can just be dropped, as mentioned in
my comment.

Otherwise:

Reviewed-by: Max Carrara <m.carrara@proxmox.com>
Tested-by: Max Carrara <m.carrara@proxmox.com>

>
> Stefan Lendl (5):
>   refactor(controllers): extract read_etc_network_interfaces
>   refactor(evpn): extract read_local_frr_config
>   tests: mocking more functions to avoid system access
>   tests: disable failing DNS tests
>   tests: run tests in sbuild
>
>  src/Makefile                                  |  2 +-
>  src/PVE/Network/SDN/Controllers.pm            | 16 ++++++---
>  src/PVE/Network/SDN/Controllers/EvpnPlugin.pm | 10 ++++--
>  src/test/Makefile                             |  2 +-
>  src/test/run_test_zones.pl                    | 36 ++++++++++++++++++-
>  5 files changed, 56 insertions(+), 10 deletions(-)

Re: [pve-devel] [PATCH v2 pve-network 0/5] SDN tests in sbuild

[Stefan Lendl]

Thanks for the review.

I rebased the commits for v3
I re-enabled DNS. I do not recall why it failed on my last rebase
attempt.