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.
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.
+ $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
+ 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);
+ }
+}
+
sub file_exists {
my $file_path = shift;
return -e $file_path
Else consider this:
Reviewed-by: Daniel Kral <d.k...@proxmox.com>
Tested-by: Daniel Kral <d.k...@proxmox.com>
_______________________________________________
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
