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

[pve-devel] [PATCH firewall/cluster 0/2] 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.


pve-firewall
---
 src/PVE/Firewall.pm | 110 +++++++++++++++++++++++++++++++-------------
 1 file changed, 77 insertions(+), 33 deletions(-)


pve-cluster
---
 data/PVE/Cluster.pm | 1 +
 data/src/status.c   | 1 +
 2 files changed, 2 insertions(+)

-- 
2.30.2

[pve-devel] [PATCH firewall/cluster 1/2] 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 | 110 +++++++++++++++++++++++++++++++-------------
 1 file changed, 77 insertions(+), 33 deletions(-)

diff --git a/src/PVE/Firewall.pm b/src/PVE/Firewall.pm
index c56e448..9077995 100644
--- a/src/PVE/Firewall.pm
+++ b/src/PVE/Firewall.pm
@@ -24,6 +24,12 @@ use PVE::SafeSyslog;
 use PVE::Tools qw($IPV4RE $IPV6RE);
 use PVE::Tools qw(run_command lock_file dir_glob_foreach);
 
+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";
 
@@ -2951,23 +2957,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) {
@@ -3130,6 +3141,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 {
@@ -3564,26 +3595,44 @@ 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) {
+	$filename = $clusterfw_conf_filename if !defined($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");
+	};
+
+	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};
@@ -3615,13 +3664,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 {
@@ -4617,4 +4660,5 @@ sub update {
     run_locked($code);
 }
 
+
 1;
-- 
2.30.2

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

[Wolfgang Bumiller]

sorry for the delayed reply

some nits & please rebase ;-)

On Mon, Oct 24, 2022 at 04:33:58PM +0200, 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 | 110 +++++++++++++++++++++++++++++++-------------
>  1 file changed, 77 insertions(+), 33 deletions(-)
> 
> diff --git a/src/PVE/Firewall.pm b/src/PVE/Firewall.pm
> index c56e448..9077995 100644
> --- a/src/PVE/Firewall.pm
> +++ b/src/PVE/Firewall.pm
> @@ -24,6 +24,12 @@ use PVE::SafeSyslog;
>  use PVE::Tools qw($IPV4RE $IPV6RE);
>  use PVE::Tools qw(run_command lock_file dir_glob_foreach);
>  
> +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";
>  
> @@ -2951,23 +2957,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) {
> @@ -3130,6 +3141,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 {
> @@ -3564,26 +3595,44 @@ 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) {
> +	$filename = $clusterfw_conf_filename if !defined($filename);

^ The above line can be dropped given its condition now ;-)

> +	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 = "";

^ should be {}

> +	eval {
> +	    $res = PVE::Cluster::cfs_read_file("firewall/cluster.fw");
> +	};

I think a `warn $@ if $@` might be good here now.

> +
> +	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};
> @@ -3615,13 +3664,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 {
> @@ -4617,4 +4660,5 @@ sub update {
>      run_locked($code);
>  }
>  
> +
>  1;
> -- 
> 2.30.2

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

[Wolfgang Bumiller]

On Thu, Nov 17, 2022 at 11:23:19AM +0100, Roland wrote:
> nice!
> 
> btw, what about this one ?
> 
> https://bugzilla.proxmox.com/show_bug.cgi?id=3909#c3
> 
> actually, the firewall stuff is getting blindly executed every 10
> seconds, that's causing a lot of noise.
> 
> couldn't/shouldn't this be handled more intelligenty ?

I've been experimenting with a more event-driven approach but I can't
give you an ETA on when that'll happen as it's a lot more involved as it
requires support by our cluster-filesystem (since editing files on
another node needs to properly propagate events all the way through to
the firewall daemon on the other affected nodes), which requires a lot
more scrutiny.

[pve-devel] [PATCH firewall/cluster 2/2] register new file firewall/cluster.fw

[Stefan Hrdlicka]

added file for cache from bugzilla case #1965

Signed-off-by: Stefan Hrdlicka <s.hrdlicka@proxmox.com>
---
 data/PVE/Cluster.pm | 1 +
 data/src/status.c   | 1 +
 2 files changed, 2 insertions(+)

diff --git a/data/PVE/Cluster.pm b/data/PVE/Cluster.pm
index abcc46d..2afae73 100644
--- a/data/PVE/Cluster.pm
+++ b/data/PVE/Cluster.pm
@@ -51,6 +51,7 @@ my $observed = {
     'replication.cfg' => 1,
     'corosync.conf' => 1,
     'corosync.conf.new' => 1,
+    'firewall/cluster.fw' => 1,
     'user.cfg' => 1,
     'domains.cfg' => 1,
     'priv/shadow.cfg' => 1,
diff --git a/data/src/status.c b/data/src/status.c
index 9bceaeb..9679240 100644
--- a/data/src/status.c
+++ b/data/src/status.c
@@ -106,6 +106,7 @@ static memdb_change_t memdb_change_array[] = {
 	{ .path = "sdn/dns.cfg" },
 	{ .path = "sdn/.running-config" },
 	{ .path = "virtual-guest/cpu-models.conf" },
+	{ .path = "firewall/cluster.fw" },
 };
 
 static GMutex mutex;
-- 
2.30.2

[pve-devel] applied: [PATCH firewall/cluster 2/2] register new file firewall/cluster.fw

[Thomas Lamprecht]

Am 24/10/2022 um 16:33 schrieb Stefan Hrdlicka:
> added file for cache from bugzilla case #1965
> 
> Signed-off-by: Stefan Hrdlicka <s.hrdlicka@proxmox.com>
> ---
>  data/PVE/Cluster.pm | 1 +
>  data/src/status.c   | 1 +
>  2 files changed, 2 insertions(+)
> 
>

applied this one already, thanks!