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

["Max Carrara" <m.carrara@proxmox.com>]

On Tue Jul 23, 2024 at 11:56 AM CEST, Fiona Ebner wrote:
> The new_backup_provider() method can be used by storage plugins for
> external backup providers. If the method returns a provider, Proxmox
> VE will use callbacks to that provider for backups and restore instead
> of using its usual backup/restore mechanisms.
>
> API age and version are both bumped.
>
> The backup provider API is split into two parts, both of which again
> need different implementations for VM and LXC guests:
>
> 1. Backup API
>
> There hook callbacks for the start/end/abort phases of guest backups
> as well as for start/end/abort phases of a whole backup job.
>
> The backup_get_mechanism() method is used to decide on the backup
> mechanism. Currently only 'nbd' for VMs and 'directory' for containers
> are possible. It also let's the plugin decide whether to use a bitmap
> for incremental VM backup or not.
>
> Next, there are methods for backing up guest and firewall
> configuration as well as for the backup mechanisms:
>
> - a container filesystem using a provided directory. The directory
>   contains an unchanging copy of the container's file system.
>
> - a VM disk using a provided NBD export. The export is an unchanging
>   copy of the VM's disk. Either the full image, or in case a bitmap is
>   used, the dirty parts of the image since the last time the bitmap
>   was used for a successful backup. Reading outside of the dirty parts
>   will result in an error. After backing up each part of the disk, it
>   should be discarded in the export to avoid unnecessary space usage
>   on the Proxmox VE side (there is an associated fleecing image).
>
> Finally, some helpers like getting the provider name or volume ID for
> the backup target, as well as for handling the backup log.
>
> 2. Restore API
>
> The restore_get_mechanism() method is used to decide on the restore
> mechanism. Currently, only 'qemu-img' for VMs and 'directory' and
> 'tar' for containers are possible.
>
> Next, methods for extracting the guest and firewall configuration and
> the implementations of the restore mechanism. It is enough to
> implement one restore mechanism per guest type of course:
>
> - for VMs, with the 'qemu-img' mechanism, the backup provider gives a
>   path to the disk image that will be restore. The path should be
>   something qemu-img can deal with, e.g. can also be an NBD URI.
>
> - for containers, with the 'directory' mechanism, the backup provider
>   gives the path to a directory with the full filesystem structure of
>   the container.
>
> - for containers, with the 'tar' mechanism, the backup provider gives
>   the path to a (potentially compressed) tar archive with the full
>   filesystem structure of the container.
>
> For VMs, there also is a restore_qemu_get_device_info() helper
> required, to get the disks included in the backup and their sizes.
>
> See the PVE::BackupProvider::Plugin module for the full API
> documentation.
>
> Signed-off-by: Fiona Ebner <f.ebner@proxmox.com>

Some overall thoughts:

1.  I'm really, really happy to see documentation in this module here,
    that's fantastic! :)

    While the contents of the docs seem fine, I would suggest you used
    POD instead. You can find an example in one of my recent series. [1]
    I mainly prefer POD solely because it's what Perl uses; it also
    indirectly makes sure we all use the same kind of format for
    documenting our Perl code.

    Of course, we've currently not decided on any particular format, but
    because the opportunity arose, I wanted to pitch POD here
    nevertheless. ;)

2.  I would personally prefer a namespace like `PVE::Backup::Provider`
    instead of `PVE::BackupProvider`, simply because it leaves room for
    further packages and reduces churn in the long term, IMO.

    The same goes for backup provider plugins - IMO namespacing them
    like e.g. `PVE::Backup::Provider::Plugin::Foo` where `Foo` is a
    (concrete) plugin.

    While this seems long or somewhat excessive, I think it enforces a
    clear package / module hierarchy and keeps things tidier in the long
    term, and those couple extra keystrokes don't really hurt anyone.

There are some more comments inline, though I want to mention that I
really like your work so far! :)

[1]: https://lists.proxmox.com/pipermail/pve-devel/2024-July/064703.html

