[pve-devel] [RFC storage v2 10/25] plugin: introduce new_backup_provider() method

[Fabian Grünbichler]

> Fiona Ebner <f.ebner at proxmox.com> hat am 12.09.2024 15:21 CEST geschrieben:
> 
>  
> Am 12.09.24 um 14:43 schrieb Fabian Grünbichler:
> > On August 13, 2024 3:28 pm, Fiona Ebner wrote:
> >>
> >> Container mechanism 'directory':
> >>
> >> The backup provider gives the path to a directory with the full
> >> filesystem structure of the container.
> >>
> >> Container mechanism 'directory':
> >>
> >> The backup provider gives the path to a (potentially compressed) tar
> >> archive with the full filesystem structure of the container.
> > 
> > same as in the cover letter ;) base on the code here I assume the second
> > one should be tar. I wonder whether just tar wouldn't be enough (easier
> > to not get ACLs/xattrs/ownership/.. right)?
> > 
> 
> Yes, should be tar, will fix!
> 
> I'd guess it is more convenient for many providers to expose (a FUSE
> mount of) a directory. Using tar can mean more work. E.g. with Borg,
> mounting the archive seems much cheaper than creating the tar. It's also
> that the archive looks like:
> guest.config
> firewall.config
> filesystem/
> and while borg has "export-tar" where one can specify specific paths,
> the tar will still contain the "filesystem/" prefix. Not sure if there
> is an easy way to get rid of that.
> 
> >> diff --git a/src/PVE/Storage/Plugin.pm b/src/PVE/Storage/Plugin.pm
> >> index 6444390..d5b76ae 100644
> >> --- a/src/PVE/Storage/Plugin.pm
> >> +++ b/src/PVE/Storage/Plugin.pm
> >> @@ -1755,6 +1755,21 @@ sub rename_volume {
> >>      return "${storeid}:${base}${target_vmid}/${target_volname}";
> >>  }
> >>  
> >> +# Used by storage plugins for external backup providers. See PVE::BackupProvider::Plugin for the API
> >> +# the provider needs to implement.
> >> +#
> >> +# $scfg - the storage configuration
> >> +# $storeid - the storage ID
> >> +# $log_function($log_level, $message) - this log function can be used to write to the backup task
> >> +#   log in Proxmox VE. $log_level is 'info', 'warn' or 'err', $message is the message to be printed.
> >> +#
> >> +# Returns a blessed reference to the backup provider class.
> >> +sub new_backup_provider {
> >> +    my ($class, $scfg, $storeid, $log_function) = @_;
> >> +
> >> +    return;
> >> +}
> > 
> > would it maybe make sense to make this a "die implement me" and make the
> > opt-in via the storage plugin features? it would be more in line with
> > what we do in other parts and less subtle..
> > 
> 
> We don't have a method for storage plugin features yet, only
> volume_has_feature() and the stand-alone storage_can_replicate(). We
> could generalize (and deprecate) the latter though.

ah yeah, I was thinking of volume_has_feature, but that is not a good fit, you are right. could also be handled via plugindata and a new helper though - it seems a bit nicer to differentiate "supports external backups" from "get instance to do external backup/restore"..