Re: [pve-devel] [PATCH qemu-server v7 6/9] api: enable live migration for marked mapped pci devices

[Dominik Csapak <d.csapak@proxmox.com>]

On 4/3/25 15:00, Thomas Lamprecht wrote:
> Am 11.03.25 um 14:20 schrieb Dominik Csapak:
>> They have to be marked as 'live-migration-capable' in the mapping
>> config, and the driver and qemu must support it.
>>
>> For the gui checks, we now return the whole object of the mapped
>> resources, which includes info like the name and if it's marked as
>> live-migration capable. (while deprecating the old 'mapped-resource'
>> return value, since that returns strictly less information)
>>
>> Reviewed-by: Christoph Heiss <c.heiss@proxmox.com>
>> Reviewed-by: Fiona Ebner <f.ebner@proxmox.com>
>> Signed-off-by: Dominik Csapak <d.csapak@proxmox.com>
>> ---
>>   PVE/API2/Qemu.pm   |  8 +++++++-
>>   PVE/QemuMigrate.pm | 17 ++++++++++++-----
>>   2 files changed, 19 insertions(+), 6 deletions(-)
>>
>> diff --git a/PVE/API2/Qemu.pm b/PVE/API2/Qemu.pm
>> index 060bca8b..15bf7548 100644
>> --- a/PVE/API2/Qemu.pm
>> +++ b/PVE/API2/Qemu.pm
>> @@ -4665,13 +4665,18 @@ __PACKAGE__->register_method({
>>   		},
>>   		description => "List local resources (e.g. pci, usb) that block migration."
>>   	    },
>> +	    # FIXME: remove with 9.0
>>   	    'mapped-resources' => {
>>   		type => 'array',
>>   		items => {
>>   		    type => 'string',
>>   		    description => "A mapped resource",
>>   		},
>> -		description => "List of mapped resources e.g. pci, usb"
>> +		description => "List of mapped resources e.g. pci, usb. Deprecated, use 'mapped-resource-info' instead."
>> +	    },
>> +	    'mapped-resource-info' => {
>> +		type => 'object',
>> +		description => "Object of mapped resources with additional information such if they're live migratable.",
> 
> Where is the schema/format for this? New stuff really should be documented
> here, I mean, have you not sent enough schema updates to unlock basic features
> in the rust based PVE api crate used in PDM? ;-)

Yes, you're right. I'll send a follow up for that if that's fine with you.

> 
>>   	    },
>>   	},
>>       },
>> @@ -4737,6 +4742,7 @@ __PACKAGE__->register_method({
>>   
>>   	$res->{local_resources} = $local_resources;
>>   	$res->{'mapped-resources'} = [ sort keys $mapped_resources->%* ];
>> +	$res->{'mapped-resource-info'} = $mapped_resources;
> 
> would be IMO nicer to add returning that info upfront and then combine this
> with patch with 4/9 to have two patches that each do one thing, and not a
> split/mix.
> 
> But I probably will just apply this as is with trying to bend the commit
> message from implementation details to the effect of the patch, will not
> be ideal, but I want to finish this finally..

If you want I can try to rework this part of the series, since it does make
sense like you described, but I'm not sure I could finish that today.

> 
> No offense to anybody, but IMO does not pain the best picture considering
> this has two R-b tags..
> 
>>   
>>   	return $res;
>>   
>> diff --git a/PVE/QemuMigrate.pm b/PVE/QemuMigrate.pm
>> index 2153ac42..6909fc82 100644
>> --- a/PVE/QemuMigrate.pm
>> +++ b/PVE/QemuMigrate.pm
>> @@ -243,11 +243,18 @@ sub prepare {
>>   
>>       if (scalar(keys $mapped_res->%*)) {
>>   	my $missing_mappings = $missing_mappings_by_node->{$self->{node}};
>> -	if ($running) {
>> -	    my $mapped_text = join(", ", sort keys $mapped_res->%*);
>> -	    die "can't migrate running VM which uses mapped devices: $mapped_text\n";
>> -	} elsif (scalar($missing_mappings->@*)) {
>> -	    die "can't migrate to '$self->{node}': missing mapped devices " . join(", ", $missing_mappings->@*) . "\n";
>> +	my $missing_live_mappings = [];
>> +	for my $key (sort keys $mapped_res->%*) {
>> +	    my $res = $mapped_res->{$key};
>> +	    my $name = "$key:$res->{name}";
>> +	    push $missing_live_mappings->@*, $name if !$res->{'live-migration'};
>> +	}
>> +	if (scalar($missing_mappings->@*)) {
>> +	    my $missing = join(", ", $missing_mappings->@*);
>> +	    die "can't migrate to '$self->{node}': missing mapped devices $missing\n";
>> +	} elsif ($running && scalar($missing_live_mappings->@*)) {
>> +	    my $missing = join(", ", $missing_live_mappings->@*);
>> +	    die "can't live migrate running VM which uses following mapped devices: $missing\n";
>>   	} else {
>>   	    $self->log('info', "migrating VM which uses mapped local devices");
>>   	}
> 



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