> ---
>  src/PVE/BackupProvider/Makefile  |   6 +
>  src/PVE/BackupProvider/Plugin.pm | 343 +++++++++++++++++++++++++++++++
>  src/PVE/Makefile                 |   1 +
>  src/PVE/Storage.pm               |  12 +-
>  src/PVE/Storage/Plugin.pm        |  15 ++
>  5 files changed, 375 insertions(+), 2 deletions(-)
>  create mode 100644 src/PVE/BackupProvider/Makefile
>  create mode 100644 src/PVE/BackupProvider/Plugin.pm
>
> diff --git a/src/PVE/BackupProvider/Makefile b/src/PVE/BackupProvider/Makefile
> new file mode 100644
> index 0000000..53cccac
> --- /dev/null
> +++ b/src/PVE/BackupProvider/Makefile
> @@ -0,0 +1,6 @@
> +SOURCES = Plugin.pm
> +
> +.PHONY: install
> +install:
> +	for i in ${SOURCES}; do install -D -m 0644 $$i ${DESTDIR}${PERLDIR}/PVE/BackupProvider/$$i; done
> +
> diff --git a/src/PVE/BackupProvider/Plugin.pm b/src/PVE/BackupProvider/Plugin.pm
> new file mode 100644
> index 0000000..6ae8a01
> --- /dev/null
> +++ b/src/PVE/BackupProvider/Plugin.pm
> @@ -0,0 +1,343 @@
> +package PVE::BackupProvider::Plugin;
> +
> +use strict;
> +use warnings;
> +
> +# Backup Provider API
> +
> +# Get the backup provider class for a new backup job.
> +#
> +# $storage_plugin - the associated storage plugin class
> +# $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 class.
> +sub new {
> +    my ($class, $storage_plugin, $scfg, $storeid, $log_function) = @_;
> +
> +    die "implement me in subclass\n";
> +}
> +
> +# Returns the name of the backup provider. It will be printed in some log lines.
> +sub provider_name {
> +    my ($self) = @_;
> +
> +    die "implement me in subclass\n";
> +}
> +
> +# The following hooks are called during various phases of the backup job.
> +
> +# Called when the job starts, before the first backup is made.
> +#
> +# $start_time - Unix time-stamp of when the job started.
> +sub job_start {
> +    my ($self, $start_time) = @_;
> +
> +    die "implement me in subclass\n";
> +}
> +
> +# Called when the job ends, after all backups are finished, even if some backups failed.
> +sub job_end {
> +    my ($self) = @_;
> +
> +    die "implement me in subclass\n";
> +}
> +
> +# Called when the job is aborted (e.g. interrupted by signal, other fundamental failure)
> +#
> +# $error - the error message indicating the failure.
> +sub job_abort {
> +    my ($self, $error) = @_;
> +
> +    die "implement me in subclass\n";
> +}
> +
> +# Called before the backup of a given guest is made.
> +#
> +# $vmid - ID of the guest being backed up.
> +# $vmtype - type of the guest being backed up. Currently either 'qemu' or 'lxc'.
> +sub backup_start {
> +    my ($self, $vmid, $vmtype) = @_;
> +
> +    die "implement me in subclass\n";
> +}
> +
> +# Called after the backup of a given guest finished successfully.
> +#
> +# $vmid - ID of the guest being backed up.
> +sub backup_end {
> +    my ($self, $vmid) = @_;
> +
> +    die "implement me in subclass\n";
> +}
> +
> +# Called if the backup of a given guest encountered an error or was aborted.
> +#
> +# $vmid - ID of the guest being backed up.
> +sub backup_abort {
> +    my ($self, $vmid, $error) = @_;
> +
> +    die "implement me in subclass\n";
> +}
> +
> +# What mechanism should be used to back up the guest.
> +#
> +# $vmid - ID of the guest being backed up.
> +# $vmtype - type of the guest being backed up. Currently either 'qemu' or 'lxc'.
> +#
> +# Returns ($mechanism, $bitmap_id)
> +# $mechanism - currently only 'nbd' for type 'qemu' and 'directory' for type 'lxc' are possible. If
> +#   there is no support for one of the guest types, the method should either die or return undef.
> +#   The plugin needs to implement the corresponding backup_<mechanism>() function.
> +# $bitmap_id - if the plugin supports backing up with a bitmap, the ID of the bitmap to use. Return
> +#   undef otherwise. Re-use the same ID multiple times for incremental backup.
> +sub backup_get_mechanism {
> +    my ($self, $vmid, $vmtype) = @_;
> +
> +    die "implement me in subclass\n";
> +}
> +
> +# Get the target/volume name of the backup archive that will be created by the current backup.
> +#
> +# $vmid - ID of the guest being backed up.
> +# $vmtype - type of the guest being backed up. Currently either 'qemu' or 'lxc'.
> +#
> +# Returns the volume name the archive can later be accessed via the corresponding storage plugin.
> +sub backup_get_target {
> +    my ($self, $vmid, $vmtype) = @_;
> +
> +    die "implement me in subclass\n";
> +}
> +
> +# Returns the size of the backup after completion.
> +#
> +# $vmid - ID of the guest being backed up.
> +sub backup_get_task_size {
> +    my ($self, $vmid) = @_;
> +
> +    die "implement me in subclass\n";
> +}
> +
> +# The following functions are for backing up guest volumes and guest configuration.
> +
> +# Backup the guest's configuration file.
> +#
> +# $vmid - ID of the guest being backed up.
> +# $filename - path to the file with the guest configuration.
> +sub backup_guest_config {
> +    my ($self, $vmid, $filename) = @_;
> +
> +    die "implement me in subclass\n";
> +}
> +
> +# Backup the guest's firewall configuration file.
> +#
> +# $vmid - ID of the guest being backed up.
> +# $filename - path to the file with the guest's firewall configuration.
> +sub backup_firewall_config {
> +    my ($self, $vmid, $filename) = @_;
> +
> +    die "implement me in subclass\n";
> +}
> +
> +# Handle the backup's log file which contains the task log for the backup. For example, upload a
> +# copy to the backup server.
> +#
> +# $vmid - ID of the guest being backed up.
> +# $filename - path to the file with the guest configuration.
> +sub backup_handle_log_file {
> +    my ($self, $vmid, $filename) = @_;
> +
> +    die "implement me in subclass\n";
> +}
> +
> +# Backup a volume that is made available via NBD (VM only). This method will be called for each
> +# volume of the VM.
> +#
> +# $vmid - ID of the guest being backed up.
> +# $devicename - device name for the current volume as well as the name of the NBD export. Needs to
> +#   be remembered for restoring.
> +# $nbd_path - path to the NBD socket.
> +# $bitmap-mode - either of:
> +#   'none' if not used
> +#   'new' if newly created
> +#   'reuse' if re-using bitmap with the same ID as requested
> +# $bitmap_name - the name of the dirty bitmap if a bitmap is used.
> +# $bandwidth_limit - value is in bytes/second. The backup provider is expected to honor this rate
> +#   limit for IO on the backup source and network traffic.
> +#
> +# Returns when done backing up. Ideally, the method should log the progress during backup.
> +sub backup_nbd {
> +    my ($self, $vmid, $devicename, $nbd_path, $bitmap_mode, $bitmap_name, $bandwidth_limit) = @_;
> +
> +    die "implement me in subclass\n";
> +}
> +
> +# Backup a directory (LXC only).
> +#
> +# $vmid - ID of the guest being backed up.
> +# $directory - path to the directory to back up.
> +# $id_map - list of UID/GID mappings for the container, each mapping is itself a list with four
> +#   entries, e.g. ["u", "0", "100000", "65536"]:
> +#   1. a character 'u' (for a user mapping), 'g' (for a group mapping)
> +#   2. the first userid in the user namespace
> +#   3. the first userid as seen on the host
> +#   4. the number of ids to be mapped.
> +# $exclude_paths - list of glob patterns of files and directories to be excluded (compatible with
> +#   rsync). See also https://pve.proxmox.com/pve-docs/chapter-vzdump.html#_file_exclusions
> +#   and https://pbs.proxmox.com/docs/backup-client.html#excluding-files-directories-from-a-backup
> +# $sources - list of paths (for separate mount points, including "." for the root) inside the
> +#   directory to be backed up.
> +# $bandwidth_limit - value is in bytes/second. The backup provider is expected to honor this
> +#   ratelimit for IO on the backup source and network traffic.
> +#
> +# Returns when done backing up. Ideally, the method should log the progress during backup.
> +sub backup_directory {
> +    my ($self, $vmid, $directory, $id_map, $exclude_paths, $sources, $bandwidth_limit) = @_;
> +
> +    die "implement me in subclass\n";
> +}

