Re: [pve-devel] [PATCH v2 3/12] fix #4225: qemuserver: add function to eject isofiles

[Daniel Herzig <d.herzig@proxmox.com>]

Daniel Kral <d.kral@proxmox.com> writes:

> On 1/13/25 09:55, Daniel Herzig wrote:
>> Current behaviour prevents a VM from starting, if an ISO file defined
>> in the configuration becomes unavailable.
>> The function eject_nonrequired_isos checks on whether a cdrom drive
>> is
>> marked as 'required' or not. If the parameter 'required' is not
>> defined, it will assume 'required' to be true and keep the current
>> behaviour.
>> If 'required' is set to 0, the function 'ejects' the ISO file by
>> setting the drive's file value to 'none', if the underlying storage is
>> unavailable or if the defined file is unavailable for another reason.
>> The function is called while config_to_command iterates over all
>> volumes to allow for early storage activation and early exit in the
>> case of missing required files.
>> Signed-off-by: Daniel Herzig <d.herzig@proxmox.com>
>> ---
>>   PVE/QemuServer.pm | 39 +++++++++++++++++++++++++++++++++++++++
>>   1 file changed, 39 insertions(+)
>> diff --git a/PVE/QemuServer.pm b/PVE/QemuServer.pm
>> index d07c170e..f72878d3 100644
>> --- a/PVE/QemuServer.pm
>> +++ b/PVE/QemuServer.pm
>> @@ -4041,6 +4041,8 @@ sub config_to_command {
>>       PVE::QemuConfig->foreach_volume($conf, sub {
>>   	my ($ds, $drive) = @_;
>>   +	eject_nonrequired_isos($ds, $drive, $vmid, $storecfg, $conf);
>> +
>
> This change will unfortunately make two config2cmd test cases fail and
> therefore the build process will also fail. It is important that the
> package can be built at each individual commit so to make the package
> bisectable.
>
> IMO this patch could be split into "introduce eject_norequired_isos"
> and patches #4-#9 could be squashed and put together with adding
> "eject_nonrequired_isos" to config_to_command in the same
> patch. Therefore someone reviewing (now or in the future) and know
> what tests needed to be added/changed when adding this function call
> to config_to_command.
>

Thanks for pointing that out. Batch-applying the patches on my
test-system I completely missed this.

I'll work out something along your lines for v3.


>>   	if (PVE::Storage::parse_volume_id($drive->{file}, 1)) {
>>   	    check_volume_storage_type($storecfg, $drive->{file});
>>   	    push @$vollist, $drive->{file};
>> @@ -8999,6 +9001,43 @@ sub delete_ifaces_ipams_ips {
>>       }
>>   }
>>   +sub eject_nonrequired_isos {
>> +    my ($ds, $drive, $vmid, $storecfg, $conf) = @_;
>> +    # set 1 to exclude cloudinit. cloudinit isos are always required.
>> +    if (drive_is_cdrom($drive, 1)
>> +	&& $drive->{file} ne 'none'
>> +	&& $drive->{file} ne 'cdrom') {
>
> nit: IMO, this could be an early return:
>
> return if !drive_is_cdrom($drive, 1);
> return if $drive->{file} eq 'none' || $drive->{file} eq 'cdrom';
>
> So that we can reduce the following to only 2 indentation levels.
>

I like this idea.

>> +	$drive->{required} = 1 if !defined($drive->{required});
>> +	my $iso_volid = $drive->{file};
>> +	my $iso_path = get_iso_path($storecfg, $vmid, $drive->{file});
>
> nit: third argument could be $iso_volid
>

Scratching my head how I missed this :).

>> +	my $store_err;
>> +	if ($iso_volid !~ m|^/|) {
>> +	    my $iso_storage  = PVE::Storage::parse_volume_id($iso_volid, 1);
>> +	    eval { PVE::Storage::activate_storage($storecfg, $iso_storage); };
>> +	    $store_err = $@;
>> +	}
>> +	if ($store_err) {
>> +	    if ($drive->{required}) {
>> +		die "cannot access required file: '${ds}: ${iso_volid}': ${store_err}\n";
>> +	    } else {
>> +		log_warn("eject '${ds}: ${iso_volid}': ${store_err}");
>> +		$drive->{file} = 'none';
>> +		$conf->{$ds} = print_drive($drive);
>> +	    }
>> +	} else {
>> +	    if (!file_exists($iso_path)) {
>> +		if ($drive->{required}) {
>> +		    die "required file does not exist: '${ds}: ${iso_volid}'\n";
>> +		} else {
>> +		    log_warn("eject '${ds}: ${iso_volid}': file does not exist");
>> +		    $drive->{file} = 'none';
>> +		    $conf->{$ds} = print_drive($drive);
>> +		}
>> +	    }
>> +	}
>
> nit: the logic between an unavailable storage and an unavailable ISO
> image are very similar (both `$drive->{required} && $store_err` as
> well as `$drive->{required} && !file_exists($iso_path)` have the same
> exit control path), so we could simplify this e.g. to this (changes
> the warning message to a generic message for unavailable storages
> too):
>
> if ($drive->{required}) {
>     die "cannot access required file: '${ds}: ${iso_volid}':
>     ${store_err}\n" if $store_err;
>     die "required file does not exist: '${ds}: ${iso_volid}'\n" if
>     !file_exists($iso_path);
> }
>
> return if !$store_err && file_exists($iso_path);
>
> log_warn("eject '${ds}: ${iso_volid}': storage unavailable or file
> does not exist");
>
> $drive->{file} = 'none';
> $conf->{$ds} = print_drive($drive);
>

Good point, thanks.



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