Re: [pve-devel] [PATCH pve-manager 4/8] ceph: fix edge case of wrong files being deleted on purge

["Fabian Grünbichler" <f.gruenbichler@proxmox.com>]

On January 30, 2024 7:40 pm, Max Carrara wrote:
> Having a file named e.g. "60" in your current directory will cause it
> to be deleted when executind `pveceph purge`. This commit fixes that
> by making the config hash differ between which values represent file
> paths and which don't.
> 
> Signed-off-by: Max Carrara <m.carrara@proxmox.com>
> ---
>  PVE/Ceph/Tools.pm | 54 ++++++++++++++++++++++++++++++++++++-----------
>  1 file changed, 42 insertions(+), 12 deletions(-)
> 
> diff --git a/PVE/Ceph/Tools.pm b/PVE/Ceph/Tools.pm
> index a1458b40..3acef11b 100644
> --- a/PVE/Ceph/Tools.pm
> +++ b/PVE/Ceph/Tools.pm
> @@ -36,15 +36,42 @@ my $ceph_service = {
>  };
>  
>  my $config_hash = {
> -    ccname => $ccname,
> -    pve_ceph_cfgpath => $pve_ceph_cfgpath,
> -    pve_mon_key_path => $pve_mon_key_path,
> -    pve_ckeyring_path => $pve_ckeyring_path,
> -    ceph_bootstrap_osd_keyring => $ceph_bootstrap_osd_keyring,
> -    ceph_bootstrap_mds_keyring => $ceph_bootstrap_mds_keyring,
> -    ceph_mds_data_dir => $ceph_mds_data_dir,
> -    long_rados_timeout => 60,
> -    ceph_cfgpath => $ceph_cfgpath,
> +    ccname => {
> +	value => $ccname,
> +	is_file => 0,
> +    },
> +    pve_ceph_cfgpath => {
> +	value => $pve_ceph_cfgpath,
> +	is_file => 1,
> +    },
> +    pve_mon_key_path => {
> +	value => $pve_mon_key_path,
> +	is_file => 1,
> +    },
> +    pve_ckeyring_path => {
> +	value => $pve_ckeyring_path,
> +	is_file => 1,
> +    },
> +    ceph_bootstrap_osd_keyring => {
> +	value => $ceph_bootstrap_osd_keyring,
> +	is_file => 1,
> +    },
> +    ceph_bootstrap_mds_keyring => {
> +	value => $ceph_bootstrap_mds_keyring,
> +	is_file => 1,
> +    },
> +    ceph_mds_data_dir => {
> +	value => $ceph_mds_data_dir,
> +	is_file => 0,
> +    },
> +    long_rados_timeout => {
> +	value => 60,
> +	is_file => 0,
> +    },
> +    ceph_cfgpath => {
> +	value => $ceph_cfgpath,
> +	is_file => 1,
> +    },
>  };

this would be less verbose if the hash is just split into two, with
get_config using both, and the removal using the one just containing
files.

>  
>  sub get_local_version {
> @@ -84,7 +111,7 @@ sub get_cluster_versions {
>  sub get_config {
>      my $key = shift;
>  
> -    my $value = $config_hash->{$key};
> +    my $value = $config_hash->{$key}->{value};
>  
>      die "no such ceph config '$key'" if !$value;
>  
> @@ -123,8 +150,11 @@ sub purge_all_ceph_files {
>  	warn "Foreign MON address in ceph.conf. Keeping config & keyrings\n"
>      } else {
>  	print "Removing config & keyring files\n";
> -	foreach my $file (%$config_hash) {
> -	    unlink $file if (-e $file);
> +	for my $conf_value (values %$config_hash) {
> +	    if ($conf_value->{is_file}) {
> +		my $file = $conf_value->{value};
> +		unlink $file if (-e $file);
> +	    }
>  	}
>      }
>  }
> -- 
> 2.39.2
> 
> 
> 
> _______________________________________________
> pve-devel mailing list
> pve-devel@lists.proxmox.com
> https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
> 
> 
>