[pve-devel] [PATCH V2 pve-firewall 0/1] cache firewall/cluster.fw

[pve-devel] [PATCH V2 pve-firewall 0/1] cache firewall/cluster.fw

[Stefan Hrdlicka]

This patch adds the firewall/cluster.fw to caching. On my system with a
list of 25k IP sets CPU consumption for the process went from ~20 % to
~10 % with this caching enabled. Still pretty high but better then
before.


V1 -> V2
# pve-firewall
* fix review issues

# pve-cluster
* already accepted

Stefan Hrdlicka (1):
  fix #1965: cache firewall/cluster.fw file

 src/PVE/Firewall.pm | 108 ++++++++++++++++++++++++++++++--------------
 1 file changed, 75 insertions(+), 33 deletions(-)

-- 
2.30.2

[pve-devel] [PATCH V2 pve-firewall 1/1] fix #1965: cache firewall/cluster.fw file

[Stefan Hrdlicka]

for large IP sets (for example > 25k) it takes noticable longer to parse the
files, this commit caches the cluster.fw file and reduces parsing time

Signed-off-by: Stefan Hrdlicka <s.hrdlicka@proxmox.com>
---
 src/PVE/Firewall.pm | 108 ++++++++++++++++++++++++++++++--------------
 1 file changed, 75 insertions(+), 33 deletions(-)

diff --git a/src/PVE/Firewall.pm b/src/PVE/Firewall.pm
index d40a9b1..8bb41ba 100644
--- a/src/PVE/Firewall.pm
+++ b/src/PVE/Firewall.pm
@@ -26,6 +26,12 @@ use PVE::Tools qw(run_command lock_file dir_glob_foreach);
 
 use PVE::Firewall::Helpers;
 
+PVE::Cluster::cfs_register_file(
+    "firewall/cluster.fw",
+    \&parse_clusterfw_config,
+    \&_save_clusterfw_conf
+);
+
 my $pvefw_conf_dir = "/etc/pve/firewall";
 my $clusterfw_conf_filename = "$pvefw_conf_dir/cluster.fw";
 
@@ -2953,23 +2959,28 @@ sub parse_alias {
     return undef;
 }
 
-sub generic_fw_config_parser {
-    my ($filename, $cluster_conf, $empty_conf, $rule_env) = @_;
-
-    my $section;
-    my $group;
+sub parse_clusterfw_config {
+    my ($filename, $raw) = @_;
+    my $empty_conf = {
+	rules => [],
+	options => {},
+	aliases => {},
+	groups => {},
+	group_comments => {},
+	ipset => {} ,
+	ipset_comments => {},
+    };
 
-    my $res = $empty_conf;
+    return _generic_fw_config_parser($filename, $empty_conf, $empty_conf, 'cluster', $raw);
+}
 
-    my $raw;
-    if ($filename =~ m!^/etc/pve/(.*)$!) {
-	$raw = PVE::Cluster::get_config($1);
-    } else {
-	$raw = eval { PVE::Tools::file_get_contents($filename) }; # ignore errors
-    }
-    return {} if !$raw;
+sub _generic_fw_config_parser {
+    my ($filename, $cluster_conf, $empty_conf, $rule_env, $raw) = @_;
 
+    my $section;
+    my $group;
     my $curr_group_keys = {};
+    my $res = $empty_conf;
 
     my $linenr = 0;
     while ($raw =~ /^\h*(.*?)\h*$/gm) {
@@ -3132,6 +3143,26 @@ sub generic_fw_config_parser {
     return $res;
 }
 
+sub generic_fw_config_parser {
+    my ($filename, $cluster_conf, $empty_conf, $rule_env) = @_;
+
+    my $section;
+    my $group;
+
+    my $res = $empty_conf;
+
+    my $raw;
+    if ($filename =~ m!^/etc/pve/(.*)$!) {
+	$raw = PVE::Cluster::get_config($1);
+    } else {
+	$raw = eval { PVE::Tools::file_get_contents($filename) }; # ignore errors
+    }
+    return {} if !$raw;
+
+    my $curr_group_keys = {};
+    return _generic_fw_config_parser($filename, $cluster_conf, $empty_conf, $rule_env, $raw);
+}
+
 # this is only used to prevent concurrent runs of rule compilation/application
 # see lock_*_conf for cfs locks protectiong config modification
 sub run_locked {
@@ -3544,26 +3575,42 @@ sub lock_clusterfw_conf {
 sub load_clusterfw_conf {
     my ($filename) = @_;
 
-    $filename = $clusterfw_conf_filename if !defined($filename);
-    my $empty_conf = {
-	rules => [],
-	options => {},
-	aliases => {},
-	groups => {},
-	group_comments => {},
-	ipset => {} ,
-	ipset_comments => {},
-    };
+    # special case for tests
+    if ($filename) {
+	my $empty_conf = {
+	    rules => [],
+	    options => {},
+	    aliases => {},
+	    groups => {},
+	    group_comments => {},
+	    ipset => {} ,
+	    ipset_comments => {},
+	};
+
+	my $cluster_conf = generic_fw_config_parser($filename, $empty_conf, $empty_conf, 'cluster');
+	$set_global_log_ratelimit->($cluster_conf->{options});
 
-    my $cluster_conf = generic_fw_config_parser($filename, $empty_conf, $empty_conf, 'cluster');
-    $set_global_log_ratelimit->($cluster_conf->{options});
+	return $cluster_conf;
+
+    } else {
+	my $res = {};
+	eval { $res = PVE::Cluster::cfs_read_file("firewall/cluster.fw"); };
+	warn $@ if $@;
+
+	return $res;
+    }
 
-    return $cluster_conf;
 }
 
 sub save_clusterfw_conf {
     my ($cluster_conf) = @_;
 
+    PVE::Cluster::cfs_write_file("firewall/cluster.fw", $cluster_conf)
+}
+
+sub _save_clusterfw_conf {
+    my ($filename, $cluster_conf) = @_;
+
     my $raw = '';
 
     my $options = $cluster_conf->{options};
@@ -3595,13 +3642,7 @@ sub save_clusterfw_conf {
 	    $raw .= "\n";
 	}
     }
-
-    if ($raw) {
-	mkdir $pvefw_conf_dir;
-	PVE::Tools::file_set_contents($clusterfw_conf_filename, $raw);
-    } else {
-	unlink $clusterfw_conf_filename;
-    }
+    return $raw
 }
 
 sub lock_hostfw_conf : prototype($$$@) {
@@ -4601,4 +4642,5 @@ sub update {
     run_locked($code);
 }
 
+
 1;
-- 
2.30.2

Re: [pve-devel] [PATCH V2 pve-firewall 1/1] fix #1965: cache firewall/cluster.fw file

[Lukas Wagner]

On 11/25/22 15:47, Stefan Hrdlicka wrote:
> for large IP sets (for example > 25k) it takes noticable longer to parse the
> files, this commit caches the cluster.fw file and reduces parsing time
> 
> Signed-off-by: Stefan Hrdlicka <s.hrdlicka@proxmox.com>
> ---
>  src/PVE/Firewall.pm | 108 ++++++++++++++++++++++++++++++--------------
>  1 file changed, 75 insertions(+), 33 deletions(-)
> 

Stumbled over this by accident 👻. Seems like this was never applied,
and the author has left the company since, hence they were not able to
ping this any more. Bugzilla entry [0] has been in 'PATCH AVAILABLE'
ever since this was originally posted.

The patch still seems to apply cleanly, but I did not do any
verification/testing apart from this.

[0] https://bugzilla.proxmox.com/show_bug.cgi?id=1965

-- 
- Lukas