The above two methods - `backup_nbd` and `backup_directory` - is there
perhaps a way to merge them? I'm not sure if what I'm having in mind
here is actually feasible, but what I mean is "making the method
agnostic to the type of backup". As in, perhaps pass a hash that
contains a `type` key for the type of backup being made, and instead of
having long method signatures, include the remaining parameters as the
remaining keys. For example:

{
    'type' => 'lxc-dir',  # type names are just examples here
    'directory' => '/foo/bar/baz',
    'bandwidth_limit' => 42,
    ...
}

{
    'type' => 'vm-nbd',
    'device_name' => '...',
    'nbd_path' => '...',
    ...
}

You get the point :P

IMO it would make it easier to extend later, and also make it more
straightforward to introduce new parameters / deprecate old ones, while
the method signature stays stable otherwise.

The same goes for the different cleanup methods further down below;
instead of having a separate method for each "type of cleanup being
performed", let the implementor handle it according to the data the
method receives.

IMHO I think it's best to be completely agnostic over VM / LXC backups
(and their specific types) wherever possible and let the data describe
what's going on instead.

For the specific types we can always then provide helper functions that
handle common cases that implementors can use.

Extending on my namespace idea above, those helpers could then land in
e.g. `PVE::Backup::Provider::Common`, `PVE::Backup::Provider::Common::LXC`,
etc.

