Re: [pve-devel] [RFC qemu-server v3 21/34] backup: implement backup for external providers

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

On 12.11.24 1:27 PM, Fabian Grünbichler wrote:
> On November 7, 2024 5:51 pm, Fiona Ebner wrote:
>> +    if ($waited == $wait_limit && scalar(keys $cpids->%*)) {
>> +	kill 9, $_ for keys $cpids->%*;
>> +	sleep 1;
>> +	while ((my $cpid = waitpid(-1, POSIX::WNOHANG)) > 0) {
> 
> this could be a bit dangerous, since we have an explicit list of cpids
> we want to wait for, couldn't we just waitpid explicitly for them?
> 
> just wary of potential side-effects on things like hookscripts or future
> features that also require forking ;)
> 

Will do!

>> +	    delete($cpids->{$cpid});
>> +	}
>> +	$self->log('warn', "unable to collect nbdinfo child process '$_'") for keys $cpids->%*;
>> +    }
>> +}
>> +
>> +my sub block_device_backup_prepare {
>> +    my ($self, $devicename, $size, $nbd_path, $bitmap_name, $count) = @_;
> 
> nit: $device_name for consistency's sake?
> 

Will rename, as well as in the API and backup provider plugins.

---snip---

>> +
>> +    my $blockdev = "/dev/nbd${count}";
> 
> what if that is already used/taken by somebody? I think we'd need logic
> to find a free slot here..
> 

Then the command will fail. I haven't found an obvious way to see
whether it is in-use yet, except interpreting a failure to mean that
(which it doesn't necessarily). We could go for that, but I'll take
another look if there is a better way.

>> +
>> +    eval {
>> +	run_command(["qemu-nbd", "-c", $blockdev, $qemu_nbd_uri, "--format=raw", "--discard=on"]);
>> +    };

---snip---

>> +    for my $info ($backup_access_info->@*) {
>> +	my $bitmap_status = 'none';
>> +	my $bitmap_name;
>> +	if (my $bitmap_action = $info->{'bitmap-action'}) {
>> +	    my $bitmap_action_to_status = {
>> +		'not-used' => 'none',
>> +		'not-used-removed' => 'none',
>> +		'new' => 'new',
>> +		'used' => 'reuse',
>> +		'invalid' => 'new',
>> +	    };
> 
> nit: should we move this outside of the loop? it's a static map after
> all.. (or maybe the perl interpreter is smart enough anyway ;))
> 

Ack.

---snip---

>> +
>> +	my $fs_frozen = $self->qga_fs_freeze($task, $vmid);
> 
> should we move this (A)
> 
>> +
>> +	my $target_id = $opts->{storage};
>> +
>> +	my $params = {
>> +	    'target-id' => $target_id,
>> +	    devlist => $devlist,
>> +	    timeout => 60,
>> +	};
> 
> and this (B)
> 
>> +
>> +	my ($mechanism, $bitmap_name) = $backup_provider->backup_get_mechanism($vmid, 'qemu');
>> +	die "mechanism '$mechanism' requested by backup provider is not supported for VMs\n"
>> +	    if $mechanism ne 'block-device' && $mechanism ne 'nbd';
>> +
>> +	if ($mechanism eq 'block-device') {
>> +	    # For mechanism 'block-device' the bitmap needs to be passed to the provider. The bitmap
>> +	    # cannot be dumped via QMP and doing it via qemu-img is experimental, so use nbdinfo.
>> +	    die "need 'nbdinfo' binary from package libnbd-bin\n" if !-e "/usr/bin/nbdinfo";
>> +
>> +	    # NOTE nbds_max won't change if module is already loaded
>> +	    run_command(["modprobe", "nbd", "nbds_max=128"]);
> 
> should this maybe be put into a modprobe snippet somewhere, and we just
> verify here that nbd is available? not that we can currently reach 128
> guest disks ;)
> 

Will look into it!

>> +	}
> 
> down here (B)
> 

Ack.

>> +
>> +	if ($bitmap_name) {
>> +	    # prepend storage ID so different providers can never cause clashes
>> +	    $bitmap_name = "$opts->{storage}-" . $bitmap_name;
>> +	    $params->{'bitmap-name'} = $bitmap_name;
> 
> not related to this patch directly - if we do this for external
> providers, do we also want to do it for different PBS targets maybe? :)
> 

Yes, I thought about that too. Will need a QEMU patch to support it with
the 'backup' QMP command, but don't see any real blocker :)

>> +	}
>> +
>> +	$self->loginfo("setting up snapshot-access for backup");
>> +
> 
> and down here (A)?

Agreed, but I'll move it before the log line ;)

>> +	my $backup_access_info = eval { mon_cmd($vmid, "backup-access-setup", $params->%*) };
>> +	my $qmperr = $@;
>> +
>> +	$task->{cleanup}->{'backup-access-teardown'} = { 'target-id' => $target_id, success => 0 };
> 
> should we differentiate here between setup success or failure? if not,
> should we move it directly before the setup call?
> 

No, there should be no differentiation. The teardown QMP command needs
to be called in any case. But how could it happen that we do reach
cleanup but haven't gotten through here after the setup call? The setup
call is in an eval and there is nothing that can die in between. I can
still move it if you feel that is cleaner.

--snip---

>> +	my $child_pids = {}; # used for nbdinfo calls
>> +	my $volumes = {};
>> +
>> +	eval {
>> +	    ($volumes, $child_pids) =
>> +		backup_access_to_volume_info($self, $backup_access_info, $mechanism, $nbd_path);
> 
> so this here forks child processes (via block_device_backup_prepare),
> but it might fail halfway through after having forked X/N children, then
> we don't have any information about the forked processes here (C)
> 
>> +
>> +	    my $param = {};
>> +	    $param->{'bandwidth-limit'} = $opts->{bwlimit} * 1024 if $opts->{bwlimit};
>> +	    $param->{'firewall-config'} = PVE::Tools::file_get_contents($firewall_file)
>> +		if -e $firewall_file;
>> +
>> +	    $backup_provider->backup_vm($vmid, $guest_config, $volumes, $param);
>> +	};
>> +	my $err = $@;
>> +
>> +	if ($mechanism eq 'block-device') {
>> +	    my $cleanup_paths = [map { $volumes->{$_}->{path} } keys $volumes->%*];
>> +	    block_device_backup_cleanup($self, $cleanup_paths, $child_pids)
> 
> C: to do this cleanup here.. should we maybe record both cpids and
> volumes as part of $self->{cleanup}, instead of returning them, so that
> we can handle that case as well?
> 

Good catch!


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