[pve-devel] [PATCH v5 qemu-server 01/19] Switch to using foreach_volume instead of foreach_drive
Fabian Ebner
f.ebner at proxmox.com
Tue Apr 14 11:48:44 CEST 2020
On 09.04.20 09:53, Fabian Grünbichler wrote:
> small comment in-line
>
> On April 8, 2020 11:24 am, Fabian Ebner wrote:
>> It was necessary to move foreach_volid back to QemuServer.pm
>>
>> In VZDump/QemuServer.pm and QemuMigrate.pm the dependency on
>> QemuConfig.pm was already there, just the explicit "use" was missing.
>>
>> Signed-off-by: Fabian Ebner <f.ebner at proxmox.com>
>> ---
>> PVE/API2/Qemu.pm | 6 ++--
>> PVE/QemuConfig.pm | 2 +-
>> PVE/QemuMigrate.pm | 5 +--
>> PVE/QemuServer.pm | 70 ++++++++++++++++++++++++++++++-------
>> PVE/QemuServer/Cloudinit.pm | 2 +-
>> PVE/QemuServer/Drive.pm | 61 --------------------------------
>> PVE/VZDump/QemuServer.pm | 3 +-
>> 7 files changed, 68 insertions(+), 81 deletions(-)
>>
>> diff --git a/PVE/API2/Qemu.pm b/PVE/API2/Qemu.pm
>> index b220934..87fbd72 100644
>> --- a/PVE/API2/Qemu.pm
>> +++ b/PVE/API2/Qemu.pm
>> @@ -63,7 +63,7 @@ my $NEW_DISK_RE = qr!^(([^/:\s]+):)?(\d+(\.\d+)?)$!;
>> my $check_storage_access = sub {
>> my ($rpcenv, $authuser, $storecfg, $vmid, $settings, $default_storage) = @_;
>>
>> - PVE::QemuServer::foreach_drive($settings, sub {
>> + PVE::QemuConfig->foreach_volume($settings, sub {
>> my ($ds, $drive) = @_;
>>
>> my $isCDROM = PVE::QemuServer::drive_is_cdrom($drive);
>> @@ -96,7 +96,7 @@ my $check_storage_access_clone = sub {
>>
>> my $sharedvm = 1;
>>
>> - PVE::QemuServer::foreach_drive($conf, sub {
>> + PVE::QemuConfig->foreach_volume($conf, sub {
>> my ($ds, $drive) = @_;
>>
>> my $isCDROM = PVE::QemuServer::drive_is_cdrom($drive);
>> @@ -216,7 +216,7 @@ my $create_disks = sub {
>> }
>> };
>>
>> - eval { PVE::QemuServer::foreach_drive($settings, $code); };
>> + eval { PVE::QemuConfig->foreach_volume($settings, $code); };
>>
>> # free allocated images on error
>> if (my $err = $@) {
>> diff --git a/PVE/QemuConfig.pm b/PVE/QemuConfig.pm
>> index d29b88b..78f5c60 100644
>> --- a/PVE/QemuConfig.pm
>> +++ b/PVE/QemuConfig.pm
>> @@ -66,7 +66,7 @@ sub has_feature {
>> my ($class, $feature, $conf, $storecfg, $snapname, $running, $backup_only) = @_;
>>
>> my $err;
>> - PVE::QemuServer::foreach_drive($conf, sub {
>> + $class->foreach_volume($conf, sub {
>> my ($ds, $drive) = @_;
>>
>> return if PVE::QemuServer::drive_is_cdrom($drive);
>> diff --git a/PVE/QemuMigrate.pm b/PVE/QemuMigrate.pm
>> index e83d60d..2925b90 100644
>> --- a/PVE/QemuMigrate.pm
>> +++ b/PVE/QemuMigrate.pm
>> @@ -17,6 +17,7 @@ use PVE::ReplicationState;
>> use PVE::Storage;
>> use PVE::Tools;
>>
>> +use PVE::QemuConfig;
>> use PVE::QemuServer::CPUConfig;
>> use PVE::QemuServer::Drive;
>> use PVE::QemuServer::Helpers qw(min_version);
>> @@ -477,7 +478,7 @@ sub sync_disks {
>> }
>>
>> my $live_replicatable_volumes = {};
>> - PVE::QemuServer::foreach_drive($conf, sub {
>> + PVE::QemuConfig->foreach_volume($conf, sub {
>> my ($ds, $drive) = @_;
>>
>> my $volid = $drive->{file};
>> @@ -508,7 +509,7 @@ sub sync_disks {
>> # sizes in config have to be accurate for remote node to correctly
>> # allocate disks, rescan to be sure
>> my $volid_hash = PVE::QemuServer::scan_volids($storecfg, $vmid);
>> - PVE::QemuServer::foreach_drive($conf, sub {
>> + PVE::QemuConfig->foreach_volume($conf, sub {
>> my ($key, $drive) = @_;
>> my ($updated, $old_size, $new_size) = PVE::QemuServer::Drive::update_disksize($drive, $volid_hash);
>> if (defined($updated)) {
>> diff --git a/PVE/QemuServer.pm b/PVE/QemuServer.pm
>> index 12f76b8..cd34bf6 100644
>> --- a/PVE/QemuServer.pm
>> +++ b/PVE/QemuServer.pm
>> @@ -44,7 +44,7 @@ use PVE::QemuConfig;
>> use PVE::QemuServer::Helpers qw(min_version config_aware_timeout);
>> use PVE::QemuServer::Cloudinit;
>> use PVE::QemuServer::CPUConfig qw(print_cpu_device get_cpu_options);
>> -use PVE::QemuServer::Drive qw(is_valid_drivename drive_is_cloudinit drive_is_cdrom parse_drive print_drive foreach_drive foreach_volid);
>> +use PVE::QemuServer::Drive qw(is_valid_drivename drive_is_cloudinit drive_is_cdrom parse_drive print_drive);
>> use PVE::QemuServer::Machine;
>> use PVE::QemuServer::Memory;
>> use PVE::QemuServer::Monitor qw(mon_cmd);
>> @@ -2037,7 +2037,7 @@ sub destroy_vm {
>>
>> if ($conf->{template}) {
>> # check if any base image is still used by a linked clone
>> - foreach_drive($conf, sub {
>> + PVE::QemuConfig->foreach_volume($conf, sub {
>> my ($ds, $drive) = @_;
>> return if drive_is_cdrom($drive);
>>
>> @@ -2051,7 +2051,7 @@ sub destroy_vm {
>> }
>>
>> # only remove disks owned by this VM
>> - foreach_drive($conf, sub {
>> + PVE::QemuConfig->foreach_volume($conf, sub {
>> my ($ds, $drive) = @_;
>> return if drive_is_cdrom($drive, 1);
>>
>> @@ -2340,7 +2340,7 @@ sub check_local_resources {
>> sub check_storage_availability {
>> my ($storecfg, $conf, $node) = @_;
>>
>> - foreach_drive($conf, sub {
>> + PVE::QemuConfig->foreach_volume($conf, sub {
>> my ($ds, $drive) = @_;
>>
>> my $volid = $drive->{file};
>> @@ -2363,7 +2363,7 @@ sub shared_nodes {
>> my $nodehash = { map { $_ => 1 } @$nodelist };
>> my $nodename = nodename();
>>
>> - foreach_drive($conf, sub {
>> + PVE::QemuConfig->foreach_volume($conf, sub {
>> my ($ds, $drive) = @_;
>>
>> my $volid = $drive->{file};
>> @@ -2395,7 +2395,7 @@ sub check_local_storage_availability {
>> my $nodelist = PVE::Cluster::get_nodelist();
>> my $nodehash = { map { $_ => {} } @$nodelist };
>>
>> - foreach_drive($conf, sub {
>> + PVE::QemuConfig->foreach_volume($conf, sub {
>> my ($ds, $drive) = @_;
>>
>> my $volid = $drive->{file};
>> @@ -3463,7 +3463,7 @@ sub config_to_command {
>> push @$devices, '-iscsi', "initiator-name=$initiator";
>> }
>>
>> - foreach_drive($conf, sub {
>> + PVE::QemuConfig->foreach_volume($conf, sub {
>> my ($ds, $drive) = @_;
>>
>> if (PVE::Storage::parse_volume_id($drive->{file}, 1)) {
>> @@ -4252,7 +4252,7 @@ sub qemu_volume_snapshot_delete {
>>
>> $running = undef;
>> my $conf = PVE::QemuConfig->load_config($vmid);
>> - foreach_drive($conf, sub {
>> + PVE::QemuConfig->foreach_volume($conf, sub {
>> my ($ds, $drive) = @_;
>> $running = 1 if $drive->{file} eq $volid;
>> });
>> @@ -4290,6 +4290,52 @@ sub set_migration_caps {
>> mon_cmd($vmid, "migrate-set-capabilities", capabilities => $cap_ref);
>> }
>>
>> +sub foreach_volid {
>> + my ($conf, $func, @param) = @_;
>> +
>> + my $volhash = {};
>> +
>> + my $test_volid = sub {
>> + my ($volid, $is_cdrom, $replicate, $shared, $snapname, $size) = @_;
>> +
>> + return if !$volid;
>> +
>> + $volhash->{$volid}->{cdrom} //= 1;
>> + $volhash->{$volid}->{cdrom} = 0 if !$is_cdrom;
>> +
>> + $volhash->{$volid}->{replicate} //= 0;
>> + $volhash->{$volid}->{replicate} = 1 if $replicate;
>> +
>> + $volhash->{$volid}->{shared} //= 0;
>> + $volhash->{$volid}->{shared} = 1 if $shared;
>> +
>> + $volhash->{$volid}->{referenced_in_config} //= 0;
>> + $volhash->{$volid}->{referenced_in_config} = 1 if !defined($snapname);
>> +
>> + $volhash->{$volid}->{referenced_in_snapshot}->{$snapname} = 1
>> + if defined($snapname);
>> + $volhash->{$volid}->{size} = $size if $size;
>> + };
>> +
>> + PVE::QemuConfig->foreach_volume($conf, sub {
>> + my ($ds, $drive) = @_;
>> + $test_volid->($drive->{file}, drive_is_cdrom($drive), $drive->{replicate} // 1, $drive->{shared}, undef, $drive->{size});
>> + });
>> +
>> + foreach my $snapname (keys %{$conf->{snapshots}}) {
>> + my $snap = $conf->{snapshots}->{$snapname};
>> + $test_volid->($snap->{vmstate}, 0, 1, $snapname);
>
> if vmstate would be moved to QemuServer/Drive.pm 's schema, we could
> pass it here as extra key to foreach_volume.
The line of code I wrote is of course very wrong (the parameters don't
even match) and only works by chance.
Using extra_keys with vmstate is already possible, because
foreach_volume uses parse_volume and there vmstate gets parsed
correctly. I didn't want to use parse_drive and add it to the schema,
because state files cannot/shouldn't be attached to a VM (so it doesn't
feel like a real drive) and 'vmstate' is not a drive name (so special
handling would be needed).
It turns out that the code in QemuMigrate.pm needs to be adapted to
ignore vmstate files when checking whether the snapshots are migratable,
because when test_volid is called with the correct parameters, the
vmstate file is referenced by a snapshot and a raw file.
My suggestion for fixing this is:
1. use vmstate as an extra key in the iteration
2. include the key as a parameter in the test_volid closure, to be able
to check if the volume is a state file
3. adapt the check in QemuMigrate.pm
> also, isn't the vmstate
> of a suspended VM missing here? both as potential follow-ups and don't
> warrant another round ;)
>
You're right. This can also be fixed by adding the extra key.
>> + PVE::QemuConfig->foreach_volume($snap, sub {
>> + my ($ds, $drive) = @_;
>> + $test_volid->($drive->{file}, drive_is_cdrom($drive), $drive->{replicate} // 1, $drive->{shared}, $snapname);
>> + });
>> + }
>> +
>> + foreach my $volid (keys %$volhash) {
>> + &$func($volid, $volhash->{$volid}, @param);
>> + }
>> +}
>> +
>> my $fast_plug_option = {
>> 'lock' => 1,
>> 'name' => 1,
>> @@ -4749,7 +4795,7 @@ sub vm_migrate_get_nbd_disks {
>> my ($storecfg, $conf, $replicated_volumes) = @_;
>>
>> my $local_volumes = {};
>> - foreach_drive($conf, sub {
>> + PVE::QemuConfig->foreach_volume($conf, sub {
>> my ($ds, $drive) = @_;
>>
>> return if drive_is_cdrom($drive);
>> @@ -5608,7 +5654,7 @@ sub restore_file_archive {
>> my $restore_cleanup_oldconf = sub {
>> my ($storecfg, $vmid, $oldconf, $virtdev_hash) = @_;
>>
>> - foreach_drive($oldconf, sub {
>> + PVE::QemuConfig->foreach_volume($oldconf, sub {
>> my ($ds, $drive) = @_;
>>
>> return if drive_is_cdrom($drive, 1);
>> @@ -6478,7 +6524,7 @@ sub foreach_storage_used_by_vm {
>>
>> my $sidhash = {};
>>
>> - foreach_drive($conf, sub {
>> + PVE::QemuConfig->foreach_volume($conf, sub {
>> my ($ds, $drive) = @_;
>> return if drive_is_cdrom($drive);
>>
>> @@ -6529,7 +6575,7 @@ sub template_create {
>>
>> my $storecfg = PVE::Storage::config();
>>
>> - foreach_drive($conf, sub {
>> + PVE::QemuConfig->foreach_volume($conf, sub {
>> my ($ds, $drive) = @_;
>>
>> return if drive_is_cdrom($drive);
>> diff --git a/PVE/QemuServer/Cloudinit.pm b/PVE/QemuServer/Cloudinit.pm
>> index 559f331..b3ae57b 100644
>> --- a/PVE/QemuServer/Cloudinit.pm
>> +++ b/PVE/QemuServer/Cloudinit.pm
>> @@ -466,7 +466,7 @@ sub generate_cloudinitconfig {
>>
>> my $format = get_cloudinit_format($conf);
>>
>> - PVE::QemuServer::foreach_drive($conf, sub {
>> + PVE::QemuConfig->foreach_volume($conf, sub {
>> my ($ds, $drive) = @_;
>>
>> my ($storeid, $volname) = PVE::Storage::parse_volume_id($drive->{file}, 1);
>> diff --git a/PVE/QemuServer/Drive.pm b/PVE/QemuServer/Drive.pm
>> index a08df66..f84333f 100644
>> --- a/PVE/QemuServer/Drive.pm
>> +++ b/PVE/QemuServer/Drive.pm
>> @@ -14,8 +14,6 @@ drive_is_cloudinit
>> drive_is_cdrom
>> parse_drive
>> print_drive
>> -foreach_drive
>> -foreach_volid
>> );
>>
>> our $QEMU_FORMAT_RE = qr/raw|cow|qcow|qcow2|qed|vmdk|cloop/;
>> @@ -503,65 +501,6 @@ sub print_drive {
>> return PVE::JSONSchema::print_property_string($drive, $alldrive_fmt, $skip);
>> }
>>
>> -sub foreach_drive {
>> - my ($conf, $func, @param) = @_;
>> -
>> - foreach my $ds (valid_drive_names()) {
>> - next if !defined($conf->{$ds});
>> -
>> - my $drive = parse_drive($ds, $conf->{$ds});
>> - next if !$drive;
>> -
>> - &$func($ds, $drive, @param);
>> - }
>> -}
>> -
>> -sub foreach_volid {
>> - my ($conf, $func, @param) = @_;
>> -
>> - my $volhash = {};
>> -
>> - my $test_volid = sub {
>> - my ($volid, $is_cdrom, $replicate, $shared, $snapname, $size) = @_;
>> -
>> - return if !$volid;
>> -
>> - $volhash->{$volid}->{cdrom} //= 1;
>> - $volhash->{$volid}->{cdrom} = 0 if !$is_cdrom;
>> -
>> - $volhash->{$volid}->{replicate} //= 0;
>> - $volhash->{$volid}->{replicate} = 1 if $replicate;
>> -
>> - $volhash->{$volid}->{shared} //= 0;
>> - $volhash->{$volid}->{shared} = 1 if $shared;
>> -
>> - $volhash->{$volid}->{referenced_in_config} //= 0;
>> - $volhash->{$volid}->{referenced_in_config} = 1 if !defined($snapname);
>> -
>> - $volhash->{$volid}->{referenced_in_snapshot}->{$snapname} = 1
>> - if defined($snapname);
>> - $volhash->{$volid}->{size} = $size if $size;
>> - };
>> -
>> - foreach_drive($conf, sub {
>> - my ($ds, $drive) = @_;
>> - $test_volid->($drive->{file}, drive_is_cdrom($drive), $drive->{replicate} // 1, $drive->{shared}, undef, $drive->{size});
>> - });
>> -
>> - foreach my $snapname (keys %{$conf->{snapshots}}) {
>> - my $snap = $conf->{snapshots}->{$snapname};
>> - $test_volid->($snap->{vmstate}, 0, 1, $snapname);
>> - foreach_drive($snap, sub {
>> - my ($ds, $drive) = @_;
>> - $test_volid->($drive->{file}, drive_is_cdrom($drive), $drive->{replicate} // 1, $drive->{shared}, $snapname);
>> - });
>> - }
>> -
>> - foreach my $volid (keys %$volhash) {
>> - &$func($volid, $volhash->{$volid}, @param);
>> - }
>> -}
>> -
>> sub bootdisk_size {
>> my ($storecfg, $conf) = @_;
>>
>> diff --git a/PVE/VZDump/QemuServer.pm b/PVE/VZDump/QemuServer.pm
>> index 08c29ba..38dcdb1 100644
>> --- a/PVE/VZDump/QemuServer.pm
>> +++ b/PVE/VZDump/QemuServer.pm
>> @@ -19,6 +19,7 @@ use PVE::Storage;
>> use PVE::Tools;
>> use PVE::VZDump;
>>
>> +use PVE::QemuConfig;
>> use PVE::QemuServer;
>> use PVE::QemuServer::Machine;
>> use PVE::QemuServer::Monitor qw(mon_cmd);
>> @@ -68,7 +69,7 @@ sub prepare {
>>
>> my $vollist = [];
>> my $drivehash = {};
>> - PVE::QemuServer::foreach_drive($conf, sub {
>> + PVE::QemuConfig->foreach_volume($conf, sub {
>> my ($ds, $drive) = @_;
>>
>> return if PVE::QemuServer::drive_is_cdrom($drive);
>> --
>> 2.20.1
>>
>>
>> _______________________________________________
>> pve-devel mailing list
>> pve-devel at pve.proxmox.com
>> https://pve.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
>>
>>
>
> _______________________________________________
> pve-devel mailing list
> pve-devel at pve.proxmox.com
> https://pve.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
>
More information about the pve-devel
mailing list