> +
> +# Restore API
> +
> +# What mechanism should be used to restore the guest.
> +#
> +# $volname - volume name of the backup being restored.
> +# $storeid - storage ID of the backup storage.
> +#
> +# Returns ($mechanism, $vmtype)
> +# $mechanism - currently 'qemu-img' for type 'qemu' and 'tar' or 'directory' for type 'lxc' are
> +#   possible. The plugin needs to implement the corresponding restore_<mechanism>_init() and
> +#   restore_<mechanism>_cleanup() functions. For type 'qemu', restore_qemu_get_device_info() needs
> +#   to be implemented too.
> +# $vmtype - type of the guest being restored. Currently either 'qemu' or 'lxc'.
> +sub restore_get_mechanism {
> +    my ($self, $volname, $storeid) = @_;
> +
> +    die "implement me in subclass\n";
> +}
> +
> +# Extract the guest configuration from the given backup.
> +#
> +# $volname - volume name of the backup being restored.
> +# $storeid - storage ID of the backup storage.
> +#
> +# Returns the raw contents of the backed-up configuration file.
> +sub extract_guest_config {
> +    my ($self, $volname, $storeid) = @_;
> +
> +    die "implement me in subclass\n";
> +}
> +
> +# Extract the firewall configuration from the given backup.
> +#
> +# $volname - volume name of the backup being restored.
> +# $storeid - storage ID of the backup storage.
> +#
> +# Returns the raw contents of the backed-up configuration file.
> +# Returns undef if there is no firewall config, dies if it can't be extracted.
> +sub extract_firewall_config {
> +    my ($self, $volname, $storeid) = @_;
> +
> +    die "implement me in subclass\n";
> +}
> +
> +# For VM backups, get basic information about volumes in the backup.
> +#
> +# $volname - volume name of the backup being restored.
> +# $storeid - storage ID of the backup storage.
> +#
> +# Returns a hash with the following structure:
> +# {
> +#   $devicenameA => { size => $sizeA },
> +#   $devicenameB => { size => $sizeB },
> +#   ...
> +# }
> +# $devicename - the device name that was given as an argument to the backup routine when the backup
> +#   was created.
> +# $size - the size of the resulting image file after restore. For the 'qemu-img' mechanism, it needs
> +#   to be (at least) the same size as the backed-up image, i.e. the block device referenced by the
> +#   path returned by restore_qemu_img_init().
> +sub restore_qemu_get_device_info {
> +    my ($self, $volname, $storeid) = @_;
> +
> +    die "implement me in subclass\n";
> +}
> +
> +# For VM backups, set up restore via 'qemu-img'.
> +#
> +# $volname - volume name of the backup being restored.
> +# $storeid - storage ID of the backup storage.
> +# $devicename - the device that should be prepared for restore. Same as the argument to the backup
> +#   routine when the backup was created.
> +#
> +# Returns a path that 'qemu-img' can use as a source for the 'qemu-img convert' command. E.g. this
> +# can also be an NBD URI.
> +sub restore_qemu_img_init {
> +    my ($self, $volname, $storeid, $devicename) = @_;
> +
> +    die "implement me in subclass\n";
> +}
> +
> +# For VM backups, clean up after the restore with 'qemu-img'. Called in both, success and failure
> +# scenarios.
> +#
> +# $volname - volume name of the backup being restored.
> +# $storeid - storage ID of the backup storage.
> +# $devicename - the device that was restored.
> +sub restore_qemu_img_cleanup {
> +    my ($self, $volname, $storeid, $devicename) = @_;
> +
> +    die "implement me in subclass\n";
> +}
> +
> +# For LXC backups, set up restore via 'tar'.
> +#
> +# $volname - volume name of the backup being restored.
> +# $storeid - storage ID of the backup storage.
> +#
> +# Returns the path to the (compressed) '.tar' archive.
> +sub restore_tar_init {
> +    my ($self, $volname, $storeid) = @_;
> +
> +    die "implement me in subclass\n";
> +}
> +
> +# For LXC backups, clean up after the restore with 'tar'. Called in both, success and failure
> +# scenarios.
> +#
> +# $volname - volume name of the backup being restored.
> +# $storeid - storage ID of the backup storage.
> +sub restore_tar_cleanup {
> +    my ($self, $volname, $storeid) = @_;
> +
> +    die "implement me in subclass\n";
> +}
> +
> +# For LXC backups, set up restore by providing a directory containing the full filesystem structure
> +# of the container.
> +#
> +# $volname - volume name of the backup being restored.
> +# $storeid - storage ID of the backup storage.
> +#
> +# Returns the path to a directory containing the full filesystem structure of the container.
> +sub restore_directory_init {
> +    my ($self, $volname, $storeid) = @_;
> +
> +    die "implement me in subclass\n";
> +}
> +
> +# For LXC backups, clean up after the restore with 'directory' method. Called in both, success and
> +# failure scenarios.
> +#
> +# $volname - volume name of the backup being restored.
> +# $storeid - storage ID of the backup storage.
> +sub restore_directory_cleanup {
> +    my ($self, $volname, $storeid) = @_;
> +
> +    die "implement me in subclass\n";
> +}
> +
> +1;
> diff --git a/src/PVE/Makefile b/src/PVE/Makefile
> index d438804..8605a40 100644
> --- a/src/PVE/Makefile
> +++ b/src/PVE/Makefile
> @@ -5,6 +5,7 @@ install:
>  	install -D -m 0644 Storage.pm ${DESTDIR}${PERLDIR}/PVE/Storage.pm
>  	install -D -m 0644 Diskmanage.pm ${DESTDIR}${PERLDIR}/PVE/Diskmanage.pm
>  	install -D -m 0644 CephConfig.pm ${DESTDIR}${PERLDIR}/PVE/CephConfig.pm
> +	make -C BackupProvider install
>  	make -C Storage install
>  	make -C API2 install
>  	make -C CLI install
> diff --git a/src/PVE/Storage.pm b/src/PVE/Storage.pm
> index 57b2038..aea57ab 100755
> --- a/src/PVE/Storage.pm
> +++ b/src/PVE/Storage.pm
> @@ -42,11 +42,11 @@ use PVE::Storage::BTRFSPlugin;
>  use PVE::Storage::ESXiPlugin;
>  
>  # Storage API version. Increment it on changes in storage API interface.
> -use constant APIVER => 10;
> +use constant APIVER => 11;
>  # Age is the number of versions we're backward compatible with.
>  # This is like having 'current=APIVER' and age='APIAGE' in libtool,
>  # see https://www.gnu.org/software/libtool/manual/html_node/Libtool-versioning.html
> -use constant APIAGE => 1;
> +use constant APIAGE => 2;
>  
>  our $KNOWN_EXPORT_FORMATS = ['raw+size', 'tar+size', 'qcow2+size', 'vmdk+size', 'zfs', 'btrfs'];
>  
> @@ -1994,6 +1994,14 @@ sub volume_export_start {
>      PVE::Tools::run_command($cmds, %$run_command_params);
>  }
>  
> +sub new_backup_provider {
> +    my ($cfg, $storeid, $log_function) = @_;
> +
> +    my $scfg = storage_config($cfg, $storeid);
> +    my $plugin = PVE::Storage::Plugin->lookup($scfg->{type});
> +    return $plugin->new_backup_provider($scfg, $storeid, $log_function);
> +}
> +
>  # bash completion helper
>  
>  sub complete_storage {
> 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;
> +}
> +
>  sub config_aware_base_mkdir {
>      my ($class, $scfg, $path) = @_;
>  



_______________________________________________
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel