[pve-devel] [PATCH SERIES common/firewall/qemu-server/container 0/1] Cleanup use of PVE::Firewall

[pve-devel] [PATCH SERIES common/firewall/qemu-server/container 0/1] Cleanup use of PVE::Firewall

[Alexandre Derumier]

Hi,

I was doing some profiling of qm && pct command,
and I have found around 100ms cpu time just for the
use of PVE::Firewall in qemu-server && pve-container
for 2 small helpers. (clone_vmfw_conf && remove_vmfw_conf)

as PVE::Firewall require QemuServer && LXC,
we are loading full lxc in qm too.

Also, LXC have some include of PVE::Firewall
in different packages, not used,
but it was magically including QemuServer,
which including the json schema "pve-targetstorage",
used by lxc too.

I have moved this schema to main jsonschema in pve-common


Alexandre Derumier (1):
  schema: add pve-targetstorage (moved from qemu-server)

 src/PVE/JSONSchema.pm | 7 +++++++
 1 file changed, 7 insertions(+)

-- 

Alexandre Derumier (1):
  move clone_vmfw_conf && remove_vmfw_conf to a Helpers

 src/PVE/Firewall.pm         | 25 ----------------------
 src/PVE/Firewall/Helpers.pm | 41 +++++++++++++++++++++++++++++++++++++
 src/PVE/Firewall/Makefile   | 12 +++++++++++
 src/PVE/Makefile            |  2 ++
 4 files changed, 55 insertions(+), 25 deletions(-)
 create mode 100644 src/PVE/Firewall/Helpers.pm
 create mode 100644 src/PVE/Firewall/Makefile

-- 

Alexandre Derumier (2):
  api2: use firewall helpers
  qemu-server: remove json schema pve-targetstorage (moved to
    pve-common)

 PVE/API2/Qemu.pm  | 8 ++++----
 PVE/QemuServer.pm | 7 -------
 2 files changed, 4 insertions(+), 11 deletions(-)

-- 

Alexandre Derumier (2):
  remove unused use PVE::Firewall
  api2 : use firewall helpers

 src/PVE/API2/LXC.pm          | 10 +++++-----
 src/PVE/API2/LXC/Config.pm   |  1 -
 src/PVE/API2/LXC/Snapshot.pm |  1 -
 src/PVE/API2/LXC/Status.pm   |  1 -
 4 files changed, 5 insertions(+), 8 deletions(-)

-- 
2.30.2

[pve-devel] [PATCH qemu-server 1/2] api2: use firewall helpers

[Alexandre Derumier]

Signed-off-by: Alexandre Derumier <aderumier@odiso.com>
---
 PVE/API2/Qemu.pm | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/PVE/API2/Qemu.pm b/PVE/API2/Qemu.pm
index 99b426e..fe1465e 100644
--- a/PVE/API2/Qemu.pm
+++ b/PVE/API2/Qemu.pm
@@ -31,7 +31,7 @@ use PVE::RPCEnvironment;
 use PVE::AccessControl;
 use PVE::INotify;
 use PVE::Network;
-use PVE::Firewall;
+use PVE::Firewall::Helpers qw(remove_vmfw_conf clone_vmfw_conf);
 use PVE::API2::Firewall::VM;
 use PVE::API2::Qemu::Agent;
 use PVE::VZDump::Plugin;
