* [pmg-devel] [PATCH pmg-api] backup: restore: keep relevant subdirs in /etc/pmg for inotify
@ 2022-11-30 8:27 Stoiko Ivanov
2022-11-30 13:46 ` Fiona Ebner
0 siblings, 1 reply; 3+ messages in thread
From: Stoiko Ivanov @ 2022-11-30 8:27 UTC (permalink / raw)
To: pmg-devel
By wiping the subdirectories in /etc/pmg/, we lose the inotify
watchers upon restore (/etc/pmg itself and thus most configs are
currently handled by the keep_root flag to rmtree)
This can lead to inconsistencies after restoring for parts relying on
config in a subdirectory (e.g. /etc/pmg/pbs/pbs.conf).
This patch keeps a list of relevant config directories in place by
iterating over all entries and checking if they should be kept.
an alternative approach would be to simply reload pmgdaemon/pmgproxy
upon config-restore, but that feels more likely to miss some
(potentially future) service, expecting inotify to work.
Reported-by: Fiona Ebner <f.ebner@proxmox.com>
Signed-off-by: Stoiko Ivanov <s.ivanov@proxmox.com>
---
Huge Thanks to Fiona for not only reporting, but also analyzing the issue!
src/PMG/Backup.pm | 31 ++++++++++++++++++++++++++++---
1 file changed, 28 insertions(+), 3 deletions(-)
diff --git a/src/PMG/Backup.pm b/src/PMG/Backup.pm
index a46167c..71193e9 100644
--- a/src/PMG/Backup.pm
+++ b/src/PMG/Backup.pm
@@ -8,7 +8,7 @@ use File::Path;
use POSIX qw(strftime);
use PVE::JSONSchema qw(get_standard_option);
-use PVE::Tools;
+use PVE::Tools qw(dir_glob_foreach);
use PMG::pmgcfg;
use PMG::AtomicFile;
@@ -306,8 +306,33 @@ sub pmg_restore {
unlink "$dirname/config/etc/pmg/cluster.conf"; # never restore cluster config
rmtree "$dirname/config/etc/pmg/master";
- # remove current config, but keep directory for INotify
- rmtree("/etc/pmg", { keep_root => 1 });
+ # remove current config, but keep directories for INotify
+ my $keep_dirs = {
+ acme => { accounts => 1 },
+ dkim => 1,
+ pbs => 1,
+ };
+
+ my $remove_with_exclude;
+ $remove_with_exclude = sub {
+ my ($dir, $excl) = @_;
+ dir_glob_foreach($dir, '.*', sub {
+ my ($fn) = @_;
+ return if $fn eq '.';
+ return if $fn eq '..';
+ if ( my $e = $excl->{$fn}) {
+ if (ref($e)) {
+ $remove_with_exclude->("$dir/$fn", $e);
+ } else {
+ rmtree("$dir/$fn", { keep_root => 1});
+ }
+ } else {
+ rmtree("$dir/$fn");
+ }
+ });
+ };
+ $remove_with_exclude->('/etc/pmg', $keep_dirs);
+
# copy files
system("cp -a $dirname/config/etc/pmg/* /etc/pmg/") == 0 ||
die "unable to restore system configuration: ERROR";
--
2.30.2
^ permalink raw reply [flat|nested] 3+ messages in thread
* Re: [pmg-devel] [PATCH pmg-api] backup: restore: keep relevant subdirs in /etc/pmg for inotify
2022-11-30 8:27 [pmg-devel] [PATCH pmg-api] backup: restore: keep relevant subdirs in /etc/pmg for inotify Stoiko Ivanov
@ 2022-11-30 13:46 ` Fiona Ebner
2022-11-30 15:05 ` Stoiko Ivanov
0 siblings, 1 reply; 3+ messages in thread
From: Fiona Ebner @ 2022-11-30 13:46 UTC (permalink / raw)
To: pmg-devel, Stoiko Ivanov
Am 30.11.22 um 09:27 schrieb Stoiko Ivanov:
> By wiping the subdirectories in /etc/pmg/, we lose the inotify
> watchers upon restore (/etc/pmg itself and thus most configs are
> currently handled by the keep_root flag to rmtree)
> This can lead to inconsistencies after restoring for parts relying on
> config in a subdirectory (e.g. /etc/pmg/pbs/pbs.conf).
>
> This patch keeps a list of relevant config directories in place by
> iterating over all entries and checking if they should be kept.
>
> an alternative approach would be to simply reload pmgdaemon/pmgproxy
> upon config-restore, but that feels more likely to miss some
> (potentially future) service, expecting inotify to work.
>
> Reported-by: Fiona Ebner <f.ebner@proxmox.com>
Tested-by: Fiona Ebner <f.ebner@proxmox.com>
> Signed-off-by: Stoiko Ivanov <s.ivanov@proxmox.com>
> ---
> Huge Thanks to Fiona for not only reporting, but also analyzing the issue!
>
> src/PMG/Backup.pm | 31 ++++++++++++++++++++++++++++---
> 1 file changed, 28 insertions(+), 3 deletions(-)
>
> diff --git a/src/PMG/Backup.pm b/src/PMG/Backup.pm
> index a46167c..71193e9 100644
> --- a/src/PMG/Backup.pm
> +++ b/src/PMG/Backup.pm
> @@ -8,7 +8,7 @@ use File::Path;
> use POSIX qw(strftime);
>
> use PVE::JSONSchema qw(get_standard_option);
> -use PVE::Tools;
> +use PVE::Tools qw(dir_glob_foreach);
>
> use PMG::pmgcfg;
> use PMG::AtomicFile;
> @@ -306,8 +306,33 @@ sub pmg_restore {
> unlink "$dirname/config/etc/pmg/cluster.conf"; # never restore cluster config
> rmtree "$dirname/config/etc/pmg/master";
>
> - # remove current config, but keep directory for INotify
> - rmtree("/etc/pmg", { keep_root => 1 });
> + # remove current config, but keep directories for INotify
> + my $keep_dirs = {
> + acme => { accounts => 1 },
> + dkim => 1,
> + pbs => 1,
> + };
Style nit: indentation
I wonder if we should have a helper for removing only files (or
alternatively keeping all directories) from a directory structure,
rather than passing explicitly what to keep? Otherwise, this list needs
to be updated when the directory structure changes.
Also might make sense to put it somewhere more central, as it likely
will be needed when PVE gets host backup/restore support (for PBS) too?
But can still be done when actually needed of course.
> +
> + my $remove_with_exclude;
> + $remove_with_exclude = sub {
> + my ($dir, $excl) = @_;
> + dir_glob_foreach($dir, '.*', sub {
> + my ($fn) = @_;
> + return if $fn eq '.';
> + return if $fn eq '..';
> + if ( my $e = $excl->{$fn}) {
> + if (ref($e)) {
> + $remove_with_exclude->("$dir/$fn", $e);
> + } else {
> + rmtree("$dir/$fn", { keep_root => 1});
> + }
> + } else {
> + rmtree("$dir/$fn");
> + }
> + });
> + };
> + $remove_with_exclude->('/etc/pmg', $keep_dirs);
> +
> # copy files
> system("cp -a $dirname/config/etc/pmg/* /etc/pmg/") == 0 ||
> die "unable to restore system configuration: ERROR";
^ permalink raw reply [flat|nested] 3+ messages in thread
* Re: [pmg-devel] [PATCH pmg-api] backup: restore: keep relevant subdirs in /etc/pmg for inotify
2022-11-30 13:46 ` Fiona Ebner
@ 2022-11-30 15:05 ` Stoiko Ivanov
0 siblings, 0 replies; 3+ messages in thread
From: Stoiko Ivanov @ 2022-11-30 15:05 UTC (permalink / raw)
To: Fiona Ebner; +Cc: pmg-devel
On Wed, Nov 30, 2022 at 02:46:16PM +0100, Fiona Ebner wrote:
> Am 30.11.22 um 09:27 schrieb Stoiko Ivanov:
> > By wiping the subdirectories in /etc/pmg/, we lose the inotify
> > watchers upon restore (/etc/pmg itself and thus most configs are
> > currently handled by the keep_root flag to rmtree)
> > This can lead to inconsistencies after restoring for parts relying on
> > config in a subdirectory (e.g. /etc/pmg/pbs/pbs.conf).
> >
> > This patch keeps a list of relevant config directories in place by
> > iterating over all entries and checking if they should be kept.
> >
> > an alternative approach would be to simply reload pmgdaemon/pmgproxy
> > upon config-restore, but that feels more likely to miss some
> > (potentially future) service, expecting inotify to work.
> >
> > Reported-by: Fiona Ebner <f.ebner@proxmox.com>
>
> Tested-by: Fiona Ebner <f.ebner@proxmox.com>
Thanks for testing!
>
> > Signed-off-by: Stoiko Ivanov <s.ivanov@proxmox.com>
> > ---
> > Huge Thanks to Fiona for not only reporting, but also analyzing the issue!
> >
> > src/PMG/Backup.pm | 31 ++++++++++++++++++++++++++++---
> > 1 file changed, 28 insertions(+), 3 deletions(-)
> >
> > diff --git a/src/PMG/Backup.pm b/src/PMG/Backup.pm
> > index a46167c..71193e9 100644
> > --- a/src/PMG/Backup.pm
> > +++ b/src/PMG/Backup.pm
> > @@ -8,7 +8,7 @@ use File::Path;
> > use POSIX qw(strftime);
> >
> > use PVE::JSONSchema qw(get_standard_option);
> > -use PVE::Tools;
> > +use PVE::Tools qw(dir_glob_foreach);
> >
> > use PMG::pmgcfg;
> > use PMG::AtomicFile;
> > @@ -306,8 +306,33 @@ sub pmg_restore {
> > unlink "$dirname/config/etc/pmg/cluster.conf"; # never restore cluster config
> > rmtree "$dirname/config/etc/pmg/master";
> >
> > - # remove current config, but keep directory for INotify
> > - rmtree("/etc/pmg", { keep_root => 1 });
> > + # remove current config, but keep directories for INotify
> > + my $keep_dirs = {
> > + acme => { accounts => 1 },
> > + dkim => 1,
> > + pbs => 1,
> > + };
>
> Style nit: indentation
>
> I wonder if we should have a helper for removing only files (or
> alternatively keeping all directories) from a directory structure,
> rather than passing explicitly what to keep? Otherwise, this list needs
> to be updated when the directory structure changes.
was one of my ideas, but then I thought it would be a larger change - from
the current behavior and leave potentially unrelated, manually created
directories lying around.
Then again - you have a point with a few directory-leftovers being
preferable to manually tracking (and likely forgetting) added
config-directories in an additional place - I'll try to add
`rmtree_keep_skel` to PVE::Utils, and use it here
>
> Also might make sense to put it somewhere more central, as it likely
> will be needed when PVE gets host backup/restore support (for PBS) too?
> But can still be done when actually needed of course.
>
> > +
> > + my $remove_with_exclude;
> > + $remove_with_exclude = sub {
> > + my ($dir, $excl) = @_;
> > + dir_glob_foreach($dir, '.*', sub {
> > + my ($fn) = @_;
> > + return if $fn eq '.';
> > + return if $fn eq '..';
> > + if ( my $e = $excl->{$fn}) {
> > + if (ref($e)) {
> > + $remove_with_exclude->("$dir/$fn", $e);
> > + } else {
> > + rmtree("$dir/$fn", { keep_root => 1});
> > + }
> > + } else {
> > + rmtree("$dir/$fn");
> > + }
> > + });
> > + };
> > + $remove_with_exclude->('/etc/pmg', $keep_dirs);
> > +
> > # copy files
> > system("cp -a $dirname/config/etc/pmg/* /etc/pmg/") == 0 ||
> > die "unable to restore system configuration: ERROR";
^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2022-11-30 15:05 UTC | newest]
Thread overview: 3+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-11-30 8:27 [pmg-devel] [PATCH pmg-api] backup: restore: keep relevant subdirs in /etc/pmg for inotify Stoiko Ivanov
2022-11-30 13:46 ` Fiona Ebner
2022-11-30 15:05 ` Stoiko Ivanov
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox