Daniel Kral <d.k...@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.her...@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