@@ -1911,7 +1911,7 @@ __PACKAGE__->register_method({
 		);
 
 		PVE::AccessControl::remove_vm_access($vmid);
-		PVE::Firewall::remove_vmfw_conf($vmid);
+		remove_vmfw_conf($vmid);
 		if ($param->{purge}) {
 		    print "purging VM $vmid from related configurations..\n";
 		    PVE::ReplicationConfig::remove_vmid_jobs($vmid);
@@ -3416,7 +3416,7 @@ __PACKAGE__->register_method({
 	    # FIXME use PVE::QemuConfig->create_and_lock_config and adapt code
 	    PVE::Tools::file_set_contents($conffile, "# qmclone temporary file\nlock: clone\n");
 
-	    PVE::Firewall::clone_vmfw_conf($vmid, $newid);
+	    clone_vmfw_conf($vmid, $newid);
 
 	    my $newvollist = [];
 	    my $jobs = {};
@@ -3512,7 +3512,7 @@ __PACKAGE__->register_method({
 		    warn $@ if $@;
 		}
 
-		PVE::Firewall::remove_vmfw_conf($newid);
+		remove_vmfw_conf($newid);
 
 		unlink $conffile; # avoid races -> last thing before die
 
-- 
2.30.2

[pve-devel] [PATCH pve-firewall 1/1] move clone_vmfw_conf && remove_vmfw_conf to a Helpers

[Alexandre Derumier]

Signed-off-by: Alexandre Derumier <aderumier@odiso.com>
---
 src/PVE/Firewall.pm         | 25 ----------------------
 src/PVE/Firewall/Helpers.pm | 41 +++++++++++++++++++++++++++++++++++++
 src/PVE/Firewall/Makefile   | 12 +++++++++++
 src/PVE/Makefile            |  2 ++
 4 files changed, 55 insertions(+), 25 deletions(-)
 create mode 100644 src/PVE/Firewall/Helpers.pm
 create mode 100644 src/PVE/Firewall/Makefile

diff --git a/src/PVE/Firewall.pm b/src/PVE/Firewall.pm
index 71746d2..e6cf4cd 100644
--- a/src/PVE/Firewall.pm
+++ b/src/PVE/Firewall.pm
@@ -3340,31 +3340,6 @@ sub save_vmfw_conf {
     }
 }
 
-sub remove_vmfw_conf {
-    my ($vmid) = @_;
-
-    my $vmfw_conffile = "$pvefw_conf_dir/$vmid.fw";
-
-    unlink $vmfw_conffile;
-}
-
-sub clone_vmfw_conf {
-    my ($vmid, $newid) = @_;
-
-    my $sourcevm_conffile = "$pvefw_conf_dir/$vmid.fw";
-    my $clonevm_conffile = "$pvefw_conf_dir/$newid.fw";
-
-    lock_vmfw_conf($newid, 10, sub {
-	if (-f $clonevm_conffile) {
-	    unlink $clonevm_conffile;
-	}
-	if (-f $sourcevm_conffile) {
-	    my $data = PVE::Tools::file_get_contents($sourcevm_conffile);
-	    PVE::Tools::file_set_contents($clonevm_conffile, $data);
-	}
-    });
-}
-
 sub read_vm_firewall_configs {
     my ($cluster_conf, $vmdata, $dir) = @_;
 
diff --git a/src/PVE/Firewall/Helpers.pm b/src/PVE/Firewall/Helpers.pm
new file mode 100644
index 0000000..3112ebc
--- /dev/null
+++ b/src/PVE/Firewall/Helpers.pm
@@ -0,0 +1,41 @@
+package PVE::Firewall::Helpers;
+
+use strict;
+use warnings;
+
+use PVE::Tools qw(file_get_contents file_set_contents);
+
+use base 'Exporter';
+our @EXPORT_OK = qw(
+remove_vmfw_conf
+clone_vmfw_conf
+);
+
+my $pvefw_conf_dir = "/etc/pve/firewall";
+
+sub remove_vmfw_conf {
+    my ($vmid) = @_;
+
+    my $vmfw_conffile = "$pvefw_conf_dir/$vmid.fw";
+
+    unlink $vmfw_conffile;
+}
+
+sub clone_vmfw_conf {
+    my ($vmid, $newid) = @_;
+
+    my $sourcevm_conffile = "$pvefw_conf_dir/$vmid.fw";
+    my $clonevm_conffile = "$pvefw_conf_dir/$newid.fw";
+
+    lock_vmfw_conf($newid, 10, sub {
+	if (-f $clonevm_conffile) {
+	    unlink $clonevm_conffile;
+	}
+	if (-f $sourcevm_conffile) {
+	    my $data = file_get_contents($sourcevm_conffile);
+	    file_set_contents($clonevm_conffile, $data);
+	}
+    });
+}
+
+1;
\ No newline at end of file
diff --git a/src/PVE/Firewall/Makefile b/src/PVE/Firewall/Makefile
new file mode 100644
index 0000000..1707cb5
--- /dev/null
+++ b/src/PVE/Firewall/Makefile
@@ -0,0 +1,12 @@
+DESTDIR=
+PREFIX=/usr
+PERLDIR=${DESTDIR}/${PREFIX}/share/perl5
+
+SOURCES=Helpers.pm
+
+.PHONY: install
+install: ${SOURCES}
+	install -d -m 0755 ${PERLDIR}/PVE/Firewall
+	for i in ${SOURCES}; do install -D -m 0644 $$i ${PERLDIR}/PVE/Firewall/$$i; done
+
+clean:
diff --git a/src/PVE/Makefile b/src/PVE/Makefile
index 9fdfd9b..4edde9e 100644
--- a/src/PVE/Makefile
+++ b/src/PVE/Makefile
@@ -14,9 +14,11 @@ install:
 	for i in ${LIB_SOURCES}; do install -D -m 0644 $$i ${PERLDIR}/PVE/$$i; done
 	make -C API2 install
 	make -C Service install
+	make -C Firewall install
 
 .PHONY: clean
 clean:
 	rm -rf *~
 	make -C API2 clean
 	make -C Service clean
+	make -C Firewall clean
-- 
2.30.2

[pve-devel] [PATCH pve-container 1/2] remove unused use PVE::Firewall

[Alexandre Derumier]

note: this was including the pve-targetstorage schema
from qemu-server.

this need pve-common patch to move the schema from qemu-server
to pve-common

Signed-off-by: Alexandre Derumier <aderumier@odiso.com>
---
 src/PVE/API2/LXC/Config.pm   | 1 -
 src/PVE/API2/LXC/Snapshot.pm | 1 -
 src/PVE/API2/LXC/Status.pm   | 1 -
 3 files changed, 3 deletions(-)

diff --git a/src/PVE/API2/LXC/Config.pm b/src/PVE/API2/LXC/Config.pm
index 1fec048..a0807e0 100644
--- a/src/PVE/API2/LXC/Config.pm
+++ b/src/PVE/API2/LXC/Config.pm
@@ -9,7 +9,6 @@ use PVE::Exception qw(raise raise_param_exc);
 use PVE::INotify;
 use PVE::Cluster qw(cfs_read_file);
 use PVE::AccessControl;
-use PVE::Firewall;
 use PVE::Storage;
 use PVE::RESTHandler;
 use PVE::RPCEnvironment;
diff --git a/src/PVE/API2/LXC/Snapshot.pm b/src/PVE/API2/LXC/Snapshot.pm
index 4be16ad..385380c 100644
--- a/src/PVE/API2/LXC/Snapshot.pm
+++ b/src/PVE/API2/LXC/Snapshot.pm
@@ -9,7 +9,6 @@ use PVE::Exception qw(raise raise_param_exc);
 use PVE::INotify;
 use PVE::Cluster qw(cfs_read_file);
 use PVE::AccessControl;
-use PVE::Firewall;
 use PVE::GuestHelpers;
 use PVE::Storage;
 use PVE::RESTHandler;
diff --git a/src/PVE/API2/LXC/Status.pm b/src/PVE/API2/LXC/Status.pm
index f7e3128..b81e203 100644
--- a/src/PVE/API2/LXC/Status.pm
+++ b/src/PVE/API2/LXC/Status.pm
@@ -9,7 +9,6 @@ use PVE::Exception qw(raise raise_param_exc);
 use PVE::INotify;
 use PVE::Cluster qw(cfs_read_file);
 use PVE::AccessControl;
-use PVE::Firewall;
 use PVE::Storage;
 use PVE::RESTHandler;
 use PVE::RPCEnvironment;
-- 
2.30.2

[pve-devel] [PATCH pve-common 1/1] schema: add pve-targetstorage (moved from qemu-server)

[Alexandre Derumier]

Signed-off-by: Alexandre Derumier <aderumier@odiso.com>
---
 src/PVE/JSONSchema.pm | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/src/PVE/JSONSchema.pm b/src/PVE/JSONSchema.pm
index ab718f3..0db363c 100644
--- a/src/PVE/JSONSchema.pm
+++ b/src/PVE/JSONSchema.pm
@@ -89,6 +89,13 @@ register_standard_option('pve-bridge-id', {
     format_description => 'bridge',
 });
 
+register_standard_option('pve-targetstorage', {
+    description => "Mapping from source to target storages. Providing only a single storage ID maps all source storages to that storage. Providing the special value '1' will map each source storage to itself.",
+    type => 'string',
+    format => 'storage-pair-list',
+    optional => 1,
+});
+
 register_standard_option('pve-config-digest', {
     description => 'Prevent changes if current configuration file has different SHA1 digest. This can be used to prevent concurrent modifications.',
     type => 'string',
-- 
2.30.2

[pve-devel] [PATCH pve-container 2/2] api2 : use firewall helpers

[Alexandre Derumier]

Signed-off-by: Alexandre Derumier <aderumier@odiso.com>
---
 src/PVE/API2/LXC.pm | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/src/PVE/API2/LXC.pm b/src/PVE/API2/LXC.pm
index e909cb0..b15e327 100644
--- a/src/PVE/API2/LXC.pm
+++ b/src/PVE/API2/LXC.pm
@@ -11,7 +11,7 @@ use PVE::Cluster qw(cfs_read_file);
 use PVE::RRD;
 use PVE::DataCenterConfig;
 use PVE::AccessControl;
-use PVE::Firewall;
+use PVE::Firewall::Helpers qw(remove_vmfw_conf clone_vmfw_conf);
 use PVE::Storage;
 use PVE::RESTHandler;
 use PVE::RPCEnvironment;
@@ -761,7 +761,7 @@ __PACKAGE__->register_method({
 	    );
 
 	    PVE::AccessControl::remove_vm_access($vmid);
-	    PVE::Firewall::remove_vmfw_conf($vmid);
+	    remove_vmfw_conf($vmid);
 	    if ($param->{purge}) {
 	        print "purging CT $vmid from related configurations..\n";
 		PVE::ReplicationConfig::remove_vmid_jobs($vmid);
@@ -1462,7 +1462,7 @@ __PACKAGE__->register_method({
 	my $running;
 
 	PVE::LXC::Config->create_and_lock_config($newid, 0);
-	PVE::Firewall::clone_vmfw_conf($vmid, $newid);
+	clone_vmfw_conf($vmid, $newid);
 
 	my $lock_and_reload = sub {
 	    my ($vmid, $code) = @_;
@@ -1569,7 +1569,7 @@ __PACKAGE__->register_method({
 	    eval {
 		$lock_and_reload->($newid, sub {
 		    PVE::LXC::Config->destroy_config($newid);
-		    PVE::Firewall::remove_vmfw_conf($newid);
+		    remove_vmfw_conf($newid);
 		});
 	    };
 	    warn "Failed to remove target CT config - $@\n" if $@;
@@ -1661,7 +1661,7 @@ __PACKAGE__->register_method({
 		eval {
 		    $lock_and_reload->($newid, sub {
 			PVE::LXC::Config->destroy_config($newid);
-			PVE::Firewall::remove_vmfw_conf($newid);
+			remove_vmfw_conf($newid);
 		    });
 		};
 		warn "Failed to remove target CT config - $@\n" if $@;
-- 
2.30.2

[pve-devel] [PATCH qemu-server 2/2] qemu-server: remove json schema pve-targetstorage (moved to pve-common)

[Alexandre Derumier]

This schema is also used by pve-container,
and currently it's working with using use PVE::Firewall

Signed-off-by: Alexandre Derumier <aderumier@odiso.com>
---
 PVE/QemuServer.pm | 7 -------
 1 file changed, 7 deletions(-)

diff --git a/PVE/QemuServer.pm b/PVE/QemuServer.pm
index b3964bc..edc9622 100644
--- a/PVE/QemuServer.pm
+++ b/PVE/QemuServer.pm
@@ -119,13 +119,6 @@ PVE::JSONSchema::register_standard_option('pve-qemu-machine', {
 	optional => 1,
 });
 
-PVE::JSONSchema::register_standard_option('pve-targetstorage', {
-    description => "Mapping from source to target storages. Providing only a single storage ID maps all source storages to that storage. Providing the special value '1' will map each source storage to itself.",
-    type => 'string',
-    format => 'storage-pair-list',
-    optional => 1,
-});
-
 #no warnings 'redefine';
 
 my $nodename_cache;
-- 
2.30.2

Re: [pve-devel] [PATCH pve-common 1/1] schema: add pve-targetstorage (moved from qemu-server)

[Thomas Lamprecht]

this and "qemu-server: remove json schema pve-targetstorage (moved to pve-common)"
seems rather unrelated from the firewall series.

Besides that I welcome splitting the Firewall.pm monster module up.

For the record: the firewall part of this series needs a break/depends cycle:
- new firewall package needs to break old qemu-server and old pve-container
- new qemu-server and pve-container need a versioned dependency for new
  pve-firewall

Re: [pve-devel] [PATCH pve-common 1/1] schema: add pve-targetstorage (moved from qemu-server)

[Fabian Ebner]

Am 29.06.22 um 11:34 schrieb Thomas Lamprecht:
> this and "qemu-server: remove json schema pve-targetstorage (moved to pve-common)"
> seems rather unrelated from the firewall series.
> 
> Besides that I welcome splitting the Firewall.pm monster module up.
> 
> For the record: the firewall part of this series needs a break/depends cycle:
> - new firewall package needs to break old qemu-server and old pve-container
> - new qemu-server and pve-container need a versioned dependency for new
>   pve-firewall
> 

The targetstorage part of the series also needs the proper break/depends
between qemu-server and common. Fabian G. already sent this before:
https://lists.proxmox.com/pipermail/pve-devel/2022-February/051717.html

Re: [pve-devel] [PATCH pve-common 1/1] schema: add pve-targetstorage (moved from qemu-server)

[Thomas Lamprecht]

On 29/06/2022 11:46, Fabian Ebner wrote:
> Am 29.06.22 um 11:34 schrieb Thomas Lamprecht:
>> this and "qemu-server: remove json schema pve-targetstorage (moved to pve-common)"
>> seems rather unrelated from the firewall series.
>>
>> Besides that I welcome splitting the Firewall.pm monster module up.
>>
>> For the record: the firewall part of this series needs a break/depends cycle:
>> - new firewall package needs to break old qemu-server and old pve-container
>> - new qemu-server and pve-container need a versioned dependency for new
>>   pve-firewall
>>
> 
> The targetstorage part of the series also needs the proper break/depends
> between qemu-server and common. Fabian G. already sent this before:
> https://lists.proxmox.com/pipermail/pve-devel/2022-February/051717.html

yeah sure, but as said, that part should be its own series.

Re: [pve-devel] [PATCH pve-common 1/1] schema: add pve-targetstorage (moved from qemu-server)

[DERUMIER, Alexandre]

Le mercredi 29 juin 2022 à 11:34 +0200, Thomas Lamprecht a écrit :
> this and "qemu-server: remove json schema pve-targetstorage (moved to
> pve-common)"
> seems rather unrelated from the firewall series.
> 

I haved added it, because if you remove "use Firewall.pm" from lxc,

it's doesn't find the schema anymore (because QemuServer is require in
Firewall.pm)


But yes, it could be done firstly in another patch series. (and It seem
than fabian has already send a patch some months ago)


> Besides that I welcome splitting the Firewall.pm monster module up.
> 

yes, indeed, it's a little big monster ;)


BTW, Do you known why we need  in pve-firewall :

# dynamically include PVE::QemuServer and PVE::LXC
# to avoid dependency problems
my $have_qemu_server;
eval {
    require PVE::QemuServer;
    require PVE::QemuConfig;
    $have_qemu_server = 1;
};

my $have_lxc;
eval {
    require PVE::LXC;
    $have_lxc = 1;
};



?

is it because PVE::Qemuserver && PVE::LXC are also using use
PVE::Firewall ?






> For the record: the firewall part of this series needs a
> break/depends cycle:
> - new firewall package needs to break old qemu-server and old pve-
> container
> - new qemu-server and pve-container need a versioned dependency for
> new
>   pve-firewall
> 

> 

Re: [pve-devel] [PATCH pve-common 1/1] schema: add pve-targetstorage (moved from qemu-server)

[Thomas Lamprecht]

On 30/06/2022 09:30, DERUMIER, Alexandre wrote:
>> this and "qemu-server: remove json schema pve-targetstorage (moved to
>> pve-common)"
>> seems rather unrelated from the firewall series.
>>
> I haved added it, because if you remove "use Firewall.pm" from lxc,
> 
> it's doesn't find the schema anymore (because QemuServer is require in
> Firewall.pm)
> 

Ah, ok, so an implicit, "invisible" dependency from pve-container to
qemu-server, yuck. Then I understand the relation (could be more obviously
noted in a commit message though :-) - anyhow

> 
> But yes, it could be done firstly in another patch series. (and It seem
> than fabian has already send a patch some months ago)
> 
> 

I can take a look a the ones from Fabian so we can get that out of the way
first.

> BTW, Do you known why we need  in pve-firewall :> > # dynamically include PVE::QemuServer and PVE::LXC> # to avoid dependency problems> my $have_qemu_server;> eval {>     require PVE::QemuServer;>     require PVE::QemuConfig;>     $have_qemu_server = 1;> };> > my $have_lxc;> eval {>     require PVE::LXC;>     $have_lxc = 1;> };> > > > ?> > is it because PVE::Qemuserver && PVE::LXC are also using use> PVE::Firewall ?> 
Yes. It's mostly to allow solving the circular dependency issue for bootstrapping
PVE in a easy way, e.g., for a new Debian release like Bookworm next year.

I mean, we only uses it to parse the networks of VM/CTs, maybe we could reorganise
that - from top of my head: either write a simple standalone parser in firewall that
gets out the relevant info or split out the config schema stuff in separate packages
for each VM and CTs, which could be a bit more work but maybe fit in our project to
convert more of the schema from perl to rust (for datacenter manager).
Anyhow, that is probably not as important now and can stay as is with the eval'd
requires.

[pve-devel] applied: [PATCH pve-firewall 1/1] move clone_vmfw_conf && remove_vmfw_conf to a Helpers

[Thomas Lamprecht]

applied, but had to do some follow ups (see below)

Am 29/06/2022 um 11:08 schrieb Alexandre Derumier:
> Signed-off-by: Alexandre Derumier <aderumier@odiso.com>
> ---
>  src/PVE/Firewall.pm         | 25 ----------------------
>  src/PVE/Firewall/Helpers.pm | 41 +++++++++++++++++++++++++++++++++++++
>  src/PVE/Firewall/Makefile   | 12 +++++++++++
>  src/PVE/Makefile            |  2 ++
>  4 files changed, 55 insertions(+), 25 deletions(-)
>  create mode 100644 src/PVE/Firewall/Helpers.pm
>  create mode 100644 src/PVE/Firewall/Makefile
> 
> diff --git a/src/PVE/Firewall.pm b/src/PVE/Firewall.pm
> index 71746d2..e6cf4cd 100644
> --- a/src/PVE/Firewall.pm
> +++ b/src/PVE/Firewall.pm
> @@ -3340,31 +3340,6 @@ sub save_vmfw_conf {
>      }
>  }
>  
> -sub remove_vmfw_conf {
> -    my ($vmid) = @_;
> -
> -    my $vmfw_conffile = "$pvefw_conf_dir/$vmid.fw";
> -
> -    unlink $vmfw_conffile;
> -}
> -
> -sub clone_vmfw_conf {
> -    my ($vmid, $newid) = @_;
> -
> -    my $sourcevm_conffile = "$pvefw_conf_dir/$vmid.fw";
> -    my $clonevm_conffile = "$pvefw_conf_dir/$newid.fw";
> -
> -    lock_vmfw_conf($newid, 10, sub {
> -	if (-f $clonevm_conffile) {
> -	    unlink $clonevm_conffile;
> -	}
> -	if (-f $sourcevm_conffile) {
> -	    my $data = PVE::Tools::file_get_contents($sourcevm_conffile);
> -	    PVE::Tools::file_set_contents($clonevm_conffile, $data);
> -	}
> -    });
> -}

keeping above as skeleton allows to avoid a breakage on older qemu-server/pve-container
so I did that in a followup.re


> -
>  sub read_vm_firewall_configs {
>      my ($cluster_conf, $vmdata, $dir) = @_;
>  
> diff --git a/src/PVE/Firewall/Helpers.pm b/src/PVE/Firewall/Helpers.pm
> new file mode 100644
> index 0000000..3112ebc
> --- /dev/null
> +++ b/src/PVE/Firewall/Helpers.pm
> @@ -0,0 +1,41 @@
> +package PVE::Firewall::Helpers;
> +
> +use strict;
> +use warnings;
> +
> +use PVE::Tools qw(file_get_contents file_set_contents);
> +
> +use base 'Exporter';
> +our @EXPORT_OK = qw(
> +remove_vmfw_conf
> +clone_vmfw_conf
> +);
> +
> +my $pvefw_conf_dir = "/etc/pve/firewall";
> +
> +sub remove_vmfw_conf {
> +    my ($vmid) = @_;
> +
> +    my $vmfw_conffile = "$pvefw_conf_dir/$vmid.fw";
> +
> +    unlink $vmfw_conffile;
> +}
> +
> +sub clone_vmfw_conf {
> +    my ($vmid, $newid) = @_;
> +
> +    my $sourcevm_conffile = "$pvefw_conf_dir/$vmid.fw";
> +    my $clonevm_conffile = "$pvefw_conf_dir/$newid.fw";
> +
> +    lock_vmfw_conf($newid, 10, sub {

this was not in scope here so it only worked by luck, if at all?

As it only depends on PVE::Cluster, which is probably in the use paths
in most more involved/API facing modules anyway, I moved it over in a follow up.