* [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
* 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
[parent not found: <12ddfc25-7445-064e-7c6a-beaacab9d17a@web.de>]
* 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] [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
* [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 an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.