Re: [pmg-devel] [PATCH pmg-api] backup: restore: keep relevant subdirs in /etc/pmg for inotify

[Fiona Ebner <f.ebner@proxmox.com>]

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