* [pve-devel] [PATCH firewall/cluster 0/2] cache firewall/cluster.fw
@ 2022-10-24 14:33 Stefan Hrdlicka
2022-10-24 14:33 ` [pve-devel] [PATCH firewall/cluster 1/2] fix #1965: cache firewall/cluster.fw file Stefan Hrdlicka
2022-10-24 14:33 ` [pve-devel] [PATCH firewall/cluster 2/2] register new file firewall/cluster.fw Stefan Hrdlicka
0 siblings, 2 replies; 6+ messages in thread
From: Stefan Hrdlicka @ 2022-10-24 14:33 UTC (permalink / raw)
To: pve-devel
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
^ permalink raw reply [flat|nested] 6+ messages in thread
* [pve-devel] [PATCH firewall/cluster 1/2] fix #1965: cache firewall/cluster.fw file
2022-10-24 14:33 [pve-devel] [PATCH firewall/cluster 0/2] cache firewall/cluster.fw Stefan Hrdlicka
@ 2022-10-24 14:33 ` Stefan Hrdlicka
2022-11-17 9:48 ` Wolfgang Bumiller
2022-10-24 14:33 ` [pve-devel] [PATCH firewall/cluster 2/2] register new file firewall/cluster.fw Stefan Hrdlicka
1 sibling, 1 reply; 6+ messages in thread
From: Stefan Hrdlicka @ 2022-10-24 14:33 UTC (permalink / raw)
To: pve-devel
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
^ permalink raw reply [flat|nested] 6+ messages in thread
* [pve-devel] [PATCH firewall/cluster 2/2] register new file firewall/cluster.fw
2022-10-24 14:33 [pve-devel] [PATCH firewall/cluster 0/2] cache firewall/cluster.fw Stefan Hrdlicka
2022-10-24 14:33 ` [pve-devel] [PATCH firewall/cluster 1/2] fix #1965: cache firewall/cluster.fw file Stefan Hrdlicka
@ 2022-10-24 14:33 ` Stefan Hrdlicka
2022-11-17 11:52 ` [pve-devel] applied: " Thomas Lamprecht
1 sibling, 1 reply; 6+ messages in thread
From: Stefan Hrdlicka @ 2022-10-24 14:33 UTC (permalink / raw)
To: pve-devel
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
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [pve-devel] [PATCH firewall/cluster 1/2] fix #1965: cache firewall/cluster.fw file
2022-10-24 14:33 ` [pve-devel] [PATCH firewall/cluster 1/2] fix #1965: cache firewall/cluster.fw file Stefan Hrdlicka
@ 2022-11-17 9:48 ` Wolfgang Bumiller
[not found] ` <12ddfc25-7445-064e-7c6a-beaacab9d17a@web.de>
0 siblings, 1 reply; 6+ messages in thread
From: Wolfgang Bumiller @ 2022-11-17 9:48 UTC (permalink / raw)
To: Stefan Hrdlicka; +Cc: pve-devel
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
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [pve-devel] [PATCH firewall/cluster 1/2] fix #1965: cache firewall/cluster.fw file
[not found] ` <12ddfc25-7445-064e-7c6a-beaacab9d17a@web.de>
@ 2022-11-17 10:38 ` Wolfgang Bumiller
0 siblings, 0 replies; 6+ messages in thread
From: Wolfgang Bumiller @ 2022-11-17 10:38 UTC (permalink / raw)
To: Roland; +Cc: Proxmox VE development discussion, Stefan Hrdlicka
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.
^ permalink raw reply [flat|nested] 6+ messages in thread
* [pve-devel] applied: [PATCH firewall/cluster 2/2] register new file firewall/cluster.fw
2022-10-24 14:33 ` [pve-devel] [PATCH firewall/cluster 2/2] register new file firewall/cluster.fw Stefan Hrdlicka
@ 2022-11-17 11:52 ` Thomas Lamprecht
0 siblings, 0 replies; 6+ messages in thread
From: Thomas Lamprecht @ 2022-11-17 11:52 UTC (permalink / raw)
To: Proxmox VE development discussion, Stefan Hrdlicka
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!
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2022-11-17 11:52 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-10-24 14:33 [pve-devel] [PATCH firewall/cluster 0/2] cache firewall/cluster.fw Stefan Hrdlicka
2022-10-24 14:33 ` [pve-devel] [PATCH firewall/cluster 1/2] fix #1965: cache firewall/cluster.fw file Stefan Hrdlicka
2022-11-17 9:48 ` Wolfgang Bumiller
[not found] ` <12ddfc25-7445-064e-7c6a-beaacab9d17a@web.de>
2022-11-17 10:38 ` Wolfgang Bumiller
2022-10-24 14:33 ` [pve-devel] [PATCH firewall/cluster 2/2] register new file firewall/cluster.fw Stefan Hrdlicka
2022-11-17 11:52 ` [pve-devel] applied: " Thomas Lamprecht
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox