[pve-devel] [PATCH qemu-server 1/2] schema: vga: mention that type 'cirrus' should not be used
[0]: https://www.kraxel.org/blog/2014/10/qemu-using-cirrus-considered-harmful/
[1]:
https://lore.kernel.org/qemu-devel/usd6hvncbao47zklcb5qlpvjcuk7odryu57f45imxienyltlec@2ujm6g2gr2od/
Signed-off-by: Fiona Ebner
---
PVE/QemuServer.pm | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/PVE/QemuServer.pm b/PVE/QemuServer.pm
index 5df0c96d..cf593383 100644
--- a/PVE/QemuServer.pm
+++ b/PVE/QemuServer.pm
@@ -179,7 +179,7 @@ my $agent_fmt = {
my $vga_fmt = {
type => {
- description => "Select the VGA type.",
+ description => "Select the VGA type. Using type 'cirrus' is not
recommended.",
type => 'string',
default => 'std',
optional => 1,
--
2.39.2
___
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
[pve-devel] [PATCH qemu-server 2/2] cfg2cmd: split out helper for vga properties
To remove some line bloat from the config_to_command() function.
Signed-off-by: Fiona Ebner
---
Original motivation was to prepare for a deprectation warning for
display type 'cirrus'. However, it might not get deprecated after
all:
https://lore.kernel.org/qemu-devel/usd6hvncbao47zklcb5qlpvjcuk7odryu57f45imxienyltlec@2ujm6g2gr2od/
Still, adding the helper doesn't hurt IMHO.
PVE/QemuServer.pm | 35 ++-
1 file changed, 22 insertions(+), 13 deletions(-)
diff --git a/PVE/QemuServer.pm b/PVE/QemuServer.pm
index cf593383..a1f8adea 100644
--- a/PVE/QemuServer.pm
+++ b/PVE/QemuServer.pm
@@ -3493,6 +3493,27 @@ my sub print_ovmf_drive_commandlines {
return ("if=pflash,unit=0,format=raw,readonly=on,file=$ovmf_code",
$var_drive_str);
}
+my sub get_vga_properties {
+my ($conf, $arch, $machine_version, $winversion) = @_;
+
+my $vga = parse_vga($conf->{vga});
+
+my $qxlnum = vga_conf_has_spice($conf->{vga});
+$vga->{type} = 'qxl' if $qxlnum;
+
+if (!$vga->{type}) {
+ if ($arch eq 'aarch64') {
+ $vga->{type} = 'virtio';
+ } elsif (min_version($machine_version, 2, 9)) {
+ $vga->{type} = (!$winversion || $winversion >= 6) ? 'std' :
'cirrus';
+ } else {
+ $vga->{type} = ($winversion >= 6) ? 'std' : 'cirrus';
+ }
+}
+
+return ($vga, $qxlnum);
+}
+
sub config_to_command {
my ($storecfg, $vmid, $conf, $defaults, $forcemachine, $forcecpu,
$live_restore_backing) = @_;
@@ -3644,20 +3665,8 @@ sub config_to_command {
my @usbcontrollers = PVE::QemuServer::USB::get_usb_controllers(
$conf, $bridges, $arch, $machine_type, $machine_version);
push @$devices, @usbcontrollers if @usbcontrollers;
-my $vga = parse_vga($conf->{vga});
-my $qxlnum = vga_conf_has_spice($conf->{vga});
-$vga->{type} = 'qxl' if $qxlnum;
-
-if (!$vga->{type}) {
- if ($arch eq 'aarch64') {
- $vga->{type} = 'virtio';
- } elsif (min_version($machine_version, 2, 9)) {
- $vga->{type} = (!$winversion || $winversion >= 6) ? 'std' :
'cirrus';
- } else {
- $vga->{type} = ($winversion >= 6) ? 'std' : 'cirrus';
- }
-}
+my ($vga, $qxlnum) = get_vga_properties($conf, $arch, $machine_version,
$winversion);
# enable absolute mouse coordinates (needed by vnc)
my $tablet = $conf->{tablet};
--
2.39.2
___
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
Re: [pve-devel] [PATCH guest-common/qemu-server/manager/docs v3 0/4] implement experimental vgpu live migration
Am 19.04.24 um 14:45 schrieb Dominik Csapak: > > qemu-server: > > Dominik Csapak (10): > usb: mapping: move implementation of find_on_current_node here > pci: mapping: move implementation of find_on_current_node here > pci: mapping: check mdev config against hardware > stop cleanup: remove unnecessary tpmstate cleanup > vm_stop_cleanup: add noerr parameter > migrate: call vm_stop_cleanup after stopping in phase3_cleanup > pci: set 'enable-migration' to on for live-migration marked mapped > devices > check_local_resources: add more info per mapped device and return as > hash > api: enable live migration for marked mapped pci devices > api: include not mapped resources for running vms in migrate > preconditions > With the hunk that doesn't belong to 07/10 moved to 03/10, consider the qemu-server patches up to 09/10: Reviewed-by: Fiona Ebner ___ pve-devel mailing list pve-devel@lists.proxmox.com https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
Re: [pve-devel] [PATCH qemu-server v3 10/10] api: include not mapped resources for running vms in migrate preconditions
Am 19.04.24 um 14:45 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 allowed_nodes too.
>
> 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
> ---
> PVE/API2/Qemu.pm | 39 ++-
> 1 file changed, 22 insertions(+), 17 deletions(-)
>
> diff --git a/PVE/API2/Qemu.pm b/PVE/API2/Qemu.pm
> index f95d8d95..94aa9942 100644
> --- a/PVE/API2/Qemu.pm
> +++ b/PVE/API2/Qemu.pm
> @@ -4450,18 +4450,20 @@ __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 => { type => 'boolean' },
> allowed_nodes => {
> type => 'array',
> optional => 1,
> - description => "List nodes allowed for offline migration, only
> passed if VM is offline"
> + description => "List nodes allowed for offline migration.",
It still only returns the actual list of allowed nodes if not running.
My idea was to return the allowed nodes in both cases. If we keep the
parameter specific to offline migration, I'd still keep the return guarded.
> },
> not_allowed_nodes => {
> type => 'object',
> optional => 1,
> - description => "List not allowed nodes with additional
> informations, only passed if VM is offline"
> + description => "List not allowed nodes with additional
> information.",
> },
> local_disks => {
> type => 'array',
> @@ -4518,28 +4520,31 @@ __PACKAGE__->register_method({
>
> # if vm is not running, return target nodes where local storage/mapped
> devices are available
> # for offline migration
> + $res->{allowed_nodes} = [];
> + $res->{not_allowed_nodes} = {};
> 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->{not_allowed_nodes} =
> PVE::QemuServer::check_local_storage_availability($vmconf, $storecfg);
> + delete $res->{not_allowed_nodes}->{$localnode};
> + }
>
> - if (!defined($checked_nodes->{$node}->{unavailable_storages})) {
> - push @{$res->{allowed_nodes}}, $node;
> - }
> + my $merged = { $res->{not_allowed_nodes}->%*,
> $missing_mappings_by_node->%* };
>
If we'd need this, I'd just get the keys directly:
my @keys = keys { ... }->%*;
But it just reads wrong. Why even consider the keys for the
not_allowed_nodes? Doesn't this just work because
$missing_mappings_by_node already contains all other node keys (and so
we can simply iterate over those keys)?
> + for my $node (keys $merged->%*) {
> + my $missing_mappings = $missing_mappings_by_node->{$node};
> + if (scalar($missing_mappings->@*)) {
> + $res->{not_allowed_nodes}->{$node}->{'unavailable-resources'} =
> $missing_mappings;
> + next;
> + }
> +
> + if (!$res->{running}) {
> + if
> (!defined($res->{not_allowed_nodes}->{$node}->{unavailable_storages})) {
> + 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'} = [ keys $mapped_resources->%* ];
___
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
Re: [pve-devel] [PATCH qemu-server v3 09/10] api: enable live migration for marked mapped pci devices
Am 19.04.24 um 14:45 schrieb Dominik Csapak:
> They have to be marked as 'live-migration-capable' in the mapping
> config, and the driver and qemu must support it.
>
> For the gui checks, we now return the whole object of the mapped
> resources, which includes info like the name and if it's marked as
> live-migration capable. (while deprecating the old 'mapped-resource'
> return value, since that returns strictly less information)
>
> Signed-off-by: Dominik Csapak
> ---
> PVE/API2/Qemu.pm | 7 ++-
> PVE/QemuMigrate.pm | 13 +
> 2 files changed, 15 insertions(+), 5 deletions(-)
>
> diff --git a/PVE/API2/Qemu.pm b/PVE/API2/Qemu.pm
> index f2fa345d..f95d8d95 100644
> --- a/PVE/API2/Qemu.pm
> +++ b/PVE/API2/Qemu.pm
> @@ -4473,7 +4473,11 @@ __PACKAGE__->register_method({
> },
> 'mapped-resources' => {
> type => 'array',
> - description => "List of mapped resources e.g. pci, usb"
> + description => "List of mapped resources e.g. pci, usb.
> Deprecated, use 'mapped-resource-info' instead."
> + },
Please add a comment to remind us to remove it in PVE 9
> + 'mapped-resource-info' => {
> + type => 'object',
> + description => "Object of mapped resources with additional
> information such if they're live migratable.",
> },
> },
> },
> @@ -4539,6 +4543,7 @@ __PACKAGE__->register_method({
>
> $res->{local_resources} = $local_resources;
> $res->{'mapped-resources'} = [ keys $mapped_resources->%* ];
> + $res->{'mapped-resource-info'} = $mapped_resources;
>
> return $res;
>
> diff --git a/PVE/QemuMigrate.pm b/PVE/QemuMigrate.pm
> index a46eb2a3..89caefc7 100644
> --- a/PVE/QemuMigrate.pm
> +++ b/PVE/QemuMigrate.pm
> @@ -247,11 +247,16 @@ sub prepare {
>
> if (scalar(keys $mapped_res->%*)) {
> my $missing_mappings = $missing_mappings_by_node->{$self->{node}};
> - my $mapped_text = join(", ", keys $mapped_res->%*);
> - if ($running) {
> - die "can't migrate running VM which uses mapped devices:
> $mapped_text\n";
> - } elsif (scalar($missing_mappings->@*)) {
> + my $missing_live_mappings = [];
> + for my $key (keys $mapped_res->%*) {
Maybe sort
> + my $res = $mapped_res->{$key};
> + my $name = "$key:$res->{name}";
> + push $missing_live_mappings->@*, $name if !$res->{'live-migration'};
> + }
> + if (scalar($missing_mappings->@*)) {
> die "can't migrate to '$self->{node}': missing mapped devices " .
> join(", ", $missing_mappings->@*) . "\n";
> + } elsif ($running && scalar($missing_live_mappings->@*)) {
> + die "can't live migrate running VM which uses following mapped
> devices: " . join(", ", $missing_live_mappings->@*) . "\n";
Style nit: line too long
> } else {
> $self->log('info', "migrating VM which uses mapped local devices");
> }
___
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
Re: [pve-devel] [PATCH qemu-server v3 08/10] check_local_resources: add more info per mapped device and return as hash
Am 19.04.24 um 14:45 schrieb Dominik Csapak:
> such as the mapping name and if it's marked for live-migration (pci only)
>
> Signed-off-by: Dominik Csapak
> ---
> 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 2a349c8c..f2fa345d 100644
> --- a/PVE/API2/Qemu.pm
> +++ b/PVE/API2/Qemu.pm
> @@ -4538,7 +4538,7 @@ __PACKAGE__->register_method({
> $res->{local_disks} = [ values %$local_disks ];;
>
> $res->{local_resources} = $local_resources;
> - $res->{'mapped-resources'} = $mapped_resources;
> + $res->{'mapped-resources'} = [ keys $mapped_resources->%* ];
Sorting the keys leads to a nicer API result IMHO.
>
> return $res;
>
> diff --git a/PVE/QemuMigrate.pm b/PVE/QemuMigrate.pm
> index 381022f5..a46eb2a3 100644
> --- a/PVE/QemuMigrate.pm
> +++ b/PVE/QemuMigrate.pm
> @@ -233,7 +233,7 @@ sub prepare {
> my ($loc_res, $mapped_res, $missing_mappings_by_node) =
> PVE::QemuServer::check_local_resources($conf, 1);
> my $blocking_resources = [];
> for my $res ($loc_res->@*) {
> - if (!grep($res, $mapped_res->@*)) {
> + if (!defined($mapped_res->{$res})) {
> push $blocking_resources->@*, $res;
> }
> }
> @@ -245,10 +245,11 @@ sub prepare {
> }
> }
>
> -if (scalar($mapped_res->@*)) {
> +if (scalar(keys $mapped_res->%*)) {
> my $missing_mappings = $missing_mappings_by_node->{$self->{node}};
> + my $mapped_text = join(", ", keys $mapped_res->%*);
Nit: Can be moved into the if, keys can be sorted.
> if ($running) {
> - die "can't migrate running VM which uses mapped devices: " .
> join(", ", $mapped_res->@*) . "\n";
> + 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 {
___
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
Re: [pve-devel] [PATCH qemu-server v3 07/10] pci: set 'enable-migration' to on for live-migration marked mapped devices
Am 19.04.24 um 14:45 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
> ---
> PVE/QemuServer/PCI.pm | 9 -
> 1 file changed, 8 insertions(+), 1 deletion(-)
>
> diff --git a/PVE/QemuServer/PCI.pm b/PVE/QemuServer/PCI.pm
> index 6ba43ee8..df2ea3eb 100644
> --- a/PVE/QemuServer/PCI.pm
> +++ b/PVE/QemuServer/PCI.pm
> @@ -435,8 +435,11 @@ sub parse_hostpci {
> my $devices = PVE::Mapping::PCI::get_node_mapping($config, $mapping,
> $node);
> die "PCI device mapping not found for '$mapping'\n" if !$devices ||
> !scalar($devices->@*);
>
> + 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) };
This hunk belongs in patch 03/10
> die "PCI device mapping invalid (hardware probably changed): $@\n"
> if $@;
> push $alternatives->@*, [split(/;/, $device->{path})];
> }
> @@ -635,6 +638,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}";
>
___
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
Re: [pve-devel] [PATCH qemu-server v3 06/10] migrate: call vm_stop_cleanup after stopping in phase3_cleanup
Am 19.04.24 um 14:45 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
> ---
> 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 8d9b35ae..381022f5 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::Cluster;
Needs a rebase (because of added include for PVE::AccessControl)
> @@ -1455,7 +1456,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};
> @@ -1586,12 +1588,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 for vm failed - $err");
Nit: "Cleanup after stopping VM failed"
Is it better to only execute this in case vm_stop() did not return an
error? Although I guess attempting cleanup in that case also doesn't hurt.
> $self->{errors} = 1;
> }
>
> diff --git a/test/MigrationTest/Shared.pm b/test/MigrationTest/Shared.pm
> index aa7203d1..2347e60a 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 1;
Nit: I'd just have it be return; without a value.
> +},
> get_efivars_size => sub {
>return 128 * 1024;
> },
___
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
Re: [pve-devel] [PATCH qemu-server v3 04/10] stop cleanup: remove unnecessary tpmstate cleanup
Adding the rationale for third-party plugins from last time:
https://lists.proxmox.com/pipermail/pve-devel/2024-March/062354.html
Am 19.04.24 um 14:45 schrieb Dominik Csapak:
> tpmstate0 is already included in `get_vm_volumes`, and our only storage
> plugin that has unmap_volume implemented is the RBDPlugin, where we call
> unmap in `deactivate_volume`. So it's already ummapped by the
> `deactivate_volumes` calls above.
>
For third-party storage plugins, it's natural to expect that
deactivate_volume() would also remove a mapping for the volume just
like RBDPlugin does.
While there is an explicit map_volume() call in start_swtpm(), a
third-party plugin might expect an explicit unmap_volume() call too.
However, the order of calls right now is
1. activate_volume()
2. map_volume()
3. deactivate_volume()
4. unmap_volume()
Which seems like it could cause problems already for third-party
plugins relying on an explicit unmap call.
All that said, it is unlikely that a third-party plugin breaks. If it
really happens, it can be discussed/adapted to the actual needs still.
> Signed-off-by: Dominik Csapak
Acked-by: Fiona Ebner
> ---
> PVE/QemuServer.pm | 8
> 1 file changed, 8 deletions(-)
>
> diff --git a/PVE/QemuServer.pm b/PVE/QemuServer.pm
> index 28e630d3..680c36f2 100644
> --- a/PVE/QemuServer.pm
> +++ b/PVE/QemuServer.pm
> @@ -6172,14 +6172,6 @@ sub vm_stop_cleanup {
> if (!$keepActive) {
> my $vollist = get_vm_volumes($conf);
> PVE::Storage::deactivate_volumes($storecfg, $vollist);
> -
> - if (my $tpmdrive = $conf->{tpmstate0}) {
> - my $tpm = parse_drive("tpmstate0", $tpmdrive);
> - my ($storeid, $volname) =
> PVE::Storage::parse_volume_id($tpm->{file}, 1);
> - if ($storeid) {
> - PVE::Storage::unmap_volume($storecfg, $tpm->{file});
> - }
> - }
> }
>
> foreach my $ext (qw(mon qmp pid vnc qga)) {
___
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
Re: [pve-devel] [PATCH guest-common v3 4/4] mapping: remove find_on_current_node
Am 19.04.24 um 14:45 schrieb Dominik Csapak:
> they only have one user each (where we can inline the implementation).
> It's easy enough to recreate should we need to.
>
> Signed-off-by: Dominik Csapak
> ---
> src/PVE/Mapping/PCI.pm | 10 --
> src/PVE/Mapping/USB.pm | 9 -
> 2 files changed, 19 deletions(-)
>
> diff --git a/src/PVE/Mapping/PCI.pm b/src/PVE/Mapping/PCI.pm
> index b525c07..9c2b8b2 100644
> --- a/src/PVE/Mapping/PCI.pm
> +++ b/src/PVE/Mapping/PCI.pm
> @@ -214,16 +214,6 @@ sub write_pci_config {
> cfs_write_file($FILENAME, $cfg);
> }
>
> -sub find_on_current_node {
> -my ($id) = @_;
> -
> -my $cfg = PVE::Mapping::PCI::config();
> -my $node = PVE::INotify::nodename();
> -
> -# ignore errors
> -return get_node_mapping($cfg, $id, $node);
> -}
> -
> sub get_node_mapping {
> my ($cfg, $id, $nodename) = @_;
>
> diff --git a/src/PVE/Mapping/USB.pm b/src/PVE/Mapping/USB.pm
> index 34bc3cb..6dd15c4 100644
> --- a/src/PVE/Mapping/USB.pm
> +++ b/src/PVE/Mapping/USB.pm
> @@ -155,15 +155,6 @@ sub write_usb_config {
> cfs_write_file($FILENAME, $cfg);
> }
>
> -sub find_on_current_node {
> -my ($id) = @_;
> -
> -my $cfg = config();
> -my $node = PVE::INotify::nodename();
> -
> -return get_node_mapping($cfg, $id, $node);
> -}
> -
> sub get_node_mapping {
> my ($cfg, $id, $nodename) = @_;
>
Seems like the use PVE::INotify; statements can be dropped too now.
___
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
Re: [pve-devel] [PATCH guest-common v3 2/4] mapping: pci: check the mdev configuration on the device too
Am 19.04.24 um 14:45 schrieb Dominik Csapak:
> 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
> ---
> changes from v2:
> * adapt to changes of previous patch
> src/PVE/Mapping/PCI.pm | 5 -
> 1 file changed, 4 insertions(+), 1 deletion(-)
>
> diff --git a/src/PVE/Mapping/PCI.pm b/src/PVE/Mapping/PCI.pm
> index ef1bd8d..b412c4d 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, $cfg) = @_;
>
> my @paths = split(';', $mapping->{path} // '');
>
> @@ -151,6 +151,7 @@ sub assert_valid {
> my $expected_props = {
> id => "$info->{vendor}:$info->{device}",
> iommugroup => $info->{iommugroup},
> + mdev => $info->{mdev},
The value here returned by SysFSTools is undef when not supported...
> };
>
> if (defined($info->{'subsystem_vendor'}) &&
> defined($info->{'subsystem_device'})) {
> @@ -158,6 +159,8 @@ sub assert_valid {
> }
>
> my $configured_props = { $mapping->%{qw(id iommugroup subsystem-id)} };
> + # check mdev from globabl mapping config
> + $configured_props->{mdev} = $cfg->{mdev};
>
...while the value in the config can be an explicit '0', which will trip
up the check below. (It can also be undef in the config and will be via UI).
Also leads to the error
unexpected property 'mdev' configured for device ':00:1b.0'
when SysFSTools returns undef and the property is present in the in the
config, even though it's not per-se unexpected. If we convert to
explicit "0" and "1" values, we would also get a better error message
that points out the value mismatch.
___
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
Re: [pve-devel] [PATCH guest-common v3 1/4] mapping: pci: rework properties check
Am 19.04.24 um 14:45 schrieb Dominik Csapak:
> rename '$cfg' to '$mapping', 'correct' to 'expected'
> reword the error messages
> also check keys from the configured props not only the expected ones
>
Would've been nicer as multiple commits.
> previously we only checked the keys from the 'correct_props' hash
> but that was unintended. We now check the keys from both, but extract
> the relevant properties first.
>
> Signed-off-by: Dominik Csapak
> ---
> changes from v2:
> * don't refactor the properties check out
> * use properties from both configured and expected hashes
> * extract the relevant configured properties from the mapping
> instead of using all (previously we only used the expected ones
> by accident)
> src/PVE/Mapping/PCI.pm | 34 +++---
> 1 file changed, 19 insertions(+), 15 deletions(-)
>
> diff --git a/src/PVE/Mapping/PCI.pm b/src/PVE/Mapping/PCI.pm
> index 725e106..ef1bd8d 100644
> --- a/src/PVE/Mapping/PCI.pm
> +++ b/src/PVE/Mapping/PCI.pm
> @@ -131,9 +131,9 @@ sub options {
>
> # checks if the given config is valid for the current node
> sub assert_valid {
> -my ($name, $cfg) = @_;
> +my ($name, $mapping) = @_;
>
> -my @paths = split(';', $cfg->{path} // '');
> +my @paths = split(';', $mapping->{path} // '');
>
> my $idx = 0;
> for my $path (@paths) {
> @@ -148,32 +148,36 @@ sub assert_valid {
> my $info = PVE::SysFSTools::pci_device_info($path, 1);
> die "pci device '$path' not found\n" if !defined($info);
>
> - my $correct_props = {
My suggestion is the following code changes. See below for rationale[0].
# make sure to initialize all keys that should be checked below!
> + my $expected_props = {
> id => "$info->{vendor}:$info->{device}",
> iommugroup => $info->{iommugroup},
'subsystem-id' => undef,
> };
>
> if (defined($info->{'subsystem_vendor'}) &&
> defined($info->{'subsystem_device'})) {
> - $correct_props->{'subsystem-id'} =
> "$info->{'subsystem_vendor'}:$info->{'subsystem_device'}";
> + $expected_props->{'subsystem-id'} =
> "$info->{'subsystem_vendor'}:$info->{'subsystem_device'}";
> }
>
> - for my $prop (sort keys %$correct_props) {
> + my $configured_props = { $mapping->%{qw(id iommugroup subsystem-id)} };
> +
> + my $merged = { %$expected_props, %$configured_props }; # just for the
> keys
> + for my $prop (sort keys %$merged) {
I'd prefer to just extract the keys directly and avoid the comment:
my @keys = keys { $expected_props->%*, $configured_props->%* }->%*;
[0]: But we could also just initialize $expected_props like mentioned
above and then simply use the keys from there. Then you also don't need
to construct a new hash for $configured_props and introduce a new
hard-coded list ;)
> next if $prop eq 'iommugroup' && $idx > 0; # check iommu only on
> the first device
>
> - next if !defined($correct_props->{$prop}) &&
> !defined($cfg->{$prop});
> - die "no '$prop' for device '$path'\n"
> - if defined($correct_props->{$prop}) && !defined($cfg->{$prop});
> - die "'$prop' configured but should not be\n"
> - if !defined($correct_props->{$prop}) && defined($cfg->{$prop});
> + next if !defined($expected_props->{$prop}) &&
> !defined($configured_props->{$prop});
> + die "missing expected property '$prop' for device '$path'\n"
> + if defined($expected_props->{$prop}) &&
> !defined($configured_props->{$prop});
> + die "unexpected property '$prop' configured for device '$path'\n"
> + if !defined($expected_props->{$prop}) &&
> defined($configured_props->{$prop});
>
> - my $correct_prop = $correct_props->{$prop};
> - $correct_prop =~ s/0x//g;
> - my $configured_prop = $cfg->{$prop};
> + my $expected_prop = $expected_props->{$prop};
> + $expected_prop =~ s/0x//g;
> + my $configured_prop = $configured_props->{$prop};
> $configured_prop =~ s/0x//g;
>
> - die "'$prop' does not match for '$name' ($correct_prop !=
> $configured_prop)\n"
> - if $correct_prop ne $configured_prop;
> + die "'$prop' does not match for '$name' ($expected_prop !=
> $configured_prop)\n"
> + if $expected_prop ne $configured_prop;
> }
> +
> $idx++;
> }
>
___
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
Re: [pve-devel] [PATCH guest-common/qemu-server/manager/docs v3 0/4] implement experimental vgpu live migration
Am 19.04.24 um 14:45 schrieb Dominik Csapak: > and some useful cleanups > > Resending even there was not much feedback, because i worked in some > minor fixes/changes in the meantime. > > A user tested the previous patch series and only found one issue with > the ui, see the comments on bug #5175 > > https://bugzilla.proxmox.com/show_bug.cgi?id=5175 > > > This is implemented for mapped resources. This requires driver and > hardware support, but aside from nvidia vgpus there don't seem to be > many drivers (if any) that do support that. > > qemu already supports that for vfio-pci devices, so nothing to be > done there besides actively enabling it. > > Since we currently can't properly test it here and very much depends on > hardware/driver support, mark it as experimental everywhere (docs/api/gui). > (though i tested the live-migration part manually here by using > "exec:cat > /tmp/test" for the migration target, and "exec: cat > /tmp/test" as the 'incoming' parameter for a new vm start, which worked ;) ) > > i opted for marking them migratable at the mapping level, but we could > theoretically also put it in the hostpciX config instead. > (though imho it fits better in the cluster-wide resource mapping config) > > also the naming/texts could probably be improved, but i think > 'live-migration-capable' is very descriptive and i didn't want to > use an overly short name for it (which can be confusing, see the > 'shared' flag for storages) > > guest-common 2/4 semi-breaks pve-manager without pve-manager 1/5 > and qemu-server without 3/10 > (would always fail for devices capable of mediated devices) > And guest-common 4/4 fully breaks old qemu-server because it removes the find_on_current_node() functions. > guest-common 1,2; qemu-server 1-6; pve-manager 1,2 > are preparations/cleanups mostly and could be applied independently > ___ pve-devel mailing list pve-devel@lists.proxmox.com https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
[pve-devel] [PATCH docs] backup: warn that tar does not honor exclusion pattern with a trailing slash
As reported in the community forum [0], for tar, an exclusion pattern with a trailing slash will not match a folder with that name. For rsync and proxmox-backup-client however, such a pattern will exclude a directory with that name, but not a file. rsync is used for 'suspend' mode backup and tar is used for all non-PBS backups to create the archive. So currently, in the presence of an exclusion pattern with a trailing slash, there is inconsistency between different backup modes (because for 'suspend' mode, rsync will already do the exclusion too) as well as between PBS and non-PBS backups. There doesn't seem to be a straight-forward way to align the behavior for tar with command-line options exactly. The trailing slash can't be removed from the pattern, because that would also match files. Matching with > some/pattern/* > some/pattern/.* rather than > some/pattern/ gets pretty close, which was suggested by Dominik. Just the empty directory is still included. In any case, modifying current behavior would be a breaking change, so actually aligning the exclusion (more closely) is better done in the next major release. Signed-off-by: Fiona Ebner --- vzdump.adoc | 6 ++ 1 file changed, 6 insertions(+) diff --git a/vzdump.adoc b/vzdump.adoc index 79d4bbc..3918e9a 100644 --- a/vzdump.adoc +++ b/vzdump.adoc @@ -578,6 +578,12 @@ You can also manually specify (additional) exclude paths, for example: excludes the directory `/tmp/` and any file or directory named `/var/foo`, `/var/foobar`, and so on. +WARNING: For backups to Proxmox Backup Server (PBS) and `suspend` mode backups, +patterns with a trailing slash will match directories, but not files. On the +other hand, for non-PBS `snapshot` mode and `stop` mode backups, patterns with a +trailing slash currently do not match at all, because the `tar` command does not +support that. + Paths that do not start with a `/` are not anchored to the container's root, but will match relative to any subdirectory. For example: -- 2.39.2 ___ pve-devel mailing list pve-devel@lists.proxmox.com https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
[pve-devel] [PATCH container] backup: warn that tar does not honor exclusion pattern with a trailing slash
As reported in the community forum [0], for tar, an exclusion pattern
with a trailing slash will not match a folder with that name. For
rsync and proxmox-backup-client however, such a pattern will exclude
a directory with that name, but not a file.
rsync is used for 'suspend' mode backup and tar is used for all
non-PBS backups to create the archive. So currently, in the presence
of an exclusion pattern with a trailing slash, there is inconsistency
between different backup modes (because for 'suspend' mode, rsync will
already do the exclusion too) as well as between PBS and non-PBS
backups.
There doesn't seem to be a straight-forward way to align the behavior
for tar with command-line options exactly. The trailing slash can't be
removed from the pattern, because that would also match files.
Matching with
> some/pattern/*
> some/pattern/.*
rather than
> some/pattern/
gets pretty close, which was suggested by Dominik. Just the empty
directory is still included.
In any case, modifying current behavior would be a breaking change, so
actually aligning the exclusion (more closely) is better done in the
next major release.
Signed-off-by: Fiona Ebner
---
One could argue such a change is a bug-fix and so does not need to
wait until the next major release. I opted for the safer variant for
now, but happy to go with the aligning already if that is preferred.
src/PVE/VZDump/LXC.pm | 11 ++-
1 file changed, 10 insertions(+), 1 deletion(-)
diff --git a/src/PVE/VZDump/LXC.pm b/src/PVE/VZDump/LXC.pm
index 8c28a5e..914ede2 100644
--- a/src/PVE/VZDump/LXC.pm
+++ b/src/PVE/VZDump/LXC.pm
@@ -442,7 +442,16 @@ sub archive {
} else {
push @findexcl_anchored, $pattern;
}
- }
+
+ # NOTE rsync and proxmox-backup-client will match directories, but
not files when there
+ # is a trailing slash, tar won't match either. For suspend mode,
rsync already did the
+ # exclusion, so no need to warn.
+ # TODO PVE 9 - consider matching "$pattern*" and "$pattern.*" in
this case, which will
+ # only include the empty directory to more closely align the
behavior between different
+ # modes. Don't forget to update the docs!
+ $self->log("warn", "tar does not match exclusion with a trailing
slash '$pattern'")
+ if $pattern =~ m|/$| && $task->{mode} ne 'suspend';
+}
push @$tar, '--no-anchored';
push @$tar, '--exclude=lost+found' if $userns_cmd;
--
2.39.2
___
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
Re: [pve-devel] [RFC PATCH manager] api/ui: include the node ha status in resources call and show as icon
sorry there is a word missing in the subject: 'include the node ha status in resources call and show maintenance as icon' ___ pve-devel mailing list pve-devel@lists.proxmox.com https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
Re: [pve-devel] [PATCH guest-common/qemu-server/manager/docs v3 0/4] implement experimental vgpu live migration
On 5/31/24 10:11, Eneko Lacunza wrote: Hi Dominik, Hi, El 28/5/24 a las 9:25, Dominik Csapak escribió: ping? i know we cannot really test this atm but there are a few users/customers that would like to use/test this since i marked it experimental i think it should be ok to include this if its ok code wise Would this patch-series help in this scenario: - Running VM has a Nvidia mdev asigned in hw. - We clone that VM. VM can't be started until we change the hw id to another Nvidia mdev. No that should already work with the cluster device mappings, there you can configure a set of cards that will be used for the mdev creation instead of having one fixed card. see https://pve.proxmox.com/pve-docs/pve-admin-guide.html#resource_mapping for how to configure that this series would enable experimental live migration between hosts for cards/drivers that support this. If so, I have a potential customer that is looking to migrate a VDI deployment from Nutanix to Proxmox. They're currently testing Proxmox with one server with a Nvidia card, and can test this if packages are prepared (in testing?). We deployed this Proxmox server yesterday, we used latest NVIDIA host driver. Latest kernel 6.8 didn't work but latest 6.5 did (will report exact versions when I get remote access). yes we know, see the known issues section of the release noteshttps://pve.proxmox.com/wiki/Roadmap#Proxmox_VE_8.2 Thanks Eneko Lacunza Zuzendari teknikoa | Director técnico Binovo IT Human Project Tel. +34 943 569 206 | https://www.binovo.es Astigarragako Bidea, 2 - 2º izda. Oficina 10-11, 20180 Oiartzun https://www.youtube.com/user/CANALBINOVO https://www.linkedin.com/company/37269706/ ___ pve-devel mailing list pve-devel@lists.proxmox.com https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
Re: [pve-devel] [PATCH guest-common/qemu-server/manager/docs v3 0/4] implement experimental vgpu live migration
--- Begin Message --- Hi Dominik, El 28/5/24 a las 9:25, Dominik Csapak escribió: ping? i know we cannot really test this atm but there are a few users/customers that would like to use/test this since i marked it experimental i think it should be ok to include this if its ok code wise Would this patch-series help in this scenario: - Running VM has a Nvidia mdev asigned in hw. - We clone that VM. VM can't be started until we change the hw id to another Nvidia mdev. If so, I have a potential customer that is looking to migrate a VDI deployment from Nutanix to Proxmox. They're currently testing Proxmox with one server with a Nvidia card, and can test this if packages are prepared (in testing?). We deployed this Proxmox server yesterday, we used latest NVIDIA host driver. Latest kernel 6.8 didn't work but latest 6.5 did (will report exact versions when I get remote access). Thanks Eneko Lacunza Zuzendari teknikoa | Director técnico Binovo IT Human Project Tel. +34 943 569 206 | https://www.binovo.es Astigarragako Bidea, 2 - 2º izda. Oficina 10-11, 20180 Oiartzun https://www.youtube.com/user/CANALBINOVO https://www.linkedin.com/company/37269706/ --- End Message --- ___ pve-devel mailing list pve-devel@lists.proxmox.com https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
[pve-devel] [RFC PATCH manager] api/ui: include the node ha status in resources call and show as icon
we already have the information parsed, so it's cheap, and we already
have a mechanism in place that adds 'ha-' as a css class, so
let's reuse that.
I chose a yellow wrench, as wrenches are associated with 'maintenance',
and because the state warrants more notice than 'online' but less than
'offline'.
Users mentioned in the forum that this would be nice:
https://forum.proxmox.com/threads/125768/
Signed-off-by: Dominik Csapak
---
not sure about the color, since the yellow has relatively low contrast
in the light mode (in dark mode it's fine). It's the same yellow as for
'io-errors' though.
PVE/API2/Cluster.pm | 3 +++
www/css/ext6-pve.css | 8
2 files changed, 11 insertions(+)
diff --git a/PVE/API2/Cluster.pm b/PVE/API2/Cluster.pm
index 04387ab4..4fc838be 100644
--- a/PVE/API2/Cluster.pm
+++ b/PVE/API2/Cluster.pm
@@ -463,6 +463,9 @@ __PACKAGE__->register_method({
if (defined(my $mode = $info->{'cgroup-mode'})) {
$entry->{'cgroup-mode'} = int($mode);
}
+ if (defined(my $status = $hastatus->{node_status}->{$node})) {
+ $entry->{'hastate'} = $status;
+ }
push @$res, $entry;
}
diff --git a/www/css/ext6-pve.css b/www/css/ext6-pve.css
index b5a3683a..83580afb 100644
--- a/www/css/ext6-pve.css
+++ b/www/css/ext6-pve.css
@@ -206,6 +206,14 @@
font-size: 0.75em;
}
+/* yellow wrench */
+.x-tree-icon-custom.ha-maintenance:after,
+.x-grid-icon-custom.ha-maintenance:after {
+content: "\f0ad";
+color: #FFCC00;
+}
+
+
/* yellow ! triangle */
.x-tree-icon-custom.pending:after,
.x-grid-icon-custom.pending:after,
--
2.39.2
___
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
