* [pve-devel] [PATCH common v6 1/1] sysfs tools: add 'nvidia' -> 'mdev' workaround to pci_device_info
2025-02-13 13:16 [pve-devel] [PATCH (guest-)common/qemu-server/manager/docs v6] implement Dominik Csapak
@ 2025-02-13 13:16 ` Dominik Csapak
2025-03-06 13:00 ` [pve-devel] applied: " Thomas Lamprecht
2025-02-13 13:16 ` [pve-devel] [PATCH guest-common v6 1/2] mapping: pci: check the mdev configuration on the device too Dominik Csapak
` (21 subsequent siblings)
22 siblings, 1 reply; 51+ messages in thread
From: Dominik Csapak @ 2025-02-13 13:16 UTC (permalink / raw)
To: pve-devel
we added it to the lspci one, but we'll also need it when querying
a single device
code is the same as in the lspci sub
Signed-off-by: Dominik Csapak <d.csapak@proxmox.com>
---
new in v6
src/PVE/SysFSTools.pm | 4 ++++
1 file changed, 4 insertions(+)
diff --git a/src/PVE/SysFSTools.pm b/src/PVE/SysFSTools.pm
index cf9854b..f7acb9a 100644
--- a/src/PVE/SysFSTools.pm
+++ b/src/PVE/SysFSTools.pm
@@ -280,6 +280,10 @@ sub pci_device_info {
if (-d "$devdir/mdev_supported_types") {
$res->{mdev} = 1;
+ } elsif (-d "$devdir/nvidia") {
+ # nvidia driver for kernel 6.8 or higher
+ $res->{mdev} = 1; # for api compatibility
+ $res->{nvidia} = 1;
}
}
--
2.39.5
_______________________________________________
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
^ permalink raw reply [flat|nested] 51+ messages in thread* [pve-devel] [PATCH guest-common v6 1/2] mapping: pci: check the mdev configuration on the device too
2025-02-13 13:16 [pve-devel] [PATCH (guest-)common/qemu-server/manager/docs v6] implement Dominik Csapak
2025-02-13 13:16 ` [pve-devel] [PATCH common v6 1/1] sysfs tools: add 'nvidia' -> 'mdev' workaround to pci_device_info Dominik Csapak
@ 2025-02-13 13:16 ` Dominik Csapak
2025-03-07 10:52 ` Fiona Ebner
2025-02-13 13:16 ` [pve-devel] [PATCH guest-common v6 2/2] mapping: pci: add 'live-migration-capable' flag to mappings Dominik Csapak
` (20 subsequent siblings)
22 siblings, 1 reply; 51+ messages in thread
From: Dominik Csapak @ 2025-02-13 13:16 UTC (permalink / raw)
To: pve-devel
but that lives int he 'global' part of the mapping config, not in a
specific mapping. To check that, add it to the $configured_props from
there.
this requires all call sites to be adapted otherwise the check will
always fail for devices that are capable of mediated devices
Signed-off-by: Dominik Csapak <d.csapak@proxmox.com>
---
no changes in v6
src/PVE/Mapping/PCI.pm | 8 +++++++-
1 file changed, 7 insertions(+), 1 deletion(-)
diff --git a/src/PVE/Mapping/PCI.pm b/src/PVE/Mapping/PCI.pm
index aa56496..cdd73d9 100644
--- a/src/PVE/Mapping/PCI.pm
+++ b/src/PVE/Mapping/PCI.pm
@@ -131,7 +131,7 @@ sub options {
# checks if the given config is valid for the current node
sub assert_valid {
- my ($name, $mapping) = @_;
+ my ($name, $mapping, $cluster_mapping_cfg) = @_;
my @paths = split(';', $mapping->{path} // '');
@@ -161,6 +161,12 @@ sub assert_valid {
my $configured_props = { $mapping->%{qw(id iommugroup subsystem-id)} };
+ # check mdev from globabl mapping config, if that is given
+ if (defined($cluster_mapping_cfg)) {
+ $expected_props->{mdev} = $info->{mdev} ? 1 : 0;
+ $configured_props->{mdev} = $cluster_mapping_cfg->{mdev} ? 1 : 0;
+ }
+
for my $prop (sort keys $expected_props->%*) {
next if $prop eq 'iommugroup' && $idx > 0; # check iommu only on the first device
--
2.39.5
_______________________________________________
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
^ permalink raw reply [flat|nested] 51+ messages in thread* [pve-devel] [PATCH guest-common v6 2/2] mapping: pci: add 'live-migration-capable' flag to mappings
2025-02-13 13:16 [pve-devel] [PATCH (guest-)common/qemu-server/manager/docs v6] implement Dominik Csapak
2025-02-13 13:16 ` [pve-devel] [PATCH common v6 1/1] sysfs tools: add 'nvidia' -> 'mdev' workaround to pci_device_info Dominik Csapak
2025-02-13 13:16 ` [pve-devel] [PATCH guest-common v6 1/2] mapping: pci: check the mdev configuration on the device too Dominik Csapak
@ 2025-02-13 13:16 ` Dominik Csapak
2025-03-07 10:52 ` Fiona Ebner
2025-02-13 13:17 ` [pve-devel] [PATCH qemu-server v6 01/10] vm stop-cleanup: allow callers to decide error behavior Dominik Csapak
` (19 subsequent siblings)
22 siblings, 1 reply; 51+ messages in thread
From: Dominik Csapak @ 2025-02-13 13:16 UTC (permalink / raw)
To: pve-devel
so that we can decide in qemu-server to allow live-migration.
The driver and QEMU must be capable of that, and it's the
admin's responsibility to know and configure that
Mark the option as experimental in the description.
Signed-off-by: Dominik Csapak <d.csapak@proxmox.com>
---
no changes in v6
src/PVE/Mapping/PCI.pm | 8 ++++++++
1 file changed, 8 insertions(+)
diff --git a/src/PVE/Mapping/PCI.pm b/src/PVE/Mapping/PCI.pm
index cdd73d9..3e93429 100644
--- a/src/PVE/Mapping/PCI.pm
+++ b/src/PVE/Mapping/PCI.pm
@@ -105,6 +105,13 @@ my $defaultData = {
optional => 1,
default => 0,
},
+ 'live-migration-capable' => {
+ description => "Marks the device(s) as being able to be live-migrated (Experimental)."
+ ." This needs hardware and driver support to work.",
+ type => 'boolean',
+ optional => 1,
+ default => 0,
+ },
map => {
type => 'array',
description => 'A list of maps for the cluster nodes.',
@@ -125,6 +132,7 @@ sub options {
return {
description => { optional => 1 },
mdev => { optional => 1 },
+ 'live-migration-capable' => { optional => 1 },
map => {},
};
}
--
2.39.5
_______________________________________________
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
^ permalink raw reply [flat|nested] 51+ messages in thread* [pve-devel] [PATCH qemu-server v6 01/10] vm stop-cleanup: allow callers to decide error behavior
2025-02-13 13:16 [pve-devel] [PATCH (guest-)common/qemu-server/manager/docs v6] implement Dominik Csapak
` (2 preceding siblings ...)
2025-02-13 13:16 ` [pve-devel] [PATCH guest-common v6 2/2] mapping: pci: add 'live-migration-capable' flag to mappings Dominik Csapak
@ 2025-02-13 13:17 ` Dominik Csapak
2025-03-06 16:42 ` [pve-devel] applied: " Fiona Ebner
2025-02-13 13:17 ` [pve-devel] [PATCH qemu-server v6 02/10] migrate: call vm_stop_cleanup after stopping in phase3_cleanup Dominik Csapak
` (18 subsequent siblings)
22 siblings, 1 reply; 51+ messages in thread
From: Dominik Csapak @ 2025-02-13 13:17 UTC (permalink / raw)
To: pve-devel
and keep it the same for all current callers as before by setting the
additional 'noerr' parameter to '1'.
Signed-off-by: Dominik Csapak <d.csapak@proxmox.com>
---
reordered in v6, was 4/11 in v5, no changes otherwise
PVE/CLI/qm.pm | 2 +-
PVE/QemuServer.pm | 13 ++++++++-----
2 files changed, 9 insertions(+), 6 deletions(-)
diff --git a/PVE/CLI/qm.pm b/PVE/CLI/qm.pm
index 4214a7ca..3e3a4c91 100755
--- a/PVE/CLI/qm.pm
+++ b/PVE/CLI/qm.pm
@@ -963,7 +963,7 @@ __PACKAGE__->register_method({
if (!$clean || $guest) {
# vm was shutdown from inside the guest or crashed, doing api cleanup
- PVE::QemuServer::vm_stop_cleanup($storecfg, $vmid, $conf, 0, 0);
+ PVE::QemuServer::vm_stop_cleanup($storecfg, $vmid, $conf, 0, 0, 1);
}
PVE::GuestHelpers::exec_hookscript($conf, $vmid, 'post-stop');
diff --git a/PVE/QemuServer.pm b/PVE/QemuServer.pm
index 808c0e1c..fc27d0e6 100644
--- a/PVE/QemuServer.pm
+++ b/PVE/QemuServer.pm
@@ -6109,7 +6109,7 @@ sub cleanup_pci_devices {
}
sub vm_stop_cleanup {
- my ($storecfg, $vmid, $conf, $keepActive, $apply_pending_changes) = @_;
+ my ($storecfg, $vmid, $conf, $keepActive, $apply_pending_changes, $noerr) = @_;
eval {
@@ -6135,7 +6135,10 @@ sub vm_stop_cleanup {
vmconfig_apply_pending($vmid, $conf, $storecfg) if $apply_pending_changes;
};
- warn $@ if $@; # avoid errors - just warn
+ if (my $err = $@) {
+ die $err if !$noerr;
+ warn $err;
+ }
}
# call only in locked context
@@ -6186,7 +6189,7 @@ sub _do_vm_stop {
die "VM quit/powerdown failed - got timeout\n";
}
} else {
- vm_stop_cleanup($storecfg, $vmid, $conf, $keepActive, 1) if $conf;
+ vm_stop_cleanup($storecfg, $vmid, $conf, $keepActive, 1, 1) if $conf;
return;
}
} else {
@@ -6217,7 +6220,7 @@ sub _do_vm_stop {
sleep 1;
}
- vm_stop_cleanup($storecfg, $vmid, $conf, $keepActive, 1) if $conf;
+ vm_stop_cleanup($storecfg, $vmid, $conf, $keepActive, 1, 1) if $conf;
}
# Note: use $nocheck to skip tests if VM configuration file exists.
@@ -6232,7 +6235,7 @@ sub vm_stop {
my $pid = check_running($vmid, $nocheck, $migratedfrom);
kill 15, $pid if $pid;
my $conf = PVE::QemuConfig->load_config($vmid, $migratedfrom);
- vm_stop_cleanup($storecfg, $vmid, $conf, $keepActive, 0);
+ vm_stop_cleanup($storecfg, $vmid, $conf, $keepActive, 0, 1);
return;
}
--
2.39.5
_______________________________________________
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
^ permalink raw reply [flat|nested] 51+ messages in thread* [pve-devel] [PATCH qemu-server v6 02/10] migrate: call vm_stop_cleanup after stopping in phase3_cleanup
2025-02-13 13:16 [pve-devel] [PATCH (guest-)common/qemu-server/manager/docs v6] implement Dominik Csapak
` (3 preceding siblings ...)
2025-02-13 13:17 ` [pve-devel] [PATCH qemu-server v6 01/10] vm stop-cleanup: allow callers to decide error behavior Dominik Csapak
@ 2025-02-13 13:17 ` Dominik Csapak
2025-03-06 16:42 ` [pve-devel] applied: " Fiona Ebner
2025-02-13 13:17 ` [pve-devel] [PATCH qemu-server v6 03/10] tests: cfg2cmd: fix mdev tests Dominik Csapak
` (17 subsequent siblings)
22 siblings, 1 reply; 51+ messages in thread
From: Dominik Csapak @ 2025-02-13 13:17 UTC (permalink / raw)
To: pve-devel
we currently only call deactivate_volumes, but we actually want to call
the whole vm_stop_cleanup, since that is not invoked by the vm_stop
above (we cannot parse the config anymore) and might do other cleanups
we also want to do (like mdev cleanup).
For this to work properly we have to clone the original config at the
beginning, since we might modify the volids.
Signed-off-by: Dominik Csapak <d.csapak@proxmox.com>
---
reordered in v6, was 5/11 in v5, no changes otherwise
PVE/QemuMigrate.pm | 12 ++++++------
test/MigrationTest/Shared.pm | 3 +++
2 files changed, 9 insertions(+), 6 deletions(-)
diff --git a/PVE/QemuMigrate.pm b/PVE/QemuMigrate.pm
index ed5ede30..c2e36334 100644
--- a/PVE/QemuMigrate.pm
+++ b/PVE/QemuMigrate.pm
@@ -5,6 +5,7 @@ use warnings;
use IO::File;
use IPC::Open2;
+use Storable qw(dclone);
use Time::HiRes qw( usleep );
use PVE::AccessControl;
@@ -1457,7 +1458,8 @@ sub phase3_cleanup {
my $tunnel = $self->{tunnel};
- my $sourcevollist = PVE::QemuServer::get_vm_volumes($conf);
+ # we'll need an unmodified copy of the config later for the cleanup
+ my $oldconf = dclone($conf);
if ($self->{volume_map} && !$self->{opts}->{remote}) {
my $target_drives = $self->{target_drive};
@@ -1588,12 +1590,10 @@ sub phase3_cleanup {
$self->{errors} = 1;
}
- # always deactivate volumes - avoid lvm LVs to be active on several nodes
- eval {
- PVE::Storage::deactivate_volumes($self->{storecfg}, $sourcevollist);
- };
+ # stop with nocheck does not do a cleanup, so do it here with the original config
+ eval { PVE::QemuServer::vm_stop_cleanup($self->{storecfg}, $vmid, $oldconf) };
if (my $err = $@) {
- $self->log('err', $err);
+ $self->log('err', "Cleanup after stopping VM failed - $err");
$self->{errors} = 1;
}
diff --git a/test/MigrationTest/Shared.pm b/test/MigrationTest/Shared.pm
index aa7203d1..e69bf84f 100644
--- a/test/MigrationTest/Shared.pm
+++ b/test/MigrationTest/Shared.pm
@@ -130,6 +130,9 @@ $qemu_server_module->mock(
clear_reboot_request => sub {
return 1;
},
+ vm_stop_cleanup => sub {
+ return;
+ },
get_efivars_size => sub {
return 128 * 1024;
},
--
2.39.5
_______________________________________________
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
^ permalink raw reply [flat|nested] 51+ messages in thread* [pve-devel] applied: [PATCH qemu-server v6 02/10] migrate: call vm_stop_cleanup after stopping in phase3_cleanup
2025-02-13 13:17 ` [pve-devel] [PATCH qemu-server v6 02/10] migrate: call vm_stop_cleanup after stopping in phase3_cleanup Dominik Csapak
@ 2025-03-06 16:42 ` Fiona Ebner
2025-03-07 11:02 ` Dominik Csapak
0 siblings, 1 reply; 51+ messages in thread
From: Fiona Ebner @ 2025-03-06 16:42 UTC (permalink / raw)
To: Proxmox VE development discussion, Dominik Csapak
Am 13.02.25 um 14:17 schrieb Dominik Csapak:
> we currently only call deactivate_volumes, but we actually want to call
> the whole vm_stop_cleanup, since that is not invoked by the vm_stop
> above (we cannot parse the config anymore) and might do other cleanups
> we also want to do (like mdev cleanup).
>
> For this to work properly we have to clone the original config at the
> beginning, since we might modify the volids.
>
> Signed-off-by: Dominik Csapak <d.csapak@proxmox.com>
applied, thanks!
It seems like other callers with nocheck are affected by this too?
Namely, live-restore upon failure and the remote_migration in some cases
(see the mtunnel API endpoint) also won't clean up the PCI devices etc.
IIUC, or am I missing something?
_______________________________________________
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
^ permalink raw reply [flat|nested] 51+ messages in thread
* Re: [pve-devel] applied: [PATCH qemu-server v6 02/10] migrate: call vm_stop_cleanup after stopping in phase3_cleanup
2025-03-06 16:42 ` [pve-devel] applied: " Fiona Ebner
@ 2025-03-07 11:02 ` Dominik Csapak
0 siblings, 0 replies; 51+ messages in thread
From: Dominik Csapak @ 2025-03-07 11:02 UTC (permalink / raw)
To: Fiona Ebner, Proxmox VE development discussion
On 3/6/25 17:42, Fiona Ebner wrote:
> Am 13.02.25 um 14:17 schrieb Dominik Csapak:
>> we currently only call deactivate_volumes, but we actually want to call
>> the whole vm_stop_cleanup, since that is not invoked by the vm_stop
>> above (we cannot parse the config anymore) and might do other cleanups
>> we also want to do (like mdev cleanup).
>>
>> For this to work properly we have to clone the original config at the
>> beginning, since we might modify the volids.
>>
>> Signed-off-by: Dominik Csapak <d.csapak@proxmox.com>
>
> applied, thanks!
>
> It seems like other callers with nocheck are affected by this too?
> Namely, live-restore upon failure and the remote_migration in some cases
> (see the mtunnel API endpoint) also won't clean up the PCI devices etc.
> IIUC, or am I missing something?
probably yes, I'll have to look, thanks for noticing that!
_______________________________________________
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
^ permalink raw reply [flat|nested] 51+ messages in thread
* [pve-devel] [PATCH qemu-server v6 03/10] tests: cfg2cmd: fix mdev tests
2025-02-13 13:16 [pve-devel] [PATCH (guest-)common/qemu-server/manager/docs v6] implement Dominik Csapak
` (4 preceding siblings ...)
2025-02-13 13:17 ` [pve-devel] [PATCH qemu-server v6 02/10] migrate: call vm_stop_cleanup after stopping in phase3_cleanup Dominik Csapak
@ 2025-02-13 13:17 ` Dominik Csapak
2025-03-07 11:20 ` [pve-devel] applied: " Fiona Ebner
2025-02-13 13:17 ` [pve-devel] [PATCH qemu-server v6 04/10] pci: mapping: check mdev config against hardware Dominik Csapak
` (16 subsequent siblings)
22 siblings, 1 reply; 51+ messages in thread
From: Dominik Csapak @ 2025-02-13 13:17 UTC (permalink / raw)
To: pve-devel
this will fail with the new checks for mdev when we don't have the
correct config.
namely a device that has mediated devices, should have 'mdev' set in the
mapping config
Signed-off-by: Dominik Csapak <d.csapak@proxmox.com>
---
reordered in v6, was 10/11 in v5, no changes otherwise
test/run_config2command_tests.pl | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/test/run_config2command_tests.pl b/test/run_config2command_tests.pl
index 2feebd4a..b2cda648 100755
--- a/test/run_config2command_tests.pl
+++ b/test/run_config2command_tests.pl
@@ -101,6 +101,7 @@ my $pci_map_config = {
ids => {
someGpu => {
type => 'pci',
+ mdev => 1,
map => [
'node=localhost,path=0000:01:00.4,id=10de:2231,iommugroup=1',
'node=localhost,path=0000:01:00.5,id=10de:2231,iommugroup=1',
@@ -323,7 +324,6 @@ $pve_common_sysfstools->mock(
} elsif ($path =~ m/^0000:07:10/) {
return {
iommugroup => 2,
- mdev => 0,
vendor => "0x8086",
device => "0x1520",
};
--
2.39.5
_______________________________________________
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
^ permalink raw reply [flat|nested] 51+ messages in thread* [pve-devel] applied: [PATCH qemu-server v6 03/10] tests: cfg2cmd: fix mdev tests
2025-02-13 13:17 ` [pve-devel] [PATCH qemu-server v6 03/10] tests: cfg2cmd: fix mdev tests Dominik Csapak
@ 2025-03-07 11:20 ` Fiona Ebner
2025-03-07 11:54 ` Fiona Ebner
0 siblings, 1 reply; 51+ messages in thread
From: Fiona Ebner @ 2025-03-07 11:20 UTC (permalink / raw)
To: Proxmox VE development discussion, Dominik Csapak
Am 13.02.25 um 14:17 schrieb Dominik Csapak:
> this will fail with the new checks for mdev when we don't have the
> correct config.
Nit: it would only fail after the next commit ;)
>
> namely a device that has mediated devices, should have 'mdev' set in the
> mapping config
>
> Signed-off-by: Dominik Csapak <d.csapak@proxmox.com>
Reviewed-by: Fiona Ebner <f.ebner@proxmox.com>
_______________________________________________
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
^ permalink raw reply [flat|nested] 51+ messages in thread
* Re: [pve-devel] applied: [PATCH qemu-server v6 03/10] tests: cfg2cmd: fix mdev tests
2025-03-07 11:20 ` [pve-devel] applied: " Fiona Ebner
@ 2025-03-07 11:54 ` Fiona Ebner
0 siblings, 0 replies; 51+ messages in thread
From: Fiona Ebner @ 2025-03-07 11:54 UTC (permalink / raw)
To: Proxmox VE development discussion, Dominik Csapak
Oh sorry, messed up the Subject, I briefly thought about applying it
already, but it should come together with the dependency bump for
guest-common, so I didn't yet.
Am 07.03.25 um 12:20 schrieb Fiona Ebner:
> Am 13.02.25 um 14:17 schrieb Dominik Csapak:
>> this will fail with the new checks for mdev when we don't have the
>> correct config.
>
> Nit: it would only fail after the next commit ;)
>
>>
>> namely a device that has mediated devices, should have 'mdev' set in the
>> mapping config
>>
>> Signed-off-by: Dominik Csapak <d.csapak@proxmox.com>
>
> Reviewed-by: Fiona Ebner <f.ebner@proxmox.com>
_______________________________________________
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
^ permalink raw reply [flat|nested] 51+ messages in thread
* [pve-devel] [PATCH qemu-server v6 04/10] pci: mapping: check mdev config against hardware
2025-02-13 13:16 [pve-devel] [PATCH (guest-)common/qemu-server/manager/docs v6] implement Dominik Csapak
` (5 preceding siblings ...)
2025-02-13 13:17 ` [pve-devel] [PATCH qemu-server v6 03/10] tests: cfg2cmd: fix mdev tests Dominik Csapak
@ 2025-02-13 13:17 ` Dominik Csapak
2025-03-07 11:20 ` Fiona Ebner
2025-02-13 13:17 ` [pve-devel] [PATCH qemu-server v6 05/10] pci: set 'enable-migration' to on for live-migration marked mapped devices Dominik Csapak
` (15 subsequent siblings)
22 siblings, 1 reply; 51+ messages in thread
From: Dominik Csapak @ 2025-02-13 13:17 UTC (permalink / raw)
To: pve-devel
by giving the mapping config to assert_valid, not only the specific mapping
Signed-off-by: Dominik Csapak <d.csapak@proxmox.com>
---
changes in v6:
* add `my $config ...` line since that was introduced in a different
patch in v5 that was dropped with v6
depends on changes from pve-guest-common
PVE/QemuServer/PCI.pm | 4 +++-
1 file changed, 3 insertions(+), 1 deletion(-)
diff --git a/PVE/QemuServer/PCI.pm b/PVE/QemuServer/PCI.pm
index d758ae9d..a0d99692 100644
--- a/PVE/QemuServer/PCI.pm
+++ b/PVE/QemuServer/PCI.pm
@@ -432,8 +432,10 @@ sub parse_hostpci {
my $devices = PVE::Mapping::PCI::find_on_current_node($mapping);
die "PCI device mapping not found for '$mapping'\n" if !$devices || !scalar($devices->@*);
+ my $config = PVE::Mapping::PCI::config();
+
for my $device ($devices->@*) {
- eval { PVE::Mapping::PCI::assert_valid($mapping, $device) };
+ eval { PVE::Mapping::PCI::assert_valid($mapping, $device, $config->{ids}->{$mapping}) };
die "PCI device mapping invalid (hardware probably changed): $@\n" if $@;
push $alternatives->@*, [split(/;/, $device->{path})];
}
--
2.39.5
_______________________________________________
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
^ permalink raw reply [flat|nested] 51+ messages in thread* [pve-devel] [PATCH qemu-server v6 05/10] pci: set 'enable-migration' to on for live-migration marked mapped devices
2025-02-13 13:16 [pve-devel] [PATCH (guest-)common/qemu-server/manager/docs v6] implement Dominik Csapak
` (6 preceding siblings ...)
2025-02-13 13:17 ` [pve-devel] [PATCH qemu-server v6 04/10] pci: mapping: check mdev config against hardware Dominik Csapak
@ 2025-02-13 13:17 ` Dominik Csapak
2025-03-07 11:20 ` Fiona Ebner
2025-02-13 13:17 ` [pve-devel] [PATCH qemu-server v6 06/10] check_local_resources: add more info per mapped device and return as hash Dominik Csapak
` (14 subsequent siblings)
22 siblings, 1 reply; 51+ messages in thread
From: Dominik Csapak @ 2025-02-13 13:17 UTC (permalink / raw)
To: pve-devel
the default is 'auto', but for those which are marked as capable for
live migration, we want to explicitly enable that, so we get an early
error on start if the driver does not support that.
Signed-off-by: Dominik Csapak <d.csapak@proxmox.com>
---
no changes in v6
PVE/QemuServer/PCI.pm | 8 +++++++-
1 file changed, 7 insertions(+), 1 deletion(-)
diff --git a/PVE/QemuServer/PCI.pm b/PVE/QemuServer/PCI.pm
index a0d99692..6bde248b 100644
--- a/PVE/QemuServer/PCI.pm
+++ b/PVE/QemuServer/PCI.pm
@@ -433,9 +433,11 @@ sub parse_hostpci {
die "PCI device mapping not found for '$mapping'\n" if !$devices || !scalar($devices->@*);
my $config = PVE::Mapping::PCI::config();
+ my $cfg = $config->{ids}->{$mapping};
+ $res->{'live-migration-capable'} = 1 if $cfg->{'live-migration-capable'};
for my $device ($devices->@*) {
- eval { PVE::Mapping::PCI::assert_valid($mapping, $device, $config->{ids}->{$mapping}) };
+ eval { PVE::Mapping::PCI::assert_valid($mapping, $device, $cfg) };
die "PCI device mapping invalid (hardware probably changed): $@\n" if $@;
push $alternatives->@*, [split(/;/, $device->{path})];
}
@@ -692,6 +694,10 @@ sub print_hostpci_devices {
$devicestr .= ",host=$pcidevice->{id}";
}
+ if ($d->{'live-migration-capable'}) {
+ $devicestr .= ",enable-migration=on"
+ }
+
my $mf_addr = $multifunction ? ".$j" : '';
$devicestr .= ",id=${id}${mf_addr}${pciaddr}${mf_addr}";
--
2.39.5
_______________________________________________
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
^ permalink raw reply [flat|nested] 51+ messages in thread* Re: [pve-devel] [PATCH qemu-server v6 05/10] pci: set 'enable-migration' to on for live-migration marked mapped devices
2025-02-13 13:17 ` [pve-devel] [PATCH qemu-server v6 05/10] pci: set 'enable-migration' to on for live-migration marked mapped devices Dominik Csapak
@ 2025-03-07 11:20 ` Fiona Ebner
0 siblings, 0 replies; 51+ messages in thread
From: Fiona Ebner @ 2025-03-07 11:20 UTC (permalink / raw)
To: Proxmox VE development discussion, Dominik Csapak
Am 13.02.25 um 14:17 schrieb Dominik Csapak:
> the default is 'auto', but for those which are marked as capable for
> live migration, we want to explicitly enable that, so we get an early
> error on start if the driver does not support that.
>
> Signed-off-by: Dominik Csapak <d.csapak@proxmox.com>
Reviewed-by: Fiona Ebner <f.ebner@proxmox.com>
> ---
> no changes in v6
> PVE/QemuServer/PCI.pm | 8 +++++++-
> 1 file changed, 7 insertions(+), 1 deletion(-)
>
> diff --git a/PVE/QemuServer/PCI.pm b/PVE/QemuServer/PCI.pm
> index a0d99692..6bde248b 100644
> --- a/PVE/QemuServer/PCI.pm
> +++ b/PVE/QemuServer/PCI.pm
> @@ -433,9 +433,11 @@ sub parse_hostpci {
> die "PCI device mapping not found for '$mapping'\n" if !$devices || !scalar($devices->@*);
>
> my $config = PVE::Mapping::PCI::config();
> + my $cfg = $config->{ids}->{$mapping};
Style nit: bit confusing to have such similarly named variables
> + $res->{'live-migration-capable'} = 1 if $cfg->{'live-migration-capable'};
>
> for my $device ($devices->@*) {
> - eval { PVE::Mapping::PCI::assert_valid($mapping, $device, $config->{ids}->{$mapping}) };
> + eval { PVE::Mapping::PCI::assert_valid($mapping, $device, $cfg) };
> die "PCI device mapping invalid (hardware probably changed): $@\n" if $@;
> push $alternatives->@*, [split(/;/, $device->{path})];
> }
> @@ -692,6 +694,10 @@ sub print_hostpci_devices {
> $devicestr .= ",host=$pcidevice->{id}";
> }
>
> + if ($d->{'live-migration-capable'}) {
> + $devicestr .= ",enable-migration=on"
Style nit: missing semicolon
> + }
> +
> my $mf_addr = $multifunction ? ".$j" : '';
> $devicestr .= ",id=${id}${mf_addr}${pciaddr}${mf_addr}";
>
_______________________________________________
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
^ permalink raw reply [flat|nested] 51+ messages in thread
* [pve-devel] [PATCH qemu-server v6 06/10] check_local_resources: add more info per mapped device and return as hash
2025-02-13 13:16 [pve-devel] [PATCH (guest-)common/qemu-server/manager/docs v6] implement Dominik Csapak
` (7 preceding siblings ...)
2025-02-13 13:17 ` [pve-devel] [PATCH qemu-server v6 05/10] pci: set 'enable-migration' to on for live-migration marked mapped devices Dominik Csapak
@ 2025-02-13 13:17 ` Dominik Csapak
2025-03-07 11:20 ` Fiona Ebner
2025-02-13 13:17 ` [pve-devel] [PATCH qemu-server v6 07/10] api: enable live migration for marked mapped pci devices Dominik Csapak
` (13 subsequent siblings)
22 siblings, 1 reply; 51+ messages in thread
From: Dominik Csapak @ 2025-02-13 13:17 UTC (permalink / raw)
To: pve-devel
such as the mapping name and if it's marked for live-migration (pci only)
Signed-off-by: Dominik Csapak <d.csapak@proxmox.com>
---
no changes in v6
PVE/API2/Qemu.pm | 2 +-
PVE/QemuMigrate.pm | 7 ++++---
PVE/QemuServer.pm | 17 ++++++++++-------
3 files changed, 15 insertions(+), 11 deletions(-)
diff --git a/PVE/API2/Qemu.pm b/PVE/API2/Qemu.pm
index 295260e7..41dd8812 100644
--- a/PVE/API2/Qemu.pm
+++ b/PVE/API2/Qemu.pm
@@ -4730,7 +4730,7 @@ __PACKAGE__->register_method({
$res->{local_disks} = [ values %$local_disks ];;
$res->{local_resources} = $local_resources;
- $res->{'mapped-resources'} = $mapped_resources;
+ $res->{'mapped-resources'} = [ sort keys $mapped_resources->%* ];
return $res;
diff --git a/PVE/QemuMigrate.pm b/PVE/QemuMigrate.pm
index c2e36334..2153ac42 100644
--- a/PVE/QemuMigrate.pm
+++ b/PVE/QemuMigrate.pm
@@ -229,7 +229,7 @@ sub prepare {
my ($loc_res, $mapped_res, $missing_mappings_by_node) = PVE::QemuServer::check_local_resources($conf, $running, 1);
my $blocking_resources = [];
for my $res ($loc_res->@*) {
- if (!grep($res, $mapped_res->@*)) {
+ if (!defined($mapped_res->{$res})) {
push $blocking_resources->@*, $res;
}
}
@@ -241,10 +241,11 @@ sub prepare {
}
}
- if (scalar($mapped_res->@*)) {
+ if (scalar(keys $mapped_res->%*)) {
my $missing_mappings = $missing_mappings_by_node->{$self->{node}};
if ($running) {
- die "can't migrate running VM which uses mapped devices: " . join(", ", $mapped_res->@*) . "\n";
+ 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";
} else {
diff --git a/PVE/QemuServer.pm b/PVE/QemuServer.pm
index fc27d0e6..f3497583 100644
--- a/PVE/QemuServer.pm
+++ b/PVE/QemuServer.pm
@@ -2471,7 +2471,7 @@ sub check_local_resources {
my ($conf, $state, $noerr) = @_;
my @loc_res = ();
- my $mapped_res = [];
+ my $mapped_res = {};
my @non_migratable_resources = check_non_migratable_resources($conf, $state, $noerr);
push(@loc_res, @non_migratable_resources);
@@ -2506,16 +2506,19 @@ sub check_local_resources {
if ($k =~ m/^usb/) {
my $entry = parse_property_string('pve-qm-usb', $conf->{$k});
next if $entry->{host} && $entry->{host} =~ m/^spice$/i;
- if ($entry->{mapping}) {
- $add_missing_mapping->('usb', $k, $entry->{mapping});
- push @$mapped_res, $k;
+ if (my $name = $entry->{mapping}) {
+ $add_missing_mapping->('usb', $k, $name);
+ $mapped_res->{$k} = { name => $name };
}
}
if ($k =~ m/^hostpci/) {
my $entry = parse_property_string('pve-qm-hostpci', $conf->{$k});
- if ($entry->{mapping}) {
- $add_missing_mapping->('pci', $k, $entry->{mapping});
- push @$mapped_res, $k;
+ if (my $name = $entry->{mapping}) {
+ $add_missing_mapping->('pci', $k, $name);
+ my $mapped_device = { name => $name };
+ $mapped_device->{'live-migration'} = 1
+ if $pci_map->{ids}->{$name}->{'live-migration-capable'};
+ $mapped_res->{$k} = $mapped_device;
}
}
# sockets are safe: they will recreated be on the target side post-migrate
--
2.39.5
_______________________________________________
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
^ permalink raw reply [flat|nested] 51+ messages in thread* [pve-devel] [PATCH qemu-server v6 07/10] api: enable live migration for marked mapped pci devices
2025-02-13 13:16 [pve-devel] [PATCH (guest-)common/qemu-server/manager/docs v6] implement Dominik Csapak
` (8 preceding siblings ...)
2025-02-13 13:17 ` [pve-devel] [PATCH qemu-server v6 06/10] check_local_resources: add more info per mapped device and return as hash Dominik Csapak
@ 2025-02-13 13:17 ` Dominik Csapak
2025-03-07 11:20 ` Fiona Ebner
2025-02-13 13:17 ` [pve-devel] [PATCH qemu-server v6 08/10] api: include not mapped resources for running vms in migrate preconditions Dominik Csapak
` (12 subsequent siblings)
22 siblings, 1 reply; 51+ messages in thread
From: Dominik Csapak @ 2025-02-13 13:17 UTC (permalink / raw)
To: pve-devel
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)
Signed-off-by: Dominik Csapak <d.csapak@proxmox.com>
---
no changes in v6
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 41dd8812..1ed0d3dc 100644
--- a/PVE/API2/Qemu.pm
+++ b/PVE/API2/Qemu.pm
@@ -4659,13 +4659,18 @@ __PACKAGE__->register_method({
},
description => "List local resources e.g. pci, usb"
},
+ # 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.",
},
},
},
@@ -4731,6 +4736,7 @@ __PACKAGE__->register_method({
$res->{local_resources} = $local_resources;
$res->{'mapped-resources'} = [ sort keys $mapped_resources->%* ];
+ $res->{'mapped-resource-info'} = $mapped_resources;
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");
}
--
2.39.5
_______________________________________________
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
^ permalink raw reply [flat|nested] 51+ messages in thread* [pve-devel] [PATCH qemu-server v6 08/10] api: include not mapped resources for running vms in migrate preconditions
2025-02-13 13:16 [pve-devel] [PATCH (guest-)common/qemu-server/manager/docs v6] implement Dominik Csapak
` (9 preceding siblings ...)
2025-02-13 13:17 ` [pve-devel] [PATCH qemu-server v6 07/10] api: enable live migration for marked mapped pci devices Dominik Csapak
@ 2025-02-13 13:17 ` Dominik Csapak
2025-03-07 12:20 ` Fiona Ebner
2025-02-13 13:17 ` [pve-devel] [PATCH qemu-server v6 09/10] migrate: show vfio state transferred too Dominik Csapak
` (11 subsequent siblings)
22 siblings, 1 reply; 51+ messages in thread
From: Dominik Csapak @ 2025-02-13 13:17 UTC (permalink / raw)
To: pve-devel
so that we can show a proper warning in the migrate dialog and check it
in the bulk migrate precondition check
the unavailable_storages and should be the same as before, but
we now always return (not_)allowed_nodes too.
to make the code a bit easier to read, reorganize how we construct
the (not_)allowed_nodes properties.
also add a note that we want to redesign the return values here, to make
* the api call simpler
* return better structured values
Signed-off-by: Dominik Csapak <d.csapak@proxmox.com>
---
no changes in v6
PVE/API2/Qemu.pm | 45 ++++++++++++++++++++++++++-------------------
1 file changed, 26 insertions(+), 19 deletions(-)
diff --git a/PVE/API2/Qemu.pm b/PVE/API2/Qemu.pm
index 1ed0d3dc..6a303337 100644
--- a/PVE/API2/Qemu.pm
+++ b/PVE/API2/Qemu.pm
@@ -4595,6 +4595,8 @@ __PACKAGE__->register_method({
},
},
returns => {
+ # TODO 9.x: rework the api call to return more sensible structures
+ # e.g. a simple list of nodes with their blockers and/or notices to show
type => "object",
properties => {
running => {
@@ -4608,7 +4610,7 @@ __PACKAGE__->register_method({
description => "An allowed node",
},
optional => 1,
- description => "List nodes allowed for offline migration, only passed if VM is offline"
+ description => "List nodes allowed for migration.",
},
not_allowed_nodes => {
type => 'object',
@@ -4624,7 +4626,7 @@ __PACKAGE__->register_method({
},
},
},
- description => "List not allowed nodes with additional information, only passed if VM is offline"
+ description => "List not allowed nodes with additional information.",
},
local_disks => {
type => 'array',
@@ -4702,7 +4704,6 @@ __PACKAGE__->register_method({
my ($local_resources, $mapped_resources, $missing_mappings_by_node) =
PVE::QemuServer::check_local_resources($vmconf, $res->{running}, 1);
- delete $missing_mappings_by_node->{$localnode};
my $vga = PVE::QemuServer::parse_vga($vmconf->{vga});
if ($res->{running} && $vga->{'clipboard'} && $vga->{'clipboard'} eq 'vnc') {
@@ -4711,28 +4712,34 @@ __PACKAGE__->register_method({
# if vm is not running, return target nodes where local storage/mapped devices are available
# for offline migration
- if (!$res->{running}) {
- $res->{allowed_nodes} = [];
- my $checked_nodes = PVE::QemuServer::check_local_storage_availability($vmconf, $storecfg);
- delete $checked_nodes->{$localnode};
-
- foreach my $node (keys %$checked_nodes) {
- my $missing_mappings = $missing_mappings_by_node->{$node};
- if (scalar($missing_mappings->@*)) {
- $checked_nodes->{$node}->{'unavailable-resources'} = $missing_mappings;
- next;
- }
+ $res->{allowed_nodes} = [];
+ $res->{not_allowed_nodes} = {};
- if (!defined($checked_nodes->{$node}->{unavailable_storages})) {
- push @{$res->{allowed_nodes}}, $node;
- }
+ my $storage_nodehash = PVE::QemuServer::check_local_storage_availability($vmconf, $storecfg);
+
+ my $nodelist = PVE::Cluster::get_nodelist();
+ for my $node ($nodelist->@*) {
+ next if $node eq $localnode;
+
+ # extract missing storage info
+ if (my $storage_info = $storage_nodehash->{$node}) {
+ $res->{not_allowed_nodes}->{$node} = $storage_info;
+ }
+
+ # extract missing mappings info
+ my $missing_mappings = $missing_mappings_by_node->{$node};
+ if (scalar($missing_mappings->@*)) {
+ $res->{not_allowed_nodes}->{$node}->{'unavailable-resources'} = $missing_mappings;
+ }
+ # if nothing came up, add it to the allowed nodes
+ if (!$res->{not_allowed_nodes}->{$node}) {
+ push $res->{allowed_nodes}->@*, $node;
}
- $res->{not_allowed_nodes} = $checked_nodes;
}
my $local_disks = &$check_vm_disks_local($storecfg, $vmconf, $vmid);
- $res->{local_disks} = [ values %$local_disks ];;
+ $res->{local_disks} = [ values %$local_disks ];
$res->{local_resources} = $local_resources;
$res->{'mapped-resources'} = [ sort keys $mapped_resources->%* ];
--
2.39.5
_______________________________________________
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
^ permalink raw reply [flat|nested] 51+ messages in thread* Re: [pve-devel] [PATCH qemu-server v6 08/10] api: include not mapped resources for running vms in migrate preconditions
2025-02-13 13:17 ` [pve-devel] [PATCH qemu-server v6 08/10] api: include not mapped resources for running vms in migrate preconditions Dominik Csapak
@ 2025-03-07 12:20 ` Fiona Ebner
2025-03-07 12:56 ` Fiona Ebner
0 siblings, 1 reply; 51+ messages in thread
From: Fiona Ebner @ 2025-03-07 12:20 UTC (permalink / raw)
To: Proxmox VE development discussion, Dominik Csapak
Am 13.02.25 um 14:17 schrieb Dominik Csapak:
> so that we can show a proper warning in the migrate dialog and check it
> in the bulk migrate precondition check
>
> the unavailable_storages and should be the same as before, but
> we now always return (not_)allowed_nodes too.
What do you mean by "unavailable_storages"?
Maybe add a quick rationale for why it's okay to always return the node
lists, i.e.: API consumers that did not check the node lists for online
migration will not break (e.g. our UI without patches) and API consumers
that already tried to check the node lists for online migration too will
just get more accurate information.
> to make the code a bit easier to read, reorganize how we construct
> the (not_)allowed_nodes properties.
Yes, it's very nice now :)
> also add a note that we want to redesign the return values here, to make
> * the api call simpler
> * return better structured values
>
> Signed-off-by: Dominik Csapak <d.csapak@proxmox.com>
With the outdated comment removed/adapted and the hunk from the next
patch squashed in (see below):
Reviewed-by: Fiona Ebner <f.ebner@proxmox.com>
> ---
> no changes in v6
> PVE/API2/Qemu.pm | 45 ++++++++++++++++++++++++++-------------------
> 1 file changed, 26 insertions(+), 19 deletions(-)
>
> diff --git a/PVE/API2/Qemu.pm b/PVE/API2/Qemu.pm
> index 1ed0d3dc..6a303337 100644
> --- a/PVE/API2/Qemu.pm
> +++ b/PVE/API2/Qemu.pm
> @@ -4595,6 +4595,8 @@ __PACKAGE__->register_method({
> },
> },
> returns => {
> + # TODO 9.x: rework the api call to return more sensible structures
> + # e.g. a simple list of nodes with their blockers and/or notices to show
> type => "object",
> properties => {
> running => {
> @@ -4608,7 +4610,7 @@ __PACKAGE__->register_method({
> description => "An allowed node",
> },
> optional => 1,
> - description => "List nodes allowed for offline migration, only passed if VM is offline"
> + description => "List nodes allowed for migration.",
Nit (pre-existing): "List of nodes ..."
> },
> not_allowed_nodes => {
> type => 'object',
> @@ -4624,7 +4626,7 @@ __PACKAGE__->register_method({
> },
> },
> },
> - description => "List not allowed nodes with additional information, only passed if VM is offline"
> + description => "List not allowed nodes with additional information.",
Nit (pre-existing): "List of not ..."
> },
> local_disks => {
> type => 'array',
> @@ -4702,7 +4704,6 @@ __PACKAGE__->register_method({
>
> my ($local_resources, $mapped_resources, $missing_mappings_by_node) =
> PVE::QemuServer::check_local_resources($vmconf, $res->{running}, 1);
> - delete $missing_mappings_by_node->{$localnode};
>
> my $vga = PVE::QemuServer::parse_vga($vmconf->{vga});
> if ($res->{running} && $vga->{'clipboard'} && $vga->{'clipboard'} eq 'vnc') {
> @@ -4711,28 +4712,34 @@ __PACKAGE__->register_method({
>
> # if vm is not running, return target nodes where local storage/mapped devices are available
> # for offline migration
This comment is outdated.
> - if (!$res->{running}) {
> - $res->{allowed_nodes} = [];
> - my $checked_nodes = PVE::QemuServer::check_local_storage_availability($vmconf, $storecfg);
> - delete $checked_nodes->{$localnode};
> -
> - foreach my $node (keys %$checked_nodes) {
> - my $missing_mappings = $missing_mappings_by_node->{$node};
> - if (scalar($missing_mappings->@*)) {
> - $checked_nodes->{$node}->{'unavailable-resources'} = $missing_mappings;
> - next;
> - }
> + $res->{allowed_nodes} = [];
> + $res->{not_allowed_nodes} = {};
>
> - if (!defined($checked_nodes->{$node}->{unavailable_storages})) {
> - push @{$res->{allowed_nodes}}, $node;
> - }
> + my $storage_nodehash = PVE::QemuServer::check_local_storage_availability($vmconf, $storecfg);
> +
> + my $nodelist = PVE::Cluster::get_nodelist();
> + for my $node ($nodelist->@*) {
> + next if $node eq $localnode;
> +
> + # extract missing storage info
> + if (my $storage_info = $storage_nodehash->{$node}) {
> + $res->{not_allowed_nodes}->{$node} = $storage_info;
> + }
> +
> + # extract missing mappings info
> + my $missing_mappings = $missing_mappings_by_node->{$node};
> + if (scalar($missing_mappings->@*)) {
> + $res->{not_allowed_nodes}->{$node}->{'unavailable-resources'} = $missing_mappings;
> + }
>
> + # if nothing came up, add it to the allowed nodes
> + if (!$res->{not_allowed_nodes}->{$node}) {
There's a hunk in the next patch improving the readability here that
could/should be squashed in.
> + push $res->{allowed_nodes}->@*, $node;
> }
> - $res->{not_allowed_nodes} = $checked_nodes;
> }
>
> my $local_disks = &$check_vm_disks_local($storecfg, $vmconf, $vmid);
> - $res->{local_disks} = [ values %$local_disks ];;
> + $res->{local_disks} = [ values %$local_disks ];
>
> $res->{local_resources} = $local_resources;
> $res->{'mapped-resources'} = [ sort keys $mapped_resources->%* ];
_______________________________________________
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
^ permalink raw reply [flat|nested] 51+ messages in thread* Re: [pve-devel] [PATCH qemu-server v6 08/10] api: include not mapped resources for running vms in migrate preconditions
2025-03-07 12:20 ` Fiona Ebner
@ 2025-03-07 12:56 ` Fiona Ebner
0 siblings, 0 replies; 51+ messages in thread
From: Fiona Ebner @ 2025-03-07 12:56 UTC (permalink / raw)
To: Proxmox VE development discussion, Dominik Csapak
Am 07.03.25 um 13:20 schrieb Fiona Ebner:
> Am 13.02.25 um 14:17 schrieb Dominik Csapak:
>> so that we can show a proper warning in the migrate dialog and check it
>> in the bulk migrate precondition check
>>
>> the unavailable_storages and should be the same as before, but
>> we now always return (not_)allowed_nodes too.
>
> What do you mean by "unavailable_storages"?
Nevermind this, that was just my tunnel vision, because it's not
explicitly mentioned in the function anymore :P
_______________________________________________
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
^ permalink raw reply [flat|nested] 51+ messages in thread
* [pve-devel] [PATCH qemu-server v6 09/10] migrate: show vfio state transferred too
2025-02-13 13:16 [pve-devel] [PATCH (guest-)common/qemu-server/manager/docs v6] implement Dominik Csapak
` (10 preceding siblings ...)
2025-02-13 13:17 ` [pve-devel] [PATCH qemu-server v6 08/10] api: include not mapped resources for running vms in migrate preconditions Dominik Csapak
@ 2025-02-13 13:17 ` Dominik Csapak
2025-03-07 12:35 ` Fiona Ebner
2025-02-13 13:17 ` [pve-devel] [PATCH qemu-server v6 10/10] migrate: add transfer summary Dominik Csapak
` (10 subsequent siblings)
22 siblings, 1 reply; 51+ messages in thread
From: Dominik Csapak @ 2025-02-13 13:17 UTC (permalink / raw)
To: pve-devel
Show the transferred VFIO state (when there is one), but since there is
no total here, so we can't show that, just what was transferred up until
now.
Signed-off-by: Dominik Csapak <d.csapak@proxmox.com>
---
no changes in v6
PVE/API2/Qemu.pm | 2 +-
PVE/QemuMigrate.pm | 12 +++++++++++-
2 files changed, 12 insertions(+), 2 deletions(-)
diff --git a/PVE/API2/Qemu.pm b/PVE/API2/Qemu.pm
index 6a303337..8e744107 100644
--- a/PVE/API2/Qemu.pm
+++ b/PVE/API2/Qemu.pm
@@ -4733,7 +4733,7 @@ __PACKAGE__->register_method({
}
# if nothing came up, add it to the allowed nodes
- if (!$res->{not_allowed_nodes}->{$node}) {
+ if (scalar($res->{not_allowed_nodes}->{$node}->%*) == 0) {
push $res->{allowed_nodes}->@*, $node;
}
}
diff --git a/PVE/QemuMigrate.pm b/PVE/QemuMigrate.pm
index 6909fc82..d6f9132b 100644
--- a/PVE/QemuMigrate.pm
+++ b/PVE/QemuMigrate.pm
@@ -1241,6 +1241,7 @@ sub phase2 {
$self->log('info', "migrate uri => $migrate_uri failed: $merr") if $merr;
my $last_mem_transferred = 0;
+ my $last_vfio_transferred = 0;
my $usleep = 1000000;
my $i = 0;
my $err_count = 0;
@@ -1300,8 +1301,11 @@ sub phase2 {
last;
}
- if ($memstat->{transferred} ne $last_mem_transferred) {
+ if ($memstat->{transferred} ne $last_mem_transferred ||
+ $stat->{vfio}->{transferred} ne $last_vfio_transferred
+ ) {
my $trans = $memstat->{transferred} || 0;
+ my $vfio_transferred = $stat->{vfio}->{transferred} || 0;
my $rem = $memstat->{remaining} || 0;
my $total = $memstat->{total} || 0;
my $speed = ($memstat->{'pages-per-second'} // 0) * ($memstat->{'page-size'} // 0);
@@ -1319,6 +1323,11 @@ sub phase2 {
my $progress = "transferred $transferred_h of $total_h VM-state, ${speed_h}/s";
+ if ($vfio_transferred > 0) {
+ my $vfio_h = render_bytes($vfio_transferred, 1);
+ $progress .= " (+ $vfio_h VFIO-state)";
+ }
+
if ($dirty_rate > $speed) {
my $dirty_rate_h = render_bytes($dirty_rate, 1);
$progress .= ", VM dirties lots of memory: $dirty_rate_h/s";
@@ -1360,6 +1369,7 @@ sub phase2 {
}
$last_mem_transferred = $memstat->{transferred};
+ $last_vfio_transferred = $stat->{vfio}->{transferred};
}
if ($self->{storage_migration}) {
--
2.39.5
_______________________________________________
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
^ permalink raw reply [flat|nested] 51+ messages in thread* Re: [pve-devel] [PATCH qemu-server v6 09/10] migrate: show vfio state transferred too
2025-02-13 13:17 ` [pve-devel] [PATCH qemu-server v6 09/10] migrate: show vfio state transferred too Dominik Csapak
@ 2025-03-07 12:35 ` Fiona Ebner
0 siblings, 0 replies; 51+ messages in thread
From: Fiona Ebner @ 2025-03-07 12:35 UTC (permalink / raw)
To: Proxmox VE development discussion, Dominik Csapak
Am 13.02.25 um 14:17 schrieb Dominik Csapak:
> Show the transferred VFIO state (when there is one), but since there is
> no total here, so we can't show that, just what was transferred up until
> now.
>
> Signed-off-by: Dominik Csapak <d.csapak@proxmox.com>
Without the unrelated hunk below:
Reviewed-by: Fiona Ebner <f.ebner@proxmox.com>
> ---
> no changes in v6
> PVE/API2/Qemu.pm | 2 +-
> PVE/QemuMigrate.pm | 12 +++++++++++-
> 2 files changed, 12 insertions(+), 2 deletions(-)
>
> diff --git a/PVE/API2/Qemu.pm b/PVE/API2/Qemu.pm
> index 6a303337..8e744107 100644
> --- a/PVE/API2/Qemu.pm
> +++ b/PVE/API2/Qemu.pm
> @@ -4733,7 +4733,7 @@ __PACKAGE__->register_method({
> }
>
> # if nothing came up, add it to the allowed nodes
> - if (!$res->{not_allowed_nodes}->{$node}) {
> + if (scalar($res->{not_allowed_nodes}->{$node}->%*) == 0) {
> push $res->{allowed_nodes}->@*, $node;
> }
Seems like this should be squashed into the previous patch.
_______________________________________________
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
^ permalink raw reply [flat|nested] 51+ messages in thread
* [pve-devel] [PATCH qemu-server v6 10/10] migrate: add transfer summary
2025-02-13 13:16 [pve-devel] [PATCH (guest-)common/qemu-server/manager/docs v6] implement Dominik Csapak
` (11 preceding siblings ...)
2025-02-13 13:17 ` [pve-devel] [PATCH qemu-server v6 09/10] migrate: show vfio state transferred too Dominik Csapak
@ 2025-02-13 13:17 ` Dominik Csapak
2025-03-07 12:35 ` Fiona Ebner
2025-02-13 13:17 ` [pve-devel] [PATCH manager v6 1/5] mapping: pci: include mdev in config checks Dominik Csapak
` (9 subsequent siblings)
22 siblings, 1 reply; 51+ messages in thread
From: Dominik Csapak @ 2025-02-13 13:17 UTC (permalink / raw)
To: pve-devel
showing a final transfer log line helps with identifying what was
actually transferred. E.g. it could happen that the VFIO state was only
transferred in the last iteration. In such a case we would not see that
information at all.
Signed-off-by: Dominik Csapak <d.csapak@proxmox.com>
---
new in v6
PVE/QemuMigrate.pm | 14 ++++++++++++++
1 file changed, 14 insertions(+)
diff --git a/PVE/QemuMigrate.pm b/PVE/QemuMigrate.pm
index d6f9132b..babd81a1 100644
--- a/PVE/QemuMigrate.pm
+++ b/PVE/QemuMigrate.pm
@@ -1288,6 +1288,20 @@ sub phase2 {
my $downtime = $stat->{downtime} || 0;
$self->log('info', "average migration speed: $avg_speed/s - downtime $downtime ms");
}
+ my $trans = $memstat->{transferred} || 0;
+ my $vfio_transferred = $stat->{vfio}->{transferred} || 0;
+
+ if ($trans > 0 || $vfio_transferred > 0) {
+ my $transferred_h = render_bytes($trans, 1);
+ my $summary = "transferred $transferred_h VM-state";
+
+ if ($vfio_transferred > 0) {
+ my $vfio_h = render_bytes($vfio_transferred, 1);
+ $summary .= " (+ $vfio_h VFIO-state)";
+ }
+
+ $self->log('info', "migration $status, $summary");
+ }
}
if ($status eq 'failed' || $status eq 'cancelled') {
--
2.39.5
_______________________________________________
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
^ permalink raw reply [flat|nested] 51+ messages in thread* [pve-devel] [PATCH manager v6 1/5] mapping: pci: include mdev in config checks
2025-02-13 13:16 [pve-devel] [PATCH (guest-)common/qemu-server/manager/docs v6] implement Dominik Csapak
` (12 preceding siblings ...)
2025-02-13 13:17 ` [pve-devel] [PATCH qemu-server v6 10/10] migrate: add transfer summary Dominik Csapak
@ 2025-02-13 13:17 ` Dominik Csapak
2025-03-07 13:00 ` Fiona Ebner
2025-02-13 13:17 ` [pve-devel] [PATCH manager v6 2/5] bulk migrate: improve precondition checks Dominik Csapak
` (8 subsequent siblings)
22 siblings, 1 reply; 51+ messages in thread
From: Dominik Csapak @ 2025-02-13 13:17 UTC (permalink / raw)
To: pve-devel
by also providing the global config in assert_valid, and by also
adding the mdev config in the 'toCheck' object in the gui
For the gui, we extract the mdev property from the global entry, and add
it to the individual mapping entries, that way we can reuse the checking
logic of the other properties.
Signed-off-by: Dominik Csapak <d.csapak@proxmox.com>
---
no changes in v6
PVE/API2/Cluster/Mapping/PCI.pm | 2 +-
www/manager6/dc/PCIMapView.js | 6 ++++++
2 files changed, 7 insertions(+), 1 deletion(-)
diff --git a/PVE/API2/Cluster/Mapping/PCI.pm b/PVE/API2/Cluster/Mapping/PCI.pm
index 64462d25..f85623bb 100644
--- a/PVE/API2/Cluster/Mapping/PCI.pm
+++ b/PVE/API2/Cluster/Mapping/PCI.pm
@@ -115,7 +115,7 @@ __PACKAGE__->register_method ({
};
}
for my $mapping ($mappings->@*) {
- eval { PVE::Mapping::PCI::assert_valid($id, $mapping) };
+ eval { PVE::Mapping::PCI::assert_valid($id, $mapping, $entry) };
if (my $err = $@) {
push $entry->{checks}->@*, {
severity => 'error',
diff --git a/www/manager6/dc/PCIMapView.js b/www/manager6/dc/PCIMapView.js
index 80fe3c0f..f9d43aa2 100644
--- a/www/manager6/dc/PCIMapView.js
+++ b/www/manager6/dc/PCIMapView.js
@@ -20,10 +20,15 @@ Ext.define('PVE.dc.PCIMapView', {
data.forEach((entry) => {
ids[entry.id] = entry;
});
+ let mdev;
me.getRootNode()?.cascade(function(rec) {
+ if (rec.data.type === 'entry') {
+ mdev = rec.data.mdev ?? 0;
+ }
if (rec.data.node !== node || rec.data.type !== 'map') {
return;
}
+ rec.data.mdev = mdev;
let id = rec.data.path;
if (!id.match(/\.\d$/)) {
@@ -44,6 +49,7 @@ Ext.define('PVE.dc.PCIMapView', {
let toCheck = {
id: deviceId,
'subsystem-id': subId,
+ mdev: device.mdev ?? 0,
iommugroup: device.iommugroup !== -1 ? device.iommugroup : undefined,
};
--
2.39.5
_______________________________________________
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
^ permalink raw reply [flat|nested] 51+ messages in thread* Re: [pve-devel] [PATCH manager v6 1/5] mapping: pci: include mdev in config checks
2025-02-13 13:17 ` [pve-devel] [PATCH manager v6 1/5] mapping: pci: include mdev in config checks Dominik Csapak
@ 2025-03-07 13:00 ` Fiona Ebner
0 siblings, 0 replies; 51+ messages in thread
From: Fiona Ebner @ 2025-03-07 13:00 UTC (permalink / raw)
To: Proxmox VE development discussion, Dominik Csapak
Am 13.02.25 um 14:17 schrieb Dominik Csapak:
> by also providing the global config in assert_valid, and by also
> adding the mdev config in the 'toCheck' object in the gui
>
> For the gui, we extract the mdev property from the global entry, and add
> it to the individual mapping entries, that way we can reuse the checking
> logic of the other properties.
>
> Signed-off-by: Dominik Csapak <d.csapak@proxmox.com>
Reviewed-by: Fiona Ebner <f.ebner@proxmox.com>
> ---
> no changes in v6
> PVE/API2/Cluster/Mapping/PCI.pm | 2 +-
> www/manager6/dc/PCIMapView.js | 6 ++++++
> 2 files changed, 7 insertions(+), 1 deletion(-)
>
> diff --git a/PVE/API2/Cluster/Mapping/PCI.pm b/PVE/API2/Cluster/Mapping/PCI.pm
> index 64462d25..f85623bb 100644
> --- a/PVE/API2/Cluster/Mapping/PCI.pm
> +++ b/PVE/API2/Cluster/Mapping/PCI.pm
> @@ -115,7 +115,7 @@ __PACKAGE__->register_method ({
> };
> }
> for my $mapping ($mappings->@*) {
> - eval { PVE::Mapping::PCI::assert_valid($id, $mapping) };
> + eval { PVE::Mapping::PCI::assert_valid($id, $mapping, $entry) };
> if (my $err = $@) {
> push $entry->{checks}->@*, {
> severity => 'error',
> diff --git a/www/manager6/dc/PCIMapView.js b/www/manager6/dc/PCIMapView.js
> index 80fe3c0f..f9d43aa2 100644
> --- a/www/manager6/dc/PCIMapView.js
> +++ b/www/manager6/dc/PCIMapView.js
> @@ -20,10 +20,15 @@ Ext.define('PVE.dc.PCIMapView', {
> data.forEach((entry) => {
> ids[entry.id] = entry;
> });
Nit: maybe worth a brief code comment here (e.g. what you already wrote
in the commit message).
> + let mdev;
> me.getRootNode()?.cascade(function(rec) {
> + if (rec.data.type === 'entry') {
> + mdev = rec.data.mdev ?? 0;
> + }
> if (rec.data.node !== node || rec.data.type !== 'map') {
> return;
> }
> + rec.data.mdev = mdev;
>
> let id = rec.data.path;
> if (!id.match(/\.\d$/)) {
> @@ -44,6 +49,7 @@ Ext.define('PVE.dc.PCIMapView', {
> let toCheck = {
> id: deviceId,
> 'subsystem-id': subId,
> + mdev: device.mdev ?? 0,
> iommugroup: device.iommugroup !== -1 ? device.iommugroup : undefined,
> };
>
_______________________________________________
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
^ permalink raw reply [flat|nested] 51+ messages in thread
* [pve-devel] [PATCH manager v6 2/5] bulk migrate: improve precondition checks
2025-02-13 13:16 [pve-devel] [PATCH (guest-)common/qemu-server/manager/docs v6] implement Dominik Csapak
` (13 preceding siblings ...)
2025-02-13 13:17 ` [pve-devel] [PATCH manager v6 1/5] mapping: pci: include mdev in config checks Dominik Csapak
@ 2025-02-13 13:17 ` Dominik Csapak
2025-03-07 13:19 ` Fiona Ebner
2025-02-13 13:17 ` [pve-devel] [PATCH manager v6 3/5] bulk migrate: include checks for live-migratable local resources Dominik Csapak
` (7 subsequent siblings)
22 siblings, 1 reply; 51+ messages in thread
From: Dominik Csapak @ 2025-02-13 13:17 UTC (permalink / raw)
To: pve-devel
this now takes into account the 'not_allowed_nodes' hash we get from the
api call. With that, we can now limit the 'local_resources' check for
online vms only, as for offline guests, the 'unavailable-resources' hash
already includes mapped devices that don't exist on the target node.
This now also includes unavailable storages on target nodes.
Signed-off-by: Dominik Csapak <d.csapak@proxmox.com>
---
changes in v6:
* added missing colon in log output
PVE/API2/Nodes.pm | 14 +++++++++++++-
1 file changed, 13 insertions(+), 1 deletion(-)
diff --git a/PVE/API2/Nodes.pm b/PVE/API2/Nodes.pm
index 9cdf19db..f504e1b1 100644
--- a/PVE/API2/Nodes.pm
+++ b/PVE/API2/Nodes.pm
@@ -2331,11 +2331,23 @@ my $create_migrate_worker = sub {
$invalidConditions .= join(', ', map { $_->{volid} } @{$preconditions->{local_disks}});
}
- if (@{$preconditions->{local_resources}}) {
+ if ($online && scalar($preconditions->{local_resources}->@*)) {
$invalidConditions .= "\n Has local resources: ";
$invalidConditions .= join(', ', @{$preconditions->{local_resources}});
}
+ if (my $not_allowed_nodes = $preconditions->{not_allowed_nodes}) {
+ if (my $unavail_storages = $not_allowed_nodes->{$target}->{unavailable_storages}) {
+ $invalidConditions .= "\n Has unavailable storages: ";
+ $invalidConditions .= join(', ', $unavail_storages->@*);
+ }
+
+ if (my $unavail_resources = $not_allowed_nodes->{$target}->{'unavailable-resources'}) {
+ $invalidConditions .= "\n Has unavailable resources: ";
+ $invalidConditions .= join(', ', $unavail_resources->@*);
+ }
+ }
+
if ($invalidConditions && $invalidConditions ne '') {
print STDERR "skip VM $vmid - precondition check failed:";
die "$invalidConditions\n";
--
2.39.5
_______________________________________________
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
^ permalink raw reply [flat|nested] 51+ messages in thread* Re: [pve-devel] [PATCH manager v6 2/5] bulk migrate: improve precondition checks
2025-02-13 13:17 ` [pve-devel] [PATCH manager v6 2/5] bulk migrate: improve precondition checks Dominik Csapak
@ 2025-03-07 13:19 ` Fiona Ebner
2025-03-07 13:23 ` Fiona Ebner
0 siblings, 1 reply; 51+ messages in thread
From: Fiona Ebner @ 2025-03-07 13:19 UTC (permalink / raw)
To: Proxmox VE development discussion, Dominik Csapak
Am 13.02.25 um 14:17 schrieb Dominik Csapak:
> this now takes into account the 'not_allowed_nodes' hash we get from the
> api call. With that, we can now limit the 'local_resources' check for
> online vms only, as for offline guests, the 'unavailable-resources' hash
> already includes mapped devices that don't exist on the target node.
>
> This now also includes unavailable storages on target nodes.
>
> Signed-off-by: Dominik Csapak <d.csapak@proxmox.com>
> ---
> changes in v6:
> * added missing colon in log output
> PVE/API2/Nodes.pm | 14 +++++++++++++-
> 1 file changed, 13 insertions(+), 1 deletion(-)
>
> diff --git a/PVE/API2/Nodes.pm b/PVE/API2/Nodes.pm
> index 9cdf19db..f504e1b1 100644
> --- a/PVE/API2/Nodes.pm
> +++ b/PVE/API2/Nodes.pm
> @@ -2331,11 +2331,23 @@ my $create_migrate_worker = sub {
> $invalidConditions .= join(', ', map { $_->{volid} } @{$preconditions->{local_disks}});
> }
>
> - if (@{$preconditions->{local_resources}}) {
> + if ($online && scalar($preconditions->{local_resources}->@*)) {
Hmm, what about non-usb/pci local devices that are not covered by the
'unavailable-resources' below? I.e. ivshmem/serial/parallel. You break
the check for offline migration against those.
> $invalidConditions .= "\n Has local resources: ";
> $invalidConditions .= join(', ', @{$preconditions->{local_resources}});
> }
>
> + if (my $not_allowed_nodes = $preconditions->{not_allowed_nodes}) {
> + if (my $unavail_storages = $not_allowed_nodes->{$target}->{unavailable_storages}) {
> + $invalidConditions .= "\n Has unavailable storages: ";
> + $invalidConditions .= join(', ', $unavail_storages->@*);
> + }
> +
> + if (my $unavail_resources = $not_allowed_nodes->{$target}->{'unavailable-resources'}) {
> + $invalidConditions .= "\n Has unavailable resources: ";
> + $invalidConditions .= join(', ', $unavail_resources->@*);
> + }
> + }
> +
> if ($invalidConditions && $invalidConditions ne '') {
> print STDERR "skip VM $vmid - precondition check failed:";
> die "$invalidConditions\n";
_______________________________________________
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
^ permalink raw reply [flat|nested] 51+ messages in thread* Re: [pve-devel] [PATCH manager v6 2/5] bulk migrate: improve precondition checks
2025-03-07 13:19 ` Fiona Ebner
@ 2025-03-07 13:23 ` Fiona Ebner
0 siblings, 0 replies; 51+ messages in thread
From: Fiona Ebner @ 2025-03-07 13:23 UTC (permalink / raw)
To: Proxmox VE development discussion, Dominik Csapak
Am 07.03.25 um 14:19 schrieb Fiona Ebner:
> Am 13.02.25 um 14:17 schrieb Dominik Csapak:
>> this now takes into account the 'not_allowed_nodes' hash we get from the
>> api call. With that, we can now limit the 'local_resources' check for
>> online vms only, as for offline guests, the 'unavailable-resources' hash
>> already includes mapped devices that don't exist on the target node.
>>
>> This now also includes unavailable storages on target nodes.
>>
>> Signed-off-by: Dominik Csapak <d.csapak@proxmox.com>
>> ---
>> changes in v6:
>> * added missing colon in log output
>> PVE/API2/Nodes.pm | 14 +++++++++++++-
>> 1 file changed, 13 insertions(+), 1 deletion(-)
>>
>> diff --git a/PVE/API2/Nodes.pm b/PVE/API2/Nodes.pm
>> index 9cdf19db..f504e1b1 100644
>> --- a/PVE/API2/Nodes.pm
>> +++ b/PVE/API2/Nodes.pm
>> @@ -2331,11 +2331,23 @@ my $create_migrate_worker = sub {
>> $invalidConditions .= join(', ', map { $_->{volid} } @{$preconditions->{local_disks}});
>> }
>>
>> - if (@{$preconditions->{local_resources}}) {
>> + if ($online && scalar($preconditions->{local_resources}->@*)) {
>
> Hmm, what about non-usb/pci local devices that are not covered by the
> 'unavailable-resources' below? I.e. ivshmem/serial/parallel. You break
> the check for offline migration against those.
And actually also the not-mapped USB/PCI devices or?
_______________________________________________
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
^ permalink raw reply [flat|nested] 51+ messages in thread
* [pve-devel] [PATCH manager v6 3/5] bulk migrate: include checks for live-migratable local resources
2025-02-13 13:16 [pve-devel] [PATCH (guest-)common/qemu-server/manager/docs v6] implement Dominik Csapak
` (14 preceding siblings ...)
2025-02-13 13:17 ` [pve-devel] [PATCH manager v6 2/5] bulk migrate: improve precondition checks Dominik Csapak
@ 2025-02-13 13:17 ` Dominik Csapak
2025-03-07 13:30 ` Fiona Ebner
2025-02-13 13:17 ` [pve-devel] [PATCH manager v6 4/5] ui: adapt migration window to precondition api change Dominik Csapak
` (6 subsequent siblings)
22 siblings, 1 reply; 51+ messages in thread
From: Dominik Csapak @ 2025-02-13 13:17 UTC (permalink / raw)
To: pve-devel
those should be able to migrate even for online vms. If the mapping does
not exist on the target node, that will be caught further down anyway.
Signed-off-by: Dominik Csapak <d.csapak@proxmox.com>
---
no changes in v6
PVE/API2/Nodes.pm | 13 +++++++++++--
1 file changed, 11 insertions(+), 2 deletions(-)
diff --git a/PVE/API2/Nodes.pm b/PVE/API2/Nodes.pm
index f504e1b1..f5484280 100644
--- a/PVE/API2/Nodes.pm
+++ b/PVE/API2/Nodes.pm
@@ -2331,9 +2331,18 @@ my $create_migrate_worker = sub {
$invalidConditions .= join(', ', map { $_->{volid} } @{$preconditions->{local_disks}});
}
+ # for a live migration all local_resources must be marked as live-migratable
if ($online && scalar($preconditions->{local_resources}->@*)) {
- $invalidConditions .= "\n Has local resources: ";
- $invalidConditions .= join(', ', @{$preconditions->{local_resources}});
+ my $resource_not_live = [];
+ for my $resource ($preconditions->{local_resources}->@*) {
+ next if $preconditions->{'mapped-resource-info'}->{$resource}->{'live-migration'};
+ push $resource_not_live->@*, $resource;
+ }
+
+ if (scalar($resource_not_live->@*)) {
+ $invalidConditions .= "\n Has local resources not marked as live migratable: ";
+ $invalidConditions .= join(', ', $resource_not_live->@*);
+ }
}
if (my $not_allowed_nodes = $preconditions->{not_allowed_nodes}) {
--
2.39.5
_______________________________________________
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
^ permalink raw reply [flat|nested] 51+ messages in thread* Re: [pve-devel] [PATCH manager v6 3/5] bulk migrate: include checks for live-migratable local resources
2025-02-13 13:17 ` [pve-devel] [PATCH manager v6 3/5] bulk migrate: include checks for live-migratable local resources Dominik Csapak
@ 2025-03-07 13:30 ` Fiona Ebner
2025-03-07 13:40 ` Fiona Ebner
0 siblings, 1 reply; 51+ messages in thread
From: Fiona Ebner @ 2025-03-07 13:30 UTC (permalink / raw)
To: Proxmox VE development discussion, Dominik Csapak
Am 13.02.25 um 14:17 schrieb Dominik Csapak:
> those should be able to migrate even for online vms. If the mapping does
> not exist on the target node, that will be caught further down anyway.
>
> Signed-off-by: Dominik Csapak <d.csapak@proxmox.com>
> ---
> no changes in v6
> PVE/API2/Nodes.pm | 13 +++++++++++--
> 1 file changed, 11 insertions(+), 2 deletions(-)
>
> diff --git a/PVE/API2/Nodes.pm b/PVE/API2/Nodes.pm
> index f504e1b1..f5484280 100644
> --- a/PVE/API2/Nodes.pm
> +++ b/PVE/API2/Nodes.pm
> @@ -2331,9 +2331,18 @@ my $create_migrate_worker = sub {
> $invalidConditions .= join(', ', map { $_->{volid} } @{$preconditions->{local_disks}});
> }
>
> + # for a live migration all local_resources must be marked as live-migratable
> if ($online && scalar($preconditions->{local_resources}->@*)) {
> - $invalidConditions .= "\n Has local resources: ";
> - $invalidConditions .= join(', ', @{$preconditions->{local_resources}});
> + my $resource_not_live = [];
> + for my $resource ($preconditions->{local_resources}->@*) {
> + next if $preconditions->{'mapped-resource-info'}->{$resource}->{'live-migration'};
> + push $resource_not_live->@*, $resource;
> + }
> +
> + if (scalar($resource_not_live->@*)) {
> + $invalidConditions .= "\n Has local resources not marked as live migratable: ";
> + $invalidConditions .= join(', ', $resource_not_live->@*);
> + }
> }
>
> if (my $not_allowed_nodes = $preconditions->{not_allowed_nodes}) {
Should we rather not add those to the "local_resources" result in the
first place? I.e. in check_local_resources() we know whether it's a live
migration or not based on the $state argument.
And towards the end of that function we could...
> if ($k =~ m/^hostpci/) {
> my $entry = parse_property_string('pve-qm-hostpci', $conf->{$k});
> if (my $name = $entry->{mapping}) {
> $add_missing_mapping->('pci', $k, $name);
> my $mapped_device = { name => $name };
> $mapped_device->{'live-migration'} = 1
> if $pci_map->{ids}->{$name}->{'live-migration-capable'};
> $mapped_res->{$k} = $mapped_device;
> }
> }
> # sockets are safe: they will recreated be on the target side post-migrate
> next if $k =~ m/^serial/ && ($conf->{$k} eq 'socket');
...do "next if live-migration" and not even add it.
> push @loc_res, $k if $k =~ m/^(usb|hostpci|serial|parallel)\d+$/;
_______________________________________________
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
^ permalink raw reply [flat|nested] 51+ messages in thread* Re: [pve-devel] [PATCH manager v6 3/5] bulk migrate: include checks for live-migratable local resources
2025-03-07 13:30 ` Fiona Ebner
@ 2025-03-07 13:40 ` Fiona Ebner
2025-03-10 12:52 ` Dominik Csapak
0 siblings, 1 reply; 51+ messages in thread
From: Fiona Ebner @ 2025-03-07 13:40 UTC (permalink / raw)
To: Proxmox VE development discussion, Dominik Csapak
Am 07.03.25 um 14:30 schrieb Fiona Ebner:
> Am 13.02.25 um 14:17 schrieb Dominik Csapak:
>> those should be able to migrate even for online vms. If the mapping does
>> not exist on the target node, that will be caught further down anyway.
>>
>> Signed-off-by: Dominik Csapak <d.csapak@proxmox.com>
>> ---
>> no changes in v6
>> PVE/API2/Nodes.pm | 13 +++++++++++--
>> 1 file changed, 11 insertions(+), 2 deletions(-)
>>
>> diff --git a/PVE/API2/Nodes.pm b/PVE/API2/Nodes.pm
>> index f504e1b1..f5484280 100644
>> --- a/PVE/API2/Nodes.pm
>> +++ b/PVE/API2/Nodes.pm
>> @@ -2331,9 +2331,18 @@ my $create_migrate_worker = sub {
>> $invalidConditions .= join(', ', map { $_->{volid} } @{$preconditions->{local_disks}});
>> }
>>
>> + # for a live migration all local_resources must be marked as live-migratable
>> if ($online && scalar($preconditions->{local_resources}->@*)) {
>> - $invalidConditions .= "\n Has local resources: ";
>> - $invalidConditions .= join(', ', @{$preconditions->{local_resources}});
>> + my $resource_not_live = [];
>> + for my $resource ($preconditions->{local_resources}->@*) {
>> + next if $preconditions->{'mapped-resource-info'}->{$resource}->{'live-migration'};
>> + push $resource_not_live->@*, $resource;
>> + }
>> +
>> + if (scalar($resource_not_live->@*)) {
>> + $invalidConditions .= "\n Has local resources not marked as live migratable: ";
>> + $invalidConditions .= join(', ', $resource_not_live->@*);
>> + }
>> }
>>
>> if (my $not_allowed_nodes = $preconditions->{not_allowed_nodes}) {
>
> Should we rather not add those to the "local_resources" result in the
> first place? I.e. in check_local_resources() we know whether it's a live
> migration or not based on the $state argument.
>
> And towards the end of that function we could...
>
>> if ($k =~ m/^hostpci/) {
>> my $entry = parse_property_string('pve-qm-hostpci', $conf->{$k});
>> if (my $name = $entry->{mapping}) {
>> $add_missing_mapping->('pci', $k, $name);
>> my $mapped_device = { name => $name };
>> $mapped_device->{'live-migration'} = 1
>> if $pci_map->{ids}->{$name}->{'live-migration-capable'};
>> $mapped_res->{$k} = $mapped_device;
>> }
>> }
>> # sockets are safe: they will recreated be on the target side post-migrate
>> next if $k =~ m/^serial/ && ($conf->{$k} eq 'socket');
>
> ...do "next if live-migration" and not even add it.
Or rather, next if !missing mapping && (!$state or live-migration). I.e.
also not adding them for offline migration to the local resources in the
first place. AFAIU, local_resources/loc_res was intended to be the
current blockers for the offline or online migration at hand. Can we go
back and align the behavior to that meaning? Currently, we add mapped
devices even if they are not blockers. Do we already rely too much on that?
>
>> push @loc_res, $k if $k =~ m/^(usb|hostpci|serial|parallel)\d+$/;
>
_______________________________________________
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
^ permalink raw reply [flat|nested] 51+ messages in thread* Re: [pve-devel] [PATCH manager v6 3/5] bulk migrate: include checks for live-migratable local resources
2025-03-07 13:40 ` Fiona Ebner
@ 2025-03-10 12:52 ` Dominik Csapak
2025-03-10 13:21 ` Fiona Ebner
0 siblings, 1 reply; 51+ messages in thread
From: Dominik Csapak @ 2025-03-10 12:52 UTC (permalink / raw)
To: Proxmox VE development discussion
On 3/7/25 14:40, Fiona Ebner wrote:
> Am 07.03.25 um 14:30 schrieb Fiona Ebner:
>> Am 13.02.25 um 14:17 schrieb Dominik Csapak:
>>> those should be able to migrate even for online vms. If the mapping does
>>> not exist on the target node, that will be caught further down anyway.
>>>
>>> Signed-off-by: Dominik Csapak <d.csapak@proxmox.com>
>>> ---
>>> no changes in v6
>>> PVE/API2/Nodes.pm | 13 +++++++++++--
>>> 1 file changed, 11 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/PVE/API2/Nodes.pm b/PVE/API2/Nodes.pm
>>> index f504e1b1..f5484280 100644
>>> --- a/PVE/API2/Nodes.pm
>>> +++ b/PVE/API2/Nodes.pm
>>> @@ -2331,9 +2331,18 @@ my $create_migrate_worker = sub {
>>> $invalidConditions .= join(', ', map { $_->{volid} } @{$preconditions->{local_disks}});
>>> }
>>>
>>> + # for a live migration all local_resources must be marked as live-migratable
>>> if ($online && scalar($preconditions->{local_resources}->@*)) {
>>> - $invalidConditions .= "\n Has local resources: ";
>>> - $invalidConditions .= join(', ', @{$preconditions->{local_resources}});
>>> + my $resource_not_live = [];
>>> + for my $resource ($preconditions->{local_resources}->@*) {
>>> + next if $preconditions->{'mapped-resource-info'}->{$resource}->{'live-migration'};
>>> + push $resource_not_live->@*, $resource;
>>> + }
>>> +
>>> + if (scalar($resource_not_live->@*)) {
>>> + $invalidConditions .= "\n Has local resources not marked as live migratable: ";
>>> + $invalidConditions .= join(', ', $resource_not_live->@*);
>>> + }
>>> }
>>>
>>> if (my $not_allowed_nodes = $preconditions->{not_allowed_nodes}) {
>>
>> Should we rather not add those to the "local_resources" result in the
>> first place? I.e. in check_local_resources() we know whether it's a live
>> migration or not based on the $state argument.
>>
>> And towards the end of that function we could...
>>
>>> if ($k =~ m/^hostpci/) {
>>> my $entry = parse_property_string('pve-qm-hostpci', $conf->{$k});
>>> if (my $name = $entry->{mapping}) {
>>> $add_missing_mapping->('pci', $k, $name);
>>> my $mapped_device = { name => $name };
>>> $mapped_device->{'live-migration'} = 1
>>> if $pci_map->{ids}->{$name}->{'live-migration-capable'};
>>> $mapped_res->{$k} = $mapped_device;
>>> }
>>> }
>>> # sockets are safe: they will recreated be on the target side post-migrate
>>> next if $k =~ m/^serial/ && ($conf->{$k} eq 'socket');
>>
>> ...do "next if live-migration" and not even add it.
>
> Or rather, next if !missing mapping && (!$state or live-migration). I.e.
> also not adding them for offline migration to the local resources in the
> first place. AFAIU, local_resources/loc_res was intended to be the
> current blockers for the offline or online migration at hand. Can we go
> back and align the behavior to that meaning? Currently, we add mapped
> devices even if they are not blockers. Do we already rely too much on that?
hmm i can try that, but i have a question on how to handle some situations:
there are the following possibilities (if I didn't forget one):
* non-mapped hostpci device -> local resource
* mapped hostpci device with no live migration capabilities -> local resource + mapped
* mapped hostpci device with live migration capabilities -> mapped only
did I understand you correctly?
>
>>
>>> push @loc_res, $k if $k =~ m/^(usb|hostpci|serial|parallel)\d+$/;
>>
_______________________________________________
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
^ permalink raw reply [flat|nested] 51+ messages in thread* Re: [pve-devel] [PATCH manager v6 3/5] bulk migrate: include checks for live-migratable local resources
2025-03-10 12:52 ` Dominik Csapak
@ 2025-03-10 13:21 ` Fiona Ebner
2025-03-10 13:58 ` Dominik Csapak
0 siblings, 1 reply; 51+ messages in thread
From: Fiona Ebner @ 2025-03-10 13:21 UTC (permalink / raw)
To: Proxmox VE development discussion, Dominik Csapak
Am 10.03.25 um 13:52 schrieb Dominik Csapak:
> On 3/7/25 14:40, Fiona Ebner wrote:
>> Am 07.03.25 um 14:30 schrieb Fiona Ebner:
>>> Am 13.02.25 um 14:17 schrieb Dominik Csapak:
>>>> those should be able to migrate even for online vms. If the mapping
>>>> does
>>>> not exist on the target node, that will be caught further down anyway.
>>>>
>>>> Signed-off-by: Dominik Csapak <d.csapak@proxmox.com>
>>>> ---
>>>> no changes in v6
>>>> PVE/API2/Nodes.pm | 13 +++++++++++--
>>>> 1 file changed, 11 insertions(+), 2 deletions(-)
>>>>
>>>> diff --git a/PVE/API2/Nodes.pm b/PVE/API2/Nodes.pm
>>>> index f504e1b1..f5484280 100644
>>>> --- a/PVE/API2/Nodes.pm
>>>> +++ b/PVE/API2/Nodes.pm
>>>> @@ -2331,9 +2331,18 @@ my $create_migrate_worker = sub {
>>>> $invalidConditions .= join(', ', map { $_->{volid} }
>>>> @{$preconditions->{local_disks}});
>>>> }
>>>> + # for a live migration all local_resources must be marked as
>>>> live-migratable
>>>> if ($online && scalar($preconditions->{local_resources}->@*)) {
>>>> - $invalidConditions .= "\n Has local resources: ";
>>>> - $invalidConditions .= join(', ', @{$preconditions-
>>>> >{local_resources}});
>>>> + my $resource_not_live = [];
>>>> + for my $resource ($preconditions->{local_resources}->@*) {
>>>> + next if $preconditions->{'mapped-resource-info'}-
>>>> >{$resource}->{'live-migration'};
>>>> + push $resource_not_live->@*, $resource;
>>>> + }
>>>> +
>>>> + if (scalar($resource_not_live->@*)) {
>>>> + $invalidConditions .= "\n Has local resources not marked
>>>> as live migratable: ";
>>>> + $invalidConditions .= join(', ', $resource_not_live->@*);
>>>> + }
>>>> }
>>>> if (my $not_allowed_nodes = $preconditions-
>>>> >{not_allowed_nodes}) {
>>>
>>> Should we rather not add those to the "local_resources" result in the
>>> first place? I.e. in check_local_resources() we know whether it's a live
>>> migration or not based on the $state argument.
>>>
>>> And towards the end of that function we could...
>>>
>>>> if ($k =~ m/^hostpci/) {
>>>> my $entry = parse_property_string('pve-qm-hostpci', $conf-
>>>> >{$k});
>>>> if (my $name = $entry->{mapping}) {
>>>> $add_missing_mapping->('pci', $k, $name);
>>>> my $mapped_device = { name => $name };
>>>> $mapped_device->{'live-migration'} = 1
>>>> if $pci_map->{ids}->{$name}->{'live-migration-capable'};
>>>> $mapped_res->{$k} = $mapped_device;
>>>> }
>>>> }
>>>> # sockets are safe: they will recreated be on the target side
>>>> post-migrate
>>>> next if $k =~ m/^serial/ && ($conf->{$k} eq 'socket');
>>>
>>> ...do "next if live-migration" and not even add it.
>>
>> Or rather, next if !missing mapping && (!$state or live-migration). I.e.
>> also not adding them for offline migration to the local resources in the
>> first place. AFAIU, local_resources/loc_res was intended to be the
>> current blockers for the offline or online migration at hand. Can we go
>> back and align the behavior to that meaning? Currently, we add mapped
>> devices even if they are not blockers. Do we already rely too much on
>> that?
>
> hmm i can try that, but i have a question on how to handle some situations:
>
> there are the following possibilities (if I didn't forget one):
> * non-mapped hostpci device -> local resource
> * mapped hostpci device with no live migration capabilities -> local
> resource + mapped
I'd only add it to local resources if $state is set too, i.e. if it is a
live migration. If it's mapped and if it's an offline migration, don't
add it.
> * mapped hostpci device with live migration capabilities -> mapped only
>
> did I understand you correctly?
>
I.e. local resources should only contain the config keys that are actual
blockers for the migration at hand, which differs when it's online or
offline.
_______________________________________________
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
^ permalink raw reply [flat|nested] 51+ messages in thread* Re: [pve-devel] [PATCH manager v6 3/5] bulk migrate: include checks for live-migratable local resources
2025-03-10 13:21 ` Fiona Ebner
@ 2025-03-10 13:58 ` Dominik Csapak
2025-03-10 14:40 ` Fiona Ebner
0 siblings, 1 reply; 51+ messages in thread
From: Dominik Csapak @ 2025-03-10 13:58 UTC (permalink / raw)
To: Fiona Ebner, Proxmox VE development discussion
On 3/10/25 14:21, Fiona Ebner wrote:
> Am 10.03.25 um 13:52 schrieb Dominik Csapak:
>> On 3/7/25 14:40, Fiona Ebner wrote:
>>> Am 07.03.25 um 14:30 schrieb Fiona Ebner:
>>>> Am 13.02.25 um 14:17 schrieb Dominik Csapak:
>>>>> those should be able to migrate even for online vms. If the mapping
>>>>> does
>>>>> not exist on the target node, that will be caught further down anyway.
>>>>>
>>>>> Signed-off-by: Dominik Csapak <d.csapak@proxmox.com>
>>>>> ---
>>>>> no changes in v6
>>>>> PVE/API2/Nodes.pm | 13 +++++++++++--
>>>>> 1 file changed, 11 insertions(+), 2 deletions(-)
>>>>>
>>>>> diff --git a/PVE/API2/Nodes.pm b/PVE/API2/Nodes.pm
>>>>> index f504e1b1..f5484280 100644
>>>>> --- a/PVE/API2/Nodes.pm
>>>>> +++ b/PVE/API2/Nodes.pm
>>>>> @@ -2331,9 +2331,18 @@ my $create_migrate_worker = sub {
>>>>> $invalidConditions .= join(', ', map { $_->{volid} }
>>>>> @{$preconditions->{local_disks}});
>>>>> }
>>>>> + # for a live migration all local_resources must be marked as
>>>>> live-migratable
>>>>> if ($online && scalar($preconditions->{local_resources}->@*)) {
>>>>> - $invalidConditions .= "\n Has local resources: ";
>>>>> - $invalidConditions .= join(', ', @{$preconditions-
>>>>>> {local_resources}});
>>>>> + my $resource_not_live = [];
>>>>> + for my $resource ($preconditions->{local_resources}->@*) {
>>>>> + next if $preconditions->{'mapped-resource-info'}-
>>>>>> {$resource}->{'live-migration'};
>>>>> + push $resource_not_live->@*, $resource;
>>>>> + }
>>>>> +
>>>>> + if (scalar($resource_not_live->@*)) {
>>>>> + $invalidConditions .= "\n Has local resources not marked
>>>>> as live migratable: ";
>>>>> + $invalidConditions .= join(', ', $resource_not_live->@*);
>>>>> + }
>>>>> }
>>>>> if (my $not_allowed_nodes = $preconditions-
>>>>>> {not_allowed_nodes}) {
>>>>
>>>> Should we rather not add those to the "local_resources" result in the
>>>> first place? I.e. in check_local_resources() we know whether it's a live
>>>> migration or not based on the $state argument.
>>>>
>>>> And towards the end of that function we could...
>>>>
>>>>> if ($k =~ m/^hostpci/) {
>>>>> my $entry = parse_property_string('pve-qm-hostpci', $conf-
>>>>>> {$k});
>>>>> if (my $name = $entry->{mapping}) {
>>>>> $add_missing_mapping->('pci', $k, $name);
>>>>> my $mapped_device = { name => $name };
>>>>> $mapped_device->{'live-migration'} = 1
>>>>> if $pci_map->{ids}->{$name}->{'live-migration-capable'};
>>>>> $mapped_res->{$k} = $mapped_device;
>>>>> }
>>>>> }
>>>>> # sockets are safe: they will recreated be on the target side
>>>>> post-migrate
>>>>> next if $k =~ m/^serial/ && ($conf->{$k} eq 'socket');
>>>>
>>>> ...do "next if live-migration" and not even add it.
>>>
>>> Or rather, next if !missing mapping && (!$state or live-migration). I.e.
>>> also not adding them for offline migration to the local resources in the
>>> first place. AFAIU, local_resources/loc_res was intended to be the
>>> current blockers for the offline or online migration at hand. Can we go
>>> back and align the behavior to that meaning? Currently, we add mapped
>>> devices even if they are not blockers. Do we already rely too much on
>>> that?
>>
>> hmm i can try that, but i have a question on how to handle some situations:
>>
>> there are the following possibilities (if I didn't forget one):
>> * non-mapped hostpci device -> local resource
>> * mapped hostpci device with no live migration capabilities -> local
>> resource + mapped
>
> I'd only add it to local resources if $state is set too, i.e. if it is a
> live migration. If it's mapped and if it's an offline migration, don't
> add it.
>
>> * mapped hostpci device with live migration capabilities -> mapped only
>>
>> did I understand you correctly?
>>
>
> I.e. local resources should only contain the config keys that are actual
> blockers for the migration at hand, which differs when it's online or
> offline.
ok makes sense, did a quick test here, and it seems the UI handles it already gracefully,
(we ignore the local_resources for mapped devices alread) but wouldn't that be a 'breaking' change,
since we did return the devices in the 'local_resources' list until now?
I'm not saying that the change would be bad, but the current description and results would not
indicate to an api user that it's only for blocking migrations, as the current description is
"List local resources e.g. pci, usb"
_______________________________________________
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
^ permalink raw reply [flat|nested] 51+ messages in thread* Re: [pve-devel] [PATCH manager v6 3/5] bulk migrate: include checks for live-migratable local resources
2025-03-10 13:58 ` Dominik Csapak
@ 2025-03-10 14:40 ` Fiona Ebner
0 siblings, 0 replies; 51+ messages in thread
From: Fiona Ebner @ 2025-03-10 14:40 UTC (permalink / raw)
To: Dominik Csapak, Proxmox VE development discussion
Am 10.03.25 um 14:58 schrieb Dominik Csapak:
> On 3/10/25 14:21, Fiona Ebner wrote:
>> Am 10.03.25 um 13:52 schrieb Dominik Csapak:
>>> hmm i can try that, but i have a question on how to handle some
>>> situations:
>>>
>>> there are the following possibilities (if I didn't forget one):
>>> * non-mapped hostpci device -> local resource
>>> * mapped hostpci device with no live migration capabilities -> local
>>> resource + mapped
>>
>> I'd only add it to local resources if $state is set too, i.e. if it is a
>> live migration. If it's mapped and if it's an offline migration, don't
>> add it.
>>
>>> * mapped hostpci device with live migration capabilities -> mapped only
>>>
>>> did I understand you correctly?
>>>
>>
>> I.e. local resources should only contain the config keys that are actual
>> blockers for the migration at hand, which differs when it's online or
>> offline.
>
>
> ok makes sense, did a quick test here, and it seems the UI handles it
> already gracefully,
> (we ignore the local_resources for mapped devices alread) but wouldn't
> that be a 'breaking' change,
> since we did return the devices in the 'local_resources' list until now?
>
> I'm not saying that the change would be bad, but the current description
> and results would not
> indicate to an api user that it's only for blocking migrations, as the
> current description is
> "List local resources e.g. pci, usb"
It comes down to arguing what "local" means in this context. One could
argue that a mapped device is a logical abstraction over equivalent
devices on multiple nodes and thus not local in that sense. Looking at
the API endpoint naively, I would intuitively expect local_resources and
mapped-resources to be disjoint.
Regarding breakage, you do have a point. If a consumer is relying only
on local_resources and not looking at the mappings, or using the keys
from local resources to try to iterate over the mappings, the
precondition check would be wrong in the case of missing mappings.
IMHO we can go for this, as it's only the precondition check, migration
will just fail later for such API consumers. We can make the description
more precise, i.e. mention these are the local resources that cannot be
migrated, do not include mapped resources and they can be different for
offline vs. online.
_______________________________________________
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
^ permalink raw reply [flat|nested] 51+ messages in thread
* [pve-devel] [PATCH manager v6 4/5] ui: adapt migration window to precondition api change
2025-02-13 13:16 [pve-devel] [PATCH (guest-)common/qemu-server/manager/docs v6] implement Dominik Csapak
` (15 preceding siblings ...)
2025-02-13 13:17 ` [pve-devel] [PATCH manager v6 3/5] bulk migrate: include checks for live-migratable local resources Dominik Csapak
@ 2025-02-13 13:17 ` Dominik Csapak
2025-03-07 14:03 ` Fiona Ebner
2025-02-13 13:17 ` [pve-devel] [PATCH manager v6 5/5] fix #5175: ui: allow configuring and live migration of mapped pci resources Dominik Csapak
` (5 subsequent siblings)
22 siblings, 1 reply; 51+ messages in thread
From: Dominik Csapak @ 2025-02-13 13:17 UTC (permalink / raw)
To: pve-devel
we now return the 'allowed_nodes'/'not_allowed_nodes' also if the vm is
running, when it has mapped resources. So do that checks independently
so that the user has instant feedback where those resources exist.
Signed-off-by: Dominik Csapak <d.csapak@proxmox.com>
---
no changes in v6
www/manager6/window/Migrate.js | 26 +++++++++++++-------------
1 file changed, 13 insertions(+), 13 deletions(-)
diff --git a/www/manager6/window/Migrate.js b/www/manager6/window/Migrate.js
index 78d03921..604b63e7 100644
--- a/www/manager6/window/Migrate.js
+++ b/www/manager6/window/Migrate.js
@@ -104,7 +104,7 @@ Ext.define('PVE.window.Migrate', {
onTargetChange: function(nodeSelector) {
// Always display the storages of the currently selected migration target
this.lookup('pveDiskStorageSelector').setNodename(nodeSelector.value);
- this.checkMigratePreconditions();
+ this.checkMigratePreconditions(true);
},
startMigration: function() {
@@ -214,12 +214,12 @@ Ext.define('PVE.window.Migrate', {
migration.possible = true;
}
migration.preconditions = [];
+ let target = me.lookup('pveNodeSelector').value;
+ let disallowed = migrateStats.not_allowed_nodes?.[target] ?? {};
- if (migrateStats.allowed_nodes) {
+ if (migrateStats.allowed_nodes && !vm.get('running')) {
migration.allowedNodes = migrateStats.allowed_nodes;
- let target = me.lookup('pveNodeSelector').value;
if (target.length && !migrateStats.allowed_nodes.includes(target)) {
- let disallowed = migrateStats.not_allowed_nodes[target] ?? {};
if (disallowed.unavailable_storages !== undefined) {
let missingStorages = disallowed.unavailable_storages.join(', ');
@@ -230,17 +230,17 @@ Ext.define('PVE.window.Migrate', {
severity: 'error',
});
}
+ }
+ }
- if (disallowed['unavailable-resources'] !== undefined) {
- let unavailableResources = disallowed['unavailable-resources'].join(', ');
+ if (disallowed['unavailable-resources'] !== undefined) {
+ let unavailableResources = disallowed['unavailable-resources'].join(', ');
- migration.possible = false;
- migration.preconditions.push({
- text: 'Mapped Resources (' + unavailableResources + ') not available on selected target. ',
- severity: 'error',
- });
- }
- }
+ migration.possible = false;
+ migration.preconditions.push({
+ text: 'Mapped Resources (' + unavailableResources + ') not available on selected target. ',
+ severity: 'error',
+ });
}
let blockingResources = [];
--
2.39.5
_______________________________________________
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
^ permalink raw reply [flat|nested] 51+ messages in thread* [pve-devel] [PATCH manager v6 5/5] fix #5175: ui: allow configuring and live migration of mapped pci resources
2025-02-13 13:16 [pve-devel] [PATCH (guest-)common/qemu-server/manager/docs v6] implement Dominik Csapak
` (16 preceding siblings ...)
2025-02-13 13:17 ` [pve-devel] [PATCH manager v6 4/5] ui: adapt migration window to precondition api change Dominik Csapak
@ 2025-02-13 13:17 ` Dominik Csapak
2025-03-07 14:33 ` Fiona Ebner
2025-02-13 13:17 ` [pve-devel] [PATCH docs v6 1/2] qm: resource mapping: add description for `mdev` option Dominik Csapak
` (4 subsequent siblings)
22 siblings, 1 reply; 51+ messages in thread
From: Dominik Csapak @ 2025-02-13 13:17 UTC (permalink / raw)
To: pve-devel
if the hardware/driver is capable, the admin can now mark a pci device
as 'live-migration-capable', which then tries enabling live migration
for such devices.
mark it as experimental when configuring and in the migrate window
Signed-off-by: Dominik Csapak <d.csapak@proxmox.com>
---
no changes in v6
www/manager6/window/Migrate.js | 25 ++++++++++++++++++++-----
www/manager6/window/PCIMapEdit.js | 12 ++++++++++++
2 files changed, 32 insertions(+), 5 deletions(-)
diff --git a/www/manager6/window/Migrate.js b/www/manager6/window/Migrate.js
index 604b63e7..46fc0cf3 100644
--- a/www/manager6/window/Migrate.js
+++ b/www/manager6/window/Migrate.js
@@ -244,10 +244,10 @@ Ext.define('PVE.window.Migrate', {
}
let blockingResources = [];
- let mappedResources = migrateStats['mapped-resources'] ?? [];
+ let mappedResources = migrateStats['mapped-resource-info'] ?? {};
for (const res of migrateStats.local_resources) {
- if (mappedResources.indexOf(res) === -1) {
+ if (!mappedResources[res]) {
blockingResources.push(res);
}
}
@@ -271,14 +271,29 @@ Ext.define('PVE.window.Migrate', {
}
}
- if (mappedResources && mappedResources.length) {
- if (vm.get('running')) {
+ if (mappedResources && vm.get('running')) {
+ let allowed = [];
+ let notAllowed = [];
+ for (const [key, resource] of Object.entries(mappedResources)) {
+ if (resource['live-migration']) {
+ allowed.push(key);
+ } else {
+ notAllowed.push(key);
+ }
+ }
+ if (notAllowed.length > 0) {
migration.possible = false;
migration.preconditions.push({
text: Ext.String.format('Can\'t migrate running VM with mapped resources: {0}',
- mappedResources.join(', ')),
+ notAllowed.join(', ')),
severity: 'error',
});
+ } else if (allowed.length > 0) {
+ migration.preconditions.push({
+ text: Ext.String.format('Live-migrating running VM with mapped resources (Experimental): {0}',
+ allowed.join(', ')),
+ severity: 'warning',
+ });
}
}
diff --git a/www/manager6/window/PCIMapEdit.js b/www/manager6/window/PCIMapEdit.js
index faf58255..9f2ea651 100644
--- a/www/manager6/window/PCIMapEdit.js
+++ b/www/manager6/window/PCIMapEdit.js
@@ -244,6 +244,18 @@ Ext.define('PVE.window.PCIMapEditWindow', {
disabled: '{hideComment}',
},
},
+ {
+ xtype: 'proxmoxcheckbox',
+ fieldLabel: gettext('Live Migration Capable'),
+ labelWidth: 200,
+ boxLabel: `<i class="fa fa-exclamation-triangle warning"></i> ${gettext('Experimental')}`,
+ reference: 'live-migration-capable',
+ name: 'live-migration-capable',
+ cbind: {
+ deleteEmpty: '{!isCreate}',
+ disabled: '{hideComment}',
+ },
+ },
],
columnB: [
--
2.39.5
_______________________________________________
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
^ permalink raw reply [flat|nested] 51+ messages in thread* Re: [pve-devel] [PATCH manager v6 5/5] fix #5175: ui: allow configuring and live migration of mapped pci resources
2025-02-13 13:17 ` [pve-devel] [PATCH manager v6 5/5] fix #5175: ui: allow configuring and live migration of mapped pci resources Dominik Csapak
@ 2025-03-07 14:33 ` Fiona Ebner
0 siblings, 0 replies; 51+ messages in thread
From: Fiona Ebner @ 2025-03-07 14:33 UTC (permalink / raw)
To: Proxmox VE development discussion, Dominik Csapak
Am 13.02.25 um 14:17 schrieb Dominik Csapak:
> if the hardware/driver is capable, the admin can now mark a pci device
> as 'live-migration-capable', which then tries enabling live migration
> for such devices.
>
> mark it as experimental when configuring and in the migrate window
>
> Signed-off-by: Dominik Csapak <d.csapak@proxmox.com>
Reviewed-by: Fiona Ebner <f.ebner@proxmox.com>
> ---
> no changes in v6
> www/manager6/window/Migrate.js | 25 ++++++++++++++++++++-----
> www/manager6/window/PCIMapEdit.js | 12 ++++++++++++
> 2 files changed, 32 insertions(+), 5 deletions(-)
>
> diff --git a/www/manager6/window/Migrate.js b/www/manager6/window/Migrate.js
> index 604b63e7..46fc0cf3 100644
> --- a/www/manager6/window/Migrate.js
> +++ b/www/manager6/window/Migrate.js
> @@ -244,10 +244,10 @@ Ext.define('PVE.window.Migrate', {
> }
>
> let blockingResources = [];
> - let mappedResources = migrateStats['mapped-resources'] ?? [];
> + let mappedResources = migrateStats['mapped-resource-info'] ?? {};
>
> for (const res of migrateStats.local_resources) {
> - if (mappedResources.indexOf(res) === -1) {
> + if (!mappedResources[res]) {
> blockingResources.push(res);
> }
> }
> @@ -271,14 +271,29 @@ Ext.define('PVE.window.Migrate', {
> }
> }
>
> - if (mappedResources && mappedResources.length) {
> - if (vm.get('running')) {
> + if (mappedResources && vm.get('running')) {
Correct me if I'm wrong, but doesn't mappedResources always evaluate to
true because it's {} (and same was true for [])? To get the same as
before, don't you need to check the number of keys now? But I suppose
the code handles the case where it's empty just fine, so we could also
skip that check.
> + let allowed = [];
> + let notAllowed = [];
> + for (const [key, resource] of Object.entries(mappedResources)) {
> + if (resource['live-migration']) {
> + allowed.push(key);
> + } else {
> + notAllowed.push(key);
> + }
> + }
> + if (notAllowed.length > 0) {
> migration.possible = false;
> migration.preconditions.push({
> text: Ext.String.format('Can\'t migrate running VM with mapped resources: {0}',
> - mappedResources.join(', ')),
> + notAllowed.join(', ')),
> severity: 'error',
> });
> + } else if (allowed.length > 0) {
> + migration.preconditions.push({
> + text: Ext.String.format('Live-migrating running VM with mapped resources (Experimental): {0}',
> + allowed.join(', ')),
> + severity: 'warning',
> + });
> }
> }
>
> diff --git a/www/manager6/window/PCIMapEdit.js b/www/manager6/window/PCIMapEdit.js
> index faf58255..9f2ea651 100644
> --- a/www/manager6/window/PCIMapEdit.js
> +++ b/www/manager6/window/PCIMapEdit.js
> @@ -244,6 +244,18 @@ Ext.define('PVE.window.PCIMapEditWindow', {
> disabled: '{hideComment}',
> },
> },
> + {
> + xtype: 'proxmoxcheckbox',
> + fieldLabel: gettext('Live Migration Capable'),
> + labelWidth: 200,
> + boxLabel: `<i class="fa fa-exclamation-triangle warning"></i> ${gettext('Experimental')}`,
> + reference: 'live-migration-capable',
> + name: 'live-migration-capable',
> + cbind: {
> + deleteEmpty: '{!isCreate}',
> + disabled: '{hideComment}',
Pre-existing above, but might be better to rename the 'hideComment' if
we reuse it for different things.
> + },
> + },
> ],
>
> columnB: [
_______________________________________________
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
^ permalink raw reply [flat|nested] 51+ messages in thread
* [pve-devel] [PATCH docs v6 1/2] qm: resource mapping: add description for `mdev` option
2025-02-13 13:16 [pve-devel] [PATCH (guest-)common/qemu-server/manager/docs v6] implement Dominik Csapak
` (17 preceding siblings ...)
2025-02-13 13:17 ` [pve-devel] [PATCH manager v6 5/5] fix #5175: ui: allow configuring and live migration of mapped pci resources Dominik Csapak
@ 2025-02-13 13:17 ` Dominik Csapak
2025-02-13 13:17 ` [pve-devel] [PATCH docs v6 2/2] qm: resource mapping: document `live-migration-capable` setting Dominik Csapak
` (3 subsequent siblings)
22 siblings, 0 replies; 51+ messages in thread
From: Dominik Csapak @ 2025-02-13 13:17 UTC (permalink / raw)
To: pve-devel
in a new section about additional options
Signed-off-by: Dominik Csapak <d.csapak@proxmox.com>
---
no changes in v6
qm.adoc | 12 ++++++++++++
1 file changed, 12 insertions(+)
diff --git a/qm.adoc b/qm.adoc
index 4bb8f2c..6f337fe 100644
--- a/qm.adoc
+++ b/qm.adoc
@@ -1944,6 +1944,18 @@ To create mappings `Mapping.Modify` on `/mapping/<type>/<name>` is necessary
To use these mappings, `Mapping.Use` on `/mapping/<type>/<name>` is necessary
(in addition to the normal guest privileges to edit the configuration).
+.Additional Options
+
+There are additional options when defining a cluster wide resource mapping.
+Currently there are the following options:
+
+* `mdev` (PCI): This marks the PCI device as being capable of providing
+ `mediated devices`. When this is enabled, you can select a type when
+ configuring it on the guest. If multiple PCI devices are selected for the
+ mapping, the mediated device will be created on the first one where there are
+ any available instances of the selected type.
+
+
Managing Virtual Machines with `qm`
------------------------------------
--
2.39.5
_______________________________________________
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
^ permalink raw reply [flat|nested] 51+ messages in thread* [pve-devel] [PATCH docs v6 2/2] qm: resource mapping: document `live-migration-capable` setting
2025-02-13 13:16 [pve-devel] [PATCH (guest-)common/qemu-server/manager/docs v6] implement Dominik Csapak
` (18 preceding siblings ...)
2025-02-13 13:17 ` [pve-devel] [PATCH docs v6 1/2] qm: resource mapping: add description for `mdev` option Dominik Csapak
@ 2025-02-13 13:17 ` Dominik Csapak
2025-02-13 13:30 ` [pve-devel] [PATCH (guest-)common/qemu-server/manager/docs v6] implement Dominik Csapak
` (2 subsequent siblings)
22 siblings, 0 replies; 51+ messages in thread
From: Dominik Csapak @ 2025-02-13 13:17 UTC (permalink / raw)
To: pve-devel
Signed-off-by: Dominik Csapak <d.csapak@proxmox.com>
---
no changes in v6
qm.adoc | 6 ++++++
1 file changed, 6 insertions(+)
diff --git a/qm.adoc b/qm.adoc
index 6f337fe..0d18d7e 100644
--- a/qm.adoc
+++ b/qm.adoc
@@ -1955,6 +1955,12 @@ Currently there are the following options:
mapping, the mediated device will be created on the first one where there are
any available instances of the selected type.
+* `live-migration-capable` (PCI): This marks the PCI device as being capable of
+ being live migrated between nodes. This requires driver and hardware support.
+ Only NVIDIA GPUs with recent kernel are known to support this. Note that live
+ migrating passed through devices is an experimental feature and may not work
+ or cause issues.
+
Managing Virtual Machines with `qm`
------------------------------------
--
2.39.5
_______________________________________________
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
^ permalink raw reply [flat|nested] 51+ messages in thread* Re: [pve-devel] [PATCH (guest-)common/qemu-server/manager/docs v6] implement
2025-02-13 13:16 [pve-devel] [PATCH (guest-)common/qemu-server/manager/docs v6] implement Dominik Csapak
` (19 preceding siblings ...)
2025-02-13 13:17 ` [pve-devel] [PATCH docs v6 2/2] qm: resource mapping: document `live-migration-capable` setting Dominik Csapak
@ 2025-02-13 13:30 ` Dominik Csapak
2025-03-07 14:39 ` Fiona Ebner
2025-03-11 13:23 ` Dominik Csapak
22 siblings, 0 replies; 51+ messages in thread
From: Dominik Csapak @ 2025-02-13 13:30 UTC (permalink / raw)
To: pve-devel
meh, somehow the commit subject line got cut off (maybe forgot to save?)
should have been:
"implement experimental vgpu live migration"
sorry for the inconvenience
_______________________________________________
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
^ permalink raw reply [flat|nested] 51+ messages in thread* Re: [pve-devel] [PATCH (guest-)common/qemu-server/manager/docs v6] implement
2025-02-13 13:16 [pve-devel] [PATCH (guest-)common/qemu-server/manager/docs v6] implement Dominik Csapak
` (20 preceding siblings ...)
2025-02-13 13:30 ` [pve-devel] [PATCH (guest-)common/qemu-server/manager/docs v6] implement Dominik Csapak
@ 2025-03-07 14:39 ` Fiona Ebner
2025-03-11 13:23 ` Dominik Csapak
22 siblings, 0 replies; 51+ messages in thread
From: Fiona Ebner @ 2025-03-07 14:39 UTC (permalink / raw)
To: Proxmox VE development discussion, Dominik Csapak
Am 13.02.25 um 14:16 schrieb Dominik Csapak:
> pve-docs:
>
> Dominik Csapak (2):
> qm: resource mapping: add description for `mdev` option
> qm: resource mapping: document `live-migration-capable` setting
Those two as well:
Reviewed-by: Fiona Ebner <f.ebner@proxmox.com>
Good work on this series!
_______________________________________________
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
^ permalink raw reply [flat|nested] 51+ messages in thread* Re: [pve-devel] [PATCH (guest-)common/qemu-server/manager/docs v6] implement
2025-02-13 13:16 [pve-devel] [PATCH (guest-)common/qemu-server/manager/docs v6] implement Dominik Csapak
` (21 preceding siblings ...)
2025-03-07 14:39 ` Fiona Ebner
@ 2025-03-11 13:23 ` Dominik Csapak
22 siblings, 0 replies; 51+ messages in thread
From: Dominik Csapak @ 2025-03-11 13:23 UTC (permalink / raw)
To: pve-devel
sent a v7:
https://lore.proxmox.com/pve-devel/20250311132055.2826686-1-d.csapak@proxmox.com/
_______________________________________________
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
^ permalink raw reply [flat|nested] 51+ messages in thread